Add support for the JSON.MERGE command #1852
Conversation
src/types/json.h
Outdated
| static jsoncons::json GenerateJsonFromPath(const std::string &path, const jsoncons::json &value) { | ||
| std::vector<std::string> parts; | ||
| std::stringstream ss(path); | ||
| std::string part; | ||
|
|
||
| while (std::getline(ss, part, '.')) { | ||
| if (part != "$") { | ||
| parts.push_back(part); | ||
| } | ||
| } | ||
|
|
||
| jsoncons::json result = value; | ||
| for (auto it = parts.rbegin(); it != parts.rend(); ++it) { | ||
| jsoncons::json obj; | ||
| obj[*it] = result; | ||
| result = obj; | ||
| } | ||
|
|
||
| return result; |
There was a problem hiding this comment.
Seems it is not general enough, e.g. it cannot process path like $[0], $["hello"] or $..a.
I think here we can just use Get.
There was a problem hiding this comment.
Sorry, I was busy the last couple of days. Now, I can finally finish it.
What exactly do you mean by I think here we can just use Get.?
I was thinking about extracting keys from a string and using jsonpointer to create an object. However, it still won't handle all possible grammar.
There was a problem hiding this comment.
I mean maybe we can just use jsonpath::json_query here (as well as JsonValue::Get).
There was a problem hiding this comment.
A brief update: I have researched a bit, and it appears that we might require the path parser.
The function jsonpath::json_query can return a normalized path instead of a value, but only if a value exists at that path. The same applies to jsonpath::json_replace—it operates only when a value exists.
Moreover, the parser could be beneficial for the Set function, as it currently cannot process the following expression:
127.0.0.1:6666> JSON.SET doc $ '{"a":2}'
OK
127.0.0.1:6666> JSON.SET doc $.b '8'
OK
127.0.0.1:6666> JSON.GET doc $
"[{\"a\":2}]"Ideally, in the Merge() function, we should invoke the Set() function for this case, I presume.
I will also reach out to the maintainer of jsoncons to check whether there is a more elegant implementation of this.
There was a problem hiding this comment.
Oh, I check the document of JSON.MERGE and now get your point.
I think currently you can just implement the one that is supported by jsoncons (specifically, the case when a value exists in the jsonpath provided by the user, which can be handled by json_query) and return some error message in other cases. (you can add a TODO comment in the code.)
And after this PR being merged, you can try to cover more cases if you are interested.
And of course it is a great idea to open a discussion or issue in the jsoncons community to address this problem : )
There was a problem hiding this comment.
I've updated the method slightly and added a TODO comment. I've also initiated a discussion danielaparker/jsoncons#468
I will attempt to implement this in a separate pull request, as it could be quite complex.
Co-authored-by: Twice <twice@apache.org>
|
Seems you also need to change the method declaration to make it pass the compiler. |
|
Is a SonarCloud analysis required for a PR to pass? It seems to display a duplication error for C++ test cases, although there are no duplicates. |
Nope. It is fine to leave it alone. @git-hulk Do you have free time to help review and merge this PR? |
Sure, I would take a look soon. |
|
SonarCloud Quality Gate failed.
|
|
Thank you! @2rueSid |
|
Hi @2rueSid , in the new version of jsoncons, some new features have been added for removing nodes. Refer to danielaparker/jsoncons#467 (comment). Are you willing to improve this MERGE command using that? |
Hello. Yes for sure! I will take a look tomorrow. |
support JSON.MERGE, this closes #1815
JSON.MERGE
It's a bit messy, so I added comments. (After review I may remove them)
handling of the null value is a bit tricky so I utilize these commands to do this:
Also here is info about mergepatch
Mostly for references I used the original command: JSON.MERGE
please review @mapleFU @PragmaTwice @torwig