Stop throwing eagerly when looking up a ValueGenerator (#32930)

* Stop throwing eagerly when looking up a ValueGenerator

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.

* Update to remove the breaking change and still throw for non-composite keys set for generation without any generator.

* Remove interface default implementation

src/EFCore.InMemory/ValueGeneration/Internal/InMemoryValueGeneratorSelector.cs

@@ -35,31 +35,30 @@ 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)
-        => property.GetValueGeneratorFactory() == null
-            && property.ClrType.IsInteger()
-            && property.ClrType.UnwrapNullableType() != typeof(char)
-                ? GetOrCreate(property)
-                : base.Select(property, typeBase);
+    [Obsolete("Use TrySelect and throw if needed when the generator is not found.")]
+    public override ValueGenerator? Select(IProperty property, ITypeBase typeBase)
+    {
+        if (TrySelect(property, typeBase, out var valueGenerator))
+        {
+            return valueGenerator;
+        }
+
+        throw new NotSupportedException(
+            CoreStrings.NoValueGenerator(property.Name, property.DeclaringType.DisplayName(), property.ClrType.ShortDisplayName()));
+    }
 
     /// <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
     ///     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>
-    private ValueGenerator GetOrCreate(IProperty property)
-    {
-        var type = property.ClrType.UnwrapNullableType().UnwrapEnumType();
-        if (FindGenerator(property, type, out var valueGenerator))
-        {
-            return valueGenerator!;
-        }
-
-        throw new ArgumentException(
-            CoreStrings.InvalidValueGeneratorFactoryProperty(
-                "InMemoryIntegerValueGeneratorFactory", property.Name, property.DeclaringType.DisplayName()));
-    }
+    public override bool TrySelect(IProperty property, ITypeBase typeBase, out ValueGenerator? valueGenerator)
+        => property.GetValueGeneratorFactory() == null
+            && property.ClrType.IsInteger()
+            && property.ClrType.UnwrapNullableType() != typeof(char)
+                ? FindGenerator(property, property.ClrType.UnwrapNullableType().UnwrapEnumType(), out valueGenerator)
+                : base.TrySelect(property, typeBase, out valueGenerator);
 
     private bool FindGenerator(IProperty property, Type type, out ValueGenerator? valueGenerator)
     {

src/EFCore.SqlServer/ValueGeneration/Internal/SqlServerValueGeneratorSelector.cs

@@ -53,50 +53,67 @@ 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)
+    [Obsolete("Use TrySelect and throw if needed when the generator is not found.")]
+    public override ValueGenerator? Select(IProperty property, ITypeBase typeBase)
+    {
+        if (TrySelect(property, typeBase, out var valueGenerator))
+        {
+            return valueGenerator;
+        }
+
+        throw new NotSupportedException(
+            CoreStrings.NoValueGenerator(property.Name, property.DeclaringType.DisplayName(), property.ClrType.ShortDisplayName()));
+    }
+
+    /// <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
+    ///     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 bool TrySelect(IProperty property, ITypeBase typeBase, out ValueGenerator? valueGenerator)
     {
         if (property.GetValueGeneratorFactory() != null
             || property.GetValueGenerationStrategy() != SqlServerValueGenerationStrategy.SequenceHiLo)
         {
-            return base.Select(property, typeBase);
+            return base.TrySelect(property, typeBase, out valueGenerator);
         }
 
         var propertyType = property.ClrType.UnwrapNullableType().UnwrapEnumType();
 
-        var generator = _sequenceFactory.TryCreate(
+        valueGenerator = _sequenceFactory.TryCreate(
             property,
             propertyType,
             Cache.GetOrAddSequenceState(property, _connection),
             _connection,
             _rawSqlCommandBuilder,
             _commandLogger);
 
-        if (generator != null)
+        if (valueGenerator != null)
         {
-            return generator;
+            return true;
         }
 
         var converter = property.GetTypeMapping().Converter;
         if (converter != null
             && converter.ProviderClrType != propertyType)
         {
-            generator = _sequenceFactory.TryCreate(
+            valueGenerator = _sequenceFactory.TryCreate(
                 property,
                 converter.ProviderClrType,
                 Cache.GetOrAddSequenceState(property, _connection),
                 _connection,
                 _rawSqlCommandBuilder,
                 _commandLogger);
 
-            if (generator != null)
+            if (valueGenerator != null)
             {
-                return generator.WithConverter(converter);
+                valueGenerator = valueGenerator.WithConverter(converter);
+                return true;
             }
         }
 
-        throw new ArgumentException(
-            CoreStrings.InvalidValueGeneratorFactoryProperty(
-                nameof(SqlServerSequenceValueGeneratorFactory), property.Name, property.DeclaringType.DisplayName()));
+        return false;
     }
 
     /// <summary>

src/EFCore/ChangeTracking/Internal/KeyPropagator.cs

@@ -167,7 +167,20 @@ private static void SetValue(InternalEntityEntry entry, IProperty property, Valu
     }
 
     private ValueGenerator? TryGetValueGenerator(IProperty? generationProperty, ITypeBase? typeBase)
-        => generationProperty != null
-            ? _valueGeneratorSelector.Select(generationProperty, typeBase!)
-            : null;
+    {
+        if (generationProperty == null)
+        {
+            return null;
+        }
+
+        if (!_valueGeneratorSelector.TrySelect(generationProperty, typeBase!, out var valueGenerator))
+        {
+            throw new NotSupportedException(
+                CoreStrings.NoValueGenerator(
+                    generationProperty.Name, generationProperty.DeclaringType.DisplayName(),
+                    generationProperty.ClrType.ShortDisplayName()));
+        }
+
+        return valueGenerator!;
+    }
 }

src/EFCore/ChangeTracking/Internal/ValueGenerationManager.cs

@@ -92,37 +92,26 @@ public virtual bool Generate(InternalEntityEntry entry, bool includePrimaryKey =
         var entityEntry = new EntityEntry(entry);
         var hasStableValues = false;
         var hasNonStableValues = false;
+        IProperty? propertyWithNoGenerator = null;
 
         //TODO: Handle complex properties
         foreach (var property in entry.EntityType.GetValueGeneratingProperties())
         {
-            var valueGenerator = GetValueGenerator(property);
-            if (valueGenerator.GeneratesStableValues)
-            {
-                hasStableValues = true;
-            }
-            else
-            {
-                hasNonStableValues = true;
-            }
-
-            if (entry.HasExplicitValue(property)
-                || (!includePrimaryKey
-                    && property.IsPrimaryKey()))
+            if (!TryFindValueGenerator(
+                    entry, includePrimaryKey, property,
+                    ref hasStableValues, ref hasNonStableValues, ref propertyWithNoGenerator,
+                    out var valueGenerator))
             {
                 continue;
             }
 
-            var generatedValue = valueGenerator.Next(entityEntry);
-            var temporary = valueGenerator.GeneratesTemporaryValues;
-
-            Log(entry, property, generatedValue, temporary);
-
-            SetGeneratedValue(entry, property, generatedValue, temporary);
+            var generatedValue = valueGenerator!.Next(entityEntry);
 
-            MarkKeyUnknown(entry, includePrimaryKey, property, valueGenerator);
+            FinishGenerate(entry, includePrimaryKey, valueGenerator, property, generatedValue);
         }
 
+        CheckPropertyWithNoGenerator(propertyWithNoGenerator);
+
         return hasStableValues && !hasNonStableValues;
     }
 
@@ -152,44 +141,91 @@ public virtual async Task<bool> GenerateAsync(
         var entityEntry = new EntityEntry(entry);
         var hasStableValues = false;
         var hasNonStableValues = false;
+        IProperty? propertyWithNoGenerator = null;
+
+        //TODO: Handle complex properties
         foreach (var property in entry.EntityType.GetValueGeneratingProperties())
         {
-            var valueGenerator = GetValueGenerator(property);
-            if (valueGenerator.GeneratesStableValues)
+            if (!TryFindValueGenerator(
+                    entry, includePrimaryKey, property,
+                    ref hasStableValues, ref hasNonStableValues, ref propertyWithNoGenerator,
+                    out var valueGenerator))
+            {
+                continue;
+            }
+
+            var generatedValue = await valueGenerator!.NextAsync(entityEntry, cancellationToken).ConfigureAwait(false);
+
+            FinishGenerate(entry, includePrimaryKey, valueGenerator, property, generatedValue);
+        }
+
+        CheckPropertyWithNoGenerator(propertyWithNoGenerator);
+
+        return hasStableValues && !hasNonStableValues;
+    }
+
+    private void FinishGenerate(
+        InternalEntityEntry entry,
+        bool includePrimaryKey,
+        ValueGenerator valueGenerator,
+        IProperty property,
+        object? generatedValue)
+    {
+        var temporary = valueGenerator.GeneratesTemporaryValues;
+        Log(entry, property, generatedValue, temporary);
+        SetGeneratedValue(entry, property, generatedValue, temporary);
+        MarkKeyUnknown(entry, includePrimaryKey, property, valueGenerator);
+    }
+
+    private static void CheckPropertyWithNoGenerator(IProperty? property)
+    {
+        if (property != null)
+        {
+            throw new NotSupportedException(
+                CoreStrings.NoValueGenerator(property.Name, property.DeclaringType.DisplayName(), property.ClrType.ShortDisplayName()));
+        }
+    }
+
+    private bool TryFindValueGenerator(
+        InternalEntityEntry entry,
+        bool includePrimaryKey,
+        IProperty property,
+        ref bool hasStableValues,
+        ref bool hasNonStableValues,
+        ref IProperty? propertyWithNoGenerator,
+        out ValueGenerator? valueGenerator)
+    {
+        if (_valueGeneratorSelector.TrySelect(property, property.DeclaringType, out valueGenerator))
+        {
+            if (valueGenerator!.GeneratesStableValues)
             {
                 hasStableValues = true;
             }
             else
             {
                 hasNonStableValues = true;
             }
+        }
 
-            if (entry.HasExplicitValue(property)
-                || (!includePrimaryKey
-                    && property.IsPrimaryKey()))
+        if (valueGenerator == null)
+        {
+            if (property.GetContainingKeys().Any(k => k.Properties.Count == 1))
             {
-                continue;
+                propertyWithNoGenerator ??= property;
             }
+            return false;
+        }
 
-            var generatedValue = await valueGenerator.NextAsync(entityEntry, cancellationToken).ConfigureAwait(false);
-            var temporary = valueGenerator.GeneratesTemporaryValues;
-
-            Log(entry, property, generatedValue, temporary);
-
-            SetGeneratedValue(
-                entry,
-                property,
-                generatedValue,
-                temporary);
-
-            MarkKeyUnknown(entry, includePrimaryKey, property, valueGenerator);
+        if (entry.HasExplicitValue(property)
+            || (!includePrimaryKey
+                && property.IsPrimaryKey()))
+        {
+            return false;
         }
 
-        return hasStableValues && !hasNonStableValues;
-    }
 
-    private ValueGenerator GetValueGenerator(IProperty property)
-        => _valueGeneratorSelector.Select(property, property.DeclaringType);
+        return true;
+    }
 
     private static void SetGeneratedValue(InternalEntityEntry entry, IProperty property, object? generatedValue, bool isTemporary)
     {

src/EFCore/ValueGeneration/IValueGeneratorCache.cs

@@ -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);
 }

src/EFCore/ValueGeneration/IValueGeneratorSelector.cs

@@ -35,5 +35,18 @@ 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);
+    [Obsolete("Use TrySelect and throw if needed when the generator is not found.")]
+    ValueGenerator? Select(IProperty property, ITypeBase typeBase);
+
+    /// <summary>
+    ///     Selects the appropriate value generator for a given property, if available.
+    /// </summary>
+    /// <param name="property">The property to get the value generator for.</param>
+    /// <param name="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>
+    /// <param name="valueGenerator">The value generator, or <see langword="null"/> if none is available.</param>
+    /// <returns><see langword="true"/> if a value generator was selected; <see langword="false"/> if none was available.</returns>
+    bool TrySelect(IProperty property, ITypeBase typeBase, out ValueGenerator? valueGenerator);
 }

src/EFCore/ValueGeneration/ValueGeneratorCache.cs

@@ -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>
     {
@@ -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));
 }