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 #10087

Merged
merged 28 commits into from
Apr 3, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
7846c14
Add 'getString', 'getInteger', 'getBoolean' functions
haenoe Feb 26, 2024
91cd38d
Add 'getObject', 'getArray' functions
haenoe Feb 26, 2024
08e86ae
Merge branch 'NixOS:master' into checked-json-casting
haenoe Mar 1, 2024
ca5c19c
Add implementation for `getStringList`
haenoe Mar 1, 2024
5cc2fbf
Add implementation of `getStringMap`
haenoe Mar 1, 2024
0a40691
Add implementation of `getStringSet`
haenoe Mar 1, 2024
8284b5b
Fix small mistakes in `getObject` and `getStringList`
haenoe Mar 5, 2024
8d89c30
Fix variables that make test segfault
haenoe Mar 5, 2024
bd92d24
Add the failing value to JSON conversions
haenoe Mar 5, 2024
2e31f09
Move implementation to `ensureType`
haenoe Mar 11, 2024
dff4a94
Fix `valueAt` segfaults`
haenoe Mar 31, 2024
fbd750d
Implement `getNullable`
haenoe Mar 31, 2024
8525624
Tests for JSON utils
haenoe Mar 31, 2024
b6700cc
Fix unnecessary copy
haenoe Mar 31, 2024
5859930
Change `getNullable`, create `optionalValueAt`
haenoe Apr 2, 2024
27a16f4
Implement JSON (de)serialization for `PublicKey`
haenoe Apr 2, 2024
147d190
Fix unnecessary copies
haenoe Apr 2, 2024
769a514
Merge branch 'checked-json-casting' of github.com:haenoe/nix into che…
haenoe Apr 2, 2024
489a1d1
Fix another unneccessary copy
haenoe Apr 2, 2024
cf62af5
Revert "Implement JSON (de)serialization for `PublicKey`"
haenoe Apr 2, 2024
46814db
Add tests for `libfetchers`
haenoe Apr 3, 2024
f829e21
Simplify tests for `json-utils`
haenoe Apr 3, 2024
f1f5846
Merge branch 'master' into checked-json-casting
Ericson2314 Apr 3, 2024
2da5b88
Makefile order
Ericson2314 Apr 3, 2024
1f2c792
.gitignore order
Ericson2314 Apr 3, 2024
78f42f2
No need for libfetchers test support yet
Ericson2314 Apr 3, 2024
52eaf26
Slight fixes
Ericson2314 Apr 3, 2024
65355fb
Fix last bug
Ericson2314 Apr 3, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/libfetchers/git.cc
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,14 @@ std::vector<PublicKey> getPublicKeys(const Attrs & attrs)
std::vector<PublicKey> publicKeys;
if (attrs.contains("publicKeys")) {
nlohmann::json publicKeysJson = nlohmann::json::parse(getStrAttr(attrs, "publicKeys"));
ensureType(publicKeysJson, nlohmann::json::value_t::array);
publicKeys = publicKeysJson.get<std::vector<PublicKey>>();
auto pubKeys = getArray(publicKeysJson);
haenoe marked this conversation as resolved.
Show resolved Hide resolved
publicKeys.clear();
for (auto jsonKey : pubKeys) {
auto keyObj = getObject(jsonKey);
auto type = getString(getNullable(keyObj, "type").value_or("ssh-ed25519"));
auto key = getString(valueAt(keyObj, "key"));
publicKeys.push_back({ type, key });
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't inline things like this. Instead we can change the JSON deserializer for PublicKey to use the new combinators. (And we could test that deserializer too.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just that I understand that correctly this means implementing a new static method for PublicKey::fromJSON? ^^'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grep for JSON_IMPL. We want to keep that pattern.

Also, How did it work before? If each on was a string, it should stay that way (at least for now). This PR shouldn't be changing behavior.

}
if (attrs.contains("publicKey"))
publicKeys.push_back(PublicKey{maybeGetStrAttr(attrs, "keytype").value_or("ssh-ed25519"),getStrAttr(attrs, "publicKey")});
Expand Down
32 changes: 15 additions & 17 deletions src/libstore/derivations.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1239,8 +1239,7 @@ DerivationOutput DerivationOutput::fromJSON(
const ExperimentalFeatureSettings & xpSettings)
{
std::set<std::string_view> keys;
ensureType(_json, nlohmann::detail::value_t::object);
auto json = (std::map<std::string, nlohmann::json>) _json;
auto json = getObject(_json);

for (const auto & [key, _] : json)
keys.insert(key);
Expand Down Expand Up @@ -1357,20 +1356,20 @@ nlohmann::json Derivation::toJSON(const StoreDirConfig & store) const

Derivation Derivation::fromJSON(
const StoreDirConfig & store,
const nlohmann::json & json,
const nlohmann::json & _json,
const ExperimentalFeatureSettings & xpSettings)
{
using nlohmann::detail::value_t;

Derivation res;

ensureType(json, value_t::object);
auto json = getObject(_json);

res.name = ensureType(valueAt(json, "name"), value_t::string);
res.name = getString(valueAt(json, "name"));

try {
auto & outputsObj = ensureType(valueAt(json, "outputs"), value_t::object);
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved
for (auto & [outputName, output] : outputsObj.items()) {
auto outputsObj = getObject(valueAt(json, "outputs"));
for (auto & [outputName, output] : outputsObj) {
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved
res.outputs.insert_or_assign(
outputName,
DerivationOutput::fromJSON(store, res.name, outputName, output));
Expand All @@ -1381,7 +1380,7 @@ Derivation Derivation::fromJSON(
}

try {
auto & inputsList = ensureType(valueAt(json, "inputSrcs"), value_t::array);
auto inputsList = getArray(valueAt(json, "inputSrcs"));
for (auto & input : inputsList)
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved
res.inputSrcs.insert(store.parseStorePath(static_cast<const std::string &>(input)));
} catch (Error & e) {
Expand All @@ -1393,27 +1392,26 @@ Derivation Derivation::fromJSON(
std::function<DerivedPathMap<StringSet>::ChildNode(const nlohmann::json &)> doInput;
doInput = [&](const auto & json) {
DerivedPathMap<StringSet>::ChildNode node;
node.value = static_cast<const StringSet &>(
ensureType(valueAt(json, "outputs"), value_t::array));
for (auto & [outputId, childNode] : ensureType(valueAt(json, "dynamicOutputs"), value_t::object).items()) {
node.value = getStringSet(valueAt(json, "outputs"));
for (auto & [outputId, childNode] : getObject(valueAt(json, "dynamicOutputs"))) {
xpSettings.require(Xp::DynamicDerivations);
node.childMap[outputId] = doInput(childNode);
}
return node;
};
auto & inputDrvsObj = ensureType(valueAt(json, "inputDrvs"), value_t::object);
for (auto & [inputDrvPath, inputOutputs] : inputDrvsObj.items())
auto inputDrvsObj = getObject(valueAt(json, "inputDrvs"));
for (auto & [inputDrvPath, inputOutputs] : inputDrvsObj)
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved
res.inputDrvs.map[store.parseStorePath(inputDrvPath)] =
doInput(inputOutputs);
} catch (Error & e) {
e.addTrace({}, "while reading key 'inputDrvs'");
throw;
}

res.platform = ensureType(valueAt(json, "system"), value_t::string);
res.builder = ensureType(valueAt(json, "builder"), value_t::string);
res.args = ensureType(valueAt(json, "args"), value_t::array);
res.env = ensureType(valueAt(json, "env"), value_t::object);
res.platform = getString(valueAt(json, "system"));
res.builder = getString(valueAt(json, "builder"));
res.args = getStringList(valueAt(json, "args"));
res.env = getStringMap(valueAt(json, "env"));

return res;
}
Expand Down
9 changes: 4 additions & 5 deletions src/libstore/nar-info.cc
Original file line number Diff line number Diff line change
Expand Up @@ -172,19 +172,18 @@ NarInfo NarInfo::fromJSON(
};

if (json.contains("url"))
res.url = ensureType(valueAt(json, "url"), value_t::string);
res.url = getString(valueAt(json, "url"));

if (json.contains("compression"))
res.compression = ensureType(valueAt(json, "compression"), value_t::string);
res.compression = getString(valueAt(json, "compression"));

if (json.contains("downloadHash"))
res.fileHash = Hash::parseAny(
static_cast<const std::string &>(
ensureType(valueAt(json, "downloadHash"), value_t::string)),
getString(valueAt(json, "downloadHash")),
std::nullopt);

if (json.contains("downloadSize"))
res.fileSize = ensureType(valueAt(json, "downloadSize"), value_t::number_integer);
res.fileSize = getInteger(valueAt(json, "downloadSize"));

return res;
}
Expand Down
27 changes: 9 additions & 18 deletions src/libstore/path-info.cc
Original file line number Diff line number Diff line change
Expand Up @@ -190,23 +190,18 @@ nlohmann::json UnkeyedValidPathInfo::toJSON(

UnkeyedValidPathInfo UnkeyedValidPathInfo::fromJSON(
const Store & store,
const nlohmann::json & json)
const nlohmann::json & _json)
{
using nlohmann::detail::value_t;

UnkeyedValidPathInfo res {
Hash(Hash::dummy),
};

ensureType(json, value_t::object);
res.narHash = Hash::parseAny(
static_cast<const std::string &>(
ensureType(valueAt(json, "narHash"), value_t::string)),
std::nullopt);
res.narSize = ensureType(valueAt(json, "narSize"), value_t::number_integer);
auto json = getObject(_json);
res.narHash = Hash::parseAny(getString(valueAt(json, "narHash")), std::nullopt);
res.narSize = getInteger(valueAt(json, "narSize"));

try {
auto & references = ensureType(valueAt(json, "references"), value_t::array);
auto references = getStringList(valueAt(json, "references"));
for (auto & input : references)
res.references.insert(store.parseStorePath(static_cast<const std::string &>
(input)));
Expand All @@ -216,20 +211,16 @@ UnkeyedValidPathInfo UnkeyedValidPathInfo::fromJSON(
}

if (json.contains("ca"))
res.ca = ContentAddress::parse(
static_cast<const std::string &>(
ensureType(valueAt(json, "ca"), value_t::string)));
res.ca = ContentAddress::parse(getString(valueAt(json, "ca")));

if (json.contains("deriver"))
res.deriver = store.parseStorePath(
static_cast<const std::string &>(
ensureType(valueAt(json, "deriver"), value_t::string)));
res.deriver = store.parseStorePath(getString(valueAt(json, "deriver")));

if (json.contains("registrationTime"))
res.registrationTime = ensureType(valueAt(json, "registrationTime"), value_t::number_integer);
res.registrationTime = getInteger(valueAt(json, "registrationTime"));

if (json.contains("ultimate"))
res.ultimate = ensureType(valueAt(json, "ultimate"), value_t::boolean);
res.ultimate = getBoolean(valueAt(json, "ultimate"));

if (json.contains("signatures"))
res.sigs = valueAt(json, "signatures");
Expand Down
90 changes: 87 additions & 3 deletions src/libutil/json-utils.cc
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
#include "json-utils.hh"
#include "error.hh"
#include "types.hh"
#include <nlohmann/json_fwd.hpp>
#include <iostream>

namespace nix {

Expand All @@ -22,22 +25,103 @@ const nlohmann::json & valueAt(
const std::string & key)
{
if (!map.contains(key))
throw Error("Expected JSON object to contain key '%s' but it doesn't", key);
throw Error("Expected JSON object to contain key '%s' but it doesn't: %s", key, map.dump());

return map[key];
}

const nlohmann::json & valueAt(
const nlohmann::json::object_t & map,
const std::string & key)
{
if (!map.contains(key))
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<nlohmann::json> getNullable(const nlohmann::json & value, const std::string & key)
{
try {
auto & v = valueAt(value, key);
return v.get<nlohmann::json>();
} catch (...) {
return std::nullopt;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah this is different thing than the one I indented, which would turn null to std::nullopt and anything else to std::optional { <original> }.

This one is useful too, but maybe lets call it optionalValueAt or something. "nullable" implies null but this one isn't about null at all! Just about potentially-missing fields.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I hope we will transfer from having fewer sometimes-there-sometimes-missing fields, and more explicit nulls, so this function is hopefully just used for things we don't control and some backwards compat.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay -- makes sense! Thanks :D


const nlohmann::json & ensureType(
const nlohmann::json & value,
nlohmann::json::value_type expectedType
)
{
if (value.type() != expectedType)
throw Error(
"Expected JSON value to be of type '%s' but it is of type '%s'",
"Expected JSON value to be of type '%s' but it is of type '%s': %s",
nlohmann::json(expectedType).type_name(),
value.type_name());
value.type_name(), value.dump());

return value;
}

const nlohmann::json::object_t & getObject(const nlohmann::json & value)
{
return ensureType(value, nlohmann::json::value_t::object).get_ref<const nlohmann::json::object_t &>();
}

const nlohmann::json::array_t & getArray(const nlohmann::json & value)
{
return ensureType(value, nlohmann::json::value_t::array).get_ref<const nlohmann::json::array_t &>();
}

const nlohmann::json::string_t & getString(const nlohmann::json & value)
{
return ensureType(value, nlohmann::json::value_t::string).get_ref<const nlohmann::json::string_t &>();
}

const nlohmann::json::number_integer_t & getInteger(const nlohmann::json & value)
{
return ensureType(value, nlohmann::json::value_t::number_integer).get_ref<const nlohmann::json::number_integer_t &>();
}

const nlohmann::json::boolean_t & getBoolean(const nlohmann::json & value)
{
return ensureType(value, nlohmann::json::value_t::boolean).get_ref<const nlohmann::json::boolean_t &>();
}

Strings getStringList(const nlohmann::json & value)
{
auto jsonArray = getArray(value);

Strings stringList;

for (const auto & elem: jsonArray)
stringList.push_back(getString(elem));

return stringList;
}

StringMap getStringMap(const nlohmann::json & value)
{
auto jsonArray = getObject(value);

StringMap stringMap;

for (const auto & [key, value]: jsonArray)
stringMap[getString(key)] = getString(value);

return stringMap;
}

StringSet getStringSet(const nlohmann::json & value)
{
auto jsonArray = getArray(value);

StringSet stringSet;

for (const auto & elem: jsonArray)
stringSet.insert(getString(elem));

return stringSet;
}
}
23 changes: 22 additions & 1 deletion src/libutil/json-utils.hh
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

#include <nlohmann/json.hpp>
#include <list>
#include <nlohmann/json_fwd.hpp>
#include "types.hh"

namespace nix {

Expand All @@ -12,7 +14,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.
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved
*
* Use instead of nlohmann::json::at() to avoid ugly exceptions.
*
Expand All @@ -22,6 +24,12 @@ const nlohmann::json & valueAt(
const nlohmann::json & map,
const std::string & key);

const nlohmann::json & valueAt(
const nlohmann::json::object_t & map,
const std::string & key);

std::optional<nlohmann::json> getNullable(const nlohmann::json & value, const std::string & key);
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved

/**
* Ensure the type of a json object is what you expect, failing
* with a Nix Error if it isn't.
Expand All @@ -32,6 +40,19 @@ const nlohmann::json & ensureType(
const nlohmann::json & value,
nlohmann::json::value_type expectedType);

/**
* Downcast the json object, failing with a nice error if the conversion fails.
* See https://json.nlohmann.me/features/types/
*/
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);
const nlohmann::json::number_integer_t & getInteger(const nlohmann::json & value);
const nlohmann::json::boolean_t & getBoolean(const nlohmann::json & value);
Strings getStringList(const nlohmann::json & value);
StringMap getStringMap(const nlohmann::json & value);
StringSet getStringSet(const nlohmann::json & value);

/**
* For `adl_serializer<std::optional<T>>` below, we need to track what
* types are not already using `null`. Only for them can we use `null`
Expand Down