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

Error message is misleading when defining an attribute incorrectly in schema JSON #417

Closed
3 tasks done
cdisselkoen opened this issue Nov 9, 2023 · 2 comments · Fixed by #1270
Closed
3 tasks done
Labels
papercut Small annoyances in the Cedar SDK. Lower priority fixes than bugs. Smaller than a fature request

Comments

@cdisselkoen
Copy link
Contributor

Before opening, please confirm:

Bug Category

Schemas and Validation

Describe the bug

For the below schema, the reported error is

unknown field `baz`, expected one of `type`, `element`, `attributes`, `additionalAttributes`, `name`

This is misleading because baz is a completely valid field (attribute name), and changing baz to some other string (like the ones listed) will not fix the problem.

Instead, the issue is in the bar field. The set element type was listed as "Long" instead of { "type": "Long" }.

If you fix the set-element mistake, you see that a similar mistake was made in the baz field, but that one actually has a reasonable-ish error message once you fix the set-element mistake:

invalid type: string "Boolean", expected struct TypeOfAttribute

This actually points to the correct problem location ("Boolean"), and from this error message an end-user could probably decipher a hint that they need a JSON object instead of just the string "Boolean". (This error message itself could definitely be improved, too; for instance, we could suggest that you need { "type": ... } instead of using the cryptic term TypeOfAttribute; and we could even perhaps suggest { "type": "Boolean" } in this particular case.)

Expected behavior

A better error message -- at least one that points to the bar attribute instead of baz, and ideally a reasonable suggested fix. See above.

Reproduction steps

Try to parse the schema given below

Code Snippet

{ "": {
   "entityTypes": {
       "a": {
           "shape": {
               "type": "Record",
               "attributes": {
                    "foo": { "type": "Entity", "name": "b" },
                    "bar": { "type": "Set", "element": "Long" },
                    "baz": { "type": "Record", "attributes": { "z": "Boolean" } }
               }
           }
       },
       "b": {}
   }
} }

Log output

No response

Additional configuration

No response

Operating System

No response

Additional information and screenshots

No response

@cdisselkoen cdisselkoen added bug Something isn't working. This is as high priority issue. pending-triage The cedar maintainers haven't looked at this yet. Automicaly added to all new issues. backlog and removed pending-triage The cedar maintainers haven't looked at this yet. Automicaly added to all new issues. labels Nov 9, 2023
@john-h-kastner-aws
Copy link
Contributor

john-h-kastner-aws commented Nov 9, 2023

That's a subtle bug.

It's an unintended consequence of the behavior noted here and how it interacts with some artifact of how serde handles its internal state.

What's happening is that

  1. [derived deserializer] See, shape field, start parsing its value as a Type
  2. [custom deserializer] See, attributes field, start parsing its value as a HashMap<SmolStr, Type>
  3. [derived deserializer] Parse each value in the map as a Type, in the order they appear
  4. [custom deserializer] Parse value for foo
  5. [custom deserializer] Parse value for bar. Error due to "element": "Long".
  6. [derived deserializer] See error result from bar value. Return error immediately and without consuming remaining key/value pairs.
  7. [custom deserializer] See error result from parsing attributes. Note error, but don't return yet. If things were working properly, we'd return this error after checking if there are any keys we know shouldn't exist, because we want to return that type of error before complaining about the value of particular keys.
  8. [custom deserializer] Ask serde for the next key/value pair. It returns the baz entry. That's not a valid field, so now we error. This would be the intended behavior if baz did exist in the JSON object for the record type.

@sarahcec sarahcec moved this to Todo in Cedar 3.2 Status Nov 14, 2023
@sarahcec sarahcec added papercut Small annoyances in the Cedar SDK. Lower priority fixes than bugs. Smaller than a fature request and removed bug Something isn't working. This is as high priority issue. labels Jan 2, 2024
@john-h-kastner-aws
Copy link
Contributor

Another example of this class of error came up recently. The only difference is that the error on the top level attribute instead of a nested attribute.

{
  "": {
    "entityTypes": {
      "User": {
        "shape": {
          "type": "Record",
          "attributes": {
            "foo": {
              "type": "Record",
              "element": { "type": "Long" }
            },
            "bar": { "type": "Long" }
          }
        }
      }
    },
    "actions": {}
  }
}

error is

  × failed to parse schema from file test.cedarschema.json
  ╰─▶ unknown field `bar`, expected one of `type`, `element`, `attributes`, `additionalAttributes`, `name` at line 12 column 17

FWIW, this issue should be easy to fix if we don't mind having slight less helpful error messages when a JSON schema contains parse errors inside unexpected fields. It would complain about the error in the field before complaining about the existence of the field. This error as it stands is bad enough that IMO it's easily worth it.

john-h-kastner-aws added a commit that referenced this issue Sep 23, 2024
Signed-off-by: John Kastner <jkastner@amazon.com>
john-h-kastner-aws added a commit that referenced this issue Oct 8, 2024
Signed-off-by: John Kastner <jkastner@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
papercut Small annoyances in the Cedar SDK. Lower priority fixes than bugs. Smaller than a fature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants