Skip to content

Commit

Permalink
Fix to #28881 - Consider removing unnecessary CASTs around JSON_VALUE
Browse files Browse the repository at this point in the history 
We can remove cast for string scalars, other types maybe cause conversion issues in the generated sql.

Fixes #28881
  • Loading branch information
@maumar
maumar committed Nov 19, 2022
1 parent c771d25 commit 3c03f34
Show file tree
Hide file tree
Showing 7 changed files with 628 additions and 29 deletions.
7 changes: 5 additions & 2 deletions src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -313,14 +313,17 @@ protected override Expression VisitJsonScalar(JsonScalarExpression jsonScalarExp
}
else
{
Sql.Append("CAST(JSON_VALUE(");
// JSON_VALUE always returns nvarchar(4000) (https://learn.microsoft.com/sql/t-sql/functions/json-value-transact-sql),
// so we cast the result to the expected type - except if it's a string (since the cast interferes with indexes over
// the JSON property).
Sql.Append(jsonScalarExpression.TypeMapping is StringTypeMapping ? "JSON_VALUE(" : "CAST(JSON_VALUE(");
}

Visit(jsonScalarExpression.JsonColumn);

Sql.Append($",'{string.Join("", jsonScalarExpression.Path.Select(e => e.ToString()))}')");

if (jsonScalarExpression.Type != typeof(JsonElement))
if (jsonScalarExpression.TypeMapping is not SqlServerJsonTypeMapping and not StringTypeMapping)
{
Sql.Append(" AS ");
Sql.Append(jsonScalarExpression.TypeMapping!.StoreType);
Expand Down
4 changes: 4 additions & 0 deletions test/EFCore.Relational.Specification.Tests/Query/JsonQueryFixtureBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,8 @@ public static void AssertCustomNameBranch(JsonOwnedCustomNameBranch expected, Js

public static void AssertAllTypes(JsonOwnedAllTypes expected, JsonOwnedAllTypes actual)
{
Assert.Equal(expected.TestDefaultString, actual.TestDefaultString);
Assert.Equal(expected.TestMaxLengthString, actual.TestMaxLengthString);
Assert.Equal(expected.TestBoolean, actual.TestBoolean);
Assert.Equal(expected.TestCharacter, actual.TestCharacter);
Assert.Equal(expected.TestDateTime, actual.TestDateTime);
Expand Down Expand Up @@ -502,6 +504,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
x => x.Reference, b =>
{
b.ToJson();
b.Property(x => x.TestMaxLengthString).HasMaxLength(5);
b.Property(x => x.TestDecimal).HasPrecision(18, 3);
b.Property(x => x.TestEnumWithIntConverter).HasConversion<int>();
b.Property(x => x.TestNullableEnumWithIntConverter).HasConversion<int>();
Expand Down Expand Up @@ -529,6 +532,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
x => x.Collection, b =>
{
b.ToJson();
b.Property(x => x.TestMaxLengthString).HasMaxLength(5);
b.Property(x => x.TestDecimal).HasPrecision(18, 3);
});
}
Expand Down
235 changes: 234 additions & 1 deletion test/EFCore.Relational.Specification.Tests/Query/JsonQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -779,6 +779,8 @@ public virtual Task Json_all_types_projection_individual_properties(bool async)
ss => ss.Set<JsonEntityAllTypes>().Select(
x => new
{
x.Reference.TestDefaultString,
x.Reference.TestMaxLengthString,
x.Reference.TestBoolean,
x.Reference.TestByte,
x.Reference.TestCharacter,
Expand All @@ -796,7 +798,6 @@ public virtual Task Json_all_types_projection_individual_properties(bool async)
x.Reference.TestUnsignedInt16,
x.Reference.TestUnsignedInt32,
x.Reference.TestUnsignedInt64,
x.Reference.TestNullableInt32,
x.Reference.TestEnum,
x.Reference.TestEnumWithIntConverter,
x.Reference.TestNullableEnum,
Expand Down Expand Up @@ -834,6 +835,238 @@ public virtual Task Json_boolean_projection_negated(bool async)
async,
ss => ss.Set<JsonEntityAllTypes>().Select(x => !x.Reference.TestBoolean));

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_default_string(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestDefaultString != "MyDefaultStringInReference1"),
entryCount: 3);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_max_length_string(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestMaxLengthString != "Foo"),
entryCount: 3);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_string_condition(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => (!x.Reference.TestBoolean ? x.Reference.TestMaxLengthString : x.Reference.TestDefaultString) == "MyDefaultStringInReference1"),
entryCount: 3);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_byte(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestByte != 3),
entryCount: 6);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_character(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestCharacter != 'z'),
entryCount: 6);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_datetime(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestDateTime != new DateTime(2000, 1, 3)),
entryCount: 6);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_datetimeoffset(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestDateTimeOffset != new DateTimeOffset(new DateTime(2000, 1, 4), new TimeSpan(3, 2, 0))),
entryCount: 6);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_decimal(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestDecimal != 1.35M),
entryCount: 6);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_double(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestDouble != 33.25),
entryCount: 6);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_guid(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestGuid != new Guid()),
entryCount: 6);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_int16(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestInt16 != 3),
entryCount: 6);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_int32(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestInt32 != 33),
entryCount: 6);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_int64(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestInt64 != 333),
entryCount: 6);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_signedbyte(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestSignedByte != 100),
entryCount: 6);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_single(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestSingle != 10.4f),
entryCount: 6);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_timespan(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestTimeSpan != new TimeSpan(3, 2, 0)),
entryCount: 6);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_unisgnedint16(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestUnsignedInt16 != 100),
entryCount: 6);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_unsignedint32(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestUnsignedInt32 != 1000),
entryCount: 6);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_unsignedint64(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestUnsignedInt64 != 10000),
entryCount: 6);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_enum(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestEnum != JsonEnum.Two),
entryCount: 3);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_enumwithintconverter(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestEnumWithIntConverter != JsonEnum.Three),
entryCount: 3);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_nullableenum1(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestNullableEnum != JsonEnum.One),
entryCount: 3);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_nullableenum2(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestNullableEnum != null),
entryCount: 3);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_nullableenumwithconverterthathandlesnulls1(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestNullableEnumWithConverterThatHandlesNulls != JsonEnum.One),
entryCount: 6);

[ConditionalTheory(Skip = "issue #29416")]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_nullableenumwithconverterthathandlesnulls2(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestNullableEnumWithConverterThatHandlesNulls != null),
entryCount: 6);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_nullableenumwithconverter1(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestNullableEnumWithIntConverter != JsonEnum.Two),
entryCount: 3);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_nullableenumwithconverter2(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestNullableEnumWithIntConverter != null),
entryCount: 3);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_nullableint321(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestNullableInt32 != 100),
entryCount: 6);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Json_predicate_on_nullableint322(bool async)
=> AssertQuery(
async,
ss => ss.Set<JsonEntityAllTypes>().Where(x => x.Reference.TestNullableInt32 != null),
entryCount: 3);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task FromSql_on_entity_with_json_basic(bool async)
Expand Down
2 changes: 2 additions & 0 deletions test/EFCore.Relational.Specification.Tests/TestModels/JsonQuery/JsonOwnedAllTypes.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ namespace Microsoft.EntityFrameworkCore.TestModels.JsonQuery;

public class JsonOwnedAllTypes
{
public string TestDefaultString { get; set; }
public string TestMaxLengthString { get; set; }
public short TestInt16 { get; set; }
public int TestInt32 { get; set; }
public long TestInt64 { get; set; }
Expand Down
8 changes: 8 additions & 0 deletions test/EFCore.Relational.Specification.Tests/TestModels/JsonQuery/JsonQueryData.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -632,6 +632,8 @@ public static IReadOnlyList<JsonEntityAllTypes> CreateJsonEntitiesAllTypes()
{
var r1 = new JsonOwnedAllTypes
{
TestDefaultString = "MyDefaultStringInReference1",
TestMaxLengthString = "Foo",
TestInt16 = -1234,
TestInt32 = -123456789,
TestInt64 = -1234567890123456789L,
Expand Down Expand Up @@ -659,6 +661,8 @@ public static IReadOnlyList<JsonEntityAllTypes> CreateJsonEntitiesAllTypes()

var r2 = new JsonOwnedAllTypes
{
TestDefaultString = "MyDefaultStringInReference2",
TestMaxLengthString = "Bar",
TestInt16 = -123,
TestInt32 = -12356789,
TestInt64 = -123567890123456789L,
Expand Down Expand Up @@ -686,6 +690,8 @@ public static IReadOnlyList<JsonEntityAllTypes> CreateJsonEntitiesAllTypes()

var c1 = new JsonOwnedAllTypes
{
TestDefaultString = "MyDefaultStringInCollection1",
TestMaxLengthString = "Baz",
TestInt16 = -12,
TestInt32 = -12345,
TestInt64 = -1234567890L,
Expand Down Expand Up @@ -713,6 +719,8 @@ public static IReadOnlyList<JsonEntityAllTypes> CreateJsonEntitiesAllTypes()

var c2 = new JsonOwnedAllTypes
{
TestDefaultString = "MyDefaultStringInCollection2",
TestMaxLengthString = "Qux",
TestInt16 = -1,
TestInt32 = -1245,
TestInt64 = -123567890L,
Expand Down
Loading

0 comments on commit 3c03f34

Please sign in to comment.