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

Improve checked JSON casting #9994

Closed
Ericson2314 opened this issue Feb 12, 2024 · 7 comments
Closed

Improve checked JSON casting #9994

Ericson2314 opened this issue Feb 12, 2024 · 7 comments
Labels
contributor-experience Developer experience for Nix contributors good first issue Quick win for first-time contributors

Comments

@Ericson2314
Copy link
Member

Ericson2314 commented Feb 12, 2024

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 Ericson2314 added feature Feature request or proposal good first issue Quick win for first-time contributors labels Feb 12, 2024
Ericson2314 added a commit that referenced this issue Feb 12, 2024
Blocked on #9994, tests will fail until this
Ericson2314 added a commit that referenced this issue Feb 12, 2024
Blocked on #9994, tests will fail until this
@Ericson2314 Ericson2314 added contributor-experience Developer experience for Nix contributors and removed feature Feature request or proposal labels Feb 12, 2024
@haenoe
Copy link
Contributor

haenoe commented Feb 20, 2024

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!

@iFreilicht
Copy link
Contributor

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.

@haenoe
Copy link
Contributor

haenoe commented Feb 26, 2024

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

@domagojding
Copy link

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?

@iFreilicht
Copy link
Contributor

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

@Ericson2314
Copy link
Member Author

Ericson2314 commented Mar 19, 2024

@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 #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
Copy link
Member Author

OTOH #10087 has already started it and should be done soon :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor-experience Developer experience for Nix contributors good first issue Quick win for first-time contributors
Projects
None yet
Development

No branches or pull requests

4 participants