Set max length for string discriminator properties

Fixes #10691
dotnet · Dec 29, 2022 · 8f5621a · 8f5621a
1 parent 0993a63
commit 8f5621a
Show file tree

Hide file tree

Showing 13 changed files with 106 additions and 17 deletions.
diff --git a/src/EFCore.Relational/Metadata/Conventions/DiscriminatorLengthConvention.cs b/src/EFCore.Relational/Metadata/Conventions/DiscriminatorLengthConvention.cs
@@ -0,0 +1,72 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.EntityFrameworkCore.Metadata.Conventions;
+
+/// <summary>
+///     A convention that sets the maximum length for string discriminator properties.
+/// </summary>
+/// <remarks>
+///     <para>
+///         The maximum length is set to a value large enough to cover all discriminator values in the hierarchy.
+///     </para>
+///     <para>
+///         See <see href="https://aka.ms/efcore-docs-conventions">Model building conventions</see> and
+///         <see href="https://aka.ms/efcore-docs-inheritance">TPH mapping of inheritance hierarchies</see> for more information
+///         and examples.
+///     </para>
+/// </remarks>
+public class DiscriminatorLengthConvention : IModelFinalizingConvention
+{
+    /// <summary>
+    ///     Creates a new instance of <see cref="DiscriminatorLengthConvention" />.
+    /// </summary>
+    /// <param name="dependencies">Parameter object containing dependencies for this convention.</param>
+    /// <param name="relationalDependencies"> Parameter object containing relational dependencies for this convention.</param>
+    public DiscriminatorLengthConvention(
+        ProviderConventionSetBuilderDependencies dependencies,
+        RelationalConventionSetBuilderDependencies relationalDependencies)
+    {
+        Dependencies = dependencies;
+        RelationalDependencies = relationalDependencies;
+    }
+
+    /// <summary>
+    ///     Dependencies for this service.
+    /// </summary>
+    protected virtual ProviderConventionSetBuilderDependencies Dependencies { get; }
+
+    /// <summary>
+    ///     Relational provider-specific dependencies for this service.
+    /// </summary>
+    protected virtual RelationalConventionSetBuilderDependencies RelationalDependencies { get; }
+
+    /// <inheritdoc />
+    public virtual void ProcessModelFinalizing(IConventionModelBuilder modelBuilder, IConventionContext<IConventionModelBuilder> context)
+    {
+        foreach (var entityType in modelBuilder.Metadata.GetEntityTypes()
+                     .Where(entityType => entityType.BaseType == null))
+        {
+            var discriminatorProperty = entityType.FindDiscriminatorProperty();
+            if (discriminatorProperty != null
+                && discriminatorProperty.ClrType == typeof(string)
+                && !discriminatorProperty.IsKey()
+                && !discriminatorProperty.IsForeignKey())
+            {
+                var maxDiscriminatorValueLength =
+                    entityType.GetDerivedTypesInclusive().Select(e => ((string)e.GetDiscriminatorValue()!).Length).Max();
+
+                var previous = 1;
+                var current = 1;
+                while (maxDiscriminatorValueLength > current)
+                {
+                    var next = current + previous;
+                    previous = current;
+                    current = next;
+                }
+
+                discriminatorProperty.Builder.HasMaxLength(current);
+            }
+        }
+    }
+}
diff --git a/src/EFCore.Relational/Metadata/Conventions/Infrastructure/RelationalConventionSetBuilder.cs b/src/EFCore.Relational/Metadata/Conventions/Infrastructure/RelationalConventionSetBuilder.cs
@@ -73,6 +73,7 @@ public override ConventionSet CreateConventionSet()
         conventionSet.Add(new TableValuedDbFunctionConvention(Dependencies, RelationalDependencies));
         conventionSet.Add(new StoreGenerationConvention(Dependencies, RelationalDependencies));
         conventionSet.Add(new EntitySplittingConvention(Dependencies, RelationalDependencies));
+        conventionSet.Add(new DiscriminatorLengthConvention(Dependencies, RelationalDependencies));
         conventionSet.Add(new EntityTypeHierarchyMappingConvention(Dependencies, RelationalDependencies));
         conventionSet.Add(new SequenceUniquificationConvention(Dependencies, RelationalDependencies));
         conventionSet.Add(new SharedTableConvention(Dependencies, RelationalDependencies));

diff --git a/test/EFCore.Design.Tests/Migrations/ModelSnapshotSqlServerTest.cs b/test/EFCore.Design.Tests/Migrations/ModelSnapshotSqlServerTest.cs
@@ -584,7 +584,8 @@ public virtual void Entities_are_stored_in_model_snapshot_for_TPT_with_one_exclu
                     SqlServerPropertyBuilderExtensions.UseIdentityColumn(b.Property<int>(""Id""));
 
                     b.Property<string>(""Discriminator"")
-                        .HasColumnType(""nvarchar(max)"");
+                        .HasMaxLength(13)
+                        .HasColumnType(""nvarchar(13)"");
 
                     b.HasKey(""Id"");
 
@@ -1400,7 +1401,8 @@ public virtual void CheckConstraint_is_only_stored_in_snapshot_once_for_TPH()
 
                     b.Property<string>(""Discriminator"")
                         .IsRequired()
-                        .HasColumnType(""nvarchar(max)"");
+                        .HasMaxLength(13)
+                        .HasColumnType(""nvarchar(13)"");
 
                     b.HasKey(""Id"");
 
@@ -1711,7 +1713,8 @@ public virtual void BaseType_is_stored_in_snapshot()
 
                     b.Property<string>(""Discriminator"")
                         .IsRequired()
-                        .HasColumnType(""nvarchar(max)"");
+                        .HasMaxLength(21)
+                        .HasColumnType(""nvarchar(21)"");
 
                     b.HasKey(""Id"");
 
@@ -1780,7 +1783,8 @@ public virtual void Discriminator_annotations_are_stored_in_snapshot()
 
                     b.Property<string>(""Discriminator"")
                         .IsRequired()
-                        .HasColumnType(""nvarchar(max)"");
+                        .HasMaxLength(21)
+                        .HasColumnType(""nvarchar(21)"");
 
                     b.HasKey(""Id"");
 
@@ -4238,7 +4242,8 @@ public virtual void Property_column_name_on_specific_table_is_stored_in_snapshot
 
                     b.Property<string>(""Discriminator"")
                         .IsRequired()
-                        .HasColumnType(""nvarchar(max)"");
+                        .HasMaxLength(34)
+                        .HasColumnType(""nvarchar(34)"");
 
                     b.HasKey(""Id"");
 
@@ -6234,7 +6239,8 @@ public virtual void Do_not_generate_entity_type_builder_again_if_no_foreign_key_
 
                     b.Property<string>(""Discriminator"")
                         .IsRequired()
-                        .HasColumnType(""nvarchar(max)"");
+                        .HasMaxLength(13)
+                        .HasColumnType(""nvarchar(13)"");
 
                     b.Property<int?>(""NavigationId"")
                         .HasColumnType(""int"");

diff --git a/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs b/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs
@@ -8350,7 +8350,17 @@ public void Add_shared_property_with_foreign_key_on_subtypes()
                         x.HasIndex("HunterId").HasDatabaseName("IX_Animal_HunterId");
                     });
             },
-            operations => Assert.Equal(0, operations.Count));
+            operations =>
+            {
+                Assert.Equal(1, operations.Count);
+
+                var operation = Assert.IsType<AlterColumnOperation>(operations[0]);
+                Assert.Equal("Animal", operation.Table);
+                Assert.Equal("Discriminator", operation.Name);
+                Assert.Equal(typeof(string), operation.ClrType);
+                Assert.Equal("just_string(21)", operation.ColumnType);
+                Assert.Equal(21, operation.MaxLength);
+            });
 
     [ConditionalFact]
     public void Add_foreign_key_to_subtype()

diff --git a/test/EFCore.SqlServer.FunctionalTests/CustomConvertersSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/CustomConvertersSqlServerTest.cs
@@ -33,7 +33,7 @@ public virtual void Columns_have_expected_data_types()
 BinaryKeyDataType.Ex ---> [nullable nvarchar] [MaxLength = -1]
 BinaryKeyDataType.Id ---> [varbinary] [MaxLength = 900]
 Blog.BlogId ---> [int] [Precision = 10 Scale = 0]
-Blog.Discriminator ---> [nvarchar] [MaxLength = -1]
+Blog.Discriminator ---> [nvarchar] [MaxLength = 8]
 Blog.IndexerVisible ---> [nvarchar] [MaxLength = 3]
 Blog.IsVisible ---> [nvarchar] [MaxLength = 1]
 Blog.RssUrl ---> [nullable nvarchar] [MaxLength = -1]

diff --git a/.../EFCore.SqlServer.FunctionalTests/Query/IncompleteMappingInheritanceQuerySqlServerTest.cs b/.../EFCore.SqlServer.FunctionalTests/Query/IncompleteMappingInheritanceQuerySqlServerTest.cs
@@ -658,7 +658,7 @@ FROM [Animals] AS [a]
             //
 """
 @p0='0'
-@p1='Eagle' (Nullable = false) (Size = 4000)
+@p1='Eagle' (Nullable = false) (Size = 8)
 @p2='2' (Nullable = true)
 @p3='1' (Nullable = true)
 @p4='False' (Nullable = true)

diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/InheritanceQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/InheritanceQuerySqlServerTest.cs
@@ -629,7 +629,7 @@ FROM [Animals] AS [a]
             //
 """
 @p0='0'
-@p1='Eagle' (Nullable = false) (Size = 4000)
+@p1='Eagle' (Nullable = false) (Size = 8)
 @p2='2' (Nullable = true)
 @p3='1' (Nullable = true)
 @p4='False' (Nullable = true)

diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs
@@ -4079,7 +4079,7 @@ public virtual async Task Multilevel_owned_entities_determine_correct_nullabilit
 
             AssertSql(
 """
-@p0='BaseEntity13079' (Nullable = false) (Size = 4000)
+@p0='BaseEntity13079' (Nullable = false) (Size = 21)
 
 SET IMPLICIT_TRANSACTIONS OFF;
 SET NOCOUNT ON;

diff --git a/test/EFCore.SqlServer.FunctionalTests/TableSplittingSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/TableSplittingSqlServerTest.cs
@@ -122,7 +122,7 @@ public override async Task Can_change_dependent_instance_non_derived()
         AssertSql(
 """
 @p3='Trek Pro Fit Madone 6 Series' (Nullable = false) (Size = 450)
-@p0='LicensedOperator' (Nullable = false) (Size = 4000)
+@p0='LicensedOperator' (Nullable = false) (Size = 21)
 @p1='Repair' (Size = 4000)
 @p2='repairman' (Size = 4000)
 

diff --git a/test/EFCore.SqlServer.FunctionalTests/Update/StoredProcedureUpdateSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Update/StoredProcedureUpdateSqlServerTest.cs
@@ -560,7 +560,7 @@ AS BEGIN
         AssertSql(
 """
 @p0=NULL (Nullable = false) (Direction = Output) (DbType = Int32)
-@p1='Child1' (Nullable = false) (Size = 4000)
+@p1='Child1' (Nullable = false) (Size = 8)
 @p2='Child' (Size = 4000)
 @p3=NULL (DbType = Int32)
 @p4=NULL (Direction = Output) (DbType = Int32)

diff --git a/test/EFCore.SqlServer.FunctionalTests/UpdatesSqlServerTPCTest.cs b/test/EFCore.SqlServer.FunctionalTests/UpdatesSqlServerTPCTest.cs
@@ -21,7 +21,7 @@ public override void Save_with_shared_foreign_key()
 
         AssertContainsSql(
             @"@p0=NULL (Size = 8000) (DbType = Binary)
-@p1='ProductWithBytes' (Nullable = false) (Size = 4000)
+@p1='ProductWithBytes' (Nullable = false) (Size = 21)
 @p2=NULL (Size = 4000)
 
 SET IMPLICIT_TRANSACTIONS OFF;

diff --git a/test/EFCore.SqlServer.FunctionalTests/UpdatesSqlServerTPTTest.cs b/test/EFCore.SqlServer.FunctionalTests/UpdatesSqlServerTPTTest.cs
@@ -25,7 +25,7 @@ public override void Save_with_shared_foreign_key()
 
         AssertContainsSql(
             @"@p0=NULL (Size = 8000) (DbType = Binary)
-@p1='ProductWithBytes' (Nullable = false) (Size = 4000)
+@p1='ProductWithBytes' (Nullable = false) (Size = 21)
 @p2=NULL (Size = 4000)
 
 SET IMPLICIT_TRANSACTIONS OFF;

diff --git a/test/EFCore.SqlServer.FunctionalTests/UpdatesSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/UpdatesSqlServerTest.cs
@@ -19,15 +19,15 @@ public override void Save_with_shared_foreign_key()
 
         AssertContainsSql(
             @"@p0=NULL (Size = 8000) (DbType = Binary)
-@p1='ProductWithBytes' (Nullable = false) (Size = 4000)
+@p1='ProductWithBytes' (Nullable = false) (Size = 21)
 @p2=NULL (Size = 4000)
 
 SET IMPLICIT_TRANSACTIONS OFF;
 SET NOCOUNT ON;
 INSERT INTO [ProductBase] ([Bytes], [Discriminator], [ProductWithBytes_Name])
 OUTPUT INSERTED.[Id]
 VALUES (@p0, @p1, @p2);",
-            @"@p0='SpecialCategory' (Nullable = false) (Size = 4000)
+            @"@p0='SpecialCategory' (Nullable = false) (Size = 21)
 @p1=NULL (Size = 4000)
 @p2='777'