Fix propagation of fixed length facet

Fixes #18961 Fixes #14163
dotnet · Dec 30, 2019 · b230bc0 · b230bc0
1 parent 85bb77b
commit b230bc0
Show file tree

Hide file tree

Showing 12 changed files with 509 additions and 182 deletions.
diff --git a/src/EFCore.Design/Scaffolding/Internal/CSharpDbContextGenerator.cs b/src/EFCore.Design/Scaffolding/Internal/CSharpDbContextGenerator.cs
@@ -664,7 +664,7 @@ private void GenerateProperty(IProperty property, bool useDataAnnotations)
                     $"({(property.IsUnicode() == false ? "false" : "")})");
             }
 
-            if (property.IsFixedLength())
+            if (property.IsFixedLength() != null)
             {
                 lines.Add(
                     $".{nameof(RelationalPropertyBuilderExtensions.IsFixedLength)}()");

diff --git a/src/EFCore.Design/Scaffolding/Internal/ScaffoldingTypeMapper.cs b/src/EFCore.Design/Scaffolding/Internal/ScaffoldingTypeMapper.cs
@@ -134,13 +134,12 @@ public ScaffoldingTypeMapper([NotNull] IRelationalTypeMappingSource typeMappingS
                         ? (bool?)stringMapping.IsFixedLength
                         : null;
 
-                    // Check for size
                     var sizedMapping = _typeMappingSource.FindMapping(
                         typeof(string),
                         null,
                         keyOrIndex,
                         unicode: mapping.IsUnicode,
-                        fixedLength: mapping.IsFixedLength);
+                        fixedLength: false); // Fixed length with no size is not valid
 
                     scaffoldMaxLength = sizedMapping.Size != stringMapping.Size ? stringMapping.Size : null;
                 }

diff --git a/src/EFCore.Relational/Extensions/RelationalPropertyExtensions.cs b/src/EFCore.Relational/Extensions/RelationalPropertyExtensions.cs
@@ -360,24 +360,16 @@ private static object ConvertDefaultValue([NotNull] IProperty property, [CanBeNu
         /// </summary>
         /// <param name="property"> The property. </param>
         /// <returns> A flag indicating if the property as capable of storing only fixed-length data, such as strings. </returns>
-        public static bool IsFixedLength([NotNull] this IProperty property)
-            => (bool?)property[RelationalAnnotationNames.IsFixedLength] ?? GetDefaultIsFixedLength(property);
-
-        private static bool GetDefaultIsFixedLength(IProperty property)
-        {
-            var sharedTablePrincipalPrimaryKeyProperty = property.FindSharedTableRootPrimaryKeyProperty();
-            return sharedTablePrincipalPrimaryKeyProperty != null && IsFixedLength(sharedTablePrincipalPrimaryKeyProperty);
-        }
+        public static bool? IsFixedLength([NotNull] this IProperty property)
+            => (bool?)property[RelationalAnnotationNames.IsFixedLength];
 
         /// <summary>
         ///     Sets a flag indicating whether the property as capable of storing only fixed-length data, such as strings.
         /// </summary>
         /// <param name="property"> The property. </param>
         /// <param name="fixedLength"> A value indicating whether the property is constrained to fixed length values. </param>
         public static void SetIsFixedLength([NotNull] this IMutableProperty property, bool? fixedLength)
-            => property.SetOrRemoveAnnotation(
-                RelationalAnnotationNames.IsFixedLength,
-                fixedLength);
+            => property.SetOrRemoveAnnotation(RelationalAnnotationNames.IsFixedLength, fixedLength);
 
         /// <summary>
         ///     Sets a flag indicating whether the property as capable of storing only fixed-length data, such as strings.

diff --git a/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs b/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs
@@ -1200,7 +1200,7 @@ protected virtual IUpdateSqlGenerator SqlGenerator
             {
                 if (operation.IsUnicode == property.IsUnicode()
                     && operation.MaxLength == property.GetMaxLength()
-                    && (operation.IsFixedLength ?? false) == property.IsFixedLength()
+                    && operation.IsFixedLength == property.IsFixedLength()
                     && operation.IsRowVersion == (property.IsConcurrencyToken && property.ValueGenerated == ValueGenerated.OnAddOrUpdate))
                 {
                     return Dependencies.TypeMappingSource.FindMapping(property).StoreType;

diff --git a/src/EFCore.SqlServer/Storage/Internal/SqlServerTypeMappingSource.cs b/src/EFCore.SqlServer/Storage/Internal/SqlServerTypeMappingSource.cs
@@ -319,12 +319,21 @@ private RelationalTypeMapping FindRawMapping(RelationalTypeMappingInfo mappingIn
                         size = isFixedLength ? maxSize : (int?)null;
                     }
 
-                    return size == null
-                        ? isAnsi ? _variableLengthMaxAnsiString : _variableLengthMaxUnicodeString
-                        : new SqlServerStringTypeMapping(
-                            unicode: !isAnsi,
-                            size: size,
-                            fixedLength: isFixedLength);
+                    if (size == null)
+                    {
+                        return isAnsi
+                            ? isFixedLength
+                                ? _fixedLengthAnsiString
+                                : _variableLengthMaxAnsiString
+                            : isFixedLength
+                                ? _fixedLengthUnicodeString
+                                : _variableLengthMaxUnicodeString;
+                    }
+
+                    return new SqlServerStringTypeMapping(
+                        unicode: !isAnsi,
+                        size: size,
+                        fixedLength: isFixedLength);
                 }
 
                 if (clrType == typeof(byte[]))

diff --git a/test/EFCore.Design.Tests/Migrations/ModelSnapshotSqlServerTest.cs b/test/EFCore.Design.Tests/Migrations/ModelSnapshotSqlServerTest.cs
@@ -1651,7 +1651,7 @@ public virtual void Property_unicodeness_is_stored_in_snapshot()
         public virtual void Property_fixedlengthness_is_stored_in_snapshot()
         {
             Test(
-                builder => builder.Entity<EntityWithStringProperty>().Property<string>("Name").IsFixedLength(),
+                builder => builder.Entity<EntityWithStringProperty>().Property<string>("Name").IsFixedLength().HasMaxLength(100),
                 AddBoilerPlate(
                     GetHeading()
                     + @"
@@ -1663,8 +1663,9 @@ public virtual void Property_fixedlengthness_is_stored_in_snapshot()
                         .HasAnnotation(""SqlServer:ValueGenerationStrategy"", SqlServerValueGenerationStrategy.IdentityColumn);
 
                     b.Property<string>(""Name"")
-                        .HasColumnType(""nvarchar(max)"")
-                        .IsFixedLength(true);
+                        .HasColumnType(""nchar(100)"")
+                        .IsFixedLength(true)
+                        .HasMaxLength(100);
 
                     b.HasKey(""Id"");
 

diff --git a/test/EFCore.Relational.Specification.Tests/MigrationSqlGeneratorTestBase.cs b/test/EFCore.Relational.Specification.Tests/MigrationSqlGeneratorTestBase.cs
@@ -123,15 +123,16 @@ public virtual void AddColumnOperation_with_unicode_no_model()
         [ConditionalFact]
         public virtual void AddColumnOperation_with_fixed_length()
             => Generate(
-                modelBuilder => modelBuilder.Entity("Person").Property<string>("Name").IsFixedLength(),
+                modelBuilder => modelBuilder.Entity("Person").Property<string>("Name").HasMaxLength(100).IsFixedLength(),
                 new AddColumnOperation
                 {
                     Table = "Person",
                     Name = "Name",
                     ClrType = typeof(string),
                     IsUnicode = true,
                     IsNullable = true,
-                    IsFixedLength = true
+                    IsFixedLength = true,
+                    MaxLength = 100
                 });
 
         [ConditionalFact]
@@ -144,7 +145,8 @@ public virtual void AddColumnOperation_with_fixed_length_no_model()
                     ClrType = typeof(string),
                     IsUnicode = false,
                     IsNullable = true,
-                    IsFixedLength = true
+                    IsFixedLength = true,
+                    MaxLength = 100
                 });
 
         [ConditionalFact]

diff --git a/test/EFCore.Relational.Tests/Metadata/RelationalMetadataExtensionsTest.cs b/test/EFCore.Relational.Tests/Metadata/RelationalMetadataExtensionsTest.cs
@@ -21,7 +21,7 @@ public void Can_get_and_set_fixed_length()
                 .Property(e => e.Name)
                 .Metadata;
 
-            Assert.False(property.IsFixedLength());
+            Assert.Null(property.IsFixedLength());
 
             property.SetIsFixedLength(true);
 
@@ -30,6 +30,10 @@ public void Can_get_and_set_fixed_length()
             property.SetIsFixedLength(false);
 
             Assert.False(property.IsFixedLength());
+
+            property.SetIsFixedLength(null);
+
+            Assert.Null(property.IsFixedLength());
         }
 
         [ConditionalFact]

diff --git a/test/EFCore.Relational.Tests/Storage/RelationalTypeMapperTest.cs b/test/EFCore.Relational.Tests/Storage/RelationalTypeMapperTest.cs
@@ -159,32 +159,32 @@ private static IRelationalTypeMappingSource CreateTestTypeMapper()
             => CreateTestTypeMapper().FindMapping(property);
 
         [ConditionalFact]
-        public void String_key_with_max_length_is_picked_up_by_FK()
+        public void String_key_with_max_fixed_length_is_picked_up_by_FK()
         {
             var model = CreateModel();
             var mapper = CreateTestTypeMapper();
 
             Assert.Equal(
-                "just_string(200)",
+                "just_string_fixed(200)",
                 GetMapping(mapper, model.FindEntityType(typeof(MyRelatedType1)).FindProperty("Id")).StoreType);
 
             Assert.Equal(
-                "just_string(200)",
+                "just_string_fixed(200)",
                 GetMapping(mapper, model.FindEntityType(typeof(MyRelatedType2)).FindProperty("Relationship1Id")).StoreType);
         }
 
         [ConditionalFact]
-        public void Binary_key_with_max_length_is_picked_up_by_FK()
+        public void Binary_key_with_max_fixed_length_is_picked_up_by_FK()
         {
             var model = CreateModel();
             var mapper = CreateTestTypeMapper();
 
             Assert.Equal(
-                "just_binary(100)",
+                "just_binary_fixed(100)",
                 GetMapping(mapper, model.FindEntityType(typeof(MyRelatedType2)).FindProperty("Id")).StoreType);
 
             Assert.Equal(
-                "just_binary(100)",
+                "just_binary_fixed(100)",
                 GetMapping(mapper, model.FindEntityType(typeof(MyRelatedType3)).FindProperty("Relationship1Id")).StoreType);
         }
 
@@ -225,11 +225,11 @@ public void String_FK_max_length_is_preferred_if_specified()
             var mapper = CreateTestTypeMapper();
 
             Assert.Equal(
-                "just_string(200)",
+                "just_string_fixed(200)",
                 GetMapping(mapper, model.FindEntityType(typeof(MyRelatedType1)).FindProperty("Id")).StoreType);
 
             Assert.Equal(
-                "just_string(787)",
+                "just_string_fixed(787)",
                 GetMapping(mapper, model.FindEntityType(typeof(MyRelatedType2)).FindProperty("Relationship2Id")).StoreType);
         }
 
@@ -240,11 +240,11 @@ public void Binary_FK_max_length_is_preferred_if_specified()
             var mapper = CreateTestTypeMapper();
 
             Assert.Equal(
-                "just_binary(100)",
+                "just_binary_fixed(100)",
                 GetMapping(mapper, model.FindEntityType(typeof(MyRelatedType2)).FindProperty("Id")).StoreType);
 
             Assert.Equal(
-                "just_binary(767)",
+                "just_binary_fixed(767)",
                 GetMapping(mapper, model.FindEntityType(typeof(MyRelatedType3)).FindProperty("Relationship2Id")).StoreType);
         }
 

diff --git a/test/EFCore.Relational.Tests/Storage/RelationalTypeMapperTestBase.cs b/test/EFCore.Relational.Tests/Storage/RelationalTypeMapperTestBase.cs
@@ -15,9 +15,9 @@ protected IMutableModel CreateModel()
             var builder = CreateModelBuilder();
 
             builder.Entity<MyType>().Property(e => e.Id).HasColumnType("money");
-            builder.Entity<MyRelatedType1>().Property(e => e.Id).HasMaxLength(200);
+            builder.Entity<MyRelatedType1>().Property(e => e.Id).HasMaxLength(200).IsFixedLength();
             builder.Entity<MyRelatedType1>().Property(e => e.Relationship2Id).HasColumnType("dec(6,1)");
-            builder.Entity<MyRelatedType2>().Property(e => e.Id).HasMaxLength(100);
+            builder.Entity<MyRelatedType2>().Property(e => e.Id).HasMaxLength(100).IsFixedLength();
             builder.Entity<MyRelatedType2>().Property(e => e.Relationship2Id).HasMaxLength(787);
             builder.Entity<MyRelatedType3>().Property(e => e.Id).IsUnicode(false);
             builder.Entity<MyRelatedType3>().Property(e => e.Relationship2Id).HasMaxLength(767);

diff --git a/test/EFCore.Relational.Tests/TestUtilities/TestRelationalTypeMappingSource.cs b/test/EFCore.Relational.Tests/TestUtilities/TestRelationalTypeMappingSource.cs
@@ -188,9 +188,10 @@ protected override RelationalTypeMapping FindMapping(in RelationalTypeMappingInf
                     }
 
                     var size = mappingInfo.Size ?? (mappingInfo.IsKeyOrIndex ? (int?)900 : null);
+                    var isFixedLength = mappingInfo.IsFixedLength == true;
 
                     return new ByteArrayTypeMapping(
-                        storeTypeName ?? "just_binary(" + (size == null ? "max" : size.ToString()) + ")",
+                        storeTypeName ?? (isFixedLength ? "just_binary_fixed(" : "just_binary(") + (size == null ? "max" : size.ToString()) + ")",
                         DbType.Binary,
                         size);
                 }