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

Delete by json_pointer #1248

Closed
SimonEbner opened this issue Sep 19, 2018 · 23 comments
Closed

Delete by json_pointer #1248

SimonEbner opened this issue Sep 19, 2018 · 23 comments
Labels
kind: enhancement/improvement state: please discuss please discuss the issue or vote for your favorite option state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated

Comments

@SimonEbner
Copy link

  • Describe the feature in as much detail as possible.
    Use json_pointer for erasing keys from json

  • Include sample usage where appropriate.

#include "nlohmann/json.hpp"
#include <iostream>

int main() {
    auto j = R"({"x": {"y": 1}})"_json;
    nlohmann::json::json_pointer ptr("/x/y");

    std::cout << "Delete by ptr: " << j.erase(ptr) << std::endl;
    std::cout << "j: " << j << std::endl;
}

Actual output:

Delete by ptr: 0
j: {"x":{"y":1}}

Expected output:

Delete by ptr: 1
j: {"x":{}}
@nlohmann
Copy link
Owner

Right now, the JSON Pointer is converted to a string and the erase function assuming object keys is called. It would be really nice to support JSON Pointers which should be definitely easier than #1194 as we can just return 0 if an element was not found.

@SimonEbner
Copy link
Author

@nlohmann If you point me roughly towards some piece of code, I am happy to take a look if I could provide a PR

@timprepscius
Copy link

I made a simple version of this. Just copied existing code and added the erase part. There is also a bug with json pointers when they have only a single forward slash.

I will probably eventually create a different json pointer which doesn't vectorize, but instead provide iterator functionality without vectorization. But no time at the moment.

erase_json_pointer.diff.txt

@nlohmann
Copy link
Owner

@timprepscius Could you open a PR?

@nlohmann
Copy link
Owner

@timprepscius And: what is the bug with JSON Pointers with a single slash?

@timprepscius
Copy link

Will do, will take a few days.

Bug JSON Pointer, make tests involving just "/". Crashes:
The for loop for unraveling the string is complicated, difficult to modify, so I just put in a statement looking for string length 1, after check that 1st character was '/' and returned if true.

@nlohmann
Copy link
Owner

I cannot reproduce the bug. This code runs fine:

#include "json.hpp"
#include <iostream>

using json = nlohmann::json;

int main()
{
    json::json_pointer ptr("/");
    
    json j;
    j[""] = 42;
    
    std::cout << j[ptr] << std::endl;
}

Output: 42.

@timprepscius
Copy link

    json j;
    j["sub"] = "value";

    json j2;
    j2.push_back("value");
	
    json::json_pointer psub("/sub");
    json::json_pointer pindex("/0");
    json::json_pointer prootOK("");
    json::json_pointer prootFAIL("/");

    // these are ok
    std::cout << j.dump() << std::endl;
    std::cout << j[psub] << std::endl;
    std::cout << j[prootOK] << std::endl;
    std::cout << j[prootFAIL] << std::endl;

    // prootFAIL fails, should it?
    std::cout << j2.dump() << std::endl;
    std::cout << j2[pindex] << std::endl;
    std::cout << j2[prootOK] << std::endl;
    std::cout << j2[prootFAIL] << std::endl;

prootFAIL fails for arrays. Should it? I don't know. Seems like a bug.

@nlohmann
Copy link
Owner

The exception states the problem:

[json.exception.parse_error.109] parse error: array index '' is not a number

j2 is an array, and JSON Pointer / (just a slash) means accessing a JSON object at key "" (empty string) which would be equivalent to j2[""]. The error message states that the value after the slash must be a number, because we are accessing values of an array. As the empty string is not a number, the exception is thrown.

tl;dr: The library is correct to throw an exception in this case.

@timprepscius
Copy link

Hmmm. My intuition says something is still wrong, but not in the way I thought. Maybe wrong is not the right word.

If I have:

json j;
j["hi"] = "there";
std::cout << j << std::endl;
std::cout << j["wrong"] << std::endl;
std::cout << j << std::endl;

It seems to me that this should create an exception- not create the key "wrong" with a null value.
But I have a feeling that this change would be extremely difficult, perhaps impossible to do.

@timprepscius
Copy link

Any how, has no practical impact on my side, I will make a work around to avoid this scenario, change my understanding of "root" to only be "" not "/"

@nlohmann
Copy link
Owner

This is the default behavior of operator[] for non-const values. The same code with a std::map:

std::map<std::string, std::string> j;
j["hi"] = "there";
std::cout << j.size() << std::endl;
std::cout << j["wrong"] << std::endl;
std::cout << j.size() << std::endl;

The semantics of operator[] is to add a default value for the given key if it does not exist before returning a reference to that value.

As for the JSON Pointer /: This is part of an example in RFC6901.

@timprepscius
Copy link

Yeah. Not my favorite part of the operator[] of map either.

@gregmarr
Copy link
Contributor

If std::map didn't do this, then you couldn't do j["hi"] = "there"; either.

@timprepscius
Copy link

It's true. But I find that, because of the "construct if not present" feature of std::map, I almost always use find(), !=end(), *i... and/or insert..

It just seems to work out that I almost never want std::map to default create a value. If it is there, and I'm creating it again, it probably means something wrong in code. If it is not there, then I'd want to use insert.

Personal coding style I suppose.

@timprepscius
Copy link

And actually, let's say we were to make std::map not function this way. It would definitely be a bit gross, but would it be super gross?

std::map::operator[] could return a "lazy reference", by which I mean a class which contains the key reference, and the map reference.

when the lazy reference is dereferenced, via an casting operator T(), if it does not exist it would through an exception. If it is assigned to, via operator =(T &&rhs), it would emplace the T into the map, without doing any default construction.

would this work? on a scale of grossness, I would rate this, maybe a 2/10..

you could have j["hi"] = "there".
and you could have auto a = j["look-for-a-key-which-does-not-exist-throw-exception"];

anyhow, irrelevant to json I suppose

@timprepscius
Copy link

sorry, not emplace.. etc, etc.. point made anyhow. beep borp.

@timprepscius
Copy link

One clarification needed, on the edge case of erase of root pointer. What is correct behavior?

json j;
j["hi"] = "there";
j.erase(""_json_pointer_I_forget_correct_syntax);

what should happen?

@nlohmann
Copy link
Owner

I think the best behavior would be to set j to null.

@timprepscius
Copy link

Ok, will modify

@timprepscius
Copy link

Actually, one more question.

json j;
j["hi"] = "there";
j.erase("/not-a-valid-key"_json_pointer_I_forget_correct_syntax);

So, what does this do?
In my opinion, it should make no modification and throw an exception.
But I seem to be exception happy.

Also, what does this do:
j.erase("/not-a-valid-key1/not-a-valid-leaf-key2"_json_pointer_I_forget);
In my opinion, it should also make no modification, and throw an exception
But based on previous conversations, it is plausible that you would want it to create "not-a-valid-key1"
Also, I'm exception happy.

As I'm thinking about std::map, I believe map.erase("key-which-doesnt-exist") will not throw an exception.

So I'm guessing that, no exceptions should be thrown, but, in the second example, "not-a-valid-key1" should not be created.

but what about
j.erase("/1/not-a-valid-leaf-key2"_json_pointer_I_forget); should this throw an exception, because of interpreting a map as an array? Or not?

@nlohmann nlohmann added the state: please discuss please discuss the issue or vote for your favorite option label Oct 24, 2018
@nlohmann
Copy link
Owner

I would see a JSON Pointer as an iterator rather than an object key. Therefore, I would propose to throw in case the argument to erase cannot be dereferenced.

@stale
Copy link

stale bot commented Nov 23, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Nov 23, 2018
@stale stale bot closed this as completed Nov 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement/improvement state: please discuss please discuss the issue or vote for your favorite option state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated
Projects
None yet
Development

No branches or pull requests

4 participants