From 0a2363ed97bc7ec2e0960ed65f5467d96d107ce8 Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Tue, 2 Apr 2024 15:34:18 +0100 Subject: [PATCH] Stop eagerly throwing when setting value generation strategy Fixes #33413 Because value converters are now supported for generated properties, but the converter may not be configured yet. --- .../SqlServerPropertyBuilderExtensions.cs | 20 +++---- .../Extensions/SqlServerPropertyExtensions.cs | 46 ++-------------- .../Internal/SqlServerModelValidator.cs | 33 ++++++++++++ .../SqlServerModelValidatorTest.cs | 49 +++++++++++++++++ .../SqlServerMetadataBuilderExtensionsTest.cs | 54 ------------------- .../SqlServerMetadataExtensionsTest.cs | 34 ------------ 6 files changed, 94 insertions(+), 142 deletions(-) diff --git a/src/EFCore.SqlServer/Extensions/SqlServerPropertyBuilderExtensions.cs b/src/EFCore.SqlServer/Extensions/SqlServerPropertyBuilderExtensions.cs index f959733c7e3..2f92f859692 100644 --- a/src/EFCore.SqlServer/Extensions/SqlServerPropertyBuilderExtensions.cs +++ b/src/EFCore.SqlServer/Extensions/SqlServerPropertyBuilderExtensions.cs @@ -695,10 +695,8 @@ public static bool CanSetValueGenerationStrategy( this IConventionPropertyBuilder propertyBuilder, SqlServerValueGenerationStrategy? valueGenerationStrategy, bool fromDataAnnotation = false) - => (valueGenerationStrategy == null - || SqlServerPropertyExtensions.IsCompatibleWithValueGeneration(propertyBuilder.Metadata)) - && propertyBuilder.CanSetAnnotation( - SqlServerAnnotationNames.ValueGenerationStrategy, valueGenerationStrategy, fromDataAnnotation); + => propertyBuilder.CanSetAnnotation( + SqlServerAnnotationNames.ValueGenerationStrategy, valueGenerationStrategy, fromDataAnnotation); /// /// Returns a value indicating whether the given value can be set as the value generation strategy for a particular table. @@ -718,14 +716,12 @@ public static bool CanSetValueGenerationStrategy( SqlServerValueGenerationStrategy? valueGenerationStrategy, in StoreObjectIdentifier storeObject, bool fromDataAnnotation = false) - => (valueGenerationStrategy == null - || SqlServerPropertyExtensions.IsCompatibleWithValueGeneration(propertyBuilder.Metadata)) - && (propertyBuilder.Metadata.FindOverrides(storeObject)?.Builder - .CanSetAnnotation( - SqlServerAnnotationNames.ValueGenerationStrategy, - valueGenerationStrategy, - fromDataAnnotation) - ?? true); + => propertyBuilder.Metadata.FindOverrides(storeObject)?.Builder + .CanSetAnnotation( + SqlServerAnnotationNames.ValueGenerationStrategy, + valueGenerationStrategy, + fromDataAnnotation) + ?? true; /// /// Configures whether the property's column is created as sparse when targeting SQL Server. diff --git a/src/EFCore.SqlServer/Extensions/SqlServerPropertyExtensions.cs b/src/EFCore.SqlServer/Extensions/SqlServerPropertyExtensions.cs index 079a85b724e..75cea6b8c53 100644 --- a/src/EFCore.SqlServer/Extensions/SqlServerPropertyExtensions.cs +++ b/src/EFCore.SqlServer/Extensions/SqlServerPropertyExtensions.cs @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using Microsoft.EntityFrameworkCore.SqlServer.Internal; using Microsoft.EntityFrameworkCore.SqlServer.Metadata.Internal; // ReSharper disable once CheckNamespace @@ -852,9 +851,7 @@ private static SqlServerValueGenerationStrategy GetDefaultValueGenerationStrateg public static void SetValueGenerationStrategy( this IMutableProperty property, SqlServerValueGenerationStrategy? value) - => property.SetOrRemoveAnnotation( - SqlServerAnnotationNames.ValueGenerationStrategy, - CheckValueGenerationStrategy(property, value)); + => property.SetOrRemoveAnnotation(SqlServerAnnotationNames.ValueGenerationStrategy, value); /// /// Sets the to use for the property. @@ -868,9 +865,7 @@ public static void SetValueGenerationStrategy( SqlServerValueGenerationStrategy? value, bool fromDataAnnotation = false) => (SqlServerValueGenerationStrategy?)property.SetOrRemoveAnnotation( - SqlServerAnnotationNames.ValueGenerationStrategy, - CheckValueGenerationStrategy(property, value), - fromDataAnnotation)?.Value; + SqlServerAnnotationNames.ValueGenerationStrategy, value, fromDataAnnotation)?.Value; /// /// Sets the to use for the property for a particular table. @@ -909,9 +904,7 @@ public static void SetValueGenerationStrategy( public static void SetValueGenerationStrategy( this IMutableRelationalPropertyOverrides overrides, SqlServerValueGenerationStrategy? value) - => overrides.SetOrRemoveAnnotation( - SqlServerAnnotationNames.ValueGenerationStrategy, - CheckValueGenerationStrategy(overrides.Property, value)); + => overrides.SetOrRemoveAnnotation(SqlServerAnnotationNames.ValueGenerationStrategy, value); /// /// Sets the to use for the property for a particular table. @@ -925,39 +918,8 @@ public static void SetValueGenerationStrategy( SqlServerValueGenerationStrategy? value, bool fromDataAnnotation = false) => (SqlServerValueGenerationStrategy?)overrides.SetOrRemoveAnnotation( - SqlServerAnnotationNames.ValueGenerationStrategy, - CheckValueGenerationStrategy(overrides.Property, value), - fromDataAnnotation)?.Value; - - private static SqlServerValueGenerationStrategy? CheckValueGenerationStrategy( - IReadOnlyProperty property, - SqlServerValueGenerationStrategy? value) - { - if (value == null) - { - return null; - } + SqlServerAnnotationNames.ValueGenerationStrategy, value, fromDataAnnotation)?.Value; - var propertyType = property.ClrType; - - if (value == SqlServerValueGenerationStrategy.IdentityColumn - && !IsCompatibleWithValueGeneration(property)) - { - throw new ArgumentException( - SqlServerStrings.IdentityBadType( - property.Name, property.DeclaringType.DisplayName(), propertyType.ShortDisplayName())); - } - - if (value is SqlServerValueGenerationStrategy.SequenceHiLo or SqlServerValueGenerationStrategy.Sequence - && !IsCompatibleWithValueGeneration(property)) - { - throw new ArgumentException( - SqlServerStrings.SequenceBadType( - property.Name, property.DeclaringType.DisplayName(), propertyType.ShortDisplayName())); - } - - return value; - } /// /// Returns the for the . diff --git a/src/EFCore.SqlServer/Infrastructure/Internal/SqlServerModelValidator.cs b/src/EFCore.SqlServer/Infrastructure/Internal/SqlServerModelValidator.cs index a3ef71c4494..68cc2d344c5 100644 --- a/src/EFCore.SqlServer/Infrastructure/Internal/SqlServerModelValidator.cs +++ b/src/EFCore.SqlServer/Infrastructure/Internal/SqlServerModelValidator.cs @@ -135,6 +135,39 @@ protected override void ValidateValueGeneration( } } + /// + protected override void ValidateTypeMappings( + IModel model, + IDiagnosticsLogger logger) + { + base.ValidateTypeMappings(model, logger); + + foreach (var entityType in model.GetEntityTypes()) + { + foreach (var property in entityType.GetFlattenedDeclaredProperties()) + { + var strategy = property.GetValueGenerationStrategy(); + var propertyType = property.ClrType; + + if (strategy == SqlServerValueGenerationStrategy.IdentityColumn + && !SqlServerPropertyExtensions.IsCompatibleWithValueGeneration(property)) + { + throw new InvalidOperationException( + SqlServerStrings.IdentityBadType( + property.Name, property.DeclaringType.DisplayName(), propertyType.ShortDisplayName())); + } + + if (strategy is SqlServerValueGenerationStrategy.SequenceHiLo or SqlServerValueGenerationStrategy.Sequence + && !SqlServerPropertyExtensions.IsCompatibleWithValueGeneration(property)) + { + throw new InvalidOperationException( + SqlServerStrings.SequenceBadType( + property.Name, property.DeclaringType.DisplayName(), propertyType.ShortDisplayName())); + } + } + } + } + /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to /// the same compatibility standards as public APIs. It may be changed or removed without notice in diff --git a/test/EFCore.SqlServer.Tests/Infrastructure/SqlServerModelValidatorTest.cs b/test/EFCore.SqlServer.Tests/Infrastructure/SqlServerModelValidatorTest.cs index a2010b52d42..b75a5f95e3c 100644 --- a/test/EFCore.SqlServer.Tests/Infrastructure/SqlServerModelValidatorTest.cs +++ b/test/EFCore.SqlServer.Tests/Infrastructure/SqlServerModelValidatorTest.cs @@ -72,6 +72,55 @@ public override void Detects_store_generated_PK_in_TPC() Assert.Equal(SqlServerValueGenerationStrategy.Sequence, keyProperty.GetValueGenerationStrategy()); } + [ConditionalFact] + public virtual void Throws_for_identity_on_bad_type() + { + var modelBuilder = CreateConventionModelBuilder(); + + modelBuilder.Entity( + b => + { + b.Property(e => e.Name).UseIdentityColumn(); + }); + + VerifyError( + SqlServerStrings.IdentityBadType(nameof(LivingBeing.Name), nameof(Animal), "string"), + modelBuilder); + } + + [ConditionalFact] + public virtual void Throws_for_sequence_on_bad_type() + { + var modelBuilder = CreateConventionModelBuilder(); + + modelBuilder.Entity( + b => + { + b.Property(e => e.Name).UseSequence(); + }); + + VerifyError( + SqlServerStrings.SequenceBadType(nameof(LivingBeing.Name), nameof(Animal), "string"), + modelBuilder); + } + + + [ConditionalFact] + public virtual void Throws_for_sequence_HiLo_on_bad_type() + { + var modelBuilder = CreateConventionModelBuilder(); + + modelBuilder.Entity( + b => + { + b.Property(e => e.Name).UseHiLo(); + }); + + VerifyError( + SqlServerStrings.SequenceBadType(nameof(LivingBeing.Name), nameof(Animal), "string"), + modelBuilder); + } + [ConditionalFact] public virtual void Passes_for_duplicate_column_names_within_hierarchy_with_identity() { diff --git a/test/EFCore.SqlServer.Tests/Metadata/SqlServerMetadataBuilderExtensionsTest.cs b/test/EFCore.SqlServer.Tests/Metadata/SqlServerMetadataBuilderExtensionsTest.cs index 11aa809d13b..f138a6465f4 100644 --- a/test/EFCore.SqlServer.Tests/Metadata/SqlServerMetadataBuilderExtensionsTest.cs +++ b/test/EFCore.SqlServer.Tests/Metadata/SqlServerMetadataBuilderExtensionsTest.cs @@ -305,60 +305,6 @@ public void Can_access_property() Assert.Null(propertyBuilder.Metadata.GetHiLoSequenceNameConfigurationSource()); } - [ConditionalFact] - public void Throws_setting_sequence_generation_for_invalid_type() - { - var propertyBuilder = CreateBuilder() - .Entity(typeof(Splot)) - .Property(typeof(string), "Name"); - - Assert.Equal( - SqlServerStrings.SequenceBadType("Name", nameof(Splot), "string"), - Assert.Throws( - () => propertyBuilder.HasValueGenerationStrategy(SqlServerValueGenerationStrategy.SequenceHiLo)).Message); - - Assert.Equal( - SqlServerStrings.SequenceBadType("Name", nameof(Splot), "string"), - Assert.Throws( - () => new PropertyBuilder((IMutableProperty)propertyBuilder.Metadata).UseHiLo()).Message); - } - - [ConditionalFact] - public void Throws_setting_key_sequence_generation_for_invalid_type() - { - var propertyBuilder = CreateBuilder() - .Entity(typeof(Splot)) - .Property(typeof(string), "Name"); - - Assert.Equal( - SqlServerStrings.SequenceBadType("Name", nameof(Splot), "string"), - Assert.Throws( - () => propertyBuilder.HasValueGenerationStrategy(SqlServerValueGenerationStrategy.Sequence)).Message); - - Assert.Equal( - SqlServerStrings.SequenceBadType("Name", nameof(Splot), "string"), - Assert.Throws( - () => new PropertyBuilder((IMutableProperty)propertyBuilder.Metadata).UseSequence()).Message); - } - - [ConditionalFact] - public void Throws_setting_identity_generation_for_invalid_type_only_with_explicit() - { - var propertyBuilder = CreateBuilder() - .Entity(typeof(Splot)) - .Property(typeof(string), "Name"); - - Assert.Equal( - SqlServerStrings.IdentityBadType("Name", nameof(Splot), "string"), - Assert.Throws( - () => propertyBuilder.HasValueGenerationStrategy(SqlServerValueGenerationStrategy.IdentityColumn)).Message); - - Assert.Equal( - SqlServerStrings.IdentityBadType("Name", nameof(Splot), "string"), - Assert.Throws( - () => new PropertyBuilder((IMutableProperty)propertyBuilder.Metadata).UseIdentityColumn()).Message); - } - [ConditionalFact] public void Can_access_key() { diff --git a/test/EFCore.SqlServer.Tests/Metadata/SqlServerMetadataExtensionsTest.cs b/test/EFCore.SqlServer.Tests/Metadata/SqlServerMetadataExtensionsTest.cs index f2873886964..b49eb7fe9d6 100644 --- a/test/EFCore.SqlServer.Tests/Metadata/SqlServerMetadataExtensionsTest.cs +++ b/test/EFCore.SqlServer.Tests/Metadata/SqlServerMetadataExtensionsTest.cs @@ -1,8 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using Microsoft.EntityFrameworkCore.SqlServer.Internal; - // ReSharper disable InconsistentNaming namespace Microsoft.EntityFrameworkCore.Metadata; @@ -281,38 +279,6 @@ public void Can_get_and_set_value_generation_on_nullable_property() Assert.Equal(SqlServerValueGenerationStrategy.IdentityColumn, property.GetValueGenerationStrategy()); } - [ConditionalFact] - public void Throws_setting_sequence_generation_for_invalid_type() - { - var modelBuilder = GetModelBuilder(); - - var property = modelBuilder - .Entity() - .Property(e => e.Name) - .Metadata; - - Assert.Equal( - SqlServerStrings.SequenceBadType("Name", nameof(Customer), "string"), - Assert.Throws( - () => property.SetValueGenerationStrategy(SqlServerValueGenerationStrategy.SequenceHiLo)).Message); - } - - [ConditionalFact] - public void Throws_setting_identity_generation_for_invalid_type() - { - var modelBuilder = GetModelBuilder(); - - var property = modelBuilder - .Entity() - .Property(e => e.Name) - .Metadata; - - Assert.Equal( - SqlServerStrings.IdentityBadType("Name", nameof(Customer), "string"), - Assert.Throws( - () => property.SetValueGenerationStrategy(SqlServerValueGenerationStrategy.IdentityColumn)).Message); - } - [ConditionalFact] public void Can_get_and_set_sequence_name_on_property() {