Improve checked json casting #10087

haenoe · 2024-02-26T10:58:09Z

Motivation

This introduces new utility functions to get elements from JSON - with nice error messages, if the expected type does not match, as discussed in #9994.

Right now I only implemented a first set of functions, namely for getString, getInteger and getBoolean, getArray and getObject, because I'm a bit unsure if I'm on the right track @iFreilicht envisioned.

Moreover I encountered some problems using the new implementations in derivations.cc (here, here and here) where there is no cast possible from object_t/array_t to the desired types (e.g. StringSet / std::set<std::string>).

I would be grateful for any help in that regard, as I sadly don't have much C++ experience and the error messages are rather cryptic - many thanks!

Context

See #9994

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

This introduces new utility functions to get elements from JSON - with nice error messages, if the expected type does not match.

iFreilicht

Thank you for your efforts so far! This is exactly what we were thinking of, the direction is perfect!

I encountered some problems using the new implementations in derivations.cc [...] where there is no cast possible [...]

Ah yes. Here, introducing more type safety broke the type checks, because there is a cast defined from nlohmann::json & to std::set<std::string> (and basically anything else that can be represented as a JSON), but none from nlohmann::json::array_t & to std::set<std::string>.

To fix this, you have to define additional functions that validate the required nested types properly. If you want to have a go at this for yourself, these are the function definitions:

StringSet getStringSet(const nlohmann::json & value);
Strings getStringList(const nlohmann::json & value);
StringMap getStringMap(const nlohmann::json & value);

(You also need to add #include "types.hh" at the top of json-utils.hh, else you'll get "Cannot overload functions by return type alone" errors when trying to add the definition to json-utils.cc)

If you get stuck or you'd rather use a finished solution, check iFreilicht@bddf5c5.

A few more notes on things you can improve:

Now that there's a proper replacement for ensureType, you should update the valueAt function to take an nlohmann::json::object_t instead of a nlohmann::json. This makes it type safe, and you can remove the disclaimer about it not checking the type of its input, as the compiler will not allow passing anything in that wasn't checked by getObject() before.

Additionally, you should run the functional tests and see if any of them broke. Potentially, you'll have to adapt the tests to check for the updated error message.

It would be even cooler if you could also add unit tests for your new functions, as nix is very under-tested currently. The tests for this file are in tests/unit/libutil/json-utils.cc.

src/libutil/json-utils.hh

src/libutil/json-utils.cc

nixos-discourse · 2024-03-05T07:41:41Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-03-04-nix-team-meeting-minute-130/40830/1

haenoe · 2024-03-05T09:27:23Z

Hello again!

Sorry this took so long. I had everything implemented, then realized (bless the tests) that somehow the program segfaulted. Should be fixed now :D (I hope this doesn't introduce new bugs ^^')
Also when trying to change the valueAt argument type to nlohman::json::object_t the linker gives me an undefined symbols error (see below, possibly related is this). How should I proceed in this regard?

The last place where ensureType is used is ./src/libfetchers/git.cc, should I create another function specifically for getting a list of PublicKey, or is there an easier option?

Undefined symbols for architecture arm64:
"nix::valueAt(nlohmann::json_abi_v3_11_2::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator>, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::json_abi_v3_11_2::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator>> const&, std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator> const&)", referenced from:
nix::Derivation::fromJSON(nix::StoreDirConfig const&, nlohmann::json_abi_v3_11_2::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator>, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::json_abi_v3_11_2::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator>> const&, nix::ExperimentalFeatureSettings const&) in derivations.o
nix::NarInfo::fromJSON(nix::Store const&, nix::StorePath const&, nlohmann::json_abi_v3_11_2::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator>, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::json_abi_v3_11_2::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator>> const&) in nar-info.o
nix::UnkeyedValidPathInfo::fromJSON(nix::Store const&, nlohmann::json_abi_v3_11_2::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator>, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::json_abi_v3_11_2::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator>> const&) in path-info.o
ld: symbol(s) not found for architecture arm64
clang-16: error: linker command failed with exit code 1 (use -v to see invocation)

iFreilicht · 2024-03-05T18:15:17Z

Sorry this took so long. I had everything implemented, then realized (bless the tests) that somehow the program segfaulted. Should be fixed now :D (I hope this doesn't introduce new bugs ^^')

No worries! We're doing this in our free time, I understand that it can take a few days to get a response.

Hmm, segfaults would be a bit problematic. However, I couldn't reproduce it. Which test did you run that gave you that error?

If returning references causes segfaults, then the functions shouldn't do that, and return values instead. I don't know if this is problematic in terms of performance (could even be beneficial), but I think for the current use-case, we don't have to worry about that, as the files we parse are pretty small.

Interesting side-note here: I found out that you can skip get_ref entirely, and just return value;, which will be converted to the return type of the function implicitly. In this case, the compiler prints a warning which may explain the segfault you saw:

src/libutil/json-utils.cc:82:12: warning: returning reference to local temporary object [-Wreturn-stack-address]

So maybe, get_ref actually does the same thing, except that the compiler can't warn us, because it doesn't know.

The last place where ensureType is used is ./src/libfetchers/git.cc, should I create another function specifically for getting a list of PublicKey, or is there an easier option?

That seems unnecessary. I feel like this is a super-specific use-case that doesn't relate to JSON in general. I would suggest to just write a proper parsing routine directly within that function, something like this:

    if (attrs.contains("publicKeys")) {
        nlohmann::json publicKeysJson = nlohmann::json::parse(getStrAttr(attrs, "publicKeys"));
        auto someArray = getArray(publicKeysJson);
        publicKeys.clear();
        for (auto elem : someArray) {
            auto keyObject = getObject(elem);
            auto type = getString(valueAt(keyObject, "type"));
            auto key = getString(valueAt(keyObject, "key"));
            publicKeys.push_back({ type, key });
        }

I didn't try this out or test it at all, and there may be better names for those variables, but I think it shows the basic idea.

Also when trying to change the valueAt argument type to nlohman::json::object_t the linker gives me an undefined symbols error (see below, possibly related is nlohmann/json#285). How should I proceed in this regard?

I think you may have forgotten to update the type of the implementation (or of the definition). You have to update the type in both the header and the source file, otherwise you'll get errors like that. That's how I was able to reproduce it.

BTW, another thing I noticed: It would be better to actually implement all the basic get... functions in terms of ensureType to reduce code duplication:

const nlohmann::json::object_t getObject(const nlohmann::json & value)
{
    return ensureType(value, nlohmann::json::value_t::object);
}

haenoe · 2024-03-05T19:52:38Z

Thank you for the quick response.

Regarding the segfault I have created a separate branch segfault, which hopefully helps bringing the problem across.. (more context)

Making json an auto & seems to fix the problem though I am not 100% sure why that is (does C++ create a copy when using only auto?)
Your remark with get_ref is indeed very interesting, but I fear my knowledge is too limited to make an educated guess in that regard.

I will update the implementation and write the unit tests as soon as possible and then get back to you :D

haenoe · 2024-03-15T17:03:12Z

Hello again @iFreilicht!

I tried to progress but came across more problems that I don't understand fully.

The problems start when you try to access more complex JSON structures like nested object or arrays. Then come the segfaults and/or nulls.
I've tried to reproduce part of the problem in this repo.
Could it be that there are some implicit type conversions going on in the background?

Sadly my time right now is pretty limited because of exams, so I can't try out as much as I wish - If you have the time I would be very thankful if you maybe could point me to some direction (e.g. regarding the question whether we should even return references) ^^'

In any case many thanks and I wish you a great weekend!!!

iFreilicht · 2024-03-18T08:10:55Z

Hey @haenoe, thank you for your continued efforts, I'll have a look as soon as possible. Best of luck on your exams!

iFreilicht · 2024-03-27T16:10:16Z

Alright, I had a look now, and I found a fix! The issue is typical C++ nonsense, and I pushed the fix to a fork of your repo, so let's look at it.

First off, haenoe/json-questions@cdb7da0 fixes your use of const and &. const is not necessary at all in this context, and while you can use & to create references to the returned object, that has no benefit at all. It's only important to use & when a reference is returned, otherwise the variable will just contain a copy of the referenced value.

Secondly, haenoe/json-questions@e081b14 fixes the memory access errors, simply by adding an additional overload for the valueAt function:

const nlohmann::json & valueAt(const nlohmann::json::object_t & map, const std::string & key)
{
  return map.at(key);
}

The reason why the original definition is not enough is very subtle. This is just my theory, I'm not an expert in C++ and I feel this is going way over my head. Let's look at it again:

const nlohmann::json & valueAt(const nlohmann::json & map, const std::string & key)
{
  return map[key];
}

This works when calling it with an nlohmann::json, like you did:

  auto object = getObject(valueAt(j2, "object"));

It seemingly also works when calling it with an nlohmann::json::object_t (which is just an alias for std::map<std::string, nlohmann::json>):

  auto nestedObject = getString(valueAt(object, "currency"));

However, this call actually has a lot more going on than you might think. Because object is an std::map and not a nlohmann::json, the only way to call this function is by converting object into a json object, which C++ just decides to implicitly do in this case. I'm not sure why, but I think constructor (3) of basic_json is to blame.

Anyway, when that new json value is implicitly constructed, new memory is allocated for its contents somewhere, a reference to that memory is given to valueAt, which in turn returns a reference to some data inside of that json, and once the variable assignment is complete, the json value is destroyed, leaving you with a dangling reference. In the case of getString, this doesn't matter, because it already extracted all the necessary data and copied it to a new object that was assigned to nestedObject.
But in the case of getStringRef, that dangling reference is still present after the variable assignment.

Adding the new overload for valueAt will prevent that pesky conversion from happening, meaning valueAt returns a reference to memory that is on the stack and not in danger of being freed.

What to do now

nlohmann::json offers a parameter JSON_USE_IMPLICIT_CONVERSIONS, but that only works for the other direction (converting from json). I added a comment to an existing discussion and will have a look at creating a pull request for making this flag also prevent the case we stumbled into, as it seems like a obviously good change.

As for you, you just have to add a similar implementation of valueAt to your PR here.

haenoe · 2024-03-29T13:47:19Z

Hello again!
First of all thank you very much for your help and the amazing write-up!!

I'll try to work on the fix and the rest of the unit tests asap.
One question: Should I somehow squash all of the commits together when I'm finished and the tests pass?

haenoe · 2024-03-31T20:56:05Z

Hello!

After writing most of the test this feels really refreshing -- as everything works as it should without crashing :D

Should I look for a way to make the tests less repetitive?
Aside from that, is everything okay with the commits?

Thank you

src/libutil/json-utils.hh

Ericson2314 · 2024-03-31T21:07:31Z

src/libutil/json-utils.cc

+std::optional<nlohmann::json> getNullable(const nlohmann::json & value, const std::string & key)
+{
+    try {
+        auto & v = valueAt(value, key);
+        return v.get<nlohmann::json>();
+    } catch (...) {
+        return std::nullopt;
+    }
+}


Ah this is different thing than the one I indented, which would turn null to std::nullopt and anything else to std::optional { <original> }.

This one is useful too, but maybe lets call it optionalValueAt or something. "nullable" implies null but this one isn't about null at all! Just about potentially-missing fields.

Also, I hope we will transfer from having fewer sometimes-there-sometimes-missing fields, and more explicit nulls, so this function is hopefully just used for things we don't control and some backwards compat.

Ah okay -- makes sense! Thanks :D

Ericson2314 · 2024-03-31T21:08:11Z

src/libutil/json-utils.cc

+std::optional<nlohmann::json> getNullable(const nlohmann::json & value, const std::string & key)
+{
+    try {
+        auto & v = valueAt(value, key);
+        return v.get<nlohmann::json>();
+    } catch (...) {
+        return std::nullopt;
+    }
+}


Also, I hope we will transfer from having fewer sometimes-there-sometimes-missing fields, and more explicit nulls, so this function is hopefully just used for things we don't control and some backwards compat.

Ericson2314 · 2024-03-31T21:09:20Z

src/libfetchers/git.cc

-        publicKeys = publicKeysJson.get<std::vector<PublicKey>>();
+        auto pubKeys = getArray(publicKeysJson);
+        publicKeys.clear();
+        for (auto jsonKey : pubKeys) {
+            auto keyObj = getObject(jsonKey);
+            auto type = getString(getNullable(keyObj, "type").value_or("ssh-ed25519"));
+            auto key = getString(valueAt(keyObj, "key"));
+            publicKeys.push_back({ type, key });
+        }


We shouldn't inline things like this. Instead we can change the JSON deserializer for PublicKey to use the new combinators. (And we could test that deserializer too.)

Just that I understand that correctly this means implementing a new static method for PublicKey::fromJSON? ^^'

Grep for JSON_IMPL. We want to keep that pattern.

Also, How did it work before? If each on was a string, it should stay that way (at least for now). This PR shouldn't be changing behavior.

src/libfetchers/git.cc

…cked-json-casting

haenoe · 2024-04-02T13:23:28Z

Hi, I tried to apply all of the recommendations :D

I thought about the get* tests again and it seems to me that writing a macro would lead to much less code duplication (maybe similar to TEST_JSON here
You said I should test the new json (de)serialization for nix::fetchers::PublicKey. Since no tests/unit/libfetchers exists, should I create it?

As always -- many thanks ^^'

Ericson2314 · 2024-04-02T13:27:35Z

src/libfetchers/fetchers.hh

-NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULT(PublicKey, type, key)
-


Oh sorry, I didn't see this! With this we don't need JSON_IMPL because we already have a JSON impl.

I thought we needed the new (custom) JSON Impl for the improved error messages (by using getString etc.)

@haenoe That might be a great follow-up task (and you are welcome to submit it as a separate PR!), but it should not be part of this PR, which should be a "pure" refactor --- without any behavioral changes.

@haenoe Oh wait... I am being stupid. It is a non-behavioral change! Still it might be better as a (quick easy) follow-up PR.

src/libfetchers/fetchers.cc

src/libfetchers/fetchers.hh

src/libfetchers/git.cc

src/libstore/derivations.cc

Ericson2314 · 2024-04-02T14:07:11Z

Since no tests/unit/libfetchers exists, should I create it?

Yes, please do! We can create it in this PR, and then ensure in the next one where you redo that serializer that no behavior is changed.

This reverts commit 27a16f4.

haenoe · 2024-04-03T10:38:17Z

Hopefully this addresses everything and I didn't miss anything when creating the tests.
I would be glad to experiment a bit more with the serializer for PublicKey, but can probably only work on it next week ^^'

haenoe · 2024-04-03T20:45:42Z

@iFreilicht @Ericson2314 thank you for being so kind and welcoming throughout this whole process and investing your time.
This is not something you can take for granted and I am very grateful.

I hope I can contribute more in the future :'D

haenoe added 2 commits February 26, 2024 10:28

Add 'getString', 'getInteger', 'getBoolean' functions

7846c14

This introduces new utility functions to get elements from JSON - with nice error messages, if the expected type does not match.

Add 'getObject', 'getArray' functions

91cd38d

haenoe mentioned this pull request Feb 26, 2024

Improve checked JSON casting #9994

Closed

iFreilicht reviewed Feb 29, 2024

View reviewed changes

src/libutil/json-utils.hh Outdated Show resolved Hide resolved

src/libutil/json-utils.cc Outdated Show resolved Hide resolved

haenoe and others added 3 commits March 1, 2024 08:21

Merge branch 'NixOS:master' into checked-json-casting

08e86ae

Add implementation for getStringList

ca5c19c

Add implementation of getStringMap

5cc2fbf

thufschmitt self-assigned this Mar 4, 2024

haenoe added 3 commits March 5, 2024 10:10

Add implementation of getStringSet

0a40691

Fix small mistakes in getObject and getStringList

8284b5b

Fix variables that make test segfault

8d89c30

Add the failing value to JSON conversions

bd92d24

Move implementation to ensureType

2e31f09

iFreilicht mentioned this pull request Mar 27, 2024

std::tuple dangling reference - implicit conversion nlohmann/json#2226

Open

5 tasks

haenoe added 3 commits March 31, 2024 22:51

Fix valueAt segfaults`

dff4a94

Implement getNullable

fbd750d

Tests for JSON utils

8525624

github-actions bot added the fetching Networking with the outside (non-Nix) world, input locking label Mar 31, 2024

Ericson2314 reviewed Mar 31, 2024

View reviewed changes

src/libutil/json-utils.hh Outdated Show resolved Hide resolved

Ericson2314 reviewed Mar 31, 2024

View reviewed changes

haenoe added 4 commits April 2, 2024 14:20

Change getNullable, create optionalValueAt

5859930

Implement JSON (de)serialization for PublicKey

27a16f4

Fix unnecessary copies

147d190

Merge branch 'checked-json-casting' of github.com:haenoe/nix into che…

769a514

…cked-json-casting