Conversation
WalkthroughThis pull request refactors the branch merge operation to add automatic retry logic for transient database errors. The changes extract inline database query execution into dedicated private methods that wrap queries with retry transaction handling. Two new methods handle merging node diffs and property diffs respectively, each wrapped with retry-enabled transactions. The overall merge flow remains unchanged, with these new helpers invoked for each batch of changes. A changelog entry documents the addition of automatic retries to database queries during branch merge operations. 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
github-actions
bot
added
the
group/backend
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/infrahub/core/diff/merger/merger.py (2)
128-128:list[dict]is unparameterized — considerlist[dict[str, Any]].
list[dict]leaves the dictionary value type unspecified. Preferlist[dict[str, Any]](withAnyimported fromtyping) to satisfy the full type-hint requirement.As per coding guidelines: "Type hint all function parameters and returns" / "Use type hints for all function parameters and return values in Python".
Also applies to: 142-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/core/diff/merger/merger.py` at line 128, The parameter type hints use unparameterized dicts (e.g., in the method signature containing "self, at: Timestamp, node_diff_dicts: list[dict], migrated_kinds_id_map: dict[str, str]") which violates the guideline to fully type-hint; import Any from typing and change occurrences of list[dict] to list[dict[str, Any]] (and any other plain dict parameters/annotations on nearby lines such as the similar signature around line 142) so all dict value types are explicitly annotated while keeping dict keys as str.
93-115:DiffMergeMigratedKindsQueryandDiffMergeMetadataQueryare not covered by retry logic.The PR aims to add retry logic to all merge DB queries, but these two write queries remain unprotected. A transient error in either will still surface as an unhandled failure while the batched node/property merges now retry automatically. Consider extracting them into similarly decorated private methods (e.g.,
_merge_migrated_kinds,_update_metadata) for consistent resilience.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/core/diff/merger/merger.py` around lines 93 - 115, The calls to DiffMergeMigratedKindsQuery and DiffMergeMetadataQuery are not wrapped with the same retry protection as other DB write queries; extract the logic that builds and executes these queries into two private methods (e.g., _merge_migrated_kinds and _update_metadata) and apply the same retry/decorator used elsewhere for merge DB writes, then replace the inline await DiffMergeMigratedKindsQuery.init(...).execute(...) and the per-batch DiffMergeMetadataQuery.init(...).execute(...) calls with calls to these new decorated helpers so transient DB errors will be retried consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/infrahub/core/diff/merger/merger.py`:
- Around line 126-152: Add Google-style triple-quote docstrings to both private
methods _merge_nodes and _merge_properties: include a one-line description, an
Args section describing at, node_diff_dicts/property_diff_dicts, and
migrated_kinds_id_map (no type annotations), a Returns section stating None, and
a Raises section noting that exceptions from
DiffMergeQuery.init/DiffMergePropertiesQuery.init or .execute and the
retry_db_transaction wrapper may be propagated; place the docstrings immediately
below each def declaration for _merge_nodes and _merge_properties.
---
Nitpick comments:
In `@backend/infrahub/core/diff/merger/merger.py`:
- Line 128: The parameter type hints use unparameterized dicts (e.g., in the
method signature containing "self, at: Timestamp, node_diff_dicts: list[dict],
migrated_kinds_id_map: dict[str, str]") which violates the guideline to fully
type-hint; import Any from typing and change occurrences of list[dict] to
list[dict[str, Any]] (and any other plain dict parameters/annotations on nearby
lines such as the similar signature around line 142) so all dict value types are
explicitly annotated while keeping dict keys as str.
- Around line 93-115: The calls to DiffMergeMigratedKindsQuery and
DiffMergeMetadataQuery are not wrapped with the same retry protection as other
DB write queries; extract the logic that builds and executes these queries into
two private methods (e.g., _merge_migrated_kinds and _update_metadata) and apply
the same retry/decorator used elsewhere for merge DB writes, then replace the
inline await DiffMergeMigratedKindsQuery.init(...).execute(...) and the
per-batch DiffMergeMetadataQuery.init(...).execute(...) calls with calls to
these new decorated helpers so transient DB errors will be retried consistently.
| @retry_db_transaction(name="diff_merge_nodes") | ||
| async def _merge_nodes( | ||
| self, at: Timestamp, node_diff_dicts: list[dict], migrated_kinds_id_map: dict[str, str] | ||
| ) -> None: | ||
| merge_query = await DiffMergeQuery.init( | ||
| db=self.db, | ||
| branch=self.source_branch, | ||
| at=at, | ||
| target_branch=self.destination_branch, | ||
| node_diff_dicts=node_diff_dicts, | ||
| migrated_kinds_id_map=migrated_kinds_id_map, | ||
| ) | ||
| await merge_query.execute(db=self.db) | ||
|
|
||
| @retry_db_transaction(name="diff_merge_properties") | ||
| async def _merge_properties( | ||
| self, at: Timestamp, property_diff_dicts: list[dict], migrated_kinds_id_map: dict[str, str] | ||
| ) -> None: | ||
| merge_properties_query = await DiffMergePropertiesQuery.init( | ||
| db=self.db, | ||
| branch=self.source_branch, | ||
| at=at, | ||
| target_branch=self.destination_branch, | ||
| property_diff_dicts=property_diff_dicts, | ||
| migrated_kinds_id_map=migrated_kinds_id_map, | ||
| ) | ||
| await merge_properties_query.execute(db=self.db) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing docstrings on both new private methods — coding guideline violation.
Both _merge_nodes and _merge_properties have no docstrings. The coding guidelines require Google-style docstrings with at least a one-line description, an Args section, a Returns section, and a Raises section on all Python functions/methods.
📝 Proposed docstrings
`@retry_db_transaction`(name="diff_merge_nodes")
async def _merge_nodes(
self, at: Timestamp, node_diff_dicts: list[dict], migrated_kinds_id_map: dict[str, str]
) -> None:
+ """Merge a batch of node diffs into the destination branch with automatic retry.
+
+ Args:
+ at: Timestamp for the merge operation.
+ node_diff_dicts: Serialized node diff dictionaries to apply.
+ migrated_kinds_id_map: Mapping of migrated node UUIDs to their target database IDs.
+
+ Returns:
+ None
+
+ Raises:
+ DatabaseError: Re-raised after all retry attempts are exhausted.
+ """
merge_query = await DiffMergeQuery.init( `@retry_db_transaction`(name="diff_merge_properties")
async def _merge_properties(
self, at: Timestamp, property_diff_dicts: list[dict], migrated_kinds_id_map: dict[str, str]
) -> None:
+ """Merge a batch of property diffs into the destination branch with automatic retry.
+
+ Args:
+ at: Timestamp for the merge operation.
+ property_diff_dicts: Serialized property diff dictionaries to apply.
+ migrated_kinds_id_map: Mapping of migrated node UUIDs to their target database IDs.
+
+ Returns:
+ None
+
+ Raises:
+ DatabaseError: Re-raised after all retry attempts are exhausted.
+ """
merge_properties_query = await DiffMergePropertiesQuery.init(As per coding guidelines: "Always use triple quotes (""") for Python docstrings", "Follow Google-style docstring format in Python", "Include brief one-line description in Python docstrings", "Include Args/Parameters section in Python docstrings without typing information", "Include Returns section in Python docstrings", "Include Raises section in Python docstrings".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/infrahub/core/diff/merger/merger.py` around lines 126 - 152, Add
Google-style triple-quote docstrings to both private methods _merge_nodes and
_merge_properties: include a one-line description, an Args section describing
at, node_diff_dicts/property_diff_dicts, and migrated_kinds_id_map (no type
annotations), a Returns section stating None, and a Raises section noting that
exceptions from DiffMergeQuery.init/DiffMergePropertiesQuery.init or .execute
and the retry_db_transaction wrapper may be propagated; place the docstrings
immediately below each def declaration for _merge_nodes and _merge_properties.
1 participant
use existing
retry_db_transactionlogic on the database queries that merge a branch to catch transient database errors and retrySummary by CodeRabbit