ErikEJ / EFCorePowerTools

Entity Framework Core Power Tools - reverse engineering, migrations and model visualization in Visual Studio & CLI
MIT License
2.13k stars 294 forks source link

Pluralization issues on SQL table named 'TargetLens' #1603

Closed jdanielpa closed 1 year ago

jdanielpa commented 1 year ago

First I would like to say EF Power Tools is an amazing tool!

I have a SQL table named TargetLens. Using EF Core Power Tools (EF Core 7) reverse engineering results in a class named TargetLen (the 's' is missing from the end of the class name)

I am able to use efpt.renaming.json file (see below) to set the names of the columns as needed but NewName for the table still drops the trailiing 's'. If I change the NewName to: "NewName": "TargetLenss" (2 trailing s) then the class is named TargetLenss (also with 2 trailing s). I need the class to be named TargetLens.

Any suggestions?

efpt.renaming.json [ { "SchemaName": "dbo", "Tables": [ { "Columns": [ { "Name": "LensID", "NewName": "LensID" }, { "Name": "SKU", "NewName": "SKU" }, { "Name": "DesignIDN", "NewName": "DesignIDN" } ], "Name": "TargetLens", "NewName": "TargetLens" } ], "UseSchemaName": false } ]

I am using: .NET 7 Visual Studio 2022

Thanks for the help!

ErikEJ commented 1 year ago

Happy to help, but I need a full repro, with your efpt.config.json file and a CREATE TABLE statrement.

jdanielpa commented 1 year ago

Hi Erik,

Here is the SQL Create script:

-- --Create Tables -- CREATE TABLE dbo.Lot ( LotID int NOT NULL IDENTITY (1, 1), LotNumber varchar(20) NOT NULL ) ON [PRIMARY] GO ALTER TABLE dbo.Lot ADD CONSTRAINT PK_Lot PRIMARY KEY CLUSTERED (LotID) WITH( STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY] GO

CREATE TABLE dbo.TargetLens ( LensID int NOT NULL IDENTITY (1, 1), LotID int NOT NULL, ProjectCode varchar(4) NOT NULL, DesignIDN int NOT NULL, SKU varchar(10) NOT NULL ) ON [PRIMARY] GO ALTER TABLE dbo.TargetLens ADD CONSTRAINT PK_TargetLens PRIMARY KEY CLUSTERED (LensID) WITH( STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY] GO

ALTER TABLE dbo.TargetLens ADD CONSTRAINT FK_TargetLens_Lot FOREIGN KEY (LotID) REFERENCES dbo.Lot (LotID) ON DELETE CASCADE GO

-- --End of Create Tables --

--Here is the efpt.config.json:

{ "CodeGenerationMode": 3, "ContextClassName": "DEBUGContext", "ContextNamespace": null, "DefaultDacpacSchema": null, "FilterSchemas": false, "IncludeConnectionString": true, "ModelNamespace": null, "OutputContextPath": null, "OutputPath": "Models", "PreserveCasingWithRegex": true, "ProjectRootNamespace": "ClassLibrary1", "Schemas": null, "SelectedHandlebarsLanguage": 0, "SelectedToBeGenerated": 0, "Tables": [ { "Name": "[dbo].[Lot]", "ObjectType": 0 }, { "Name": "[dbo].[TargetLens]", "ObjectType": 0 } ], "UiHint": "PAW111000.DEBUG", "UseBoolPropertiesWithoutDefaultSql": false, "UseDatabaseNames": false, "UseDbContextSplitting": true, "UseFluentApiOnly": false, "UseHandleBars": true, "UseHierarchyId": false, "UseInflector": true, "UseLegacyPluralizer": true, "UseManyToManyEntity": true, "UseNoConstructor": false, "UseNoDefaultConstructor": false, "UseNoNavigations": false, "UseNoObjectFilter": false, "UseNodaTime": false, "UseNullableReferences": false, "UseSchemaFolders": false, "UseSpatial": false, "UseT4": false }

There are two issues I am having with Pluralization. The TargetLens table creates a class called 'TargetLen' - no trailing 's'. The other issue is in the Lot class - ICollectionTargetLens This property should be named 'TargetLenses'. If I try to use the efpt.property-renaming.json, I can change the property name to TargetLenses but this breaks the InverseProperty "TargetLens" in the TargetLen class on the Lot property because the InverseProperty does not get renamed to "TargetLenses".

Please let me know if this makes sense and thanks again for taking time to help me out. Here is a zip to the code which includes the SQL script:

WinFormsApp1.zip

ErikEJ commented 1 year ago

Why do you need pluralization?

jdanielpa commented 1 year ago

The project I am working on contains many SQL tables with many relationships. If I turn pluralization off then yes - I get a table named TargetLens. However, all of my collections are no longer pluralized which is not convenient. I could possibly make this work if I could use the efpt.property-renaming.json file to rename the collections. I can't do that right now though since that breaks the InverseProperty attributes.

The project started with framework 4.8 using edmx. We are trying to migrate to .NET and keep the class names and property names unchanged.

Thanks again for the help.

ErikEJ commented 1 year ago

I will have a look.

@R4ND3LL Thoughts about the renaming issue mentioned above?

@jdanielpa I assume you are referring to the InverseProperty attribute?

jdanielpa commented 1 year ago

Yes and thank you!

R4ND3LL commented 1 year ago

Unfortunately, this is caused by the package Bricelam.EntityFrameworkCore.Pluralizer, which is used by EFPT during scaffolding. I have confirmed with a unit test that this pluralizer produces "Lenss".

I hit this issue a number of times in my package and have since switched to Humanizer.Core, which is what EF itself uses. Humanizer pluralizes the word as "Lens".

@jdanielpa, for what it's worth, I tested this custom name with Entity Framework Ruler and the scaffolding result is perfect.

Since you are upgrading from EDMX, I suggest you give it a try. You can literally have an identical EF Core 6 or 7 model from your EDMX in minutes. I'm currently working on a UI for the rules, which will be available shortly.

This part of the linked doc will be of interest to you:

Upgrading from EF6 couldn't be simpler:

1) The the CLI tool to analyze the EDMX for all customizations and generate the rules. 2) Reference EntityFrameworkRuler.Design from the EF Core project. 3) Run the ef dbcontext scaffold command and the design-time service will do the rest.

ErikEJ commented 1 year ago

@jdanielpa Ah, you use the legacy pluralizer. Try to switch to the standard one (Humanizer)...

ErikEJ commented 1 year ago

@R4ND3LL I am more concerned about the InverseProperty issue.

jdanielpa commented 1 year ago

How do I switch to Humanizer.Core? Is it one of the options in the Reverse Engineer UI?

I will also look at the option of upgrading from EF6. The main issue there is the database is still being updated so I believe I will have to manage a separate edmx and run the upgrade tools every time there is a DB change?

Thank you both for all the help.

ErikEJ commented 1 year ago

Yes - do not use the legacy pluralizer like you do now ,(under Advanced)

R4ND3LL commented 1 year ago

@R4ND3LL I am more concerned about the InverseProperty issue.

I loaded up the provided test project. I see the issue. The inverse property looks like this:

        [ForeignKey(nameof(LotId))]
        [InverseProperty("TargetLens")]
        public virtual Lot Lot { get; set; }

By default, Roslyn will not rename instances of the identifier in strings.

If this attribute was written like the following, it would work properly:

        [InverseProperty(nameof(Log.TargetLens))]

CommentCSharpEntityTypeGenerator.GenerateInversePropertyAttribute() could be modified to use the nameof convention for BOTH ends and that would resolve this issue. I'm guessing that this is what provides the handlebars annotation output? See this piece of code:

                        !navigation.DeclaringEntityType.GetPropertiesAndNavigations().Any(
                                m => m.Name == inverseNavigation.DeclaringEntityType.Name)
                            ? $"nameof({inverseNavigation.DeclaringEntityType.Name}.{inverseNavigation.Name})"
                            : code.Literal(inverseNavigation.Name));

Alternatively, in EFPT, the rename command (in ApplyRenamingRulesAsync) can enable "rename in strings" by using the following:

                        docWithRename = await project.Documents.RenamePropertyAsync(
                            classRename.Name,
                            fromName,
                            refRename.NewName,
                            renameInStrings: true);

I tested it and it works for this case, however, it's possible this option could rename the wrong string sometimes so it may be best if this option was configurable. That said, I recommend fixing the "nameof" output instead.

R4ND3LL commented 1 year ago

I will also look at the option of upgrading from EF6. The main issue there is the database is still being updated so I believe I will have to manage a separate edmx and run the upgrade tools every time there is a DB change?

@jdanielpa, EF Ruler's upgrade path does not require you to hang on to an EDMX. Every time there is a DB change, you just run the scaffold command again. (same as with any DB first approach)

By default, the scaffolding will be limited to the tables and columns found in your original EDMX only. However, you can opt-in elements by enabling the IncludeUnknownTables or IncludeUnknownColumns flags accordingly.

If you add a new table, for example, you can set the IncludeUnknownTables flag to true and it will bring in all the new tables. The json file will automatically update to include the found tables so that you can tweak the column names as necessary. Then rerun the scaffold to apply the changes.

But if you have a lot of tables in the DB that you DON'T want in the model, you can leave the IncludeUnknownTables flag off, and add the table stub manually to the json.

If you have questions about EF Ruler, you can post an issue here and I'll help you over there.

ErikEJ commented 1 year ago

I was not able to find a solution that fulfilled your contradicting needs, so Handlebars or T4 templates seem the best option.

jdanielpa commented 1 year ago

Thank you guys for the help. I will take a look at Entity Framework Ruler.