feat: Evaluate flags that depend on other flags - #343
Conversation
4e7cf34 to
b0b6007
Compare
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against aac80cb |
de6c9d1 to
ba4a8e0
Compare
ba4a8e0 to
63716d9
Compare
|
@themis-blindfold review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe evaluation context now accepts optional flag results. Segment evaluation now injects a lazy Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to Dependent flags can now influence segment matching and returned values, while circular dependencies are contained. A sufficiently deep dependency chain could still consume excessive evaluator resources, so owners should consider adding an explicit depth or work limit; the PR remains mergeable with that bounded risk understood. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![themis-blindfold[bot]](https://avatars.githubusercontent.com/in/4278417?s=60&v=4){kind=small}
⚖️ Themis review: 🟠 Fix before mergeTL;DR: Dependent flags work for the straightforward cases covered by the shared corpus, and all completed CI checks passed. Two paths need correction before merging: ordinary evaluations now pay resolver setup costs, and cycles can return a default flag while reporting its dependent segment as matched.
🟠 Majors
🧹 Nits
📝 Walkthrough
🧪 How to verify
Product take: This is a major capability for composing rollouts, but evaluation is a hot path and cycle handling must stay deterministic. Once those paths are fixed, the feature is a solid improvement. 🧭 Assumptions & unverified claimsThe cycle reproduction should be rerun with the pinned Dependent flags are useful; dependent defaults are less so. · reviewed at 63716d9 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2fe46096-59c6-498b-8776-03a0eaba34c8
📒 Files selected for processing (4)
.gitmodulesflag_engine/context/types.pyflag_engine/segments/evaluator.pytests/engine_tests/engine-test-data
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
Adds support for segments conditioned on another flag's result, via a `$.flags.<feature name>` condition property, as the evaluation half of dependent flags. Segment conditions are already JSONPath, so a dependency needs no new operator or condition type. What it does need is for the flag to be resolved by the time the condition reads it, and evaluation resolved every segment before any flag. Rather than establish up front which segments depend on which flags, `$.flags` is a mapping that resolves a flag when a condition first reads it. Resolving a flag evaluates the segments overriding it, which in turn resolves whatever flags their conditions read, memoised throughout. The existing single pass is otherwise untouched: a context whose segments read no flag never enters the resolver, and pays only for one empty mapping. Resolving on read rather than in advance means there is no need to recognise a dependency in a condition property, and so no need to reimplement enough of the JSONPath grammar to tell that `$.flags.a['enabled']` and `$.flags.a.enabled` are the same query. Every spelling works because resolution is triggered by the read itself. It also costs only what is read: rule short-circuiting means a condition that is never evaluated resolves nothing. Measured against main across the benchmark contexts, interleaved to cancel drift: +3.7%, from the mapping and a shallow copy of the context to hold it. Establishing up front that an environment has no dependencies would remove that residual entirely, but needs a field on the context to carry it. A flag whose dependencies form a cycle is not resolvable. It serves its environment default and reports `ERROR; code=CIRCULAR_DEPENDENCY`, so that a flag which could not be resolved is distinguishable from one that was never gated, and only flags in the cycle are reported that way. Crucially the value it falls back to is never published to `$.flags`, so no other condition can match on a value that exists only because the cycle was cut — otherwise a segment gated on a flag that defaults to enabled would be reported as matched while the flag it overrides stayed at its default. Cycles are still expected to be rejected where dependencies are written. Behaviour is covered by test cases in Flagsmith/engine-test-data#59 rather than by unit tests here, so that every engine is held to it.
REVERT BEFORE MERGE. Dependent flags behaviour is covered by test cases in Flagsmith/engine-test-data#59 rather than by unit tests here, so that every engine is held to it. Those cases aren't in a release yet, so without this the new code is exercised by nothing and CI's 100% coverage gate fails. Once #59 is merged and tagged, this goes back to a semver tag, which Renovate now tracks as of #337.
63716d9 to
99dea3e
Compare
emyller
left a comment
emyller
left a comment
There was a problem hiding this comment.
Left a few comments towards improving the architecture. I believe we could achieve a simpler and clearer design overall if this was re-approached, but let me know if you'd rather move forward for now — tests pass.
Follows Flagsmith/engine-test-data#59, where two cases were dropped as redundant and one gained a clarifying note. Coverage is unaffected.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Team
Run ID: 8d03f90a-0de7-469e-851d-96c427239b41
📒 Files selected for processing (2)
flag_engine/segments/evaluator.pytests/engine_tests/engine-test-data
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
`IS_NOT_SET` matches when a property has no value, and a flag left unresolved by a cycle has none. A segment overriding the very flag it tests for absence therefore had its condition made true by the cycle, and its override applied — reported with the circular dependency reason but carrying the override's value rather than the environment default the reason claims. A segment whose own evaluation cut a cycle read a flag that could not be resolved, so neither its match nor its failure to match rests on anything, and it must not be applied either way. The cycle counter moves onto the flags mapping so that both the resolver and the single pass over segments can see it, and the mapping is now passed to `evaluate_segments` explicitly rather than read back out of the context. Covered by `cyclic_is_not_set__segment_should_not_match` in Flagsmith/engine-test-data#59, which fails without this.
Detecting whether a segment consulted a flag was done by reaching into `__dict__` to see whether a cached property had been built, which is both obscure and an access to a private member from outside. An empty mapping cannot answer the question on its own, because a flag in a cycle resolves to nothing and is deliberately not published. The mapping now records it directly. `used` says a condition read a flag, and `cyclic` — which the resolver already maintained — moves alongside it, so nothing outside the resolver reaches into it. Both are created on the first read rather than up front, so a context whose segments read no flag allocates neither.
Which of two competing segment overrides wins was implemented twice, once while collecting overrides across segments and once while resolving a single feature's dependencies. Two copies of a precedence rule is the kind of thing that drifts, and the second copy tracked the winning priority in a separate variable to do it. Both now call `_wins_over`, which states the rule once: lower priority wins, and the first seen wins a tie.
Discharges the REVERT BEFORE MERGE on 99dea3e. Flagsmith/engine-test-data#59 is merged and released, so the submodule goes back to a semver tag — the form Renovate tracks as of #337 — rather than a branch.
3 participants
Contributes to Flagsmith/flagsmith#8394
Implements Flagsmith/engine-test-data#59
Closes #345