ensure query can be evaluated during preparation #20
Conversation
dev-rice
force-pushed
the
check_query_during_preparation
branch
from
adaeb74 to
f6cea42
Compare
patrick-east
left a comment
patrick-east
left a comment
There was a problem hiding this comment.
I think it's a bit of a deviation from how query preparation works with the Go implementation. And I could see this check becoming much more difficult or impossible to implement as query capabilities expand
Yeah, things get a little blurry with the different evaluator types. The aim was to keep the top level APIs a little more agnostic of how a query got evaluated (IR interpreter, topdown evaluator, etc). There was a point in time on this where it would just give back "undefined" if a query that wasn't in the plan was specified, which is maybe more in line with the Go rego APIs.
That being said, if we are going to give back an error saying it isn't a query that can be supported, returning that in the prepare step makes a lot of sense to me.
Sources/Rego/IREvaluator.swift
Outdated
| } | ||
|
|
||
| // The above guard has already ensured that the policy has a plan for the given entrypoint, so force unwrap is safe. | ||
| return (policy, policy.plans[entrypoint]!) |
There was a problem hiding this comment.
⛏️ reworking this as a for loop to find and return in-line can eliminate the need for a force unwrap and reasoning about whether it is safe in favor of compile-time checks.
| self.policies = policies.map { IndexedIRPolicy(policy: $0) } | ||
| } | ||
|
|
||
| func findPlanAndPolicy(entrypoint: String) -> (IndexedIRPolicy, Plan)? { |
There was a problem hiding this comment.
Does this change warrant a dedicated test?
There was a problem hiding this comment.
It's still covered by IREvaluatorTests.testInvalidEvaluations because those test cases both prepare a query, then try to evaluate it. Before, the evaluation would fail, but now the query preparation step fails. I considered splitting out a distinct set of tests (for the Engine) around only query preparation to put tighter bounds around where the errors are thrown, but ultimately decided against it since it'd be duplicating most of what's covered in IREvaluatorTests.
What do you think?
Continuing my trend of shoving more validation into `OPA.Engine.prepareForEvaluation`, this method now checks that the provided query can be evaluated. This should help catch misconfigurations earlier in application bootstrapping, without necessarily having to evaluate something first. I think it's a bit of a deviation from how query preparation works with the Go implementation. And I could see this check becoming much more difficult or impossible to implement as query capabilities expand, so I won't be upset if this is rejected. There are already some tests checking for this behavior, but they were expecting it to happen on `evaluate`. Now the error pops up during `prepareForEvaluation`, but the tests pass either way. Signed-off-by: Chris Rice <[email protected]>
dev-rice
force-pushed
the
check_query_during_preparation
branch
from
f6cea42 to
2cbbcea
Compare
3 participants
Continuing my trend of shoving more validation into
OPA.Engine.prepareForEvaluation, this method now checks that the provided query can be evaluated. This should help catch misconfigurations earlier in application bootstrapping, without necessarily having to evaluate something first.I think it's a bit of a deviation from how query preparation works with the Go implementation. And I could see this check becoming much more difficult or impossible to implement as query capabilities expand, so I won't be upset if this is rejected.
There are already some tests checking for this behavior, but they were expecting it to happen on
evaluate. Now the error pops up duringprepareForEvaluation, but the tests pass either way.