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

Draft for Json comparison function using Utf8JsonReader #2493

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

ivarne
Copy link

@ivarne ivarne commented Dec 2, 2023

System.Text.Json provides a neat api for parsing Json in a forward only way with Utf8JsonReader that just returns tokens, ignoring whitespace and decoding escape sequences in strings.

This would make a lot of sense in tests that produce json to check that it still matches the expected content, while ignoring whitespace and other artefacts that are ignored by a json deserialiser. Currently the only options are Should().Be() and string.Should().BeEquivalentTo().

There are previous issues related to Json and the use of System.Text.Json in newer dotnet versions. #2205.

I just wanted to add my suggestion for how this might be done. I hope it is OK to suggest in PR form, instead of opening an issue to discuss this somewhat different approach.

string actualJson = ...
actualJson.Should().BeJsonEquivalentTo("""
{
    "a": 1,
    "b": 2
}
""", new JsonComparatorOptions()
{
    CommentHandling = JsonCommentHandling.Skip,
    AllowTrailingCommas = false,
    MaxDepth = 64,
});

I have added a new JsonComparatorOptions struct extending System.Text.Json.JsonReaderOptions with more comparison relevant options. It is optional to provide, but I think the api needs a way to specify CommentHandeling, AllowTrailingCommas and MaxDepth, and I also have some ideas for more of these Mode options for how to compare structures.

Future ideas for possible options (currently not implemented)

  • LooseObjectOrderComparison: Ignore the order of properties in json objects so that {"a": 1, "b": 2} and {"b": 2, "a": 1} can be considered equal, even though the order is different.
  • IgnoreExtraProperites: Ignore if objects in the actual json text has properties that are missing from the actual json text. This is nice if you have a big structure, and just want to assert that your requirements is present.
  • CaseInsensitiveProperties: Compare property names in a case insensitive way.

IMPORTANT

  • If the PR touches the public API, the changes have been approved in a separate issue with the "api-approved" label.
  • The code complies with the Coding Guidelines for C#.
  • The changes are covered by unit tests which follow the Arrange-Act-Assert syntax and the naming conventions such as is used in these tests.
  • If the PR adds a feature or fixes a bug, please update the release notes with a functional description that explains what the change means to consumers of this library, which are published on the website.
  • If the PR changes the public API the changes needs to be included by running AcceptApiChanges.ps1 or AcceptApiChanges.sh.
  • If the PR affects the documentation, please include your changes in this pull request so the documentation will appear on the website.
    • Please also run ./build.sh --target spellcheck or .\build.ps1 --target spellcheck before pushing and check the good outcome

Copy link

github-actions bot commented Dec 2, 2023

Qodana for .NET

It seems all right 👌

No new problems were found according to the checks applied

💡 Qodana analysis was run in the pull request mode: only the changed files were checked
☁️ View the detailed Qodana report

Contact Qodana team

Contact us at qodana-support@jetbrains.com

@dennisdoomen
Copy link
Member

I like this. I'm just wondering whether this should be part of broader support for System.Text.Json.

@ivarne
Copy link
Author

ivarne commented Dec 5, 2023

I like this.

😁 me too 😀

I'm just wondering whether this should be part of broader support for System.Text.Json.

Maybe?, but the API in this PR does not expose anything related to a specific json library (apart from the comment handling enum). It’s just a new mode for string comparisons along with case sensitive and case insensitive.

I thought about extending the assertion to work on general objects, but ended up not liking the idea, because I’d only save the user from explicit serializing, and have issues about what serializer and settings to use.

@dennisdoomen
Copy link
Member

Maybe?, but the API in this PR does not expose anything related to a specific json library (apart from the comment handling enum). It’s just a new mode for string comparisons along with case sensitive and case insensitive.

True. What do you think about this @jnyrup ?

@ivarne
Copy link
Author

ivarne commented Dec 14, 2023

What is a proper way forward now? I think this api is a generally useful way of comparing objects that I would like to see available in the core FluentAssertions library to have it available in all projects, (in lines of code it is very small!), but as far as I can tell there is nothing stopping me from creating and maintaining a separate package to refine and get some feedback on the api and port over later.

@dennisdoomen
Copy link
Member

What is a proper way forward now? I think this api is a generally useful way of comparing objects that I would like to see available in the core FluentAssertions library to have it available in all projects, (in lines of code it is very small!), but as far as I can tell there is nothing stopping me from creating and maintaining a separate package to refine and get some feedback on the api and port over later.

I'm waiting for some feedback from @jnyrup. Remember, this is an open-source project that we work on in our private time, which is limited...

@jnyrup
Copy link
Member

jnyrup commented Dec 15, 2023

I find the approach interesting because it treats the json as json structures and not C# structures.
I.e. respecting that the JSON data types are: string, number, object, array, boolean and null.

On the other hand, I fail to see the use cases that aren't covered by Be or BeEquivalent.
I'm sure I'm missing something here.

It’s just a new mode for string comparisons along with case sensitive and case insensitive.

Case insensitive property matching sounds like something that might be useful in the general equivalency engine.

@dennisdoomen
Copy link
Member

On the other hand, I fail to see the use cases that aren't covered by Be or BeEquivalent.

In his example, comparing to JSON strings would do just that, comparing strings.

@ivarne
Copy link
Author

ivarne commented Dec 19, 2023

On the other hand, I fail to see the use cases that aren't covered by Be or BeEquivalent.
I'm sure I'm missing something here.

Funny thing. I tried to use this and it caught what I consider a bug. Note that the timezones are different, but the instance is the same.

public class Test
{
    public DateTime date { get; set; }
}

var myActualString = """{"date": "2020-02-07T10:46:36+01:00"}""";
var expectedString = """{"date": "2020-02-07T09:46:36+00:00"}""";
var myActual = JsonSerializer.Deserialize<Test>(myActualString);
var expected = JsonSerializer.Deserialize<Test>(expectedString);

// This thinks that they are equal
expected.Should().BeEquivalentTo(myActual); // Test OK

// But clients of an API might care that we now serialise with another timezone.
// The strings are not Json equivalent
expectedString.Should().BeJsonEquivalentTo(myActualString); // Throws

@ivarne
Copy link
Author

ivarne commented Dec 19, 2023

The big difference between BeJsonEquivalent and BeEquvialent is that the Json option uses only Json data types. That means a much simpler type system and tests with fewer assumptions about issues that don't show up because the difference disappear when deserialising before the equivalency check.

@dennisdoomen
Copy link
Member

expectedString.Should().BeJsonEquivalentTo(myActualString);

Actually, this shouldn't throw at all, since both dates are equivalent.

@ivarne
Copy link
Author

ivarne commented Dec 19, 2023

Actually, this shouldn't throw at all, since both dates are equivalent.

Depends on the eye looking I suppose. I think this would be nice to be informed about a change in timezone by a failing assertion. Also the JSON specification doesn't define how dates should look, so I don't know how you'd know that a string should be parsed as a date/timestamp.

@dennisdoomen
Copy link
Member

Depends on the eye looking I suppose.
Also the JSON specification doesn't define how dates should look, so I don't know how you'd know that a string should be parsed as a date/timestamp.

Those statements alone are enough to make this discussion even more complicated. I gave both examples to ChatGPT and it confirmed they were functionally equivalent, assuming you treat them as ISO 8601 dates. You're right about the lack of a specification, but people are used to seeing dates and times in that format. In fact, ISO 8601 is the default for the parses in System.Text.Json.

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

Successfully merging this pull request may close these issues.

None yet

3 participants