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

Support System.Text.Json #96

Closed
niemyjski opened this issue Mar 25, 2022 · 4 comments
Closed

Support System.Text.Json #96

niemyjski opened this issue Mar 25, 2022 · 4 comments

Comments

@niemyjski
Copy link
Member

niemyjski commented Mar 25, 2022

Currently users of this library using System.Text.Json (STJ) cannot return search aggregation results (from redis cache, or from api endpoints) due to serialization dependencies on Newtonsoft.Json. Our goal is to update the Foundatio.Repositories to work with System.Text.Json. However, we don’t want to break other applications so we need to support Newtonsoft.Json by leaving the existing JsonConverters in place unless we can find a way to support both out of the box without the dependency or an easy way for it to be configured externally.

I see this happening in the following steps:

  1. Copy the following code snippet to end of AggregationQueryTests.GetTermAggregationsWithTopHitsAsync().
            string systemTextJson = System.Text.Json.JsonSerializer.Serialize(result);
            Assert.Equal(json, systemTextJson);
            roundTripped = System.Text.Json.JsonSerializer.Deserialize<CountResult>(systemTextJson);
            Assert.Equal(10, roundTripped.Total);
            Assert.Equal(1, roundTripped.Aggregations.Count);
            Assert.Equal(10, roundTripped.Aggregations.Terms<int>("terms_age").Buckets.Count);
            bucket = roundTripped.Aggregations.Terms<int>("terms_age").Buckets.First(f => f.Key == 19);
            Assert.Equal(1, bucket.Total);
  1. Add custom converter to handle writing polymorphic json. The nest team is doing something like this: https://github.com/elastic/elasticsearch-net/blob/main/src/Elastic.Clients.Elasticsearch/_Generated/Types/Aggregations/TermsAggregation.g.cs#L27 for one of there aggretaions. I think you'd pass this converter in options of the first call to Serialize in the snippet above. This will solve the first assert.
  2. You'd need to add and convert the existing JsonConverters in the Foundatio.Repositories.Utility to System.Text.Json and then add the json converter attributes next to the existing converter attributes. This will fix the round trip part of reading the json text.
  3. Request PR Review

Not sure if this link will help on the converters: https://bengribaudo.com/blog/2022/02/22/6569/recursive-polymorphic-deserialization-with-system-text-json

@ejsmith
Copy link
Contributor

ejsmith commented Mar 26, 2022

I removed some usages of JSON.NET down to where our only usage is pretty much the JSON patch support.

Spent some time taking a look at json-everything's JsonPatch support and it only uses JSON Pointers for paths and does not support some of the things we are doing where we are allowing full JSON Path support where we can do things like $.books[?(@.author == 'John Steinbeck')] to remove all array elements from the books array that has an author sub-property that matches John Steinbeck. So basically we would need to make major changes to their libraries or talk them into supporting JSON Path expressions even though the unofficial JSON Patch spec only supports JSON Pointer.

@ejsmith
Copy link
Contributor

ejsmith commented Mar 26, 2022

Found this issue where someone asked him and he said no to supporting JSON Path.

@niemyjski
Copy link
Member Author

Comment for previous history:

For Json Patching it would be nice to see if we can get it to work without a dependency or we can internalize the dependency. I see several spots doing patching

@niemyjski
Copy link
Member Author

Initial support for STJ was added in #97 We still need to support Patching

@niemyjski niemyjski changed the title Support System.Text.Json Support System.Text.Json Patching Jun 22, 2022
@niemyjski niemyjski changed the title Support System.Text.Json Patching Support System.Text.Json Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants