Skip to content

Commit 0f0d925

Browse files
Ericson2314xokdvium
Ericson2314
and
xokdvium
committed
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 408c09a, which prevents bugs / unnecessary copies. `adl_serializer<DerivationOptions::OutputChecks>::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 <[email protected]>
1 parent f0b95b6 commit 0f0d925

File tree

6 files changed

+99
-92
lines changed

6 files changed

+99
-92
lines changed

‎src/libfetchers/fetchers.cc‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -519,10 +519,11 @@ using namespace nix;
519519
fetchers::PublicKey adl_serializer<fetchers::PublicKey>::from_json(const json & json)
520520
{
521521
fetchers::PublicKey res = {};
522-
if (auto type = optionalValueAt(json, "type"))
522+
auto & obj = getObject(json);
523+
if (auto * type = optionalValueAt(obj, "type"))
523524
res.type = getString(*type);
524525

525-
res.key = getString(valueAt(json, "key"));
526+
res.key = getString(valueAt(obj, "key"));
526527

527528
return res;
528529
}

‎src/libstore/derivation-options.cc‎

Lines changed: 35 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -116,27 +116,29 @@ DerivationOptions::fromStructuredAttrs(const StringMap & env, const StructuredAt
116116
DerivationOptions defaults = {};
117117

118118
if (shouldWarn && parsed) {
119-
if (get(parsed->structuredAttrs, "allowedReferences")) {
119+
auto & structuredAttrs = getObject(parsed->structuredAttrs);
120+
121+
if (get(structuredAttrs, "allowedReferences")) {
120122
warn(
121123
"'structuredAttrs' disables the effect of the top-level attribute 'allowedReferences'; use 'outputChecks' instead");
122124
}
123-
if (get(parsed->structuredAttrs, "allowedRequisites")) {
125+
if (get(structuredAttrs, "allowedRequisites")) {
124126
warn(
125127
"'structuredAttrs' disables the effect of the top-level attribute 'allowedRequisites'; use 'outputChecks' instead");
126128
}
127-
if (get(parsed->structuredAttrs, "disallowedRequisites")) {
129+
if (get(structuredAttrs, "disallowedRequisites")) {
128130
warn(
129131
"'structuredAttrs' disables the effect of the top-level attribute 'disallowedRequisites'; use 'outputChecks' instead");
130132
}
131-
if (get(parsed->structuredAttrs, "disallowedReferences")) {
133+
if (get(structuredAttrs, "disallowedReferences")) {
132134
warn(
133135
"'structuredAttrs' disables the effect of the top-level attribute 'disallowedReferences'; use 'outputChecks' instead");
134136
}
135-
if (get(parsed->structuredAttrs, "maxSize")) {
137+
if (get(structuredAttrs, "maxSize")) {
136138
warn(
137139
"'structuredAttrs' disables the effect of the top-level attribute 'maxSize'; use 'outputChecks' instead");
138140
}
139-
if (get(parsed->structuredAttrs, "maxClosureSize")) {
141+
if (get(structuredAttrs, "maxClosureSize")) {
140142
warn(
141143
"'structuredAttrs' disables the effect of the top-level attribute 'maxClosureSize'; use 'outputChecks' instead");
142144
}
@@ -145,11 +147,15 @@ DerivationOptions::fromStructuredAttrs(const StringMap & env, const StructuredAt
145147
return {
146148
.outputChecks = [&]() -> OutputChecksVariant {
147149
if (parsed) {
150+
auto & structuredAttrs = getObject(parsed->structuredAttrs);
151+
148152
std::map<std::string, OutputChecks> res;
149-
if (auto outputChecks = get(parsed->structuredAttrs, "outputChecks")) {
150-
for (auto & [outputName, output] : getObject(*outputChecks)) {
153+
if (auto * outputChecks = get(structuredAttrs, "outputChecks")) {
154+
for (auto & [outputName, output_] : getObject(*outputChecks)) {
151155
OutputChecks checks;
152156

157+
auto & output = getObject(output_);
158+
153159
if (auto maxSize = get(output, "maxSize"))
154160
checks.maxSize = maxSize->get<uint64_t>();
155161

@@ -195,7 +201,9 @@ DerivationOptions::fromStructuredAttrs(const StringMap & env, const StructuredAt
195201
std::map<std::string, bool> res;
196202

197203
if (parsed) {
198-
if (auto udr = get(parsed->structuredAttrs, "unsafeDiscardReferences")) {
204+
auto & structuredAttrs = getObject(parsed->structuredAttrs);
205+
206+
if (auto * udr = get(structuredAttrs, "unsafeDiscardReferences")) {
199207
for (auto & [outputName, output] : getObject(*udr)) {
200208
if (!output.is_boolean())
201209
throw Error("attribute 'unsafeDiscardReferences.\"%s\"' must be a Boolean", outputName);
@@ -226,7 +234,7 @@ DerivationOptions::fromStructuredAttrs(const StringMap & env, const StructuredAt
226234
std::map<std::string, StringSet> ret;
227235

228236
if (parsed) {
229-
auto e = optionalValueAt(parsed->structuredAttrs, "exportReferencesGraph");
237+
auto e = optionalValueAt(getObject(parsed->structuredAttrs), "exportReferencesGraph");
230238
if (!e || !e->is_object())
231239
return ret;
232240
for (auto & [key, value] : getObject(*e)) {
@@ -333,8 +341,10 @@ namespace nlohmann {
333341

334342
using namespace nix;
335343

336-
DerivationOptions adl_serializer<DerivationOptions>::from_json(const json & json)
344+
DerivationOptions adl_serializer<DerivationOptions>::from_json(const json & json_)
337345
{
346+
auto & json = getObject(json_);
347+
338348
return {
339349
.outputChecks = [&]() -> OutputChecksVariant {
340350
auto outputChecks = getObject(valueAt(json, "outputChecks"));
@@ -397,13 +407,24 @@ void adl_serializer<DerivationOptions>::to_json(json & json, const DerivationOpt
397407
json["allowSubstitutes"] = o.allowSubstitutes;
398408
}
399409

400-
DerivationOptions::OutputChecks adl_serializer<DerivationOptions::OutputChecks>::from_json(const json & json)
410+
template<typename T>
411+
static inline std::optional<T> ptrToOwned(const json * ptr)
401412
{
413+
if (ptr)
414+
return std::optional{*ptr};
415+
else
416+
return std::nullopt;
417+
}
418+
419+
DerivationOptions::OutputChecks adl_serializer<DerivationOptions::OutputChecks>::from_json(const json & json_)
420+
{
421+
auto & json = getObject(json_);
422+
402423
return {
403424
.ignoreSelfRefs = getBoolean(valueAt(json, "ignoreSelfRefs")),
404-
.allowedReferences = nullableValueAt(json, "allowedReferences"),
425+
.allowedReferences = ptrToOwned<StringSet>(getNullable(valueAt(json, "allowedReferences"))),
405426
.disallowedReferences = getStringSet(valueAt(json, "disallowedReferences")),
406-
.allowedRequisites = nullableValueAt(json, "allowedRequisites"),
427+
.allowedRequisites = ptrToOwned<StringSet>(getNullable(valueAt(json, "allowedRequisites"))),
407428
.disallowedRequisites = getStringSet(valueAt(json, "disallowedRequisites")),
408429
};
409430
}

‎src/libstore/nar-info.cc‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -159,17 +159,19 @@ NarInfo NarInfo::fromJSON(const StoreDirConfig & store, const StorePath & path,
159159
UnkeyedValidPathInfo::fromJSON(store, json),
160160
}};
161161

162+
auto & obj = getObject(json);
163+
162164
if (json.contains("url"))
163-
res.url = getString(valueAt(json, "url"));
165+
res.url = getString(valueAt(obj, "url"));
164166

165167
if (json.contains("compression"))
166-
res.compression = getString(valueAt(json, "compression"));
168+
res.compression = getString(valueAt(obj, "compression"));
167169

168170
if (json.contains("downloadHash"))
169-
res.fileHash = Hash::parseAny(getString(valueAt(json, "downloadHash")), std::nullopt);
171+
res.fileHash = Hash::parseAny(getString(valueAt(obj, "downloadHash")), std::nullopt);
170172

171173
if (json.contains("downloadSize"))
172-
res.fileSize = getUnsigned(valueAt(json, "downloadSize"));
174+
res.fileSize = getUnsigned(valueAt(obj, "downloadSize"));
173175

174176
return res;
175177
}

‎src/libutil-tests/json-utils.cc‎

Lines changed: 36 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ TEST(valueAt, simpleObject)
7070

7171
auto nested = R"({ "hello": { "world": "" } })"_json;
7272

73-
ASSERT_EQ(valueAt(valueAt(getObject(nested), "hello"), "world"), "");
73+
ASSERT_EQ(valueAt(getObject(valueAt(getObject(nested), "hello")), "world"), "");
7474
}
7575

7676
TEST(valueAt, missingKey)
@@ -119,10 +119,12 @@ TEST(getArray, wrongAssertions)
119119
{
120120
auto json = R"({ "object": {}, "array": [], "string": "", "int": 0, "boolean": false })"_json;
121121

122-
ASSERT_THROW(getArray(valueAt(json, "object")), Error);
123-
ASSERT_THROW(getArray(valueAt(json, "string")), Error);
124-
ASSERT_THROW(getArray(valueAt(json, "int")), Error);
125-
ASSERT_THROW(getArray(valueAt(json, "boolean")), Error);
122+
auto & obj = getObject(json);
123+
124+
ASSERT_THROW(getArray(valueAt(obj, "object")), Error);
125+
ASSERT_THROW(getArray(valueAt(obj, "string")), Error);
126+
ASSERT_THROW(getArray(valueAt(obj, "int")), Error);
127+
ASSERT_THROW(getArray(valueAt(obj, "boolean")), Error);
126128
}
127129

128130
TEST(getString, rightAssertions)
@@ -136,10 +138,12 @@ TEST(getString, wrongAssertions)
136138
{
137139
auto json = R"({ "object": {}, "array": [], "string": "", "int": 0, "boolean": false })"_json;
138140

139-
ASSERT_THROW(getString(valueAt(json, "object")), Error);
140-
ASSERT_THROW(getString(valueAt(json, "array")), Error);
141-
ASSERT_THROW(getString(valueAt(json, "int")), Error);
142-
ASSERT_THROW(getString(valueAt(json, "boolean")), Error);
141+
auto & obj = getObject(json);
142+
143+
ASSERT_THROW(getString(valueAt(obj, "object")), Error);
144+
ASSERT_THROW(getString(valueAt(obj, "array")), Error);
145+
ASSERT_THROW(getString(valueAt(obj, "int")), Error);
146+
ASSERT_THROW(getString(valueAt(obj, "boolean")), Error);
143147
}
144148

145149
TEST(getIntegralNumber, rightAssertions)
@@ -156,18 +160,20 @@ TEST(getIntegralNumber, wrongAssertions)
156160
auto json =
157161
R"({ "object": {}, "array": [], "string": "", "int": 0, "signed": -256, "large": 128, "boolean": false })"_json;
158162

159-
ASSERT_THROW(getUnsigned(valueAt(json, "object")), Error);
160-
ASSERT_THROW(getUnsigned(valueAt(json, "array")), Error);
161-
ASSERT_THROW(getUnsigned(valueAt(json, "string")), Error);
162-
ASSERT_THROW(getUnsigned(valueAt(json, "boolean")), Error);
163-
ASSERT_THROW(getUnsigned(valueAt(json, "signed")), Error);
163+
auto & obj = getObject(json);
164+
165+
ASSERT_THROW(getUnsigned(valueAt(obj, "object")), Error);
166+
ASSERT_THROW(getUnsigned(valueAt(obj, "array")), Error);
167+
ASSERT_THROW(getUnsigned(valueAt(obj, "string")), Error);
168+
ASSERT_THROW(getUnsigned(valueAt(obj, "boolean")), Error);
169+
ASSERT_THROW(getUnsigned(valueAt(obj, "signed")), Error);
164170

165-
ASSERT_THROW(getInteger<int8_t>(valueAt(json, "object")), Error);
166-
ASSERT_THROW(getInteger<int8_t>(valueAt(json, "array")), Error);
167-
ASSERT_THROW(getInteger<int8_t>(valueAt(json, "string")), Error);
168-
ASSERT_THROW(getInteger<int8_t>(valueAt(json, "boolean")), Error);
169-
ASSERT_THROW(getInteger<int8_t>(valueAt(json, "large")), Error);
170-
ASSERT_THROW(getInteger<int8_t>(valueAt(json, "signed")), Error);
171+
ASSERT_THROW(getInteger<int8_t>(valueAt(obj, "object")), Error);
172+
ASSERT_THROW(getInteger<int8_t>(valueAt(obj, "array")), Error);
173+
ASSERT_THROW(getInteger<int8_t>(valueAt(obj, "string")), Error);
174+
ASSERT_THROW(getInteger<int8_t>(valueAt(obj, "boolean")), Error);
175+
ASSERT_THROW(getInteger<int8_t>(valueAt(obj, "large")), Error);
176+
ASSERT_THROW(getInteger<int8_t>(valueAt(obj, "signed")), Error);
171177
}
172178

173179
TEST(getBoolean, rightAssertions)
@@ -181,24 +187,28 @@ TEST(getBoolean, wrongAssertions)
181187
{
182188
auto json = R"({ "object": {}, "array": [], "string": "", "int": 0, "boolean": false })"_json;
183189

184-
ASSERT_THROW(getBoolean(valueAt(json, "object")), Error);
185-
ASSERT_THROW(getBoolean(valueAt(json, "array")), Error);
186-
ASSERT_THROW(getBoolean(valueAt(json, "string")), Error);
187-
ASSERT_THROW(getBoolean(valueAt(json, "int")), Error);
190+
auto & obj = getObject(json);
191+
192+
ASSERT_THROW(getBoolean(valueAt(obj, "object")), Error);
193+
ASSERT_THROW(getBoolean(valueAt(obj, "array")), Error);
194+
ASSERT_THROW(getBoolean(valueAt(obj, "string")), Error);
195+
ASSERT_THROW(getBoolean(valueAt(obj, "int")), Error);
188196
}
189197

190198
TEST(optionalValueAt, existing)
191199
{
192200
auto json = R"({ "string": "ssh-rsa" })"_json;
193201

194-
ASSERT_EQ(optionalValueAt(json, "string"), std::optional{"ssh-rsa"});
202+
auto * ptr = optionalValueAt(getObject(json), "string");
203+
ASSERT_TRUE(ptr);
204+
ASSERT_EQ(*ptr, R"("ssh-rsa")"_json);
195205
}
196206

197207
TEST(optionalValueAt, empty)
198208
{
199209
auto json = R"({})"_json;
200210

201-
ASSERT_EQ(optionalValueAt(json, "string"), std::nullopt);
211+
ASSERT_EQ(optionalValueAt(getObject(json), "string"), nullptr);
202212
}
203213

204214
TEST(getNullable, null)

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

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
///@file
33

44
#include <nlohmann/json.hpp>
5-
#include <list>
65

76
#include "nix/util/error.hh"
87
#include "nix/util/types.hh"
@@ -12,20 +11,25 @@ namespace nix {
1211

1312
enum struct ExperimentalFeature;
1413

15-
const nlohmann::json * get(const nlohmann::json & map, const std::string & key);
16-
17-
nlohmann::json * get(nlohmann::json & map, const std::string & key);
18-
1914
/**
2015
* Get the value of a json object at a key safely, failing with a nice
2116
* error if the key does not exist.
2217
*
2318
* Use instead of nlohmann::json::at() to avoid ugly exceptions.
2419
*/
25-
const nlohmann::json & valueAt(const nlohmann::json::object_t & map, const std::string & key);
20+
const nlohmann::json & valueAt(const nlohmann::json::object_t & map, std::string_view key);
2621

27-
std::optional<nlohmann::json> optionalValueAt(const nlohmann::json::object_t & value, const std::string & key);
28-
std::optional<nlohmann::json> nullableValueAt(const nlohmann::json::object_t & value, const std::string & key);
22+
/**
23+
* @return A pointer to the value assiocated with `key` if `value`
24+
* contains `key`, otherwise return `nullptr` (not JSON `null`!).
25+
*/
26+
const nlohmann::json * optionalValueAt(const nlohmann::json::object_t & value, std::string_view key);
27+
28+
/**
29+
* Prevents bugs; see `get` for the same trick.
30+
*/
31+
const nlohmann::json & valueAt(nlohmann::json::object_t && map, std::string_view key) = delete;
32+
const nlohmann::json * optionalValueAt(nlohmann::json::object_t && value, std::string_view key) = delete;
2933

3034
/**
3135
* Downcast the json object, failing with a nice error if the conversion fails.

‎src/libutil/json-utils.cc‎

Lines changed: 7 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,52 +1,21 @@
11
#include "nix/util/json-utils.hh"
22
#include "nix/util/error.hh"
33
#include "nix/util/types.hh"
4-
#include <nlohmann/json_fwd.hpp>
5-
#include <iostream>
6-
#include <optional>
4+
#include "nix/util/util.hh"
75

86
namespace nix {
97

10-
const nlohmann::json * get(const nlohmann::json & map, const std::string & key)
8+
const nlohmann::json & valueAt(const nlohmann::json::object_t & map, std::string_view key)
119
{
12-
auto i = map.find(key);
13-
if (i == map.end())
14-
return nullptr;
15-
return &*i;
16-
}
17-
18-
nlohmann::json * get(nlohmann::json & map, const std::string & key)
19-
{
20-
auto i = map.find(key);
21-
if (i == map.end())
22-
return nullptr;
23-
return &*i;
24-
}
25-
26-
const nlohmann::json & valueAt(const nlohmann::json::object_t & map, const std::string & key)
27-
{
28-
if (!map.contains(key))
10+
if (auto * p = optionalValueAt(map, key))
11+
return *p;
12+
else
2913
throw Error("Expected JSON object to contain key '%s' but it doesn't: %s", key, nlohmann::json(map).dump());
30-
31-
return map.at(key);
3214
}
3315

34-
std::optional<nlohmann::json> optionalValueAt(const nlohmann::json::object_t & map, const std::string & key)
16+
const nlohmann::json * optionalValueAt(const nlohmann::json::object_t & map, std::string_view key)
3517
{
36-
if (!map.contains(key))
37-
return std::nullopt;
38-
39-
return std::optional{map.at(key)};
40-
}
41-
42-
std::optional<nlohmann::json> nullableValueAt(const nlohmann::json::object_t & map, const std::string & key)
43-
{
44-
auto value = valueAt(map, key);
45-
46-
if (value.is_null())
47-
return std::nullopt;
48-
49-
return std::optional{std::move(value)};
18+
return get(map, key);
5019
}
5120

5221
const nlohmann::json * getNullable(const nlohmann::json & value)

0 commit comments

Comments
 (0)