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: Strive for code generation templates that are not provider specific #2465

Closed
divega opened this issue Jun 24, 2015 · 13 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 Jun 24, 2015

Currently we may generate DbContext code that uses ForSqlServer() in the model generation or at least we may have logic in the template that is SQL Server specific. This issue is about avoiding that if possible and using relational APIs instead.

There is value in having templates that can be customized once and then used against different providers. There is also value in not requiring providers to develop their own templates in order to fully support the reverse engineering experiences we want to enable, e.g. what is described in #2415 (although this is less important if they can just ship copies of our template in their reveng packages).

On the other hand we are not sure yet that our templates won't require provider specific logic and we can't assume that other providers won't need to extend code generation to accommodate their own concepts.

This came up with in a conversation with @ajcvickers and I just wanted to make sure we capture it. He and @lajones can provide mode details about what we have now, what is possible and the pros and cons of this.

@ErikEJ
Copy link
Contributor

ErikEJ commented Jun 28, 2015

Is this a good time to start investigating using Reverse Engineering with SQLCE, or is it too early?

@divega
Copy link
Contributor Author

divega commented Jun 29, 2015

@ErikEJ I personally believe it is a good time for you to get started with it. If you hit issues or have any feedback we can make adjustments.

@ErikEJ
Copy link
Contributor

ErikEJ commented Jun 29, 2015

@divega Sounds great, is there some docs/design specs? Or a drawing? 😄
I have a few questions:
1: Does this info still apply: https://stackoverflow.com/questions/29300777/is-there-an-entity-framework-7-database-first-poco-generator?
2: If so, how does the command know which provider to use?
3: Is the rev-eng package simply installed as needed? (EntityFramework.SqlServer.Design)
4: Will I "simply" implement the equivalent of EntityFramework.SqlServer.Design ?

@divega
Copy link
Contributor Author

divega commented Jun 29, 2015

No docs yet. I will defer to @lajones on answering any questions and or drawing it for you 😄

@lajones lajones changed the title RevEng: Strive for code generation templates are are not provider specific RevEng: Strive for code generation templates that are not provider specific Jul 1, 2015
@lajones
Copy link
Contributor

lajones commented Jul 1, 2015

@ErikEJ This feels like a more general question about how RevEng currently works - moving to the general issue tracking RevEng: #830. But note: this issue will have an effect on how you implement that.

@lajones
Copy link
Contributor

lajones commented Aug 7, 2015

Fixed with commit 5c30958

@lajones lajones closed this as completed Aug 7, 2015
@lajones
Copy link
Contributor

lajones commented Aug 7, 2015

@ErikEJ Hi Erik. The changes are now in. I'll write it up but broadly you should only have to implement the ConstructRelationalModel() method in your provider and override the ExtensionsProvider getter now.

See https://github.com/aspnet/EntityFramework/blob/dev/src/EntityFramework.SqlServer.Design/ReverseEngineering/SqlServerMetadataModelProvider.cs for an example.

@ErikEJ
Copy link
Contributor

ErikEJ commented Aug 7, 2015

Thanks. Still blocked on #2747 and also need to implement MetaData (i could avoid that in beta 6, but not any longer, but glad I postponed)

@ErikEJ
Copy link
Contributor

ErikEJ commented Aug 10, 2015

A few comments:

1: I see the Templates are now shared, and they have UseSqlServer hardcoded....

 options.UseSqlServer(@Model.Helper.VerbatimStringLiteral(Model.ConnectionString));

2: The only thing I needed to override in DbContextCodeGeneratorHelper was ClassName!

3: Wonder why I am getting this strange code:

        modelBuilder.Entity<PropertyConfiguration>(entity =>
        {
            entity.Property(e => e.PropertyConfigurationID)
                .ValueGeneratedOnAdd()
                .ValueGeneratedNever();

CREATE SQL for the one above:

 CREATE TABLE [PropertyConfiguration] (
   [PropertyConfigurationID] int IDENTITY (2,1) NOT NULL

@lajones
Copy link
Contributor

lajones commented Aug 10, 2015

@ErikEJ - Re 1 - my bad - I missed that. I'm re-opening to address that. Re 3 in SqlServerDbContextGeneratorHelper I override AddPropertyFacetsConfiguration to add the ValueGeneratedNever() API because the KeyConvention will automatically assume ValueGeneratedOnAdd() if you have a single, integer primary key property - see the comments lines 56-58. I'm not sure if that applies for you. If you've just copied that code you may have to update the assumptions there.

@lajones lajones reopened this Aug 10, 2015
@ErikEJ
Copy link
Contributor

ErikEJ commented Aug 10, 2015

Thanks. Re 1: no worries, it is a Work in Progress!

Re 3: I have the same test schema as you do, and that table does have a single int primary identity key. Trying to figure out if I have a bug somewhere. And yes, I just copied the code, assuming it would apply fine to sqlce

@ErikEJ
Copy link
Contributor

ErikEJ commented Aug 10, 2015

@lajones Re 3: had a closer look at the SQL Server schema. I think you will encounter the same issue with the SQL Server provider, actually. It looks like you are missing test coverage of a table with a valid IDENTITY column!

My tools changed the column type from byte to int due to it knowing that only int and bigint are valid datatypes for IDENTITY columns with SQLCE.

@lajones
Copy link
Contributor

lajones commented Aug 11, 2015

Closing. Fixed with PRs #2812 and #2816. @ErikEJ - you'll need to implement the UseMethodName getter on your SqlCeDbContextCodeGeneratorHelper but this should fix problems 1 and 3 above. Please re-open if not.

@lajones lajones closed this as completed Aug 11, 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

5 participants