feat(Feature Versioning): add multi variate capabilities to the experimental update-flag endpoints#7955
feat(Feature Versioning): add multi variate capabilities to the experimental update-flag endpoints#7955emyller wants to merge 10 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
📝 WalkthroughWalkthroughThis pull request renames the "update-flag-v1"/"v2" endpoints, serializers, and services to "Option A"/"Option B" naming throughout the API layer. It introduces typed TypedDict payload schemas and new dataclasses (FeatureValue, FlagChangeSetOptionA/B, MultivariateOptionChangeSet) replacing prior untyped/string-based structures. The versioning service is refactored around shared helpers for applying enabled/value/multivariate changes, including new environment-level multivariate option reconciliation logic. Experimentation rollout code, URL routing, views, tests, OpenAPI specs, and documentation are updated to match the new naming and typed structures. 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
github-actions
Bot
added
docs
github-actions
Bot
added
feature
emyller
force-pushed
the
feat/multivariate-update-flag-endpoints
branch
from
7f75746 to
8d5469c
Compare
github-actions
Bot
added
docs
github-actions
Bot
added
docs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7955 +/- ##
========================================
Coverage 98.63% 98.63%
========================================
Files 1496 1498 +2
Lines 59072 59251 +179
========================================
+ Hits 58266 58445 +179
Misses 806 806 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
|
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
| def validate(self, attrs: UpdateFlagOptionAPayload) -> UpdateFlagOptionAPayload: | ||
| options = attrs.get("multivariate_options") | ||
| if options is None: | ||
| return attrs | ||
| if attrs.get("segment"): | ||
| _validate_segment_options(attrs["feature"], options) | ||
| else: | ||
| _validate_environment_options(attrs["feature"], options) | ||
| return attrs | ||
|
|
||
| @property | ||
| def flag_change_set(self) -> FlagChangeSet: | ||
| validated_data = self.validated_data | ||
| value_data = validated_data["value"] | ||
| def flag_change_set(self) -> FlagChangeSetOptionA: | ||
| validated_data: UpdateFlagOptionAPayload = self.validated_data | ||
| value = _feature_value(validated_data.get("value")) | ||
| segment_data = validated_data.get("segment") | ||
|
|
||
| return FlagChangeSet( | ||
| segment_id = segment_data.get("id") if segment_data else None | ||
| options = validated_data.get("multivariate_options") | ||
|
|
||
| multivariate_values = None | ||
| multivariate_options = None | ||
| if options is not None: | ||
| if segment_id is None: | ||
| multivariate_options = _option_change_sets(options) | ||
| else: | ||
| multivariate_values = _reweight_change_sets(options) | ||
|
|
||
| return FlagChangeSetOptionA( | ||
| author=AuthorData.from_request(self.context["request"]), | ||
| enabled=validated_data["enabled"], | ||
| feature_state_value=value_data["value"], | ||
| type_=value_data["type"], | ||
| segment_id=segment_data.get("id") if segment_data else None, | ||
| enabled=validated_data.get("enabled"), | ||
| value=value, | ||
| segment_id=segment_id, | ||
| segment_priority=segment_data.get("priority") if segment_data else None, | ||
| multivariate_values=multivariate_values, | ||
| multivariate_options=multivariate_options, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Segment-level multivariate reweight silently drops a submitted value.
UpdateFlagOptionASerializer reuses MultivariateOptionSerializer/MultivariateOptionPayload (which includes an optional value) for the segment-scoped reweight path (flag_change_set, lines 163-169). _validate_segment_options (lines 376-384) only checks that id is present and owned by the feature — it never rejects an accompanying value. _reweight_change_sets (lines 433-443) then only reads id/percentage_allocation, so any value a client sends for a segment-level entry is accepted by validation but silently discarded.
Option B avoids this entirely by using a dedicated SegmentOverrideMultivariateOptionPayload/SegmentOverrideMultivariateOptionSerializer that has no value field. Consider applying the same approach for Option A's segment path, or explicitly rejecting value in _validate_segment_options when present.
🐛 Proposed fix
def _validate_segment_options(feature: Feature, options: _OptionPayloads) -> None:
"""Segment overrides can only re-weight options already on the feature."""
if any("id" not in option for option in options):
raise serializers.ValidationError(
{"multivariate_options": "Segment overrides require an option 'id'."}
)
+ if any(option.get("value") is not None for option in options):
+ raise serializers.ValidationError(
+ {
+ "multivariate_options": (
+ "Segment overrides cannot set an option's 'value'; "
+ "update the value at the environment level instead."
+ )
+ }
+ )
if error := _option_ownership_error(feature, _option_ids(options)):
raise serializers.ValidationError({"multivariate_options": error})Also applies to: 376-384, 433-443
| def apply_feature_state_changes( | ||
| feature_state: FeatureState, | ||
| *, | ||
| enabled: bool | None, | ||
| value: FeatureValue | None, | ||
| multivariate_values: list[MultivariateValueChangeSet] | None, | ||
| ) -> None: | ||
| """Apply only the provided parts of a change; omissions are left intact.""" | ||
| if enabled is not None: | ||
| feature_state.enabled = enabled | ||
| feature_state.save() | ||
| if value is not None: | ||
| _update_feature_state_value(feature_state.feature_state_value, value) | ||
| update_multivariate_values(feature_state, multivariate_values) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make apply_feature_state_changes atomic for direct callers.
update_multivariate_values() can raise after enabled / value have already been saved. update_flag* callers are wrapped, but api/experimentation/services.py:_update_rollout_in_place calls this helper directly, so invalid MV allocations can leave a partially updated state.
Proposed fix
def apply_feature_state_changes(
feature_state: FeatureState,
*,
enabled: bool | None,
value: FeatureValue | None,
multivariate_values: list[MultivariateValueChangeSet] | None,
) -> None:
"""Apply only the provided parts of a change; omissions are left intact."""
- if enabled is not None:
- feature_state.enabled = enabled
- feature_state.save()
- if value is not None:
- _update_feature_state_value(feature_state.feature_state_value, value)
- update_multivariate_values(feature_state, multivariate_values)
+ with transaction.atomic():
+ if enabled is not None:
+ feature_state.enabled = enabled
+ feature_state.save()
+ if value is not None:
+ _update_feature_state_value(feature_state.feature_state_value, value)
+ update_multivariate_values(feature_state, multivariate_values)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def apply_feature_state_changes( | |
| feature_state: FeatureState, | |
| *, | |
| enabled: bool | None, | |
| value: FeatureValue | None, | |
| multivariate_values: list[MultivariateValueChangeSet] | None, | |
| ) -> None: | |
| """Apply only the provided parts of a change; omissions are left intact.""" | |
| if enabled is not None: | |
| feature_state.enabled = enabled | |
| feature_state.save() | |
| if value is not None: | |
| _update_feature_state_value(feature_state.feature_state_value, value) | |
| update_multivariate_values(feature_state, multivariate_values) | |
| def apply_feature_state_changes( | |
| feature_state: FeatureState, | |
| *, | |
| enabled: bool | None, | |
| value: FeatureValue | None, | |
| multivariate_values: list[MultivariateValueChangeSet] | None, | |
| ) -> None: | |
| """Apply only the provided parts of a change; omissions are left intact.""" | |
| with transaction.atomic(): | |
| if enabled is not None: | |
| feature_state.enabled = enabled | |
| feature_state.save() | |
| if value is not None: | |
| _update_feature_state_value(feature_state.feature_state_value, value) | |
| update_multivariate_values(feature_state, multivariate_values) |
| else: | ||
| option_id = option.id | ||
| if option.value is not None: | ||
| mv_option = feature.multivariate_options.get(id=option_id) | ||
| mv_option.set_value(option.value.value, option.value.type_) | ||
| mv_option.save() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep existing option default allocations in sync.
New options persist default_percentage_allocation, but existing options only update their value; changing option.percentage_allocation leaves MultivariateFeatureOption.default_percentage_allocation stale.
Proposed fix
else:
option_id = option.id
+ mv_option = feature.multivariate_options.get(id=option_id)
if option.value is not None:
- mv_option = feature.multivariate_options.get(id=option_id)
mv_option.set_value(option.value.value, option.value.type_)
+ mv_option.default_percentage_allocation = option.percentage_allocation
+ mv_option.save()
- mv_option.save()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| else: | |
| option_id = option.id | |
| if option.value is not None: | |
| mv_option = feature.multivariate_options.get(id=option_id) | |
| mv_option.set_value(option.value.value, option.value.type_) | |
| mv_option.save() | |
| else: | |
| option_id = option.id | |
| mv_option = feature.multivariate_options.get(id=option_id) | |
| if option.value is not None: | |
| mv_option.set_value(option.value.value, option.value.type_) | |
| mv_option.default_percentage_allocation = option.percentage_allocation | |
| mv_option.save() |
| def test_update_flag__segment_override__updates_multivariate_options( | ||
| admin_client: APIClient, | ||
| environment_api_key: str, | ||
| feature: int, | ||
| mv_option_value: str, | ||
| mv_option_50_percent: int, | ||
| segment: int, | ||
| # segment_featurestate: int, | ||
| versioned_environment: Environment, | ||
| endpoint: str, | ||
| payload: Callable[[int, int, int], FeatureUpdatePayload], | ||
| ) -> None: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Remove leftover commented-out parameter.
# segment_featurestate: int, on line 205 appears to be a debug/leftover artifact.
🧹 Proposed fix
mv_option_value: str,
mv_option_50_percent: int,
segment: int,
- # segment_featurestate: int,
versioned_environment: Environment,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_update_flag__segment_override__updates_multivariate_options( | |
| admin_client: APIClient, | |
| environment_api_key: str, | |
| feature: int, | |
| mv_option_value: str, | |
| mv_option_50_percent: int, | |
| segment: int, | |
| # segment_featurestate: int, | |
| versioned_environment: Environment, | |
| endpoint: str, | |
| payload: Callable[[int, int, int], FeatureUpdatePayload], | |
| ) -> None: | |
| def test_update_flag__segment_override__updates_multivariate_options( | |
| admin_client: APIClient, | |
| environment_api_key: str, | |
| feature: int, | |
| mv_option_value: str, | |
| mv_option_50_percent: int, | |
| segment: int, | |
| versioned_environment: Environment, | |
| endpoint: str, | |
| payload: Callable[[int, int, int], FeatureUpdatePayload], | |
| ) -> None: |
| pytest.param( | ||
| "update-flag-v1", | ||
| lambda feature, segment: { | ||
| "feature": {"id": feature}, | ||
| "segment": {"id": segment}, | ||
| "multivariate_options": [ | ||
| { | ||
| "percentage_allocation": 50, | ||
| "value": {"type": "string", "value": "variant"}, | ||
| } | ||
| ], | ||
| }, | ||
| "require an option 'id'", | ||
| id="option_a-segment-option-without-id", | ||
| ), | ||
| pytest.param( | ||
| "update-flag-v1", | ||
| lambda feature, segment: { | ||
| "feature": {"id": feature}, | ||
| "segment": {"id": segment}, | ||
| "multivariate_options": [{"id": 999999, "percentage_allocation": 50}], | ||
| }, | ||
| "do not belong to the feature", | ||
| id="option_a-segment-unknown-option", | ||
| ), | ||
| pytest.param( | ||
| "update-flag-v2", | ||
| lambda feature, segment: { | ||
| "feature": {"id": feature}, | ||
| "segment_overrides": [ | ||
| { | ||
| "segment_id": segment, | ||
| "multivariate_options": [ | ||
| {"id": 999999, "percentage_allocation": 50} | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| "do not belong to the feature", | ||
| id="option_b-segment-unknown-option", | ||
| ), | ||
| pytest.param( | ||
| "update-flag-v1", | ||
| lambda feature, segment: { | ||
| "feature": {"id": feature}, | ||
| "segment": {"id": segment}, | ||
| "multivariate_options": [ | ||
| {"id": 999999, "percentage_allocation": 20}, | ||
| {"id": 999999, "percentage_allocation": 30}, | ||
| ], | ||
| }, | ||
| "must be unique", | ||
| id="option_a-segment-duplicate-option", | ||
| ), | ||
| pytest.param( | ||
| "update-flag-v2", | ||
| lambda feature, segment: { | ||
| "feature": {"id": feature}, | ||
| "segment_overrides": [ | ||
| { | ||
| "segment_id": segment, | ||
| "multivariate_options": [ | ||
| {"id": 999999, "percentage_allocation": 20}, | ||
| {"id": 999999, "percentage_allocation": 30}, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| "must be unique", | ||
| id="option_b-segment-duplicate-option", | ||
| ), | ||
| pytest.param( | ||
| "update-flag-v1", | ||
| lambda feature, segment: { | ||
| "feature": {"name": "missing-feature"}, | ||
| "multivariate_options": [{"id": 999999, "percentage_allocation": 50}], | ||
| }, | ||
| "not found in project", | ||
| id="option_a-unknown-feature", | ||
| ), | ||
| ], |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Segment-override and unknown-feature validation cases lack Option B counterparts.
option_a-segment-option-without-id, option_a-segment-unknown-option, option_a-segment-duplicate-option, and option_a-unknown-feature only test the update-flag-v1 (Option A) endpoint. Since the PR's goal is parity between Option A and Option B for multivariate support, adding matching update-flag-v2 (Option B) parametrizations for these validation paths would close the coverage gap and confirm both endpoints share the same validation guarantees.
| 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.
👻
Only certain spirits are expected to have different names depending on the realm they're in, but these are just code.3 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Contributes to #7642
How did you test this code?
Unit tests.