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

Add ability to write JSON Merge Patch JSON from MutableJsonDocument change list #38058

Merged
merged 32 commits into from
Aug 9, 2023

Conversation

annelo-msft
Copy link
Member

This is the first change addressing #27402, and implements the core functionality described in the issue, i.e. writing JSON Merge Patch JSON from the MutableJsonDocument change list.

These types will need to be moved to internal shared source with a few helper types added in order to generate convenience APIs for Merge Patch operations as described in #37341.

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

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

A few thoughts, but overall LGTM. But would it be worth storing changes against elements as a trie (actually, even a tree split on path separators would be good enough) instead of parsing and comparing (effectively) JSON paths each time? I wouldn't hold the PR up for it, but maybe something to consider. A lot of time is spent comparing paths and making sure, as you said in a comment, "a" isn't treated as an ancestor of "abc". That said, I'm not sure it's faster or uses less memory (probably not the latter). I'm only concerned with large JSON documents, though. Maybe, practically, that isn't an issue currently.

@annelo-msft
Copy link
Member Author

annelo-msft commented Aug 8, 2023

But would it be worth storing changes against elements as a trie

The goal of this implementation is to avoid allocating data structures, with the hypothesis that span operations will outperform an allocating implementation for the majority case. I didn't implement both to compare performance, but until we know we have a perf issue here, I'd prefer to stick with this approach.

@annelo-msft annelo-msft merged commit df67b2d into Azure:main Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants