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 the caveat expr limit to be processed by our code #1638

Merged
merged 1 commit into from
Nov 5, 2023

Conversation

josephschorr
Copy link
Member

Right now, as a hack to ensure caveat error messages have the correct line and col information, we replace the preceding schema with newlines. However, the CEL parser has a default limit on the size of the expression it can be given, which was causing the error. With this change, we turn off the CEL check and instead check the trimmed expression ourselves for a limit.

Long term, we'll want to issue a change in CEL to allow updating of the positioning information instead of this hack

Adds tests and fixes #1637

@josephschorr josephschorr requested review from vroldanbet and a team November 3, 2023 18:34
@github-actions github-actions bot added area/schema Affects the Schema Language area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Nov 3, 2023
@josephschorr josephschorr force-pushed the fix-caveat-expr-limit branch from cc0b6e2 to 8352d00 Compare November 3, 2023 18:36
@josephschorr
Copy link
Member Author

@TristonianJones if you have any insights on how we could do this in a "supported" fashion, I'd be quite happy for the guidance

Right now, as a hack to ensure caveat error messages have the correct line and col information, we replace the preceding schema with newlines. However, the CEL parser has a default limit on the size of the expression it can be given, which was causing the error. With this change, we turn off the CEL check and instead check the *trimmed* expression ourselves for a limit.

Long term, we'll want to issue a change in CEL to allow updating of the positioning information instead of this hack

Adds tests and fixes authzed#1637
@josephschorr josephschorr force-pushed the fix-caveat-expr-limit branch from 8352d00 to 49bfa31 Compare November 3, 2023 18:44
@josephschorr josephschorr added this pull request to the merge queue Nov 5, 2023
@TristonianJones
Copy link

@TristonianJones if you have any insights on how we could do this in a "supported" fashion, I'd be quite happy for the guidance

The common.Source or maybe ast.SourceInfo can support a base line and column which can be used to ensure error messages include source offsets. Are you really parsing strings greater than 100KiB in length?

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 5, 2023
@josephschorr
Copy link
Member Author

josephschorr commented Nov 5, 2023

The common.Source or maybe ast.SourceInfo can support a base line and column which can be used to ensure error messages include source offsets

Can you point me to where I can set these? Last time I tried to do so, the error messages weren't be adjusted.

Are you really parsing strings greater than 100KiB in length?

To ensure that the positioning is correct, we generate whitespace to mimic the position of the expression in the overall file, so yes, this can happen. The vast majority of it is just newlines.

@josephschorr josephschorr added this pull request to the merge queue Nov 5, 2023
Merged via the queue into authzed:main with commit 2bdabe3 Nov 5, 2023
20 checks passed
@josephschorr josephschorr deleted the fix-caveat-expr-limit branch November 5, 2023 17:48
@github-actions github-actions bot locked and limited conversation to collaborators Nov 5, 2023
@josephschorr
Copy link
Member Author

@TristonianJones if you have any pointers for the above, I'd be quite grateful :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/schema Affects the Schema Language area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Caveat must appear within the first 100,000 bytes of the schema
3 participants