Skip to content

Commit

Permalink
Stop throwing eagerly when looking up a ValueGenerator
Browse files Browse the repository at this point in the history
Fixes #32892

The issue was introduced by the fix for stable/unstable values. In that fix, we were missing that a value generator exists and influences whether the generated value is considered stable or not. However, some types, most notably bools, don't have a default generator. This is fine, it just means that there isn't a generator to use.

I removed the exception entirely because we never now throw if we can't find a generator--it's always okay. This is technically a breaking change, and people do create their own selectors, so we should probably doc it.
  • Loading branch information
ajcvickers committed Jan 26, 2024
1 parent 327fbc3 commit eee787c
Show file tree
Hide file tree
Showing 19 changed files with 243 additions and 86 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public InMemoryValueGeneratorSelector(
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public override ValueGenerator Select(IProperty property, ITypeBase typeBase)
public override ValueGenerator? Select(IProperty property, ITypeBase typeBase)
=> property.GetValueGeneratorFactory() == null
&& property.ClrType.IsInteger()
&& property.ClrType.UnwrapNullableType() != typeof(char)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public SqlServerValueGeneratorSelector(
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public override ValueGenerator Select(IProperty property, ITypeBase typeBase)
public override ValueGenerator? Select(IProperty property, ITypeBase typeBase)
{
if (property.GetValueGeneratorFactory() != null
|| property.GetValueGenerationStrategy() != SqlServerValueGenerationStrategy.SequenceHiLo)
Expand Down
38 changes: 23 additions & 15 deletions src/EFCore/ChangeTracking/Internal/ValueGenerationManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -97,16 +97,20 @@ public virtual bool Generate(InternalEntityEntry entry, bool includePrimaryKey =
foreach (var property in entry.EntityType.GetValueGeneratingProperties())
{
var valueGenerator = GetValueGenerator(property);
if (valueGenerator.GeneratesStableValues)
if (valueGenerator != null)
{
hasStableValues = true;
}
else
{
hasNonStableValues = true;
if (valueGenerator.GeneratesStableValues)
{
hasStableValues = true;
}
else
{
hasNonStableValues = true;
}
}

if (entry.HasExplicitValue(property)
if (valueGenerator == null
|| entry.HasExplicitValue(property)
|| (!includePrimaryKey
&& property.IsPrimaryKey()))
{
Expand Down Expand Up @@ -155,16 +159,20 @@ public virtual async Task<bool> GenerateAsync(
foreach (var property in entry.EntityType.GetValueGeneratingProperties())
{
var valueGenerator = GetValueGenerator(property);
if (valueGenerator.GeneratesStableValues)
{
hasStableValues = true;
}
else
if (valueGenerator != null)
{
hasNonStableValues = true;
if (valueGenerator.GeneratesStableValues)
{
hasStableValues = true;
}
else
{
hasNonStableValues = true;
}
}

if (entry.HasExplicitValue(property)
if (valueGenerator == null
|| entry.HasExplicitValue(property)
|| (!includePrimaryKey
&& property.IsPrimaryKey()))
{
Expand All @@ -188,7 +196,7 @@ public virtual async Task<bool> GenerateAsync(
return hasStableValues && !hasNonStableValues;
}

private ValueGenerator GetValueGenerator(IProperty property)
private ValueGenerator? GetValueGenerator(IProperty property)
=> _valueGeneratorSelector.Select(property, property.DeclaringType);

private static void SetGeneratedValue(InternalEntityEntry entry, IProperty property, object? generatedValue, bool isTemporary)
Expand Down
8 changes: 0 additions & 8 deletions src/EFCore/Properties/CoreStrings.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions src/EFCore/Properties/CoreStrings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -1276,9 +1276,6 @@
<data name="NotQueryingEnumerable" xml:space="preserve">
<value>The given 'IQueryable' does not support generation of query strings.</value>
</data>
<data name="NoValueGenerator" xml:space="preserve">
<value>The property '{1_entityType}.{0_property}' does not have a value set and no value generator is available for properties of type '{propertyType}'. Either set a value for the property before adding the entity or configure a value generator for properties of type '{propertyType}' in 'OnModelCreating'.</value>
</data>
<data name="NullableKey" xml:space="preserve">
<value>A key on entity type '{entityType}' cannot contain the property '{property}' because it is nullable/optional. All properties on which a key is declared must be marked as non-nullable/required.</value>
</data>
Expand Down
4 changes: 2 additions & 2 deletions src/EFCore/ValueGeneration/IValueGeneratorCache.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ public interface IValueGeneratorCache
/// </param>
/// <param name="factory">Factory to create a new value generator if one is not present in the cache.</param>
/// <returns>The existing or newly created value generator.</returns>
ValueGenerator GetOrAdd(
ValueGenerator? GetOrAdd(
IProperty property,
ITypeBase typeBase,
Func<IProperty, ITypeBase, ValueGenerator> factory);
Func<IProperty, ITypeBase, ValueGenerator?> factory);
}
2 changes: 1 addition & 1 deletion src/EFCore/ValueGeneration/IValueGeneratorSelector.cs
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,5 @@ public interface IValueGeneratorSelector
/// this type may be different from the declaring type for <paramref name="property" />
/// </param>
/// <returns>The value generator to be used.</returns>
ValueGenerator Select(IProperty property, ITypeBase typeBase);
ValueGenerator? Select(IProperty property, ITypeBase typeBase);
}
6 changes: 3 additions & 3 deletions src/EFCore/ValueGeneration/ValueGeneratorCache.cs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public ValueGeneratorCache(ValueGeneratorCacheDependencies dependencies)
/// </summary>
protected virtual ValueGeneratorCacheDependencies Dependencies { get; }

private readonly ConcurrentDictionary<CacheKey, ValueGenerator> _cache = new();
private readonly ConcurrentDictionary<CacheKey, ValueGenerator?> _cache = new();

private readonly struct CacheKey : IEquatable<CacheKey>
{
Expand Down Expand Up @@ -79,10 +79,10 @@ public override int GetHashCode()
/// </param>
/// <param name="factory">Factory to create a new value generator if one is not present in the cache.</param>
/// <returns>The existing or newly created value generator.</returns>
public virtual ValueGenerator GetOrAdd(
public virtual ValueGenerator? GetOrAdd(
IProperty property,
ITypeBase typeBase,
Func<IProperty, ITypeBase, ValueGenerator> factory)
Func<IProperty, ITypeBase, ValueGenerator?> factory)
=> _cache.GetOrAdd(
new CacheKey(property, typeBase), static (ck, p) => p.factory(p.property, p.typeBase), (factory, typeBase, property));
}
11 changes: 5 additions & 6 deletions src/EFCore/ValueGeneration/ValueGeneratorSelector.cs
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ public ValueGeneratorSelector(ValueGeneratorSelectorDependencies dependencies)
/// The entity type that the value generator will be used for. When called on inherited properties on derived entity types,
/// this entity type may be different from the declared entity type on <paramref name="property" />
/// </param>
/// <returns>The value generator to be used.</returns>
public virtual ValueGenerator Select(IProperty property, ITypeBase typeBase)
/// <returns>The value generator to be used, or <see langword="null"/> if none is available.</returns>
public virtual ValueGenerator? Select(IProperty property, ITypeBase typeBase)
=> Cache.GetOrAdd(property, typeBase, (p, t) => CreateFromFactory(p, t) ?? Create(p, t));

private static ValueGenerator? CreateFromFactory(IProperty property, ITypeBase structuralType)
Expand Down Expand Up @@ -84,8 +84,8 @@ public virtual ValueGenerator Select(IProperty property, ITypeBase typeBase)
/// The entity type that the value generator will be used for. When called on inherited properties on derived entity types,
/// this entity type may be different from the declared entity type on <paramref name="property" />
/// </param>
/// <returns>The newly created value generator.</returns>
public virtual ValueGenerator Create(IProperty property, ITypeBase typeBase)
/// <returns>The newly created value generator, or <see langword="null"/> if none is available.</returns>
public virtual ValueGenerator? Create(IProperty property, ITypeBase typeBase)
{
var propertyType = property.ClrType.UnwrapNullableType().UnwrapEnumType();
var generator = FindForType(property, typeBase, propertyType);
Expand All @@ -105,8 +105,7 @@ public virtual ValueGenerator Create(IProperty property, ITypeBase typeBase)
}
}

throw new NotSupportedException(
CoreStrings.NoValueGenerator(property.Name, property.DeclaringType.DisplayName(), propertyType.ShortDisplayName()));
return null;
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,18 @@ protected GraphUpdatesInMemoryTestBase(TFixture fixture)
{
}

// In-memory database does not have database default values
public override Task Can_insert_when_bool_PK_in_composite_key_has_sentinel_value(bool async, bool initialValue)
=> Task.CompletedTask;

// In-memory database does not have database default values
public override Task Can_insert_when_int_PK_in_composite_key_has_sentinel_value(bool async, int initialValue)
=> Task.CompletedTask;

// In-memory database does not have database default values
public override Task Can_insert_when_nullable_bool_PK_in_composite_key_has_sentinel_value(bool async, bool? initialValue)
=> Task.CompletedTask;

// In-memory database does not have database default values
public override Task Can_insert_when_composite_FK_has_default_value_for_one_part(bool async)
=> Task.CompletedTask;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,16 +73,14 @@ private static object CreateAndUseFactory(IProperty property)
}

[ConditionalFact]
public void Throws_for_unsupported_combinations()
public void Returns_null_for_unsupported_combinations()
{
var model = BuildModel();
var entityType = model.FindEntityType(typeof(AnEntity));
var entityType = model.FindEntityType(typeof(AnEntity))!;

var selector = InMemoryTestHelpers.Instance.CreateContextServices(model).GetRequiredService<IValueGeneratorSelector>();

Assert.Equal(
CoreStrings.NoValueGenerator("Float", "AnEntity", "float"),
Assert.Throws<NotSupportedException>(() => selector.Select(entityType.FindProperty("Float"), entityType)).Message);
Assert.Null(selector.Select(entityType.FindProperty("Float")!, entityType));
}

private static IModel BuildModel(bool generateValues = true)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -614,6 +614,27 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
b.HasOne(x => x.Brother).WithOne().HasForeignKey<SneakyUncle32084>(x => x.BrotherId);
b.Property(e => e.Id).HasValueGenerator<StableGuidGenerator>();
});

modelBuilder.Entity<CompositeKeyWith<int>>(
b =>
{
b.HasKey(e => new { e.TargetId, e.SourceId, e.PrimaryGroup });
b.Property(e => e.PrimaryGroup).ValueGeneratedOnAdd();
});

modelBuilder.Entity<CompositeKeyWith<bool>>(
b =>
{
b.HasKey(e => new { e.TargetId, e.SourceId, e.PrimaryGroup });
b.Property(e => e.PrimaryGroup).ValueGeneratedOnAdd();
});

modelBuilder.Entity<CompositeKeyWith<bool?>>(
b =>
{
b.HasKey(e => new { e.TargetId, e.SourceId, e.PrimaryGroup });
b.Property(e => e.PrimaryGroup).ValueGeneratedOnAdd();
});
}

private class StableGuidGenerator : ValueGenerator<Guid>
Expand Down Expand Up @@ -4480,6 +4501,32 @@ public StableParent32084 Brother
}
}

protected class CompositeKeyWith<T> : NotifyingEntity
where T : new()
{
private Guid _targetId;
private Guid _sourceId;
private T _primaryGroup;

public Guid TargetId
{
get => _targetId;
set => SetWithNotify(value, ref _targetId);
}

public Guid SourceId
{
get => _sourceId;
set => SetWithNotify(value, ref _sourceId);
}

public T PrimaryGroup
{
get => _primaryGroup;
set => SetWithNotify(value, ref _primaryGroup);
}
}

protected class NotifyingEntity : INotifyPropertyChanging, INotifyPropertyChanged
{
protected void SetWithNotify<T>(T value, ref T field, [CallerMemberName] string propertyName = "")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,66 @@ public virtual async Task Can_insert_when_FK_has_sentinel_value(bool async)
Assert.Equal(cruiser.IdUserState, cruiser.UserState.AccessStateWithSentinelId);
});
[ConditionalTheory]
[InlineData(false, false)]
[InlineData(true, false)]
[InlineData(false, true)]
[InlineData(true, true)]
public virtual Task Can_insert_when_bool_PK_in_composite_key_has_sentinel_value(bool async, bool initialValue)
=> Can_insert_when_PK_property_in_composite_key_has_sentinel_value(async, initialValue);
[ConditionalTheory]
[InlineData(false, 0)]
[InlineData(true, 0)]
[InlineData(false, 1)]
[InlineData(true, 1)]
[InlineData(false, 2)]
[InlineData(true, 2)]
public virtual Task Can_insert_when_int_PK_in_composite_key_has_sentinel_value(bool async, int initialValue)
=> Can_insert_when_PK_property_in_composite_key_has_sentinel_value(async, initialValue);
[ConditionalTheory]
[InlineData(false, false)]
[InlineData(true, false)]
[InlineData(false, true)]
[InlineData(true, true)]
public virtual Task Can_insert_when_nullable_bool_PK_in_composite_key_has_sentinel_value(bool async, bool? initialValue)
=> Can_insert_when_PK_property_in_composite_key_has_sentinel_value(async, initialValue);
protected async Task Can_insert_when_PK_property_in_composite_key_has_sentinel_value<T>(bool async, T initialValue)
where T : new()
{
var inserted = new CompositeKeyWith<T>()
{
SourceId = Guid.NewGuid(),
TargetId = Guid.NewGuid(),
PrimaryGroup = initialValue
};
await ExecuteWithStrategyInTransactionAsync(
async context =>
{
if (async)
{
await context.AddAsync(inserted);
await context.SaveChangesAsync();
}
else
{
context.Add(inserted);
context.SaveChanges();
}
},
async context =>
{
var queryable = context.Set<CompositeKeyWith<T>>();
var loaded = async ? (await queryable.SingleAsync()) : queryable.Single();
Assert.Equal(inserted.SourceId, loaded.SourceId);
Assert.Equal(inserted.TargetId, loaded.TargetId);
Assert.Equal(initialValue, loaded.PrimaryGroup);
});
}
[ConditionalTheory] // Issue #23043
[InlineData(false)]
[InlineData(true)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,18 @@ public override void Required_one_to_one_with_AK_relationships_are_one_to_one(Ca
{
}

// No owned types
public override Task Can_insert_when_bool_PK_in_composite_key_has_sentinel_value(bool async, bool initialValue)
=> Task.CompletedTask;

// No owned types
public override Task Can_insert_when_int_PK_in_composite_key_has_sentinel_value(bool async, int initialValue)
=> Task.CompletedTask;

// No owned types
public override Task Can_insert_when_nullable_bool_PK_in_composite_key_has_sentinel_value(bool async, bool? initialValue)
=> Task.CompletedTask;

protected override void UseTransaction(DatabaseFacade facade, IDbContextTransaction transaction)
=> facade.UseTransaction(transaction.GetDbTransaction());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,24 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
.HasForeignKey<StringKeyAndIndexChild>(e => e.ParentId)
.HasPrincipalKey<StringKeyAndIndexParent>(e => e.AlternateId);
});

modelBuilder.Entity<CompositeKeyWith<int>>(
b =>
{
b.Property(e => e.PrimaryGroup).HasDefaultValue(1).HasSentinel(1);
});

modelBuilder.Entity<CompositeKeyWith<bool>>(
b =>
{
b.Property(e => e.PrimaryGroup).HasDefaultValue(true);
});

modelBuilder.Entity<CompositeKeyWith<bool?>>(
b =>
{
b.Property(e => e.PrimaryGroup).HasDefaultValue(true);
});
}
}
}
Loading

0 comments on commit eee787c

Please sign in to comment.