ValidPathInfo JSON format should use null not omit field

NixOS · May 31, 2024 · 86554c9 · 86554c9
1 parent 8949616
commit 86554c9
Show file tree

Hide file tree

Showing 9 changed files with 79 additions and 40 deletions.
diff --git a/doc/manual/rl-next/store-object-info.md b/doc/manual/rl-next/store-object-info.md
@@ -0,0 +1,11 @@
+---
+synopsis: Store object info JSON format now uses `null` rather than omitting fields.
+prs: 9995
+---
+
+The [store object info JSON format](@docroot@/protocols/json/store-object-info.md) no longer omits fields to indicate absent information, but instead includes the fields with a `null` value.
+For example, `"ca": null` is used to to indcate a store object that isn't content-addressed rather than omitting the `ca` field entirely.
+This makes records of this sort more self-describing, and easier to consume programmatically.
+
+We would like to follow this design principle going forward;
+the [JSON guidlines](@docroot@/contributing/json-guideline.md) in the contributing section have been updated accordingly.
diff --git a/src/libstore/path-info.cc b/src/libstore/path-info.cc
@@ -161,27 +161,24 @@ nlohmann::json UnkeyedValidPathInfo::toJSON(
     jsonObject["narSize"] = narSize;
 
     {
-        auto& jsonRefs = (jsonObject["references"] = json::array());
+        auto & jsonRefs = jsonObject["references"] = json::array();
         for (auto & ref : references)
             jsonRefs.emplace_back(store.printStorePath(ref));
     }
 
-    if (ca)
-        jsonObject["ca"] = renderContentAddress(ca);
+    jsonObject["ca"] = ca ? (std::optional { renderContentAddress(*ca) }) : std::nullopt;
 
     if (includeImpureInfo) {
-        if (deriver)
-            jsonObject["deriver"] = store.printStorePath(*deriver);
+        jsonObject["deriver"] = deriver ? (std::optional { store.printStorePath(*deriver) }) : std::nullopt;
 
-        if (registrationTime)
-            jsonObject["registrationTime"] = registrationTime;
+        jsonObject["registrationTime"] = registrationTime ? (std::optional { registrationTime }) : std::nullopt;
 
-        if (ultimate)
-            jsonObject["ultimate"] = ultimate;
+        jsonObject["ultimate"] = ultimate;
 
+        auto & sigsObj = jsonObject["signatures"] = json::array();
         if (!sigs.empty()) {
             for (auto & sig : sigs)
-                jsonObject["signatures"].push_back(sig);
+                sigsObj.push_back(sig);
         }
     }
 
@@ -210,20 +207,25 @@ UnkeyedValidPathInfo UnkeyedValidPathInfo::fromJSON(
         throw;
     }
 
+    // New format as this as nullable but mandatory field; handling
+    // missing is for back-compat.
     if (json.contains("ca"))
-        res.ca = ContentAddress::parse(getString(valueAt(json, "ca")));
+        if (auto * rawCa = getNullable(valueAt(json, "ca")))
+            res.ca = ContentAddress::parse(getString(*rawCa));
 
     if (json.contains("deriver"))
-        res.deriver = store.parseStorePath(getString(valueAt(json, "deriver")));
+        if (auto * rawDeriver = getNullable(valueAt(json, "deriver")))
+            res.deriver = store.parseStorePath(getString(*rawDeriver));
 
     if (json.contains("registrationTime"))
-        res.registrationTime = getInteger(valueAt(json, "registrationTime"));
+        if (auto * rawRegistrationTime = getNullable(valueAt(json, "registrationTime")))
+            res.registrationTime = getInteger(*rawRegistrationTime);
 
     if (json.contains("ultimate"))
         res.ultimate = getBoolean(valueAt(json, "ultimate"));
 
     if (json.contains("signatures"))
-        res.sigs = valueAt(json, "signatures");
+        res.sigs = getStringSet(valueAt(json, "signatures"));
 
     return res;
 }

diff --git a/src/libutil/json-utils.cc b/src/libutil/json-utils.cc
@@ -39,12 +39,9 @@ std::optional<nlohmann::json> optionalValueAt(const nlohmann::json::object_t & m
 }
 
 
-std::optional<nlohmann::json> getNullable(const nlohmann::json & value)
+const nlohmann::json * getNullable(const nlohmann::json & value)
 {
-    if (value.is_null())
-        return std::nullopt;
-
-    return value.get<nlohmann::json>();
+    return value.is_null() ? nullptr : &value;
 }
 
 /**

diff --git a/src/libutil/json-utils.hh b/src/libutil/json-utils.hh
@@ -29,7 +29,7 @@ std::optional<nlohmann::json> optionalValueAt(const nlohmann::json::object_t & v
  * Downcast the json object, failing with a nice error if the conversion fails.
  * See https://json.nlohmann.me/features/types/
  */
-std::optional<nlohmann::json> getNullable(const nlohmann::json & value);
+const nlohmann::json * getNullable(const nlohmann::json & value);
 const nlohmann::json::object_t & getObject(const nlohmann::json & value);
 const nlohmann::json::array_t & getArray(const nlohmann::json & value);
 const nlohmann::json::string_t & getString(const nlohmann::json & value);

diff --git a/tests/functional/signing.sh b/tests/functional/signing.sh
@@ -15,9 +15,9 @@ outPath=$(nix-build dependencies.nix --no-out-link --secret-key-files "$TEST_ROO
 
 # Verify that the path got signed.
 info=$(nix path-info --json $outPath)
-[[ $info =~ '"ultimate":true' ]]
-[[ $info =~ 'cache1.example.org' ]]
-[[ $info =~ 'cache2.example.org' ]]
+echo $info | jq -e '.[] | .ultimate == true'
+echo $info | jq -e '.[] | .signatures.[] | select(startswith("cache1.example.org"))'
+echo $info | jq -e '.[] | .signatures.[] | select(startswith("cache2.example.org"))'
 
 # Test "nix store verify".
 nix store verify -r $outPath
@@ -39,8 +39,8 @@ nix store verify -r $outPath
 
 # Verify that the path did not get signed but does have the ultimate bit.
 info=$(nix path-info --json $outPath2)
-[[ $info =~ '"ultimate":true' ]]
-(! [[ $info =~ 'signatures' ]])
+echo $info | jq -e '.[] | .ultimate == true'
+echo $info | jq -e '.[] | .signatures == []'
 
 # Test "nix store verify".
 nix store verify -r $outPath2
@@ -57,7 +57,7 @@ nix store verify -r $outPath2 --sigs-needed 1 --trusted-public-keys $pk1
 # Build something content-addressed.
 outPathCA=$(IMPURE_VAR1=foo IMPURE_VAR2=bar nix-build ./fixed.nix -A good.0 --no-out-link)
 
-[[ $(nix path-info --json $outPathCA) =~ '"ca":"fixed:md5:' ]]
+nix path-info --json $outPathCA | jq -e '.[] | .ca | startswith("fixed:md5:")'
 
 # Content-addressed paths don't need signatures, so they verify
 # regardless of --sigs-needed.
@@ -73,15 +73,15 @@ nix copy --to file://$cacheDir $outPath2
 
 # Verify that signatures got copied.
 info=$(nix path-info --store file://$cacheDir --json $outPath2)
-(! [[ $info =~ '"ultimate":true' ]])
-[[ $info =~ 'cache1.example.org' ]]
-(! [[ $info =~ 'cache2.example.org' ]])
+echo $info | jq -e '.[] | .ultimate == false'
+echo $info | jq -e '.[] | .signatures.[] | select(startswith("cache1.example.org"))'
+echo $info | expect 4 jq -e '.[] | .signatures.[] | select(startswith("cache2.example.org"))'
 
 # Verify that adding a signature to a path in a binary cache works.
 nix store sign --store file://$cacheDir --key-file $TEST_ROOT/sk2 $outPath2
 info=$(nix path-info --store file://$cacheDir --json $outPath2)
-[[ $info =~ 'cache1.example.org' ]]
-[[ $info =~ 'cache2.example.org' ]]
+echo $info | jq -e '.[] | .signatures.[] | select(startswith("cache1.example.org"))'
+echo $info | jq -e '.[] | .signatures.[] | select(startswith("cache2.example.org"))'
 
 # Copying to a diverted store should fail due to a lack of signatures by trusted keys.
 chmod -R u+w $TEST_ROOT/store0 || true

diff --git a/tests/unit/libstore/data/path-info/empty_impure.json b/tests/unit/libstore/data/path-info/empty_impure.json
@@ -0,0 +1,10 @@
+{
+  "ca": null,
+  "deriver": null,
+  "narHash": "sha256-FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc=",
+  "narSize": 0,
+  "references": [],
+  "registrationTime": null,
+  "signatures": [],
+  "ultimate": false
+}
diff --git a/tests/unit/libstore/data/path-info/empty_pure.json b/tests/unit/libstore/data/path-info/empty_pure.json
@@ -0,0 +1,6 @@
+{
+  "ca": null,
+  "narHash": "sha256-FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc=",
+  "narSize": 0,
+  "references": []
+}
diff --git a/tests/unit/libstore/path-info.cc b/tests/unit/libstore/path-info.cc
@@ -19,7 +19,15 @@ class PathInfoTest : public CharacterizationTest, public LibStoreTest
     }
 };
 
-static UnkeyedValidPathInfo makePathInfo(const Store & store, bool includeImpureInfo) {
+static UnkeyedValidPathInfo makeEmpty()
+{
+    return {
+        Hash::parseSRI("sha256-FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc="),
+    };
+}
+
+static UnkeyedValidPathInfo makeFull(const Store & store, bool includeImpureInfo)
+{
     UnkeyedValidPathInfo info = ValidPathInfo {
         store,
         "foo",
@@ -50,30 +58,32 @@ static UnkeyedValidPathInfo makePathInfo(const Store & store, bool includeImpure
     return info;
 }
 
-#define JSON_TEST(STEM, PURE)                                             \
+#define JSON_TEST(STEM, OBJ, PURE)                                        \
     TEST_F(PathInfoTest, PathInfo_ ## STEM ## _from_json) {               \
         readTest(#STEM, [&](const auto & encoded_) {                      \
             auto encoded = json::parse(encoded_);                         \
             UnkeyedValidPathInfo got = UnkeyedValidPathInfo::fromJSON(    \
                 *store,                                                   \
                 encoded);                                                 \
-            auto expected = makePathInfo(*store, PURE);                   \
+            auto expected = OBJ;                                          \
             ASSERT_EQ(got, expected);                                     \
         });                                                               \
     }                                                                     \
                                                                           \
     TEST_F(PathInfoTest, PathInfo_ ## STEM ## _to_json) {                 \
         writeTest(#STEM, [&]() -> json {                                  \
-            return makePathInfo(*store, PURE)                             \
-                .toJSON(*store, PURE, HashFormat::SRI);                   \
+            return OBJ.toJSON(*store, PURE, HashFormat::SRI);             \
         }, [](const auto & file) {                                        \
             return json::parse(readFile(file));                           \
         }, [](const auto & file, const auto & got) {                      \
             return writeFile(file, got.dump(2) + "\n");                   \
         });                                                               \
     }
 
-JSON_TEST(pure, false)
-JSON_TEST(impure, true)
+JSON_TEST(empty_pure, makeEmpty(), false)
+JSON_TEST(empty_impure, makeEmpty(), true)
+
+JSON_TEST(pure, makeFull(*store, false), false)
+JSON_TEST(impure, makeFull(*store, true), true)
 
 }
diff --git a/tests/unit/libutil/json-utils.cc b/tests/unit/libutil/json-utils.cc
@@ -175,13 +175,16 @@ TEST(optionalValueAt, empty) {
 TEST(getNullable, null) {
     auto json = R"(null)"_json;
 
-    ASSERT_EQ(getNullable(json), std::nullopt);
+    ASSERT_EQ(getNullable(json), nullptr);
 }
 
 TEST(getNullable, empty) {
     auto json = R"({})"_json;
 
-    ASSERT_EQ(getNullable(json), std::optional { R"({})"_json });
+    auto * p = getNullable(json);
+
+    ASSERT_NE(p, nullptr);
+    ASSERT_EQ(*p, R"({})"_json);
 }
 
 } /* namespace nix */