scope uniqueness constraints to affected objects - #10019
Conversation
…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>
github-actions
Bot
added
the
group/backend
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Optimizes uniqueness validation by scoping checks to only the nodes affected by data diffs, reducing unnecessary full scans. All changes are implementation-level; cubic reviewed and found no issues.
Re-trigger cubic
Merging this PR will improve performance by 30.77%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_base_schema_duplicate_CoreProposedChange |
2.2 ms | 1.5 ms | +49.4% |
| ⚡ | test_schemabranch_duplicate |
7.7 ms | 6.7 ms | +14.46% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ifc-2796-node-scope-uniqueness-2 (b7c6994) with stable (0ad9131)1
Footnotes
There was a problem hiding this comment.
All reported issues were addressed
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| self, candidate_schema: SchemaBranch, schema_diff_constraints: list[SchemaUpdateConstraintInfo] | ||
| ) -> MergeConstraintValidationResult: | ||
| determiner = ConstraintValidatorDeterminer(schema_branch=candidate_schema) | ||
| determiner = build_constraint_validator_determiner( |
There was a problem hiding this comment.
this should be built as part of construction, but the dependencies require schema_branch which is not ready until runtime. I think the right solution is to move schema_branch from dependency to runtime argument for all of these, but I'm going to do that separately
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>
There was a problem hiding this comment.
0 issues found across 14 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Scopes uniqueness validation to affected nodes per diff, with fallback to full scan for unsupported paths; a bounded, clearly beneficial performance optimization. Implementation verified by Cubic.
Re-trigger cubic
| return constraints | ||
|
|
||
|
|
||
| def build_constraint_validator_determiner( |
There was a problem hiding this comment.
only matter of taste but I like this pattern when we need this factory functions for collocation and less imports. Would it be possible here?
class ConstraintValidatorDeterminer
@classmethod
def from_(cls, ...) -> "ConstraintValidatorDeterminer":
....
return cls(....)
There was a problem hiding this comment.
@polmichel explained that this is intentional and we prefer this pattern to separate concerns
There was a problem hiding this comment.
My opinion is that we'd rather limit these class methods to prevent increasing class responsibilities. A small component or method can take care of the object instantiation.
There was a problem hiding this comment.
yes I agree with Pol here. constructing the class is not the responsibility of the class
| self, | ||
| db: InfrahubDatabase, | ||
| max_concurrent_execution: int = 5, | ||
| query_batch_size: int = 500, |
There was a problem hiding this comment.
I think a comment on how we decided to be 500, even if its a heurestic would be useful for the future
There was a problem hiding this comment.
small comment added
| def merge(self, *constraint_lists: list[SchemaUpdateConstraintInfo]) -> list[SchemaUpdateConstraintInfo]: | ||
| # Collapse the same constraint from multiple producers onto one entry. A constraint both | ||
| # broadened by a schema change (full population) and hit by a data change (node-scoped) must | ||
| # re-check every node, so a None node_uuids always wins; two node-scoped entries union. |
There was a problem hiding this comment.
should this be a docstring?
There was a problem hiding this comment.
yes it should. updated
|
|
||
| def _chunked(items: list[str], size: int) -> Iterator[list[str]]: | ||
| for start in range(0, len(items), size): | ||
| yield items[start : start + size] |
There was a problem hiding this comment.
There was a problem hiding this comment.
added consolidated chunks utility in 25fbe7f
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
polmichel
left a comment
polmichel
left a comment
There was a problem hiding this comment.
Great PR and feature. Nice use of dependency injection, which makes the code readable and maintainable. The tests aren't all integration tests, and in my opinion that's a sign the design is good.
A few optional comments.
| class NodeDiffFieldSummary: | ||
| """Which fields changed for a kind, and which nodes changed each one. | ||
|
|
||
| The per-field maps keep the correlation between a changed field and the nodes that changed it, | ||
| so a consumer can scope work to the nodes touching a specific field rather than every changed | ||
| node of the kind. ``attribute_names``/``relationship_names``/``node_uuids`` are derived views. | ||
| """ | ||
|
|
There was a problem hiding this comment.
This looks pretty useful, I'll keep this data structure in mind.
| node_uuids |= await self.dependent_resolver.resolve( | ||
| node_kind=schema.kind, | ||
| relationship_identifier=peer_change.relationship_identifier, | ||
| peer_uuids=sorted(peer_change.changed_peer_uuids), | ||
| ) |
There was a problem hiding this comment.
DependentResolver object usage is clearer now. Nice injection
| """Return the diffed kinds where `field_name` changed, on `schema` or a kind that inherits it. | ||
|
|
||
| An inherited field keeps its name on the implementing kind, so a generic-level constraint | ||
| is implicated when an implementation's copy of the field is what changed in the diff. A | ||
| generic knows its implementing kinds through `used_by`; the diff check then keeps only the | ||
| kinds actually present in the diff. |
There was a problem hiding this comment.
Not sure whether we should keep the last sentence since the used_by is an implementation detail, which should be documented on its own.
There was a problem hiding this comment.
good point. trimmed the last sentence
| def _compute_scope(self, schema: MainSchemaTypes) -> UniquenessScopeForKind: | ||
| """Compute why and how a data change implicates `schema`'s uniqueness. | ||
|
|
||
| Uniqueness spans single unique attributes and multi-field constraint groups. A group | ||
| element such as "owner__name" reads an attribute of a related peer, so a data change on | ||
| the peer kind can create a violation without any change to the constrained kind itself: | ||
| that case yields a cross-kind peer change. Directly changed nodes of the constrained kind | ||
| are collected as object uuids. | ||
| """ |
There was a problem hiding this comment.
This algorithm is really clean.
The method is a bit long though. From a readability perspective, would it be better to extract the two parts (unique_attributes / uniqueness_constraints) into private methods, each returning "UniquenessScopeFragment" objects? This method would then only assemble them and pass them to the UniquenessScopeForKind constructor. This would bring a better readability without having to know about the implementation details, and would give UniquenessScopeForKind a certain level immutability
Just an idea, feel free to ignore it or to do it differently.
There was a problem hiding this comment.
good suggestion. refactored and it is easier to read now
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Performance optimization and refactor: scoped uniqueness validation to affected nodes instead of full-population scans. No user-facing changes, no schema/API changes, and falls back to full scan when unsupported.
Re-trigger cubic
a67223e
into
ifc-2796-node-scope-uniqueness-full
3 participants
Why
Uniqueness validation scanned the entire population of a kind on every data change. Merging, rebasing, or proposing a change that touched a handful of nodes of a large kind still ran a population-wide scan — slow on big kinds (hundreds of thousands of nodes) and it shipped large duplicate result sets back to Python.
Goal: scope a data-triggered uniqueness check to only the nodes the diff actually affects, so the work is proportional to the size of the change rather than the size of the kind.
Non-goals: this does not change node-save-time uniqueness (
grouped_uniqueness), does not touch other constraint validators, and does not change the uniqueness error surfaced to users.Note: uniqueness constraints do not support attribute of relationships (e.g.
"device__name"). some parts of the code support this kind of uniqueness constraint, but not all of it and it is still blocked when creating/updating a schema.Planned follow-ups
MergeConstraintValidatorconstruction.schema_branchshould probably be an argument for this class and its dependents, not a dependencyPart of IFC-2796 (epic IFC-2706).
What changed
Behavioral
owner__name) fall back to full-population validation, which supports them; the targeted query does not. Again, this type of uniqueness constraint cannot be added to schemas currently.Implementation notes
node_uuidscarrier is threaded end to end:Nonemeans full scan, a list means validate exactly those nodes. It rides from the diff summary through the determiner, the constraint info, and the validator request into the checker.NodeDiffFieldSummary/NodeDiffIndexnow keep the correlation between each changed field and the nodes that changed it (per-field UUID maps), so scoping targets a specific field's nodes rather than every changed node of the kind.build_constraint_validator_determiner):UniquenessConstraintScoperdecides, per kind, whether uniqueness must run and which nodes it affects (None→ full scan); theConstraintValidatorDeterminerupdate threads the affected set through; constraint dedup/merge helpers collapse a node's uniqueness check into its generic's where the generic already covers it.UniquenessCheckernow runs the targeted query for a scoped set, and falls back to the full-population scan when a constraint reads a peer attribute.What stayed the same
grouped_uniqueness) are untouched.Suggested review order
core/diff/model/path.py,core/diff/query/field_summary.py,core/validators/node_diff_index.py— how per-field changed-node UUIDs are captured.core/validators/uniqueness/scope.py+core/validators/determiner.py— how the affected-node set (orNone) is decided.core/validators/uniqueness/checker.py— running the targeted query for a scoped set vs the full-population fallback.proposed_change/tasks.py,merge/constraints.py,branch/tasks.py— the three call sites that thread the affected set through.How to review
Focus on the scope decision (
UniquenessConstraintScoper._compute_scope) and the checker's targeted-vs-full-population branch (UniquenessChecker.check/_supports_targeted) — those govern correctness. Mechanical: theattribute_node_uuidsconversions across the test files.Impact & rollout
Checklist
Summary by cubic
Scope node-level uniqueness validation to only the nodes that changed each unique field (and their dependents), using diff-derived IDs and batched, index-backed queries. Full scans now run only when a constraint is added/broadened or when peer-attribute elements are present, cutting worst-case merge and schema-update time (IFC-2796).
Bug Fixes
Refactors
TargetedUniquenessValidationQuery, paging over changed nodes with one read-only session per change.UniquenessCheckerreads branch/schema from the validator request; dependency builder and m018 migration updated.node_uuidsthroughSchemaUpdateConstraintInfoand validator requests across proposed-change, merge, rebase, and migrations; merge/dedup constraints and standardize onConstraintIdentifier.NODE_UNIQUENESS_CONSTRAINTS_UPDATE.infrahub.utilities.chunks.chunkedand replace local helpers.UniquenessConstraintScoperfor clarity; trim docstrings.Written for commit b7c6994. Summary will update on new commits.