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

Fix derivation load assertion errors #8760

Merged

Conversation

iFreilicht
Copy link
Contributor

@iFreilicht iFreilicht commented Jul 30, 2023

Display nice error messages instead and give some indication where the error happened.

Before:

$ echo 4 | nix derivation add
error: [json.exception.type_error.305] cannot use operator[] with a string argument with number

$ nix derivation show nixpkgs#hello | nix derivation add
Assertion failed: (it != m_value.object->end()), function operator[], file /nix/store/8h9pxgq1776ns6qi5arx08ifgnhmgl22-nlohmann_json-3.11.2/include/nlohmann/json.hpp, line 2135.

$ nix derivation show nixpkgs#hello | jq '.[] | .name = 5' | nix derivation add
error: [json.exception.type_error.302] type must be string, but is object

$ nix derivation show nixpkgs#hello | jq '.[] | .outputs = { out: "/nix/store/8j3f8j-hello" }' | nix derivation add
error: [json.exception.type_error.302] type must be object, but is string

After:

$ echo 4 | nix derivation add
error: Expected JSON of derivation to be of type 'object', but it is of type 'number'

$ nix derivation show nixpkgs#hello | nix derivation add
error: Expected JSON object to contain key 'name' but it doesn't

$ nix derivation show nixpkgs#hello | jq '.[] | .name = 5' | nix derivation add
error:
       … while reading value with key 'name'

       error: Expected JSON value to be of type 'string' but it is of type 'number'

$ nix derivation show nixpkgs#hello | jq '.[] | .outputs = { out: "/nix/store/8j3f8j-hello" }' | nix derivation add
error:
       … while reading key 'outputs'

       … while reading value with key 'out'

       error: Expected JSON value to be of type 'object' but it is of type 'string'

Motivation

When loading a derivation from a JSON, malformed input would trigger cryptic "assertion failed" errors. Simply replacing calls to operator [] with calls to .at() was not good enough IMO, as this would cause json.execptions to be printed verbatim.

Context

Fixes the complaint about bad error messages in #8747 as requested in this comment.

The PR adds a new method valueOfTypeAt that accesses the value via a key in a JSON object, ensures it is of a certain type and errors with a decent explanation text if the key does not exist or it is of the wrong type.
It specifically does not check whether map is actually an object. This should be done at the callsite, where information about the context of map is available.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • documentation in the internal API docs
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

@iFreilicht iFreilicht force-pushed the fix-json-load-assertion-errors branch from 2057dc1 to b72be3b Compare July 30, 2023 20:48
@roberth roberth added error-messages Confusing messages and better diagnostics store Issues and pull requests concerning the Nix store labels Jul 31, 2023
@thufschmitt thufschmitt added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Jul 31, 2023
@iFreilicht iFreilicht force-pushed the fix-json-load-assertion-errors branch from b72be3b to c02e6e6 Compare July 31, 2023 18:19
@github-actions github-actions bot removed the store Issues and pull requests concerning the Nix store label Jul 31, 2023
@iFreilicht iFreilicht force-pushed the fix-json-load-assertion-errors branch from c02e6e6 to 03b676f Compare July 31, 2023 18:25
@iFreilicht iFreilicht force-pushed the fix-json-load-assertion-errors branch from 03b676f to 45f490b Compare August 2, 2023 19:57
When loading a derivation from a JSON, malformed input would trigger
cryptic "assertion failed" errors. Simply replacing calls to `operator []`
with calls to `.at()` was not enough, as this would cause json.execptions
to be printed verbatim.

Display nice error messages instead and give some indication where the
error happened.

*Before:*

```
$ echo 4 | nix derivation add
error: [json.exception.type_error.305] cannot use operator[] with a string argument with number

$ nix derivation show nixpkgs#hello | nix derivation add
Assertion failed: (it != m_value.object->end()), function operator[], file /nix/store/8h9pxgq1776ns6qi5arx08ifgnhmgl22-nlohmann_json-3.11.2/include/nlohmann/json.hpp, line 2135.

$ nix derivation show nixpkgs#hello | jq '.[] | .name = 5' | nix derivation add
error: [json.exception.type_error.302] type must be string, but is object

$ nix derivation show nixpkgs#hello | jq '.[] | .outputs = { out: "/nix/store/8j3f8j-hello" }' | nix derivation add
error: [json.exception.type_error.302] type must be object, but is string

```

*After:*

```
$ echo 4 | nix derivation add
error: Expected JSON of derivation to be of type 'object', but it is of type 'number'

$ nix derivation show nixpkgs#hello | nix derivation add
error: Expected JSON object to contain key 'name' but it doesn't

$ nix derivation show nixpkgs#hello | jq '.[] | .name = 5' | nix derivation add
error: Expected JSON value to be of type 'string' but it is of type 'number'

$ nix derivation show nixpkgs#hello | jq '.[] | .outputs = { out: "/nix/store/8j3f8j-hello" }' | nix derivation add
error:
       … while reading key 'outputs'

       error: Expected JSON value to be of type 'object' but it is of type 'string'
```
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Thanks! This looks quite good now.

@Ericson2314 Ericson2314 merged commit 9113b42 into NixOS:master Aug 7, 2023
8 checks passed
@Ericson2314
Copy link
Member

BTW see nlohmann/json#4100

@iFreilicht iFreilicht deleted the fix-json-load-assertion-errors branch August 7, 2023 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error-messages Confusing messages and better diagnostics idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants