EntityFramework.Docs icon indicating copy to clipboard operation
EntityFramework.Docs copied to clipboard

Pre-model convention configuration overrides foreign key data type

Open wdhenrik opened this issue 3 years ago • 4 comments

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();

wdhenrik avatar Mar 02 '22 14:03 wdhenrik

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 avatar Mar 07 '22 10:03 ajcvickers

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?

wdhenrik avatar Mar 08 '22 14:03 wdhenrik

@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 (https://github.com/dotnet/efcore/issues/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.

AndriySvyryd avatar Apr 20 '22 17:04 AndriySvyryd

I agree that it is a blunt tool, and that was what I was hoping for. I didn’t want nvarchar(max) columns all over the place so I was considering setting the length to 50 which is our most common, or maybe just a length of 1 to force the developer’s hand. Pre-model convention seemed like the perfect tool for the job. Unfortunately, it is so blunt to be unusable for this purpose since it also affects Foreign Key definitions. Requiring the FK definition to be kept in synch with the PK just pushes the configuration issue to another place so the pre-model convention effectively becomes pointless.

Somewhere in EFCore there is a default string mapping of nvarchar(max). All of the other datatypes have a default mapping as well. I would like the pre-model convention to affect the model at that level. The great thing about affecting it there is the foreign key configuration is already built in and shouldn’t require any additional work.

Every database I’ve worked with has FKs, and I can’t think of any data types that would benefit from configuration, but would also never be used as a PK. With its current implementation, I struggle to see the use case, knowing that other built-in configuration conventions will be ignored.

wdhenrik avatar Apr 20 '22 17:04 wdhenrik