feat: Evaluate flags that depend on other flags - #343
Conversation
4e7cf34 to
b0b6007
Compare
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 419d686 |
de6c9d1 to
ba4a8e0
Compare
ba4a8e0 to
63716d9
Compare
|
@themis-blindfold review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Warning Review limit reachedNext included review available in 38 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe evaluation context now supports optional flag results. Segment evaluation injects a lazy Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to Dependent-flag evaluation can incorrectly select targeting overrides when flags form a circular dependency, and the test-data reference is not pinned, so builds may change without a repository change. The PR should not merge until cycle handling is made consistently non-matching and the test data is pinned or these risks are explicitly accepted. 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}
| path = tests/engine_tests/engine-test-data | ||
| url = https://github.com/flagsmith/engine-test-data.git | ||
| branch = v3.9.0 | ||
| branch = test/flag-dependencies |
There was a problem hiding this comment.
🧹 Nit · ⚡ Quick win
Pin the dependency corpus to its release tag. Observed: this switches from v3.9.0 to the mutable test/flag-dependencies branch instead of the release tag required for the dependency cases.
There was a problem hiding this comment.
Certainly, as soon as it's released.
There was a problem hiding this comment.
Keep this open until the release is available; the mutable branch makes the dependency corpus non-reproducible.
🧠 Learning recorded — lands in .themis/learnings.jsonl via the next digest PR.
⚖️ 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.
| identity: NotRequired[Optional[IdentityContext]] | ||
| segments: NotRequired[Dict[str, SegmentContext[SegmentMetadataT, FeatureMetadataT]]] | ||
| features: NotRequired[Dict[str, FeatureContext[FeatureMetadataT]]] | ||
| flags: NotRequired[Dict[str, FlagResult[FeatureMetadataT]]] |
There was a problem hiding this comment.
I wonder if this should belong to a new InEngineEvaluationContext(EvaluationContext) type.
| priority = override_feature_context.get( | ||
| "priority", | ||
| constants.DEFAULT_PRIORITY, | ||
| ) | ||
| if segment_override is None or priority < override_priority: | ||
| segment_override = SegmentOverride( | ||
| feature_context=override_feature_context, | ||
| segment_name=segment_context["name"], | ||
| ) | ||
| override_priority = priority |
There was a problem hiding this comment.
note: Feels like we're duplicating code.
| segments, segment_overrides = evaluate_segments(context) | ||
| flags = evaluate_features(context, segment_overrides) | ||
|
|
||
| if (resolver := resolved.__dict__.get("_resolver")) is not None: |
There was a problem hiding this comment.
if (resolver := resolved.__dict__.get("_resolver")) is not None:This hints at a few design issues IMO:
- Having to resort to
__dict__to peek at the cached property feels like obscure heuristics to detect whether a segment consulted a flag; - Accessing a private member from the outside.
Perhaps we could already lose some complexity if _DependencyResolver stops doing building _segment_keys_by_feature_name on initialisation, which would allow for dropping the if statement above for free.
3 participants
Contributes to Flagsmith/flagsmith#8394
Implements Flagsmith/engine-test-data#59
Closes #345