-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Comments
Is this a good time to start investigating using Reverse Engineering with SQLCE, or is it too early? |
@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. |
@divega Sounds great, is there some docs/design specs? Or a drawing? 😄 |
No docs yet. I will defer to @lajones on answering any questions and or drawing it for you 😄 |
Fixed with commit 5c30958 |
@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. |
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) |
A few comments: 1: I see the Templates are now shared, and they have UseSqlServer hardcoded....
2: The only thing I needed to override in DbContextCodeGeneratorHelper was ClassName! 3: Wonder why I am getting this strange code:
CREATE SQL for the one above:
|
@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. |
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 |
@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. |
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.
The text was updated successfully, but these errors were encountered: