Skip to content

Commit

Permalink
Remove leading $ from JSON expressions
Browse files Browse the repository at this point in the history 
And handle it in query SQL generation.

Closes dotnet#30747
  • Loading branch information
@roji
roji committed Apr 25, 2023
1 parent 796b254 commit c2f3d42
Show file tree
Hide file tree
Showing 7 changed files with 47 additions and 53 deletions.
10 changes: 4 additions & 6 deletions src/EFCore.Relational/Query/JsonQueryExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public JsonQueryExpression(
entityType,
jsonColumn,
keyPropertyMap,
path: new List<PathSegment> { new("$") },
path: Array.Empty<PathSegment>(),
type,
collection,
jsonColumn.IsNullable)
Expand Down Expand Up @@ -169,9 +169,7 @@ public virtual JsonQueryExpression BindCollectionElement(SqlExpression collectio
{
// this needs to be changed IF JsonQueryExpression will also be used for collection of primitives
// see issue #28688
Check.DebugAssert(
Path.Last().ArrayIndex == null,
"Already accessing JSON array element.");
Check.DebugAssert(Path.Count == 0 || Path[^1].ArrayIndex == null, "Already accessing JSON array element.");

var newPath = Path.ToList();
newPath.Add(new PathSegment(collectionIndexExpression));
Expand Down Expand Up @@ -215,7 +213,7 @@ public virtual void Print(ExpressionPrinter expressionPrinter)
{
expressionPrinter.Append("JsonQueryExpression(");
expressionPrinter.Visit(JsonColumn);
expressionPrinter.Append($", {string.Join("", Path.Select(e => e.ToString()))})");
expressionPrinter.Append($", {string.Join(".", Path.Select(e => e.ToString()))})");
}

/// <inheritdoc />
Expand Down Expand Up @@ -282,6 +280,6 @@ private bool KeyPropertyMapEquals(IReadOnlyDictionary<IProperty, ColumnExpressio

/// <inheritdoc />
public override int GetHashCode()
// not incorporating _keyPropertyMap into the hash, too much work
// not incorporating _keyPropertyMap into the hash, too much work
=> HashCode.Combine(EntityType, JsonColumn, IsCollection, Path, IsNullable);
}
6 changes: 1 addition & 5 deletions src/EFCore.Relational/Query/PathSegment.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,18 +48,14 @@ public PathSegment(SqlExpression arrayIndex)

/// <inheritdoc />
public override string ToString()
{
var arrayIndex = ArrayIndex switch
=> PropertyName ?? ArrayIndex switch
{
null => "",
SqlConstantExpression { Value: not null } sqlConstant => $"[{sqlConstant.Value}]",
SqlParameterExpression sqlParameter => $"[{sqlParameter.Name}]",
_ => "[(...)]"
};

return (PropertyName == "$" ? "" : ".") + (PropertyName ?? arrayIndex);
}

/// <inheritdoc />
public override bool Equals(object? obj)
=> obj is PathSegment pathSegment && Equals(pathSegment);
Expand Down
2 changes: 1 addition & 1 deletion src/EFCore.Relational/Query/SqlExpressions/JsonScalarExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ protected override void Print(ExpressionPrinter expressionPrinter)
{
expressionPrinter.Append("JsonScalarExpression(column: ");
expressionPrinter.Visit(JsonColumn);
expressionPrinter.Append($", {string.Join("", Path.Select(e => e.ToString()))})");
expressionPrinter.Append($", {string.Join(".", Path.Select(e => e.ToString()))})");
}

/// <inheritdoc />
Expand Down
2 changes: 1 addition & 1 deletion src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1616,7 +1616,7 @@ static Dictionary<JsonQueryExpression, JsonScalarExpression> BuildJsonProjection
var ordered = projections
.OrderBy(x => $"{x.JsonColumn.TableAlias}.{x.JsonColumn.Name}")
.ThenBy(x => x.Path.Count)
.ThenBy(x => x.Path[^1].ArrayIndex != null);
.ThenBy(x => x.Path.Count > 0 && x.Path[^1].ArrayIndex != null);

var needed = new List<JsonScalarExpression>();
foreach (var orderedElement in ordered)
Expand Down
55 changes: 30 additions & 25 deletions src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -392,11 +392,11 @@ when tableExpression.FindAnnotation(SqlServerAnnotationNames.TemporalOperationTy
/// </summary>
protected override Expression VisitJsonScalar(JsonScalarExpression jsonScalarExpression)
{
if (jsonScalarExpression.Path.Count == 1
&& jsonScalarExpression.Path[0].ToString() == "$")
// TODO: Look into not having empty JsonScalarExpression
var path = jsonScalarExpression.Path;
if (path.Count == 0)
{
Visit(jsonScalarExpression.JsonColumn);

return jsonScalarExpression;
}

Expand All @@ -414,33 +414,38 @@ protected override Expression VisitJsonScalar(JsonScalarExpression jsonScalarExp

Visit(jsonScalarExpression.JsonColumn);

Sql.Append(", '");
Sql.Append(", '$");
foreach (var pathSegment in jsonScalarExpression.Path)
{
if (pathSegment.PropertyName != null)
switch (pathSegment)
{
Sql.Append((pathSegment.PropertyName == "$" ? "" : ".") + pathSegment.PropertyName);
case { PropertyName: string propertyName }:
Sql.Append(".").Append(propertyName);
break;

case { ArrayIndex: SqlExpression arrayIndex }:
Sql.Append("[");

if (arrayIndex is SqlConstantExpression)
{
Visit(pathSegment.ArrayIndex);
}
else
{
Sql.Append("' + CAST(");
Visit(arrayIndex);
Sql.Append(" AS ");
Sql.Append(_typeMappingSource.GetMapping(typeof(string)).StoreType);
Sql.Append(") + '");
}

Sql.Append("]");
break;

default:
throw new ArgumentOutOfRangeException();
}

if (pathSegment.ArrayIndex != null)
{
Sql.Append("[");

if (pathSegment.ArrayIndex is SqlConstantExpression)
{
Visit(pathSegment.ArrayIndex);
}
else
{
Sql.Append("' + CAST(");
Visit(pathSegment.ArrayIndex);
Sql.Append(" AS ");
Sql.Append(_typeMappingSource.GetMapping(typeof(string)).StoreType);
Sql.Append(") + '");
}

Sql.Append("]");
}
}

Sql.Append("')");
Expand Down
9 changes: 2 additions & 7 deletions src/EFCore.Sqlite.Core/Query/Internal/SqliteQuerySqlGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,13 +126,8 @@ private Expression VisitRegexp(RegexpExpression regexpExpression)
/// </summary>
protected override Expression VisitJsonScalar(JsonScalarExpression jsonScalarExpression)
{
// 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;

// TODO: Look into not having empty JsonScalarExpression
var path = jsonScalarExpression.Path;
if (path.Count == 0)
{
Visit(jsonScalarExpression.JsonColumn);
Expand Down
16 changes: 8 additions & 8 deletions test/EFCore.Relational.Specification.Tests/Query/JsonQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -761,7 +761,7 @@ public virtual async Task Json_collection_element_access_in_projection_using_unt
ss => ss.Set<JsonEntityBasic>().Select(x => x.OwnedCollectionRoot[MyMethod(x.Id)]).AsNoTracking()))).Message;

Assert.Equal(
CoreStrings.TranslationFailed("JsonQueryExpression(j.OwnedCollectionRoot, $)"),
CoreStrings.TranslationFailed("JsonQueryExpression(j.OwnedCollectionRoot, )"),
message);
}

Expand All @@ -775,7 +775,7 @@ public virtual async Task Json_collection_element_access_in_projection_using_unt
ss => ss.Set<JsonEntityBasic>().Select(x => x.OwnedCollectionRoot[0].OwnedReferenceBranch.OwnedCollectionLeaf[MyMethod(x.Id)]).AsNoTracking()))).Message;

Assert.Equal(
CoreStrings.TranslationFailed("JsonQueryExpression(j.OwnedCollectionRoot, $.[0].OwnedReferenceBranch.OwnedCollectionLeaf)"),
CoreStrings.TranslationFailed("JsonQueryExpression(j.OwnedCollectionRoot, [0].OwnedReferenceBranch.OwnedCollectionLeaf)"),
message);
}

Expand Down Expand Up @@ -967,7 +967,7 @@ public virtual async Task Json_collection_element_access_manual_Element_at_and_p
CollectionElement = x.OwnedCollectionRoot[prm].OwnedCollectionBranch.Select(xx => "Foo").ElementAt(0)
})))).Message;

Assert.Equal(CoreStrings.TranslationFailed("JsonQueryExpression(j.OwnedCollectionRoot, $.[__prm_0].OwnedCollectionBranch)"), message);
Assert.Equal(CoreStrings.TranslationFailed("JsonQueryExpression(j.OwnedCollectionRoot, [__prm_0].OwnedCollectionBranch)"), message);
}

[ConditionalTheory]
Expand All @@ -984,7 +984,7 @@ public virtual async Task Json_collection_element_access_manual_Element_at_and_p
CollectionElement = x.OwnedCollectionRoot[prm + x.Id].OwnedCollectionBranch.Select(xx => x.Id).ElementAt(0)
})))).Message;

Assert.Equal(CoreStrings.TranslationFailed("JsonQueryExpression(j.OwnedCollectionRoot, $.[(...)].OwnedCollectionBranch)"), message);
Assert.Equal(CoreStrings.TranslationFailed("JsonQueryExpression(j.OwnedCollectionRoot, [(...)].OwnedCollectionBranch)"), message);
}

[ConditionalTheory]
Expand All @@ -1000,7 +1000,7 @@ public virtual async Task Json_collection_element_access_manual_Element_at_and_p
CollectionElement = x.OwnedCollectionRoot.Select(xx => x.OwnedReferenceRoot).ElementAt(0)
})))).Message;

Assert.Equal(CoreStrings.TranslationFailed("JsonQueryExpression(j.OwnedCollectionRoot, $)"), message);
Assert.Equal(CoreStrings.TranslationFailed("JsonQueryExpression(j.OwnedCollectionRoot, )"), message);
}

[ConditionalTheory]
Expand All @@ -1016,7 +1016,7 @@ public virtual async Task Json_collection_element_access_manual_Element_at_and_p
CollectionElement = x.OwnedCollectionRoot.Select(xx => x.OwnedCollectionRoot).ElementAt(0)
})))).Message;

Assert.Equal(CoreStrings.TranslationFailed("JsonQueryExpression(j.OwnedCollectionRoot, $)"), message);
Assert.Equal(CoreStrings.TranslationFailed("JsonQueryExpression(j.OwnedCollectionRoot, )"), message);
}

[ConditionalTheory]
Expand All @@ -1032,7 +1032,7 @@ public virtual async Task Json_collection_element_access_manual_Element_at_and_p
CollectionElement = x.OwnedCollectionRoot.Select(xx => new { xx.OwnedReferenceBranch }).ElementAt(0)
})))).Message;

Assert.Equal(CoreStrings.TranslationFailed("JsonQueryExpression(j.OwnedCollectionRoot, $)"), message);
Assert.Equal(CoreStrings.TranslationFailed("JsonQueryExpression(j.OwnedCollectionRoot, )"), message);
}

[ConditionalTheory]
Expand All @@ -1048,7 +1048,7 @@ public virtual async Task Json_collection_element_access_manual_Element_at_and_p
CollectionElement = x.OwnedCollectionRoot.Select(xx => new JsonEntityBasic { Id = x.Id }).ElementAt(0)
})))).Message;

Assert.Equal(CoreStrings.TranslationFailed("JsonQueryExpression(j.OwnedCollectionRoot, $)"), message);
Assert.Equal(CoreStrings.TranslationFailed("JsonQueryExpression(j.OwnedCollectionRoot, )"), message);
}

[ConditionalTheory]
Expand Down

0 comments on commit c2f3d42

Please # to comment.