Skip to content

Commit

Permalink
Stop eagerly throwing when setting value generation strategy (#3147)
Browse files Browse the repository at this point in the history
Closes #3141
  • Loading branch information
roji committed Apr 3, 2024
1 parent 18008c9 commit ac91e9a
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 74 deletions.
Expand Up @@ -425,6 +425,26 @@ public static PropertyBuilder UseIdentityByDefaultColumn(this PropertyBuilder pr
return null;
}

/// <summary>
/// Returns a value indicating whether the given value can be set as the value generation strategy for a particular table.
/// </summary>
/// <param name="propertyBuilder">The builder for the property being configured.</param>
/// <param name="valueGenerationStrategy">The value generation strategy.</param>
/// <param name="storeObject">The table identifier.</param>
/// <param name="fromDataAnnotation">Indicates whether the configuration was specified using a data annotation.</param>
/// <returns><see langword="true" /> if the given value can be set as the default value generation strategy.</returns>
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;

/// <summary>
/// Returns a value indicating whether the given value can be set as the value generation strategy.
/// </summary>
Expand All @@ -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

Expand Down
Expand Up @@ -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);

/// <summary>
/// Sets the <see cref="NpgsqlValueGenerationStrategy" /> to use for the property.
Expand All @@ -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;

/// <summary>
/// Sets the <see cref="NpgsqlValueGenerationStrategy" /> to use for the property for a particular table.
Expand Down Expand Up @@ -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);

/// <summary>
/// Sets the <see cref="NpgsqlValueGenerationStrategy" /> to use for the property for a particular table.
Expand All @@ -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;

/// <summary>
/// Returns the <see cref="ConfigurationSource" /> for the <see cref="NpgsqlValueGenerationStrategy" />.
Expand Down
49 changes: 49 additions & 0 deletions src/EFCore.PG/Infrastructure/Internal/NpgsqlModelValidator.cs
Expand Up @@ -103,6 +103,55 @@ protected virtual void ValidateIdentityVersionCompatibility(IModel model)
}
}

/// <inheritdoc/>
protected override void ValidateTypeMappings(
IModel model,
IDiagnosticsLogger<DbLoggerCategory.Model.Validation> 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();
}
}
}
}

/// <summary>
/// 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
Expand Down
Expand Up @@ -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<ArgumentException>(
() => propertyBuilder.HasValueGenerationStrategy(NpgsqlValueGenerationStrategy.SequenceHiLo)).Message);

Assert.Equal(
NpgsqlStrings.SequenceBadType("Name", nameof(Splot), "string"),
Assert.Throws<ArgumentException>(
() => new PropertyBuilder((IMutableProperty)propertyBuilder.Metadata).UseHiLo()).Message);
}

[ConditionalFact]
public void Can_access_index()
{
Expand Down

0 comments on commit ac91e9a

Please sign in to comment.