From f88abe0de5e638fb39b3189380f41111255670d9 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Sun, 23 Apr 2023 11:41:03 +0200 Subject: [PATCH] Use -> operator for SQLite JsonScalarExpression Closes #30334 --- .../Query/QuerySqlGenerator.cs | 18 +-- .../SqlExpressions/JsonScalarExpression.cs | 2 +- .../Query/SqlExpressions/SelectExpression.cs | 2 +- .../Query/Internal/SqliteQuerySqlGenerator.cs | 109 +++++++++++++----- .../Query/JsonQuerySqliteTest.cs | 43 ++++++- 5 files changed, 131 insertions(+), 43 deletions(-) diff --git a/src/EFCore.Relational/Query/QuerySqlGenerator.cs b/src/EFCore.Relational/Query/QuerySqlGenerator.cs index 07855455d00..94ce376d396 100644 --- a/src/EFCore.Relational/Query/QuerySqlGenerator.cs +++ b/src/EFCore.Relational/Query/QuerySqlGenerator.cs @@ -560,32 +560,32 @@ protected virtual void CheckComposableSqlTrimmed(ReadOnlySpan sql) /// protected override Expression VisitSqlBinary(SqlBinaryExpression sqlBinaryExpression) { - var requiresBrackets = RequiresParentheses(sqlBinaryExpression, sqlBinaryExpression.Left); + var requiresParentheses = RequiresParentheses(sqlBinaryExpression, sqlBinaryExpression.Left); - if (requiresBrackets) + if (requiresParentheses) { _relationalCommandBuilder.Append("("); } Visit(sqlBinaryExpression.Left); - if (requiresBrackets) + if (requiresParentheses) { _relationalCommandBuilder.Append(")"); } _relationalCommandBuilder.Append(GetOperator(sqlBinaryExpression)); - requiresBrackets = RequiresParentheses(sqlBinaryExpression, sqlBinaryExpression.Right); + requiresParentheses = RequiresParentheses(sqlBinaryExpression, sqlBinaryExpression.Right); - if (requiresBrackets) + if (requiresParentheses) { _relationalCommandBuilder.Append("("); } Visit(sqlBinaryExpression.Right); - if (requiresBrackets) + if (requiresParentheses) { _relationalCommandBuilder.Append(")"); } @@ -765,14 +765,14 @@ protected override Expression VisitSqlUnary(SqlUnaryExpression sqlUnaryExpressio case ExpressionType.Convert: { _relationalCommandBuilder.Append("CAST("); - var requiresBrackets = RequiresParentheses(sqlUnaryExpression, sqlUnaryExpression.Operand); - if (requiresBrackets) + var requiresParentheses = RequiresParentheses(sqlUnaryExpression, sqlUnaryExpression.Operand); + if (requiresParentheses) { _relationalCommandBuilder.Append("("); } Visit(sqlUnaryExpression.Operand); - if (requiresBrackets) + if (requiresParentheses) { _relationalCommandBuilder.Append(")"); } diff --git a/src/EFCore.Relational/Query/SqlExpressions/JsonScalarExpression.cs b/src/EFCore.Relational/Query/SqlExpressions/JsonScalarExpression.cs index 7e6f047333d..7629c0c1d7e 100644 --- a/src/EFCore.Relational/Query/SqlExpressions/JsonScalarExpression.cs +++ b/src/EFCore.Relational/Query/SqlExpressions/JsonScalarExpression.cs @@ -44,7 +44,7 @@ internal JsonScalarExpression( } /// - /// The column containg JSON value. + /// The column containing the JSON value. /// public virtual ColumnExpression JsonColumn { get; } diff --git a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs index e3e46f7912c..e39a89f4ad5 100644 --- a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs +++ b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs @@ -1592,7 +1592,7 @@ static Dictionary BuildJsonProjection { // force reference comparison for this one, even if we implement custom equality for JsonQueryExpression in the future var deduplicationMap = new Dictionary(ReferenceEqualityComparer.Instance); - if (projections.Count() > 0) + if (projections.Any()) { var ordered = projections .OrderBy(x => $"{x.JsonColumn.TableAlias}.{x.JsonColumn.Name}") diff --git a/src/EFCore.Sqlite.Core/Query/Internal/SqliteQuerySqlGenerator.cs b/src/EFCore.Sqlite.Core/Query/Internal/SqliteQuerySqlGenerator.cs index 997e5f58b12..db25b86d71b 100644 --- a/src/EFCore.Sqlite.Core/Query/Internal/SqliteQuerySqlGenerator.cs +++ b/src/EFCore.Sqlite.Core/Query/Internal/SqliteQuerySqlGenerator.cs @@ -126,56 +126,106 @@ private Expression VisitRegexp(RegexpExpression regexpExpression) /// protected override Expression VisitJsonScalar(JsonScalarExpression jsonScalarExpression) { - if (jsonScalarExpression.Path.Count == 1 - && jsonScalarExpression.Path[0].ToString() == "$") + // TODO: This trims the leading $ PathSegment, which isn't actually part of the path (but rather path of the JSONPATH language + // used to generate the path in SQL for *some* databases). + // Instead, we should not be producing JsonScalarExpression at all with the leading $. + var path = jsonScalarExpression.Path is [{ PropertyName: "$" }, ..] + ? jsonScalarExpression.Path.Skip(1).ToArray() + : jsonScalarExpression.Path; + + if (path.Count == 0) { Visit(jsonScalarExpression.JsonColumn); - return jsonScalarExpression; } - Sql.Append("json_extract("); - Visit(jsonScalarExpression.JsonColumn); - Sql.Append(", '"); - foreach (var pathSegment in jsonScalarExpression.Path) + var inJsonpathString = false; + + for (var i = 0; i < path.Count; i++) { - if (pathSegment.PropertyName != null) - { - Sql.Append((pathSegment.PropertyName == "$" ? "" : ".") + pathSegment.PropertyName); - } + var pathSegment = path[i]; + var isLast = i == path.Count - 1; - if (pathSegment.ArrayIndex != null) + switch (pathSegment) { - Sql.Append("["); + case { PropertyName: string propertyName }: + if (inJsonpathString) + { + Sql.Append(".").Append(propertyName); + continue; + } - if (pathSegment.ArrayIndex is SqlConstantExpression) - { - Visit(pathSegment.ArrayIndex); - } - else - { - Sql.Append("' || "); - if (pathSegment.ArrayIndex is SqlParameterExpression) + Sql.Append(" ->> "); + + // No need to start a $. JSONPATH string if we're the last segment or the next segment isn't a constant + if (isLast || path[i + 1] is { ArrayIndex: not null and not SqlConstantExpression }) { + Sql.Append("'").Append(propertyName).Append("'"); + continue; + } + + Sql.Append("'$.").Append(propertyName); + inJsonpathString = true; + continue; + + case { ArrayIndex: SqlConstantExpression arrayIndex }: + if (inJsonpathString) + { + Sql.Append("["); Visit(pathSegment.ArrayIndex); + Sql.Append("]"); + continue; } - else + + Sql.Append(" ->> "); + + // No need to start a $. JSONPATH string if we're the last segment or the next segment isn't a constant + if (isLast || path[i + 1] is { ArrayIndex: not null and not SqlConstantExpression }) + { + Visit(arrayIndex); + continue; + } + + Sql.Append("'$["); + Visit(arrayIndex); + Sql.Append("]"); + inJsonpathString = true; + continue; + + default: + if (inJsonpathString) + { + Sql.Append("'"); + inJsonpathString = false; + } + + Sql.Append(" ->> "); + + Check.DebugAssert(pathSegment.ArrayIndex is not null, "pathSegment.ArrayIndex is not null"); + + var requiresParentheses = RequiresParentheses(jsonScalarExpression, pathSegment.ArrayIndex); + if (requiresParentheses) { Sql.Append("("); - Visit(pathSegment.ArrayIndex); - Sql.Append(")"); } - Sql.Append(" || '"); - } + Visit(pathSegment.ArrayIndex); + + if (requiresParentheses) + { + Sql.Append(")"); + } - Sql.Append("]"); + continue; } } - Sql.Append("')"); + if (inJsonpathString) + { + Sql.Append("'"); + } return jsonScalarExpression; } @@ -196,7 +246,7 @@ protected override bool TryGetOperatorInfo(SqlExpression expression, out int pre ExpressionType.Multiply => (900, true), ExpressionType.Divide => (900, false), ExpressionType.Modulo => (900, false), - ExpressionType.Add when sqlBinaryExpression.Type == typeof(string) => (1100, true), + ExpressionType.Add when sqlBinaryExpression.Type == typeof(string) => (1000, true), ExpressionType.Add when sqlBinaryExpression.Type != typeof(string) => (800, true), ExpressionType.Subtract => (800, false), ExpressionType.And => (600, true), @@ -227,6 +277,7 @@ protected override bool TryGetOperatorInfo(SqlExpression expression, out int pre CollateExpression => (1100, false), LikeExpression => (500, false), + JsonScalarExpression => (1000, true), _ => default, }; diff --git a/test/EFCore.Sqlite.FunctionalTests/Query/JsonQuerySqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/Query/JsonQuerySqliteTest.cs index 517b9c1aae6..87530f53119 100644 --- a/test/EFCore.Sqlite.FunctionalTests/Query/JsonQuerySqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/Query/JsonQuerySqliteTest.cs @@ -16,6 +16,29 @@ public JsonQuerySqliteTest(JsonQuerySqliteFixture fixture, ITestOutputHelper tes //Fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper); } + public override async Task Json_scalar_length(bool async) + { + await base.Json_scalar_length(async); + + AssertSql( +""" +SELECT "j"."Name" +FROM "JsonEntitiesBasic" AS "j" +WHERE length("j"."OwnedReferenceRoot" ->> 'Name') > 2 +"""); + } + + public override async Task Basic_json_projection_enum_inside_json_entity(bool async) + { + await base.Basic_json_projection_enum_inside_json_entity(async); + + AssertSql( +""" +SELECT "j"."Id", "j"."OwnedReferenceRoot" ->> '$.OwnedReferenceBranch.Enum' AS "Enum" +FROM "JsonEntitiesBasic" AS "j" +"""); + } + public override async Task Project_json_entity_FirstOrDefault_subquery(bool async) => Assert.Equal( SqliteStrings.ApplyNotSupported, @@ -67,6 +90,20 @@ await AssertQuery( """); } + public override async Task Json_collection_element_access_in_predicate_nested_mix(bool async) + { + await base.Json_collection_element_access_in_predicate_nested_mix(async); + + AssertSql( +""" +@__prm_0='0' + +SELECT "j"."Id", "j"."EntityBasicId", "j"."Name", "j"."OwnedCollectionRoot", "j"."OwnedReferenceRoot" +FROM "JsonEntitiesBasic" AS "j" +WHERE "j"."OwnedCollectionRoot" ->> '$[1].OwnedCollectionBranch' ->> @__prm_0 ->> 'OwnedCollectionLeaf' ->> ("j"."Id" - 1) ->> 'SomethingSomething' = 'e1_c2_c1_c1' +"""); + } + public override async Task Json_predicate_on_bool_converted_to_int_zero_one(bool async) { await base.Json_predicate_on_bool_converted_to_int_zero_one(async); @@ -75,7 +112,7 @@ public override async Task Json_predicate_on_bool_converted_to_int_zero_one(bool """ SELECT "j"."Id", "j"."Reference" FROM "JsonEntitiesConverters" AS "j" -WHERE json_extract("j"."Reference", '$.BoolConvertedToIntZeroOne') = 1 +WHERE "j"."Reference" ->> 'BoolConvertedToIntZeroOne' = 1 """); } @@ -87,7 +124,7 @@ public override async Task Json_predicate_on_bool_converted_to_string_True_False """ SELECT "j"."Id", "j"."Reference" FROM "JsonEntitiesConverters" AS "j" -WHERE json_extract("j"."Reference", '$.BoolConvertedToStringTrueFalse') = 'True' +WHERE "j"."Reference" ->> 'BoolConvertedToStringTrueFalse' = 'True' """); } @@ -99,7 +136,7 @@ public override async Task Json_predicate_on_bool_converted_to_string_Y_N(bool a """ SELECT "j"."Id", "j"."Reference" FROM "JsonEntitiesConverters" AS "j" -WHERE json_extract("j"."Reference", '$.BoolConvertedToStringYN') = 'Y' +WHERE "j"."Reference" ->> 'BoolConvertedToStringYN' = 'Y' """); }