add metadata order by to GET cypher queries#9382
Conversation
There was a problem hiding this comment.
3 issues found across 14 files
Confidence score: 3/5
- There is a concrete regression risk in
backend/infrahub/core/schema/schema_branch.py:validate_order_by()currently validates only metadata entries, so invalid direction suffixes on attribute/relationship sorts may pass through and cause inconsistent query behavior. - The change in
backend/infrahub/core/query/node.pyhas medium, somewhat uncertain impact: treatingnode_metadataas an override even when empty can bypass schemaorder_byand unexpectedly fall back to UUID sorting. dev/specs/infp-530-order-by-metadata-direction/tasks.mdhas a duplicate task ID (T019), which is non-runtime but can create planning/traceability confusion; this is minor to fix.- Pay close attention to
backend/infrahub/core/schema/schema_branch.py,backend/infrahub/core/query/node.py- sorting validation and override behavior could change user-visible ordering semantics.
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/core/schema/schema_branch.py">
<violation number="1" location="backend/infrahub/core/schema/schema_branch.py:966">
P1: `validate_order_by()` parses only metadata entries, so invalid direction suffixes on attribute/relationship entries are not validated and can slip through.
(Based on your team's feedback about requiring explicit property paths and rejecting ambiguous direction-only forms in `order_by`.) [FEEDBACK_USED]</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| for order_by_path in node_schema.order_by: | ||
| element_name = "order_by" | ||
| for order_by_entry in node_schema.order_by: | ||
| if is_metadata_order_by_entry(order_by_entry): |
There was a problem hiding this comment.
P1: validate_order_by() parses only metadata entries, so invalid direction suffixes on attribute/relationship entries are not validated and can slip through.
(Based on your team's feedback about requiring explicit property paths and rejecting ambiguous direction-only forms in order_by.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/core/schema/schema_branch.py, line 966:
<comment>`validate_order_by()` parses only metadata entries, so invalid direction suffixes on attribute/relationship entries are not validated and can slip through.
(Based on your team's feedback about requiring explicit property paths and rejecting ambiguous direction-only forms in `order_by`.) </comment>
<file context>
@@ -954,20 +955,26 @@ def validate_uniqueness_constraints(self) -> None:
- for order_by_path in node_schema.order_by:
- element_name = "order_by"
+ for order_by_entry in node_schema.order_by:
+ if is_metadata_order_by_entry(order_by_entry):
+ try:
+ parse_order_by_entry(entry=order_by_entry, node_schema=node_schema)
</file context>
There was a problem hiding this comment.
I do not think this is right. the method attempts parsing metadata order_by entries within the conditional if is_metadata_order_by_entry(order_by_entry):, but non-metadata entries pass through the existing self.validate_schema_path() call
There was a problem hiding this comment.
Thanks for the feedback.
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Shadow auto-approve: would require human review. This PR modifies core query logic for ordering in multiple critical paths (NodeGetListQuery, RelationshipGetPeerQuery, NodeGetHierarchyQuery), adds new subquery building functions, changes schema parsing and validation for order_by entries, and touches inheritance handling—all of which could affect
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would require human review. This PR introduces significant new ordering functionality across multiple query paths (node list, relationship peer, hierarchy) with new grammar parsing, subquery logic, and a precedence change, all of which impact core business logic and carry a high risk of breakage if bugs exist.
Re-trigger cubic
| # Grouped constants for validation/iteration | ||
| METADATA_CREATED_FIELDS = (METADATA_CREATED_AT, METADATA_CREATED_BY) | ||
| METADATA_UPDATED_FIELDS = (METADATA_UPDATED_AT, METADATA_UPDATED_BY) | ||
| # TODO: import from constants and append the "__" |
There was a problem hiding this comment.
Is that something missing in the PR, or just a note for later cleanup?
| if is_metadata_order_by_entry(entry): | ||
| updated_entries.append(entry) | ||
| continue | ||
| updated_entries.extend(self._get_updated_renamed_attrs_data(attr_data=[entry], renamed_attrs=renamed_attrs)) |
There was a problem hiding this comment.
_get_updated_renamed_attrs_data accepts a list, could we avoid multiple calls to it by going through the for loop then sending a list of entries to _get_updated_renamed_attrs_data?
| # Grouped constants for validation/iteration | ||
| METADATA_CREATED_FIELDS = (METADATA_CREATED_AT, METADATA_CREATED_BY) | ||
| METADATA_UPDATED_FIELDS = (METADATA_UPDATED_AT, METADATA_UPDATED_BY) | ||
| # TODO: import from constants and append the "__" |
There was a problem hiding this comment.
Since this isn't explicitly stated in the Non-goals section of the PR description, I'm just bringing this line up in case it's not meant to be left in this PR.
| ], | ||
| "relationships": [ | ||
| {"name": "node_metadata", "peer": "TestCriticality", "cardinality": "one"}, | ||
| {"name": "node_metadata", "peer": "Criticality", "cardinality": "one"}, |
There was a problem hiding this comment.
I am not sure why this has changed since the test seems to pass with or without the change.
Moreover, that is true that the other upper schemas using this relationship have the same peer but it seems that the classic convention is to concatenate the name and the namespace (e.g: TestCriticality).
But I guess this does not really matter.
Why
Today, schema authors can declare
order_by: ["name__value"]on a schema, but they cannot ask for descending order or for ordering by node-level metadata (created_at,updated_at). For example, aDocumentationNoterelationship that should always render newest-first on the parent detail page requires per-query ordering arguments at every callsite.Goal: make
order_by: ["node_metadata__created_at__desc"](and the rest of the new grammar) honored consistently across the three list paths — top-level node list, relationship-peer list, and hierarchy list — including direction suffixes on regular attributes (name__value__desc).Non-goals: stricter rejection messages for malformed
order_byentries (US3, deferred) and US2 verification beyond what falls out of US1's direction propagation.Closes IFC-2650
What changed
Behavioral
order_bygrammar accepted by the schema validator:<attr>__<prop>[__<dir>],<rel>__<attr>__<prop>[__<dir>],node_metadata__<field>[__<dir>]where<field>∈{created_at, updated_at}and<dir>∈{asc, desc}. Direction defaults to ascending when no suffix is present. Existing schemas continue to behave as ascending without modification.<attr>paths is now applied at query time. AffectsNodeGetListQuery,RelationshipGetPeerQuery, andNodeGetHierarchyQuery.order_by: ["node_metadata__created_at__desc"]is honored across all three list paths and through the matching GraphQL fields (top-level + nested relationship-peer).orderargument (REST/GraphQL/SDK), the schema'sorder_byis now ignored entirely. The UUID tiebreaker is appended once per query in all cases for deterministic ordering.Implementation notes
validate_order_byroutes metadata-prefixed entries (node_metadata__…) through the new parser and leaves attribute/relationship-attribute path entries tovalidate_schema_pathso existing error messages ("Xis not a valid attribute ofY", "cannot useXrelationship") are preserved.account__name__desc) and lenient on path existence that requires knowledge of other schemas (remains delegated tovalidate_schema_path).build_subquery_order_metadata(node_alias, metadata_field, …)incore/query/subquery.pyproduces the inner Cypher forCALL (<alias>) { … }ordering bycreated_at/updated_at. Handles both default/global-branch shortcuts and the branch-aware traversal. Used by bothRelationshipGetPeerQueryandNodeGetHierarchyQuery.RelationshipGetPeerQueryandNodeGetHierarchyQueryeach gained a_add_peer_order_by(...)private method; theirquery_initordering sections collapse to one helper call plus an unconditionalpeer.uuid ASCtiebreaker.NodeMetaOrdergained__bool__so an empty input (order: {node_metadata: {}}) doesn't override the schema'sorder_by.NodeInheritanceHandler._update_order_by_for_renamed_attributesskips entries that are metadata (viais_metadata_order_by_entry) so renames can't corruptnode_metadata__*references on the inheriting node.What stayed the same
order_byis stilllist[str] | None.order_bybehave identically to today — UUID-only sort, unchanged.validate_schema_patherror messages for malformed paths are unchanged; the existingtest_validate_order_by_errorparametrized cases all still pass with their exact expected messages.Impact & rollout
order_bydeclarations are unchanged in behavior (ascending by default, same paths accepted). The one observable change is the new replace-not-stack precedence when a caller passes a query-timeorderargument — previously the schema's entries would still apply as secondary sort, now they're dropped. Surfaces only for callers that set both schemaorder_byand a query-timeorder; the customary pattern (set one or the other) is unaffected.ASC/DESC) to the outerORDER BY. Each new metadata entry on the relationship-peer / hierarchy paths adds one subquery comparable in cost to the existing top-level metadata subquery. UUID tiebreaker is appended once per query on a projected property; no extra traversal.Checklist
uv run towncrier create ...)Summary by cubic
Adds schema-level order_by for node metadata (created_at, updated_at) with ASC/DESC and consistent, directional ordering across node lists, relationship peers, and hierarchies. Query-time order now replaces schema order for top-level lists (REST/GraphQL); when no query-time order is given, schema order is honored (INFP-530).
New Features
valueproperties allowed.NodeGetListQuery,RelationshipGetPeerQuery, andNodeGetHierarchyQuerywith per-entry direction and a UUID ASC tiebreaker; addedbuild_subquery_order_metadatafor branch-aware created_at/updated_at.orderargument is provided, and both top-level and relationship-peer lists honor schema order when no query-time order is passed. Tests added across component, unit, and GraphQL suites.Bug Fixes
NodeMetaOrdertruthiness so empty query-timenode_metadatano longer suppresses schema order_by.Written for commit f418c00. Summary will update on new commits.
Review in cubic