-
Notifications
You must be signed in to change notification settings - Fork 429
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
feat: Clone identity flag states #3773
Changes from 11 commits
0f95266
0327df9
d0aa54d
2c6fa65
80edfbc
1d440eb
24522c4
4c4f4fe
fd504fb
3ba5df8
2a70bbe
1032376
476005a
d59c38c
81e6e55
40abe1b
938ade1
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 |
---|---|---|
|
@@ -7,7 +7,7 @@ | |
from django.shortcuts import get_object_or_404 | ||
from django.utils.decorators import method_decorator | ||
from drf_yasg.utils import swagger_auto_schema | ||
from flag_engine.identities.models import IdentityModel | ||
from flag_engine.identities.models import IdentityFeaturesList, IdentityModel | ||
from flag_engine.identities.traits.models import TraitModel | ||
from pyngo import drf_error_details | ||
from rest_framework import status, viewsets | ||
|
@@ -38,6 +38,7 @@ | |
EdgeIdentityFsQueryparamSerializer, | ||
EdgeIdentityIdentifierSerializer, | ||
EdgeIdentitySerializer, | ||
EdgeIdentitySourceIdentityRequestSerializer, | ||
EdgeIdentityTraitsSerializer, | ||
EdgeIdentityWithIdentifierFeatureStateDeleteRequestBody, | ||
EdgeIdentityWithIdentifierFeatureStateRequestBody, | ||
|
@@ -202,7 +203,6 @@ class EdgeIdentityFeatureStateViewSet(viewsets.ModelViewSet): | |
lookup_field = "featurestate_uuid" | ||
|
||
serializer_class = EdgeIdentityFeatureStateSerializer | ||
|
||
# Patch is not supported | ||
http_method_names = [ | ||
"get", | ||
|
@@ -215,10 +215,9 @@ class EdgeIdentityFeatureStateViewSet(viewsets.ModelViewSet): | |
] | ||
pagination_class = None | ||
|
||
def initial(self, request, *args, **kwargs): | ||
super().initial(request, *args, **kwargs) | ||
def get_identity(self, edge_identity_identity_uuid) -> EdgeIdentity: | ||
matthewelwell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
identity_document = EdgeIdentity.dynamo_wrapper.get_item_from_uuid_or_404( | ||
self.kwargs["edge_identity_identity_uuid"] | ||
edge_identity_identity_uuid | ||
) | ||
|
||
if ( | ||
|
@@ -233,8 +232,15 @@ def initial(self, request, *args, **kwargs): | |
environment__api_key=identity.environment_api_key | ||
).values_list("feature__name", flat=True) | ||
) | ||
identity.synchronise_features(valid_feature_names) | ||
self.identity = identity | ||
identity.synchronise_features(valid_feature_names=valid_feature_names) | ||
|
||
return identity | ||
|
||
def initial(self, request, *args, **kwargs): | ||
super().initial(request, *args, **kwargs) | ||
self.identity: EdgeIdentity = self.get_identity( | ||
edge_identity_identity_uuid=self.kwargs["edge_identity_identity_uuid"] | ||
) | ||
|
||
def get_object(self): | ||
feature_state = self.identity.get_feature_state_by_featurestate_uuid( | ||
|
@@ -261,7 +267,7 @@ def list(self, request, *args, **kwargs): | |
) | ||
q_params_serializer.is_valid(raise_exception=True) | ||
|
||
identity_features = self.identity.feature_overrides | ||
identity_features: IdentityFeaturesList = self.identity.feature_overrides | ||
|
||
feature = q_params_serializer.data.get("feature") | ||
if feature: | ||
|
@@ -300,6 +306,29 @@ def all(self, request, *args, **kwargs): | |
|
||
return Response(serializer.data) | ||
|
||
@swagger_auto_schema(request_body=EdgeIdentitySourceIdentityRequestSerializer()) | ||
@action(detail=False, methods=["POST"], url_path="clone-from-given-identity") | ||
def clone_from_given_identity(self, request, *args, **kwargs) -> Response: | ||
""" | ||
Clone feature states from a given source identity. | ||
""" | ||
# Get and validate source identity | ||
serializer = EdgeIdentitySourceIdentityRequestSerializer( | ||
data=request.data, context={"request": request} | ||
) | ||
serializer.is_valid(raise_exception=True) | ||
|
||
source_identity: EdgeIdentity = self.get_identity( | ||
edge_identity_identity_uuid=request.data.get("source_identity_uuid") | ||
) | ||
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. Nit: Use the serializer to get the |
||
|
||
self.identity.clone_flag_states_from(source_identity) | ||
self.identity.save( | ||
user=request.user.id, | ||
master_api_key=getattr(request, "master_api_key", None), | ||
) | ||
return Response(status=status.HTTP_200_OK) | ||
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. Couldn't this return the identity to save another API request from the FE? 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. Yeah, I was originally sending the new Identity feature states but in the FE that would require a lot of additional logic, currently, the API client we use (RTK Query) invalidates the state to build the view, and then calls the endpoint originally used to get the data view, the get all feature states, and while we could tweak it, that's the RTK's philosophy. 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. It seems strange to me that RTK can't use data included in the response. 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. It can, I just would need extra logic to merge the data coming from the original endpoint which includes much more data and this one, while in the RTKway you just invalidate the data being changed and it will make the request, update the view, or pick it from cache, or do whatever is needed automatically. 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. Ok, I'm not sure that necessarily means that the view shouldn't return the data though - other direct API users might want to see that data. 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. Right. Returning the identity feature flags again. |
||
|
||
|
||
class EdgeIdentityWithIdentifierFeatureStateView(APIView): | ||
permission_classes = [IsAuthenticated, EdgeIdentityWithIdentifierViewPermissions] | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -141,6 +141,32 @@ def get_all_feature_states( | |||||||||
|
||||||||||
return list(identity_flags.values()) | ||||||||||
|
||||||||||
def get_overridden_feature_states(self) -> dict[str, FeatureState]: | ||||||||||
matthewelwell marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
""" | ||||||||||
Get all overridden feature states for an identity. | ||||||||||
|
||||||||||
:return: dict[str, FeatureState] - Key: feature ID. Value: Overridden feature_state. | ||||||||||
""" | ||||||||||
overridden_feature_states: dict[str, FeatureState] = {} | ||||||||||
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 part needs updating too.
Suggested change
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. Correct, my changes will include also that, thanks! 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. Usually the suggestions breke code since are amde on the fly without an IDE so I apply them in my IDE checking what else is needed, and usually additional changes are needed :) |
||||||||||
|
||||||||||
query = Q(environment=self.environment) & Q(identity=self) | ||||||||||
query &= Q(live_from__lte=timezone.now(), version__isnull=False) | ||||||||||
matthewelwell marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
|
||||||||||
select_related_args: list[str] = [ | ||||||||||
"environment", | ||||||||||
"feature", | ||||||||||
"feature_state_value", | ||||||||||
"identity", | ||||||||||
] | ||||||||||
|
||||||||||
identity_flags: models.BaseManager[FeatureState] = ( | ||||||||||
FeatureState.objects.select_related(*select_related_args).filter(query) | ||||||||||
) | ||||||||||
|
||||||||||
for feature_state in identity_flags: | ||||||||||
overridden_feature_states[feature_state.feature.id] = feature_state | ||||||||||
return overridden_feature_states | ||||||||||
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. Small aesthetic improvement:
Suggested change
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. Yeah, this is the right/nice pythonic way :) thanks! 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. Sorry, just realised that this should also be optimised slightly, as follows: return {fs.feature_id: fs for fs in identity_flags} 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. Yeah, I noticed that while addressing your first suggestion, now it is a single-line function. 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. No, my point is that we should use |
||||||||||
|
||||||||||
def get_segments( | ||||||||||
self, traits: typing.List[Trait] = None, overrides_only: bool = False | ||||||||||
) -> typing.List[Segment]: | ||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -947,6 +947,48 @@ def _is_more_recent_version(self, other: "FeatureState") -> bool: | |||||||||||||||||||||||||
and self.version > other.version | ||||||||||||||||||||||||||
) or (self.version is not None and other.version is None) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
@staticmethod | ||||||||||||||||||||||||||
def copy_identity_feature_states( | ||||||||||||||||||||||||||
target_identity: "Identity", source_identity: "Identity" | ||||||||||||||||||||||||||
) -> None: | ||||||||||||||||||||||||||
target_feature_states: dict[str, FeatureState] = ( | ||||||||||||||||||||||||||
target_identity.get_overridden_feature_states() | ||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||
source_feature_states: dict[str, FeatureState] = ( | ||||||||||||||||||||||||||
source_identity.get_overridden_feature_states() | ||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||
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 think these dicts have
Suggested change
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. Right. I will do this as part of applying Matt's suggestion |
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
# Delete own feature states not in source_identity | ||||||||||||||||||||||||||
feature_states_to_delete = list( | ||||||||||||||||||||||||||
target_feature_states.keys() - source_feature_states.keys() | ||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||
for feature_state_id in feature_states_to_delete: | ||||||||||||||||||||||||||
target_feature_states[feature_state_id].delete() | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
# Clone source_identity's feature states to target_identity | ||||||||||||||||||||||||||
for feature_id_source in source_feature_states: | ||||||||||||||||||||||||||
matthewelwell marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
# Get target feature_state if exists in target identity or create new one | ||||||||||||||||||||||||||
target_feature_state: FeatureState = target_feature_states.get( | ||||||||||||||||||||||||||
feature_id_source | ||||||||||||||||||||||||||
) or FeatureState.objects.create( | ||||||||||||||||||||||||||
environment=target_identity.environment, | ||||||||||||||||||||||||||
identity=target_identity, | ||||||||||||||||||||||||||
feature=source_feature_states[feature_id_source].feature, | ||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
# Copy enabled value from source feature_state | ||||||||||||||||||||||||||
target_feature_state.enabled = source_feature_states[ | ||||||||||||||||||||||||||
feature_id_source | ||||||||||||||||||||||||||
].enabled | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
# Copy feature state value from source feature_state | ||||||||||||||||||||||||||
target_feature_state.feature_state_value.copy_from( | ||||||||||||||||||||||||||
source_feature_states[feature_id_source].feature_state_value | ||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
# Save changes to target feature_state | ||||||||||||||||||||||||||
target_feature_state.save() | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
class FeatureStateValue( | ||||||||||||||||||||||||||
AbstractBaseFeatureValueModel, | ||||||||||||||||||||||||||
|
@@ -972,6 +1014,14 @@ def clone(self, feature_state: FeatureState) -> "FeatureStateValue": | |||||||||||||||||||||||||
clone.save() | ||||||||||||||||||||||||||
return clone | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
def copy_from(self, source_feature_state_value: "FeatureStateValue"): | ||||||||||||||||||||||||||
# Copy feature state type and values from given feature state value. | ||||||||||||||||||||||||||
self.type = source_feature_state_value.type | ||||||||||||||||||||||||||
self.boolean_value = source_feature_state_value.boolean_value | ||||||||||||||||||||||||||
self.integer_value = source_feature_state_value.integer_value | ||||||||||||||||||||||||||
self.string_value = source_feature_state_value.string_value | ||||||||||||||||||||||||||
self.save() | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
def get_update_log_message(self, history_instance) -> typing.Optional[str]: | ||||||||||||||||||||||||||
fs = self.feature_state | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ | |
from environments.identities.models import Identity | ||
from environments.identities.serializers import ( | ||
IdentityAllFeatureStatesSerializer, | ||
IdentitySourceIdentityRequestSerializer, | ||
) | ||
from environments.models import Environment | ||
from environments.permissions.permissions import ( | ||
|
@@ -636,6 +637,31 @@ def all(self, request, *args, **kwargs): | |
|
||
return Response(serializer.data) | ||
|
||
@swagger_auto_schema(request_body=IdentitySourceIdentityRequestSerializer()) | ||
@action(methods=["POST"], detail=False, url_path="clone-from-given-identity") | ||
def clone_from_given_identity(self, request, *args, **kwargs) -> Response: | ||
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. nit: missing typing in the args 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 haven't seen our selves using typing in rest_framework handlers, in general I haven't seen that Python HTTP event handlers, even my IDE doesn't complaint about it, or including the unsed params args and kwargs. 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. There's a ton of code that sets |
||
""" | ||
Clone feature states from a given source identity. | ||
""" | ||
serializer = IdentitySourceIdentityRequestSerializer( | ||
data=request.data, context={"request": request} | ||
) | ||
serializer.is_valid(raise_exception=True) | ||
# Get and validate source and target identities | ||
target_identity = get_object_or_404( | ||
queryset=Identity, pk=self.kwargs["identity_pk"] | ||
) | ||
source_identity = get_object_or_404( | ||
queryset=Identity, pk=request.data.get("source_identity_id") | ||
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. nit: use the serializer which has already been validated 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. My IDE compliants about using the serialized.validated_data since it doesn't understand that, as you mentioned, they were alreay validated, but precily because of that we can use any of both ways right? 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. It's the right way of doing it. When people refactor code they may easily miss accessing data directly but with the moved code it could be done now before validation. To keep things safe, it's best to use the validated data directly. |
||
) | ||
|
||
# Clone feature states | ||
FeatureState.copy_identity_feature_states( | ||
target_identity=target_identity, source_identity=source_identity | ||
) | ||
|
||
return Response(status=status.HTTP_200_OK) | ||
matthewelwell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
@method_decorator( | ||
name="list", | ||
|
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 typing here is completely redundant and it's easier to follow without it.