-
Notifications
You must be signed in to change notification settings - Fork 462
feat(api): add experimental feature flag update endpoints #6305
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Adds two new experimental endpoints under /api/experiments/: - update-flag-v1: Update single feature state (environment default or segment override) - update-flag-v2: Update multiple feature states in a single request (environment default + segment overrides)
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6305 +/- ##
==========================================
+ Coverage 98.02% 98.05% +0.03%
==========================================
Files 1280 1289 +9
Lines 45406 46147 +741
==========================================
+ Hits 44508 45249 +741
Misses 898 898 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Parameterize test_update_flag_by_name for all value types (integer, string, boolean) - Remove unsupported float type from serializer (model doesn't support it) - Add test_serializers.py with unit tests for edge cases - Add test for duplicate segment_id validation - Add tests for updating existing segment overrides with priority - Add unit tests for boolean string conversion and replica path - Refactor _update_flag_for_versioning_v2 to separate segment vs environment default cases for cleaner code and full coverage
Replace SDK endpoint calls with direct database verification using get_environment_flags_list. This keeps the tests as proper unit tests rather than integration tests. - Remove sdk_client and sdk_client_factory fixtures - Remove get_identity_feature_state_from_sdk helper function - Remove Identity, Condition, SegmentRule imports and usage - Simplify segment override tests to verify database state directly
f280c7f to
b987697
Compare
e998a78 to
56443a0
Compare
…rides Creating a FeatureSegment with a priority value doesn't trigger reordering of existing segments. Use .to() after creation to properly reorder. Fixes both V1 and V2 endpoints.
56443a0 to
8d43e11
Compare
…ride Prevents cross-project segment references and provides clear error messages for non-existent segments.
emyller
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The experiment looks solid, despite the complexity brought by the 2x2 grid experiment x versioning version. Few suggestions.
|
|
||
| def update_traits( | ||
| self, | ||
| # TODO: this typing isn't true as it also supports regular dicts like {"trait_key": "foo", "trait_value": "bar"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have an issue to cover this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why it's part of this pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here: #6371
| if value_type == "integer": | ||
| try: | ||
| int(string_val) | ||
| except ValueError: | ||
| raise serializers.ValidationError( | ||
| f"'{string_val}' is not a valid integer" | ||
| ) | ||
| elif value_type == "boolean": | ||
| if string_val.lower() not in ("true", "false"): | ||
| raise serializers.ValidationError( | ||
| f"'{string_val}' is not a valid boolean (use 'true' or 'false')" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have this logic abstracted? It is repeated but raising distinct exceptions, e.g. ValueError there, and ValidationError here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentional defense in depth. The serializer validates at the API boundary and returns 400 with user-friendly errors. The model validates at the data layer to prevent corruption if set_value() is called directly (e.g.,
from management commands, tasks, or other code paths). Different layers appropriately raise different exceptions: ValidationError for API responses, ValueError for programmatic misuse.
| ) | ||
| serializer.is_valid() | ||
|
|
||
| with pytest.raises(Exception) as exc_info: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to narrow down this exception type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing Given/When/Then markers.
| ) | ||
|
|
||
|
|
||
| def update_flag( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions perform multiple database writes without @transaction.atomic. Can we wrap them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have any early exits or conditional paths here that could leave things in a bad state - it's just a straight line of create → update → save. If any step fails, it raises and stops.
Also, if something does fail midway, we'd actually want to know about it. A 500 surfacing is useful signal - silently rolling back could hide bugs we should be investigating.
Worth noting that FeatureState is one of our hottest tables, so holding transaction locks longer than needed would hurt concurrent traffic.
Happy to add one if there's a specific consistency case I'm missing though!
|
|
||
| updated_segments = [] | ||
| for override in change_set.segment_overrides: | ||
| segment_states = get_environment_flags_dict( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_environment_flags_dict is called once per segment override. Can we batch this into a single query?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to do that once this API is out of the experimentation stage
| feature_name=feature.name, | ||
| additional_filters=Q(feature_segment__isnull=True, identity_id__isnull=True), | ||
| ) | ||
| assert len(env_default_states) == 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert provides no message when it fails. Can we raise an explicit exception instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't expect it to fail. The assert is only included to improve readability and document the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a test for concurrent modifications? This is relevant since atomicity is not implemented yet (see other thread).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s a fair point regarding concurrency. However, I’m a bit hesitant to add a specific test for it right now. In my experience, tests requiring thread orchestration to hit precise race windows tend to be quite flaky and hard to maintain.
Also, looking at the logic, I’m not sure a test would give us much new signal. Since we aren't implementing strict locking:
V2: Relies on versioning (last publisher wins).
V1: Defaults to 'last write wins.'
Essentially, the test would just confirm that the last request overwrites the previous one, which is the expected behavior rather than a defect we need to catch. What do you think?
610dba4 to
2b78c5e
Compare
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature!Changes
Adds two new experimental endpoints under /api/experiments/:
Related issues: Create new experimental endpoint to update a single feature state for a given feature #6279 Create new experimental endpoints to update a given feature in an environment #6233
How did you test this code?
Unit tests