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

RevEng: Refactor to split schema acquisition from more common functionality #2713

Closed
divega opened this issue Jul 24, 2015 · 8 comments
Closed
Assignees
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-unknown
Milestone

Comments

@divega
Copy link
Contributor

divega commented Jul 24, 2015

Currently the reverse engineering provider for SQL Server contains the logic that retrieves the schema information from the database into an IModel and also the logic that translates that into an enriched IModel containing entities, relationships and annotations that is ready for the code-gen phase.

Ideally, providers should only need to implement the initial schema acquisition and could delegate the rest to common functionality in EF.

One proposal to address this is to lift the common functionality into base relational provider classes. This would allow the providers to still override the behaviors that they wish to. Creating a reverse engineering provider for a database in which schema acquisition is fundamentally different from SQL Server should help polish such design.

@divega divega changed the title RevEng: Refactor to split schema aqusition from common functionality RevEng: Refactor to split schema aquisition from common functionality Jul 24, 2015
@divega divega changed the title RevEng: Refactor to split schema aquisition from common functionality RevEng: Refactor to split schema acquisition from common functionality Jul 24, 2015
@divega
Copy link
Contributor Author

divega commented Jul 25, 2015

@bricelam re the last point, do we have a bug for reverse engineering for SQLite that we can talk about in triage? The one that was closed today was specifically for UWP projects, and I couldn't find one for SQLite.

@divega divega changed the title RevEng: Refactor to split schema acquisition from common functionality RevEng: Refactor to split schema acquisition from more common functionality Jul 25, 2015
@bricelam
Copy link
Contributor

Created #2721

@lajones lajones self-assigned this Jul 28, 2015
@lajones
Copy link
Contributor

lajones commented Jul 28, 2015

@ErikEJ Please note this might change your SQLCE provider too (make it simpler, I hope).

I'm planning on having an example checked in soon - but basics are there's a new abstract base class, ReverseEngineeringMetadataModelProvider, which does all the conversion from the relational IModel to the enhanced IModel used for code-gen. You should simply need to override the method ConstructRelationalModel() and produce an IModel that directly represents the DB i.e. has entities (= tables), properties (= columns) and foreign keys. After that ReverseEngineeringMetadataModelProvider takes over. However there will be requirements on the way the relational IModel is set up (what annotations we expect etc). I'll list them when I have the complete list.

@ErikEJ
Copy link
Contributor

ErikEJ commented Jul 29, 2015

Sounds great, will save me 600 lines of code, I did not make any significant changes anyway: https://github.com/ErikEJ/EntityFramework7.SqlServerCompact/blob/master/src/Design40/ReverseEngineering/SqlCeMetaDataModelProvider.cs

@ErikEJ
Copy link
Contributor

ErikEJ commented Jul 29, 2015

@lajones
Copy link
Contributor

lajones commented Jul 29, 2015

@ErikEJ re that line - we define IsStoreGenerated in the query at https://github.com/aspnet/EntityFramework/blob/dev/src/EntityFramework.SqlServer.Design/ReverseEngineering/Model/TableColumn.cs#L26. Given the way that query is defined at the moment the line at https://github.com/aspnet/EntityFramework/blob/dev/src/EntityFramework.SqlServer.Design/ReverseEngineering/SqlServerMetadataModelProvider.cs#L616 seems unnecessary. But I think I'd rather remove it from the query and keep it (and especially the comment below) here. Either way will work.

@lajones
Copy link
Contributor

lajones commented Jul 30, 2015

@ErikEJ - see #2743 for the fix for that specific line.

@lajones lajones modified the milestones: 7.0.0-beta7, 7.0.0 Aug 7, 2015
@lajones
Copy link
Contributor

lajones commented Aug 7, 2015

Fixed with commit 5c30958

@lajones lajones closed this as completed Aug 7, 2015
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Oct 15, 2022
@ajcvickers ajcvickers modified the milestones: 1.0.0-beta7, 1.0.0 Oct 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-unknown
Projects
None yet
Development

No branches or pull requests

6 participants