Fix to #31100 - Switch to storing enums as ints in JSON instead of st…

…rings

Problem was that in 7.0 we stored enums inside JSON as strings by default (by applying EnumToStringConverter by convention), but in 8.0 we are changing this to int.
This is a breaking change and it's extra problematic for databases that used EF JSON functionality in 7.0. This can easily create a scenario where there is a mix of string and int representation for an enum value within the same document.
(some data was added in 7.0, and then some in 8.0 before customer realized that the breaking change has been made). To mitigate this we are adding a fallback mechanism when reading enum data that is part of JSON entity. We try to read as int and if that fails we try to read again as string. This way should minimize the disruption, moreover any data saved back to the database will be saved in the new format, so over time everything should normalize.
We will still throw when projecting individual enum properties of a JSON entity (as opposed to the entire entity), because materialization for this goes through different path, indistinguishable from normal enum value read from column in relational table.

Fixes #31100

Taking over the PR

Tweaks from review

src/EFCore.Relational/Metadata/Conventions/RelationalMapToJsonConvention.cs

@@ -1,6 +1,8 @@
 // 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.Storage.Json;
+
 namespace Microsoft.EntityFrameworkCore.Metadata.Conventions;
 
 /// <summary>
@@ -52,10 +54,17 @@ public virtual void ProcessModelFinalizing(
     {
         foreach (var jsonEntityType in modelBuilder.Metadata.GetEntityTypes().Where(e => e.IsMappedToJson()))
         {
-            foreach (var enumProperty in jsonEntityType.GetDeclaredProperties().Where(p => p.ClrType.UnwrapNullableType().IsEnum))
+            foreach (var enumProperty in jsonEntityType
+                         .GetDeclaredProperties()
+                         .Where(p => p.ClrType.UnwrapNullableType().IsEnum))
             {
-                // by default store enums as strings - values should be human-readable
-                enumProperty.Builder.HasConversion(typeof(string));
+                // If the enum is mapped with no conversion, then use the reader/writer that handles legacy string values and warns.
+                if (enumProperty.GetValueConverter() == null
+                    && enumProperty.GetProviderClrType() == null)
+                {
+                    enumProperty.SetJsonValueReaderWriterType(
+                        typeof(JsonWarningEnumReaderWriter<>).MakeGenericType(enumProperty.ClrType.UnwrapNullableType()));
+                }
             }
         }
     }

src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.ClientMethods.cs

@@ -877,7 +877,7 @@ static async Task<RelationalDataReader> InitializeReaderAsync(
                     RelationalStrings.JsonRequiredEntityWithNullJson(typeof(TEntity).Name));
             }
 
-            var manager = new Utf8JsonReaderManager(jsonReaderData);
+            var manager = new Utf8JsonReaderManager(jsonReaderData, queryContext.QueryLogger);
             var tokenType = manager.CurrentReader.TokenType;
 
             if (tokenType == JsonTokenType.Null)
@@ -914,7 +914,7 @@ static async Task<RelationalDataReader> InitializeReaderAsync(
                 return default;
             }
 
-            var manager = new Utf8JsonReaderManager(jsonReaderData);
+            var manager = new Utf8JsonReaderManager(jsonReaderData, queryContext.QueryLogger);
             var tokenType = manager.CurrentReader.TokenType;
 
             if (tokenType == JsonTokenType.Null)
@@ -946,7 +946,7 @@ static async Task<RelationalDataReader> InitializeReaderAsync(
                     manager.CaptureState();
                     var entity = innerShaper(queryContext, newKeyPropertyValues, jsonReaderData);
                     result.Add(entity);
-                    manager = new Utf8JsonReaderManager(manager.Data);
+                    manager = new Utf8JsonReaderManager(manager.Data, queryContext.QueryLogger);
 
                     if (manager.CurrentReader.TokenType != JsonTokenType.EndObject)
                     {
@@ -1003,7 +1003,7 @@ private static void IncludeJsonEntityCollection<TIncludingEntity, TIncludedColle
                 return;
             }
 
-            var manager = new Utf8JsonReaderManager(jsonReaderData);
+            var manager = new Utf8JsonReaderManager(jsonReaderData, queryContext.QueryLogger);
             var tokenType = manager.CurrentReader.TokenType;
 
             if (tokenType != JsonTokenType.StartArray)
@@ -1032,7 +1032,7 @@ private static void IncludeJsonEntityCollection<TIncludingEntity, TIncludedColle
                         fixup(entity, resultElement);
                     }
 
-                    manager = new Utf8JsonReaderManager(manager.Data);
+                    manager = new Utf8JsonReaderManager(manager.Data, queryContext.QueryLogger);
                     if (manager.CurrentReader.TokenType != JsonTokenType.EndObject)
                     {
                         throw new InvalidOperationException(

src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs

@@ -44,7 +44,8 @@ private static readonly ConstructorInfo JsonReaderDataConstructor
             = typeof(JsonReaderData).GetConstructor(new[] { typeof(Stream) })!;
 
         private static readonly ConstructorInfo JsonReaderManagerConstructor
-            = typeof(Utf8JsonReaderManager).GetConstructor(new[] { typeof(JsonReaderData) })!;
+            = typeof(Utf8JsonReaderManager).GetConstructor(
+                new[] { typeof(JsonReaderData), typeof(IDiagnosticsLogger<DbLoggerCategory.Query>) })!;
 
         private static readonly MethodInfo Utf8JsonReaderManagerMoveNextMethod
             = typeof(Utf8JsonReaderManager).GetMethod(nameof(Utf8JsonReaderManager.MoveNext), Array.Empty<Type>())!;
@@ -64,6 +65,12 @@ private static readonly MethodInfo Utf8JsonReaderTrySkipMethod
         private static readonly PropertyInfo Utf8JsonReaderTokenTypeProperty
             = typeof(Utf8JsonReader).GetProperty(nameof(Utf8JsonReader.TokenType))!;
 
+        private static readonly MethodInfo Utf8JsonReaderGetStringMethod
+            = typeof(Utf8JsonReader).GetMethod(nameof(Utf8JsonReader.GetString), Array.Empty<Type>())!;
+
+        private static readonly MethodInfo EnumParseMethodInfo
+            = typeof(Enum).GetMethod(nameof(Enum.Parse), new[] { typeof(Type), typeof(string) })!;
+
         private readonly RelationalShapedQueryCompilingExpressionVisitor _parentVisitor;
         private readonly ISet<string>? _tags;
         private readonly bool _isTracking;
@@ -73,6 +80,7 @@ private static readonly PropertyInfo Utf8JsonReaderTokenTypeProperty
         private readonly bool _generateCommandCache;
         private readonly ParameterExpression _resultCoordinatorParameter;
         private readonly ParameterExpression? _executionStrategyParameter;
+        private readonly IDiagnosticsLogger<DbLoggerCategory.Query> _queryLogger;
 
         /// <summary>
         ///     States scoped to SelectExpression
@@ -154,6 +162,7 @@ public ShaperProcessingExpressionVisitor(
             bool indexMap)
         {
             _parentVisitor = parentVisitor;
+            _queryLogger = parentVisitor.QueryCompilationContext.Logger;
             _resultCoordinatorParameter = Parameter(
                 splitQuery ? typeof(SplitQueryResultCoordinator) : typeof(SingleQueryResultCoordinator), "resultCoordinator");
             _executionStrategyParameter = splitQuery ? Parameter(typeof(IExecutionStrategy), "executionStrategy") : null;
@@ -187,6 +196,7 @@ private ShaperProcessingExpressionVisitor(
             ReaderColumn?[]? readerColumns)
         {
             _parentVisitor = parentVisitor;
+            _queryLogger = parentVisitor.QueryCompilationContext.Logger;
             _resultCoordinatorParameter = resultCoordinatorParameter;
 
             _selectExpression = selectExpression;
@@ -209,6 +219,7 @@ private ShaperProcessingExpressionVisitor(
             ISet<string> tags)
         {
             _parentVisitor = parentVisitor;
+            _queryLogger = parentVisitor.QueryCompilationContext.Logger;
             _resultCoordinatorParameter = resultCoordinatorParameter;
             _executionStrategyParameter = executionStrategyParameter;
 
@@ -1295,7 +1306,9 @@ private Expression CreateJsonShapers(
                 entityShaperExpression.EntityType,
                 _isTracking,
                 jsonReaderDataShaperLambdaParameter,
-                innerShapersMap, innerFixupMap).Rewrite(entityShaperMaterializer);
+                innerShapersMap,
+                innerFixupMap,
+                _queryLogger).Rewrite(entityShaperMaterializer);
 
             var entityShaperMaterializerVariable = Variable(
                 entityShaperMaterializer.Type,
@@ -1413,6 +1426,7 @@ private sealed class JsonEntityMaterializerRewriter : ExpressionVisitor
             private readonly ParameterExpression _jsonReaderDataParameter;
             private readonly IDictionary<string, Expression> _innerShapersMap;
             private readonly IDictionary<string, LambdaExpression> _innerFixupMap;
+            private readonly IDiagnosticsLogger<DbLoggerCategory.Query> _queryLogger;
 
             private static readonly PropertyInfo JsonEncodedTextEncodedUtf8BytesProperty
                 = typeof(JsonEncodedText).GetProperty(nameof(JsonEncodedText.EncodedUtf8Bytes))!;
@@ -1426,13 +1440,15 @@ public JsonEntityMaterializerRewriter(
                 bool isTracking,
                 ParameterExpression jsonReaderDataParameter,
                 IDictionary<string, Expression> innerShapersMap,
-                IDictionary<string, LambdaExpression> innerFixupMap)
+                IDictionary<string, LambdaExpression> innerFixupMap,
+                IDiagnosticsLogger<DbLoggerCategory.Query> queryLogger)
             {
                 _entityType = entityType;
                 _isTracking = isTracking;
                 _jsonReaderDataParameter = jsonReaderDataParameter;
                 _innerShapersMap = innerShapersMap;
                 _innerFixupMap = innerFixupMap;
+                _queryLogger = queryLogger;
             }
 
             public BlockExpression Rewrite(BlockExpression jsonEntityShaperMaterializer)
@@ -1501,7 +1517,8 @@ protected override Expression VisitSwitch(SwitchExpression switchExpression)
                             managerVariable,
                             New(
                                 JsonReaderManagerConstructor,
-                                _jsonReaderDataParameter)),
+                                _jsonReaderDataParameter,
+                                Constant(_queryLogger))),
                         // tokenType = jsonReaderManager.CurrentReader.TokenType
                         Assign(
                             tokenTypeVariable,
@@ -1657,7 +1674,8 @@ protected override Expression VisitSwitch(SwitchExpression switchExpression)
                         var moveNext = Call(managerVariable, Utf8JsonReaderManagerMoveNextMethod);
                         var captureState = Call(managerVariable, Utf8JsonReaderManagerCaptureStateMethod);
                         var assignment = Assign(propertyVariable, innerShaperMapElement.Value);
-                        var managerRecreation = Assign(managerVariable, New(JsonReaderManagerConstructor, _jsonReaderDataParameter));
+                        var managerRecreation = Assign(
+                            managerVariable, New(JsonReaderManagerConstructor, _jsonReaderDataParameter, Constant(_queryLogger)));
 
                         readExpressions.Add(
                             Block(
@@ -2011,7 +2029,8 @@ private static IList<T> PopulateList<T>(IList<T> buffer, IList<T> target)
                         jsonReaderDataVariable,
                         Default(typeof(JsonReaderData))),
                     Block(
-                        Assign(jsonReaderManagerVariable, New(JsonReaderManagerConstructor, jsonReaderDataVariable)),
+                        Assign(
+                            jsonReaderManagerVariable, New(JsonReaderManagerConstructor, jsonReaderDataVariable, Constant(_queryLogger))),
                         Call(jsonReaderManagerVariable, Utf8JsonReaderManagerMoveNextMethod),
                         Call(jsonReaderManagerVariable, Utf8JsonReaderManagerCaptureStateMethod)));
 
@@ -2373,7 +2392,6 @@ private Expression CreateReadJsonPropertyValueExpression(
             {
                 // in case of null value we can't just use the JsonReader method, but rather check the current token type
                 // if it's JsonTokenType.Null means value is null, only if it's not we are safe to read the value
-
                 if (resultExpression.Type != property.ClrType)
                 {
                     resultExpression = Convert(resultExpression, property.ClrType);

src/EFCore/Diagnostics/CoreEventId.cs

@@ -75,6 +75,7 @@ private enum Id
         NavigationBaseIncludeIgnored,
         DistinctAfterOrderByWithoutRowLimitingOperatorWarning,
         QueryCanceled,
+        StringEnumValueInJson,
 
         // Infrastructure events
         SensitiveDataLoggingEnabledWarning = CoreBaseId + 400,
@@ -326,6 +327,16 @@ public static readonly EventId DistinctAfterOrderByWithoutRowLimitingOperatorWar
     public static readonly EventId QueryCanceled
         = MakeQueryId(Id.QueryCanceled);
 
+    /// <summary>
+    ///     A string value for an enum was read from JSON. Starting with EF Core 8, a breaking change was made to store enum
+    ///     values in JSON as numbers by default. See https://aka.ms/efcore-docs-jsonenums for details.
+    /// </summary>
+    /// <remarks>
+    ///     This event is in the <see cref="DbLoggerCategory.Query" /> category.
+    /// </remarks>
+    public static readonly EventId StringEnumValueInJson
+        = MakeQueryId(Id.StringEnumValueInJson);
+
     private static readonly string _infraPrefix = DbLoggerCategory.Infrastructure.Name + ".";
 
     private static EventId MakeInfraId(Id id)

src/EFCore/Diagnostics/CoreLoggerExtensions.cs

@@ -2583,6 +2583,40 @@ private static string ReferenceChangeDetectedSensitive(EventDefinitionBase defin
             p.EntityEntry.GetInfrastructure().BuildCurrentValuesString(p.Navigation.DeclaringEntityType.FindPrimaryKey()!.Properties));
     }
 
+    /// <summary>
+    ///     Logs for the <see cref="CoreEventId.StringEnumValueInJson" /> event.
+    /// </summary>
+    /// <param name="diagnostics">The diagnostics logger to use.</param>
+    /// <param name="enumType">The type.</param>
+    public static void StringEnumValueInJson(
+        this IDiagnosticsLogger<DbLoggerCategory.Query> diagnostics,
+        Type enumType)
+    {
+        var definition = CoreResources.LogStringEnumValueInJson(diagnostics);
+
+        if (diagnostics.ShouldLog(definition))
+        {
+            definition.Log(diagnostics, enumType.ShortDisplayName());
+        }
+
+        if (diagnostics.NeedsEventData(definition, out var diagnosticSourceEnabled, out var simpleLogEnabled))
+        {
+            var eventData = new TypeEventData(
+                definition,
+                StringEnumValueInJson,
+                enumType);
+
+            diagnostics.DispatchEventData(definition, eventData, diagnosticSourceEnabled, simpleLogEnabled);
+        }
+    }
+
+    private static string StringEnumValueInJson(EventDefinitionBase definition, EventData payload)
+    {
+        var d = (EventDefinition<string>)definition;
+        var p = (TypeEventData)payload;
+        return d.GenerateMessage(p.ClrType.ShortDisplayName());
+    }
+
     /// <summary>
     ///     Logs for the <see cref="CoreEventId.StartedTracking" /> event.
     /// </summary>

src/EFCore/Diagnostics/ILoggingOptions.cs

@@ -39,4 +39,11 @@ public interface ILoggingOptions : ISingletonOptions
     ///     Reflects the option set by <see cref="DbContextOptionsBuilder.ConfigureWarnings" />.
     /// </summary>
     WarningsConfiguration WarningsConfiguration { get; }
+
+    /// <summary>
+    ///    Returns <see langword="true"/> if a warning about string values for the given enum type has not yet been performed.
+    /// </summary>
+    /// <param name="type">The type to check.</param>
+    /// <returns>Whether or not a warning has been issued.</returns>
+    bool ShouldWarnForEnumType(Type type);
 }

src/EFCore/Diagnostics/Internal/LoggingOptions.cs

@@ -1,6 +1,8 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Collections.Concurrent;
+
 namespace Microsoft.EntityFrameworkCore.Diagnostics.Internal;
 
 /// <summary>
@@ -11,6 +13,8 @@ namespace Microsoft.EntityFrameworkCore.Diagnostics.Internal;
 /// </summary>
 public class LoggingOptions : ILoggingOptions
 {
+    private readonly ConcurrentDictionary<Type, bool> _warnedForStringEnums = new();
+
     /// <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
@@ -98,4 +102,16 @@ public virtual void Validate(IDbContextOptions options)
     ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
     public virtual WarningsConfiguration WarningsConfiguration { get; private set; } = null!;
+
+    /// <inheritdoc />
+    public virtual bool ShouldWarnForEnumType(Type type)
+    {
+        if (_warnedForStringEnums.ContainsKey(type))
+        {
+            return false;
+        }
+
+        _warnedForStringEnums[type] = true;
+        return true;
+    }
 }

src/EFCore/Diagnostics/LoggingDefinitions.cs

@@ -223,6 +223,15 @@ public abstract class LoggingDefinitions
     [EntityFrameworkInternal]
     public EventDefinitionBase? LogSkipCollectionChangeDetectedSensitive;
 
+    /// <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>
+    [EntityFrameworkInternal]
+    public EventDefinitionBase? LogStringEnumValueInJson;
+
     /// <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

src/EFCore/Diagnostics/TypeEventData.cs

@@ -0,0 +1,33 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.EntityFrameworkCore.Diagnostics;
+
+/// <summary>
+///     A <see cref="DiagnosticSource" /> event payload class for events that reference a <see cref="Type" />.
+/// </summary>
+/// <remarks>
+///     See <see href="https://aka.ms/efcore-docs-diagnostics">Logging, events, and diagnostics</see> for more information and examples.
+/// </remarks>
+public class TypeEventData : EventData
+{
+    /// <summary>
+    ///     Constructs the event payload.
+    /// </summary>
+    /// <param name="eventDefinition">The event definition.</param>
+    /// <param name="messageGenerator">A delegate that generates a log message for this event.</param>
+    /// <param name="clrType">The <see cref="Type"/> associated with this event.</param>
+    public TypeEventData(
+        EventDefinitionBase eventDefinition,
+        Func<EventDefinitionBase, EventData, string> messageGenerator,
+        Type clrType)
+        : base(eventDefinition, messageGenerator)
+    {
+        ClrType = clrType;
+    }
+
+    /// <summary>
+    ///     The <see cref="Type"/> associated with this event.
+    /// </summary>
+    public virtual Type ClrType { get; }
+}