improve error message for explicitly empty appliesTo

[cdisselkoen]

Category

Error message improvements

Describe the feature you'd like to request

For the following schema:

action Read appliesTo {
  principal: [],
  resource: [],
};

The error message when parsing this is currently (as of 4.2.2)

  × error parsing schema: An empty list was passed

This could be more clear. It also doesn't have any locating information, like a source location, or an indication of which action had the parsing problem.

Describe alternatives you've considered

N/A

Additional context

No response

Is this something that you'd be interested in working on?

• 👋 I may be able to implement this feature request
• ⚠️ This feature might incur a breaking change

[shaobo-he-aws]

It seems a deeper issue is that parse errors thrown by lalrpop do not get access to the source string and hence cannot properly implement the label method.

[john-h-kastner-aws]

See #948 and #977 for missing source locations in cedar schema parsing errors

[cdisselkoen]

We did adjust the lalrpop for the policy format to provide the source string (#516). Perhaps we need the same change in the schema-format lalrpop file.

[shaobo-he-aws]

We did adjust the lalrpop for the policy format to provide the source string (#516). Perhaps we need the same change in the schema-format lalrpop file.

There's certain part missing in the Cedar schema parser. I'm looking into it and see if we can reuse code for both parsers.

[shaobo-he-aws]

It seems this is a CLI issue. The Report of the schema translation command doesn't have source code. After adding it, we have an error message like,

  × error parsing schema: An empty list was passed
   ╭─[<stdin>:3:3]
 2 │   principal: [],
 3 │   resource: [],
   ·   ─────────────
 4 │ };
   ╰────

Is it good enough or we want more?

[shaobo-he-aws]

To be more specific, Kesha used to point out in #977 that ToCSTError doesn't have associated source code. The situation is the same for Cedar schema parsing errors. A comprehensive fix would be to attach source code to these errors.

[john-h-kastner-aws]

Right. We could fix this in either or both places, though attaching it to the error directly could possibly be useful to callers other than the CLI.

[shaobo-he-aws]

Probably too late to bring it up: I was wondering if we should, instead of having more involved #516, let users of error structs handle/process source code. More specifically, they can use miette::Report::with_source_code, as mentioned in #516. The current design kind of forces us to have a reference to source code everywhere (e.g., in #1351).

[john-h-kastner-aws]

IMO it is better for us to attache the source code directly. It's possible that different errors could contain references into different source strings, E.g., different policies parsed from different files used together in an auth request, which would make it almost impossible for the user to figure out the correct source for each error.

[shaobo-he-aws]

IMO it is better for us to attache the source code directly. It's possible that different errors could contain references into different source strings, E.g., different policies parsed from different files used together in an auth request, which would make it almost impossible for the user to figure out the correct source for each error.

For this case, I think that it's better that users write wrappers around Cedar parser APIs that return a miette::Report.