Skip to content
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

Merged
merged 17 commits into from
Apr 24, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
0f95266
chore: Implement Core API clone-from-given-identity endpoint
novakzaballa Apr 16, 2024
0327df9
chore: Implement Edge API clone-from-given-identity endpoint
novakzaballa Apr 16, 2024
d0aa54d
chore: Add ENVIRONMENTS_TABLE_NAME_DYNAMO setting in tests
novakzaballa Apr 16, 2024
2c6fa65
test: Include app_settings_for_dynamodb fixture in dynamo_enabled_env…
novakzaballa Apr 16, 2024
80edfbc
test: Remove unit test since it is already covered by integration test
novakzaballa Apr 16, 2024
1d440eb
test: Abstract create identity
novakzaballa Apr 16, 2024
24522c4
chore: Change the edge endpoint name to match core api
novakzaballa Apr 16, 2024
4c4f4fe
chore: Standardize clone identity view function names in edge and cor…
novakzaballa Apr 16, 2024
fd504fb
chore: Add request serializers to clone identity endpoints
novakzaballa Apr 16, 2024
3ba5df8
chore: Add serializers to clone identity endpoints
novakzaballa Apr 16, 2024
2a70bbe
chore: Remove unnecessary method get_overridden_feature_states_dict
novakzaballa Apr 16, 2024
1032376
chore: Apply several optimizations to the code
novakzaballa Apr 17, 2024
476005a
test: Remove URLs fixture and move the code to the test
novakzaballa Apr 17, 2024
d59c38c
test: Add enabled and state_value checks for feature_3
novakzaballa Apr 18, 2024
81e6e55
chore: Return identiy feature states data on endpoints response
novakzaballa Apr 24, 2024
40abe1b
chore: Optimize code to reuse existing code for endpoints response
novakzaballa Apr 24, 2024
938ade1
chore: Optimize query to get only the needed information
novakzaballa Apr 24, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion api/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -615,6 +615,7 @@ def flagsmith_identities_table(dynamodb: DynamoDBServiceResource) -> Table:
{"AttributeName": "composite_key", "AttributeType": "S"},
{"AttributeName": "environment_api_key", "AttributeType": "S"},
{"AttributeName": "identifier", "AttributeType": "S"},
{"AttributeName": "identity_uuid", "AttributeType": "S"},
],
GlobalSecondaryIndexes=[
{
Expand All @@ -624,7 +625,12 @@ def flagsmith_identities_table(dynamodb: DynamoDBServiceResource) -> Table:
{"AttributeName": "identifier", "KeyType": "RANGE"},
],
"Projection": {"ProjectionType": "ALL"},
}
},
{
"IndexName": "identity_uuid-index",
"KeySchema": [{"AttributeName": "identity_uuid", "KeyType": "HASH"}],
"Projection": {"ProjectionType": "ALL"},
},
],
BillingMode="PAY_PER_REQUEST",
)
Expand Down
17 changes: 17 additions & 0 deletions api/edge_api/identities/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -241,3 +241,20 @@ def _get_changes(self) -> IdentityChangeset:

def _reset_initial_state(self):
self._initial_state = copy.deepcopy(self)

def clone_flag_states_from(self, source_identity: "EdgeIdentity") -> None:
"""
Clone the feature states from the source identity to the target identity.
"""
# Delete identity_target's feature states
for feature_state in list(self.feature_overrides):
self.remove_feature_override(feature_state=feature_state)

# Clone identity_source's feature states to identity_target
for feature_in_source in source_identity.feature_overrides:
feature_state_target = FeatureStateModel(
feature=feature_in_source.feature,
feature_state_value=feature_in_source.feature_state_value,
enabled=feature_in_source.enabled,
)
self.add_feature_override(feature_state_target)
7 changes: 7 additions & 0 deletions api/edge_api/identities/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -264,3 +264,10 @@ def to_representation(self, instance: IdentityOverrideV2):

class GetEdgeIdentityOverridesSerializer(serializers.Serializer):
results = GetEdgeIdentityOverridesResultSerializer(many=True)


class EdgeIdentitySourceIdentityRequestSerializer(serializers.Serializer):
source_identity_uuid = serializers.CharField(
required=True,
help_text="UUID of the source identity to clone feature states from.",
)
65 changes: 57 additions & 8 deletions api/edge_api/identities/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -38,6 +38,7 @@
EdgeIdentityFsQueryparamSerializer,
EdgeIdentityIdentifierSerializer,
EdgeIdentitySerializer,
EdgeIdentitySourceIdentityRequestSerializer,
EdgeIdentityTraitsSerializer,
EdgeIdentityWithIdentifierFeatureStateDeleteRequestBody,
EdgeIdentityWithIdentifierFeatureStateRequestBody,
Expand Down Expand Up @@ -202,7 +203,6 @@ class EdgeIdentityFeatureStateViewSet(viewsets.ModelViewSet):
lookup_field = "featurestate_uuid"

serializer_class = EdgeIdentityFeatureStateSerializer

# Patch is not supported
http_method_names = [
"get",
Expand All @@ -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: str) -> EdgeIdentity:
identity_document = EdgeIdentity.dynamo_wrapper.get_item_from_uuid_or_404(
self.kwargs["edge_identity_identity_uuid"]
edge_identity_identity_uuid
)

if (
Expand All @@ -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(
Expand All @@ -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:
Expand Down Expand Up @@ -300,6 +306,49 @@ 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=serializer.validated_data[
"source_identity_uuid"
]
)

self.identity.clone_flag_states_from(source_identity)
self.identity.save(
user=request.user.id,
master_api_key=getattr(request, "master_api_key", None),
)

# Prepare response
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't we just do return self.all(request, *args, **kwargs) ? Also, bear in mind that the documentation for this endpoint needs updating as well!

(
feature_states,
identity_feature_names,
) = self.identity.get_all_feature_states()

serializer = IdentityAllFeatureStatesSerializer(
instance=feature_states,
many=True,
context={
"request": request,
"identity": self.identity,
"environment_api_key": self.identity.environment_api_key,
"identity_feature_names": identity_feature_names,
},
)

return Response(serializer.data)


class EdgeIdentityWithIdentifierFeatureStateView(APIView):
permission_classes = [IsAuthenticated, EdgeIdentityWithIdentifierViewPermissions]
Expand Down
3 changes: 2 additions & 1 deletion api/environments/dynamodb/wrappers/environment_wrapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ def write_environments(self, environments: Iterable["Environment"]) -> None:


class DynamoEnvironmentWrapper(BaseDynamoEnvironmentWrapper):
table_name = settings.ENVIRONMENTS_TABLE_NAME_DYNAMO
def get_table_name(self) -> str | None:
return settings.ENVIRONMENTS_TABLE_NAME_DYNAMO

def write_environments(self, environments: Iterable["Environment"]):
with self.table.batch_writer() as writer:
Expand Down
9 changes: 9 additions & 0 deletions api/environments/identities/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,15 @@ def get_all_feature_states(

return list(identity_flags.values())

def get_overridden_feature_states(self) -> dict[int, FeatureState]:
"""
Get all overridden feature states for an identity.

:return: dict[int, FeatureState] - Key: feature ID. Value: Overridden feature_state.
"""

return {fs.feature.id: fs for fs in self.identity_features.all()}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return {fs.feature.id: fs for fs in self.identity_features.all()}
return {fs.feature_id: fs for fs in self.identity_features.all()}

Related thread: #3773 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, we don't need to load the whole related feature but just the FK which is already in the feature_sate data.
Done


def get_segments(
self, traits: typing.List[Trait] = None, overrides_only: bool = False
) -> typing.List[Segment]:
Expand Down
7 changes: 7 additions & 0 deletions api/environments/identities/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -140,3 +140,10 @@ def get_segment(self, instance) -> typing.Optional[typing.Dict[str, typing.Any]]
instance=instance.feature_segment.segment
).data
return None


class IdentitySourceIdentityRequestSerializer(serializers.Serializer):
source_identity_id = serializers.IntegerField(
required=True,
help_text="ID of the source identity to clone feature states from.",
)
50 changes: 50 additions & 0 deletions api/features/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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[int, FeatureState] = (
target_identity.get_overridden_feature_states()
)
source_feature_states: dict[int, FeatureState] = (
source_identity.get_overridden_feature_states()
)

# 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 source_feature_id, source_feature_state in source_feature_states.items():
# Get target feature_state if exists in target identity or create new one
target_feature_state: FeatureState = target_feature_states.get(
source_feature_id
) or FeatureState.objects.create(
environment=target_identity.environment,
identity=target_identity,
feature=source_feature_state.feature,
)

# Copy enabled value from source feature_state
target_feature_state.enabled = source_feature_states[
source_feature_id
].enabled

# Copy feature state value from source feature_state
target_feature_state.feature_state_value.copy_from(
source_feature_state.feature_state_value
)

# Save changes to target feature_state
target_feature_state.save()


class FeatureStateValue(
AbstractBaseFeatureValueModel,
Expand All @@ -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

Expand Down
38 changes: 38 additions & 0 deletions api/features/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -636,6 +637,43 @@ 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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: missing typing in the args

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a ton of code that sets request: Request and our orders from @matthewelwell is to always set the typing even in places that seem silly, like typing in test fixtures.

"""
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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: use the serializer which has already been validated

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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
)

# Prepare response
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, can we use return self.all(request, *args, **kwargs) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Done. Also updated the autogenerate swagger command

feature_states = target_identity.get_all_feature_states()
serializer_target = IdentityAllFeatureStatesSerializer(
instance=feature_states,
many=True,
context={
"request": request,
"identity": target_identity,
"environment_api_key": target_identity.environment.api_key,
},
)

return Response(data=serializer_target.data)


@method_decorator(
name="list",
Expand Down
15 changes: 15 additions & 0 deletions api/tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,22 @@ def dynamo_enabled_project_environment_one_document(
@pytest.fixture()
def dynamo_environment_wrapper(
flagsmith_environment_table: Table,
settings: SettingsWrapper,
) -> DynamoEnvironmentWrapper:
settings.ENVIRONMENTS_TABLE_NAME_DYNAMO = flagsmith_environment_table.name
wrapper = DynamoEnvironmentWrapper()
wrapper.table_name = flagsmith_environment_table.name
return wrapper


@pytest.fixture()
def app_settings_for_dynamodb(
settings: SettingsWrapper,
flagsmith_environment_table: Table,
flagsmith_environments_v2_table: Table,
flagsmith_identities_table: Table,
) -> None:
settings.ENVIRONMENTS_TABLE_NAME_DYNAMO = flagsmith_environment_table.name
settings.ENVIRONMENTS_V2_TABLE_NAME_DYNAMO = flagsmith_environments_v2_table.name
settings.IDENTITIES_TABLE_NAME_DYNAMO = flagsmith_identities_table.name
return
5 changes: 4 additions & 1 deletion api/tests/integration/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,10 @@ def environment(

@pytest.fixture()
def dynamo_enabled_environment(
admin_client, dynamo_enabled_project, environment_api_key
admin_client: APIClient,
dynamo_enabled_project: int,
environment_api_key: str,
app_settings_for_dynamodb: None,
) -> int:
environment_data = {
"name": "Test Environment",
Expand Down
Loading