Skip to content

Commit

Permalink
Fix to #31100 - Switch to storing enums as ints in JSON instead of st…
Browse files Browse the repository at this point in the history 
…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
  • Loading branch information
@maumar
maumar committed Jul 20, 2023
1 parent c6b5eac commit cf27c6d
Show file tree
Hide file tree
Showing 6 changed files with 411 additions and 100 deletions.
8 changes: 0 additions & 8 deletions src/EFCore.Relational/Metadata/Conventions/RelationalMapToJsonConvention.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,5 @@ public virtual void ProcessModelFinalizing(
IConventionModelBuilder modelBuilder,
IConventionContext<IConventionModelBuilder> context)
{
foreach (var jsonEntityType in modelBuilder.Metadata.GetEntityTypes().Where(e => e.IsMappedToJson()))
{
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));
}
}
}
}
48 changes: 47 additions & 1 deletion ...uery/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,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;
Expand Down Expand Up @@ -2339,7 +2345,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);
Expand All @@ -2357,6 +2362,47 @@ private Expression CreateReadJsonPropertyValueExpression(
resultExpression);
}

// see issue #31100
// in 7.0 we stored enums as strings by default (by applying EnumToStringConverter by convention on enums inside JSON entities)
// in 8.0 we are reverting this decision and store enums as int (their natural representation in JSON as well as in database using EF)
// however it is a breaking change and it makes it very hard for people who used 7.0 version to work with legacy representation
// users can easily land in a situation where they have a mix of int and string representations
// update to 8.0, start adding more entities to the database, new entities are stored as ints, whereas old ones remain string
// to mitigate this we provide fallback mechanism here - if we can't read enum value using new default (EnumToNumberConverter)
// we assume it's the legacy scenario and attempt to parse from string
if (property.ClrType.UnwrapNullableType().IsEnum
&& property.GetRelationalTypeMapping().Converter is ValueConverter converter
&& converter.GetType() is Type { IsGenericType: true } converterType
&& converterType.GetGenericTypeDefinition() == typeof(EnumToNumberConverter<,>)
&& converterType.GetGenericArguments() is Type[] genericArgs
&& genericArgs[0] == converter.ModelClrType
&& genericArgs[1] == converter.ProviderClrType)
{
// try
// {
// JsonConvertedValueReaderWriter<MyEnumType, underlying_type>.FromJsonTyped(...));
// }
// catch(InvalidOperationException e)
// {
// (MyEnumType)Enum.Parse(typeof(MyEnumType), jsonReaderManager.CurrentReader.GetString());
// }
var exceptionParameter = Parameter(typeof(InvalidOperationException), name: "e");
var catchBlock = Catch(
exceptionParameter,
Convert(
Call(
EnumParseMethodInfo,
Constant(converter.ModelClrType),
Call(
Field(
jsonReaderManagerParameter,
Utf8JsonReaderManagerCurrentReaderField),
Utf8JsonReaderGetStringMethod)),
property.ClrType));

resultExpression = TryCatch(resultExpression, catchBlock);
}

if (_detailedErrorsEnabled)
{
var exceptionParameter = Parameter(typeof(Exception), name: "e");
Expand Down
117 changes: 117 additions & 0 deletions test/EFCore.Relational.Specification.Tests/Query/JsonQueryAdHocTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -707,6 +707,123 @@ public MyJsonEntityShadowPropertiesWithCtor(string name)

#endregion

//#region EnumLegacyValues

//[ConditionalTheory]
//[MemberData(nameof(IsAsyncData))]
//public virtual async Task Read_enum_property_with_legacy_values(bool async)
//{
// var contextFactory = await InitializeAsync<MyContextEnumLegacyValues>(
// seed: SeedEnumLegacyValues);

// using (var context = contextFactory.CreateContext())
// {
// var query = context.Entities.Select(x => new
// {
// x.Reference.IntEnum,
// x.Reference.ByteEnum,
// x.Reference.LongEnum,
// x.Reference.NullableEnum
// });

// var result = async
// ? await query.ToListAsync()
// : query.ToList();

// Assert.Equal(1, result.Count);
// //Assert.Equal("Foo", result[0].ShadowString);
// //Assert.Equal(143, result[0].ShadowInt);
// }
//}

//[ConditionalTheory]
//[MemberData(nameof(IsAsyncData))]
//public virtual async Task Read_json_entity_with_enum_properties_with_legacy_values(bool async)
//{
// var contextFactory = await InitializeAsync<MyContextEnumLegacyValues>(
// seed: SeedEnumLegacyValues);

// using (var context = contextFactory.CreateContext())
// {
// var query = context.Entities.Select(x => x.Reference).AsNoTracking();

// var result = async
// ? await query.ToListAsync()
// : query.ToList();

// Assert.Equal(1, result.Count);
// //Assert.Equal("Foo", result[0].ShadowString);
// //Assert.Equal(143, result[0].ShadowInt);
// }
//}

//protected abstract void SeedEnumLegacyValues(MyContextEnumLegacyValues ctx);

//protected class MyContextEnumLegacyValues : DbContext
//{
// public MyContextEnumLegacyValues(DbContextOptions options)
// : base(options)
// {
// }

// public DbSet<MyEntityEnumLegacyValues> Entities { get; set; }

// protected override void OnModelCreating(ModelBuilder modelBuilder)
// {
// modelBuilder.Entity<MyEntityEnumLegacyValues>().Property(x => x.Id).ValueGeneratedNever();
// modelBuilder.Entity<MyEntityEnumLegacyValues>().OwnsOne(x => x.Reference, b =>
// {
// b.ToJson();
// });
// modelBuilder.Entity<MyEntityEnumLegacyValues>().OwnsMany(x => x.Collection, b =>
// {
// b.ToJson();
// });
// }
//}

//public class MyEntityEnumLegacyValues
//{
// public int Id { get; set; }
// public string Name { get; set; }

// public MyJsonEntityEnumLegacyValues Reference { get; set; }
// public List<MyJsonEntityEnumLegacyValues> Collection { get; set; }
//}

//public class MyJsonEntityEnumLegacyValues
//{
// public string Name { get; set; }

// public IntEnumLegacyValues IntEnum { get; set; }
// public ByteEnumLegacyValues ByteEnum { get; set; }
// public LongEnumLegacyValues LongEnum { get; set; }
// public IntEnumLegacyValues? NullableEnum { get; set; }
//}

//public enum IntEnumLegacyValues
//{
// Foo,
// Bar,
// Baz,
//}

//public enum ByteEnumLegacyValues : byte
//{
// Seattle,
// Redmond,
// Bellevue,
//}

//public enum LongEnumLegacyValues : long
//{
// One,
// Two,
// Three,
//}

//#endregion

protected TestSqlLoggerFactory TestSqlLoggerFactory
=> (TestSqlLoggerFactory)ListLoggerFactory;
}
156 changes: 156 additions & 0 deletions test/EFCore.SqlServer.FunctionalTests/Query/JsonQueryAdHocSqlServerTest.cs
View file
Original file line number Diff line number Diff line change
@@ -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.Data.SqlClient;

namespace Microsoft.EntityFrameworkCore.Query;

public class JsonQueryAdHocSqlServerTest : JsonQueryAdHocTestBase
Expand Down Expand Up @@ -140,4 +142,158 @@ protected override void SeedShadowProperties(MyContextShadowProperties ctx)
1,
N'e1')");
}

#region EnumLegacyValues

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual async Task Read_enum_property_with_legacy_values(bool async)
{
var contextFactory = await InitializeAsync<MyContextEnumLegacyValues>(
seed: SeedEnumLegacyValues);

using (var context = contextFactory.CreateContext())
{
var query = context.Entities.Select(x => new
{
x.Reference.IntEnum,
x.Reference.ByteEnum,
x.Reference.LongEnum,
x.Reference.NullableEnum
});

var exception = async
? await (Assert.ThrowsAsync<SqlException>(() => query.ToListAsync()))
: Assert.Throws<SqlException>(() => query.ToList());

// Conversion failed when converting the nvarchar value '...' to data type int
Assert.Equal(245, exception.Number);
}
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual async Task Read_json_entity_with_enum_properties_with_legacy_values(bool async)
{
var contextFactory = await InitializeAsync<MyContextEnumLegacyValues>(
seed: SeedEnumLegacyValues);

using (var context = contextFactory.CreateContext())
{
var query = context.Entities.Select(x => x.Reference).AsNoTracking();

var result = async
? await query.ToListAsync()
: query.ToList();

Assert.Equal(1, result.Count);
Assert.Equal(ByteEnumLegacyValues.Redmond, result[0].ByteEnum);
Assert.Equal(IntEnumLegacyValues.Foo, result[0].IntEnum);
Assert.Equal(LongEnumLegacyValues.Three, result[0].LongEnum);
Assert.Equal(IntEnumLegacyValues.Bar, result[0].NullableEnum);
}
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual async Task Read_json_entity_collection_with_enum_properties_with_legacy_values(bool async)
{
var contextFactory = await InitializeAsync<MyContextEnumLegacyValues>(
seed: SeedEnumLegacyValues);

using (var context = contextFactory.CreateContext())
{
var query = context.Entities.Select(x => x.Collection).AsNoTracking();

var result = async
? await query.ToListAsync()
: query.ToList();

Assert.Equal(1, result.Count);
Assert.Equal(2, result[0].Count);
Assert.Equal(ByteEnumLegacyValues.Bellevue, result[0][0].ByteEnum);
Assert.Equal(IntEnumLegacyValues.Foo, result[0][0].IntEnum);
Assert.Equal(LongEnumLegacyValues.One, result[0][0].LongEnum);
Assert.Equal(IntEnumLegacyValues.Bar, result[0][0].NullableEnum);
Assert.Equal(ByteEnumLegacyValues.Seattle, result[0][1].ByteEnum);
Assert.Equal(IntEnumLegacyValues.Baz, result[0][1].IntEnum);
Assert.Equal(LongEnumLegacyValues.Two, result[0][1].LongEnum);
Assert.Null(result[0][1].NullableEnum);
}
}

private void SeedEnumLegacyValues(MyContextEnumLegacyValues ctx)
{
ctx.Database.ExecuteSqlRaw(@"INSERT INTO [Entities] ([Collection], [Reference], [Id], [Name])
VALUES(
N'[{{""ByteEnum"":""Bellevue"",""IntEnum"":""Foo"",""LongEnum"":""One"",""Name"":""e1_c1"",""NullableEnum"":""Bar""}},{{""ByteEnum"":""Seattle"",""IntEnum"":""Baz"",""LongEnum"":""Two"",""Name"":""e1_c2"",""NullableEnum"":null}}]',
N'{{""ByteEnum"":""Redmond"",""IntEnum"":""Foo"",""LongEnum"":""Three"",""Name"":""e1_r"",""NullableEnum"":""Bar""}}',
1,
N'e1')");
}

private class MyContextEnumLegacyValues : DbContext
{
public MyContextEnumLegacyValues(DbContextOptions options)
: base(options)
{
}

public DbSet<MyEntityEnumLegacyValues> Entities { get; set; }

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
modelBuilder.Entity<MyEntityEnumLegacyValues>().Property(x => x.Id).ValueGeneratedNever();
modelBuilder.Entity<MyEntityEnumLegacyValues>().OwnsOne(x => x.Reference, b =>
{
b.ToJson();
});
modelBuilder.Entity<MyEntityEnumLegacyValues>().OwnsMany(x => x.Collection, b =>
{
b.ToJson();
});
}
}

private class MyEntityEnumLegacyValues
{
public int Id { get; set; }
public string Name { get; set; }

public MyJsonEntityEnumLegacyValues Reference { get; set; }
public List<MyJsonEntityEnumLegacyValues> Collection { get; set; }
}

private class MyJsonEntityEnumLegacyValues
{
public string Name { get; set; }

public IntEnumLegacyValues IntEnum { get; set; }
public ByteEnumLegacyValues ByteEnum { get; set; }
public LongEnumLegacyValues LongEnum { get; set; }
public IntEnumLegacyValues? NullableEnum { get; set; }
}

private enum IntEnumLegacyValues
{
Foo,
Bar,
Baz,
}

private enum ByteEnumLegacyValues : byte
{
Seattle,
Redmond,
Bellevue,
}

private enum LongEnumLegacyValues : long
{
One,
Two,
Three,
}

#endregion
}
Loading

0 comments on commit cf27c6d

Please sign in to comment.