From ac91e9a0cbfb359fabca58f621c9c08b9720e5b1 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Wed, 3 Apr 2024 12:51:09 +0200 Subject: [PATCH] Stop eagerly throwing when setting value generation strategy (#3147) Closes #3141 --- .../NpgsqlPropertyBuilderExtensions.cs | 30 ++++++++--- .../NpgsqlPropertyExtensions.cs | 54 +++---------------- .../Internal/NpgsqlModelValidator.cs | 49 +++++++++++++++++ .../NpgsqlMetadataBuilderExtensionsTest.cs | 18 ------- 4 files changed, 77 insertions(+), 74 deletions(-) diff --git a/src/EFCore.PG/Extensions/BuilderExtensions/NpgsqlPropertyBuilderExtensions.cs b/src/EFCore.PG/Extensions/BuilderExtensions/NpgsqlPropertyBuilderExtensions.cs index 6c4362b7b..2698800d3 100644 --- a/src/EFCore.PG/Extensions/BuilderExtensions/NpgsqlPropertyBuilderExtensions.cs +++ b/src/EFCore.PG/Extensions/BuilderExtensions/NpgsqlPropertyBuilderExtensions.cs @@ -425,6 +425,26 @@ public static PropertyBuilder UseIdentityByDefaultColumn(this PropertyBuilder pr return null; } + /// + /// Returns a value indicating whether the given value can be set as the value generation strategy for a particular table. + /// + /// The builder for the property being configured. + /// The value generation strategy. + /// The table identifier. + /// Indicates whether the configuration was specified using a data annotation. + /// if the given value can be set as the default value generation strategy. + public static bool CanSetValueGenerationStrategy( + this IConventionPropertyBuilder propertyBuilder, + NpgsqlValueGenerationStrategy? valueGenerationStrategy, + in StoreObjectIdentifier storeObject, + bool fromDataAnnotation = false) + => propertyBuilder.Metadata.FindOverrides(storeObject)?.Builder + .CanSetAnnotation( + NpgsqlAnnotationNames.ValueGenerationStrategy, + valueGenerationStrategy, + fromDataAnnotation) + ?? true; + /// /// Returns a value indicating whether the given value can be set as the value generation strategy. /// @@ -436,14 +456,8 @@ public static PropertyBuilder UseIdentityByDefaultColumn(this PropertyBuilder pr this IConventionPropertyBuilder propertyBuilder, NpgsqlValueGenerationStrategy? valueGenerationStrategy, bool fromDataAnnotation = false) - { - Check.NotNull(propertyBuilder, nameof(propertyBuilder)); - - return (valueGenerationStrategy is null - || NpgsqlPropertyExtensions.IsCompatibleWithValueGeneration(propertyBuilder.Metadata)) - && propertyBuilder.CanSetAnnotation( - NpgsqlAnnotationNames.ValueGenerationStrategy, valueGenerationStrategy, fromDataAnnotation); - } + => propertyBuilder.CanSetAnnotation( + NpgsqlAnnotationNames.ValueGenerationStrategy, valueGenerationStrategy, fromDataAnnotation); #endregion General value generation strategy diff --git a/src/EFCore.PG/Extensions/MetadataExtensions/NpgsqlPropertyExtensions.cs b/src/EFCore.PG/Extensions/MetadataExtensions/NpgsqlPropertyExtensions.cs index da331f59f..af88adbf5 100644 --- a/src/EFCore.PG/Extensions/MetadataExtensions/NpgsqlPropertyExtensions.cs +++ b/src/EFCore.PG/Extensions/MetadataExtensions/NpgsqlPropertyExtensions.cs @@ -589,11 +589,7 @@ private static NpgsqlValueGenerationStrategy GetDefaultValueGenerationStrategy(I public static void SetValueGenerationStrategy( this IMutableProperty property, NpgsqlValueGenerationStrategy? value) - { - CheckValueGenerationStrategy(property, value); - - property.SetOrRemoveAnnotation(NpgsqlAnnotationNames.ValueGenerationStrategy, value); - } + => property.SetOrRemoveAnnotation(NpgsqlAnnotationNames.ValueGenerationStrategy, value); /// /// Sets the to use for the property. @@ -605,13 +601,8 @@ private static NpgsqlValueGenerationStrategy GetDefaultValueGenerationStrategy(I this IConventionProperty property, NpgsqlValueGenerationStrategy? value, bool fromDataAnnotation = false) - { - CheckValueGenerationStrategy(property, value); - - return (NpgsqlValueGenerationStrategy?)property.SetOrRemoveAnnotation( - NpgsqlAnnotationNames.ValueGenerationStrategy, value, fromDataAnnotation) - ?.Value; - } + => (NpgsqlValueGenerationStrategy?)property.SetOrRemoveAnnotation( + NpgsqlAnnotationNames.ValueGenerationStrategy, value, fromDataAnnotation)?.Value; /// /// Sets the to use for the property for a particular table. @@ -650,11 +641,7 @@ private static NpgsqlValueGenerationStrategy GetDefaultValueGenerationStrategy(I public static void SetValueGenerationStrategy( this IMutableRelationalPropertyOverrides overrides, NpgsqlValueGenerationStrategy? value) - { - CheckValueGenerationStrategy(overrides.Property, value); - - overrides.SetOrRemoveAnnotation(NpgsqlAnnotationNames.ValueGenerationStrategy, value); - } + => overrides.SetOrRemoveAnnotation(NpgsqlAnnotationNames.ValueGenerationStrategy, value); /// /// Sets the to use for the property for a particular table. @@ -667,37 +654,8 @@ private static NpgsqlValueGenerationStrategy GetDefaultValueGenerationStrategy(I this IConventionRelationalPropertyOverrides overrides, NpgsqlValueGenerationStrategy? value, bool fromDataAnnotation = false) - { - CheckValueGenerationStrategy(overrides.Property, value); - - return (NpgsqlValueGenerationStrategy?)overrides.SetOrRemoveAnnotation( - NpgsqlAnnotationNames.ValueGenerationStrategy, value, fromDataAnnotation) - ?.Value; - } - - private static void CheckValueGenerationStrategy(IReadOnlyProperty property, NpgsqlValueGenerationStrategy? value) - { - if (value is not null) - { - var propertyType = property.ClrType; - - if ((value is NpgsqlValueGenerationStrategy.IdentityAlwaysColumn or NpgsqlValueGenerationStrategy.IdentityByDefaultColumn) - && !IsCompatibleWithValueGeneration(property)) - { - throw new ArgumentException( - NpgsqlStrings.IdentityBadType( - property.Name, property.DeclaringType.DisplayName(), propertyType.ShortDisplayName())); - } - - if (value is NpgsqlValueGenerationStrategy.SerialColumn or NpgsqlValueGenerationStrategy.SequenceHiLo - && !IsCompatibleWithValueGeneration(property)) - { - throw new ArgumentException( - NpgsqlStrings.SequenceBadType( - property.Name, property.DeclaringType.DisplayName(), propertyType.ShortDisplayName())); - } - } - } + => (NpgsqlValueGenerationStrategy?)overrides.SetOrRemoveAnnotation( + NpgsqlAnnotationNames.ValueGenerationStrategy, value, fromDataAnnotation)?.Value; /// /// Returns the for the . diff --git a/src/EFCore.PG/Infrastructure/Internal/NpgsqlModelValidator.cs b/src/EFCore.PG/Infrastructure/Internal/NpgsqlModelValidator.cs index 14f8dcbf2..6942a4f1e 100644 --- a/src/EFCore.PG/Infrastructure/Internal/NpgsqlModelValidator.cs +++ b/src/EFCore.PG/Infrastructure/Internal/NpgsqlModelValidator.cs @@ -103,6 +103,55 @@ protected virtual void ValidateIdentityVersionCompatibility(IModel model) } } + /// + 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; + + switch (strategy) + { + case NpgsqlValueGenerationStrategy.None: + break; + + case NpgsqlValueGenerationStrategy.IdentityByDefaultColumn: + case NpgsqlValueGenerationStrategy.IdentityAlwaysColumn: + if (!NpgsqlPropertyExtensions.IsCompatibleWithValueGeneration(property)) + { + throw new InvalidOperationException( + NpgsqlStrings.IdentityBadType( + property.Name, property.DeclaringType.DisplayName(), propertyType.ShortDisplayName())); + } + + break; + + case NpgsqlValueGenerationStrategy.SequenceHiLo: + case NpgsqlValueGenerationStrategy.Sequence: + case NpgsqlValueGenerationStrategy.SerialColumn: + if (!NpgsqlPropertyExtensions.IsCompatibleWithValueGeneration(property)) + { + throw new InvalidOperationException( + NpgsqlStrings.SequenceBadType( + property.Name, property.DeclaringType.DisplayName(), propertyType.ShortDisplayName())); + } + + break; + + default: + throw new UnreachableException(); + } + } + } + } + /// /// 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.PG.Tests/Metadata/NpgsqlMetadataBuilderExtensionsTest.cs b/test/EFCore.PG.Tests/Metadata/NpgsqlMetadataBuilderExtensionsTest.cs index 287bbcd18..78e23abfb 100644 --- a/test/EFCore.PG.Tests/Metadata/NpgsqlMetadataBuilderExtensionsTest.cs +++ b/test/EFCore.PG.Tests/Metadata/NpgsqlMetadataBuilderExtensionsTest.cs @@ -75,24 +75,6 @@ public void Can_access_property() a => a.Name.StartsWith(NpgsqlAnnotationNames.Prefix, StringComparison.Ordinal))); } - [ConditionalFact] - public void Throws_setting_sequence_generation_for_invalid_type() - { - var propertyBuilder = CreateBuilder() - .Entity(typeof(Splot)) - .Property(typeof(string), "Name"); - - Assert.Equal( - NpgsqlStrings.SequenceBadType("Name", nameof(Splot), "string"), - Assert.Throws( - () => propertyBuilder.HasValueGenerationStrategy(NpgsqlValueGenerationStrategy.SequenceHiLo)).Message); - - Assert.Equal( - NpgsqlStrings.SequenceBadType("Name", nameof(Splot), "string"), - Assert.Throws( - () => new PropertyBuilder((IMutableProperty)propertyBuilder.Metadata).UseHiLo()).Message); - } - [ConditionalFact] public void Can_access_index() {