feat(__future__): Add MV capabilities to the experimental update-flag endpoints#7955
feat(__future__): Add MV capabilities to the experimental update-flag endpoints#7955emyller wants to merge 6 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
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:
📝 WalkthroughWalkthroughThis pull request renames the update-flag endpoints, serialisers, and services to Option A and Option B. It adds typed payload schemas and change-set dataclasses for feature values, multivariate options, and segment overrides. The versioning service is refactored to use shared helpers for feature-state and multivariate updates. Rollout code, routing, views, tests, OpenAPI schemas, and documentation are updated throughout. Estimated code review effort: 4 (Complex) | ~60 minutes Comment |
github-actions
Bot
added
api
emyller
force-pushed
the
feat/multivariate-update-flag-endpoints
branch
from
420e419 to
d64e257
Compare
github-actions
Bot
added
docs
emyller
force-pushed
the
feat/multivariate-update-flag-endpoints
branch
from
d64e257 to
23e4464
Compare
github-actions
Bot
added
feature
Docker builds report
|
github-actions
Bot
added
feature
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/features/feature_states/serializers.py (1)
238-254: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winN+1 query in
validate_segment_overrides.One
Segment.objects.filter(...).exists()query per segment override viavalidate_segment_id. Already flagged with a TODO — worth batching into a singlefilter(id__in=segment_ids, project_id=...).count()check once out of experimentation, as noted.♻️ Possible batched check
- for segment_id in segment_ids: - self.validate_segment_id(segment_id) + valid_count = Segment.objects.filter( + id__in=segment_ids, project_id=_environment(self.context).project_id + ).count() + if valid_count != len(segment_ids): + raise serializers.ValidationError( + "One or more segments not found in project" + )Happy to open this as a follow-up if useful.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 39f7d2ea-874a-4876-9a95-d80d6825179b
📒 Files selected for processing (18)
api/api/urls/experiments.pyapi/experimentation/services.pyapi/features/feature_states/serializers.pyapi/features/feature_states/types.pyapi/features/feature_states/views.pyapi/features/versioning/dataclasses.pyapi/features/versioning/versioning_service.pyapi/tests/integration/conftest.pyapi/tests/integration/environments/identities/test_integration_identities.pyapi/tests/integration/features/versioning/test_update_flag_endpoints.pyapi/tests/integration/helpers.pyapi/tests/unit/features/feature_states/test_serializers.pyapi/tests/unit/features/feature_states/test_unit_feature_states_views.pyapi/tests/unit/features/versioning/test_unit_versioning_versioning_service.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.mddocs/docs/integrating-with-flagsmith/flagsmith-api-overview/admin-api/updating-flags.mdmcp/src/flagsmith_mcp/openapi.jsonopenapi.yaml
| return attrs | ||
|
|
||
|
|
||
| class UpdateFlagOptionASerializer(BaseFeatureUpdateSerializer[FeatureState]): |
There was a problem hiding this comment.
Option A and Option B are becoming the new semantic for v1 and v2 ?
There was a problem hiding this comment.
To the public, they already are. The refactor here happens for:
- Eliminate ambiguity, i.e. flag-update v1/v2 over feature versioning v1/v2.
- Make it consistent with user docs.
- Improve clarity of naming of the backend components.
👻
Only certain spirits are expected to have different names depending on the realm they're in, but these are just code.There was a problem hiding this comment.
Ah ok, I didn't have context over the public documentation. Makes sense, thanks for the clarification. I thought on the opposite this changes were a specific case of renaming versioning v1 versus v2.
Just a NIT then if I may:
It looks like this example of the doc is missing the second request in Option A
If my understanding is correct, the override should be updated in a separate request. If my understanding is wrong please ignore 👍
Feel free to resolve this thread
There was a problem hiding this comment.
Thanks. Option A here is indeed one request, but the docs make it confusing. I've improved things with 42ea717 — also getting rid of the "highest=1" slop.
Please note these are unrelated to the scope of the PR, though I think the improvement warrants piggybacking.
github-actions
Bot
added
docs
emyller
force-pushed
the
feat/multivariate-update-flag-endpoints
branch
from
c4e9676 to
9d8dc5c
Compare
github-actions
Bot
added
docs
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7ed23c6d-926a-4bdb-9501-ab0991c865b6
📒 Files selected for processing (18)
api/api/urls/experiments.pyapi/experimentation/services.pyapi/features/feature_states/serializers.pyapi/features/feature_states/types.pyapi/features/feature_states/views.pyapi/features/versioning/dataclasses.pyapi/features/versioning/versioning_service.pyapi/tests/integration/conftest.pyapi/tests/integration/environments/identities/test_integration_identities.pyapi/tests/integration/features/versioning/test_update_flag_endpoints.pyapi/tests/integration/helpers.pyapi/tests/unit/features/feature_states/test_serializers.pyapi/tests/unit/features/feature_states/test_unit_feature_states_views.pyapi/tests/unit/features/versioning/test_unit_versioning_versioning_service.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.mddocs/docs/integrating-with-flagsmith/flagsmith-api-overview/admin-api/updating-flags.mdmcp/src/flagsmith_mcp/openapi.jsonopenapi.yaml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/docs/integrating-with-flagsmith/flagsmith-api-overview/admin-api/updating-flags.md (1)
235-238: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winClarify
multivariate_optionsomission semantics.
State that omitting the field leaves existing options unchanged, while providing the array replaces the current set and deletes any omitted options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f88b0d43-16e9-4175-9878-67217361caf2
📒 Files selected for processing (7)
api/features/feature_states/serializers.pyapi/features/feature_states/types.pyapi/features/models.pyapi/features/versioning/dataclasses.pyapi/features/versioning/versioning_service.pyapi/tests/integration/features/versioning/test_update_flag_endpoints.pydocs/docs/integrating-with-flagsmith/flagsmith-api-overview/admin-api/updating-flags.md
khvn26
left a comment
khvn26
left a comment
There was a problem hiding this comment.
Since the PR is still in draft, I've only reviewed the docs so far.
| These experimental endpoints let you update feature flag values and segment overrides via the Admin API. They're meant | ||
| as a simpler alternative to the current endpoints, which were designed with our dashboard user experience in mind: | ||
|
|
||
| - Convenient to use in automation via CLI | ||
| - Need no prior feature ID lookup — accepts name | ||
| - Work the same regardless of Feature Versioning |
There was a problem hiding this comment.
The bullets, especially the last one, read a little sloppy. If the end goal is to allow the users to not care about feature versioning, should we pull up that context here?
| These experimental endpoints let you update feature flag values and segment overrides via the Admin API. They're meant | |
| as a simpler alternative to the current endpoints, which were designed with our dashboard user experience in mind: | |
| - Convenient to use in automation via CLI | |
| - Need no prior feature ID lookup — accepts name | |
| - Work the same regardless of Feature Versioning | |
| These experimental endpoints let you update feature flag values and segment overrides via the Admin API. They're meant | |
| as a simpler alternative to the current endpoints, which were designed with our dashboard user experience in mind. |
There was a problem hiding this comment.
Agreed. I think these notes serve our team more than the public.
| - The `multivariate_options` list is absolute: omitting an option means deleting it. | ||
| - Segment overrides can only re-weight options configured in the environment. | ||
| - Deleting a multivariate option also deletes it in every segment override. | ||
| - Segments already overriding multivariate options do not gain new variants added to the environment automatically. |
There was a problem hiding this comment.
Perhaps reads slightly better to focus on flag evaluation behaviour:
| - Segments already overriding multivariate options do not gain new variants added to the environment automatically. | |
| - When new options added to environment, existing segment overrides do not change. |
|
|
||
| Set up multivariate flags and customise weights per segment. | ||
|
|
||
| - The `multivariate_options` list is absolute: omitting an option means deleting it. |
There was a problem hiding this comment.
Deleting it from where? Environment, or segment override?
| Set up multivariate flags and customise weights per segment. | ||
|
|
||
| - The `multivariate_options` list is absolute: omitting an option means deleting it. | ||
| - Segment overrides can only re-weight options configured in the environment. |
There was a problem hiding this comment.
Does this mean the user has to grab the IDs?
Why is re-weighting existing options important? I don't think we use the IDs for hashing anywhere?
There was a problem hiding this comment.
Does this mean the user has to grab the IDs?
Yes, unless we adopt variant keys (i.e. labels), which I attempted here but the diff increased quite a bit.
Why is re-weighting existing options important?
One major use case comes from experimentation. In fact, re-weighting segment MVs was the one thing already implemented before this PR (#7698).
I don't think we use the IDs for hashing anywhere?
Sorry, I don't understand this question in this context.
|
|
||
| - The `multivariate_options` list is absolute: omitting an option means deleting it. | ||
| - Segment overrides can only re-weight options configured in the environment. | ||
| - Deleting a multivariate option also deletes it in every segment override. |
|
|
||
| ## Configure A/B/n experiments (multivariate flags) | ||
|
|
||
| Set up multivariate flags and customise weights per segment. |
There was a problem hiding this comment.
I think the important behaviour to document here is that env-level changes can affect env defaults + segments, and segment-level changes can only affect segments. It's a little hard to infer it from the docs right now. Perhaps a paragraph or two of prose could work better than a bullet list.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/experimentation/services.py (1)
596-631: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
_update_live_feature_stateisn't self-contained atomic, unlikeupdate_flag_option_a.
_update_live_feature_statesavesenabled, thenfeature_state_value, then callsupdate_multivariate_values(which can raiseValidationErroron over-allocation) as three separate, unwrapped writes. It's reached via_update_rollout_in_place's in-place-mutation branch, which deliberately bypassesupdate_flag_option_a(and therefore itstransaction.atomic()wrap) to avoid re-randomising rollout variants.Today this is safe only because both call sites either wrap the call in an outer
transaction.atomic()(apply_experiment_rollout) or omitmultivariate_valuesentirely (enable_experiment_rollout). This mirrors exactly the "direct caller not atomic" pattern already flagged and fixed forupdate_flag_option_a's predecessor — wrapping_update_live_feature_stateitself would close the same class of risk defensively rather than relying on every future caller to remember to wrap it.Proposed fix
+from django.db import transaction + def _update_live_feature_state( feature_state: FeatureState, change_set: FlagChangeSetOptionA ) -> None: - if change_set.enabled is not None: - feature_state.enabled = change_set.enabled - feature_state.save() - if change_set.value is not None: - feature_state.feature_state_value.set_value( - change_set.value.value, change_set.value.type_ - ) - feature_state.feature_state_value.save() - update_multivariate_values(feature_state, change_set.multivariate_values) + with transaction.atomic(): + if change_set.enabled is not None: + feature_state.enabled = change_set.enabled + feature_state.save() + if change_set.value is not None: + feature_state.feature_state_value.set_value( + change_set.value.value, change_set.value.type_ + ) + feature_state.feature_state_value.save() + update_multivariate_values(feature_state, change_set.multivariate_values)api/tests/unit/features/versioning/test_unit_versioning_versioning_service.py (1)
857-857: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest name doesn't reflect the Option A rename.
The body now exercises
update_flag_option_a/FlagChangeSetOptionA, but the test retains its pre-rename name (test_update_flag__segment_override_with_mv__sets_mv_values) instead of the_option_asuffix used consistently by its Option B siblings (e.g.test_update_flag_option_b__...). Minor inconsistency with the PR's stated Option A/B taxonomy standardisation.✏️ Suggested rename
-def test_update_flag__segment_override_with_mv__sets_mv_values( +def test_update_flag_option_a__segment_override_with_mv__sets_mv_values(
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8b5b21a3-3cb1-4c22-a5b3-72b81368b095
📒 Files selected for processing (21)
api/api/urls/experiments.pyapi/experimentation/serializers.pyapi/experimentation/services.pyapi/features/feature_states/serializers.pyapi/features/feature_states/types.pyapi/features/feature_states/views.pyapi/features/models.pyapi/features/versioning/dataclasses.pyapi/features/versioning/versioning_service.pyapi/tests/integration/conftest.pyapi/tests/integration/environments/identities/test_integration_identities.pyapi/tests/integration/features/versioning/test_update_flag_endpoints.pyapi/tests/integration/helpers.pyapi/tests/unit/experimentation/test_services.pyapi/tests/unit/features/feature_states/test_serializers.pyapi/tests/unit/features/feature_states/test_unit_feature_states_views.pyapi/tests/unit/features/versioning/test_unit_versioning_versioning_service.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.mddocs/docs/integrating-with-flagsmith/flagsmith-api-overview/admin-api/updating-flags.mdmcp/src/flagsmith_mcp/openapi.jsonopenapi.yaml
3 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Timeline for context
Changes
Contributes to #7642
featurein the update-flag schema optional, to allow for isolated changes to MVs — and other properties too, why not.How did you test this code?
Unit tests.
How to review
Reading new docs should help: https://docs-lpoi8k37l-flagsmith.vercel.app/integrating-with-flagsmith/flagsmith-api-overview/admin-api/updating-flags
Reading commit-by-commit helps a ton.
3/5 effort.