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

When parsing a StaticPolicySet from a text representation, only the first error gets recorded. #1293

Open
3 tasks done
AndreasKoestler opened this issue Oct 25, 2024 · 3 comments
Labels
papercut Small annoyances in the Cedar SDK. Lower priority fixes than bugs. Smaller than a fature request

Comments

@AndreasKoestler
Copy link

Before opening, please confirm:

Bug Category

Cedar Parser

Describe the bug

When parsing a StaticPolicySet from a text representation, only the first error gets recorded. This makes handling the parsing results difficult. The use case at hand is the integration with a code editor - in general you want to handle/hint on all parsing errors for a given policy set.

Expected behavior

All errors in a provided text representation of a ``StaticPolicySet` should be returned to allow processing of the errors, e.g. displaying errors in a code editor.

Reproduction steps

Call check_parse_policy_set on the text representation of a policy set containing more than one error.

Code Snippet

let content = r#"permit(
  principal == U ser::"alice", 
  action    == Act ion::"view", 
  reso urce  == Photo::"VacationPhoto94.jpg"
); // We have three syntax errors here. 
"#;
let maybe_policies = PolicySet.from_str(content);
let res = match maybe_policies {
        Ok(policies) => check_parse_policy_set(policies), // This should report 3 errors but only reports the first. 
        Err(err) => todo!()
};

let err_count = match result {
        CheckParseAnswer::Success => 0,
        CheckParseAnswer::Failure { errors } => errors.len(),
};

assert_eq!(err_count, 3);

Log output

// Put your output below this line

Additional configuration

No response

Operating System

Linux

Additional information and screenshots

No response

@AndreasKoestler AndreasKoestler 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. labels Oct 25, 2024
@john-h-kastner-aws
Copy link
Contributor

john-h-kastner-aws commented Oct 28, 2024

Hi, thanks for the question. Our parse error messages include a related field which contain some other associated error messages. If you look at that list of errors you should see that everything is reported. These FFI interfaces aren't the most well documented, but here's a link to it in the docs: https://docs.rs/cedar-policy/latest/cedar_policy/ffi/struct.DetailedError.html#structfield.related

Let me know if this doesn't work

@john-h-kastner-aws john-h-kastner-aws added clarification-needed This issue needs further clarification before we can decide how to proceed and removed pending-triage The cedar maintainers haven't looked at this yet. Automicaly added to all new issues. labels Oct 28, 2024
@AndreasKoestler
Copy link
Author

Indeed, related contains the remaining errors. For the above example, this yields:

Top level: "failed to parse policies from string: unexpected token `ser`"
related: Array(2)
0: 'unexpected token `ion`
1: 'unexpected token `to`'

However, this breaks down if I modify the example slightly:

permit(
  prin cipal == U ser::"alice", 
  action    == Act ion::"view", 
  resource  == Pho to::"VacationPhoto94.jpg"
)

Now I only get the top-level error and no related errors:

failed to parse policies from string: unexpected token `cipal`

Is this an issue with parsing and error reporting or am I not using the parsing interface correctly?

@john-h-kastner-aws
Copy link
Contributor

john-h-kastner-aws commented Oct 30, 2024

It looks like you're using it correctly. Now you're running into a known limitation in how our parser handles error recovery. When there's a parse error in that position, it skips ahead to the start of the next policy, ignoring any other parse error which may exist in the current policy. For the other errors it is able to recover and continue parsing within the same policy. This could be improved, but likely won't be a priority for us to get to.

If you were interested in hacking on our parsers and contributing an improvement yourself, the place to start would be the relevant production in our grammar and the lalrpop documentation for error recovery.

You might also run into the related issue #1054 where a single parsing error will prevent us from reporting any higher level errors.

@john-h-kastner-aws john-h-kastner-aws 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. clarification-needed This issue needs further clarification before we can decide how to proceed labels Oct 30, 2024
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

No branches or pull requests

2 participants