-
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
scaffolded one-to-many property names generate with incorrect name #32981
Comments
Note for triage: duplicate of dotnet/EntityFramework.Docs#4608 which we decided to doc. |
Can you give some guidance on the type of change that can be made to the EntityType.t4 to go back to the old naming convention? If you look at the attached screenshot, I make it write the nagivation.Name as a comment. In many of the cases, it still generates the navigation with the preferred name but there are a couple of instances where it does not. In this example PlatformMappingType is the desired navigation.Name, but for the other two I expect navigation.Name to be DestinationPlatform and SourcePlatform. |
@bclementfidx I have some sample code here to get you started https://github.com/ErikEJ/EFCorePowerTools/tree/master/samples/CodeTemplates/EFCore |
My organization is running into the same issue as described in this ticket. It's particularly well described with the example where instead of:
You get:
In the pre-net8 example, it's clear that I've followed the issues back to the original issue that caused this change (#27832) and I'm struggling to see how this solution is an improvement. @ajcvickers mentions here that "the new behavior is preferable to the old behavior", but considering how opaque / impossible it is to tell what navigation properties mean now, this just appears to be a bug. @ErikEJ Thanks for the t4 example, but perhaps I'm having a slow moment - after fiddling around with it myself, I'm unable to figure out or get a start with restoring the old names. Using the example laid out in the ticket, it seems that the navigation properties have "lost all knowledge" that they came from |
@hallidev Please share a sample create script that demonstrates the issue, and I might have a look. |
@ErikEJ using the EntityType.t4 from your earlier provided code template with the schema defined in this ticket demonstrates the issue |
Would agree with @bclementfidx - it's a good SQL script for a minimal reproduction with just 2 tables. If you scaffold that with the net7 ef tools vs net8, you'll be able to see the difference and how the net8 result is very difficult to unpack from the developer's perspective. |
@ErikEJ I also believe there is a bug in the EntityType.t4 var renameTuple = renames.SingleOrDefault(t => t.EntityTypeName == targetType && t.PropertyName == navigation.Name); is supposed to be: var renameTuple = renames.SingleOrDefault(t => t.EntityTypeName == EntityType.Name && t.PropertyName == navigation.Name); |
I'm also affected by this issue. Will keep watching. |
Waiting on a resolution for this. |
Make sure to upvote the initial post here if you think this is important |
@bclementfidx Several tables are missing from your repro SQL script above... I cannot investigate without a complete repro |
CREATE TABLE [Platform](
[Id] [uniqueidentifier] NOT NULL,
[Code] [varchar](256) NOT NULL,
[Description] [nvarchar](256) NOT NULL,
[CreatedUtc] [datetimeoffset](7) NOT NULL,
[CreatedUser] [varchar](128) NOT NULL,
[UpdatedUtc] [datetimeoffset](7) NULL,
[UpdatedUser] [varchar](128) NULL,
CONSTRAINT [PK_Platform] PRIMARY KEY CLUSTERED
(
[Id] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON, OPTIMIZE_FOR_SEQUENTIAL_KEY = OFF) ON [PRIMARY],
CONSTRAINT [UC_Platform_Code] UNIQUE NONCLUSTERED
(
[Code] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON, OPTIMIZE_FOR_SEQUENTIAL_KEY = OFF) ON [PRIMARY]
) ON [PRIMARY]
GO
ALTER TABLE [Platform] ADD CONSTRAINT [DF_Platform_Id] DEFAULT (newid()) FOR [Id]
GO
CREATE TABLE [PlatformType](
[Id] [smallint] NOT NULL,
[Code] [varchar](256) NOT NULL,
[Description] [nvarchar](256) NOT NULL,
CONSTRAINT [PK_PlatformType] PRIMARY KEY CLUSTERED
(
[Id] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON, OPTIMIZE_FOR_SEQUENTIAL_KEY = OFF) ON [PRIMARY],
CONSTRAINT [UC_PlatformType_Code] UNIQUE NONCLUSTERED
(
[Code] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON, OPTIMIZE_FOR_SEQUENTIAL_KEY = OFF) ON [PRIMARY]
) ON [PRIMARY]
GO
CREATE TABLE [Platform_PlatformType](
[PlatformId] [uniqueidentifier] NOT NULL,
[PlatformTypeId] [smallint] NOT NULL,
CONSTRAINT [PK_Platform_PlatformType] PRIMARY KEY CLUSTERED
(
[PlatformId] ASC,
[PlatformTypeId] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON, OPTIMIZE_FOR_SEQUENTIAL_KEY = OFF) ON [PRIMARY]
) ON [PRIMARY]
GO
ALTER TABLE [Platform_PlatformType] WITH CHECK ADD CONSTRAINT [FK_Platform_PlatformType_Platform] FOREIGN KEY([PlatformId])
REFERENCES [Platform] ([Id])
GO
ALTER TABLE [Platform_PlatformType] CHECK CONSTRAINT [FK_Platform_PlatformType_Platform]
GO
ALTER TABLE [Platform_PlatformType] WITH CHECK ADD CONSTRAINT [FK_Platform_PlatformType_PlatformType] FOREIGN KEY([PlatformTypeId])
REFERENCES [PlatformType] ([Id])
GO
ALTER TABLE [Platform_PlatformType] CHECK CONSTRAINT [FK_Platform_PlatformType_PlatformType]
GO
CREATE TABLE [PlatformMappingType](
[Id] [smallint] NOT NULL,
[Code] [varchar](256) NOT NULL,
[Description] [nvarchar](256) NOT NULL,
CONSTRAINT [PK_PlatformMappingType] PRIMARY KEY CLUSTERED
(
[Id] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON, OPTIMIZE_FOR_SEQUENTIAL_KEY = OFF) ON [PRIMARY],
CONSTRAINT [UC_PlatformMappingType_Code] UNIQUE NONCLUSTERED
(
[Code] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON, OPTIMIZE_FOR_SEQUENTIAL_KEY = OFF) ON [PRIMARY]
) ON [PRIMARY]
GO
CREATE TABLE [PlatformIdentifierMapping](
[Id] [uniqueidentifier] NOT NULL,
[PlatformMappingTypeId] [smallint] NOT NULL,
[SourcePlatformId] [uniqueidentifier] NOT NULL,
[SourcePlatformTypeId] [smallint] NOT NULL,
[DestinationPlatformId] [uniqueidentifier] NOT NULL,
[DestinationPlatformTypeId] [smallint] NOT NULL,
[MappingCode] [varchar](50) NULL,
[SourceValue] [varchar](256) NOT NULL,
[DestinationValue] [varchar](256) NOT NULL,
[CreatedUtc] [datetimeoffset](7) NOT NULL,
[CreatedUser] [varchar](128) NOT NULL,
[UpdatedUtc] [datetimeoffset](7) NULL,
[UpdatedUser] [varchar](128) NULL,
CONSTRAINT [PK_PlatformIdentifierMapping] PRIMARY KEY CLUSTERED
(
[Id] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON, OPTIMIZE_FOR_SEQUENTIAL_KEY = OFF) ON [PRIMARY]
) ON [PRIMARY]
GO
ALTER TABLE [PlatformIdentifierMapping] ADD CONSTRAINT [DF_PlatformIdentifierMapping_Id] DEFAULT (newid()) FOR [Id]
GO
ALTER TABLE [PlatformIdentifierMapping] WITH CHECK ADD CONSTRAINT [FK_PlatformIdentifierMapping_PlatformMappingType] FOREIGN KEY([PlatformMappingTypeId])
REFERENCES [PlatformMappingType] ([Id])
GO
ALTER TABLE [PlatformIdentifierMapping] CHECK CONSTRAINT [FK_PlatformIdentifierMapping_PlatformMappingType]
GO
ALTER TABLE [PlatformIdentifierMapping] WITH CHECK ADD CONSTRAINT [FK_ProposalDetail_Platform_PlatformType_DestinationPlatform] FOREIGN KEY([DestinationPlatformId], [DestinationPlatformTypeId])
REFERENCES [Platform_PlatformType] ([PlatformId], [PlatformTypeId])
GO
ALTER TABLE [PlatformIdentifierMapping] CHECK CONSTRAINT [FK_ProposalDetail_Platform_PlatformType_DestinationPlatform]
GO
ALTER TABLE [PlatformIdentifierMapping] WITH CHECK ADD CONSTRAINT [FK_ProposalDetail_Platform_PlatformType_SourcePlatform] FOREIGN KEY([SourcePlatformId], [SourcePlatformTypeId])
REFERENCES [Platform_PlatformType] ([PlatformId], [PlatformTypeId])
GO
ALTER TABLE [PlatformIdentifierMapping] CHECK CONSTRAINT [FK_ProposalDetail_Platform_PlatformType_SourcePlatform]
GO |
I have updated the sample files to work with your schema (and fixed a bug) |
@ErikEJ So do you see this as desirable behavior for how it is now naming these navigations? I am working with multiple databases where this is an issue. One particular database we have is going to need at least 50 of these renames just to get it to scaffold how it once did before. |
@bclementfidx No, I do not, and it was quite painful to get the replacements to work. |
I've been following this thread closely and to me it seems that something has gone wrong with naming in the net8 scaffolder. High level, this is what I'm seeing happened:
In my organization's case (and that of @bclementfidx's clearly), the names now generated are actually impossible to understand without manually hunting through the DbContext class to see how EF Core wired them up. Furthermore, fixing the issue requires painstakingly hardcoding manual property names inside template files. I'd just like to contrast this with the original (pre-net8) naming scheme which was so good it was almost "magic". It was immediately clear what-was-what and worked in every case my organization ran into as well as the numerous side project's I've used EF Core for. Despite using EF Core for at least 5 years, I never ran into the issue reported in number 1 above. We have six databases of various sized schemas scaffolded at my current organization, and after running the new scaffolder on a branch, there are 114 property names we'll need to hardcode and manually manage in templates. This doesn't include future db schema changes where this naming scheme will need to be painstakingly maintained as new tables are added. I guess I'm just trying to understand why such an effective automatic naming scheme is being abandoned for the edge case discovered in #27832 in favor of a naming scheme that has a tendency to generate confusing names that require manually diving through auto generated code to understand. I understand that @ajcvickers has decided to handle this via documentation, but I truly believe that's the wrong solution. Please - if this is the direction EF Core wants to head with scaffolding, at least make the original naming scheme something we can opt into. The template solution is such a huge downgrade and continuing maintenance issue that I just can't understand why it would be desired. Perhaps as a compromise - maybe the naming step during scaffolding could be pluggable / overridable so that we can provide a custom implementation? The real issue for us is manually maintaining the template files with hardcoded property renames. If we could just "plug in" the old logic, that'd completely fix our issue. |
I will make a breaking change and revert the new behaviour in EF Core Power Tools and demonstrate how you can revert in your own code. |
@ErikEJ That's amazing, can't tell you how much it's appreciated. |
Look who got the ball rolling #27832 😳 I have logged ErikEJ/EFCorePowerTools#2143 |
@ErikEJ @hallidev @bclementfidx I will discuss with the team reverting/tweaking this change in an 8.0.x patch. |
The fix is now in EF Core Power Tools |
We discussed this at length, with the three options on the table being:
Ultimately, we decided on the third option; do nothing. This keeps the rules simple and avoids us creating ridiculous names. This seems like a good default. The customization options (T4) seem like appropriate way to implement more complex naming rules, and in addition the @ErikEJ has provided a community solution with optional behavior built-in. We will document this as a breaking change. |
@ajcvickers Were there more instances of confusing and bad names being generated aside from the one in #27832? The change from that introduced more bad names than anything. I haven't gone through the exercise of trying to scaffold one of our larger databases, but it is rather difficult, confusing, and brittle to do so. I will share with you excerpts from the two t4 files that I have for one of our smaller databases so you can see what it looks like. In the EntityType.T4 I have var renames = new List<(string EntityTypeName, string PropertyName, string NewPropertyName)>
{
("PlatformApplicationStatus", "PlatformPlatformType", "Platform"),
("PlatformIdentifierMapping", "PlatformPlatformType", "DestinationPlatform"),
("PlatformIdentifierMapping", "PlatformPlatformTypeNavigation", "SourcePlatform"),
("PlatformIllustrationMapping", "PlatformPlatformType", "Platform"),
("PlatformPlatformType", "PlatformIdentifierMappingPlatformPlatformTypeNavigations", "PlatformIdentifierMappingDestinationPlatforms"),
("PlatformPlatformType", "PlatformIdentifierMappingPlatformPlatformTypes", "PlatformIdentifierMappingSourcePlatforms"),
("PlatformPlatformType", "ProposalDetailPlatformPlatformTypeNavigations", "ProposalDetailCreatedByPlatforms"),
("PlatformPlatformType", "ProposalDetailPlatformPlatformTypes", "ProposalDetailSourcePlatforms"),
("ProductXmlExport", "PlatformPlatformType", "Platform"),
("Proposal", "PlatformPlatformType", "CreatedByPlatform"),
("ProposalCase", "PlatformPlatformType", "CreatedByPlatform"),
("ProposalDetail", "PlatformPlatformType", "CreatedByPlatform"),
("ProposalDetail", "PlatformPlatformTypeNavigation", "SourcePlatform"),
("ProposalDetailApplication", "PlatformPlatformType", "ProviderPlatform"),
("ProposalDetailDocumentIllustration", "PlatformPlatformType", "Platform"),
("ProposalDetailDocumentPlatform", "PlatformPlatformType", "Platform"),
("ProposalDetailPlatformCommunication", "PlatformPlatformType", "Platform")
}; and in the DbContext.T4 I have var renames = new List<(string EntityTypeName, string DependentToPrinicpalName, string PrincipalToDependentName, string? NewDependentToPrincipalName, string? NewPrincipalToDependentName)>
{
("PlatformApplicationStatus", "PlatformPlatformType", "PlatformApplicationStatuses", "Platform", "PlatformApplicationStatuses"),
("PlatformIdentifierMapping", "PlatformPlatformType", "PlatformIdentifierMappingPlatformPlatformTypes", "DestinationPlatform", "PlatformIdentifierMappingDestinationPlatforms"),
("PlatformIdentifierMapping", "PlatformPlatformTypeNavigation", "PlatformIdentifierMappingPlatformPlatformTypeNavigations", "SourcePlatform", "PlatformIdentifierMappingSourcePlatforms"),
("PlatformIllustrationMapping", "PlatformPlatformType", "PlatformIllustrationMappings", "Platform", "PlatformIllustrationMappings"),
("ProductXmlExport", "PlatformPlatformType", "ProductXmlExports", "Platform", "ProductXmlExports"),
("Proposal", "PlatformPlatformType", "Proposals", "CreatedByPlatform", "Proposals"),
("ProposalCase", "PlatformPlatformType", "ProposalCases", "CreatedByPlatform", "ProposalCases"),
("ProposalDetail", "PlatformPlatformType", "ProposalDetailPlatformPlatformTypes", "CreatedByPlatform", "ProposalDetailCreatedByPlatforms"),
("ProposalDetail", "PlatformPlatformTypeNavigation", "ProposalDetailPlatformPlatformTypeNavigations", "SourcePlatform", "ProposalDetailSourcePlatforms"),
("ProposalDetailApplication", "PlatformPlatformType", "ProposalDetailApplications", "ProviderPlatform", "ProposalDetailApplications"),
("ProposalDetailDocumentIllustration", "PlatformPlatformType", "ProposalDetailDocumentIllustrations", "Platform", "ProposalDetailDocumentIllustrations"),
("ProposalDetailDocumentPlatform", "PlatformPlatformType", "ProposalDetailDocumentPlatforms", "Platform", "ProposalDetailDocumentPlatforms"),
("ProposalDetailPlatformCommunication", "PlatformPlatformType", "ProposalDetailPlatformCommunications", "Platform", "ProposalDetailPlatformCommunications")
}; where as, before #27832 I didnt need to do any of this stuff. I had to take extra consideration in the mapping as well. I would have to figure out that "PlatformPlatformType" was the "CreatedByPlayform" and "PlatformPlatformTypeNavigation" was the "SourcePlatform". I have other databases that are 5x this size that I'm going to essentially build these large mappings. T4s are nice and we have some special customization in there to help with generating Lists and changing setters, etc... but to maintain something like this is ludicrous. Seemingly I have many more examples of bad naming surfacing after this change than what was provided before it. |
dotnet --version
8.0.101
dotnet ef --version
Entity Framework Core .NET Command-line Tools
8.0.1
scaffolding with "dotnet ef dbcontext scaffold ..."
with v7 of the framework, it would generate the following:
but after upgrading to 8.0, it now scaffolds as:
with the following SQL:
The text was updated successfully, but these errors were encountered: