Skip to content

fix: enforce uniqueness on computed attributes (closes #7924)#9359

Open
polmichel wants to merge 15 commits into
stablefrom
ai-bug-pipeline-7924-computed-attr-uniqueness
Open

fix: enforce uniqueness on computed attributes (closes #7924)#9359
polmichel wants to merge 15 commits into
stablefrom
ai-bug-pipeline-7924-computed-attr-uniqueness

Conversation

@polmichel
Copy link
Copy Markdown
Contributor

@polmichel polmichel commented May 26, 2026

Why

Loading objects whose human-friendly id (or any unique: true field) is a computed Jinja2 attribute could create duplicates.

Schema used

---
version: "1.0"

nodes:
  - name: Stuff
    namespace: Random
    display_label: name__value
    human_friendly_id:
      - name__value
    attributes:
      - name: name
        kind: Text
        computed_attribute:
          kind: Jinja2
          jinja2_template: "{{ description__value | upper }}-STUFF"
        read_only: true
        unique: true
        optional: false
      - name: description
        kind: Text

Submitting four RandomStuff objects with the same description produced four nodes sharing the same computed name / HFID, because the uniqueness check silently skipped the computed field.

Root cause: the constraint widened its check filter only for optional unique attributes. A required computed attribute is never in the user-supplied data, and the macro populates its value only after the filter was captured, so the check was bypassed.

Closes #7924

What changed

  • A required computed unique attribute is now enforced on create. Duplicates are rejected before reaching the database.
  • The violation message names the input attributes that drive the computed value, e.g. Violates uniqueness constraint 'name' (computed from: model) (or owner.name for relationship-derived inputs), so the user knows which field to change rather than seeing an opaque reference to a read-only field.

What stayed the same

  • NodeAttributeUniquenessConstraint deliberately left alone: it is registered as a dependency but not wired into the production NodeConstraintRunner.

How to test

uv run pytest backend/tests/unit/core/node/constraints/test_uniqueness_violation_message.py -v
uv run pytest backend/tests/component/core/node/test_create_node_computed_uniqueness.py -v
uv run pytest backend/tests/component/core/constraint_validators/ -v

Reproduction from the issue: load the RandomStuff schema (computed unique name), then load four objects with identical description. Before: four duplicates created. After: rejected with Violates uniqueness constraint 'name' (computed from: description).

Follow-up: complementary SDK-side guard

This PR fixes enforcement at the server boundary, which is the correct authoritative place. A complementary improvement could live in the Python SDK / infrahubctl object load to catch the common case before anything is sent to the server.

Checklist

  • Tests added/updated
  • Changelog entry added
  • External docs updated (n/a — bug fix, no user-facing doc change)
  • Internal .md docs updated (n/a)
  • I have reviewed AI generated content

polmichel and others added 12 commits May 26, 2026 19:16
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Required computed attributes were never added to the uniqueness check
filter because the widening logic only covered optional attributes,
so duplicate Jinja2-derived values silently slipped through and could
produce duplicate HFIDs. Widen the filter to include computed unique
attributes in both grouped and attribute uniqueness checkers, and
report the input attributes (e.g. "computed from: model") in the
violation message so the user can act on the right field.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… edit

Add a third replication test exercising a schema where the computed unique
attribute is not the HFID, so the violation surfaces as ValidationError
(STANDARD) rather than HFIDViolatedError. Drop the earlier edit to
NodeAttributeUniquenessConstraint: that class has no production caller — the
NodeConstraintRunner only invokes the grouped checker — so widening its
filter had no runtime effect.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The replication tests drive create_node() directly — they exercise the
production create path, not the NodeGroupedUniquenessConstraint class in
isolation. Move them to backend/tests/component/core/node/ alongside
test_template_pool_allocation.py, which is the existing home for
create_node integration tests. Drop the now-redundant constraint-level
test that overlapped with the create-flow coverage.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ation

The previous implementation extracted input attribute names from Jinja2
variables with a naive split on '__', which dropped the peer attribute
when the variable referenced a relationship path (e.g. 'owner__name__value'
collapsed to 'owner'). Reuse the schema-path validation already used by
the macro processor so relationship-attribute inputs render as
'<relationship>.<attribute>' in the violation message.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extract the inline boolean expression into _should_implicitly_check_uniqueness
with a docstring that explains why optional or computed unique attributes
must be evaluated even when absent from the user-supplied filter list.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move the violation-message formatting (including the Jinja2 template
variable resolution that names input attributes for computed fields)
out of NodeGroupedUniquenessConstraint into a dedicated builder class.
The builder takes only a SchemaBranch and is therefore unit-testable
without a database. Inject it through the constraint constructor; the
dependency builder wires up the registry-bound SchemaBranch.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lder

Add tests/helpers/schema_builders.computed_jinja2_attr to remove the
repeated read-only/unique/Jinja2 computed-attribute boilerplate across the
computed-uniqueness fixtures, and convert those fixtures from dict form to
object form so both the unit and component trees share the builder.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the group/backend Issue related to the backend (API Server, Git Agent) label May 26, 2026
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 12 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Shadow auto-approve: would auto-approve. The change fixes a specific bug where computed unique attributes were not enforced for uniqueness, and the implementation is well-contained with targeted logic, good tests, and a dedicated message builder that does not affect other parts of the system.

Re-trigger cubic

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented May 26, 2026

Merging this PR will not alter performance

✅ 12 untouched benchmarks


Comparing ai-bug-pipeline-7924-computed-attr-uniqueness (43964cb) with stable (edaeba7)1

Open in CodSpeed

Footnotes

  1. No successful run was found on stable (969810f) during the generation of this report, so edaeba7 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

polmichel and others added 2 commits May 26, 2026 21:43
Decompose UniquenessViolationMessageBuilder._collect_computed_inputs so each
method owns one level: the orchestrator deduplicates across fields,
_inputs_for_field resolves one field's Jinja2 template, and _resolve_input_name
maps a single path to a user-facing name. Hoist the static path-type mask to a
module constant.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reshape the three fixtures around the minimal RandomStuff schema from the
original report and strip attributes, display labels, and the back-relationship
that the assertions never exercise.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
constraint = NodeGroupedUniquenessConstraint(
db=db,
branch=default_branch,
message_builder=UniquenessViolationMessageBuilder(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if a Noop class could be useful to avoid instantiating this class every time we instantiate NodeGroupedUniquenessConstraint or if this is over-engineered. Not sure of the answer, so I let the code in this state for this iteration.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 issues found across 2 files (changes from recent commits).

Shadow auto-approve: would auto-approve. This is a targeted bug fix that extends the uniqueness constraint check to computed attributes, preventing duplicate HFIDs; the logic change is minimal and well-encapsulated, the new message builder is isolated and testable, and comprehensive tests confirm correctness without altering critical...

Re-trigger cubic

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 issues found across 1 file (changes from recent commits).

Shadow auto-approve: would auto-approve. This PR fixes a well-scoped bug where computed unique attributes were not being enforced for uniqueness, with a small logic change and thorough test coverage, making it low-risk for production.

Re-trigger cubic

@polmichel polmichel marked this pull request as ready for review May 26, 2026 19:59
@polmichel polmichel requested a review from a team as a code owner May 26, 2026 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

group/backend Issue related to the backend (API Server, Git Agent)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: Unique constraint not applied on computed attribute leading to duplicated HFID

1 participant