-
Notifications
You must be signed in to change notification settings - Fork 56
fix: emit group.member_added only for the group named in the mutation (closes #10314) #10316
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: stable
Are you sure you want to change the base?
Changes from 6 commits
1b413c1
3fb4a7d
c425049
5890508
a32bf15
41b14eb
bc26b3d
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 |
|---|---|---|
|
|
@@ -11,7 +11,12 @@ | |
| from pydantic import ConfigDict, ValidationError, field_validator | ||
|
|
||
| from infrahub.computed_attribute.jinja2 import InfrahubJinja2Template | ||
| from infrahub.core.constants import HashableModelState, RelationshipCardinality, RelationshipKind | ||
| from infrahub.core.constants import ( | ||
| HashableModelState, | ||
| RelationshipCardinality, | ||
| RelationshipDirection, | ||
| RelationshipKind, | ||
| ) | ||
| from infrahub.core.models import HashableModel, HashableModelDiff | ||
|
|
||
| from .attribute_schema import AttributeSchema, get_attribute_schema_class_for_kind | ||
|
|
@@ -378,6 +383,26 @@ def get_relationships_by_identifier(self, id: str) -> list[RelationshipSchema]: | |
|
|
||
| return rels | ||
|
|
||
| def get_reverse_relationship(self, relationship: RelationshipSchema) -> RelationshipSchema | None: | ||
| """Return the relationship on this schema that traverses back along the provided relationship. | ||
|
|
||
| Sharing an identifier is not enough: two kinds can both declare the same identifier on the same | ||
| side, and a single kind can declare both sides of it. The peer must point the opposite way, and a | ||
| bidirectional relationship is never its own reverse because both sides are stored asymmetrically. | ||
|
|
||
| Returns None when this schema declares no relationship pointing back. | ||
| """ | ||
| expected_direction = relationship.direction.neighbor_direction | ||
|
|
||
| for candidate in self.get_relationships_by_identifier(id=relationship.get_identifier()): | ||
| if candidate.direction != expected_direction: | ||
| continue | ||
| if candidate.direction == RelationshipDirection.BIDIR and candidate.name == relationship.name: | ||
|
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 unify this if in one and also leave a code comment explaining the logic?
Contributor
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. Done in bc26b3d: one condition plus a comment on why the name is what separates an inherited same-name pair ( |
||
| continue | ||
| return candidate | ||
|
|
||
| return None | ||
|
|
||
| def get_relationships_of_kind(self, relationship_kinds: Iterable[RelationshipKind]) -> list[RelationshipSchema]: | ||
| return [r for r in self.relationships if r.kind in relationship_kinds] | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Adding a group as a member of another group no longer reports the added group as the one that gained a member, which previously made every repository import start a Generator run that failed. |
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.
P2: When two different kinds use the same name for a bidirectional relationship, this name-only guard drops the valid peer relationship. Compare schema/relationship identity so only a genuinely self-referential relationship is skipped.
Prompt for AI agents
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 mechanism is real but the conclusion is inverted for this codebase, so keeping the name comparison.
The reported failure is two different kinds using the same name:
CoreRepositoryGroup.membersandCoreGeneratorGroup.members, both inherited fromCoreGroupwith identifiergroup_member. Skipping that is the fix, not a regression.Comparing relationship identity cannot separate the two cases. Inherited relationships get
id = None(node_inheritance_handler.py:81) and, unlike attributes, carry nosource_*_id, so there is no token shared between the twomembers. Comparing owning kinds instead only skips a same-kind pair, which leaves the reported cross-kind case emitting the spurious event. Verified: swapping the guard for an identity comparison makes the replication test fail again.Your comment did expose a genuine gap, and thanks for it: the test used one kind on both sides, so it could not tell the two implementations apart.
41b14eb8enrols aCoreStandardGroupinto aTestTrackingGroupinstead, matching the reported scenario, and it now fails under the identity-based guard.On the residual risk: a same-name bidirectional pair across two kinds that is a genuine two-sided relationship would lose its peer-side changelog. No such pair exists in the core schema — every two-sided pair names its sides differently (
checks/validator,comments/thread,roles/permissions,ip_addresses/ip_prefix), and the one identifier shared by two same-named relationships (ippool__resource) is on sibling pool kinds that are never each other's peer.