Skip to content

Use native rules for repeated item rules #339

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.

Sign up for GitHub

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

Merged
sudorandom merged 4 commits into from
Sep 4, 2026
Merged

Use native rules for repeated item rules #339

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c46151a
Use native rules for repeated item rules
sudorandom Sep 4, 2026
8859243
Retrigger CI
sudorandom Sep 4, 2026
a871309
Restructure processStandardRules to be simpler
sudorandom Sep 4, 2026
f828445
Fix case where we would look for scalar rules for map descriptors un-…
sudorandom Sep 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,7 @@ func (bldr *builder) processStandardRules(
}
}
// Try native Go evaluators for known simple rules before falling back to CEL.
if !fdesc.IsMap() && !fdesc.IsList() {
if !fdesc.IsMap() && (!fdesc.IsList() || valEval.NestedRule != nil) {

@jonbodner-buf jonbodner-buf Sep 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's already a case on lines 481-485 to handle repeated rules when there are no nested rules. Should the fix be to eliminate the check for nested rules in the earlier code? I haven't looked at this code in a while, I might be missing a detail.

@sudorandom sudorandom Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite. Those lines are to figure out when to evaluate rules related to list-level rules. However, you do have a point that this code is confusingly organized here. I switched this to use a much more understandable switch statement.

To be clear, the logic error is that this section REQUIRES NestedRules to be set, aka != nil, so it's different than the case above.

@jonbodner-buf jonbodner-buf Sep 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The switch isn't quite the same logic as before. The default case used to only process scalar values. Now it will process fdesc.IsList() and feds.IsMap() when valEval.NestedRule != nil . If that works correctly, great.

if native := bldr.tryNativeRules(fdesc, rules, valEval); native != nil {
valEval.Append(native)
}
Expand Down
Loading