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 7 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
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
|
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.
| 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.
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
khvn26
left a comment
khvn26
left a comment
There was a problem hiding this comment.
I think we need to align on our approach to internal versioning APIs before we can proceed further.
| def update_flag_v1(request: Request, environment_key: str) -> Response: | ||
| def update_flag_option_a(request: Request, environment_key: str) -> Response: | ||
| environment = Environment.objects.get(api_key=environment_key) | ||
| _check_workflow_not_enabled(environment) |
There was a problem hiding this comment.
Note: I wonder if, for the future, we could think of creating a change request here, and returning its id (perhaps under 202 Accepted).
| 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() |
There was a problem hiding this comment.
Can we verify that resulting audit logs make sense?
| if environment_default.enabled is not None: | ||
| env_default_state.enabled = environment_default.enabled | ||
| env_default_state.save() | ||
|
|
||
| if environment_default.value is not None: | ||
| _update_feature_state_value( | ||
| env_default_state.feature_state_value, environment_default.value | ||
| ) |
| enabled: bool | None, | ||
| value: FeatureValue | None, | ||
| get_environment_default: typing.Callable[[], FeatureState], | ||
| ) -> tuple[bool, FeatureStateValue | None]: |
There was a problem hiding this comment.
Would using the Django model as a DTO work here? Just want to steer away from anonymous tuples.
| ) -> tuple[bool, FeatureStateValue | None]: | |
| ) -> FeatureState | None: |
| enabled, inherited_value = _resolve_new_override_inheritance( | ||
| change_set.enabled, | ||
| change_set.value, | ||
| lambda: new_version.feature_states.get( | ||
| feature_segment__isnull=True, identity_id=None | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Looking at this, I feel like we deserve a first-class versioning API among the lines of
def add_to_version(efv: EnvironmentFeatureVersion, feature_state: FeatureState) -> None: ......where FeatureState is a memory-only ORM object, or an equivalent DTO.
This API could optionally accept segment and identity arguments.
There was a problem hiding this comment.
If you feel it would take too much additional time, feel free to not do it, as long as the other comment is addressed.
There was a problem hiding this comment.
This entire module is a headache to read through in its current state. I'm blocking this PR until we sort where we're heading with this.
There was a problem hiding this comment.
As discussed offline, we should toss out option A in favour of option B.
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.