IFC-1891: Update branch query filter path time condition #7595
Conversation
github-actions
bot
added
the
group/backend
Walkthrough
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
changelog/7338.fixed.md (1)
1-1: LGTM! Consider adding more context.The changelog entry is clear and follows the naming convention. Consider making it slightly more specific, for example: "Fixed branch query filter to prevent duplicate relationships after merge" to provide more context for readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/infrahub/core/branch/models.py(2 hunks)backend/tests/unit/core/test_branch_merge.py(1 hunks)changelog/7338.fixed.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{md,mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
Lint Markdown/MDX files with markdownlint using
.markdownlint.yaml
Files:
changelog/7338.fixed.md
backend/tests/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place backend tests in
backend/tests/
Files:
backend/tests/unit/core/test_branch_merge.py
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/tests/unit/core/test_branch_merge.pybackend/infrahub/core/branch/models.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpfulUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all Python function parameters and return values
Prefer asynchronous code in Python when feasible
Define asynchronous Python functions withasync def
Useawaitfor asynchronous calls in Python
Use Pydantic models instead of standard dataclasses
Use ruff and mypy for linting and type checking
**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Define asynchronous functions withasync def
Await asynchronous calls withawait
Use Pydantic models instead of standard dataclasses for data modeling
Use triple quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include docstring sections when applicable: one-line summary, optional details, Args (without types), Returns, Raises, Examples
Validate and lint Python with ruff and mypy
Files:
backend/tests/unit/core/test_branch_merge.pybackend/infrahub/core/branch/models.py
🧬 Code graph analysis (1)
backend/tests/unit/core/test_branch_merge.py (4)
backend/tests/unit/conftest.py (5)
person_tag_schema(1448-1485)tag_blue_main(1588-1593)tag_red_main(1597-1602)tag_black_main(1606-1611)person_jack_main(1615-1620)backend/infrahub/dependencies/registry.py (1)
get_component_registry(61-62)backend/infrahub/core/diff/coordinator.py (1)
update_branch_diff(119-162)backend/infrahub/core/diff/merger/merger.py (1)
DiffMerger(28-117)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: validate-generated-documentation
- GitHub Check: documentation
- GitHub Check: backend-docker-integration
- GitHub Check: backend-tests-integration
- GitHub Check: backend-tests-unit
- GitHub Check: backend-tests-functional
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
backend/tests/unit/core/test_branch_merge.py (1)
143-195: Excellent test coverage for the fix!This test comprehensively validates the fix for issue #7338 by:
- Setting up a cardinality-one relationship on main
- Creating a branch and updating the relationship
- Merging the branch to main
- Verifying that subsequent updates work correctly on both main and branch without the "Too many relationships" error
The test directly reproduces the failure scenario described in the issue and confirms the fix works as expected. The structure is clear and follows the existing test patterns in the file.
backend/infrahub/core/branch/models.py (1)
336-380: Verify the completeness of the timestamp inequality fix across related filtering methods.The change from
<=to<inget_query_filter_path()is applied only to that method (lines 359, 372, 375). However, a related methodget_query_filter_relationships()in the same file still uses<=at lines 323 and 326. Both methods filter relationships on timestamps and may be affected by the same issue.Issue #7338 (referenced in the review comment) cannot be verified in public documentation or release notes. Without understanding the exact scenario—whether this is specific to path-based queries or affects all timestamp-based relationship filtering—it's unclear whether the fix should also apply to
get_query_filter_relationships().Ensure the timestamp comparison semantics are consistent across all methods handling relationship time boundaries, or document why they intentionally differ.
CodSpeed Performance ReportMerging #7595 will not alter performanceComparing Summary
|
solababs
force-pushed
the
sb-20251106-ifc-1891
branch
from
d675591 to
dc924d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
changelog/7338.fixed.md (1)
1-1: Improve changelog entry clarity.The current message is overly technical and doesn't communicate the user-facing fix clearly. Based on the PR context, this resolves an issue where updating cardinality=one relationships on a branch after merge would fail. Consider a more descriptive entry:
Fixed an issue where updating cardinality=one relationships on a branch after merge would fail with a "Too many relationships" validation error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
changelog/7338.fixed.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{md,mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
Lint Markdown/MDX files with markdownlint using
.markdownlint.yaml
Files:
changelog/7338.fixed.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: E2E-testing-version-upgrade / From 1.3.0
- GitHub Check: backend-benchmark
- GitHub Check: E2E-testing-invoke-demo-start
- GitHub Check: E2E-testing-playwright
- GitHub Check: validate-generated-documentation
- GitHub Check: backend-docker-integration
- GitHub Check: documentation
- GitHub Check: backend-tests-functional
- GitHub Check: backend-tests-integration
- GitHub Check: backend-tests-unit
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
changelog/7338.fixed.md (1)
1-1: Configuration file.markdownlint.yamlnot found in repository.The repository references markdown linting configuration in CLAUDE.md and coding guidelines, but the
.markdownlint.yamlfile is missing from the codebase. Without this configuration, markdown linting compliance cannot be definitively verified forchangelog/7338.fixed.md. Verify whether this configuration file should be present in the repository and ensure it is included before enabling linting checks.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/infrahub/core/branch/models.py (2)
315-333: Document the new parameter in the docstring.The
include_exact_date_fromparameter is not documented in the method's docstring. Per coding guidelines, add an Args section documenting this parameter and explaining when to useTruevsFalse.Apply this diff to add documentation:
def get_query_filter_path( self, at: Optional[Union[Timestamp, str]] = None, is_isolated: bool = True, branch_agnostic: bool = False, variable_name: str = "r", params_prefix: str = "", include_exact_date_from: bool = False, ) -> tuple[str, dict]: """ Generate a CYPHER Query filter based on a path to query a part of the graph at a specific time and on a specific branch. + + Args: + at: The timestamp to query at. + is_isolated: Whether to treat the branch as isolated. + branch_agnostic: Whether to generate branch-agnostic filters. + variable_name: The variable name to use in the generated filter. + params_prefix: Prefix for parameter names. + include_exact_date_from: If True, uses <= for from comparison (includes exact timestamp); if False, uses < (excludes exact timestamp). Defaults to False. + + Returns: + A tuple of the filter string and parameters dictionary. Examples: >>> rels_filter, rels_params = self.branch.get_query_filter_path(at=self.at)
340-367: Update unit tests to validate the new<operator forget_query_filter_path()behavior.The code change from
<=to<is already applied and affects all 50+ callers globally. Whileget_query_filter_relationships()(which has passing tests) uses hardcoded<=, the reviewed methodget_query_filter_path()now defaults to<wheninclude_exact_date_from=False. No unit tests were found that specifically validate this new operator behavior forget_query_filter_path(). Add or update unit tests inbackend/tests/unit/core/test_branch.pyto confirm the<operator is produced correctly for all code paths (branch_agnostic and per-branch filtering), ensuring this intentional breaking change is verified before merge.
🧹 Nitpick comments (1)
backend/infrahub/core/branch/models.py (1)
338-338: Consider explicit conditional for operator assignment.While the f-string construction works correctly, an explicit conditional would be more readable:
- from_condition_operator = f"<{'=' if include_exact_date_from else ''}" + from_condition_operator = "<=" if include_exact_date_from else "<"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/infrahub/core/branch/models.py(3 hunks)backend/infrahub/core/query/attribute.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/core/query/attribute.pybackend/infrahub/core/branch/models.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpfulUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all Python function parameters and return values
Prefer asynchronous code in Python when feasible
Define asynchronous Python functions withasync def
Useawaitfor asynchronous calls in Python
Use Pydantic models instead of standard dataclasses
Use ruff and mypy for linting and type checking
**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Define asynchronous functions withasync def
Await asynchronous calls withawait
Use Pydantic models instead of standard dataclasses for data modeling
Use triple quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include docstring sections when applicable: one-line summary, optional details, Args (without types), Returns, Raises, Examples
Validate and lint Python with ruff and mypy
Files:
backend/infrahub/core/query/attribute.pybackend/infrahub/core/branch/models.py
🧬 Code graph analysis (1)
backend/infrahub/core/query/attribute.py (1)
backend/infrahub/core/branch/models.py (1)
get_query_filter_path(315-367)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: E2E-testing-version-upgrade / From 1.3.0
- GitHub Check: backend-benchmark
- GitHub Check: E2E-testing-invoke-demo-start
- GitHub Check: E2E-testing-playwright
- GitHub Check: validate-generated-documentation
- GitHub Check: backend-docker-integration
- GitHub Check: backend-tests-unit
- GitHub Check: backend-tests-integration
- GitHub Check: documentation
- GitHub Check: backend-tests-functional
- GitHub Check: Cloudflare Pages
| self.params["at"] = at.to_string() | ||
|
|
||
| rels_filter, rels_params = self.branch.get_query_filter_path(at=at.to_string()) | ||
| rels_filter, rels_params = self.branch.get_query_filter_path(at=at.to_string(), include_exact_date_from=True) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Clarify why AttributeGetQuery requires exact-date-from inclusion.
The explicit use of include_exact_date_from=True preserves the <= operator for attribute queries while the fix for issue #7338 changes the default to < for relationship queries. Add a comment explaining why attribute queries require different temporal filtering semantics than relationship queries.
Apply this diff to document the reasoning:
- rels_filter, rels_params = self.branch.get_query_filter_path(at=at.to_string(), include_exact_date_from=True)
+ # Attribute queries include exact branched_from timestamp (<=) to ensure attributes
+ # created at branch point are visible, unlike relationships which use < to avoid
+ # cardinality violations (see issue #7338)
+ rels_filter, rels_params = self.branch.get_query_filter_path(at=at.to_string(), include_exact_date_from=True)🤖 Prompt for AI Agents
In backend/infrahub/core/query/attribute.py around line 180, add an inline
comment where include_exact_date_from=True is passed to get_query_filter_path
that explains attribute queries intentionally preserve the "<=" operator (i.e.,
include exact date_from) because attribute values effective exactly at the query
timestamp must be visible to attribute lookups, whereas the related fix for
issue #7338 changed relationship queries to use "<" by default to avoid
including relationships that only become effective at the boundary; document
this difference and the reason for using include_exact_date_from=True here.
ajtmccarty
left a comment
ajtmccarty
left a comment
There was a problem hiding this comment.
added a couple comments explaining my commit and a suggestion for the changelog
I made some changes to how we instantiate AttributeQuery and AttributeGetQuery so that they default to using the current timestamp instead of the timestamp of the attribute being queried. This timestamp issue was causing the AttributeGetQuery to fail when retrieving attributes and properties to delete
| self.at = Timestamp(at) | ||
| else: | ||
| self.at = self.attr.at | ||
| self.at = Timestamp() |
There was a problem hiding this comment.
I don't think it makes sense to default to the timestamp of the attribute in the query. defaulting to the current timestamp makes more sense
| delete_at = Timestamp(at) | ||
|
|
||
| query = await AttributeGetQuery.init(db=db, attr=self) | ||
| query = await AttributeGetQuery.init(db=db, attr=self, at=delete_at) |
There was a problem hiding this comment.
we want to get the attributes at the moment that we plan to delete them
changelog/7338.fixed.md
Outdated
| @@ -0,0 +1 @@ | |||
| Fixed filter based on path relationship condition. | |||
There was a problem hiding this comment.
I don't think this means anything to someone unfamiliar with the code base.
I'd say something more like
Fixed bug that prevented retrieving cardinality-one relationships on a branch that was already merged and included changes to the relationship. This bug would be visible to the user as errors that look like
ValidationError: Too many relationships, max 1 at field_name
…rahub into sb-20251106-ifc-1891
Fixes #7338
Summary by CodeRabbit
Bug Fixes
Tests
Documentation