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

Parent pointer not properly set when using update() #3007

Closed
1 of 3 tasks
AnthonyVH opened this issue Sep 9, 2021 · 3 comments · Fixed by #3008
Closed
1 of 3 tasks

Parent pointer not properly set when using update() #3007

AnthonyVH opened this issue Sep 9, 2021 · 3 comments · Fixed by #3008
Assignees
Labels
confirmed kind: bug release item: 🐛 bug fix solution: proposed fix a fix for the issue has been proposed and waits for confirmation

Comments

@AnthonyVH
Copy link
Contributor

AnthonyVH commented Sep 9, 2021

When update() is used to insert new elements into an nlohmann::json, the parent pointers inside the newly inserted elements don't seem to be properly set.

What is the issue you have?

When parent pointers are not set correctly, an assert goes off when check_invariants() is later called on the object. This only happens when JSON_DIAGNOSTICS is enabled.

This might be related to #2926. Although as far as I can see, this has nothing to do with serialization.

Please describe the steps to reproduce the issue.

Live example: https://godbolt.org/z/Ms5hP35fW

Can you provide a small but working code example?

Same as the live code:

#include <nlohmann/json.hpp>

int main () {
  nlohmann::json root = nlohmann::json::array();
  nlohmann::json lower = nlohmann::json::object();

  {
    nlohmann::json lowest = nlohmann::json::object();
    lowest["one"] = 1;
    
    lower.update(lowest);
  }

  root.push_back(lower);
}

What is the expected behavior?

Parent pointers should be properly set. As a result no assert should go off.

And what is the actual behavior instead?

Parent pointers are not set, assertion goes off as a result.

Which compiler and operating system are you using?

Tested with:

  • g++ 10.3 on Gentoo 2.6
  • g++ 11.1 on Compiler Explorer (godbolt.org)

Which version of the library did you use?

  • latest release version 3.10.2
  • other release - please state the version: ___
  • the develop branch
@AnthonyVH
Copy link
Contributor Author

Modifing update() to the following fixes the bug:

for (auto it = j.cbegin(); it != j.cend(); ++it)
{
    m_value.object->operator[](it.key()) = it.value();
#if JSON_DIAGNOSTICS
    m_value.object->operator[](it.key()).m_parent = this;
#endif
}

I can make a pull request with an extra unit test, but probably won't be able to get that done today.

@nlohmann nlohmann added confirmed solution: proposed fix a fix for the issue has been proposed and waits for confirmation state: waiting for PR labels Sep 9, 2021
@nlohmann
Copy link
Owner

nlohmann commented Sep 9, 2021

Thanks for reporting, analyzing, and proposing a fix! It would be great if you could open a PR for this, as I won't have time to work on this before Sunday.

@AnthonyVH
Copy link
Contributor Author

The fix was super trivial as was the test, so here you go, PR submitted 😃.

AnthonyVH added a commit to AnthonyVH/json that referenced this issue Sep 9, 2021
AnthonyVH added a commit to AnthonyVH/json that referenced this issue Sep 10, 2021
…so extend to test both update() functions.
AnthonyVH added a commit to AnthonyVH/json that referenced this issue Sep 10, 2021
AnthonyVH added a commit to AnthonyVH/json that referenced this issue Sep 12, 2021
@nlohmann nlohmann linked a pull request Sep 12, 2021 that will close this issue
4 tasks
nlohmann pushed a commit that referenced this issue Sep 12, 2021
…3008)

* Set parent pointers for values inserted via update() (fixes #3007).

* Moved test for #3007 to proper file.

* Enable access to private members in diagnostics unit tests.

* Make style consistent with rest of code.

* Forced amalgamate rerun.

* Refactor test for #3007 so it doesn't use private members. Also extend to test both update() functions.

* Added fix for #3007 to update(const_iterator, const_iterator) as well.

* Added failing example code from #3007 as extra test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed kind: bug release item: 🐛 bug fix solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants