allow removing properties from attributes in merge logic

[ajtmccarty]

fixes #8583

the logic that serializes a diff and passes it to a cypher query during a merge was excluding removed properties of attributes if the attribute was not completely removed. for example, if the source of an attribute is deleted on a branch (like if the attribute stops using a profile), then this change would not make it back to the default branch during a merge

fix is a small change to make sure we send the property removals on to the merge query

adds two component tests covering this situation

• one ensures that the diff is correctly including the removed properties when calculated, which it already was
• one ensures that the removed source is merged into the default branch during a merge, which it was not

Summary by CodeRabbit

• Bug Fixes

  • Corrected handling of property removals during branch diffs/merges so value-type property removals are suppressed unless the whole attribute is removed.

• Tests

  • Added coverage for single-property updates (with and without source propagation) in diff/merge flows.
  • Added tests for source-property changes in diff calculations to validate updated/removed actions.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 362f148f-f14f-474a-9162-58783fa00bf8

📥 Commits

Reviewing files that changed from the base of the PR and between ca2e62a and 0b96388.

📒 Files selected for processing (1)

• changelog/8583.fixed

---

Walkthrough

Modified serializer logic to refine when attribute property removals are skipped: value-type property removals are suppressed unless the whole attribute is being removed, while other property removals may proceed. Added parameterized tests that exercise single-property updates and HAS_SOURCE property changes across branch diffs and merges (including cases where the source is cleared), duplicated in the diff. Added a changelog entry documenting the fix to attribute source handling during merges.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main code change: enabling property removal logic in the merge serializer for attributes that are not completely removed.
Description check	✅ Passed	The description clearly explains the problem, the fix, and includes test additions. It addresses the linked issue and provides context for the code changes.
Linked Issues check	✅ Passed	The PR fixes issue #8583 by enabling removal of attribute properties during merge operations. The serializer change [8601] allows non-value properties to be removed, and the tests verify both diff calculation [8601] and merge execution [8601] correctly handle source property removals.
Out of Scope Changes check	✅ Passed	All changes are scoped to the fix: serializer logic adjustment, a component test for diff calculation, and a component test for merge execution. No unrelated modifications detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

📝 Coding Plan

• Generate coding plan for human review comments

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Tip

CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.

Add a .trivyignore file to your project to customize which findings Trivy reports.

[codspeed-hq]

Merging this PR will not alter performance

✅ 12 untouched benchmarks

---

_{Comparing ajtm-03122026-single-prop-remove-merge-fix (0b96388) with stable (c396f66)¹}

Footnotes

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

[coderabbitai]

🧹 Nitpick comments (2)

backend/tests/component/core/diff/test_diff_and_merge.py (1)

670-681: Add a Google-style docstring to the new test method.

Line 670 introduces a new test function without the required docstring format.

Proposed update

 async def test_single_property_update(
     self,
     db: InfrahubDatabase,
     diff_repository: DiffRepository,
@@
     new_property: bool,
     expected_action: DiffAction,
 ) -> None:
+    """Validate merge behavior for single-property source updates/removals.
+
+    Args:
+        db: Database handle.
+        diff_repository: Diff repository fixture.
+        default_branch: Default branch fixture.
+        person_john_main: Source person fixture.
+        person_jane_main: Target person fixture.
+        person_alfred_main: Alternate source fixture.
+        car_camry_main: Car fixture with owner relationship.
+        new_property: Whether source should be set (True) or cleared (False).
+        expected_action: Expected diff action for the property scenario.
+
+    Returns:
+        None.
+    """

As per coding guidelines, **/*.py: “Always use triple quotes (""") for Python docstrings” and “Follow Google-style docstring format in Python”, including Args/Returns sections.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/tests/component/core/diff/test_diff_and_merge.py` around lines 670 -
681, The new test function test_single_property_update is missing a Google-style
triple-quoted docstring; add a top-level """...""" docstring above the async def
that briefly describes the test purpose, then include Google-style Args: listing
parameters like db, diff_repository, default_branch, person_john_main,
person_jane_main, person_alfred_main, car_camry_main, new_property,
expected_action with their types/meanings, and a Returns: section (e.g., None) —
ensure the docstring follows the project's Google-style format and uses triple
double-quotes.

backend/tests/component/core/diff/test_diff_calculator.py (1)

4543-4543: Prefer a full Google-style docstring for the new test.

Line 4543 uses a one-liner docstring; consider adding structured sections (Args, Returns, and if applicable Raises/Examples) to match repository standards.

As per coding guidelines, "**/*.py: Follow Google-style docstring format in Python; Include Args/Parameters section in Python docstrings without typing information; Include Returns section in Python docstrings; Include Raises section in Python docstrings; Include Examples section in Python docstrings."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@backend/tests/component/core/diff/test_diff_calculator.py` at line 4543,
Replace the one-line docstring ("Test that updating or removing a HAS_SOURCE
property from an attribute on a branch is captured in the diff.") with a full
Google-style docstring: add a short summary line, an Args section (describe any
parameters if the test takes fixtures or inputs; otherwise note none), a Returns
section (state None), and a Raises section if the test asserts exceptions (or
omit/explicitly state none), and optionally an Examples section; ensure the
docstring follows repository Google-style format and is placed directly above
the test function that contains the current one-liner.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@backend/tests/component/core/diff/test_diff_and_merge.py`:
- Around line 670-681: The new test function test_single_property_update is
missing a Google-style triple-quoted docstring; add a top-level """..."""
docstring above the async def that briefly describes the test purpose, then
include Google-style Args: listing parameters like db, diff_repository,
default_branch, person_john_main, person_jane_main, person_alfred_main,
car_camry_main, new_property, expected_action with their types/meanings, and a
Returns: section (e.g., None) — ensure the docstring follows the project's
Google-style format and uses triple double-quotes.

In `@backend/tests/component/core/diff/test_diff_calculator.py`:
- Line 4543: Replace the one-line docstring ("Test that updating or removing a
HAS_SOURCE property from an attribute on a branch is captured in the diff.")
with a full Google-style docstring: add a short summary line, an Args section
(describe any parameters if the test takes fixtures or inputs; otherwise note
none), a Returns section (state None), and a Raises section if the test asserts
exceptions (or omit/explicitly state none), and optionally an Examples section;
ensure the docstring follows repository Google-style format and is placed
directly above the test function that contains the current one-liner.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 243bcc9b-022c-422a-9d7c-b9809cb1046f

📥 Commits

Reviewing files that changed from the base of the PR and between 916f5b5 and ca2e62a.

📒 Files selected for processing (3)

• backend/infrahub/core/diff/merger/serializer.py
• backend/tests/component/core/diff/test_diff_and_merge.py
• backend/tests/component/core/diff/test_diff_calculator.py

[ogenstad]

Would be good with a news fragment for the resolved bug as well.