Unexpected behavior with fifo_map json when copy and append

[DolphinDream]

When copying a fifo_json to another fifo_json and appending values for new keys, different keys present in the original fifo_json are being modified instead of new keys being created.

 fifo_json j1;
 j1["Ender"] = "The Formics are intelligent";
 j1["Amstrong"] = "Walking on the moon";
 j1["Fairytale"] = "Once upon a time";

 cout << setw(2) << j1 << endl << endl;

 fifo_json jj(j1);
 // fifo_json &jj = j1; // this works fine (though perhaps because it doesn't copy)
 jj["My settings"] = {};
 jj["My settings"]["Darwin"] = "Evolution of Species";
 jj["Abracadabra"] = "Ta-Da";

 cout << setw(2) << jj << endl;

The expected result should be:

{
  "Ender": "The Formics are intelligent",
  "Amstrong": "Walking on the moon",
  "Fairytale": "Once upon a time"
}

{
  "Ender": "The Formics are intelligent",
  "Amstrong": "Walking on the moon",
  "Fairytale": "Once upon a time",
  "My settings": {
    "Darwin": "Evolution of Species"
  },
  "Abracadabra": "Ta-Da"
}

But instead I get this:

the output is:

{
  "Ender": "The Formics are intelligent",
  "Amstrong": "Walking on the moon",
  "Fairytale": "Once upon a time"
}

{
  "Ender": {
    "Darwin": "Evolution of Species"
  },
  "Amstrong": "Walking on the moon",
  "Fairytale": "Ta-Da"
}

I'm running this on the following system:

• OS: Ubuntu 16.04.1
• g++ (Ubuntu 5.4.0-6ubuntu1~16.04.10) 5.4.0 20160609
• using c++11 standard
• json version used is: 3.3.0

And I'm using the following definitions :

// A workaround to give to use fifo_map as map, we are just ignoring the 'less' compare
template<class K, class V, class dummy_compare, class A>
using my_fifo_map = nlohmann::fifo_map<K, V, nlohmann::fifo_map_compare<K>, A>;
using fifo_json = nlohmann::basic_json<my_fifo_map>;
using json = nlohmann::json;

[DolphinDream]

It seems that if i make the copy this way it works fine:

fifo_json jj = json::parse(j1.dump());

so, something may be buggy with the way the other copy constructors work:

fifo_json jj = j1; // problematic
fifo_json jj(j1); // problematic

[tete17]

I think I found the issue.

I believe it is actually a bug in the fifo_map library.
When copying a fifo_map thirdparty/fifo_map/fifo_map.hpp:129 specifically copying a fifo_map_compare (Line 47) only the set of keys get copied over but the timestamp of such class is not passed over, therefore the class thinks it does not have any members already when you add ones.

That is why on the first time you insert on a copied fifo_json it looks like the first element is being overwritten.

This really small patch fixes the issues since it copies over the whole object instead of just the keys, without updating the timestamp.

diff --git a/test/thirdparty/fifo_map/fifo_map.hpp b/test/thirdparty/fifo_map/fifo_map.hpp
index c281e3be..1667dd2e 100644
--- a/test/thirdparty/fifo_map/fifo_map.hpp
+++ b/test/thirdparty/fifo_map/fifo_map.hpp
@@ -126,7 +126,7 @@ template <
     fifo_map() : m_keys(), m_compare(&m_keys), m_map(m_compare) {}
 
     /// copy constructor
-    fifo_map(const fifo_map &f) : m_keys(f.m_keys), m_compare(&m_keys), m_map(f.m_map.begin(), f.m_map.end(), m_compare) {}
+    fifo_map(const fifo_map &f) : m_keys(f.m_keys), m_compare(f.m_compare), m_map(f.m_map.begin(), f.m_map.end(), m_compare) {}
 
     /// constructor for a range of elements
     template<class InputIterator>

I also have some unit tests to show it but since this is a bug on third-party library I am not sure where to submit it.
If anyone knows where this library comes from please let me know so we can report/fix this issue

PS: If any thing of this is not clear enough please let me know I am written this in a rush before going to bed so it may not be the best explanation but I am open to explain better if necessary 😄

[tete17]

I leave my branch with my fixes as a proof but is not yet ready for merging since I still have to add the CHECKS to the unit test to be production ready.

https://github.com/tete17/json/tree/fix-1763

[tete17]

It even looks like to be fixed upstream https://github.com/nlohmann/fifo_map/blob/0dfbf5dacbb15a32c43f912a7e66a54aae39d0f9/src/fifo_map.hpp#L146

Time to update dependencies?

[nlohmann]

Any idea how to proceed here?

[stale]

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.

[DolphinDream]

It even looks like to be fixed upstream https://github.com/nlohmann/fifo_map/blob/0dfbf5dacbb15a32c43f912a7e66a54aae39d0f9/src/fifo_map.hpp#L146

Time to update dependencies?

Which is the better fix for 3.3.0 though ? the one you proposed earlier m_compare(f.m_compare) .. or the one you suggested it was already fixed upstream m_compare(&m_keys, f.m_compare.m_timestamp) ?

Note: upgrading to a newer version of json library is not feasible at this point.. but a small bug fix is more doable, hence my question.

[DolphinDream]

is fifo_map still the way to go to create json objects preserving order of insertion ? or is there a different approach? (unorderd map? unordered json etc?)

[nlohmann]

There is now ordered_json, see https://json.nlohmann.me/api/ordered_json/