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

Should pre-convention configuration overwrite data annotations? #28541

Closed
Peter-B- opened this issue Jul 29, 2022 · 5 comments
Closed

Should pre-convention configuration overwrite data annotations? #28541

Peter-B- opened this issue Jul 29, 2022 · 5 comments

Comments

@Peter-B-
Copy link

I use pre-convention configuration to set the default maximum string length for my context to 256:

    protected override void ConfigureConventions(ModelConfigurationBuilder configurationBuilder)
    {
        base.ConfigureConventions(configurationBuilder);

        configurationBuilder.Properties<string>()
            .HaveMaxLength(256);
    }

When I override this with a [StringLength(100)] or [MaxLength(100)] attribute, the default value (here 256) is used and the attribute ignored. Overwriting the string length with fluent configuration is working.

public class User
{
    [Key]
    public int UserId { get; set; }

    // Attribute is ignored
    // length is set to nvarchar(256)
    [StringLength(100)] 
    public string? FirstName { get; set; }

    public string? LastName { get; set; }

    internal class DbConfiguration : IEntityTypeConfiguration<User>
    {
        public void Configure(EntityTypeBuilder<User> builder)
        {
            // Fluent API configuration is respected
            // length is set to nvarchar(100)
            builder.Property(u => u.LastName).HasMaxLength(100);
        }
    }
}

Is this intended behavior or an issue?

I personally prefer to set string length in attributes, since this is configured directly next to the property.

Include provider and version information

EF Core version: 6.0.1
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .Net 6.0

Kind regards
Peter

@smitpatel
Copy link
Contributor

Intended behavior. The bulk configuration has same priority as fluent API since it is explicitly specified by user.

@Peter-B-
Copy link
Author

Oh. That is sad :-) I would have expected, that pre-convention configurations are always overwritten by explicit (fluent or data annotation) configuration.

But thanks a lot for clarifying.

For anyone interested in a workaround: I use this extension method based on https://stackoverflow.com/a/50852517/909237

public static class ModelBuilderExtensions
{
#pragma warning disable EF1001
    public static void SetMaxStringLength(this ModelBuilder modelBuilder, int stringLength)
    {
        // https://stackoverflow.com/a/50852517/909237
        // Use explicit setting instead of pre convention configuration, since the latter breaks [MaxLength] attributes
        // https://docs.microsoft.com/en-us/ef/core/modeling/bulk-configuration#pre-convention-configuration

        var stringProperties = modelBuilder.Model.GetEntityTypes()
            .SelectMany(t => t.GetProperties())
            .Where(p => p.ClrType == typeof(string))
            .OfType<Property>();

        foreach (var property in stringProperties)
            property.Builder.HasMaxLength(stringLength, ConfigurationSource.Convention);
    }
}
    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        base.OnModelCreating(modelBuilder);

        modelBuilder.SetMaxStringLength(256);
    }

@ajcvickers ajcvickers reopened this Jul 29, 2022
@Peter-B-
Copy link
Author

Actually, I'm not sure if this is really true: According to ConfigurationSourceExtensions.Overrides(), both DataAnnotation and Explicit should overwrite Convention.

So is it really intended, that pre-convention overrides DataAnnotation?

@Peter-B- Peter-B- reopened this Jul 29, 2022
@ajcvickers
Copy link
Member

We discussed this, and while the naming is perhaps unfortunate, and the experience of overriding with an annotation would be nice, the reality is that this method sets up low-level type mapping which acts like explicit configuration. Public conventions, which are planned for EF7, will allow a convention to be written that will allow overriding via data annotations.

@DanielStout5
Copy link

For anyone else finding this and intending to use it to avoid the warning about Precision not configured (without overriding any attributes you did set) you can do:

 if (prop.Builder.Metadata.GetPrecision() == null)
 {
     prop.Builder.HasPrecision(18, ConfigurationSource.DataAnnotation);
     prop.Builder.HasScale(2, ConfigurationSource.DataAnnotation);
 }

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

No branches or pull requests

4 participants