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.

Sign up for GitHub

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(segment-crs): code reabability improvements to #4988 #5054

Merged
merged 5 commits into from
Jan 29, 2025
Merged

feat(segment-crs): code reabability improvements to #4988 #5054

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2ef4d36
First pass at wording / naming improvements
matthewelwell Jan 29, 2025
44432cf
Second pass at wording / naming improvements
matthewelwell Jan 29, 2025
4b0235e
Simplify rules method
matthewelwell Jan 29, 2025
af6df13
Wording updates for SegmentRule method, and add docstrings to new Con…
matthewelwell Jan 29, 2025
bb534a1
Update count
zachaysan Jan 29, 2025
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
242 changes: 105 additions & 137 deletions api/segments/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,106 +189,84 @@ 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:
break
for target_rule in target_segment.rules.all():
for target_sub_rule in target_rule.rules.all():

# 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 (
modified_rule in matched_rules
and modified_rule.version_of != current_rule
):
target_sub_rule_matched = False

for rule in rules:
if target_rule.type != rule.type:
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_sub_rule_matched:
# We are trying to match 1:1, so we only allow a single
# match for each target sub-rule.
break

# 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

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(
Expand Down Expand Up @@ -381,98 +359,64 @@ def get_segment(self):
return rule.segment

def assign_conditions_if_matching_rule( # noqa: C901
self, current_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 current_sub_rule.type != self.type:
if target_rule.type != self.type:
return False

current_conditions = current_sub_rule.conditions.all()
modified_conditions = self.conditions.all()
target_conditions = target_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

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).
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
# 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(
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
# 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(
target_condition
):
matched_conditions.add(condition)
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:
Expand Down Expand Up @@ -612,6 +556,30 @@ 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:
"""
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
and self.value == other.value
)

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

return self.property == other.property

def _get_segment(self) -> Segment:
"""
Temporarily cache the segment on the condition object to reduce number of queries.
Expand Down
2 changes: 1 addition & 1 deletion api/tests/unit/projects/test_unit_projects_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Expand Down
Loading
Loading