Parse allowed* = null properly in structuredAttrs

This is the explicit way to indicate that there is no allow list.

src/libstore-tests/data/derivation/ca/advanced-attributes-structured-attrs-defaults-null.drv

@@ -0,0 +1 @@
+../../../../../tests/functional/derivation/ca/advanced-attributes-structured-attrs-defaults-null.drv

src/libstore-tests/data/derivation/ca/advanced-attributes-structured-attrs-defaults-null.json

@@ -0,0 +1,45 @@
+{
+  "args": [
+    "-c",
+    "echo hello > $out"
+  ],
+  "builder": "/bin/bash",
+  "env": {
+    "dev": "/02qcpld1y6xhs5gz9bchpxaw0xdhmsp5dv88lh25r2ss44kh8dxz",
+    "out": "/1rz4g4znpzjwh1xymhjpm42vipw92pr73vdgl6xs1hycac8kf2n9"
+  },
+  "inputs": {
+    "drvs": {},
+    "srcs": []
+  },
+  "name": "advanced-attributes-structured-attrs-defaults-null",
+  "outputs": {
+    "dev": {
+      "hashAlgo": "sha256",
+      "method": "nar"
+    },
+    "out": {
+      "hashAlgo": "sha256",
+      "method": "nar"
+    }
+  },
+  "structuredAttrs": {
+    "builder": "/bin/bash",
+    "name": "advanced-attributes-structured-attrs-defaults-null",
+    "outputChecks": {
+      "out": {
+        "allowedReferences": null,
+        "allowedRequisites": null
+      }
+    },
+    "outputHashAlgo": "sha256",
+    "outputHashMode": "recursive",
+    "outputs": [
+      "out",
+      "dev"
+    ],
+    "system": "my-system"
+  },
+  "system": "my-system",
+  "version": 4
+}

src/libstore-tests/data/derivation/ia/advanced-attributes-structured-attrs-defaults-null.drv

@@ -0,0 +1 @@
+../../../../../tests/functional/derivation/ia/advanced-attributes-structured-attrs-defaults-null.drv

src/libstore-tests/data/derivation/ia/advanced-attributes-structured-attrs-defaults-null.json

@@ -0,0 +1,41 @@
+{
+  "args": [
+    "-c",
+    "echo hello > $out"
+  ],
+  "builder": "/bin/bash",
+  "env": {
+    "dev": "/nix/store/390jivcxmgr11md7knrcyzwv9v2v64cc-advanced-attributes-structured-attrs-defaults-null-dev",
+    "out": "/nix/store/s579dvk7r4jvp7rjmzq1gy3bf9sp7b4k-advanced-attributes-structured-attrs-defaults-null"
+  },
+  "inputs": {
+    "drvs": {},
+    "srcs": []
+  },
+  "name": "advanced-attributes-structured-attrs-defaults-null",
+  "outputs": {
+    "dev": {
+      "path": "390jivcxmgr11md7knrcyzwv9v2v64cc-advanced-attributes-structured-attrs-defaults-null-dev"
+    },
+    "out": {
+      "path": "s579dvk7r4jvp7rjmzq1gy3bf9sp7b4k-advanced-attributes-structured-attrs-defaults-null"
+    }
+  },
+  "structuredAttrs": {
+    "builder": "/bin/bash",
+    "name": "advanced-attributes-structured-attrs-defaults-null",
+    "outputChecks": {
+      "out": {
+        "allowedReferences": null,
+        "allowedRequisites": null
+      }
+    },
+    "outputs": [
+      "out",
+      "dev"
+    ],
+    "system": "my-system"
+  },
+  "system": "my-system",
+  "version": 4
+}

src/libstore-tests/derivation-advanced-attrs.cc

@@ -126,6 +126,7 @@ TEST_ATERM_JSON(advancedAttributes, "advanced-attributes-defaults");
 TEST_ATERM_JSON(advancedAttributes_defaults, "advanced-attributes");
 TEST_ATERM_JSON(advancedAttributes_structuredAttrs, "advanced-attributes-structured-attrs-defaults");
 TEST_ATERM_JSON(advancedAttributes_structuredAttrs_defaults, "advanced-attributes-structured-attrs");
+TEST_ATERM_JSON(advancedAttributes_structuredAttrs_defaults_null, "advanced-attributes-structured-attrs-defaults-null");
 
 #undef TEST_ATERM_JSON
 
@@ -351,6 +352,62 @@ TEST_F(CaDerivationAdvancedAttrsTest, advancedAttributes_structuredAttrs_default
     testRequiredSystemFeatures("advanced-attributes-structured-attrs-defaults.drv", {"ca-derivations"});
 };
 
+/**
+ * Test that null values for allowedReferences and allowedRequisites are
+ * treated as "not set" (no restriction), same as if the field was missing.
+ *
+ * The outputChecks map will have an entry for "out" (since outputChecks.out
+ * exists in the nix file), but the OutputChecks for that entry should have
+ * default/empty values for the nullable fields.
+ */
+DerivationOptions<SingleDerivedPath> advancedAttributes_structuredAttrs_defaults_null = {
+    .outputChecks =
+        std::map<std::string, DerivationOptions<SingleDerivedPath>::OutputChecks>{
+            // null values result in nullopt/empty, same as if not specified
+            {"out", DerivationOptions<SingleDerivedPath>::OutputChecks{}},
+        },
+    .unsafeDiscardReferences = {},
+    .passAsFile = {},
+    .exportReferencesGraph = {},
+    .additionalSandboxProfile = "",
+    .noChroot = false,
+    .impureHostDeps = {},
+    .impureEnvVars = {},
+    .allowLocalNetworking = false,
+    .requiredSystemFeatures = {},
+    .preferLocalBuild = false,
+    .allowSubstitutes = true,
+};
+
+TYPED_TEST(DerivationAdvancedAttrsBothTest, advancedAttributes_structuredAttrs_defaults_null)
+{
+    this->readTest("advanced-attributes-structured-attrs-defaults-null.drv", [&](auto encoded) {
+        auto got = parseDerivation(*this->store, std::move(encoded), "foo", this->mockXpSettings);
+
+        auto options = derivationOptionsFromStructuredAttrs(
+            *this->store, got.inputDrvs, got.env, get(got.structuredAttrs), true, this->mockXpSettings);
+
+        EXPECT_TRUE(got.structuredAttrs);
+
+        EXPECT_EQ(options, advancedAttributes_structuredAttrs_defaults_null);
+
+        EXPECT_EQ(options.canBuildLocally(*this->store, got), false);
+        EXPECT_EQ(options.willBuildLocally(*this->store, got), false);
+        EXPECT_EQ(options.substitutesAllowed(), true);
+        EXPECT_EQ(options.useUidRange(got), false);
+    });
+};
+
+TEST_F(DerivationAdvancedAttrsTest, advancedAttributes_structuredAttrs_defaults_null)
+{
+    testRequiredSystemFeatures("advanced-attributes-structured-attrs-defaults-null.drv", {});
+};
+
+TEST_F(CaDerivationAdvancedAttrsTest, advancedAttributes_structuredAttrs_defaults_null)
+{
+    testRequiredSystemFeatures("advanced-attributes-structured-attrs-defaults-null.drv", {"ca-derivations"});
+};
+
 TYPED_TEST(DerivationAdvancedAttrsBothTest, advancedAttributes_structuredAttrs)
 {
     DerivationOptions<SingleDerivedPath> expected = {

src/libstore/derivation-options.cc

@@ -21,76 +21,41 @@ static std::optional<std::string>
 getStringAttr(const StringMap & env, const StructuredAttrs * parsed, const std::string & name)
 {
     if (parsed) {
-        auto i = parsed->structuredAttrs.find(name);
-        if (i == parsed->structuredAttrs.end())
-            return {};
-        else {
-            if (!i->second.is_string())
-                throw Error("attribute '%s' of must be a string", name);
-            return i->second.get<std::string>();
-        }
+        if (auto * i = get(parsed->structuredAttrs, name))
+            return getString(*i);
+        return {};
     } else {
-        auto i = env.find(name);
-        if (i == env.end())
-            return {};
-        else
-            return i->second;
+        if (auto * i = get(env, name))
+            return *i;
+        return {};
     }
 }
 
 static bool getBoolAttr(const StringMap & env, const StructuredAttrs * parsed, const std::string & name, bool def)
 {
     if (parsed) {
-        auto i = parsed->structuredAttrs.find(name);
-        if (i == parsed->structuredAttrs.end())
-            return def;
-        else {
-            if (!i->second.is_boolean())
-                throw Error("attribute '%s' must be a Boolean", name);
-            return i->second.get<bool>();
-        }
+        if (auto * i = get(parsed->structuredAttrs, name))
+            return getBoolean(*i);
+        return def;
     } else {
-        auto i = env.find(name);
-        if (i == env.end())
-            return def;
-        else
-            return i->second == "1";
-    }
-}
-
-static std::optional<Strings>
-getStringsAttr(const StringMap & env, const StructuredAttrs * parsed, const std::string & name)
-{
-    if (parsed) {
-        auto i = parsed->structuredAttrs.find(name);
-        if (i == parsed->structuredAttrs.end())
-            return {};
-        else {
-            if (!i->second.is_array())
-                throw Error("attribute '%s' must be a list of strings", name);
-            auto & a = getArray(i->second);
-            Strings res;
-            for (auto j = a.begin(); j != a.end(); ++j) {
-                if (!j->is_string())
-                    throw Error("attribute '%s' must be a list of strings", name);
-                res.push_back(j->get<std::string>());
-            }
-            return res;
-        }
-    } else {
-        auto i = env.find(name);
-        if (i == env.end())
-            return {};
-        else
-            return tokenizeString<Strings>(i->second);
+        if (auto * i = get(env, name))
+            return *i == "1";
+        return def;
     }
 }
 
 static std::optional<StringSet>
 getStringSetAttr(const StringMap & env, const StructuredAttrs * parsed, const std::string & name)
 {
-    auto ss = getStringsAttr(env, parsed, name);
-    return ss ? (std::optional{StringSet{ss->begin(), ss->end()}}) : (std::optional<StringSet>{});
+    if (parsed) {
+        if (auto * i = get(parsed->structuredAttrs, name))
+            return getStringSet(*i);
+        return {};
+    } else {
+        if (auto * i = get(env, name))
+            return tokenizeString<StringSet>(*i);
+        return {};
+    }
 }
 
 template<typename Inputs>
@@ -233,51 +198,108 @@ DerivationOptions<SingleDerivedPath> derivationOptionsFromStructuredAttrs(
                 std::map<std::string, OutputChecks<SingleDerivedPath>> res;
                 if (auto * outputChecks = get(structuredAttrs, "outputChecks")) {
                     for (auto & [outputName, output_] : getObject(*outputChecks)) {
-                        OutputChecks<SingleDerivedPath> checks;
-
                         auto & output = getObject(output_);
 
-                        if (auto maxSize = get(output, "maxSize"))
-                            checks.maxSize = maxSize->get<uint64_t>();
-
-                        if (auto maxClosureSize = get(output, "maxClosureSize"))
-                            checks.maxClosureSize = maxClosureSize->get<uint64_t>();
-
-                        auto get_ =
-                            [&](const std::string & name) -> std::optional<std::set<DrvRef<SingleDerivedPath>>> {
-                            if (auto i = get(output, name)) {
-                                std::set<DrvRef<SingleDerivedPath>> res;
-                                for (auto j = i->begin(); j != i->end(); ++j) {
-                                    if (!j->is_string())
-                                        throw Error("attribute '%s' must be a list of strings", name);
-                                    res.insert(parseRef(j->get<std::string>()));
-                                }
-                                return res;
-                            }
-                            return {};
+                        auto getRefSet = [&](const nlohmann::json & j) -> std::set<DrvRef<SingleDerivedPath>> {
+                            std::set<DrvRef<SingleDerivedPath>> res;
+                            for (auto & s : getStringList(j))
+                                res.insert(parseRef(s));
+                            return res;
                         };
 
                         res.insert_or_assign(
                             outputName,
                             OutputChecks<SingleDerivedPath>{
                                 .maxSize = [&]() -> std::optional<uint64_t> {
-                                    if (auto maxSize = get(output, "maxSize"))
-                                        return maxSize->get<uint64_t>();
-                                    else
-                                        return std::nullopt;
+                                    if (auto * i = get(output, "maxSize")) {
+                                        try {
+                                            return *i;
+                                        } catch (Error & e) {
+                                            e.addTrace(
+                                                {},
+                                                "while parsing attribute 'outputChecks.\"%s\".maxSize'",
+                                                outputName);
+                                            throw;
+                                        }
+                                    }
+                                    return {};
                                 }(),
                                 .maxClosureSize = [&]() -> std::optional<uint64_t> {
-                                    if (auto maxClosureSize = get(output, "maxClosureSize"))
-                                        return maxClosureSize->get<uint64_t>();
-                                    else
-                                        return std::nullopt;
+                                    if (auto * i = get(output, "maxClosureSize")) {
+                                        try {
+                                            return *i;
+                                        } catch (Error & e) {
+                                            e.addTrace(
+                                                {},
+                                                "while parsing attribute 'outputChecks.\"%s\".maxClosureSize'",
+                                                outputName);
+                                            throw;
+                                        }
+                                    }
+                                    return {};
+                                }(),
+                                .allowedReferences = [&]() -> std::optional<std::set<DrvRef<SingleDerivedPath>>> {
+                                    if (auto * i = get(output, "allowedReferences")) {
+                                        if (auto * j = getNullable(*i)) {
+                                            try {
+                                                return getRefSet(*j);
+                                            } catch (Error & e) {
+                                                e.addTrace(
+                                                    {},
+                                                    "while parsing attribute 'outputChecks.\"%s\".allowedReferences'",
+                                                    outputName);
+                                                throw;
+                                            }
+                                        }
+                                    }
+                                    return {};
                                 }(),
-                                .allowedReferences = get_("allowedReferences"),
                                 .disallowedReferences =
-                                    get_("disallowedReferences").value_or(std::set<DrvRef<SingleDerivedPath>>{}),
-                                .allowedRequisites = get_("allowedRequisites"),
+                                    [&] {
+                                        if (auto * i = get(output, "disallowedReferences")) {
+                                            try {
+                                                return getRefSet(*i);
+                                            } catch (Error & e) {
+                                                e.addTrace(
+                                                    {},
+                                                    "while parsing attribute 'outputChecks.\"%s\".disallowedReferences'",
+                                                    outputName);
+                                                throw;
+                                            }
+                                        }
+                                        return std::set<DrvRef<SingleDerivedPath>>{};
+                                    }(),
+                                .allowedRequisites = [&]() -> std::optional<std::set<DrvRef<SingleDerivedPath>>> {
+                                    if (auto * i = get(output, "allowedRequisites")) {
+                                        if (auto * j = getNullable(*i)) {
+                                            try {
+                                                return getRefSet(*j);
+                                            } catch (Error & e) {
+                                                e.addTrace(
+                                                    {},
+                                                    "while parsing attribute 'outputChecks.\"%s\".allowedRequisites'",
+                                                    outputName);
+                                                throw;
+                                            }
+                                        }
+                                    }
+                                    return {};
+                                }(),
                                 .disallowedRequisites =
-                                    get_("disallowedRequisites").value_or(std::set<DrvRef<SingleDerivedPath>>{}),
+                                    [&] {
+                                        if (auto * i = get(output, "disallowedRequisites")) {
+                                            try {
+                                                return getRefSet(*i);
+                                            } catch (Error & e) {
+                                                e.addTrace(
+                                                    {},
+                                                    "while parsing attribute 'outputChecks.\"%s\".disallowedRequisites'",
+                                                    outputName);
+                                                throw;
+                                            }
+                                        }
+                                        return std::set<DrvRef<SingleDerivedPath>>{};
+                                    }(),
                             });
                     }
                 }
@@ -307,13 +329,13 @@ DerivationOptions<SingleDerivedPath> derivationOptionsFromStructuredAttrs(
                 std::map<std::string, bool> res;
 
                 if (parsed) {
-                    auto & structuredAttrs = 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);
-                            res.insert_or_assign(outputName, output.get<bool>());
+                    if (auto * udr = get(parsed->structuredAttrs, "unsafeDiscardReferences")) {
+                        try {
+                            for (auto & [outputName, output] : getObject(*udr))
+                                res.insert_or_assign(outputName, getBoolean(output));
+                        } catch (Error & e) {
+                            e.addTrace({}, "while parsing attribute 'unsafeDiscardReferences'");
+                            throw;
                         }
                     }
                 }
@@ -340,9 +362,13 @@ DerivationOptions<SingleDerivedPath> derivationOptionsFromStructuredAttrs(
                 std::map<std::string, std::set<SingleDerivedPath>> ret;
 
                 if (parsed) {
-                    auto * e = optionalValueAt(parsed->structuredAttrs, "exportReferencesGraph");
-                    if (!e || !e->is_object())
+                    auto * e = get(parsed->structuredAttrs, "exportReferencesGraph");
+                    if (!e)
                         return ret;
+                    if (!e->is_object()) {
+                        warn("'exportReferencesGraph' in structured attrs is not a JSON object, ignoring");
+                        return ret;
+                    }
                     for (auto & [key, storePathsJson] : getObject(*e)) {
                         StringSet ss;
                         flatten(storePathsJson, ss);
@@ -591,8 +617,8 @@ DerivationOptions<SingleDerivedPath> adl_serializer<DerivationOptions<SingleDeri
         .outputChecks = [&]() -> OutputChecksVariant<SingleDerivedPath> {
             auto outputChecks = getObject(valueAt(json, "outputChecks"));
 
-            auto forAllOutputsOpt = optionalValueAt(outputChecks, "forAllOutputs");
-            auto perOutputOpt = optionalValueAt(outputChecks, "perOutput");
+            auto forAllOutputsOpt = get(outputChecks, "forAllOutputs");
+            auto perOutputOpt = get(outputChecks, "perOutput");
 
             if (forAllOutputsOpt && !perOutputOpt) {
                 return static_cast<OutputChecks<SingleDerivedPath>>(*forAllOutputsOpt);

src/libutil/include/nix/util/json-utils.hh

@@ -75,6 +75,15 @@ Strings getStringList(const nlohmann::json & value);
 StringMap getStringMap(const nlohmann::json & value);
 StringSet getStringSet(const nlohmann::json & value);
 
+template<typename T>
+static inline std::optional<T> ptrToOwned(const nlohmann::json * ptr)
+{
+    if (ptr)
+        return std::optional{*ptr};
+    else
+        return std::nullopt;
+}
+
 } // namespace nix
 
 namespace nlohmann {
@@ -114,13 +123,4 @@ struct adl_serializer<std::optional<T>>
     }
 };
 
-template<typename T>
-static inline std::optional<T> ptrToOwned(const json * ptr)
-{
-    if (ptr)
-        return std::optional{*ptr};
-    else
-        return std::nullopt;
-}
-
 } // namespace nlohmann

tests/functional/derivation/advanced-attributes-structured-attrs-defaults-null.nix

@@ -0,0 +1,39 @@
+{ contentAddress }:
+
+let
+  caArgs =
+    if contentAddress then
+      {
+        __contentAddressed = true;
+        outputHashMode = "recursive";
+        outputHashAlgo = "sha256";
+      }
+    else
+      { };
+
+  derivation' = args: derivation (caArgs // args);
+
+  system = "my-system";
+
+in
+derivation' {
+  inherit system;
+  name = "advanced-attributes-structured-attrs-defaults-null";
+  builder = "/bin/bash";
+  args = [
+    "-c"
+    "echo hello > $out"
+  ];
+  outputs = [
+    "out"
+    "dev"
+  ];
+  __structuredAttrs = true;
+  outputChecks = {
+    out = {
+      # Test that null is treated as "not set" (no restriction)
+      allowedReferences = null;
+      allowedRequisites = null;
+    };
+  };
+}

tests/functional/derivation/ca/advanced-attributes-structured-attrs-defaults-null.drv

@@ -0,0 +1 @@
+Derive([("dev","","r:sha256",""),("out","","r:sha256","")],[],[],"my-system","/bin/bash",["-c","echo hello > $out"],[("__json","{\"builder\":\"/bin/bash\",\"name\":\"advanced-attributes-structured-attrs-defaults-null\",\"outputChecks\":{\"out\":{\"allowedReferences\":null,\"allowedRequisites\":null}},\"outputHashAlgo\":\"sha256\",\"outputHashMode\":\"recursive\",\"outputs\":[\"out\",\"dev\"],\"system\":\"my-system\"}"),("dev","/02qcpld1y6xhs5gz9bchpxaw0xdhmsp5dv88lh25r2ss44kh8dxz"),("out","/1rz4g4znpzjwh1xymhjpm42vipw92pr73vdgl6xs1hycac8kf2n9")])

tests/functional/derivation/ia/advanced-attributes-structured-attrs-defaults-null.drv

@@ -0,0 +1 @@
+Derive([("dev","/nix/store/390jivcxmgr11md7knrcyzwv9v2v64cc-advanced-attributes-structured-attrs-defaults-null-dev","",""),("out","/nix/store/s579dvk7r4jvp7rjmzq1gy3bf9sp7b4k-advanced-attributes-structured-attrs-defaults-null","","")],[],[],"my-system","/bin/bash",["-c","echo hello > $out"],[("__json","{\"builder\":\"/bin/bash\",\"name\":\"advanced-attributes-structured-attrs-defaults-null\",\"outputChecks\":{\"out\":{\"allowedReferences\":null,\"allowedRequisites\":null}},\"outputs\":[\"out\",\"dev\"],\"system\":\"my-system\"}"),("dev","/nix/store/390jivcxmgr11md7knrcyzwv9v2v64cc-advanced-attributes-structured-attrs-defaults-null-dev"),("out","/nix/store/s579dvk7r4jvp7rjmzq1gy3bf9sp7b4k-advanced-attributes-structured-attrs-defaults-null")])