-
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?
Changes from all commits
4ea79aa
891598e
0bc2de2
d128862
4e6cec8
b0c0a6a
2c03d5e
fc21bc0
bf7bd7b
db2477e
03d0aa6
cf91675
6ecba23
8a34a26
5627624
6a3fa62
39c4b44
8d43e11
ce92682
b8eeb6d
2b78c5e
3704c9c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| """ | ||
| Experimental API endpoints. | ||
|
|
||
| These endpoints are subject to change and should not be considered stable. | ||
| Use at your own risk - breaking changes may occur without prior notice. | ||
| """ | ||
|
|
||
| from django.urls import path | ||
|
|
||
| from features.feature_states.views import update_flag_v1, update_flag_v2 | ||
|
|
||
| app_name = "experiments" | ||
|
|
||
| urlpatterns = [ | ||
| path( | ||
| "environments/<str:environment_key>/update-flag-v1/", | ||
| update_flag_v1, | ||
| name="update-flag-v1", | ||
| ), | ||
| path( | ||
| "environments/<str:environment_key>/update-flag-v2/", | ||
| update_flag_v2, | ||
| name="update-flag-v2", | ||
| ), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| from common.environments.permissions import UPDATE_FEATURE_STATE | ||
| from rest_framework.permissions import BasePermission | ||
| from rest_framework.request import Request | ||
| from rest_framework.views import APIView | ||
|
|
||
| from environments.models import Environment | ||
|
|
||
|
|
||
| class EnvironmentUpdateFeatureStatePermission(BasePermission): | ||
| def has_permission(self, request: Request, view: APIView) -> bool: | ||
| environment_key = view.kwargs.get("environment_key") | ||
| try: | ||
| environment = Environment.objects.get(api_key=environment_key) | ||
| except Environment.DoesNotExist: | ||
| return False | ||
|
|
||
| return request.user.has_environment_permission( # type: ignore[union-attr] | ||
| UPDATE_FEATURE_STATE, environment | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,190 @@ | ||
| from rest_framework import serializers | ||
|
|
||
| from environments.models import Environment | ||
| from features.models import Feature, FeatureState | ||
| from features.versioning.dataclasses import ( | ||
| FlagChangeSet, | ||
| FlagChangeSetV2, | ||
| SegmentOverrideChangeSet, | ||
| ) | ||
| from features.versioning.versioning_service import update_flag, update_flag_v2 | ||
| from segments.models import Segment | ||
|
|
||
|
|
||
| class BaseFeatureUpdateSerializer(serializers.Serializer): # type: ignore[type-arg] | ||
| @property | ||
| def environment(self) -> Environment: | ||
| environment: Environment | None = self.context.get("environment") | ||
| if not environment: | ||
| raise serializers.ValidationError("Environment context is required") | ||
| return environment | ||
gagantrivedi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| def get_feature(self) -> Feature: | ||
| feature_data = self.validated_data["feature"] | ||
| try: | ||
| feature: Feature = Feature.objects.get( | ||
| project_id=self.environment.project_id, **feature_data | ||
| ) | ||
| return feature | ||
| except Feature.DoesNotExist: | ||
| raise serializers.ValidationError( | ||
| f"Feature '{feature_data}' not found in project" | ||
| ) | ||
|
|
||
| def validate_segment_id(self, segment_id: int) -> None: | ||
| if not Segment.objects.filter( | ||
| id=segment_id, project_id=self.environment.project_id | ||
| ).exists(): | ||
| raise serializers.ValidationError( | ||
| f"Segment with id {segment_id} not found in project" | ||
| ) | ||
|
|
||
|
|
||
| class FeatureIdentifierSerializer(serializers.Serializer): # type: ignore[type-arg] | ||
| name = serializers.CharField(required=False, allow_blank=False) | ||
| id = serializers.IntegerField(required=False) | ||
|
|
||
| def validate(self, data: dict) -> dict: # type: ignore[type-arg] | ||
| if not data.get("name") and not data.get("id"): | ||
| raise serializers.ValidationError( | ||
| "Either 'name' or 'id' is required for feature identification" | ||
| ) | ||
| if data.get("name") and data.get("id"): | ||
| raise serializers.ValidationError("Provide either 'name' or 'id', not both") | ||
| return data | ||
|
|
||
|
|
||
| class FeatureUpdateSegmentDataSerializer(serializers.Serializer): # type: ignore[type-arg] | ||
| id = serializers.IntegerField(required=True) | ||
| priority = serializers.IntegerField(required=False, allow_null=True) | ||
gagantrivedi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
|
||
| class FeatureValueSerializer(serializers.Serializer): # type: ignore[type-arg] | ||
| type = serializers.ChoiceField( | ||
| choices=["integer", "string", "boolean"], required=True | ||
| ) | ||
| string_value = serializers.CharField(required=True, allow_blank=True) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Besides the fact that the schema should already inform the field type, it looks like there is an entire section in the description for the new API dedicated to compensating for any confusion that might be induced by this naming choice. Consider renaming to |
||
|
|
||
| def validate(self, data: dict) -> dict: # type: ignore[type-arg] | ||
| value_type = data["type"] | ||
| string_val = data["string_value"] | ||
|
|
||
| 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')" | ||
| ) | ||
|
Comment on lines
+72
to
+83
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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., |
||
|
|
||
| return data | ||
|
|
||
|
|
||
| class UpdateFlagSerializer(BaseFeatureUpdateSerializer): | ||
| feature = FeatureIdentifierSerializer(required=True) | ||
| segment = FeatureUpdateSegmentDataSerializer(required=False) | ||
| enabled = serializers.BooleanField(required=True) | ||
| value = FeatureValueSerializer(required=True) | ||
|
|
||
| def validate_segment(self, value: dict) -> dict: # type: ignore[type-arg] | ||
| if value and value.get("id"): | ||
| self.validate_segment_id(value["id"]) | ||
| return value | ||
|
|
||
| @property | ||
| def flag_change_set(self) -> FlagChangeSet: | ||
| validated_data = self.validated_data | ||
| value_data = validated_data["value"] | ||
| segment_data = validated_data.get("segment") | ||
|
|
||
| change_set = FlagChangeSet( | ||
| enabled=validated_data["enabled"], | ||
| feature_state_value=value_data["string_value"], | ||
| type_=value_data["type"], | ||
| segment_id=segment_data.get("id") if segment_data else None, | ||
| segment_priority=segment_data.get("priority") if segment_data else None, | ||
| ) | ||
|
|
||
| change_set.set_audit_fields_from_request(self.context["request"]) | ||
| return change_set | ||
|
|
||
| def save(self, **kwargs: object) -> FeatureState: | ||
| feature = self.get_feature() | ||
| return update_flag(self.environment, feature, self.flag_change_set) | ||
|
|
||
|
|
||
| class EnvironmentDefaultSerializer(serializers.Serializer): # type: ignore[type-arg] | ||
| enabled = serializers.BooleanField(required=True) | ||
| value = FeatureValueSerializer(required=True) | ||
|
|
||
|
|
||
| class SegmentOverrideSerializer(serializers.Serializer): # type: ignore[type-arg] | ||
| segment_id = serializers.IntegerField(required=True) | ||
| priority = serializers.IntegerField(required=False, allow_null=True) | ||
| enabled = serializers.BooleanField(required=True) | ||
| value = FeatureValueSerializer(required=True) | ||
|
|
||
|
|
||
| class UpdateFlagV2Serializer(BaseFeatureUpdateSerializer): | ||
| feature = FeatureIdentifierSerializer(required=True) | ||
| environment_default = EnvironmentDefaultSerializer(required=True) | ||
| segment_overrides = SegmentOverrideSerializer(many=True, required=False) | ||
|
|
||
| def validate_segment_overrides( | ||
| self, | ||
| value: list[dict], # type: ignore[type-arg] | ||
| ) -> list[dict]: # type: ignore[type-arg] | ||
| if not value: | ||
| return value | ||
|
|
||
| segment_ids = [override["segment_id"] for override in value] | ||
| if len(segment_ids) != len(set(segment_ids)): | ||
| raise serializers.ValidationError( | ||
| "Duplicate segment_id values are not allowed" | ||
| ) | ||
|
|
||
| for segment_id in segment_ids: | ||
| self.validate_segment_id(segment_id) | ||
|
Comment on lines
+151
to
+152
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we need a TODO to optimise this bit in the future as well. |
||
|
|
||
| return value | ||
|
|
||
| @property | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using a property on serializer class for the mapped data is an interesting pattern, but I couldn't find examples of it anywhere else in the codebase. I appreciate the code is experimental, but, for a bit more consistency in the codebase, consider introducing a mapper function instead. |
||
| def change_set_v2(self) -> FlagChangeSetV2: | ||
| validated_data = self.validated_data | ||
|
|
||
| env_default = validated_data["environment_default"] | ||
| env_value_data = env_default["value"] | ||
|
|
||
| segment_overrides_data = validated_data.get("segment_overrides", []) | ||
| segment_overrides = [] | ||
|
|
||
| for override_data in segment_overrides_data: | ||
| value_data = override_data["value"] | ||
|
|
||
| segment_override = SegmentOverrideChangeSet( | ||
| segment_id=override_data["segment_id"], | ||
| enabled=override_data["enabled"], | ||
| feature_state_value=value_data["string_value"], | ||
| type_=value_data["type"], | ||
| priority=override_data.get("priority"), | ||
| ) | ||
| segment_overrides.append(segment_override) | ||
|
|
||
| change_set = FlagChangeSetV2( | ||
| environment_default_enabled=env_default["enabled"], | ||
| environment_default_value=env_value_data["string_value"], | ||
| environment_default_type=env_value_data["type"], | ||
| segment_overrides=segment_overrides, | ||
| ) | ||
|
|
||
| change_set.set_audit_fields_from_request(self.context["request"]) | ||
| return change_set | ||
|
|
||
| def save(self, **kwargs: object) -> dict: # type: ignore[type-arg] | ||
| feature = self.get_feature() | ||
| return update_flag_v2(self.environment, feature, self.change_set_v2) | ||
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 could define a
Literal["string", "integer", "boolean"]type, assign it to thetype_argument here and the change set dataclasses'type_fields as well. If we use something like Pydantic'sTypeAdapterto map incoming serializer data to the DTOs, we'll get a fair guarantee against invalid values here.