nix icon indicating copy to clipboard operation
nix copied to clipboard

Improve checked JSON casting

Open Ericson2314 opened this issue 1 year ago • 2 comments

In #8760, @iFreilicht added an ensureType function to improve our errors for JSON decoding ill-formed values. The errors our indeed better, but using it is cumbersome because has to remember to always first call ensureType with the right type, and then do the actual cast.

To make things a bit easier I propose this interface change:

--- a/src/libutil/json-utils.hh
+++ b/src/libutil/json-utils.hh
@@ -12,7 +12,7 @@ nlohmann::json * get(nlohmann::json & map, const std::string & key);

 /**
  * Get the value of a json object at a key safely, failing
- * with a Nix Error if the key does not exist.
+ * with a nice error if the key does not exist.
  *
  * Use instead of nlohmann::json::at() to avoid ugly exceptions.
  *
@@ -23,14 +23,18 @@ const nlohmann::json & valueAt(
     const std::string & key);

 /**
- * Ensure the type of a json object is what you expect, failing
- * with a Nix Error if it isn't.
+ * Downcast the json object, failing with a nice error if the conversion fails.
  *
- * Use before type conversions and element access to avoid ugly exceptions.
+ * See https://json.nlohmann.me/features/types/
  */
-const nlohmann::json & ensureType(
-    const nlohmann::json & value,
-    nlohmann::json::value_type expectedType);
+std::optional<nlohmann::json> getNullable(const nlohmann::json & value);
+const nlohmann::object_t & getObject(const nlohmann::json & value);
+const nlohmann::array_t & getArray(const nlohmann::json & value);
+const nlohmann::string_t & getString(const nlohmann::json & value);
+const nlohman::number_integer_t & getInteger(const nlohmann::json & value);
+const nlohman::number_unsigned_t & getNatural(const nlohmann::json & value);
+const nlohman::number_float_t & getFloat(const nlohmann::json & value);

 /**
  * For `adl_serializer<std::optional<T>>` below, we need to track what

It would be very nice if someone could change this interface (and update the current usages of ensureType to use these new functions instead).

Ericson2314 avatar Feb 12 '24 15:02 Ericson2314

Hey! I love Nix and would like to contribute - though I don't know if I can find time before the weekend. My C++ experience is also limited so I would be glad for any feedback :D

Thank you!

haenoe avatar Feb 20 '24 09:02 haenoe

Hey @haenoe, great to hear! As the Nix core team is very busy, feel free to tag me when you open a PR, I'll review your changes. You can check the PR linked in the description above to see where to start, and the API docs of the json library were using to find out how to implement the desired changes.

iFreilicht avatar Feb 20 '24 14:02 iFreilicht

Thank you for being so kind and welcoming - you seriously made my week!! I've tried to implement the changes, but encountered some challenges (more context in #10087)

I hope I'm on the right track here.. :D

haenoe avatar Feb 26 '24 10:02 haenoe

Oh I have been hunting for GSoC merge derivation proposition and reading the 'derivations.c'. It lead me here. So it looks like this one also relates to the restructuring of the JSON?

allrealmsoflife avatar Feb 29 '24 17:02 allrealmsoflife

@stablejoy Not really. This is only about improving error messages and type safety.

iFreilicht avatar Feb 29 '24 18:02 iFreilicht

@stablejoy I think it is OK to make part of a GSOC project with those things. It makes sense to improve JSON formats and improve infra about JSON formats at the same time. Indeed https://github.com/NixOS/nix/pull/9995, which is blocked on this issue, has notes from Nix team meeting (which I missed) about that it should be accompanied with updates to the JSON guidelines.

Ericson2314 avatar Mar 19 '24 14:03 Ericson2314

OTOH https://github.com/NixOS/nix/pull/10087 has already started it and should be done soon :)

Ericson2314 avatar Apr 01 '24 16:04 Ericson2314