Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove leading $ from JSON expressions #30763

Merged
1 commit merged into from
Apr 26, 2023
Merged

Remove leading $ from JSON expressions #30763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions src/EFCore.Relational/Query/JsonQueryExpression.cs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1597,7 +1597,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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,11 +318,11 @@ when tableExpression.FindAnnotation(SqlServerAnnotationNames.TemporalOperationTy
/// </summary>
protected override Expression VisitJsonScalar(JsonScalarExpression jsonScalarExpression)
{
if (jsonScalarExpression.Path.Count == 1
&& jsonScalarExpression.Path[0].ToString() == "$")
// TODO: Stop producing empty JsonScalarExpressions, #30768
var path = jsonScalarExpression.Path;
if (path.Count == 0)
{
Visit(jsonScalarExpression.JsonColumn);

return jsonScalarExpression;
}

Expand All @@ -340,33 +340,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
Open in desktop
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: Stop producing empty JsonScalarExpressions, #30768
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
Open in desktop
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