Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughEnvironment evaluation now receives cached segments and excludes segments with identity-dependent rules. The SDK endpoint evaluates feature states before selecting the requested feature by name. Client requests retain the server-key-only filter, and cache-populating queries always read from the replica. Tests cover feature filtering, segment overrides and client-key responses. Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to Flag-dependent segments can return incorrect values, and some flags requests retain response or performance regressions. Resolve these issues before merging unless their impact is 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 |
github-actions
Bot
added
api
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## stack/3-edge-identity #8576 +/- ##
======================================================
Coverage 98.81% 98.82%
======================================================
Files 1645 1645
Lines 67478 67561 +83
======================================================
+ Hits 66681 66764 +83
Misses 797 797 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
26e1f7b to
e8aa0ac
Compare
github-actions
Bot
added
feature
6abaac4 to
edbad51
Compare
github-actions
Bot
added
feature
d1d9376 to
4ce147b
Compare
github-actions
Bot
added
feature
5d5c9b8 to
82b24a8
Compare
d41d2a2 to
3d82111
Compare
github-actions
Bot
added
feature
3d82111 to
cfe9ee8
Compare
github-actions
Bot
added
feature
khvn26
changed the title
cfe9ee8 to
37506d3
Compare
Docker builds report
|
✅ private-cloud · depot-ubuntu-latest-16 — run #20841 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20841 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20841 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20841 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20839 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #20839 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20839 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20839 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #20838 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20838 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20838 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20838 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 3152f251-00cb-40ec-896f-970485f3f521
📒 Files selected for processing (3)
api/evaluation/services.pyapi/features/views.pyapi/tests/unit/features/test_unit_features_views.py
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🔵 Trivial · Add a feature-filter regression test for dependent flags. · test_unit_features_views.py:720-765
api/tests/unit/features/test_unit_features_views.py:720-765
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a feature-filter regression test for dependent flags.
SDKFeatureStates.getevaluates all flags before selectingfeature, because segment rules can read another flag through$.flags. The matching feature-filter test has no segment dependency, and the segment test does not use?feature=. The current tests can therefore pass if the endpoint prefilters flags again, causing the requested flag to miss its segment override.Add a test with an enabled dependency flag, a segment rule on
$.flags.<dependency>.enabled, and a filtered request for the overridden flag.Suggested fix
def test_sdk_feature_states_get__existing_feature_filter__returns_feature( api_client: APIClient, environment: Environment, feature: Feature, feature_state: FeatureState, @@ assert response.json() == { "id": feature_state.id, "enabled": feature.default_enabled, "environment": environment.id, "feature": mocker.ANY, "feature_segment": None, "feature_state_value": None, "identity": None, } +def test_sdk_feature_states_get__feature_filter__evaluates_dependent_flags( + api_client: APIClient, + environment: Environment, + project: Project, +) -> None: + # Given + dependency = Feature.objects.create( + name="dependency_feature", + project=project, + default_enabled=True, + ) + requested = Feature.objects.create( + name="requested_feature", + project=project, + initial_value="environment", + ) + segment = Segment.objects.create(name="dependent_segment", project=project) + Condition.objects.create( + rule=SegmentRule.objects.create(segment=segment, type=SegmentRule.ALL_RULE), + property=f"$.flags.{dependency.name}.enabled", + operator=EQUAL, + value=True, + ) + feature_segment = FeatureSegment.objects.create( + segment=segment, + feature=requested, + environment=environment, + ) + segment_override = FeatureState.objects.create( + feature=requested, + feature_segment=feature_segment, + environment=environment, + ) + segment_override.feature_state_value.string_value = "segment" + segment_override.feature_state_value.save() + api_client.credentials(HTTP_X_ENVIRONMENT_KEY=environment.api_key) + + # When + response = api_client.get(f"/api/v1/flags/?feature={requested.name}") + + # Then + assert response.status_code == status.HTTP_200_OK + assert response.json()["feature_state_value"] == "segment" + + def test_sdk_feature_states_get__missing_feature_filter__returns_404( api_client: APIClient, environment: Environment, ) -> None:
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: ba65dc8e-5ae0-4a9b-a715-850fcead740f
📒 Files selected for processing (3)
api/evaluation/services.pyapi/features/views.pyapi/tests/unit/features/test_unit_features_views.py
Included review availability: Your plan provides up to 8 included reviews per hour; 2 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 9c4fea17-648c-446e-b419-f03e772f52b6
📒 Files selected for processing (2)
api/evaluation/services.pyapi/tests/unit/features/test_unit_features_views.py
Included review availability: Your plan provides up to 8 included reviews per hour; 1 remains after this review.
| from util.engine_models.features.models import FeatureStateModel | ||
|
|
||
|
|
||
| _IDENTITY_FREE_PROPERTY_PREFIXES = ("$.environment.", "$.flags.") |
There was a problem hiding this comment.
I think we'll need to check for /^\$\.flags(?:\[|\.)/ — i.e. accept a feature name quoted within square brackets — because feature names are very flexible in their allowed characters.
For that reason, #8570 adds prerequisites using $.flags[{json_quoted_feature_name}].
There was a problem hiding this comment.
Good catch! Coordinated with edge-api#731 in 58d72ea.
| @pytest.fixture() | ||
| def environment_name_segment(environment: Environment, project: Project) -> Segment: | ||
| segment: Segment = Segment.objects.create(name="This environment", project=project) | ||
| Condition.objects.create( | ||
| rule=SegmentRule.objects.create(segment=segment, type=SegmentRule.ALL_RULE), | ||
| property="$.environment.name", | ||
| operator=EQUAL, | ||
| value=environment.name, | ||
| ) | ||
| return segment |
There was a problem hiding this comment.
nit: Can you please move fixture to the top of the test module, or to a conftest module? Adding fixtures mid-tests has been a consistent — and IMO myopic — behaviour of LLMs lately.
There was a problem hiding this comment.
note: Some of the unit tests were conceived to cover how — e.g. segments now being counted in evaluation — but fail to convey why to the reader. Consider using flag dependency examples in such tests, and how the hide_disabled_flags affects it under both client and server-type API keys.
Resolves `$.flags` conditions, i.e. flags that depend on other flags.
Filtering them out beforehand hid them from conditions reading `$.flags` too, as Edge API avoids.
…ions in environment flags Feature names can contain any character, so prerequisites are written as `$.flags["name"]`.
This branch was successfully deployed
3 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Contributes to #6654
Closes #8416
In this PR, we evaluate environment flags with the environment's segments, so that
$.environmentand$.flagsconditions resolve without an identity. Segments reading traits or identity context are left out. Edge API is set to do the same in Flagsmith/edge-api#700.This changes
GET /flags/without an identifier:$.environmentor$.flagscontext values, e.g.$.environment.name.hide_disabled_flags, disabled flags are hidden after evaluating, so a disabled override hides its flag instead of letting the enabled environment default through.hide_disabled_flags, server-key-only flags are no longer served to client keys.There are some query count / performance implications:
GET /flags/now reads the environment's segments, which is one more query per request unlessCACHE_ENVIRONMENT_SEGMENTS_SECONDSis set.GET /flags/?feature=…now evaluates all flags and then filters, because dependent flags.Flag engine is bumped to 11.1.0, enabling dependent flag evaluation.
How did you test this code?
Added tests.