From 2ef4d36df0c0996635d687201f9365087ce66638 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 29 Jan 2025 11:07:40 +0000 Subject: [PATCH 1/5] First pass at wording / naming improvements --- api/segments/models.py | 109 ++++++++---------- .../segments/test_unit_segments_models.py | 32 ++--- 2 files changed, 61 insertions(+), 80 deletions(-) diff --git a/api/segments/models.py b/api/segments/models.py index 5aeae31a8829..17b115db0956 100644 --- a/api/segments/models.py +++ b/api/segments/models.py @@ -189,69 +189,59 @@ def deep_clone(self) -> "Segment": return cloned_segment - def update_segment_with_matches_from_current_segment( - self, current_segment: "Segment" + def update_segment_with_matches_from_target_segment( + self, target_segment: "Segment" ) -> bool: """ - Assign matching rules of the calling object (i.e., self) to the rules - of the given segment (i.e., current_segment) in order to update the - rules, subrules, and conditions of the calling object. This is done - from the context of a change request related to the calling object. - - This method iterates through the rules of the provided `Segment` and - attempts to match them to the calling object's (i.e., self) rules and - sub-rules, updating versioning where matches are found. - - This is done in order to set the `version_of` field for matched rules - and conditions so that the frontend can more reliably diff rules and - conditions between a change request for a the current segment and the - calling object (i.e., self) itself. + This method handles the matching of two segments for the purpose of + diffing them, for example in the context of a change request. We take + the segment provided as an argument (`target_segment`), compare its + rules to this segment instance (`self`) and update any rules (and, + indirectly, conditions) on this segment instance to have a pointer to + the target segment's rules (and conditions). Args: - current_segment (Segment): The segment whose rules are being matched - against the current object's rules which - will have its rules and conditions updated. + target_segment (Segment): The target segment we are matching against. Returns: bool: - - `True` if any rules or sub-rules match between the calling - object (i.e., self) and the current segment. + - `True` if any rules or sub-rules match between this instance + (i.e. self) and the target segment. - `False` if no matches are found. Process: - 1. Retrieve all rules associated with the calling object and the segment. - 2. For each rule in the current segment: - - Check its sub-rules against the calling object's rules and sub-rules. + 1. Retrieve all rules for both segments. + 2. For each rule in the target segment: + - Check its sub-rules against this instance's rules and sub-rules. - A match is determined if the sub-rule's type and properties align - with those of the calling object's sub-rules. + with those of this instance's sub-rules. - If a match is found: - - Update the `version_of` field for the matched sub-rule and rule. + - Update the `version_of` field for this instance's matched sub-rule + and rule. Do not modify the target segment rules. - Track the matched rules and sub-rules to avoid duplicate processing. 3. Perform a bulk update on matched rules and sub-rules to persist versioning changes. Side Effects: - - Updates the `version_of` field for matched rules and sub-rules for the - calling object (i.e., self). + - Updates the `version_of` field for matched rules and sub-rules belonging + to this instance (i.e., self). - Indirectly updates the `version_of` field on sub-rules' conditions. """ - modified_rules = self.rules.all() + rules = self.rules.all() + matched_rules = set() matched_sub_rules = set() - for current_rule in current_segment.rules.all(): - for current_sub_rule in current_rule.rules.all(): - - sub_rule_matched = False - for modified_rule in modified_rules: - # Because we must proceed to the next current_sub_rule - # to get the next available match since it has now been - # matched to a candidate modified_sub_rule we set the - # sub_rule_matched bool to track the state between - # iterations. Otherwise different rules would have the - # same value for their version_of field. - if sub_rule_matched: + for target_rule in target_segment.rules.all(): + for target_sub_rule in target_rule.rules.all(): + + target_sub_rule_matched = False + + for rule in rules: + if target_sub_rule_matched: + # We only allow a single match for each target sub-rule, + # so break out of this loop to continue to the next. break # Because a segment's rules can only have subrules that match @@ -261,34 +251,25 @@ def update_segment_with_matches_from_current_segment( # points to a different subrule, whose owning rule differs # from the subrule's version_of's parent rule. Such a mismatch # would lead to inconsistencies and unintended behavior. - if ( - modified_rule in matched_rules - and modified_rule.version_of != current_rule - ): + if rule in matched_rules and rule.version_of != target_rule: continue - # To eliminate false matches we force the types - # to be the same for the rules. - if current_rule.type != modified_rule.type: + if target_rule.type != rule.type: continue - for modified_sub_rule in modified_rule.rules.all(): - # If a subrule has already been matched, - # we avoid assigning conditions since it - # has already been handled. - if modified_sub_rule in matched_sub_rules: - continue - - # If a subrule matches, we assign the parent - # rule and the subrule together. - if modified_sub_rule.assign_conditions_if_matching_rule( - current_sub_rule - ): - modified_sub_rule.version_of = current_sub_rule - sub_rule_matched = True - matched_sub_rules.add(modified_sub_rule) - modified_rule.version_of = current_rule - matched_rules.add(modified_rule) + for sub_rule in rule.rules.exclude( + id__in=[r.id for r in matched_sub_rules] + ): + if sub_rule.assign_conditions_if_matching_rule(target_sub_rule): + target_sub_rule_matched = True + + # If a subrule matches, we assign the parent + # rule and the subrule together. + sub_rule.version_of = target_sub_rule + rule.version_of = target_rule + + matched_sub_rules.add(sub_rule) + matched_rules.add(rule) break SegmentRule.objects.bulk_update( diff --git a/api/tests/unit/segments/test_unit_segments_models.py b/api/tests/unit/segments/test_unit_segments_models.py index e4e27d326346..c8e6337b0a9c 100644 --- a/api/tests/unit/segments/test_unit_segments_models.py +++ b/api/tests/unit/segments/test_unit_segments_models.py @@ -575,7 +575,7 @@ def test_saving_condition_with_version_of_set(segment: Segment) -> None: assert condition2.version_of == condition1 -def test_update_segment_with_matches_from_current_segment_with_two_levels_of_rules_and_two_conditions( +def test_update_segment_with_matches_from_target_segment_with_two_levels_of_rules_and_two_conditions( project: Project, ) -> None: # Given @@ -599,7 +599,7 @@ def test_update_segment_with_matches_from_current_segment_with_two_levels_of_rul operator=PERCENTAGE_SPLIT, value=0.2, rule=rule4 ) # When - segment1.update_segment_with_matches_from_current_segment(segment2) + segment1.update_segment_with_matches_from_target_segment(segment2) # Then rule1.refresh_from_db() @@ -620,7 +620,7 @@ def test_update_segment_with_matches_from_current_segment_with_two_levels_of_rul assert condition2.version_of is None -def test_update_segment_with_matches_from_current_segment_with_condition_operator_mismatch( +def test_update_segment_with_matches_from_target_segment_with_condition_operator_mismatch( project: Project, ) -> None: # Given @@ -654,7 +654,7 @@ def test_update_segment_with_matches_from_current_segment_with_condition_operato ) # When - segment1.update_segment_with_matches_from_current_segment(segment2) + segment1.update_segment_with_matches_from_target_segment(segment2) # Then rule1.refresh_from_db() @@ -673,7 +673,7 @@ def test_update_segment_with_matches_from_current_segment_with_condition_operato assert condition2.version_of is None -def test_update_segment_with_matches_from_current_segment_with_conditions_not_matching( +def test_update_segment_with_matches_from_target_segment_with_conditions_not_matching( project: Project, ) -> None: # Given @@ -706,7 +706,7 @@ def test_update_segment_with_matches_from_current_segment_with_conditions_not_ma ) # When - segment1.update_segment_with_matches_from_current_segment(segment2) + segment1.update_segment_with_matches_from_target_segment(segment2) # Then rule1.refresh_from_db() @@ -725,7 +725,7 @@ def test_update_segment_with_matches_from_current_segment_with_conditions_not_ma assert condition2.version_of is None -def test_update_segment_with_matches_from_current_segment_mismatched_rule_type( +def test_update_segment_with_matches_from_target_segment_mismatched_rule_type( project: Project, ) -> None: # Given @@ -748,7 +748,7 @@ def test_update_segment_with_matches_from_current_segment_mismatched_rule_type( operator=PERCENTAGE_SPLIT, value=0.2, rule=rule4 ) # When - segment1.update_segment_with_matches_from_current_segment(segment2) + segment1.update_segment_with_matches_from_target_segment(segment2) # Then rule1.refresh_from_db() @@ -767,7 +767,7 @@ def test_update_segment_with_matches_from_current_segment_mismatched_rule_type( assert condition2.version_of is None -def test_update_segment_with_matches_from_current_segment_mismatched_sub_rule_type( +def test_update_segment_with_matches_from_target_segment_mismatched_sub_rule_type( project: Project, ) -> None: # Given @@ -790,7 +790,7 @@ def test_update_segment_with_matches_from_current_segment_mismatched_sub_rule_ty operator=PERCENTAGE_SPLIT, value=0.2, rule=rule4 ) # When - segment1.update_segment_with_matches_from_current_segment(segment2) + segment1.update_segment_with_matches_from_target_segment(segment2) # Then rule1.refresh_from_db() @@ -809,7 +809,7 @@ def test_update_segment_with_matches_from_current_segment_mismatched_sub_rule_ty assert condition2.version_of is None -def test_update_segment_with_matches_from_current_segment_multiple_sub_rules( +def test_update_segment_with_matches_from_target_segment_multiple_sub_rules( project: Project, ) -> None: # Given @@ -838,7 +838,7 @@ def test_update_segment_with_matches_from_current_segment_multiple_sub_rules( rule11 = SegmentRule.objects.create(rule=rule4, type=SegmentRule.ALL_RULE) # When - segment1.update_segment_with_matches_from_current_segment(segment2) + segment1.update_segment_with_matches_from_target_segment(segment2) # Then rule1.refresh_from_db() @@ -870,7 +870,7 @@ def test_update_segment_with_matches_from_current_segment_multiple_sub_rules( assert rule11.version_of is None -def test_update_segment_with_matches_from_current_segment_with_multiple_conditions( +def test_update_segment_with_matches_from_target_segment_with_multiple_conditions( project: Project, ) -> None: # Given @@ -898,7 +898,7 @@ def test_update_segment_with_matches_from_current_segment_with_multiple_conditio operator=PERCENTAGE_SPLIT, value=0.4, rule=rule4 ) # When - segment1.update_segment_with_matches_from_current_segment(segment2) + segment1.update_segment_with_matches_from_target_segment(segment2) # Then rule1.refresh_from_db() @@ -923,7 +923,7 @@ def test_update_segment_with_matches_from_current_segment_with_multiple_conditio assert condition4.version_of is None -def test_update_segment_with_matches_from_current_segment_with_exact_condition_match( +def test_update_segment_with_matches_from_target_segment_with_exact_condition_match( project: Project, ) -> None: # Given @@ -954,7 +954,7 @@ def test_update_segment_with_matches_from_current_segment_with_exact_condition_m operator=PERCENTAGE_SPLIT, value=0.4, rule=rule4 ) # When - segment1.update_segment_with_matches_from_current_segment(segment2) + segment1.update_segment_with_matches_from_target_segment(segment2) # Then rule1.refresh_from_db() From 44432cfd71735cc0e24425e8a0ca972ec97c498c Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 29 Jan 2025 11:31:20 +0000 Subject: [PATCH 2/5] Second pass at wording / naming improvements --- api/segments/models.py | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/api/segments/models.py b/api/segments/models.py index 17b115db0956..baf77f547c67 100644 --- a/api/segments/models.py +++ b/api/segments/models.py @@ -239,24 +239,21 @@ def update_segment_with_matches_from_target_segment( target_sub_rule_matched = False for rule in rules: + if target_rule.type != rule.type: + continue + if target_sub_rule_matched: - # We only allow a single match for each target sub-rule, - # so break out of this loop to continue to the next. + # We are trying to match 1:1, so we only allow a single + # match for each target sub-rule. break - # Because a segment's rules can only have subrules that match - # if the segment-level rules also match, we need to ensure that - # the currently matched rules correspond to the current rule. - # Consider a scenario where a subrule's version_of attribute - # points to a different subrule, whose owning rule differs - # from the subrule's version_of's parent rule. Such a mismatch - # would lead to inconsistencies and unintended behavior. + # If we've already matched at least one of the rule's sub-rules + # (and hence, the rule has been added to matched_rules), then we + # need to ensure that any subsequent matches belong to the same + # parent rule. if rule in matched_rules and rule.version_of != target_rule: continue - if target_rule.type != rule.type: - continue - for sub_rule in rule.rules.exclude( id__in=[r.id for r in matched_sub_rules] ): From 4b0235eb3f62fc24811687384e0d1e81e230337e Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 29 Jan 2025 11:49:51 +0000 Subject: [PATCH 3/5] Simplify rules method --- api/segments/models.py | 59 ++++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 28 deletions(-) diff --git a/api/segments/models.py b/api/segments/models.py index baf77f547c67..145ab0752ae0 100644 --- a/api/segments/models.py +++ b/api/segments/models.py @@ -359,7 +359,7 @@ def get_segment(self): return rule.segment def assign_conditions_if_matching_rule( # noqa: C901 - self, current_sub_rule: "SegmentRule" + self, target_sub_rule: "SegmentRule" ) -> bool: """ Determines whether the current object matches the given rule @@ -395,13 +395,13 @@ def assign_conditions_if_matching_rule( # noqa: C901 conditions of the calling object (i.e., self). """ - if current_sub_rule.type != self.type: + if target_sub_rule.type != self.type: return False - current_conditions = current_sub_rule.conditions.all() - modified_conditions = self.conditions.all() + target_conditions = target_sub_rule.conditions.all() + conditions = self.conditions.all() - if not current_conditions and not modified_conditions: + if not target_conditions and not conditions: # Empty rule with the same type matches. return True @@ -409,35 +409,25 @@ def assign_conditions_if_matching_rule( # noqa: C901 # In order to provide accurate diffs we first go through the conditions # and collect conditions that have matching values (property, operator, value). - for current_condition in current_conditions: - for modified_condition in modified_conditions: - if modified_condition in matched_conditions: - continue - if ( - current_condition.property == modified_condition.property - and current_condition.operator == modified_condition.operator - and current_condition.value == modified_condition.value + for target_condition in target_conditions: + for condition in conditions: + if (condition not in matched_conditions) and condition.is_exact_match( + target_condition ): - matched_conditions.add(modified_condition) - modified_condition.version_of = current_condition + matched_conditions.add(condition) + condition.version_of = target_condition break # Next we go through the collection again and collect matching conditions # with special logic to collect conditions that have no properties based # on their operator equivalence. - for current_condition in current_conditions: - for modified_condition in modified_conditions: - if modified_condition in matched_conditions: - continue - if not current_condition.property and not modified_condition.property: - if current_condition.operator == modified_condition.operator: - matched_conditions.add(modified_condition) - modified_condition.version_of = current_condition - break - - elif current_condition.property == modified_condition.property: - matched_conditions.add(modified_condition) - modified_condition.version_of = current_condition + for target_condition in target_conditions: + for condition in conditions: + if (condition not in matched_conditions) and condition.is_partial_match( + target_condition + ): + matched_conditions.add(condition) + condition.version_of = target_condition break # If the subrule has no matching conditions we consider the response to @@ -590,6 +580,19 @@ def get_delete_log_message(self, history_instance) -> typing.Optional[str]: def get_audit_log_related_object_id(self, history_instance) -> int: return self._get_segment().id + def is_exact_match(self, other: "Condition") -> bool: + return ( + self.property == other.property + and self.operator == other.operator + and self.value == other.value + ) + + def is_partial_match(self, other: "Condition") -> bool: + if self.property is None and other.property is None: + return self.operator == other.operator + + return self.property == other.property + def _get_segment(self) -> Segment: """ Temporarily cache the segment on the condition object to reduce number of queries. From af6df137c09cd55a55df8536dfdfbe4c6fe68838 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 29 Jan 2025 12:01:19 +0000 Subject: [PATCH 4/5] Wording updates for SegmentRule method, and add docstrings to new Condition methods --- api/segments/models.py | 67 +++++++++++++++++------------------------- 1 file changed, 27 insertions(+), 40 deletions(-) diff --git a/api/segments/models.py b/api/segments/models.py index 145ab0752ae0..8a75641eeb0a 100644 --- a/api/segments/models.py +++ b/api/segments/models.py @@ -359,46 +359,29 @@ def get_segment(self): return rule.segment def assign_conditions_if_matching_rule( # noqa: C901 - self, target_sub_rule: "SegmentRule" + self, target_rule: "SegmentRule" ) -> bool: """ - Determines whether the current object matches the given rule - and assigns conditions with the `version_of` field. - - These assignments are done in order to allow the frontend to - provide a diff capability during change requests for segments. - By knowing which version a condition is for the frontend can - show a more accurate diff between the segment and the change request. - - This method compares the type and conditions of the current object with - the specified `SegmentRule` to determine if they are compatible. + This method handles the matching of two rules for the purpose of + diffing them, for example in the context of a change request for a + given segment. We take the rule provided as an argument (`target_rule`), + and compare its conditions to this instance's conditions. Returns: bool: - - `True` if the calling object's (i.e., self) type matches the rule's type + - `True` if this instance's (i.e. self) type matches the rule's type and the conditions are compatible. - `False` if the types do not match or no conditions are compatible. - Process: - 1. If the types do not match, return `False`. - 2. If both the rule and calling object (i.e., self) have no conditions, return `True`. - 3. Compare each condition in the rule against the calling object's (i.e., self) conditions: - - First match conditions that are an exact match of property, operator, - and value. - - A condition matches if the `property` attributes are equal or if there - is no property but has a matching operator. - - Mark matched conditions and update the versioning. - 4. Return `True` if at least one condition matches; otherwise, return `False`. - Side Effects: Updates the `version_of` field for matched conditions using a bulk update for the - conditions of the calling object (i.e., self). + conditions of this instance (i.e. self). """ - if target_sub_rule.type != self.type: + if target_rule.type != self.type: return False - target_conditions = target_sub_rule.conditions.all() + target_conditions = target_rule.conditions.all() conditions = self.conditions.all() if not target_conditions and not conditions: @@ -408,7 +391,7 @@ def assign_conditions_if_matching_rule( # noqa: C901 matched_conditions = set() # In order to provide accurate diffs we first go through the conditions - # and collect conditions that have matching values (property, operator, value). + # and collect conditions that are exact matches (i.e. have not been modified) for target_condition in target_conditions: for condition in conditions: if (condition not in matched_conditions) and condition.is_exact_match( @@ -418,9 +401,8 @@ def assign_conditions_if_matching_rule( # noqa: C901 condition.version_of = target_condition break - # Next we go through the collection again and collect matching conditions - # with special logic to collect conditions that have no properties based - # on their operator equivalence. + # Next we go through the collection again and collect conditions that have + # been modified from the target condition. for target_condition in target_conditions: for condition in conditions: if (condition not in matched_conditions) and condition.is_partial_match( @@ -430,17 +412,11 @@ def assign_conditions_if_matching_rule( # noqa: C901 condition.version_of = target_condition break - # If the subrule has no matching conditions we consider the response to - # be False, as the subrule could be a better match for some other candidate - # subrule, so the calling method can try the next subrule available. - if not matched_conditions: - return False - - Condition.objects.bulk_update(matched_conditions, fields=["version_of"]) + if matched_conditions: + Condition.objects.bulk_update(matched_conditions, fields=["version_of"]) + return True - # Since the subrule has at least partial condition overlap, we return True - # for the match indicator. - return True + return False def deep_clone(self, cloned_segment: Segment) -> "SegmentRule": if self.rule: @@ -581,6 +557,11 @@ def get_audit_log_related_object_id(self, history_instance) -> int: return self._get_segment().id def is_exact_match(self, other: "Condition") -> bool: + """ + A condition is considered an exact match for another condition + with regard to the diffing logic, if all of `property`, `operator`, + and `value` exactly match. + """ return ( self.property == other.property and self.operator == other.operator @@ -588,6 +569,12 @@ def is_exact_match(self, other: "Condition") -> bool: ) def is_partial_match(self, other: "Condition") -> bool: + """ + A condition is considered a partial match for another condition + if both `property` values are none, and the operator matches + (for example, percentage split operator conditions), or + if the property exactly matches. + """ if self.property is None and other.property is None: return self.operator == other.operator From bb534a1731da6cd7e7a4bb6d9b5a76520b5ff408 Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Wed, 29 Jan 2025 15:27:39 +0000 Subject: [PATCH 5/5] Update count --- api/tests/unit/projects/test_unit_projects_views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/tests/unit/projects/test_unit_projects_views.py b/api/tests/unit/projects/test_unit_projects_views.py index 2c7243fb9125..7da3e75dd144 100644 --- a/api/tests/unit/projects/test_unit_projects_views.py +++ b/api/tests/unit/projects/test_unit_projects_views.py @@ -177,7 +177,7 @@ def test_can_list_project_permission(client: APIClient, project: Project) -> Non # Then assert response.status_code == status.HTTP_200_OK # Hard code how many permissions we expect there to be. - assert len(response.json()) == 9 + assert len(response.json()) == 10 returned_supported_permissions = [ permission["key"]