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 panic when using a repeated field as a predefined rule #149

Merged
merged 3 commits into from
Sep 30, 2024

Conversation

rodaine
Copy link
Member

@rodaine rodaine commented Sep 30, 2024

Using a repeated field as a predefined rule value results in a panic, as CEL cannot convert a protoreflect.List value into a valid CEL type. This results in an error value standing in for the list's value in the expression. When attempting to optimize the expression via computing residuals, a type assertion without the guard boolean (i.e., typed := unknown.(T) instead of typed, ok := unknown.(T)) triggers the panic on this non-list error value.

This patch adds a new helper to the celext package to compute an appropriate value for any protoreflect.Value given its field descriptor.

Fixes #148

Copy link

github-actions bot commented Sep 30, 2024

The latest Buf updates on your PR. Results from workflow Buf / validate-protos (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed❌ failed (83)Sep 30, 2024, 11:33 PM

@rodaine rodaine added the Emergency Bypass Review Github Bot will Approve this PR for you. Slack notifications will be issued. label Sep 30, 2024
@rodaine rodaine removed the Emergency Bypass Review Github Bot will Approve this PR for you. Slack notifications will be issued. label Sep 30, 2024
@rodaine
Copy link
Member Author

rodaine commented Sep 30, 2024

Breaking changes are expected due to the transitive breaking changes from the protovalidate project.

@rodaine rodaine merged commit b477f4c into main Sep 30, 2024
8 of 9 checks passed
@rodaine rodaine deleted the rodaine/invalid-predefined-rules branch September 30, 2024 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Seeing a panic when using predefined rules
2 participants