scope uniqueness constraints to specific objects - #10053
Open
ajtmccarty wants to merge 33 commits into
Open
Conversation
queries for node-scoped uniqueness constraints
…niquenessChecker Replace the per-node targeted uniqueness path with a single batched TargetedUniquenessValidationQuery per constraint group, paged over the changed nodes. Take branch and schema_branch from the validator request rather than the constructor, and open one read-only session per change instead of per query. Update the dependency builder and the m018 migration for the new signatures, and switch the node-scoped tests to exact violation-set assertions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The constraint result model gained a node_uuids field, so model_dump() now emits 'node_uuids': None. Update the two schema-validate tests whose hardcoded expected dicts predate the field. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The targeted validation path emitted attribute values with their native graph type, so an integer attribute rendered as value=8 while the full-population path rendered '8'. Stringify the value when building the data path so error messages are identical across both paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The determiner re-initializes its node-diff index on each get_constraints call, but the uniqueness scoper's per-kind scope cache was never cleared, so a reused determiner could return scopes computed from a previous diff. Add a reset() on the scoper and call it alongside index initialization. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The batched targeted query cannot read an attribute of a related peer (a constraint element such as owner__name) and raises on one. Detect such constraints and validate the whole population for that schema instead of scoping to the changed nodes, since the full-population path supports them. Removes the xfail markers on the two peer-attribute tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The diff field summary collapsed all changed node uuids per kind, so changing one node's unique field validated every changed node of that kind. Track uuids per changed field through NodeDiffFieldSummary, the diff query, NodeDiffIndex, and the uniqueness scoper, so a uniqueness check is scoped to only the nodes whose changed field participates in it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
scope uniqueness constraints to affected objects
github-actions
Bot
added
the
group/backend
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 48 files
Confidence score: 3/5
- In
backend/infrahub/core/diff/query/field_summary.py, the targeted uniqueness subqueries dropdiff_root, so validation can mix data across concurrent unmerged diffs and produce incorrect uniqueness outcomes (false conflicts or missed conflicts) for the selected change set — preserve/applydiff_rootin those targeted subqueries so checks stay scoped to the enriched diff. - In
backend/infrahub/proposed_change/tasks.py, relationship-attribute uniqueness still falls back to an unbounded full-population scan becauseUniquenessCheckerrejects targeted paths, which can both inflate runtime and validate against more data than intended during data-triggered updates — enable targeted validation for these paths (per IFC-2796) to keep checks bounded to affected node IDs.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/infrahub/proposed_change/tasks.py">
<violation number="1" location="backend/infrahub/proposed_change/tasks.py:627">
P2: Relationship-attribute uniqueness changes still run the unbounded full-population scan because `UniquenessChecker` rejects targeted validation for those paths. According to linked Jira issue IFC-2796, data-triggered uniqueness validation must be scoped to affected IDs; extend targeted validation for these paths or avoid this fallback.</violation>
</file>
<file name="backend/infrahub/core/diff/query/field_summary.py">
<violation number="1" location="backend/infrahub/core/diff/query/field_summary.py:59">
P1: According to linked Jira issue IFC-2796, targeted uniqueness checks must receive only affected node IDs from the selected enriched diff. These subqueries discard `diff_root`, so concurrent unmerged diffs of the same kind contribute UUIDs and can trigger unrelated validation (and restore work proportional to other diffs); scope each re-match through the selected root.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| AND a.action <> $unchanged_str | ||
| WITH DISTINCT a.name AS attr_name | ||
| RETURN collect(attr_name) AS attr_names | ||
| WITH a.name AS attr_name, collect(DISTINCT n.uuid) AS attr_node_uuids |
Contributor
There was a problem hiding this comment.
P1: According to linked Jira issue IFC-2796, targeted uniqueness checks must receive only affected node IDs from the selected enriched diff. These subqueries discard diff_root, so concurrent unmerged diffs of the same kind contribute UUIDs and can trigger unrelated validation (and restore work proportional to other diffs); scope each re-match through the selected root.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/core/diff/query/field_summary.py, line 59:
<comment>According to linked Jira issue IFC-2796, targeted uniqueness checks must receive only affected node IDs from the selected enriched diff. These subqueries discard `diff_root`, so concurrent unmerged diffs of the same kind contribute UUIDs and can trigger unrelated validation (and restore work proportional to other diffs); scope each re-match through the selected root.</comment>
<file context>
@@ -47,30 +56,43 @@ async def query_init(self, db: InfrahubDatabase, **kwargs: Any) -> None: # noqa
AND a.action <> $unchanged_str
- WITH DISTINCT a.name AS attr_name
- RETURN collect(attr_name) AS attr_names
+ WITH a.name AS attr_name, collect(DISTINCT n.uuid) AS attr_node_uuids
+ WHERE attr_name IS NOT NULL
+ RETURN collect({name: attr_name, node_uuids: attr_node_uuids}) AS attr_name_uuids
</file context>
| determiner = ConstraintValidatorDeterminer(schema_branch=schema) | ||
| return await determiner.get_constraints(node_diffs=list(node_diff_field_summary_map.values())) | ||
| async with db.start_session(read_only=True) as session_db: | ||
| determiner = build_constraint_validator_determiner(db=session_db, branch=branch, schema_branch=schema) |
Contributor
There was a problem hiding this comment.
P2: Relationship-attribute uniqueness changes still run the unbounded full-population scan because UniquenessChecker rejects targeted validation for those paths. According to linked Jira issue IFC-2796, data-triggered uniqueness validation must be scoped to affected IDs; extend targeted validation for these paths or avoid this fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/proposed_change/tasks.py, line 627:
<comment>Relationship-attribute uniqueness changes still run the unbounded full-population scan because `UniquenessChecker` rejects targeted validation for those paths. According to linked Jira issue IFC-2796, data-triggered uniqueness validation must be scoped to affected IDs; extend targeted validation for these paths or avoid this fallback.</comment>
<file context>
@@ -610,18 +612,20 @@ async def _get_proposed_change_schema_integrity_constraints(
- determiner = ConstraintValidatorDeterminer(schema_branch=schema)
- return await determiner.get_constraints(node_diffs=list(node_diff_field_summary_map.values()))
+ async with db.start_session(read_only=True) as session_db:
+ determiner = build_constraint_validator_determiner(db=session_db, branch=branch, schema_branch=schema)
+ return await determiner.get_constraints(node_diffs=list(node_diff_field_summary_map.values()))
</file context>
Contributor
Author
There was a problem hiding this comment.
using relationship-attribute uniqueness constraints is currently prevented when loading/updating a schema, so we do not need to support them at this point
Contributor
There was a problem hiding this comment.
1 issue found across 14 files (changes from recent commits).
Confidence score: 5/5
- In
backend/infrahub/core/query/__init__.py,QueryArrowBidirappears unused whileget_query_arrows()routes bidirectional behavior throughQueryArrowOutbound/QueryArrowInbound, which creates a maintenance risk by implying a code path that doesn’t exist—removeQueryArrowBidir(or wire it in intentionally) to keep query-arrow behavior unambiguous.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/infrahub/core/query/__init__.py">
<violation number="1" location="backend/infrahub/core/query/__init__.py:160">
P3: `QueryArrowBidir` is unused: bidirectional paths use `QueryArrowOutbound`/`QueryArrowInbound` in `get_query_arrows()`, and no other code imports or instantiates it. Removing it avoids maintaining a misleading unused representation.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
|
|
||
|
|
||
| @dataclass | ||
| class QueryArrowBidir(QueryArrow): |
Contributor
There was a problem hiding this comment.
P3: QueryArrowBidir is unused: bidirectional paths use QueryArrowOutbound/QueryArrowInbound in get_query_arrows(), and no other code imports or instantiates it. Removing it avoids maintaining a misleading unused representation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/core/query/__init__.py, line 160:
<comment>`QueryArrowBidir` is unused: bidirectional paths use `QueryArrowOutbound`/`QueryArrowInbound` in `get_query_arrows()`, and no other code imports or instantiates it. Removing it avoids maintaining a misleading unused representation.</comment>
<file context>
@@ -138,6 +138,36 @@ def __str__(self) -> str:
+
+
+@dataclass
+class QueryArrowBidir(QueryArrow):
+ start: str = "-"
+ end: str = "-"
</file context>
Contributor
Author
There was a problem hiding this comment.
feel incomplete not to include it
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
combines two previous reviewed PRs
Only these commits (the final 3) are unreviewed
Each of these is self-contained and can be reviewed on its own:
671b39c— reject a non-positivechunked()sizechunked()letsize=0surfacerange()'s own opaqueValueErrorand silently yielded nothing for a negative size. Adds an explicit guard and replaces the single test with rejection cases for zero/negative plus round-trip cases (empty, exact multiple, trailing partial, oversized size, tuple-in/tuple-out). Defensive only — every current caller passes a positive size.dbbcad4— moveget_query_arrows()fromRelationshipSchematoQueryMapping a relationship direction to Cypher arrows is a database concern, not a schema one.
QueryArrow/QueryArrowsand the mapping now live ininfrahub.core.queryasQuery.get_query_arrows(direction); all nine call sites were already insideQuerysubclasses, andQueryArrow*had no consumers outsiderelationship_schema.py. Pure refactor — the arrows rendered per direction are unchanged.abcd7e7— traverse the relationship in its own direction when resolving dependentsAffectedUniquenessDependentsQuerymatchedIS_RELATEDundirected, so for a kind whose relationship peer is its own kind (parent/childrensharing one identifier) resolving the dependents of a changed peer returned nodes on both sides, over-scoping validation. ThreadsRelationshipDirectionthrough the scoper and resolver so the path is traversed node→Relationship→peer, matching how the edges are created and how the sibling uniqueness queries already read them. Currently latent — a peer-attribute constraint element still falls back to full-population validation — but it matters as soon as targeted validation covers those paths. Adds self-referential coverage that fails against the undirected query.