EntityFramework-Reverse-POCO-Code-First-Generator icon indicating copy to clipboard operation
EntityFramework-Reverse-POCO-Code-First-Generator copied to clipboard

Strange naming of virtual property, when there are multiple references to the same table

Open kinetiq opened this issue 4 years ago • 16 comments

I have a table, UserDocument, that has two references to the User table: UserId and CreatedByUserId.

This generates:

        public virtual User CreatedByUser { get; set; }
        public virtual User UserUser { get; set; }

To me, UserUser should definitely just be User. Is there a way to fix this without unexpected consequences?

We are in the process of porting a project from using dotnet ef dbcontext scaffold which I assume is going to be a common use case. I would be willing to document the process if you can help me get this into alignment.

kinetiq avatar Apr 02 '20 13:04 kinetiq

Hi Brian,

In the Settings.ForeignKeyName delegate in your *.tt file, try adding this after the switch statement.

if (fkName == "UserUser")
    return "User";

sjh37 avatar Apr 02 '20 15:04 sjh37

Thanks for getting back to me. Surprisingly, that didn't work. Here's what I did:

    Settings.ForeignKeyName = delegate(string tableName, ForeignKey foreignKey, string foreignKeyName, Relationship relationship, short attempt)
    {
        string fkName;

        // 5 Attempts to correctly name the foreign key
        switch (attempt)
        {
            case 1:
                // Try without appending foreign key name
                fkName = tableName;
                break;

            case 2:
                // Only called if foreign key name ends with "id"
                // Use foreign key name without "id" at end of string
                fkName = foreignKeyName.Remove(foreignKeyName.Length - 2, 2);
                break;

            case 3:
                // Use foreign key name only
                fkName = foreignKeyName;
                break;

            case 4:
                // Use table name and foreign key name
                fkName = tableName + foreignKeyName;
                break;

            case 5:
                // Used in for loop 1 to 99 to append a number to the end
                fkName = tableName;
                break;

            default:
                // Give up
                fkName = tableName;
                break;
        }

        if (fkName == "UserUser")
            return "User";

        if(fkName.ToLowerInvariant().EndsWith("id"))
        fkName = fkName.Remove(fkName.Length - 2, 2);
        
        // Apply custom foreign key renaming rules. Can be useful in applying pluralization.
        // For example:
        /*if (tableName == "Employee" && foreignKey.FkColumn == "ReportsTo")
            return "Manager";

        if (tableName == "Territories" && foreignKey.FkTableName == "EmployeeTerritories")
            return "Locations";

        if (tableName == "Employee" && foreignKey.FkTableName == "Orders" && foreignKey.FkColumn == "EmployeeID")
            return "ContactPerson";
        */

        // FK_TableName_FromThisToParentRelationshipName_FromParentToThisChildsRelationshipName
        // (e.g. FK_CustomerAddress_Customer_Addresses will extract navigation properties "address.Customer" and "customer.Addresses")
        // Feel free to use and change the following
        /*if (foreignKey.ConstraintName.StartsWith("FK_") && foreignKey.ConstraintName.Count(x => x == '_') == 3)
        {
            var parts = foreignKey.ConstraintName.Split('_');
            if (!string.IsNullOrWhiteSpace(parts[2]) && !string.IsNullOrWhiteSpace(parts[3]) && parts[1] == foreignKey.FkTableName)
            {
                if (relationship == Relationship.OneToMany)
                    fkName = parts[3];
                else if (relationship == Relationship.ManyToOne)
                    fkName = parts[2];
            }
        }*/

        return fkName;
    };

I feel like this should obviously be working, I'm going to keep fiddling with it.

kinetiq avatar Apr 02 '20 15:04 kinetiq

You can debug it. Add a line:

if (tableName.ToLowerInvariant() == "UserDocument")
    Console.WriteLine(); // add breakpoint here

Right click on the T4 template and select "Debug T4 Template" image

Hovering your mouse over variables does not work, you have to view variables in the standard variable pane.

sjh37 avatar Apr 02 '20 16:04 sjh37

Well, assuming this is the right field, here's what Is see:

image

Strangely... The values in fkName don't seem to match up with what's being generated... Nothing says UserUser at any point.

Also, when I look at the case of the CreatedByUser field mentioned above, which is being generated as expected, here's what I see for it in debug:

image

fkName, which is what this method returns, is "UserDocumentCreatedByUserId" ... Confusingly, that is not what gets generated. However, I can see that foreignKeyName is "CreatedByUserId" which is very close to what gets generated:

public virtual User CreatedByUser { get; set; }

This is confusing. These two variables essentially have the same name but do different things and have different values. I'm going to delve into the .ttinclude... Spending a lot of time on this today. Help. :)

kinetiq avatar Apr 02 '20 17:04 kinetiq

Attempt is an important number.

I can see it's on attempt 4 on the bottom screen shot, which matches the switch statement above your code. Attempt 4 does this in the switch:

// Use table name and foreign key name
fkName = tableName + foreignKeyName;

Hence why fkName comes out as UserDocumentCreatedByUserId

If there is a name clash, then it will try attempt 2, then 3 and so on. That is handled by Table.GetUniqueColumnName. This function is actually called twice. Once with checkForFkNameClashes = true and one with checkForFkNameClashes = false within the function Generator.AddForeignKeysToFilters.

ProcessForeignKeys(foreignKeys, true, filter);
ProcessForeignKeys(foreignKeys, false, filter);

First one is to pre-process the foreign keys and work out if there are any name clashes, and the second one to do the actual naming knowing up front if it would clash or not.

sjh37 avatar Apr 03 '20 11:04 sjh37

Okay, thanks for the insight.

So that leaves me with two questions, and a thought:

  • First, why doesn't UserUser ever show up as a value of fkName? I set that breakpoint and looked at every field generated for the UserDocument table. UserUser was never a value in fkName. Maybe it's being generated as part of User since it's a navigation property??
  • More importantly, how can I control what gets generated? Modifying fkName does not seem to change what gets generated. To me it is super weird that User would be generated as UserUser.
  • Also, a passing thought, and this is not very important: I imagine you are going to have a lot of frustrating support scenarios where you can't see into the user's environment well enough to provide the kind of support you want, and that may start to be an issue now that this is a commercial product. I once wrote an ORM that did a ton of code generation, and there was this intermediate step where a map file was created that modeled the entire database. Then there was a config file that could override parts of the map, and generation worked off of those files rather than the live database. I mention this because I find myself wishing I could just send you my map file.

Thanks Simon!

kinetiq avatar Apr 03 '20 13:04 kinetiq

I'll look into it and get back to you!

sjh37 avatar Apr 03 '20 13:04 sjh37

Could you give me the sql to create the User and UserDocument table, I'll use your tables as an actual test. If not don't worry, I'll come up with my own version of the same thing.

sjh37 avatar Apr 03 '20 13:04 sjh37

Sure:

CREATE TABLE [User]
(
    ID             INT         IDENTITY(1, 1) NOT NULL,
    ExternalUserID VARCHAR(50) NULL,
    CONSTRAINT PK_Contacts PRIMARY KEY CLUSTERED (ID ASC)
);
GO
CREATE TABLE User_Document
(
    ID              INT IDENTITY(1, 1) NOT NULL,
    UserID          INT NOT NULL,
    CreatedByUserID INT NOT NULL,
    CONSTRAINT PK_User_Document PRIMARY KEY CLUSTERED (ID ASC)
);
GO
ALTER TABLE User_Document WITH CHECK
ADD CONSTRAINT FK_User_Document_User FOREIGN KEY (UserID) REFERENCES [User] (ID) ON DELETE CASCADE;
GO
ALTER TABLE [User_Document] CHECK CONSTRAINT [FK_User_Document_User]
GO
ALTER TABLE [User_Document]  WITH CHECK ADD  CONSTRAINT [FK_User_Document_User1] FOREIGN KEY([CreatedByUserID])
REFERENCES [User] ([ID])
GO
ALTER TABLE [User_Document] CHECK CONSTRAINT [FK_User_Document_User1]
GO

kinetiq avatar Apr 03 '20 16:04 kinetiq

I get this generated as standard:

// User
public class User
{
    public int Id { get; set; } // ID (Primary key)
    public string ExternalUserId { get; set; } // ExternalUserID (length: 50)

    // Reverse navigation

    /// <summary>
    /// Child UserDocuments where [User_Document].[CreatedByUserID] point to this entity (FK_User_Document_User1)
    /// </summary>
    public virtual ICollection<UserDocument> UserDocuments_CreatedByUserId { get; set; } // User_Document.FK_User_Document_User1

    /// <summary>
    /// Child UserDocuments where [User_Document].[UserID] point to this entity (FK_User_Document_User)
    /// </summary>
    public virtual ICollection<UserDocument> UserDocuments_UserId { get; set; } // User_Document.FK_User_Document_User

    public User()
    {
        UserDocuments_CreatedByUserId = new List<UserDocument>();
        UserDocuments_UserId = new List<UserDocument>();
    }
}

// User_Document
public class UserDocument
{
    public int Id { get; set; } // ID (Primary key)
    public int UserId { get; set; } // UserID
    public int CreatedByUserId { get; set; } // CreatedByUserID

    // Foreign keys

    /// <summary>
    /// Parent User pointed by [User_Document].([CreatedByUserId]) (FK_User_Document_User1)
    /// </summary>
    public virtual User CreatedByUser { get; set; } // FK_User_Document_User1

    /// <summary>
    /// Parent User pointed by [User_Document].([UserId]) (FK_User_Document_User)
    /// </summary>
    public virtual User User_UserId { get; set; } // FK_User_Document_User
}

sjh37 avatar Apr 03 '20 17:04 sjh37

Adding in

if (fkName == "User_UserId")
    return "DocumentUser";

gives

public virtual User DocumentUser { get; set; } // FK_User_Document_User

Adding in

if (fkName == "User_UserId")
    return "User";

gives

public virtual User User1 { get; set; } // FK_User_Document_User

sjh37 avatar Apr 03 '20 17:04 sjh37

Standard (not renaming anything) is:

checkForFkNameClashes = true

# fkName name from Settings.ForeignKeyName
1 CreatedByUserId attempt1 = 'User'
2 UserId attempt1 = 'User' (clashes with line 1)
3 attempt2 = Strips Id from fkName, 'User' (clashes with line 1)
4 attempt3 (Use foreign key name only) 'UserId' (clashes with line 2)
5 attempt4 (Use table name and foreign key name) 'User_UserId'

checkForFkNameClashes = false

# fkName name from Settings.ForeignKeyName
6 CreatedByUserId attempt1 = 'User' (clashes with line 1)
7 attempt2 = 'CreatedByUserId'
8 UserId attempt1 = 'User' (clashes with line 1)
9 attempt2 = Strips Id from fkName, 'User' (clashes with line 1)
10 attempt3 (Use foreign key name only) 'UserId' (clashes with line 2)
11 attempt4 (Use table name and foreign key name) 'User_UserId'

Seems to me UserId FK should get first go at being named 'User' as it's almost the same as the table name it references. There are other discrepancies, such as line 6 clashing with itself on line 1.

sjh37 avatar Apr 03 '20 17:04 sjh37

Thanks Simon. Currently looking into why my version of this is generating different code... I suspect the dev who was on this before me made some changes, so I need to do a diff, figure out what's going on and re-think my life.

kinetiq avatar Apr 03 '20 19:04 kinetiq

Looks like the previous dev changed two relevant things:

  • added some code to always strip id from the end of fkName
       if(fkName.ToLowerInvariant().EndsWith("id"))
        fkName = fkName.Remove(fkName.Length - 2, 2);
  • Changed attempt 4 to remove the underscore:

fkName = tableName + "_" + foreignKeyName; was changed to...

fkName = tableName + foreignKeyName;

I'm sure this was part of an attempt to get this to generate something closer to what dotnet ef dbcontext scaffold emits.

It explains how UserUser could have happened I think. I wonder if it's creating additional collisions.

...Still grinding...

kinetiq avatar Apr 03 '20 20:04 kinetiq

FYI, after a couple days, I ended up running out of time to work on this since it is primarily a quality of life item for me and the other dev on my team and I wasn't making much progress.

The goal was to migrate from using dotnet ef dbcontext scaffold, which has some annoying problems... However, in its current form, I think the only reasonable way to make that happen will be to actually change my project to fit the conventions you use, not the other way around.

That may be a fairly big job. Just to provide you with some feedback, I find myself wishing that either (1) there was some easier migration path for projects like this, although I suspect there isn't going to be much demand on migrating projects of this type or (2) really wish the code in this area was easier to understand so I could perhaps contribute. The chart you provided both helped and made me realize I won't be able to get much done here currently, especially since you uncovered some internal problems.

kinetiq avatar Apr 23 '20 13:04 kinetiq

Hi Brian,

Yes I think you've hit on the quicker solution. To quickly change your code to use the FK names generated by this generator. An easy way to do this is using the Resharper command Ctrl-R to rename a property/variable. It will do it for your whole solution in a single go. A great time saver.

The problem I have is, if I change how the FK's are named, and push out a new release, it would break existing code, new code would be ok of course. Best way forward for this is if I did change it, would be to introduce a UseLegacyFkName boolean flag, users could turn on/off.

Please leave this case open, as I do want to make sure I get it right, for as many people as I can.

sjh37 avatar Apr 23 '20 14:04 sjh37