IFC-1197: Fix CoreNumberPool not allowing upsert by solababs · Pull Request #9214 · opsmill/infrahub

solababs · 2026-05-11T13:32:37Z

Why

This PR changes the guard from a presence check to a value-diff check: the mutation now loads the existing node and only rejects the request when the incoming value genuinely differs from the stored one.

Closes #5636

How to test

# Run the five new tests
uv run pytest backend/tests/component/graphql/resource_manager/test_resource_manager.py \
  -v -k "upsert or update_with_unchanged or node_change or node_attribute"

uv run pytest backend/tests/component/graphql/resource_manager/ -v

Checklist

Tests added/updated
Changelog entry added (changelog/5636.fixed.md)
External docs updated (if user-facing or ops-facing change)
Internal .md docs updated (internal knowledge and AI code tools knowledge)
I have reviewed AI generated content

cubic-dev-ai

No issues found across 3 files

Confidence score: 5/5

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

_{Shadow auto-approve: would require human review. This change modifies the core mutation guard logic for CoreNumberPool upsert, which involves loading existing data and comparing values, and could impact production behavior if bugs exist.}

codspeed-hq · 2026-05-11T14:03:53Z

Merging this PR will not alter performance

✅ 12 untouched benchmarks

_{Comparing fix-corenumberpool-upsert-ifc-1197 (66f02ce) with stable (2f720ae)¹}

No successful run was found on stable (74d5b24) during the generation of this report, so 2f720ae was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

ajtmccarty · 2026-05-11T19:49:09Z

+        new_node_value = data.get("node") and data.get("node").value
+        new_node_attr_value = data.get("node_attribute") and data.get("node_attribute").value
+        if new_node_value or new_node_attr_value:
+            existing = await NodeManager.find_object(


should this method be able to accept node and assume that it is the latest version of this CoreNumberPool? I don't think that is possible right now, but if a graphql mutation class inherits from CoreNumberPool in the future then it would be

ajtmccarty · 2026-05-11T19:50:36Z

+                raise ValidationError(input_value="The fields 'node' or 'node_attribute' can't be changed.")

        async with graphql_context.db.start_transaction() as dbt:
            number_pool, result = await super().mutate_update(


it looks like exisiting, if it is retrieved, should be passed in to the node parameter here so that the parent class doesn't also need to retrieve the same object from the database. existing could probably also be called node to go with the current pattern.

now it looks like existing and node are unnecessarily swapping places. is it possible to just use node all the way through? you might have to do find_object(..., kind="CoreNumberPool") for the typing to be right

ajtmccarty · 2026-05-11T19:51:11Z

+            )
+            if new_node_value and new_node_value != existing.node.value:  # type: ignore[attr-defined]
+                raise ValidationError(input_value="The fields 'node' or 'node_attribute' can't be changed.")
+            if new_node_attr_value and new_node_attr_value != existing.node_attribute.value:  # type: ignore[attr-defined]


could you use get_attribute() here to remove the typing exception?

ajtmccarty · 2026-05-11T19:54:02Z

+    )
+    assert not upsert_mutable.errors
+    assert upsert_mutable.data
+    assert upsert_mutable.data["CoreNumberPoolUpsert"]["object"]["id"] == pool_id


the test should assert that the end_range is correctly updated in the response and that the end range is correct on the object retrieved from the database

ajtmccarty · 2026-05-11T19:55:01Z

+
+async def test_number_pool_update_with_unchanged_node_fields(
+    db: InfrahubDatabase, default_branch: Branch, register_core_models_schema: SchemaBranch
+) -> None:


this looks like it tests the same thing as the test above, but maybe I am missing something

ajtmccarty · 2026-05-11T19:56:21Z

    )
+
+
+async def test_number_pool_upsert_identical_fields(


all these tests could be combined into class so that the schema only has to be loaded once and the data only has to be loaded when necessary

ajtmccarty · 2026-05-12T17:27:12Z

+                raise ValidationError(input_value="The fields 'node' or 'node_attribute' can't be changed.")

        async with graphql_context.db.start_transaction() as dbt:
            number_pool, result = await super().mutate_update(


now it looks like existing and node are unnecessarily swapping places. is it possible to just use node all the way through? you might have to do find_object(..., kind="CoreNumberPool") for the typing to be right

ajtmccarty · 2026-05-12T17:29:58Z

+
+        pool = await NodeManager.get_one(id=pool_id, db=db, branch=default_branch_scope_class)
+        assert pool is not None
+        assert pool.get_attribute("end_range").value == 30  # type: ignore[union-attr]


is the type ignore really required here? I would think value would have a type of Any

ajtmccarty · 2026-05-12T17:30:53Z

+            },
+        )
+        assert not update_same_values.errors
+        assert update_same_values.data


should confirm the expected value is returned and that the update actually persists on a retrieved instance of the object

ajtmccarty · 2026-05-13T14:21:50Z

+            if new_node_value and new_node_value != node.get_attribute("node").value:  # type: ignore[union-attr]
+                raise ValidationError(input_value="The fields 'node' or 'node_attribute' can't be changed.")
+            if new_node_attr_value and new_node_attr_value != node.get_attribute("node_attribute").value:  # type: ignore[union-attr]


I think you can remove these type: ignores

cubic-dev-ai

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

_{Shadow auto-approve: would require human review. This change modifies core business logic in a GraphQL mutation by altering the validation guard from a simple presence check to a value-diff check, which has a non-trivial impact on data integrity and behavior, so it requires human review.

Re-trigger cubic}

IFC-1197: Fix CoreNumberPool not allowing upsert

0655552

github-actions Bot added the group/backend Issue related to the backend (API Server, Git Agent) label May 11, 2026

cubic-dev-ai Bot reviewed May 11, 2026

View reviewed changes

Merge branch 'stable' into fix-corenumberpool-upsert-ifc-1197

21113af

solababs marked this pull request as ready for review May 11, 2026 13:57

solababs requested a review from a team as a code owner May 11, 2026 13:57

ajtmccarty reviewed May 11, 2026

View reviewed changes

solababs added 3 commits May 12, 2026 09:32

update tests

71076db

add missing update

c42e907

Merge branch 'stable' into fix-corenumberpool-upsert-ifc-1197

957b91e

ajtmccarty reviewed May 12, 2026

View reviewed changes

solababs added 2 commits May 13, 2026 08:33

update tests

87ce51e

Merge branch 'stable' into fix-corenumberpool-upsert-ifc-1197

8359023

ajtmccarty approved these changes May 13, 2026

View reviewed changes

solababs added 3 commits May 14, 2026 08:00

Merge branch 'stable' into fix-corenumberpool-upsert-ifc-1197

a9de8e4

Merge branch 'stable' into fix-corenumberpool-upsert-ifc-1197

b7741a7

remove type ignore's

66f02ce

cubic-dev-ai Bot reviewed May 18, 2026

View reviewed changes

solababs merged commit f17ba0b into stable May 18, 2026
57 checks passed

solababs deleted the fix-corenumberpool-upsert-ifc-1197 branch May 18, 2026 09:50

Uh oh!

Conversation

solababs commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

How to test

Checklist

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

codspeed-hq Bot commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

solababs commented May 11, 2026 •

edited

Loading

codspeed-hq Bot commented May 11, 2026 •

edited

Loading