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

Pre-model convention configuration overrides foreign key data type #3756

Closed
wdhenrik opened this issue Mar 2, 2022 · 4 comments · Fixed by #4128
Closed

Pre-model convention configuration overrides foreign key data type #3756

wdhenrik opened this issue Mar 2, 2022 · 4 comments · Fixed by #4128

Comments

@wdhenrik
Copy link

wdhenrik commented Mar 2, 2022

I've used pre-model configuration to default all my strings to a max length of 500 as I don't want MAX columns used everywhere.

HOWEVER, when I create a navigation property, the pre-model convention overrides the data of the foreign key relationship. For example, my primary key is a varchar(10). When referenced as a foreign key, it should also be a varchar(10), but the pre-model convention is applied instead, creating a varchar(500).

A foreign key column should match the primary key definition exactly. With the current implementation, using pre-model convention requires the foreign key column and type to be explicitly declared, requiring additional code, unnecessarily duplicating the defintion, and introducing a possible point of failure if they are not kept in synch.

I would prefer that foreign keys are excluded from pre-model configuration. If that is not an option, I would like to see an additional flag added to the configuration builder to prioritize the key data type in a relationship

configurationBuilder
    .Properties<string>()
    .HaveMaxLength(500)
    .AreUnicode(false)
//suggestion
    .PreferTypeFromKey();
@ajcvickers
Copy link
Member

Notes from triage: We should update the docs to make this clear. However, because pre-convention runs before conventions or regular model building, it is not feasible to predicate these behaviors on information that is not inherently part of the CLR type.

@ajcvickers ajcvickers transferred this issue from dotnet/efcore Mar 7, 2022
@ajcvickers ajcvickers added this to the Backlog milestone Mar 7, 2022
@wdhenrik
Copy link
Author

wdhenrik commented Mar 8, 2022

Can I beg that this gets more than just a documentation update?

Pre-model convention configuration will be of very limited use if it overrides the foreign key definition. It silently requires a choice between configuring all non-key columns of T or all foreign-key relationships of T. With this behavior, it adds development complexity and maintenance overhead, and the PK/FK type mismatch error doesn't occur until the migration is attempted.

Strings are defaulted to nvarchar(max), but that is overridden for generated foreign keys. Why can't the pre-model configuration change the default at that level since the desired FK behavior already exists?

@AndriySvyryd
Copy link
Member

@wdhenrik Pre-convention model configuration is really a blunt way to apply simple configuration in bulk. If you need any subtlety or conditional logic then custom conventions (dotnet/efcore#214) are the way to go. You can already add them in 6.0, but there's currently no documentation and you'd need to add an IConventionSetPlugin service implementation.

@wdhenrik
Copy link
Author

wdhenrik commented Apr 20, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants