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.

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

feat(83547): Adding escaping of property name for GetPath. #84688

Conversation

Maximys
Copy link
Contributor

@Maximys Maximys commented Apr 12, 2023

Current PR fix problem with unescaped single quote, but it does not fix problem with shorthand property.
I did so because Newtonsoft.Json 13.0.3 have the same behavior:

JToken jToken;

jToken = JToken.Parse("""{"$myRoot": {"myRoot'child": {"secondLevelChildWithoutEscaping": "value1"}}}""");

jToken = jToken["$myRoot"]["myRoot'child"]["secondLevelChildWithoutEscaping"];

Console.WriteLine(jToken.Path);

output: "$myRoot['myRoot'child'].secondLevelChildWithoutEscaping"

I think, Newtonsoft is is etalon implementation of JSON library, but nobody of us protected from mistake.
Add some comment if this behaviour invalid.

Contributes to #83547

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 12, 2023
@ghost
Copy link

ghost commented Apr 12, 2023

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

Current PR fix problem with unescaped single quote, but it does not fix problem with shorthand property.
I did so because Newtonsoft.Json 13.0.3 have the same behavior:

JToken jToken;

jToken = JToken.Parse("""{"$myRoot": {"myRoot'child": {"secondLevelChildWithoutEscaping": "value1"}}}""");

jToken = jToken["$myRoot"]["myRoot'child"]["secondLevelChildWithoutEscaping"];

Console.WriteLine(jToken.Path);

output: "$myRoot['myRoot'child'].secondLevelChildWithoutEscaping"

I think, Newtonsoft is is etalon implementation of JSON library, but nobody of us protected from mistake.
Add some comment if this behaviour invalid.

Author: Maximys
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@Maximys Maximys force-pushed the feature/83547-working-with-escapable-characters-inside-json-path branch from 9660f1c to b4b8b21 Compare April 12, 2023 13:05
string returnValue;

returnValue = rawPropertyName
.Replace(SingleQuote, EscapedSingleQuote);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to use here Regex.Replace(String, String, String, RegexOptions), but I could not understand how to use System.Text.RegularExpressions assembly from current.

@Maximys
Copy link
Contributor Author

Maximys commented May 3, 2023

@layomia , could you review current PR?

@tarekgh
Copy link
Member

tarekgh commented May 10, 2023

@Maximys looks there are some related test failures because of this change. Could you please have a look?

@tarekgh tarekgh added the needs-author-action An issue or pull request that requires more info or actions from the author. label May 10, 2023
@gregsdennis
Copy link
Contributor

Fixes #83547

If the shorthand problem isn't being addressed, the issue should remain open. This PR isn't a complete fix.

@Maximys
Copy link
Contributor Author

Maximys commented May 14, 2023

@tarekgh , ok, I'll try to look

@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label May 14, 2023
@Maximys Maximys force-pushed the feature/83547-working-with-escapable-characters-inside-json-path branch 3 times, most recently from 2d3bd99 to 5cd4b25 Compare May 15, 2023 12:16
@Maximys
Copy link
Contributor Author

Maximys commented May 16, 2023

tarekgh , failed tests was linked with invalid processing of quote('"') symbol by JsonSourceGenerator. I had removed this symbol from JsonPropertyName of ClassWithEscapablePropertyName and all tests was passed.
Now I fixed bug of JsonSourceGenerator by separate commit ea4813ebef700b1b72d00aaeacc8e24734bf3c5e.
@tarekgh, @dotnet/area-system-text-json could you review current PR again

@tarekgh
Copy link
Member

tarekgh commented May 16, 2023

@layomia @eiriktsarpalis could you please have a look at this change. The change is reasonable to me, but I want to ensure this is not breaking in anyway. Thanks!

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Escaping should consistent with what JsonSerializerOptions.Encoder is doing.

@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Jun 22, 2023
@Maximys Maximys force-pushed the feature/83547-working-with-escapable-characters-inside-json-path branch from ea4813e to 75deded Compare June 26, 2023 08:56
@Maximys
Copy link
Contributor Author

Maximys commented Jun 26, 2023

@eiriktsarpalis , could I ask little question, which is not linked with current PR? I want try to fix #86031 by separate PR. I want to use FluentAssertions for check result of my fix. As I can see, Rutime currently use this library only for test installer. May I add FluentAssertions to TestUtilities inside Common?

@eiriktsarpalis
Copy link
Member

May I add FluentAssertions to TestUtilities inside Common?

runtime only uses vanilla xunit assertions + a minimal amount of extensions defined in the common folder. I would suggest sticking with the existing style.

@eiriktsarpalis eiriktsarpalis added this to the 9.0.0 milestone Aug 7, 2023
Comment on lines 1240 to 1253
{
string returnValue;

if (value is null)
{
returnValue = "null";
}
else
{
returnValue = SyntaxFactory.Literal(value).ToFullString();
}

return returnValue;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this can be simplified

Suggested change
{
string returnValue;
if (value is null)
{
returnValue = "null";
}
else
{
returnValue = SyntaxFactory.Literal(value).ToFullString();
}
return returnValue;
}
=> value is null ? "null" : SyntaxFactory.Literal(value).ToFullString();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by 51fcbb0

@@ -1000,7 +1000,6 @@ private bool IsValidDataExtensionPropertyType(ITypeSymbol type)
{
ImmutableArray<TypedConstant> ctorArgs = attributeData.ConstructorArguments;
jsonPropertyName = (string)ctorArgs[0].Value!;
// Null check here is done at runtime within JsonSerializer.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change related?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, fixed by amend

…pertyName(string, JavaScriptEncoder?)" implementation."

This reverts commit d346c16f342c282a456c18d7f4f95e88f43a6cad.
@Maximys Maximys force-pushed the feature/83547-working-with-escapable-characters-inside-json-path branch from 75deded to 4a0d7b1 Compare October 4, 2023 08:44
@gregsdennis
Copy link
Contributor

Fixes #83547

If the shorthand problem isn't being addressed, the issue should remain open. This PR isn't a complete fix.

@Maximys of this isn't going to be addressed, can you remove the "fixes" text from the opening comment, please?

Alternatively, a secondary issue should be opened to cover the shorthand problem.

eiriktsarpalis
eiriktsarpalis previously approved these changes Oct 5, 2023
Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@eiriktsarpalis
Copy link
Member

This PR isn't a complete fix.

@gregsdennis Presumably you mean the bracketing of properties starting with the dollar sign? Or is it missing something else?

@gregsdennis
Copy link
Contributor

gregsdennis commented Oct 5, 2023

Yes, that. What's being generated isn't a valid path.

If that's not fixed, the issue isn't resolved.

@ericstj ericstj assigned eiriktsarpalis and unassigned layomia Oct 5, 2023
@eiriktsarpalis
Copy link
Member

but it does not fix problem with shorthand property.
I did so because Newtonsoft.Json 13.0.3 have the same behavior:

@Maximys I don't think we need to base our behavior on what Json.NET is doing.

yield return new object[]
{
JsonNode.Parse("""{"$myRoot":{"foo['bar":"baz"}}""")["$myRoot"]["foo['bar"],
"$.$myRoot['foo[\\u0027bar']"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm reading these tests right, they're still not creating the correct paths. $myRoot isn't valid for the dot syntax and needs to be bracketed.

Copy link
Member

@eiriktsarpalis eiriktsarpalis Oct 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Maximys would it be possible to revisit this? Thanks!

@eiriktsarpalis eiriktsarpalis added the needs-author-action An issue or pull request that requires more info or actions from the author. label Oct 29, 2023
@eiriktsarpalis eiriktsarpalis dismissed their stale review October 29, 2023 18:30

Changes requested

@ghost ghost added the no-recent-activity label Nov 12, 2023
@ghost
Copy link

ghost commented Nov 12, 2023

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

@ghost
Copy link

ghost commented Nov 27, 2023

This pull request will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the pull request, but please note that it will be locked if it remains inactive for another 30 days.

@ghost ghost closed this Nov 27, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 27, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json community-contribution Indicates that the PR has been added by a community member needs-author-action An issue or pull request that requires more info or actions from the author. no-recent-activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants