support ordering in GraphQL many relationships, respect order of ordering clauses#9397
Conversation
There was a problem hiding this comment.
2 issues found across 11 files
Confidence score: 3/5
- There is some merge risk due to a concrete behavior issue in
backend/infrahub/core/query/node.py: the metadata ORDER upsert can match non-metadata filters by name, which may misroutenode_metadata__created_atordering to an attribute requirement. - The issue above is medium severity (6/10) with fairly strong confidence (8/10), so it could cause incorrect query ordering behavior for users even though it is likely localized.
- The
backend/infrahub/graphql/loaders/peers.pyhash truthiness check is low severity (2/10) and mostly a cache-key correctness edge case, so it should not heavily block merge on its own. - Pay close attention to
backend/infrahub/core/query/node.py,backend/infrahub/graphql/loaders/peers.py- ordering resolution and cache-key hashing semantics may behave unexpectedly in edge cases.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/infrahub/graphql/loaders/peers.py">
<violation number="1" location="backend/infrahub/graphql/loaders/peers.py:48">
P3: Hash uses `__bool__` truthiness (`if self.order`) instead of an explicit `is not None` check. `OrderModel(disable=False)` is falsy and would hash identically to `None`, coupling cache-key behavior to `OrderModel.__bool__` semantics. Prefer `if self.order is not None` for robustness.</violation>
</file>
<file name="backend/infrahub/core/query/node.py">
<violation number="1" location="backend/infrahub/core/query/node.py:2220">
P2: Metadata ORDER upsert matches non-metadata filters by name, which can route `node_metadata__created_at` ordering to an attribute requirement instead of metadata.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| field is fine because the outer ORDER BY references the field name directly. | ||
| """ | ||
| for req in filter_requirements: | ||
| if req.field_name == metadata_field: |
There was a problem hiding this comment.
P2: Metadata ORDER upsert matches non-metadata filters by name, which can route node_metadata__created_at ordering to an attribute requirement instead of metadata.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/core/query/node.py, line 2220:
<comment>Metadata ORDER upsert matches non-metadata filters by name, which can route `node_metadata__created_at` ordering to an attribute requirement instead of metadata.</comment>
<file context>
@@ -2221,105 +2202,127 @@ def _get_filter_requirements(self, start_index: int) -> list[FieldAttributeRequi
+ field is fine because the outer ORDER BY references the field name directly.
+ """
+ for req in filter_requirements:
+ if req.field_name == metadata_field:
+ req.types.append(FieldAttributeRequirementType.ORDER)
+ req.order_direction = direction
</file context>
| if req.field_name == metadata_field: | |
| if req.is_metadata and req.field_name == metadata_field: |
| str(self.branch_agnostic), | ||
| str(hash(self.include_metadata)), | ||
| # TODO: would it be safer to add a __hash__ method to OrderModel or is order guaranteed in model_dump_json? | ||
| self.order.model_dump_json() if self.order else "", |
There was a problem hiding this comment.
P3: Hash uses __bool__ truthiness (if self.order) instead of an explicit is not None check. OrderModel(disable=False) is falsy and would hash identically to None, coupling cache-key behavior to OrderModel.__bool__ semantics. Prefer if self.order is not None for robustness.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/graphql/loaders/peers.py, line 48:
<comment>Hash uses `__bool__` truthiness (`if self.order`) instead of an explicit `is not None` check. `OrderModel(disable=False)` is falsy and would hash identically to `None`, coupling cache-key behavior to `OrderModel.__bool__` semantics. Prefer `if self.order is not None` for robustness.</comment>
<file context>
@@ -42,6 +44,8 @@ def __hash__(self) -> int:
str(self.branch_agnostic),
str(hash(self.include_metadata)),
+ # TODO: would it be safer to add a __hash__ method to OrderModel or is order guaranteed in model_dump_json?
+ self.order.model_dump_json() if self.order else "",
]
)
</file context>
| self.order.model_dump_json() if self.order else "", | |
| self.order.model_dump_json() if self.order is not None else "", |
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
Why
Adds tests for various ordering combinations for NodeGetListQuery, RelationshipGetPeerQuery, and NodeGetHierarchyQuery.
Adds support for ordering to many relationships within the graphql resolver. See the new
test_graphql_relationship_peer_query_order_replaces_schema_orderfor an example.Fixes a bug in
NodeGetListQuerythat caused metadata ordering to always be applied first regardless of the ordering defined on the schema or in the graphql request.Non-goals: no changes to the wire shape of
order_by, no changes to the parser grammar, no new metadata fields beyondcreated_at/updated_at.Closes IFC-2561
What changed
Behavioral:
NodeGetListQuerynow emits its outerORDER BYclauses in the schema-declared order, even when entries mix metadata and attributes. Pure-attribute and pure-metadata schemas are unchanged.orderargument is now honored. When provided, it fully replaces the peer schema'sorder_by(no stacking).Implementation:
schema_order_positionfield toFieldAttributeRequirementso ORDER FARs can be sorted back into declared order after both metadata and attribute subqueries have been emitted. The subquery helpers (_add_metadata_subqueries,_add_node_order_attributes) no longer touchself.order_by; a new_emit_schema_order_bysorts ORDER FARs byschema_order_positionand emits the outer clauses._get_order_requirementsinto two small helpers (_upsert_metadata_order_requirement,_upsert_attribute_order_requirement) that either promote an existing filter FAR to also act as an ORDER or return a fresh ORDER-only FAR. The nestedrequirements_mapdict is gone; lookup is now a direct iteration overfilter_requirements.len(filter_requirements) + 1 + position. Unique by construction, no threaded counter._get_metadata_order_fieldshelper. Thehas_any_ordercheck now leans on the newOrderModel.__bool__(true whendisableornode_metadatacarries non-default content) plus the existingNodeMetaOrder.__bool__._query_time_order_overrides_schemain bothNodeGetListQueryandRelationshipGetPeerQuerycollapses toreturn bool(self.requested_order).requested_order: OrderModel | Noneparameter toRelationshipGetPeerQuery, threaded throughNodeManager.query_peers, thePeerRelationshipsDataLoadercache key, and the many-relationship resolver._add_peer_order_bydrops the peer schema'sorder_bywhenrequested_ordercarries non-default content.What stayed the same:
order_byis unchanged.order_byin declared order, so they needed no fix.Checklist
uv run towncrier create ...)Summary by cubic
Adds ordering to GraphQL many relationships and fixes ORDER BY precedence so mixed metadata/attribute entries follow the schema’s order. Query-time
orderfully replaces schemaorder_by, cache keys vary byorder, and theorder_bywire shape and grammar are unchanged.New Features
orderis honored end-to-end (threaded throughNodeManager.query_peers,PeerRelationshipsDataLoader, andRelationshipGetPeerQuery).created_at/updated_atASC/DESC; query-time overrides replace schema defaults. Aligns with INFP-530.Bug Fixes
OrderModelis now frozen and hashable; peer loader cache keys includeorderto prevent stale results.Written for commit f0c3523. Summary will update on new commits.