From 0f0d9255c62054b606cb56594526d03b431fed51 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 24 Oct 2025 18:00:05 -0400 Subject: [PATCH] Clean up JSON utils in a few ways In particular - Remove `get`, it is redundant with `valueAt` and the `get` in `util.hh`. - Remove `nullableValueAt`. It is morally just the function composition `getNullable . valueAt`, not an orthogonal combinator like the others. - `optionalValueAt` return a pointer, not `std::optional`. This also expresses optionality, but without creating a needless copy. This brings it in line with the other combinators which also return references. - Delete `valueAt` and `optionalValueAt` taking the map by value, as we did for `get` in 408c09a1207e1f6bb7367322ceb25d187334673f, which prevents bugs / unnecessary copies. `adl_serializer::from_json` was the one use of `getNullable`. I give it a little static function for the ultimate creation of a `std::optional` it does need to do (after switching it to using `getNullable . valueAt`. That could go in `json-utils.hh` eventually, but I didn't bother for now since only one things needs it. Co-authored-by: Sergei Zimmerman --- src/libfetchers/fetchers.cc | 5 +- src/libstore/derivation-options.cc | 49 ++++++++++++----- src/libstore/nar-info.cc | 10 ++-- src/libutil-tests/json-utils.cc | 62 +++++++++++++--------- src/libutil/include/nix/util/json-utils.hh | 20 ++++--- src/libutil/json-utils.cc | 45 +++------------- 6 files changed, 99 insertions(+), 92 deletions(-) diff --git a/src/libfetchers/fetchers.cc b/src/libfetchers/fetchers.cc index 324e8884c..c9c0fffa2 100644 --- a/src/libfetchers/fetchers.cc +++ b/src/libfetchers/fetchers.cc @@ -519,10 +519,11 @@ using namespace nix; fetchers::PublicKey adl_serializer::from_json(const json & json) { fetchers::PublicKey res = {}; - if (auto type = optionalValueAt(json, "type")) + auto & obj = getObject(json); + if (auto * type = optionalValueAt(obj, "type")) res.type = getString(*type); - res.key = getString(valueAt(json, "key")); + res.key = getString(valueAt(obj, "key")); return res; } diff --git a/src/libstore/derivation-options.cc b/src/libstore/derivation-options.cc index 698485c0d..6afaf0348 100644 --- a/src/libstore/derivation-options.cc +++ b/src/libstore/derivation-options.cc @@ -116,27 +116,29 @@ DerivationOptions::fromStructuredAttrs(const StringMap & env, const StructuredAt DerivationOptions defaults = {}; if (shouldWarn && parsed) { - if (get(parsed->structuredAttrs, "allowedReferences")) { + auto & structuredAttrs = getObject(parsed->structuredAttrs); + + if (get(structuredAttrs, "allowedReferences")) { warn( "'structuredAttrs' disables the effect of the top-level attribute 'allowedReferences'; use 'outputChecks' instead"); } - if (get(parsed->structuredAttrs, "allowedRequisites")) { + if (get(structuredAttrs, "allowedRequisites")) { warn( "'structuredAttrs' disables the effect of the top-level attribute 'allowedRequisites'; use 'outputChecks' instead"); } - if (get(parsed->structuredAttrs, "disallowedRequisites")) { + if (get(structuredAttrs, "disallowedRequisites")) { warn( "'structuredAttrs' disables the effect of the top-level attribute 'disallowedRequisites'; use 'outputChecks' instead"); } - if (get(parsed->structuredAttrs, "disallowedReferences")) { + if (get(structuredAttrs, "disallowedReferences")) { warn( "'structuredAttrs' disables the effect of the top-level attribute 'disallowedReferences'; use 'outputChecks' instead"); } - if (get(parsed->structuredAttrs, "maxSize")) { + if (get(structuredAttrs, "maxSize")) { warn( "'structuredAttrs' disables the effect of the top-level attribute 'maxSize'; use 'outputChecks' instead"); } - if (get(parsed->structuredAttrs, "maxClosureSize")) { + if (get(structuredAttrs, "maxClosureSize")) { warn( "'structuredAttrs' disables the effect of the top-level attribute 'maxClosureSize'; use 'outputChecks' instead"); } @@ -145,11 +147,15 @@ DerivationOptions::fromStructuredAttrs(const StringMap & env, const StructuredAt return { .outputChecks = [&]() -> OutputChecksVariant { if (parsed) { + auto & structuredAttrs = getObject(parsed->structuredAttrs); + std::map res; - if (auto outputChecks = get(parsed->structuredAttrs, "outputChecks")) { - for (auto & [outputName, output] : getObject(*outputChecks)) { + if (auto * outputChecks = get(structuredAttrs, "outputChecks")) { + for (auto & [outputName, output_] : getObject(*outputChecks)) { OutputChecks checks; + auto & output = getObject(output_); + if (auto maxSize = get(output, "maxSize")) checks.maxSize = maxSize->get(); @@ -195,7 +201,9 @@ DerivationOptions::fromStructuredAttrs(const StringMap & env, const StructuredAt std::map res; if (parsed) { - if (auto udr = get(parsed->structuredAttrs, "unsafeDiscardReferences")) { + auto & structuredAttrs = getObject(parsed->structuredAttrs); + + if (auto * udr = get(structuredAttrs, "unsafeDiscardReferences")) { for (auto & [outputName, output] : getObject(*udr)) { if (!output.is_boolean()) throw Error("attribute 'unsafeDiscardReferences.\"%s\"' must be a Boolean", outputName); @@ -226,7 +234,7 @@ DerivationOptions::fromStructuredAttrs(const StringMap & env, const StructuredAt std::map ret; if (parsed) { - auto e = optionalValueAt(parsed->structuredAttrs, "exportReferencesGraph"); + auto e = optionalValueAt(getObject(parsed->structuredAttrs), "exportReferencesGraph"); if (!e || !e->is_object()) return ret; for (auto & [key, value] : getObject(*e)) { @@ -333,8 +341,10 @@ namespace nlohmann { using namespace nix; -DerivationOptions adl_serializer::from_json(const json & json) +DerivationOptions adl_serializer::from_json(const json & json_) { + auto & json = getObject(json_); + return { .outputChecks = [&]() -> OutputChecksVariant { auto outputChecks = getObject(valueAt(json, "outputChecks")); @@ -397,13 +407,24 @@ void adl_serializer::to_json(json & json, const DerivationOpt json["allowSubstitutes"] = o.allowSubstitutes; } -DerivationOptions::OutputChecks adl_serializer::from_json(const json & json) +template +static inline std::optional ptrToOwned(const json * ptr) { + if (ptr) + return std::optional{*ptr}; + else + return std::nullopt; +} + +DerivationOptions::OutputChecks adl_serializer::from_json(const json & json_) +{ + auto & json = getObject(json_); + return { .ignoreSelfRefs = getBoolean(valueAt(json, "ignoreSelfRefs")), - .allowedReferences = nullableValueAt(json, "allowedReferences"), + .allowedReferences = ptrToOwned(getNullable(valueAt(json, "allowedReferences"))), .disallowedReferences = getStringSet(valueAt(json, "disallowedReferences")), - .allowedRequisites = nullableValueAt(json, "allowedRequisites"), + .allowedRequisites = ptrToOwned(getNullable(valueAt(json, "allowedRequisites"))), .disallowedRequisites = getStringSet(valueAt(json, "disallowedRequisites")), }; } diff --git a/src/libstore/nar-info.cc b/src/libstore/nar-info.cc index 1e7c48287..6f1abb273 100644 --- a/src/libstore/nar-info.cc +++ b/src/libstore/nar-info.cc @@ -159,17 +159,19 @@ NarInfo NarInfo::fromJSON(const StoreDirConfig & store, const StorePath & path, UnkeyedValidPathInfo::fromJSON(store, json), }}; + auto & obj = getObject(json); + if (json.contains("url")) - res.url = getString(valueAt(json, "url")); + res.url = getString(valueAt(obj, "url")); if (json.contains("compression")) - res.compression = getString(valueAt(json, "compression")); + res.compression = getString(valueAt(obj, "compression")); if (json.contains("downloadHash")) - res.fileHash = Hash::parseAny(getString(valueAt(json, "downloadHash")), std::nullopt); + res.fileHash = Hash::parseAny(getString(valueAt(obj, "downloadHash")), std::nullopt); if (json.contains("downloadSize")) - res.fileSize = getUnsigned(valueAt(json, "downloadSize")); + res.fileSize = getUnsigned(valueAt(obj, "downloadSize")); return res; } diff --git a/src/libutil-tests/json-utils.cc b/src/libutil-tests/json-utils.cc index 7d02894c6..b5c011355 100644 --- a/src/libutil-tests/json-utils.cc +++ b/src/libutil-tests/json-utils.cc @@ -70,7 +70,7 @@ TEST(valueAt, simpleObject) auto nested = R"({ "hello": { "world": "" } })"_json; - ASSERT_EQ(valueAt(valueAt(getObject(nested), "hello"), "world"), ""); + ASSERT_EQ(valueAt(getObject(valueAt(getObject(nested), "hello")), "world"), ""); } TEST(valueAt, missingKey) @@ -119,10 +119,12 @@ TEST(getArray, wrongAssertions) { auto json = R"({ "object": {}, "array": [], "string": "", "int": 0, "boolean": false })"_json; - ASSERT_THROW(getArray(valueAt(json, "object")), Error); - ASSERT_THROW(getArray(valueAt(json, "string")), Error); - ASSERT_THROW(getArray(valueAt(json, "int")), Error); - ASSERT_THROW(getArray(valueAt(json, "boolean")), Error); + auto & obj = getObject(json); + + ASSERT_THROW(getArray(valueAt(obj, "object")), Error); + ASSERT_THROW(getArray(valueAt(obj, "string")), Error); + ASSERT_THROW(getArray(valueAt(obj, "int")), Error); + ASSERT_THROW(getArray(valueAt(obj, "boolean")), Error); } TEST(getString, rightAssertions) @@ -136,10 +138,12 @@ TEST(getString, wrongAssertions) { auto json = R"({ "object": {}, "array": [], "string": "", "int": 0, "boolean": false })"_json; - ASSERT_THROW(getString(valueAt(json, "object")), Error); - ASSERT_THROW(getString(valueAt(json, "array")), Error); - ASSERT_THROW(getString(valueAt(json, "int")), Error); - ASSERT_THROW(getString(valueAt(json, "boolean")), Error); + auto & obj = getObject(json); + + ASSERT_THROW(getString(valueAt(obj, "object")), Error); + ASSERT_THROW(getString(valueAt(obj, "array")), Error); + ASSERT_THROW(getString(valueAt(obj, "int")), Error); + ASSERT_THROW(getString(valueAt(obj, "boolean")), Error); } TEST(getIntegralNumber, rightAssertions) @@ -156,18 +160,20 @@ TEST(getIntegralNumber, wrongAssertions) auto json = R"({ "object": {}, "array": [], "string": "", "int": 0, "signed": -256, "large": 128, "boolean": false })"_json; - ASSERT_THROW(getUnsigned(valueAt(json, "object")), Error); - ASSERT_THROW(getUnsigned(valueAt(json, "array")), Error); - ASSERT_THROW(getUnsigned(valueAt(json, "string")), Error); - ASSERT_THROW(getUnsigned(valueAt(json, "boolean")), Error); - ASSERT_THROW(getUnsigned(valueAt(json, "signed")), Error); + auto & obj = getObject(json); - ASSERT_THROW(getInteger(valueAt(json, "object")), Error); - ASSERT_THROW(getInteger(valueAt(json, "array")), Error); - ASSERT_THROW(getInteger(valueAt(json, "string")), Error); - ASSERT_THROW(getInteger(valueAt(json, "boolean")), Error); - ASSERT_THROW(getInteger(valueAt(json, "large")), Error); - ASSERT_THROW(getInteger(valueAt(json, "signed")), Error); + ASSERT_THROW(getUnsigned(valueAt(obj, "object")), Error); + ASSERT_THROW(getUnsigned(valueAt(obj, "array")), Error); + ASSERT_THROW(getUnsigned(valueAt(obj, "string")), Error); + ASSERT_THROW(getUnsigned(valueAt(obj, "boolean")), Error); + ASSERT_THROW(getUnsigned(valueAt(obj, "signed")), Error); + + ASSERT_THROW(getInteger(valueAt(obj, "object")), Error); + ASSERT_THROW(getInteger(valueAt(obj, "array")), Error); + ASSERT_THROW(getInteger(valueAt(obj, "string")), Error); + ASSERT_THROW(getInteger(valueAt(obj, "boolean")), Error); + ASSERT_THROW(getInteger(valueAt(obj, "large")), Error); + ASSERT_THROW(getInteger(valueAt(obj, "signed")), Error); } TEST(getBoolean, rightAssertions) @@ -181,24 +187,28 @@ TEST(getBoolean, wrongAssertions) { auto json = R"({ "object": {}, "array": [], "string": "", "int": 0, "boolean": false })"_json; - ASSERT_THROW(getBoolean(valueAt(json, "object")), Error); - ASSERT_THROW(getBoolean(valueAt(json, "array")), Error); - ASSERT_THROW(getBoolean(valueAt(json, "string")), Error); - ASSERT_THROW(getBoolean(valueAt(json, "int")), Error); + auto & obj = getObject(json); + + ASSERT_THROW(getBoolean(valueAt(obj, "object")), Error); + ASSERT_THROW(getBoolean(valueAt(obj, "array")), Error); + ASSERT_THROW(getBoolean(valueAt(obj, "string")), Error); + ASSERT_THROW(getBoolean(valueAt(obj, "int")), Error); } TEST(optionalValueAt, existing) { auto json = R"({ "string": "ssh-rsa" })"_json; - ASSERT_EQ(optionalValueAt(json, "string"), std::optional{"ssh-rsa"}); + auto * ptr = optionalValueAt(getObject(json), "string"); + ASSERT_TRUE(ptr); + ASSERT_EQ(*ptr, R"("ssh-rsa")"_json); } TEST(optionalValueAt, empty) { auto json = R"({})"_json; - ASSERT_EQ(optionalValueAt(json, "string"), std::nullopt); + ASSERT_EQ(optionalValueAt(getObject(json), "string"), nullptr); } TEST(getNullable, null) diff --git a/src/libutil/include/nix/util/json-utils.hh b/src/libutil/include/nix/util/json-utils.hh index 4b5fb4b21..51ebb2b6c 100644 --- a/src/libutil/include/nix/util/json-utils.hh +++ b/src/libutil/include/nix/util/json-utils.hh @@ -2,7 +2,6 @@ ///@file #include -#include #include "nix/util/error.hh" #include "nix/util/types.hh" @@ -12,20 +11,25 @@ namespace nix { enum struct ExperimentalFeature; -const nlohmann::json * get(const nlohmann::json & map, const std::string & key); - -nlohmann::json * get(nlohmann::json & map, const std::string & key); - /** * Get the value of a json object at a key safely, failing with a nice * error if the key does not exist. * * Use instead of nlohmann::json::at() to avoid ugly exceptions. */ -const nlohmann::json & valueAt(const nlohmann::json::object_t & map, const std::string & key); +const nlohmann::json & valueAt(const nlohmann::json::object_t & map, std::string_view key); -std::optional optionalValueAt(const nlohmann::json::object_t & value, const std::string & key); -std::optional nullableValueAt(const nlohmann::json::object_t & value, const std::string & key); +/** + * @return A pointer to the value assiocated with `key` if `value` + * contains `key`, otherwise return `nullptr` (not JSON `null`!). + */ +const nlohmann::json * optionalValueAt(const nlohmann::json::object_t & value, std::string_view key); + +/** + * Prevents bugs; see `get` for the same trick. + */ +const nlohmann::json & valueAt(nlohmann::json::object_t && map, std::string_view key) = delete; +const nlohmann::json * optionalValueAt(nlohmann::json::object_t && value, std::string_view key) = delete; /** * Downcast the json object, failing with a nice error if the conversion fails. diff --git a/src/libutil/json-utils.cc b/src/libutil/json-utils.cc index 74b3b27cc..1502384e9 100644 --- a/src/libutil/json-utils.cc +++ b/src/libutil/json-utils.cc @@ -1,52 +1,21 @@ #include "nix/util/json-utils.hh" #include "nix/util/error.hh" #include "nix/util/types.hh" -#include -#include -#include +#include "nix/util/util.hh" namespace nix { -const nlohmann::json * get(const nlohmann::json & map, const std::string & key) +const nlohmann::json & valueAt(const nlohmann::json::object_t & map, std::string_view key) { - auto i = map.find(key); - if (i == map.end()) - return nullptr; - return &*i; -} - -nlohmann::json * get(nlohmann::json & map, const std::string & key) -{ - auto i = map.find(key); - if (i == map.end()) - return nullptr; - return &*i; -} - -const nlohmann::json & valueAt(const nlohmann::json::object_t & map, const std::string & key) -{ - if (!map.contains(key)) + if (auto * p = optionalValueAt(map, key)) + return *p; + else throw Error("Expected JSON object to contain key '%s' but it doesn't: %s", key, nlohmann::json(map).dump()); - - return map.at(key); } -std::optional optionalValueAt(const nlohmann::json::object_t & map, const std::string & key) +const nlohmann::json * optionalValueAt(const nlohmann::json::object_t & map, std::string_view key) { - if (!map.contains(key)) - return std::nullopt; - - return std::optional{map.at(key)}; -} - -std::optional nullableValueAt(const nlohmann::json::object_t & map, const std::string & key) -{ - auto value = valueAt(map, key); - - if (value.is_null()) - return std::nullopt; - - return std::optional{std::move(value)}; + return get(map, key); } const nlohmann::json * getNullable(const nlohmann::json & value)