Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dissolve Relational.Design into just Relational and Design #7004

Closed
bricelam opened this issue Nov 11, 2016 · 5 comments
Closed

Dissolve Relational.Design into just Relational and Design #7004

bricelam opened this issue Nov 11, 2016 · 5 comments
Assignees
Labels
breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. providers-beware type-cleanup
Milestone

Comments

@bricelam
Copy link
Contributor

bricelam commented Nov 11, 2016

We've briefly talked about this before, so I'm creating an issue to make any decisions.

We should either:

  • Split database creation (DbContext.Database.EnsureCreated() and Migrations) out of Microsoft.EntityFrameworkCore.Relational.

or

  • Merge Reverse Engineering (Microsoft.EntityFrameworkCore.Relational.Design) into Relational.

This would cascade into each provider. They would either need to have one more assembly (for migrations) or combine their Design package into their main one.

@bricelam bricelam changed the title Comine (or split) Relational Comine or split Relational Nov 11, 2016
@maumar maumar changed the title Comine or split Relational Combine or split Relational Nov 12, 2016
@rowanmiller rowanmiller added this to the 1.2.0 milestone Nov 14, 2016
@ajcvickers ajcvickers modified the milestones: 2.0.0-preview1, 2.0.0 Apr 19, 2017
@bricelam
Copy link
Contributor Author

bricelam commented Apr 28, 2017

I briefly looked into this, and I think if we merge them we need to rework some of the architecture to favor composition over inheritance. Right now, providers inherit a base implementation for generating C#, but a better factoring might come if we just ask provider's for the fragments of C# they need to provide. This would push all of our C#-generating code into EFCore.Design and the contracts for providers could move directly into EFCore.Relational.

@bricelam bricelam changed the title Combine or split Relational Dissolve Relational.Design into just Relational and Design Apr 28, 2017
@bricelam bricelam modified the milestones: 2.0.0, 2.0.0-preview2 May 16, 2017
@ajcvickers ajcvickers modified the milestones: 2.0.0, 2.0.0-preview2 May 16, 2017
@ajcvickers
Copy link
Member

@smitpatel This is something we would really like to get into preview2, but @bricelam is somewhat overbooked. Could you work with @bricelam to understand what is needed and try to get this either this week or early next week?

@ajcvickers
Copy link
Member

@smitpatel How close are you to getting this merged? Can we do it today, even if there are still some open issues that need to be fixed afterwards? This change impacts the packages we produce, which impacts the build, etc., so we really need to get this part in ASAP and then tidy up any lose ends after it is in.

@smitpatel
Copy link
Member

@roji @ErikEJ & other provider writers

Resolving this issue involved doing a lot of refactoring/changes on how reverse engineering pipeline worked. Following is summary of changes which went in.

Overall design
Each provider knows how to read the database. Provider will give us the DatabaseModel which contains all the information we need from database. (in the form of properties or annotations).
EF will convert the DatabaseModel to IModel. It will also copy over annotations from DatabaseModel so provider specific information is flowed through.
Based on IModel, EF will write code. In this process, any annotation which EF does not know how to process, it will ask Provider on how to render the annotation. This way provider will have chance to scaffold provider specific fluent API. (this part would be reused by snapshot generation in migrations in future).

What provider has to implement now?

  • Extension method Allows on TableSelectionSet (@bricelam do we want to improve this? Looks very weird)
  • An implementation of IDatabaseModelFactory which would return a DatabaseModel. Providers may have provider specific annotations. To annotation objects directly or use metadata extensions for that is upto provider writer.
  • An implementation of IScaffoldingHelper. This class is helper class for providers to influence scaffolding which is very specific to provider.
  • An implementation of IAnnotationRenderer. (There is base class AnnotationRendererBase in relational available which will be used by default. This class allows provider to render any provider specific annotations.

What went away?

  • IScaffoldingModelingFactory Provider will not be able to change the model building part by overriding, instead it would be done via annotations. Since any thing which is provider specific is annotations only in metadata.
  • ColumnModel removed some properties. It will contain only properties which are required for metadata in some form. Anything which is provider specific must be annotated instead.
  • All Configuration classes (ModelConfiguration/EntityTypeConfiguration etc.) are removed. We used to take IModel and create all this class object to decide fluent API/annotations to generate and those objects were passed to file writers. Instead of having 2 layers, now the filer writers will determine based on IModel & useDataAnnotations parameter, what it should be generating.
  • Since there is not much code for provider writers to implement, Provider.Design package is merged into Provider package.
  • Due to clear separation of what is expected from provider & what EF design time has to generate, all contracts and required metadata for provider was moved to Relational package & all design time code was moved to EFCore.Design. Thus, Relational.Design package was removed.
  • There are various class name changes to reflect what it is doing exactly. A lot of code which was public virtual before was made private. There are enough hooks for User to change the way how file is generated but we have removed too many knobs till we make the design sound & stable. This part should not affect provider writers though.

SQLite provider has minimalistic changes to reverse engineering.
SqlServer can provider good examples for provider specific tasks.

Due to #8594 At present testing infra has remained as is (except Design.Tests package got merged into .Tests). Once that issue is fixed, it will define good skeleton for Design.Specification.Tests.

Let us know if any feedback/questions.

@smitpatel smitpatel added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jun 2, 2017
@bricelam
Copy link
Contributor Author

bricelam commented Jun 2, 2017

I agree TableSelectionSet.Allows() is a little awkward and can probably be improved.
I also think the DatabaseModel could be restructured a bit to more closely match the Migrations operations.
We can probably also tweak [assembly: DesignTimeServices] since most providers will only have one assembly & NuGet package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. providers-beware type-cleanup
Projects
None yet
Development

No branches or pull requests

4 participants