planner: fix ref heads processing #5418
Conversation
srenatus
force-pushed
the
sr/planner/fix-planning-ref-heads
branch
from
7214708
to
b5a4a5c
Compare
srenatus
force-pushed
the
sr/planner/fix-planning-ref-heads
branch
from
b5a4a5c
to
c1ab3b0
Compare
With the introduction of ref heads in open-policy-agent#4660, the planned IR still mostly worked, but it was bypassing the CallDynamic optimization when it shouldn't have. This commit re-works some of the rule planning to more robustly handle ref heads. Also adds a few test cases to get a grip on what should and should not happen. Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
srenatus
force-pushed
the
sr/planner/fix-planning-ref-heads
branch
from
c1ab3b0
to
4834c9a
Compare
| default: // cut off | ||
| r = r[:len(r)-1] | ||
| } | ||
| val := p.rules.LookupOrInsert(r) |
There was a problem hiding this comment.
ℹ️ We now take care of the last ref segment when builtin ruletrie, not when retrieving something from it.
This is what the changes comes down to:
- putting the rules into the rule trie without any non-string pieces:
- so
a.b.c = 1is a different node thana.b[1] = 2
- so
- taking care of building objects or not when planning any given rule set, not by passing a boolean to
planRules() - the
optimizeLookupcode flow had to be adjusted to not make the change in (1.) avoid optimizations that are desired.
| default: | ||
| // Needs to be fixed if we allow non-string ref pieces, like `p.q[3][4].r = x` | ||
| pathPieces = append(pathPieces, q.String()) | ||
| panic("impossible") |
There was a problem hiding this comment.
If our functions had "path pieces" that are not strings, they'll fail in the call_dynamic mapping lookup. For #4660, we've started out with string prefixes (not generic refs), so this should be OK.
There was a problem hiding this comment.
I appreciate the battery of testcases for this PR. It looks like there was some nice refactoring done, especially in internal/planner/planner.go
|
Thanks for the review!
Mostly just reverting a shoddy first try 😅 Hope this one is more robust 🤞 |
With the introduction of ref heads in #4660, the planned IR still mostly worked, but it was bypassing the CallDynamic optimization when it shouldn't have.
This commit re-works some of the rule planning to more robustly handle ref heads.
Also adds a few test cases to get a grip on what should and should not happen.