Add GraphQL metadata sort options#7904
Conversation
WalkthroughAdds ordering end-to-end: new enums Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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
ogenstad
force-pushed
the
pog-graphql-sort-IFC-1941
branch
from
e289668 to
05a5f4a
Compare
CodSpeed Performance ReportMerging #7904 will not alter performanceComparing Summary
|
ogenstad
force-pushed
the
pog-graphql-sort-IFC-1941
branch
2 times, most recently
from
bcc5b8f to
b4c6be3
Compare
ogenstad
force-pushed
the
pog-graphql-sort-IFC-1941
branch
4 times, most recently
from
f3abb27 to
3474e58
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
backend/infrahub/graphql/types/metadata.py (1)
13-15: Add description for thedisablefield.The
disablefield on line 14 lacks adescriptionparameter, while thenode_metadatafield has one. For consistency and API discoverability, all GraphQL input fields should include descriptions explaining their purpose and behavior.Apply this diff to add a description:
class OrderInput(InputObjectType): - disable = Boolean(required=False) + disable = Boolean(required=False, description="Disable ordering to preserve insertion order") node_metadata = Field(InfrahubNodeMetadataOrder, required=False, description="Order settings for branch metadata")(Note: Adjust the description text based on the actual intended behavior of the
disablefield.)backend/tests/unit/graphql/queries/test_branch.py (1)
543-750: LGTM!The ordering tests are comprehensive and well-structured:
created_attests verify both ASC and DESC ordering with appropriate assertionsupdated_attests cleverly manipulate branch update times to control the ordering, then filter results to avoid test interference- All tests include clear docstrings and validate both timestamp order and expected node positions
Consider using
pytest.mark.parametrizeto reduce duplication between the four ordering tests:@pytest.mark.parametrize( "field,direction,branch_prefix,expected_first", [ ("created_at", "ASC", "alpha", "main"), ("created_at", "DESC", "delta", "zeta-branch"), # ... etc ] ) async def test_order_by_metadata(self, db, default_branch, service, session_admin, field, direction, branch_prefix, expected_first): # Unified test implementationHowever, the current explicit approach is clear and maintainable, so this refactoring is purely optional.
schema/schema.graphql (1)
9788-9798: Consider improving the enum docstring.The docstring
"An enumeration."is generic boilerplate and doesn't add value. A more descriptive docstring would improve API documentation.Additionally, the field description says "Order settings for branch metadata" but the type
InfrahubNodeMetadataOrdersuggests this is for general node metadata ordering. Consider aligning the description with the actual scope of the functionality.-"""An enumeration.""" +"""Specifies the sort direction for ordering query results.""" enum OrderDirection { ASC DESC } input OrderInput { disable: Boolean - """Order settings for branch metadata""" + """Order settings for node metadata (e.g., created_at, updated_at)""" node_metadata: InfrahubNodeMetadataOrder = null }backend/infrahub/core/branch/models.py (1)
176-196: Consider addingnode_orderingtoget_list_countfor API consistency.The
get_listmethod now acceptsnode_ordering, butget_list_countdoes not. While ordering may not affect count results, the caller inbranch.py(line 81) passesnode_orderingtoget_list_count. This parameter is currently absorbed by**kwargsand ignored, which could cause confusion about the API contract.Since the PR notes that dynamic options are "non-functional placeholders," this may be intentional for now—but consider documenting or aligning the signatures for maintainability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/infrahub/constants/enums.py(1 hunks)backend/infrahub/core/branch/models.py(2 hunks)backend/infrahub/core/node/standard.py(4 hunks)backend/infrahub/core/query/standard_node.py(4 hunks)backend/infrahub/graphql/manager.py(1 hunks)backend/infrahub/graphql/models.py(1 hunks)backend/infrahub/graphql/queries/branch.py(4 hunks)backend/infrahub/graphql/resolvers/ipam.py(4 hunks)backend/infrahub/graphql/resolvers/resolver.py(3 hunks)backend/infrahub/graphql/types/enums.py(2 hunks)backend/infrahub/graphql/types/metadata.py(1 hunks)backend/tests/unit/graphql/queries/test_branch.py(1 hunks)schema/schema.graphql(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Always use triple quotes (""") for Python docstrings
Follow Google-style docstring format for Python docstrings
Include brief one-line description in Python docstrings when applicable
Include detailed description in Python docstrings when applicable
Include Args/Parameters section without typing in Python docstrings when applicable
Include Returns section in Python docstrings when applicable
Include Raises section in Python docstrings when applicable
Include Examples section in Python docstrings when applicable
**/*.py: Use type hints for all function parameters and return values in Python
Use Async whenever possible in Python
Useasync deffor asynchronous functions in Python
Useawaitfor asynchronous calls in Python
Use Pydantic models for dataclasses in Python
Use ruff and mypy for type checking and code validation in PythonUse ruff and mypy to validate and lint Python files
Files:
backend/infrahub/core/query/standard_node.pybackend/infrahub/graphql/resolvers/resolver.pybackend/infrahub/constants/enums.pybackend/infrahub/core/node/standard.pybackend/infrahub/graphql/types/metadata.pybackend/infrahub/graphql/queries/branch.pybackend/infrahub/graphql/resolvers/ipam.pybackend/infrahub/graphql/models.pybackend/tests/unit/graphql/queries/test_branch.pybackend/infrahub/core/branch/models.pybackend/infrahub/graphql/manager.pybackend/infrahub/graphql/types/enums.py
backend/infrahub/**/*.py
📄 CodeRabbit inference engine (backend/AGENTS.md)
backend/infrahub/**/*.py: Use async/await for all I/O operations to maintain async-first architecture
Type hint all function parameters and returns in Python code
Use Pydantic models for defining data structures instead of plain dictionaries
Use Query class pattern (extending infrahub.core.query.Query) for all database operations instead of unparameterized Cypher queries
Use Google-style docstrings with Args, Returns, and Raises sections for all functions
Use snake_case for function and variable names
Use PascalCase for class names
Use UPPER_SNAKE_CASE for constant definitions
Do not use unparameterized Cypher queries; always use parameterized queries to prevent injection
Do not block the event loop with synchronous I/O operations
Files:
backend/infrahub/core/query/standard_node.pybackend/infrahub/graphql/resolvers/resolver.pybackend/infrahub/constants/enums.pybackend/infrahub/core/node/standard.pybackend/infrahub/graphql/types/metadata.pybackend/infrahub/graphql/queries/branch.pybackend/infrahub/graphql/resolvers/ipam.pybackend/infrahub/graphql/models.pybackend/infrahub/core/branch/models.pybackend/infrahub/graphql/manager.pybackend/infrahub/graphql/types/enums.py
backend/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use type hints for all Python code in the backend
Files:
backend/infrahub/core/query/standard_node.pybackend/infrahub/graphql/resolvers/resolver.pybackend/infrahub/constants/enums.pybackend/infrahub/core/node/standard.pybackend/infrahub/graphql/types/metadata.pybackend/infrahub/graphql/queries/branch.pybackend/infrahub/graphql/resolvers/ipam.pybackend/infrahub/graphql/models.pybackend/tests/unit/graphql/queries/test_branch.pybackend/infrahub/core/branch/models.pybackend/infrahub/graphql/manager.pybackend/infrahub/graphql/types/enums.py
backend/tests/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasksName test files as
test_<module>.pyand mirror source structure in tests directory
Files:
backend/tests/unit/graphql/queries/test_branch.py
🧠 Learnings (2)
📚 Learning: 2025-12-01T22:16:13.680Z
Learnt from: CR
Repo: opsmill/infrahub PR: 0
File: backend/AGENTS.md:0-0
Timestamp: 2025-12-01T22:16:13.680Z
Learning: Applies to backend/infrahub/**/*.py : Use Query class pattern (extending infrahub.core.query.Query) for all database operations instead of unparameterized Cypher queries
Applied to files:
backend/infrahub/core/query/standard_node.py
📚 Learning: 2025-10-30T12:43:45.663Z
Learnt from: ogenstad
Repo: opsmill/infrahub PR: 7447
File: schema/schema.graphql:324-329
Timestamp: 2025-10-30T12:43:45.663Z
Learning: In the GraphQL schema (schema/schema.graphql), the BranchStatus enum includes NEED_UPGRADE_REBASE. Branches with NEED_UPGRADE_REBASE status must support conflict resolution capabilities.
Applied to files:
backend/infrahub/graphql/queries/branch.py
🧬 Code graph analysis (9)
backend/infrahub/core/query/standard_node.py (2)
backend/infrahub/constants/enums.py (1)
OrderByField(9-12)backend/infrahub/core/node/standard.py (2)
StandardNode(83-349)StandardNodeOrdering(37-74)
backend/infrahub/graphql/resolvers/resolver.py (1)
backend/infrahub/graphql/models.py (2)
OrderModel(19-36)from_input(25-32)
backend/infrahub/graphql/queries/branch.py (2)
backend/infrahub/core/node/standard.py (2)
StandardNodeOrdering(37-74)from_order_input(42-74)backend/infrahub/graphql/types/metadata.py (1)
OrderInput(13-15)
backend/infrahub/graphql/resolvers/ipam.py (1)
backend/infrahub/graphql/models.py (2)
OrderModel(19-36)from_input(25-32)
backend/infrahub/graphql/models.py (2)
backend/infrahub/types.py (1)
Any(312-319)backend/infrahub/constants/enums.py (1)
OrderDirection(4-6)
backend/tests/unit/graphql/queries/test_branch.py (2)
backend/infrahub/graphql/initialization.py (1)
prepare_graphql_params(89-136)backend/infrahub/core/node/standard.py (1)
save(205-211)
backend/infrahub/core/branch/models.py (2)
backend/infrahub/core/node/standard.py (2)
StandardNode(83-349)StandardNodeOrdering(37-74)backend/infrahub/core/query/branch.py (1)
BranchNodeGetListQuery(153-160)
backend/infrahub/graphql/manager.py (1)
backend/infrahub/graphql/types/metadata.py (1)
OrderInput(13-15)
backend/infrahub/graphql/types/enums.py (1)
backend/infrahub/constants/enums.py (1)
OrderDirection(4-6)
🔇 Additional comments (23)
backend/infrahub/graphql/types/enums.py (1)
3-3: LGTM!The import and enum alias follow the existing pattern in the file and correctly expose the OrderDirection enum to the GraphQL layer.
Also applies to: 18-18
backend/infrahub/core/query/standard_node.py (2)
3-3: LGTM!The new imports properly support the ordering functionality with type-safe enum handling and exhaustiveness checking.
Also applies to: 5-5, 12-12
184-192: Verify ORDER BY direction parameterization approach.The
direction.valueis directly interpolated into the Cypher query string (lines 188, 190). While this is safe because the value comes from theOrderDirectionenum, the coding guidelines state: "Do not use unparameterized Cypher queries; always use parameterized queries to prevent injection."Can the ORDER BY direction be parameterized in Cypher, or is string interpolation of enum values the accepted pattern for ORDER BY clauses in this codebase?
Based on learnings, the Query class pattern should be used for all database operations with parameterized queries.
backend/infrahub/graphql/types/metadata.py (1)
8-10: LGTM!The
InfrahubNodeMetadataOrderinput type is well-structured with clear descriptions for both ordering fields.backend/infrahub/constants/enums.py (1)
1-12: LGTM!The enum definitions are clean and follow Python best practices. The string values are appropriately cased for their respective contexts.
backend/infrahub/graphql/manager.py (1)
24-24: LGTM!Centralizing the
OrderInputdefinition by importing it frommetadata.pyinstead of defining it locally is good practice. This ensures consistency across the GraphQL layer.Also applies to: 1001-1001
backend/infrahub/graphql/resolvers/resolver.py (1)
149-149: LGTM!The refactoring properly separates GraphQL input handling (raw
dict) from the internal model (OrderModel). TheOrderModel.from_inputmethod handles conversion and validation, which is a clean separation of concerns.Also applies to: 159-160, 208-208
backend/tests/unit/graphql/queries/test_branch.py (2)
484-542: LGTM!The test properly validates ascending ordering by
created_at. It creates branches in a known sequence, queries with ordering, and verifies both timestamp ordering and expected positions.
752-778: LGTM!The validation test properly ensures that specifying both
created_atandupdated_atraises an error, confirming the constraint is enforced as intended by the PR objectives.schema/schema.graphql (2)
7428-7433: LGTM!The input type is well-structured with clear docstrings. The optional nature of both fields with
nulldefaults is appropriate, and the mutual exclusivity constraint (not allowing bothcreated_atandupdated_attogether) is noted as being enforced at the resolver layer.
11137-11145: LGTM!The
orderparameter is cleanly integrated into theInfrahubBranchquery with a clear description. The optional nature of the parameter is appropriate for backward compatibility.backend/infrahub/graphql/resolvers/ipam.py (2)
19-19: LGTM!Import of
OrderModelis correctly placed and aligns with the new ordering pattern.
312-312: LGTM!The signature change from
OrderModel | Nonetodict[str, Any] | Nonecorrectly reflects how GraphQL resolver arguments are received. The conversion viaOrderModel.from_input(input_data=order)properly handles the dictionary-to-model transformation and validation, and the resultingorder_model(which may beNone) is correctly passed toNodeManager.query.Also applies to: 325-325, 404-404
backend/infrahub/core/node/standard.py (1)
340-345: LGTM!The
node_orderingparameter is properly typed and defaulted. The inline default ensures aStandardNodeOrdering()is always passed to the query, avoiding None checks downstream.backend/infrahub/graphql/queries/branch.py (4)
5-12: LGTM!Imports are properly organized with
Argumentadded for the GraphQL field definition, and bothStandardNodeOrderingandOrderInputare correctly imported from their respective modules.
47-47: LGTM!The ordering flow is well-implemented: the
orderparameter is properly typed, converted once viaStandardNodeOrdering.from_order_input(order), and passed through toInfrahubBranch.get_list. This ensures validation happens early and the ordering is consistently applied.Also applies to: 54-55, 72-72
102-106: LGTM!The
OrderInputargument is properly configured withrequired=Falseand includes a clear description for API consumers.
76-81: Thenode_orderingparameter is properly accepted and used in the method chain.The concern is unfounded.
StandardNodeGetListQuery.__init__explicitly declaresnode_ordering: StandardNodeOrderingas a named parameter (not via**kwargs), stores it asself.node_ordering, and uses it in thequery_init()method to determine the ORDER BY clause of the Cypher query. The parameter flows correctly through the entire call chain:InfrahubBranchType.get_list_count→Branch.get_list_count→BranchNodeGetListQuery→StandardNodeGetListQuery, where it is properly handled.Likely an incorrect or invalid review comment.
backend/infrahub/core/branch/models.py (2)
12-12: LGTM!Import of
StandardNodeOrderingalongsideStandardNodeis correctly placed.
159-169: LGTM!The
node_orderingparameter is properly integrated intoget_listwith correct type hints and default handling. The ordering is correctly propagated toBranchNodeGetListQuery.init.backend/infrahub/graphql/models.py (3)
1-9: LGTM!Imports are well-organized. Using
TYPE_CHECKINGforOrderDirectionavoids a circular import while maintaining type safety through string annotations withfrom __future__ import annotations.
12-15: LGTM!
NodeMetaOrderdataclass cleanly captures the ordering options with appropriate optional fields.
34-36: Validation logic is correct.The mutual exclusivity check for
created_atandupdated_atis properly implemented, aligning with the PR requirement that combining these fields should raise an error.
| @dataclass | ||
| class StandardNodeOrdering: | ||
| order_by: OrderByField = field(default=OrderByField.ID) | ||
| direction: OrderDirection = field(default=OrderDirection.ASC) | ||
|
|
||
| @classmethod | ||
| def from_order_input(cls, order: OrderInput | None = None) -> StandardNodeOrdering: | ||
| """Create a StandardNodeOrdering from an OrderInput. | ||
|
|
||
| Args: | ||
| order: Optional ordering specification from GraphQL input. | ||
|
|
||
| Returns: | ||
| StandardNodeOrdering with the specified field and direction, or defaults to ID with no direction. | ||
|
|
||
| Raises: | ||
| ValidationError: If both created_at and updated_at are specified. | ||
| """ | ||
| if order is None: | ||
| return cls(order_by=OrderByField.ID) | ||
|
|
||
| node_metadata = getattr(order, "node_metadata", None) | ||
|
|
||
| if node_metadata is None: | ||
| return cls(order_by=OrderByField.ID) | ||
|
|
||
| created_at = getattr(node_metadata, "created_at", None) | ||
| updated_at = getattr(node_metadata, "updated_at", None) | ||
|
|
||
| if created_at and updated_at: | ||
| raise ValidationError("Only one of 'created_at' or 'updated_at' can be specified for ordering.") | ||
|
|
||
| if created_at: | ||
| return cls(order_by=OrderByField.CREATED_AT, direction=OrderDirection(created_at.value)) | ||
|
|
||
| if updated_at: | ||
| return cls(order_by=OrderByField.UPDATED_AT, direction=OrderDirection(updated_at.value)) | ||
|
|
||
| return cls(order_by=OrderByField.ID) |
There was a problem hiding this comment.
Docstring inaccuracy: direction defaults to ASC, not "no direction".
Line 49 states "defaults to ID with no direction" but the class default on line 39 is direction: OrderDirection = field(default=OrderDirection.ASC). Update the docstring to reflect the actual default behavior.
Returns:
- StandardNodeOrdering with the specified field and direction, or defaults to ID with no direction.
+ StandardNodeOrdering with the specified field and direction, or defaults to ID ascending.🤖 Prompt for AI Agents
In backend/infrahub/core/node/standard.py around lines 36 to 74, the
from_order_input docstring incorrectly says the default is "ID with no
direction" but the class default sets direction to OrderDirection.ASC; update
the docstring so the Returns section accurately states the default is ID with
direction ASC (or "ascending") and adjust any related phrasing (e.g., remove "no
direction") to reflect that the ordering defaults to ID ascending.
| @classmethod | ||
| def from_input(cls, input_data: dict[str, Any] | None) -> OrderModel | None: | ||
| """Convert the dictionary type input data from GraphQL into an OrderModel instance.""" | ||
| if not input_data: | ||
| return None | ||
|
|
||
| order_model = cls(**input_data) | ||
| order_model.validate() | ||
| return order_model |
There was a problem hiding this comment.
Nested node_metadata dict won't be deserialized to NodeMetaOrder.
The from_input method uses cls(**input_data) directly, but if input_data contains a nested dict for node_metadata (e.g., {"node_metadata": {"created_at": "ASC"}}), it will remain a dict rather than being converted to a NodeMetaOrder instance. This will cause validate() to fail when accessing self.node_metadata.created_at since dict objects don't have those attributes.
Consider deserializing the nested structure:
@classmethod
def from_input(cls, input_data: dict[str, Any] | None) -> OrderModel | None:
"""Convert the dictionary type input data from GraphQL into an OrderModel instance."""
if not input_data:
return None
- order_model = cls(**input_data)
+ node_metadata = None
+ if meta_data := input_data.get("node_metadata"):
+ node_metadata = NodeMetaOrder(**meta_data)
+ order_model = cls(disable=input_data.get("disable"), node_metadata=node_metadata)
order_model.validate()
return order_modelCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/infrahub/graphql/models.py around lines 24-32, the from_input method
currently passes nested node_metadata dicts straight into cls(**input_data)
which leaves node_metadata as a plain dict and breaks validate(); detect if
input_data.get("node_metadata") is a dict and convert it to a NodeMetaOrder
instance before constructing the OrderModel (e.g., replace the dict with
NodeMetaOrder.from_input(...) or NodeMetaOrder(**...) depending on available
factory), then call cls(**input_data) and validate as before.
| node_metadata = getattr(order, "node_metadata", None) | ||
|
|
||
| if node_metadata is None: | ||
| return cls(order_by=OrderByField.ID) | ||
|
|
||
| created_at = getattr(node_metadata, "created_at", None) | ||
| updated_at = getattr(node_metadata, "updated_at", None) |
There was a problem hiding this comment.
do we really need to user getattr here?
There was a problem hiding this comment.
Unfortunately yes we do at this point. It's to make the type checker happy.
class OrderInput(InputObjectType):
disable = Boolean(required=False)
node_metadata = Field(InfrahubNodeMetadataOrder, required=False, description="Order settings for branch metadata")The node_metadata attribute is defined as a Field and not what it actually is InfrahubNodeMetadataOrder. In some places we just ignore this. We might have to update these definitions in the future to have a cleaner internal API. I agree it's not pretty. :(
| case OrderByField.UPDATED_AT: | ||
| self.order_by = [f"n.updated_at {self.node_ordering.direction.value}"] | ||
| case _: | ||
| assert_never(self.node_ordering.order_by) |
There was a problem hiding this comment.
interesting. learned something new today
There was a problem hiding this comment.
It's something that I noticed that they have in Rust and I thought it was a nice thing to check against so when looking for an alternative in Python I came across this. I asked Lucas to add it in one other place as well but I think it's something that we can probably use in more places moving forward.
| self.order_by = [f"{db.get_id_function_name()}(n)"] | ||
| match self.node_ordering.order_by: | ||
| case OrderByField.ID: | ||
| self.order_by = [f"{db.get_id_function_name()}(n)"] |
There was a problem hiding this comment.
ID usually means Node.uuid for us, not the database's elementId
There was a problem hiding this comment.
I will leave this as is for now since I didn't want to change the existing default behaviour. But we can backtrack later and switch this to the uuid instead. What do you think?
There was a problem hiding this comment.
I missed it was already the default behavior
this is fine then
| session_admin: AccountSession, | ||
| ) -> None: | ||
| """Test that branches can be ordered by created_at in descending order.""" | ||
| branch_names = ["delta-branch", "epsilon-branch", "zeta-branch"] |
| assert timestamps == sorted(timestamps, reverse=True), "Branches should be ordered by created_at descending" | ||
|
|
||
| # The last branch created should be first | ||
| assert edges[0]["node"]["name"]["value"] == "zeta-branch" |
There was a problem hiding this comment.
might as well check that all three are here in the expected order
| assert result.errors is None | ||
|
|
||
| # Update branches in a specific order: iota first, then theta, then eta | ||
| # This means eta will have the latest updated_at |
There was a problem hiding this comment.
sometimes these comments feel like they are directed at an idiot
| # The first should be main (default branch, created first) | ||
| assert edges[0]["node"]["name"]["value"] == "main" | ||
|
|
||
| async def test_order_by_created_at_descending( |
There was a problem hiding this comment.
feels like this could be combined with the ascending version of the test
| result_branch_names = [e["node"]["name"]["value"] for e in test_edges] | ||
| assert result_branch_names == update_order, "Updated branches should appear in update order (ascending)" | ||
|
|
||
| async def test_order_by_updated_at_descending( |
There was a problem hiding this comment.
also feels like it can be combined
ogenstad
force-pushed
the
pog-graphql-sort-IFC-1941
branch
from
3474e58 to
342b8fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
backend/infrahub/graphql/models.py (1)
24-32: Nestednode_metadatadict won't be deserialized toNodeMetaOrder.The
from_inputmethod usescls(**input_data)directly, but ifinput_datacontains{"node_metadata": {"created_at": "ASC"}}, the nested dict remains a dict rather than being converted to aNodeMetaOrderinstance. This will causevalidate()to fail when accessingself.node_metadata.created_at.🔎 Suggested fix:
@classmethod def from_input(cls, input_data: dict[str, Any] | None) -> OrderModel | None: """Convert the dictionary type input data from GraphQL into an OrderModel instance.""" if not input_data: return None + node_metadata = None + if meta_data := input_data.get("node_metadata"): + node_metadata = NodeMetaOrder(**meta_data) + order_model = cls(disable=input_data.get("disable"), node_metadata=node_metadata) - order_model = cls(**input_data) order_model.validate() return order_modelbackend/tests/unit/graphql/queries/test_branch.py (2)
541-601: Consider combining with the ascending test for reduced duplication.The ascending and descending tests share similar structure and could be parameterized or combined into a single test that verifies both directions. This would reduce code duplication and maintenance burden.
670-740: LGTM! Proper test isolation maintained.The test correctly creates, updates, and filters branches to verify descending order. The isolation pattern matches the ascending test.
Note: A previous reviewer suggested combining the ascending/descending tests, but the current separate structure provides clarity.
🧹 Nitpick comments (2)
backend/tests/unit/graphql/queries/test_branch.py (1)
741-767: LGTM! Validates the mutual exclusion constraint.The test correctly verifies that specifying both
created_atandupdated_atraises an error. The error message check is appropriately flexible.Optional: Make error validation more specific
If the error message format is stable, consider checking for a more specific message:
assert result.errors is not None assert len(result.errors) > 0 - assert "created_at" in str(result.errors[0]) or "updated_at" in str(result.errors[0]) + error_message = str(result.errors[0]).lower() + assert "created_at" in error_message and "updated_at" in error_message, \ + "Error should mention both conflicting fields"This would ensure users get a clear error indicating the conflict between both fields.
schema/schema.graphql (1)
9788-9798: LGTM with minor documentation note.The
OrderDirectionenum is well-defined. TheOrderInputtype correctly integrates the newnode_metadatafield.One minor observation: the doc comment says "Order settings for branch metadata" but the field/type naming (
node_metadata,InfrahubNodeMetadataOrder) suggests more general node metadata ordering. This is fine if branches are the only current use case, but consider updating the comment to "Order settings for node metadata" if this input will be reused for other node types per the PR objectives.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/infrahub/constants/enums.py(1 hunks)backend/infrahub/core/branch/models.py(2 hunks)backend/infrahub/core/node/standard.py(3 hunks)backend/infrahub/core/query/standard_node.py(4 hunks)backend/infrahub/graphql/manager.py(1 hunks)backend/infrahub/graphql/models.py(1 hunks)backend/infrahub/graphql/queries/branch.py(4 hunks)backend/infrahub/graphql/resolvers/ipam.py(4 hunks)backend/infrahub/graphql/resolvers/resolver.py(3 hunks)backend/infrahub/graphql/types/enums.py(2 hunks)backend/infrahub/graphql/types/metadata.py(1 hunks)backend/tests/unit/graphql/queries/test_branch.py(1 hunks)schema/schema.graphql(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/infrahub/graphql/types/enums.py
- backend/infrahub/constants/enums.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Always use triple quotes (""") for Python docstrings
Follow Google-style docstring format for Python docstrings
Include brief one-line description in Python docstrings when applicable
Include detailed description in Python docstrings when applicable
Include Args/Parameters section without typing in Python docstrings when applicable
Include Returns section in Python docstrings when applicable
Include Raises section in Python docstrings when applicable
Include Examples section in Python docstrings when applicable
**/*.py: Use type hints for all function parameters and return values in Python
Use Async whenever possible in Python
Useasync deffor asynchronous functions in Python
Useawaitfor asynchronous calls in Python
Use Pydantic models for dataclasses in Python
Use ruff and mypy for type checking and code validation in PythonUse ruff and mypy to validate and lint Python files
Files:
backend/tests/unit/graphql/queries/test_branch.pybackend/infrahub/graphql/queries/branch.pybackend/infrahub/graphql/resolvers/resolver.pybackend/infrahub/core/query/standard_node.pybackend/infrahub/graphql/manager.pybackend/infrahub/core/node/standard.pybackend/infrahub/graphql/models.pybackend/infrahub/graphql/types/metadata.pybackend/infrahub/graphql/resolvers/ipam.pybackend/infrahub/core/branch/models.py
backend/tests/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasksName test files as
test_<module>.pyand mirror source structure in tests directory
Files:
backend/tests/unit/graphql/queries/test_branch.py
backend/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use type hints for all Python code in the backend
Files:
backend/tests/unit/graphql/queries/test_branch.pybackend/infrahub/graphql/queries/branch.pybackend/infrahub/graphql/resolvers/resolver.pybackend/infrahub/core/query/standard_node.pybackend/infrahub/graphql/manager.pybackend/infrahub/core/node/standard.pybackend/infrahub/graphql/models.pybackend/infrahub/graphql/types/metadata.pybackend/infrahub/graphql/resolvers/ipam.pybackend/infrahub/core/branch/models.py
backend/infrahub/**/*.py
📄 CodeRabbit inference engine (backend/AGENTS.md)
backend/infrahub/**/*.py: Use async/await for all I/O operations to maintain async-first architecture
Type hint all function parameters and returns in Python code
Use Pydantic models for defining data structures instead of plain dictionaries
Use Query class pattern (extending infrahub.core.query.Query) for all database operations instead of unparameterized Cypher queries
Use Google-style docstrings with Args, Returns, and Raises sections for all functions
Use snake_case for function and variable names
Use PascalCase for class names
Use UPPER_SNAKE_CASE for constant definitions
Do not use unparameterized Cypher queries; always use parameterized queries to prevent injection
Do not block the event loop with synchronous I/O operations
Files:
backend/infrahub/graphql/queries/branch.pybackend/infrahub/graphql/resolvers/resolver.pybackend/infrahub/core/query/standard_node.pybackend/infrahub/graphql/manager.pybackend/infrahub/core/node/standard.pybackend/infrahub/graphql/models.pybackend/infrahub/graphql/types/metadata.pybackend/infrahub/graphql/resolvers/ipam.pybackend/infrahub/core/branch/models.py
🧠 Learnings (2)
📚 Learning: 2025-10-30T12:43:45.663Z
Learnt from: ogenstad
Repo: opsmill/infrahub PR: 7447
File: schema/schema.graphql:324-329
Timestamp: 2025-10-30T12:43:45.663Z
Learning: In the GraphQL schema (schema/schema.graphql), the BranchStatus enum includes NEED_UPGRADE_REBASE. Branches with NEED_UPGRADE_REBASE status must support conflict resolution capabilities.
Applied to files:
backend/infrahub/graphql/queries/branch.py
📚 Learning: 2025-12-01T22:16:13.680Z
Learnt from: CR
Repo: opsmill/infrahub PR: 0
File: backend/AGENTS.md:0-0
Timestamp: 2025-12-01T22:16:13.680Z
Learning: Applies to backend/infrahub/**/*.py : Use Query class pattern (extending infrahub.core.query.Query) for all database operations instead of unparameterized Cypher queries
Applied to files:
backend/infrahub/core/query/standard_node.pybackend/infrahub/core/node/standard.py
🧬 Code graph analysis (7)
backend/infrahub/graphql/resolvers/resolver.py (1)
backend/infrahub/graphql/models.py (2)
OrderModel(19-36)from_input(25-32)
backend/infrahub/core/query/standard_node.py (2)
backend/infrahub/constants/enums.py (1)
OrderByField(9-12)backend/infrahub/core/node/standard.py (2)
StandardNode(47-315)StandardNodeOrdering(36-38)
backend/infrahub/graphql/manager.py (1)
backend/infrahub/graphql/types/metadata.py (1)
OrderInput(13-15)
backend/infrahub/core/node/standard.py (2)
backend/infrahub/constants/enums.py (2)
OrderByField(9-12)OrderDirection(4-6)backend/infrahub/core/query/standard_node.py (1)
StandardNodeGetListQuery(133-192)
backend/infrahub/graphql/models.py (1)
backend/infrahub/constants/enums.py (1)
OrderDirection(4-6)
backend/infrahub/graphql/resolvers/ipam.py (1)
backend/infrahub/graphql/models.py (2)
OrderModel(19-36)from_input(25-32)
backend/infrahub/core/branch/models.py (1)
backend/infrahub/core/node/standard.py (2)
StandardNode(47-315)StandardNodeOrdering(36-38)
⏰ 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). (9)
- GitHub Check: E2E-testing-version-upgrade / From 1.3.0
- GitHub Check: E2E-testing-playwright
- GitHub Check: backend-benchmark
- GitHub Check: validate-generated-documentation
- GitHub Check: backend-docker-integration
- GitHub Check: backend-tests-functional
- GitHub Check: backend-tests-integration
- GitHub Check: backend-tests-unit
- GitHub Check: Cloudflare Pages
🔇 Additional comments (18)
backend/infrahub/graphql/models.py (2)
12-15: LGTM!The
NodeMetaOrderdataclass is clean and correctly defines the optional ordering fields with proper type hints.
34-36: LGTM!The validation logic correctly enforces mutual exclusivity between
created_atandupdated_atordering, as specified in the PR objectives.backend/infrahub/graphql/manager.py (2)
24-24: LGTM!Good refactoring to import
OrderInputfrom the centralized location ininfrahub.graphql.types.metadatarather than defining it locally. This reduces duplication and maintains consistency across the GraphQL layer.
1001-1002: LGTM!The
OrderInputis correctly added to the filters dictionary, enabling ordering capabilities for all dynamically generated node queries.backend/infrahub/graphql/types/metadata.py (1)
8-15: LGTM!The
InfrahubNodeMetadataOrderandOrderInputGraphQL input types are well-structured with appropriate field descriptions. The structure aligns correctly with the correspondingNodeMetaOrderandOrderModeldataclasses in the models layer.backend/infrahub/graphql/resolvers/resolver.py (2)
149-160: LGTM!The resolver correctly converts the raw
dictinput from GraphQL into anOrderModelinstance using the factory method, then passes it downstream toNodeManager.query. This provides proper separation between GraphQL input parsing and domain model usage.
38-40: LGTM!Direct instantiation of
OrderModel(disable=True)is appropriate here since this is not coming from GraphQL input and explicitly disables ordering for single-result account lookups.backend/infrahub/core/node/standard.py (2)
35-38: LGTM!The
StandardNodeOrderingdataclass is well-designed with sensible defaults (OrderByField.IDandOrderDirection.ASC). It provides a clean abstraction for ordering configuration that's consumed by the query layer.
299-312: LGTM!The
get_listmethod correctly accepts an optionalnode_orderingparameter and defaults toStandardNodeOrdering()whenNoneis provided. This ensures the query layer always receives a valid ordering configuration. The implementation follows the Query class pattern as per coding guidelines.backend/tests/unit/graphql/queries/test_branch.py (2)
484-540: LGTM! Well-structured test for ascending order.The test correctly creates branches sequentially, verifies timestamp ordering, and checks the expected first result.
602-669: LGTM! Excellent test isolation.This test properly creates its own branches, updates them in a controlled order, and filters results to only verify the test data. The filtering approach at lines 661-662 ensures isolation from other branches.
schema/schema.graphql (2)
7428-7433: LGTM!The
InfrahubNodeMetadataOrderinput type is well-structured with appropriate documentation. The mutual exclusion constraint betweencreated_atandupdated_at(as noted in the PR) is correctly handled at the resolver level, since GraphQL schema cannot express such constraints natively.
11137-11145: LGTM!The
orderparameter is correctly added to theInfrahubBranchquery with appropriate documentation. The parameter is optional (as expected), and the integration with pagination parameters (limit,offset) follows standard GraphQL patterns.backend/infrahub/core/branch/models.py (1)
12-12: LGTM! Ordering support properly threaded through Branch.get_list.The changes correctly mirror the pattern established in
StandardNode.get_list, including the import, parameter addition with proper type hints, defaulting logic, and threading through to the query layer.Also applies to: 159-174
backend/infrahub/graphql/resolvers/ipam.py (1)
19-19: LGTM! Proper conversion from GraphQL input to OrderModel.The changes correctly implement the pattern of accepting flexible dict input from GraphQL and converting it to a strongly-typed
OrderModelviafrom_input. The runtime import ofOrderModelis necessary for this conversion, and validation errors will propagate appropriately.Also applies to: 312-312, 325-325, 404-404
backend/infrahub/graphql/queries/branch.py (2)
19-46: Well-documented conversion function with appropriate validation.The function correctly converts GraphQL
OrderInputtoStandardNodeOrderingwith proper validation, type handling, and complete Google-style docstring per coding guidelines.Note: The mutual-exclusion validation for
created_atandupdated_atis also present inOrderModel.validate()(backend/infrahub/graphql/models.py). This duplication provides defense-in-depth, which is acceptable, though the two validation paths operate on different types (OrderInput vs OrderModel).
78-113: LGTM! Ordering properly wired through resolver and GraphQL field.The changes correctly:
- Accept
order: OrderInput | Nonein the resolver- Convert it to
StandardNodeOrderingvia the helper function- Thread
node_orderingthrough to both list and count operations- Expose the
orderargument in the GraphQL field definition with proper descriptionPassing
node_orderingtoget_list_countis acceptable for API consistency even if the count operation doesn't use ordering.Also applies to: 133-137
backend/infrahub/core/query/standard_node.py (1)
141-151: LGTM! Ordering parameter properly stored on query instance.The
node_orderingparameter is correctly added as a required parameter (callers handle defaulting) and stored on the instance for use inquery_init.
| match self.node_ordering.order_by: | ||
| case OrderByField.ID: | ||
| self.order_by = [f"{db.get_id_function_name()}(n)"] | ||
| case OrderByField.CREATED_AT: | ||
| self.order_by = [f"n.created_at {self.node_ordering.direction.value}"] | ||
| case OrderByField.UPDATED_AT: | ||
| self.order_by = [f"n.updated_at {self.node_ordering.direction.value}"] | ||
| case _: | ||
| assert_never(self.node_ordering.order_by) |
There was a problem hiding this comment.
Critical: ID ordering doesn't respect the direction parameter.
Line 186 constructs the order_by clause for OrderByField.ID but omits the direction:
self.order_by = [f"{db.get_id_function_name()}(n)"]This means ID ordering will always use the database's default sort direction, ignoring self.node_ordering.direction. The CREATED_AT and UPDATED_AT cases correctly include the direction.
🔎 Apply this diff to include direction for ID ordering:
match self.node_ordering.order_by:
case OrderByField.ID:
- self.order_by = [f"{db.get_id_function_name()}(n)"]
+ self.order_by = [f"{db.get_id_function_name()}(n) {self.node_ordering.direction.value}"]
case OrderByField.CREATED_AT:
self.order_by = [f"n.created_at {self.node_ordering.direction.value}"]
case OrderByField.UPDATED_AT:
self.order_by = [f"n.updated_at {self.node_ordering.direction.value}"]🤖 Prompt for AI Agents
In backend/infrahub/core/query/standard_node.py around lines 184 to 192, the
OrderByField.ID branch builds the order_by clause without appending the sort
direction so ID ordering ignores self.node_ordering.direction; change that
branch to include the direction the same way as CREATED_AT/UPDATED_AT (e.g.,
append f" {self.node_ordering.direction.value}" after the id expression) so the
generated order_by respects ascending/descending; keep the existing pattern and
formatting used for the other cases.
342b8fd to
625612d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
backend/tests/unit/graphql/queries/test_branch.py (1)
597-600: Fix test isolation: remove assertion on branch from another test.Line 600 asserts that
edges[3]contains "gamma-branch", which is only created intest_order_by_created_at_ascending(line 492), creating a dependency on test execution order. This issue was previously flagged in past review comments.🔎 Fix by filtering to test-created branches (pattern from updated_at tests):
+ # Filter to only branches created in this test + test_branch_names = set(branch_names) + test_edges = [e for e in edges if e["node"]["name"]["value"] in test_branch_names] + + result_branch_names = [e["node"]["name"]["value"] for e in test_edges] - assert edges[0]["node"]["name"]["value"] == "zeta-branch" - assert edges[1]["node"]["name"]["value"] == "epsilon-branch" - assert edges[2]["node"]["name"]["value"] == "delta-branch" - assert edges[3]["node"]["name"]["value"] == "gamma-branch" + assert result_branch_names == ["zeta-branch", "epsilon-branch", "delta-branch"]backend/infrahub/graphql/models.py (1)
24-32: Nestednode_metadatadict won't be deserialized toNodeMetaOrder.When
input_datacontains{"node_metadata": {"created_at": "ASC"}}, thecls(**input_data)call assigns the raw dict tonode_metadatainstead of aNodeMetaOrderinstance. Thevalidate()method will then fail when accessingself.node_metadata.created_atsince dict objects don't have that attribute.🔎 Apply this diff to properly deserialize the nested structure:
@classmethod def from_input(cls, input_data: dict[str, Any] | None) -> OrderModel | None: """Convert the dictionary type input data from GraphQL into an OrderModel instance.""" if not input_data: return None - order_model = cls(**input_data) + node_metadata = None + if meta_data := input_data.get("node_metadata"): + node_metadata = NodeMetaOrder(**meta_data) + order_model = cls(disable=input_data.get("disable"), node_metadata=node_metadata) order_model.validate() return order_modelbackend/infrahub/core/query/standard_node.py (1)
184-192: ID ordering doesn't respect the direction parameter.Line 186 omits the direction for
OrderByField.IDordering, whileCREATED_ATandUPDATED_ATcorrectly include it. This means ID ordering always uses the database's default sort direction.🔎 Apply this diff to include direction for ID ordering:
match self.node_ordering.order_by: case OrderByField.ID: - self.order_by = [f"{db.get_id_function_name()}(n)"] + self.order_by = [f"{db.get_id_function_name()}(n) {self.node_ordering.direction.value}"] case OrderByField.CREATED_AT: self.order_by = [f"n.created_at {self.node_ordering.direction.value}"] case OrderByField.UPDATED_AT: self.order_by = [f"n.updated_at {self.node_ordering.direction.value}"]
🧹 Nitpick comments (6)
backend/infrahub/constants/enums.py (1)
1-12: Consider adding docstrings for clarity.While the enum names and values are self-explanatory, adding brief docstrings would improve code documentation and align with the project's Google-style docstring guidelines.
🔎 Example docstrings:
class OrderDirection(StrEnum): + """Specifies the direction for ordering query results.""" ASC = "ASC" DESC = "DESC" class OrderByField(StrEnum): + """Specifies the field to use for ordering nodes.""" ID = "id" CREATED_AT = "created_at" UPDATED_AT = "updated_at"backend/infrahub/graphql/types/metadata.py (1)
8-15: Add class-level docstrings for the new InputObjectType classes.Both
InfrahubNodeMetadataOrderandOrderInputwould benefit from docstrings explaining their purpose, per the coding guidelines for Python files.🔎 Suggested docstrings:
class InfrahubNodeMetadataOrder(InputObjectType): + """Input type for specifying ordering by node metadata fields. + + Allows ordering by created_at or updated_at timestamps. + Only one field should be specified at a time. + """ created_at = Field(InfrahubOrderDirection, required=False, description="Order by creation timestamp") updated_at = Field(InfrahubOrderDirection, required=False, description="Order by updated timestamp") class OrderInput(InputObjectType): + """Input type for controlling result ordering in queries. + + Provides options to disable ordering or specify metadata-based ordering. + """ disable = Boolean(required=False) node_metadata = Field(InfrahubNodeMetadataOrder, required=False, description="Order settings for branch metadata")As per coding guidelines, use Google-style docstrings for Python classes.
backend/tests/unit/graphql/queries/test_branch.py (1)
484-540: Consider filtering to test-created branches for better isolation.While the current assertion on line 539 (checking
edges[0]is "main") is likely safe, the test would be more robust if it filtered results to only the branches created in this test, similar to the approach intest_order_by_updated_at_ascending(lines 660-661).🔎 Example pattern from updated_at tests:
test_branch_names = set(branch_names) test_edges = [e for e in edges if e["node"]["name"]["value"] in test_branch_names] # Then assert on test_edges instead of edgesbackend/infrahub/core/node/standard.py (1)
35-38: Add a docstring to the StandardNodeOrdering dataclass.Per the coding guidelines, classes should have Google-style docstrings explaining their purpose. A docstring would clarify the role of this dataclass in configuring node query ordering.
🔎 Suggested docstring:
@dataclass class StandardNodeOrdering: + """Configuration for ordering standard node queries. + + Attributes: + order_by: Field to order by (ID, CREATED_AT, or UPDATED_AT). + direction: Sort direction (ASC or DESC). + """ order_by: OrderByField = field(default=OrderByField.ID) direction: OrderDirection = field(default=OrderDirection.ASC)As per coding guidelines, use Google-style docstrings for Python classes.
schema/schema.graphql (1)
7428-7433: Consider documenting the mutual exclusion constraint.According to the PR objectives, combining
created_atwithupdated_atis forbidden and raises an error. Adding a doc comment on the input type explaining this constraint would help API consumers understand the expected usage.+""" +Order settings for node metadata timestamps. +Note: Only one of created_at or updated_at can be specified at a time. +""" input InfrahubNodeMetadataOrder { """Order by creation timestamp""" created_at: OrderDirection = null """Order by updated timestamp""" updated_at: OrderDirection = null }backend/infrahub/graphql/models.py (1)
12-22: Consider using Pydantic models instead of dataclasses.The coding guidelines specify "Use Pydantic models for dataclasses in Python". Using Pydantic would also provide automatic nested model deserialization, which would resolve the
from_inputissue.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/infrahub/constants/enums.py(1 hunks)backend/infrahub/core/branch/models.py(2 hunks)backend/infrahub/core/node/standard.py(3 hunks)backend/infrahub/core/query/standard_node.py(4 hunks)backend/infrahub/graphql/manager.py(1 hunks)backend/infrahub/graphql/models.py(1 hunks)backend/infrahub/graphql/queries/branch.py(4 hunks)backend/infrahub/graphql/resolvers/ipam.py(4 hunks)backend/infrahub/graphql/resolvers/resolver.py(3 hunks)backend/infrahub/graphql/types/enums.py(2 hunks)backend/infrahub/graphql/types/metadata.py(1 hunks)backend/tests/unit/graphql/queries/test_branch.py(1 hunks)schema/schema.graphql(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/infrahub/graphql/resolvers/resolver.py
- backend/infrahub/graphql/types/enums.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Always use triple quotes (""") for Python docstrings
Follow Google-style docstring format for Python docstrings
Include brief one-line description in Python docstrings when applicable
Include detailed description in Python docstrings when applicable
Include Args/Parameters section without typing in Python docstrings when applicable
Include Returns section in Python docstrings when applicable
Include Raises section in Python docstrings when applicable
Include Examples section in Python docstrings when applicable
**/*.py: Use type hints for all function parameters and return values in Python
Use Async whenever possible in Python
Useasync deffor asynchronous functions in Python
Useawaitfor asynchronous calls in Python
Use Pydantic models for dataclasses in Python
Use ruff and mypy for type checking and code validation in PythonUse ruff and mypy to validate and lint Python files
Files:
backend/infrahub/constants/enums.pybackend/infrahub/graphql/types/metadata.pybackend/infrahub/core/branch/models.pybackend/infrahub/core/node/standard.pybackend/infrahub/graphql/manager.pybackend/infrahub/core/query/standard_node.pybackend/infrahub/graphql/models.pybackend/infrahub/graphql/resolvers/ipam.pybackend/tests/unit/graphql/queries/test_branch.pybackend/infrahub/graphql/queries/branch.py
backend/infrahub/**/*.py
📄 CodeRabbit inference engine (backend/AGENTS.md)
backend/infrahub/**/*.py: Use async/await for all I/O operations to maintain async-first architecture
Type hint all function parameters and returns in Python code
Use Pydantic models for defining data structures instead of plain dictionaries
Use Query class pattern (extending infrahub.core.query.Query) for all database operations instead of unparameterized Cypher queries
Use Google-style docstrings with Args, Returns, and Raises sections for all functions
Use snake_case for function and variable names
Use PascalCase for class names
Use UPPER_SNAKE_CASE for constant definitions
Do not use unparameterized Cypher queries; always use parameterized queries to prevent injection
Do not block the event loop with synchronous I/O operations
Files:
backend/infrahub/constants/enums.pybackend/infrahub/graphql/types/metadata.pybackend/infrahub/core/branch/models.pybackend/infrahub/core/node/standard.pybackend/infrahub/graphql/manager.pybackend/infrahub/core/query/standard_node.pybackend/infrahub/graphql/models.pybackend/infrahub/graphql/resolvers/ipam.pybackend/infrahub/graphql/queries/branch.py
backend/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use type hints for all Python code in the backend
Files:
backend/infrahub/constants/enums.pybackend/infrahub/graphql/types/metadata.pybackend/infrahub/core/branch/models.pybackend/infrahub/core/node/standard.pybackend/infrahub/graphql/manager.pybackend/infrahub/core/query/standard_node.pybackend/infrahub/graphql/models.pybackend/infrahub/graphql/resolvers/ipam.pybackend/tests/unit/graphql/queries/test_branch.pybackend/infrahub/graphql/queries/branch.py
backend/tests/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasksName test files as
test_<module>.pyand mirror source structure in tests directory
Files:
backend/tests/unit/graphql/queries/test_branch.py
🧠 Learnings (2)
📚 Learning: 2025-12-01T22:16:13.680Z
Learnt from: CR
Repo: opsmill/infrahub PR: 0
File: backend/AGENTS.md:0-0
Timestamp: 2025-12-01T22:16:13.680Z
Learning: Applies to backend/infrahub/**/*.py : Use Query class pattern (extending infrahub.core.query.Query) for all database operations instead of unparameterized Cypher queries
Applied to files:
backend/infrahub/core/node/standard.pybackend/infrahub/core/query/standard_node.py
📚 Learning: 2025-10-30T12:43:45.663Z
Learnt from: ogenstad
Repo: opsmill/infrahub PR: 7447
File: schema/schema.graphql:324-329
Timestamp: 2025-10-30T12:43:45.663Z
Learning: In the GraphQL schema (schema/schema.graphql), the BranchStatus enum includes NEED_UPGRADE_REBASE. Branches with NEED_UPGRADE_REBASE status must support conflict resolution capabilities.
Applied to files:
backend/infrahub/graphql/queries/branch.py
🧬 Code graph analysis (8)
backend/infrahub/core/branch/models.py (2)
backend/infrahub/core/node/standard.py (2)
StandardNode(47-315)StandardNodeOrdering(36-38)backend/infrahub/core/query/branch.py (1)
BranchNodeGetListQuery(153-160)
backend/infrahub/core/node/standard.py (2)
backend/infrahub/constants/enums.py (2)
OrderByField(9-12)OrderDirection(4-6)backend/infrahub/core/query/standard_node.py (1)
StandardNodeGetListQuery(133-192)
backend/infrahub/graphql/manager.py (1)
backend/infrahub/graphql/types/metadata.py (1)
OrderInput(13-15)
backend/infrahub/core/query/standard_node.py (2)
backend/infrahub/constants/enums.py (1)
OrderByField(9-12)backend/infrahub/core/node/standard.py (2)
StandardNode(47-315)StandardNodeOrdering(36-38)
backend/infrahub/graphql/models.py (1)
backend/infrahub/constants/enums.py (1)
OrderDirection(4-6)
backend/infrahub/graphql/resolvers/ipam.py (1)
backend/infrahub/graphql/models.py (2)
OrderModel(19-36)from_input(25-32)
backend/tests/unit/graphql/queries/test_branch.py (4)
backend/infrahub/core/branch/models.py (1)
Branch(31-569)backend/tests/unit/conftest.py (1)
session_admin(2524-2526)backend/infrahub/graphql/initialization.py (1)
prepare_graphql_params(89-136)backend/tests/helpers/graphql.py (1)
graphql(26-61)
backend/infrahub/graphql/queries/branch.py (4)
backend/infrahub/constants/enums.py (2)
OrderByField(9-12)OrderDirection(4-6)backend/infrahub/core/node/standard.py (1)
StandardNodeOrdering(36-38)backend/infrahub/graphql/field_extractor.py (1)
extract_graphql_fields(67-69)backend/infrahub/graphql/types/metadata.py (1)
OrderInput(13-15)
⏰ 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). (5)
- GitHub Check: graphql-schema
- GitHub Check: validate-generated-documentation
- GitHub Check: backend-tests-integration
- GitHub Check: json-schema
- GitHub Check: Cloudflare Pages
🔇 Additional comments (14)
backend/infrahub/graphql/manager.py (1)
24-24: LGTM! Centralizing OrderInput definition.Moving the OrderInput definition to a shared metadata module eliminates duplication and improves maintainability.
backend/tests/unit/graphql/queries/test_branch.py (3)
601-668: LGTM! Proper test isolation.This test correctly filters results to only the branches created within the test (lines 660-661), avoiding dependencies on other tests. The approach of updating branches in a specific order and then verifying the ordering matches expectations is well-designed.
669-738: LGTM! Proper test isolation with descending order.This test correctly filters results to only test-created branches (lines 729-730) and properly verifies descending order. The pattern matches the ascending test and maintains good isolation.
740-766: LGTM! Important validation test.This test properly validates the mutual exclusion constraint between
created_atandupdated_atordering fields, ensuring the API returns an appropriate error when both are specified.backend/infrahub/core/node/standard.py (1)
306-311: LGTM! Clean ordering integration.The addition of the
node_orderingparameter with proper defaulting and propagation to the query layer is well-implemented. The pattern maintains backward compatibility while enabling ordering functionality.schema/schema.graphql (2)
9788-9798: LGTM!The
OrderDirectionenum andOrderInputextension are well-structured. The addition ofnode_metadataaligns with the PR objective to support metadata-based ordering.Minor note: The docstring says "branch metadata" but the field is named
node_metadata. If this ordering is intended to apply to other node types in the future (as mentioned in the PR), consider generalizing the docstring to "Order settings for node metadata" for consistency.
11137-11145: LGTM!The addition of the
orderparameter toInfrahubBranchquery is clean and follows the established pattern. The docstring clearly describes its purpose for ordering branch query results.backend/infrahub/core/branch/models.py (1)
159-171: LGTM! Clean integration of ordering into Branch.get_list.The optional
node_orderingparameter with a sensible default follows the existing pattern and correctly propagates toBranchNodeGetListQuery. The defaulting logic at line 162 ensures a validStandardNodeOrderingis always passed downstream.backend/infrahub/graphql/queries/branch.py (3)
19-46: Well-structured ordering conversion with proper validation.The function follows coding guidelines with a Google-style docstring and proper type hints. The mutual exclusion validation aligns with the PR objective of forbidding combined
created_at/updated_atordering.
133-137: LGTM! GraphQL order argument properly exposed.The
orderargument is correctly defined withOrderInputtype and appropriate description for the public API.
107-113: Remove this comment -node_orderingis explicitly used.The
node_orderingparameter is not silently ignored. It's explicitly defined inStandardNodeGetListQuery.__init__(line 141 inbackend/infrahub/core/query/standard_node.py) and actively used in thequery_initmethod to set the ORDER BY clause based on the specified ordering field.Likely an incorrect or invalid review comment.
backend/infrahub/core/query/standard_node.py (1)
138-153: LGTM! Clean integration of node_ordering parameter.The
node_orderingparameter is properly typed, stored on the instance, and theassert_neverusage ensures exhaustive handling of ordering fields.backend/infrahub/graphql/resolvers/ipam.py (1)
312-325: Pattern change looks correct, but depends onOrderModel.from_inputfix.The change from accepting
OrderModeldirectly to acceptingdict[str, Any]and converting viafrom_inputis appropriate for handling raw GraphQL input. However, this will fail at runtime whennode_metadatais provided until the nested deserialization issue inOrderModel.from_inputis resolved.backend/infrahub/graphql/models.py (1)
8-9: No runtime issues exist with this TYPE_CHECKING import.With
from __future__ import annotationsat the top of the file, type hints are no longer interpreted at runtime, so there's no NameError. Whenfrom __future__ import annotationsis used, imports only needed for type annotations can safely be placed insideif TYPE_CHECKING:.Since the file already includes
from __future__ import annotationson line 1, theOrderDirectionimport underTYPE_CHECKINGis correct and follows best practices. The annotations inNodeMetaOrderare stored as strings and never evaluated by the dataclass at runtime, preventing any import errors.Likely an incorrect or invalid review comment.
| self.order_by = [f"{db.get_id_function_name()}(n)"] | ||
| match self.node_ordering.order_by: | ||
| case OrderByField.ID: | ||
| self.order_by = [f"{db.get_id_function_name()}(n)"] |
There was a problem hiding this comment.
I missed it was already the default behavior
this is fine then
2 participants
Add sort options for metadata to GraphQL layer, fully implemented for the standard nodes (i.e. only InfrahubBranch) and just included the API bindings for the dynamically defined schema nodes.
Example query for InfrahubBranch:
In this current implementation I've forbidden anyone from combining updated_at with created_at, specifically for these two fields I don't think it matters much as there won't be much overlap with anything that has the same timestamp. However it might come into play at some later time for other fields. It seems like the order of what the user requests (i.e. if created_at is defined before updated_at) is kept within the info object but not in the created models. At some later point we could analyze the actual query and retain the internal order, for now we raise an error if someone specifies both updated_at and created_at)
I've added the same "order" part to the other dynamically created options, but those doesn't currently do anything, they are just a dummy entry so that the frontend team can experiment with what it will look like for the other node types.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.