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

Parser interpreting the two consecutive escape sequences \"\" as a single escape sequence #786

Open
brettwinters opened this issue Apr 1, 2024 · 11 comments
Assignees
Labels

Comments

@brettwinters
Copy link

brettwinters commented Apr 1, 2024

Don't ask me why I'm doing this (but the output needs to be escaped json) but when I do the following:

DynamicExpressionParser
    .ParseLambda(typeof(object), "\"{\\\"PropertyA\\\":\\\"\\\"}\"")
    .Compile()
    .DynamicInvoke()

The parser outputs: "{\"PropertyA\":\"}" vs "{\"PropertyA\":\"\"}" expected

The only thing that seems to work is if the string is as follows "\"{\\\\\\\"PropertyA\\\\\\\":\\\\\\\"\\\\\\\"}\""

Note: Parsing "\"{\\\"PropertyA\\\":\\\"abc\\\"}\"" and "\"{\\\"PropertyA\\\":\\\" \\\"}\"" (space) works correctly

@brettwinters
Copy link
Author

#163 #307

@StefH StefH self-assigned this Apr 2, 2024
@brettwinters
Copy link
Author

BTW I'm using System.Linq.Dynamic.Core v1.3.10

@StefH
Copy link
Collaborator

StefH commented Apr 5, 2024

#788

@StefH StefH added the bug label Apr 8, 2024
@StefH
Copy link
Collaborator

StefH commented Apr 8, 2024

@brettwinters
Can you please test preview version 1.3.11-preview-01?

From: https://www.myget.org/F/system-linq-dynamic-core/api/v3/index.json

@brettwinters
Copy link
Author

Hi @StefH - I'm having some trouble using myget. Sorry about this but would you mind publishing to nuget? otherwise please could you point me to a online c# ide so I can write some tests?

@StefH
Copy link
Collaborator

StefH commented Apr 9, 2024

This page describes how to use myget:
https://github.com/WireMock-Net/WireMock.Net/wiki/MyGet-preview-versions

@brettwinters
Copy link
Author

ok, got it.

When I run these tests using 1.3.10:

[Theory]
[InlineData("\"{\\\"PropertyA\\\":\\\"abc\\\"}\"", "{\"PropertyA\":\"abc\"}")]
[InlineData("\"{\\\"PropertyA\\\":\\\" \\\"}\"", "{\"PropertyA\":\" \"}")]
[InlineData("\"{\\\"PropertyA\\\":\\\"\\\"}\"", "{\"PropertyA\":\"\"}")]
[InlineData("\"{\\\\\\\"PropertyA\\\\\\\":\\\\\\\"\\\\\\\"}\"", "{\\\"PropertyA\\\":\\\"\\\"}")]
public void GivenJsonString_WhenParseLambda_ThenShouldOutputEscapedJson(
	string jsonString,
	string expectedResult)
{
	var actualResult = DynamicExpressionParser
		.ParseLambda(typeof(object), jsonString)
		.Compile()
		.DynamicInvoke();
	
	Assert.Equal(
		expectedResult,
		actualResult?.ToString()
	);
}

They fail as originally described:
image

Then when I run the same tests using 1.3.11-preview-01

image

All pass!

Thank you very much for your quick fix, I'll look out for 1.3.11

@StefH
Copy link
Collaborator

StefH commented Apr 9, 2024

@brettwinters
Some advice is needed.

I did introduce a new config enum like below to make sure that this solution is now the default, but if users want to use the old parsing, they can configure it.

Do you think it's a good naming?

namespace System.Linq.Dynamic.Core.Config;

/// <summary>
/// Defines the types of string literal parsing that can be performed.
/// </summary>
public enum StringLiteralParsingType : byte
{
    /// <summary>
    /// Represents the default string literal parsing type. Double quotes should be escaped using the default escaping.
    /// E.G. var expression = "StaticHelper.Filter(\"UserName == \\\"x\\\"\")";
    /// 
    /// [Default]
    /// </summary>
    Default = 0,

    /// <summary>
    /// Represents a string literal parsing type where a double quotes should be escaped by two double quotes.
    /// E.G. var expression = "StaticHelper.Filter(\"UserName == "\"\"x\"\"\")";
    /// </summary>
    EscapeDoubleQuoteByTwoDoubleQuotes = 1
}

@brettwinters
Copy link
Author

Hi @StefH - escaping and unescaping serialised strings get really confusing quickly haha

The rule I think is that each pair of backslashes \\ turns into a single backslash \ and \" turns into " right? so combining, like this \\\" turns into \" (i.e. " when deserialised again into memory).

So \"UserName == \\\"x\\\"\" -> "UserName == \"x\"" (i.e. UserName == "x" in memory)

But the EscapeDoubleQuoteByTwoDoubleQuotes \"UserName == "\"\"x\"\"\" -> "UserName == "x"" (i.e. UserName == x" in memory) which to me looks wrong so I'd call it "Legacy" with an [Obsolete] attribute but I'm not sure who depends on this behaviour...

@brettwinters
Copy link
Author

brettwinters commented Apr 10, 2024

wait I just confused myself. You're right \"UserName == "\"\"x\"\"\" -> UserName == "x" - it's totally correct and valid since it's another method of escaping: Escape with backslash (\" and \\) vs escape with double quotes ("" i.e. ""abc"") (so by default EscapeWithBackslash and option EscapeWithDoubleQuotes. In the second example you're using both methods. Can you combine the parser logic to handle both cases?

@StefH
Copy link
Collaborator

StefH commented Apr 10, 2024

The parser cannot handle both cases automatically. You need to use the config.

(I did not notice the error in the example, I will verify it..)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants