feat(query-compiler): support orderBy on to-many relation scalar fields#5807
Open
DevFigueiredo wants to merge 7 commits intoprisma:mainfrom
Open
feat(query-compiler): support orderBy on to-many relation scalar fields#5807DevFigueiredo wants to merge 7 commits intoprisma:mainfrom
DevFigueiredo wants to merge 7 commits intoprisma:mainfrom
Conversation
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds support for ordering by a scalar field on to-many relations via a new OrderBy::ToManyField variant, updates argument extraction to distinguish aggregation vs scalar to-many ordering, implements correlated LIMIT-1 subqueries and nulls-aware cursor logic, adjusts select projections and schema input builders, and adds tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@query-compiler/query-builders/sql-query-builder/src/cursor_condition.rs`:
- Around line 519-534: cursor_order_def_to_many_field must not call
foreign_keys_from_order_path because OrderByDefinition.joins for ToManyField
omits the final to-many hop, causing length mismatches and panics/incorrect
aliases; instead, return a CursorOrderDefinition with order_fks set to an empty
Vec (or equivalent "no FKs") and on_nullable_fields: true so the correlated
subquery handles nullability; update cursor_order_def_to_many_field (remove the
foreign_keys_from_order_path call) and ensure downstream code
(map_orderby_condition) relies on order_fks being empty to avoid emitting extra
"<alias>.<fk> IS NULL" predicates.
In `@query-compiler/query-builders/sql-query-builder/src/ordering.rs`:
- Around line 160-321: The inner correlated subquery must use the user's
original sort/nulls direction and not the reversed direction; remove the use of
needs_reversed_order when computing the inner ORDER BY. Specifically: stop
forwarding needs_reversed_order into compute_subquery_for_to_many_field and
remove it from compute_subquery_for_to_many_field,
build_one2m_correlated_subquery, and build_m2m_correlated_subquery signatures
and calls; call into_order(sort_order, nulls_order, /* no reversal */) (i.e.
pass the original sort/nulls) when constructing inner_order in
build_one2m_correlated_subquery and build_m2m_correlated_subquery so the LIMIT 1
representative is stable; update build_order_to_many_field to compute the outer
order reversal separately and pass only the needed outer order info to the outer
OrderDefinition; and add a regression test covering take: -n combined with a
to-many field orderBy to prevent regressions.
- Around line 303-320: The LEFT JOIN on the M2M junction is effectively an inner
join because so_that(...) enforces m2m_parent_col to match a concrete parent id;
change the join to an explicit inner join by replacing the call to
left_join(m2m_table.on(ConditionTree::And(left_join_conditions))) with an
inner_join variant so the subquery built in subquery (using
child_model.as_table, field_col, junction_conditions, and left_join_conditions)
reflects the intent; ensure the OrderDefinition/inner_order and subsequent
Expression::from(subquery) remain unchanged.
In `@query-compiler/query-builders/sql-query-builder/src/select/mod.rs`:
- Around line 591-592: The inner select for OrderBy::ToManyField is missing the
parent's linking fields for one-to-many relations, causing the correlated
predicate (which uses context_alias in ordering.rs) to fail when with_ordering
runs with inner_alias; update the inner_selection construction in the
one-to-many branch to include linking_fields in the FieldSelection::union
(alongside order_by_selection, distinct_selection, filtering_selection, and
relation_selection) so the parent FK/link columns are projected into the inner
layer used by with_ordering/inner_alias.
In `@query-compiler/query-structure/src/order_by.rs`:
- Around line 110-122: The two constructors OrderBy::scalar and
OrderBy::to_many_field have inconsistent parameter ordering (field vs path);
change OrderBy::to_many_field signature to take (field: ScalarFieldRef, path:
Vec<OrderByHop>, sort_order: SortOrder, nulls_order: Option<NullsOrder>) and
update the construction of OrderBy::ToManyField (OrderByToManyField { path,
field, ... }) and every call site that uses to_many_field so the field argument
comes first to match OrderBy::scalar and avoid confused call-sites.
In `@query-compiler/schema/src/build/input_types/objects/order_by_objects.rs`:
- Around line 219-251: The function order_by_to_many_aggregate_object_type
currently reuses the IdentifierType::OrderByToManyAggregateInput identifier for
both aggregate fields (e.g., _count) and direct scalar-field ordering, which
mixes semantics; to fix, introduce a separate input type identifier (e.g.,
IdentifierType::OrderByToManyScalarFieldsInput) and build a distinct
InputObjectType for scalar-field ordering inside
order_by_to_many_aggregate_object_type (or compose the two), keep the existing
aggregate object unchanged, and update places that reference the combined
identifier so aggregate-only consumers still get the original
OrderByToManyAggregateInput while scalar-field ordering is provided by the new
type (ensure you adjust init_input_object_type, set_container, and set_fields
usage for the new identifier).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e2d62bc4-3084-4c01-810b-9815649dc1f4
⛔ Files ignored due to path filters (3)
query-compiler/query-compiler/tests/snapshots/queries__queries@order-by-to-many-1m-field-nulls.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@order-by-to-many-1m-field.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@order-by-to-many-m2m-field.json.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
query-compiler/core/src/query_graph_builder/extractors/query_arguments.rsquery-compiler/query-builders/sql-query-builder/src/cursor_condition.rsquery-compiler/query-builders/sql-query-builder/src/ordering.rsquery-compiler/query-builders/sql-query-builder/src/select/mod.rsquery-compiler/query-compiler/tests/data/order-by-to-many-1m-field-nulls.jsonquery-compiler/query-compiler/tests/data/order-by-to-many-1m-field.jsonquery-compiler/query-compiler/tests/data/order-by-to-many-m2m-field.jsonquery-compiler/query-structure/src/order_by.rsquery-compiler/query-structure/src/record.rsquery-compiler/schema/src/build/input_types/objects/order_by_objects.rs
DanielFigueiredoCiandt
force-pushed
the
feat/order-by-to-many-field
branch
from
be8f40a to
654bd5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@query-compiler/query-builders/sql-query-builder/src/ordering.rs`:
- Around line 269-276: Update the stale SQL comments in ordering.rs that
describe the correlated sub-SELECT for many-to-many relations: the doc-block and
the inline comment still say "LEFT JOIN" but the implementation uses inner_join,
so change those occurrences (the doc-block starting "Builds a correlated
sub-SELECT for many-to-many relations" and the nearby inline comment around the
second occurrence) to "INNER JOIN" (or update the wording to reflect an inner
join) — search for the comment text "LEFT JOIN _Junction" or the phrase
"correlated sub-SELECT for many-to-many relations" and replace both occurrences
(including the one around the later line noted) so the comments match the
implementation.
In `@query-compiler/query-structure/src/record.rs`:
- Line 82: ManyRecords::order_by currently panics on the new
OrderBy::ToManyField variant because it hits the unimplemented!() branch; update
ManyRecords::order_by to handle OrderBy::ToManyField by either (A) implementing
a deterministic comparison strategy for to-many fields (e.g., compare by
aggregated value, first/last element, or a stable key derived from the
collection) and using that comparator during in-memory sorting, or (B) if
in-memory comparison is unsupported, replace the unimplemented!() with a clear,
typed error return (or panic with a descriptive message) and ensure callers
validate/convert OrderBy::ToManyField into a supported form before calling
ManyRecords::order_by; reference the OrderBy::ToManyField enum variant and the
ManyRecords::order_by method when implementing the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1892148f-8bc3-464f-acc7-4e0ea46cadd2
⛔ Files ignored due to path filters (9)
query-compiler/query-compiler/tests/snapshots/queries__queries@nested-distinct-incompatible-orderby-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@nested-distinct-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@nested-distinct-orderby-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@nested-distinct-pagination-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@nested-pagination-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@order-by-to-many-1m-field-nulls.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@order-by-to-many-1m-field.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@order-by-to-many-m2m-field.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@query-o2m-lateral.json.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
query-compiler/core/src/query_graph_builder/extractors/query_arguments.rsquery-compiler/query-builders/sql-query-builder/src/cursor_condition.rsquery-compiler/query-builders/sql-query-builder/src/ordering.rsquery-compiler/query-builders/sql-query-builder/src/select/mod.rsquery-compiler/query-compiler/tests/data/order-by-to-many-1m-field-nulls.jsonquery-compiler/query-compiler/tests/data/order-by-to-many-1m-field.jsonquery-compiler/query-compiler/tests/data/order-by-to-many-m2m-field.jsonquery-compiler/query-structure/src/order_by.rsquery-compiler/query-structure/src/record.rsquery-compiler/schema/src/build/input_types/objects/order_by_objects.rsquery-compiler/schema/src/identifier_type.rs
DanielFigueiredoCiandt
force-pushed
the
feat/order-by-to-many-field
branch
from
083b91a to
0c363f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
query-compiler/core/src/query_graph_builder/extractors/query_arguments.rs (1)
119-146: 🧹 Nitpick | 🔵 TrivialScalar fallback for to-many
orderBylooks correct.Parsing structure is consistent with the composite to-many branch below. A couple of small notes:
- Line 125 silently keeps only the first entry of the inner object (
.into_iter().next().unwrap()), as elsewhere in this function. Schema validation should guarantee a single-key object, but consider asserting that (e.g.,debug_assert!(object.len() <= 1)) to surface schema drift early.- Line 143's
unreachable!is fragile if the schema builder ever starts exposing non-scalar inner fields (e.g., nested relations under a to-manyorderBy). Returning aQueryGraphBuilderError::InputErrorwould be more resilient, though the current pattern is consistent with the rest of the file.Neither is blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@query-compiler/core/src/query_graph_builder/extractors/query_arguments.rs` around lines 119 - 146, In the Field::Relation(rf) if rf.is_list() arm, add a defensive debug_assert that the parsed inner object contains at most one entry (e.g., debug_assert!(object.len() <= 1)) immediately after let object: ParsedInputMap<'_> = field_value.try_into()? to catch schema drift early; also replace the fragile unreachable! in the match on related_field (currently used after finding Field::Scalar(sf)) with a proper QueryGraphBuilderError::InputError return (include a clear message referencing inner_field_name and rf) so that non-scalar inner fields yield a graceful input error instead of panicking. Ensure you use the same error type used elsewhere in this module when constructing the InputError and preserve existing behavior for the scalar case using OrderBy::to_many_field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@query-compiler/query-builders/sql-query-builder/src/ordering.rs`:
- Around line 193-219: The intermediary join loop duplicates logic already in
self.compute_one2m_join; replace the for-loop that builds intermediary_joins
(which uses compute_one2m_join from crate::join_utils) with a single call to
self.compute_one2m_join(...) so it returns the same Vec<AliasedJoin> and
preserves the parent_alias fallback behavior; ensure the returned
intermediary_joins and context_alias computation still use the resulting Vec
(last().map(|j| j.alias.clone()).or_else(|| self.parent_alias.clone())) and then
pass context_alias into the existing m2m/one2m subquery builders
(build_m2m_correlated_subquery / build_one2m_correlated_subquery).
In `@query-compiler/schema/src/identifier_type.rs`:
- Around line 37-41: The change introduces a new enum variant
OrderByToManyScalarFieldsInput (used when ParentContainer::Model(_)) which
renames emitted DMMF types from “…OrderByRelationAggregateInput” to
“…OrderByRelationInput”, while ParentContainer::CompositeType(_) still emits
OrderByToManyAggregateInput → “…OrderByCompositeAggregateInput”, causing an
inconsistent naming scheme and a potential breaking rename for client
generators; update the selection logic in order_by_objects.rs (and any places
constructing DMMF type names) to either restore the original model suffix or
unify both model/composite naming conventions, and ensure the prisma-client/TS
generators are updated to consume the new name (or keep the old name), plus add
a changelog/pr description note calling out the DMMF rename so downstream
codegen/tooling can be adjusted.
---
Outside diff comments:
In `@query-compiler/core/src/query_graph_builder/extractors/query_arguments.rs`:
- Around line 119-146: In the Field::Relation(rf) if rf.is_list() arm, add a
defensive debug_assert that the parsed inner object contains at most one entry
(e.g., debug_assert!(object.len() <= 1)) immediately after let object:
ParsedInputMap<'_> = field_value.try_into()? to catch schema drift early; also
replace the fragile unreachable! in the match on related_field (currently used
after finding Field::Scalar(sf)) with a proper
QueryGraphBuilderError::InputError return (include a clear message referencing
inner_field_name and rf) so that non-scalar inner fields yield a graceful input
error instead of panicking. Ensure you use the same error type used elsewhere in
this module when constructing the InputError and preserve existing behavior for
the scalar case using OrderBy::to_many_field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0f35529d-6ddd-4013-ab0a-0358c40b4264
⛔ Files ignored due to path filters (9)
query-compiler/query-compiler/tests/snapshots/queries__queries@nested-distinct-incompatible-orderby-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@nested-distinct-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@nested-distinct-orderby-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@nested-distinct-pagination-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@nested-pagination-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@order-by-to-many-1m-field-nulls.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@order-by-to-many-1m-field.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@order-by-to-many-m2m-field.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@query-o2m-lateral.json.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
query-compiler/core/src/query_graph_builder/extractors/query_arguments.rsquery-compiler/query-builders/sql-query-builder/src/cursor_condition.rsquery-compiler/query-builders/sql-query-builder/src/ordering.rsquery-compiler/query-builders/sql-query-builder/src/select/mod.rsquery-compiler/query-compiler/tests/data/order-by-to-many-1m-field-nulls.jsonquery-compiler/query-compiler/tests/data/order-by-to-many-1m-field.jsonquery-compiler/query-compiler/tests/data/order-by-to-many-m2m-field.jsonquery-compiler/query-structure/src/order_by.rsquery-compiler/query-structure/src/record.rsquery-compiler/schema/src/build/input_types/objects/order_by_objects.rsquery-compiler/schema/src/identifier_type.rs
DanielFigueiredoCiandt
force-pushed
the
feat/order-by-to-many-field
branch
3 times, most recently
from
e0eb1da to
56b5bc9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@query-compiler/query-builders/sql-query-builder/src/ordering.rs`:
- Around line 251-253: The subquery currently selects from the related model
table directly (Select::from_table(rf.related_model().as_table(ctx))) which
causes self-relation root queries to bind parent-column references to the inner
table; fix by aliasing the inner/child table (create a table alias for
rf.related_model().as_table(ctx) and use that alias in Select::from_table) and
then qualify field_col and any related-side columns with that alias so
comparisons reference the aliased inner table. Apply the same pattern in
build_m2m_correlated_subquery: alias child_model.as_table(ctx) and qualify
field_col plus child_ids.as_columns(ctx) with that child alias before joining
the junction table so the correlated predicates remain bound to the outer row.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e30be4cb-d5d0-4e35-9202-916ea3571d1c
⛔ Files ignored due to path filters (9)
query-compiler/query-compiler/tests/snapshots/queries__queries@nested-distinct-incompatible-orderby-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@nested-distinct-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@nested-distinct-orderby-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@nested-distinct-pagination-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@nested-pagination-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@order-by-to-many-1m-field-nulls.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@order-by-to-many-1m-field.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@order-by-to-many-m2m-field.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@query-o2m-lateral.json.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
query-compiler/core/src/query_graph_builder/extractors/query_arguments.rsquery-compiler/query-builders/sql-query-builder/src/cursor_condition.rsquery-compiler/query-builders/sql-query-builder/src/ordering.rsquery-compiler/query-builders/sql-query-builder/src/select/mod.rsquery-compiler/query-compiler/tests/data/order-by-to-many-1m-field-nulls.jsonquery-compiler/query-compiler/tests/data/order-by-to-many-1m-field.jsonquery-compiler/query-compiler/tests/data/order-by-to-many-m2m-field.jsonquery-compiler/query-structure/src/order_by.rsquery-compiler/query-structure/src/record.rsquery-compiler/schema/src/build/input_types/objects/order_by_objects.rsquery-compiler/schema/src/identifier_type.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
query-compiler/query-builders/sql-query-builder/src/ordering.rs (1)
234-253:⚠️ Potential issue | 🟠 MajorAlias the inner related table to preserve correlated predicates.
For root self-relations, the subquery selects from the same table as the outer query. Without aliasing the inner related/child table and qualifying related-side columns, predicates can bind to the inner table instead of the outer row, turning the correlation into an inner-table comparison. Apply the alias to
related_col,field_col, and M2Mchild_ids.🐛 Proposed fix
) -> Expression<'static> { + let related_alias = self.join_prefix(); let (left_fields, right_fields) = if rf.is_inlined_on_enclosing_model() { // FK is on the parent model side. (rf.scalar_fields(), rf.referenced_fields()) @@ .zip(right_fields.iter()) .map(|(left, right)| { let parent_col = left.as_column(ctx).opt_table(context_alias.clone()); - let related_col = right.as_column(ctx); + let related_col = right.as_column(ctx).opt_table(Some(related_alias.clone())); parent_col.equals(related_col).into() }) .collect(); - let field_col = field.as_column(ctx); + let field_col = field.as_column(ctx).opt_table(Some(related_alias.clone())); @@ - let subquery = Select::from_table(rf.related_model().as_table(ctx)) + let subquery = Select::from_table(rf.related_model().as_table(ctx).alias(related_alias)) .column(field_col) .so_that(ConditionTree::And(conditions)) .order_by(inner_order_def) .limit(1); @@ ) -> Expression<'static> { + let child_alias = self.join_prefix(); let m2m_table = rf.as_table(ctx); @@ let left_join_conditions: Vec<Expression<'static>> = child_ids .as_columns(ctx) - .map(|c| c.equals(m2m_child_col.clone()).into()) + .map(|c| { + c.opt_table(Some(child_alias.clone())) + .equals(m2m_child_col.clone()) + .into() + }) .collect(); - let field_col = field.as_column(ctx); + let field_col = field.as_column(ctx).opt_table(Some(child_alias.clone())); @@ - let subquery = Select::from_table(child_model.as_table(ctx)) + let subquery = Select::from_table(child_model.as_table(ctx).alias(child_alias)) .column(field_col) .so_that(ConditionTree::And(junction_conditions)) .inner_join(m2m_table.on(ConditionTree::And(left_join_conditions)))Also applies to: 296-313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@query-compiler/query-builders/sql-query-builder/src/ordering.rs` around lines 234 - 253, The subquery in Select::from_table(rf.related_model().as_table(ctx)) can resolve columns against the inner table for root self-relations, breaking correlation; fix by aliasing the inner related table (use an alias when calling as_table for the subquery) and qualifying all column expressions coming from the related side: ensure right.as_column(ctx), field.as_column(ctx), and any M2M child_ids are wrapped or transformed to use the subquery alias (e.g., produce related_col and field_col that call opt_table with the subquery alias) so that parent_col (left.as_column(ctx).opt_table(context_alias)) remains correlated to the outer row and predicates bind correctly to the outer columns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@query-compiler/schema/src/build/input_types/objects/order_by_objects.rs`:
- Around line 219-259: The current loop in
order_by_to_many_aggregate_object_type exposes every ModelField::Scalar (in the
for field in container.fields() block) including non-orderable types like
Json/Bytes; update that loop to skip non-orderable scalar types by checking the
scalar field's type (e.g., use sf.type_identifier() and skip
TypeIdentifier::Json and TypeIdentifier::Bytes) or, if available, call a helper
like sf.is_orderable() before constructing the input field so only orderable
scalar fields get added to fields.
---
Duplicate comments:
In `@query-compiler/query-builders/sql-query-builder/src/ordering.rs`:
- Around line 234-253: The subquery in
Select::from_table(rf.related_model().as_table(ctx)) can resolve columns against
the inner table for root self-relations, breaking correlation; fix by aliasing
the inner related table (use an alias when calling as_table for the subquery)
and qualifying all column expressions coming from the related side: ensure
right.as_column(ctx), field.as_column(ctx), and any M2M child_ids are wrapped or
transformed to use the subquery alias (e.g., produce related_col and field_col
that call opt_table with the subquery alias) so that parent_col
(left.as_column(ctx).opt_table(context_alias)) remains correlated to the outer
row and predicates bind correctly to the outer columns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: be99e9fe-2c63-442c-90ce-add8775c6fc3
⛔ Files ignored due to path filters (9)
query-compiler/query-compiler/tests/snapshots/queries__queries@nested-distinct-incompatible-orderby-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@nested-distinct-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@nested-distinct-orderby-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@nested-distinct-pagination-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@nested-pagination-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@order-by-to-many-1m-field-nulls.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@order-by-to-many-1m-field.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@order-by-to-many-m2m-field.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@query-o2m-lateral.json.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
query-compiler/core/src/query_graph_builder/extractors/query_arguments.rsquery-compiler/query-builders/sql-query-builder/src/cursor_condition.rsquery-compiler/query-builders/sql-query-builder/src/ordering.rsquery-compiler/query-builders/sql-query-builder/src/select/mod.rsquery-compiler/query-compiler/tests/data/order-by-to-many-1m-field-nulls.jsonquery-compiler/query-compiler/tests/data/order-by-to-many-1m-field.jsonquery-compiler/query-compiler/tests/data/order-by-to-many-m2m-field.jsonquery-compiler/query-structure/src/order_by.rsquery-compiler/query-structure/src/record.rsquery-compiler/schema/src/build/input_types/objects/order_by_objects.rsquery-compiler/schema/src/identifier_type.rs
DanielFigueiredoCiandt
force-pushed
the
feat/order-by-to-many-field
branch
2 times, most recently
from
4562973 to
ecfdfb3
Compare
Implement ordering by scalar fields of to-many (1:m and m:n) relations
using correlated subqueries. The correlated subquery selects the field
value from the first matching related record (LIMIT 1), naturally
producing NULL when no related records exist.
Given the following schema:
model Item {
id Int @id
localization ItemI18n[]
}
model ItemI18n {
id Int @id
name String
itemId Int
item Item @relation(fields: [itemId], references: [id])
}
This PR enables:
prisma.item.findMany({ orderBy: { localization: { name: 'asc' } } })
prisma.item.findMany({ orderBy: { localization: { name: { sort: 'asc', nulls: 'first' } } } })
which was previously limited to aggregate-only ordering (`_count`).
Changes:
- order_by.rs: add `OrderByToManyField` variant and `OrderBy::to_many_field` constructor
- order_by_objects.rs: expose scalar fields in `*OrderByRelationAggregateInput` DMMF type
- query_arguments.rs: parse new scalar field name inside a to-many relation orderBy
- ordering.rs: generate correlated sub-SELECT for 1:m and m:n relations
- cursor_condition.rs: handle `ToManyField` in cursor-based pagination
- select/mod.rs: skip join building for `ToManyField` (subquery is self-contained)
- record.rs: extract field value for `ToManyField` in sort record comparison
Fixes prisma/prisma#5837
DanielFigueiredoCiandt
force-pushed
the
feat/order-by-to-many-field
branch
from
ecfdfb3 to
4574f5b
Compare
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
query-compiler/query-builders/sql-query-builder/src/ordering.rs (1)
188-206:⚠️ Potential issue | 🔴 CriticalQualify the outer parent columns when there is no join alias.
context_aliasisNonefor root-level to-many ordering, so Lines 243 and 298 emit unqualified parent columns inside the subquery. Those can bind to the inner aliased related table instead of the outer row, e.g.child.itemId = idresolvingidfrom the child table. Use an explicit outer table qualifier/alias as the fallback before building the one-to-many and many-to-many correlation predicates.🐛 Proposed direction
- let context_alias = intermediary_joins.last().map(|j| j.alias.clone()).or(parent_alias); + let context_alias = intermediary_joins + .last() + .map(|j| j.alias.clone()) + .or(parent_alias) + // Fallback must still qualify the outer/root table; leaving this as + // `None` makes parent columns unqualified inside the correlated subquery. + .or_else(|| Some(to_many_hop.model().db_name().to_owned()));If
db_name()is not the right table-identifier API here, use the existing model/table helper that renders the same qualifier used by the outerFROM.Also applies to: 238-245, 294-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@query-compiler/query-builders/sql-query-builder/src/ordering.rs` around lines 188 - 206, compute_subquery_for_to_many_field can pass a None context_alias for root-level to-many ordering, causing unqualified parent columns to be emitted and accidentally bind to inner tables; fix it by ensuring context_alias is never None before calling build_m2m_correlated_subquery or build_one2m_correlated_subquery: if intermediary_joins.last() and parent_alias are both None, supply an explicit outer table qualifier (the same qualifier used in the outer FROM — e.g., render the model/table identifier via the existing helper such as db_name() or the code that renders the outer table) and pass that Some(qualifier) into context_alias so the correlation predicates use the outer table qualifier instead of leaving columns unqualified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@query-compiler/query-builders/sql-query-builder/src/cursor_condition.rs`:
- Line 445: cursor_order_def_to_many_field currently forces nullable in the
produced CursorOrderDefinition and map_orderby_condition then always emits "OR
<order> IS NULL"; instead propagate the original OrderBy's nulls_order into
CursorOrderDefinition (from OrderBy::ToManyField) and use that flag in
map_orderby_condition to only emit the NULL predicate when NULLs are on the same
side as the requested cursor direction (i.e., when nulls sort before and cursor
is a "before"/>"asc" case or when nulls sort after and cursor is a
"after"/"desc" case). Update cursor_order_def_to_many_field to set a nulls flag
from order_by.nulls_order and adjust map_orderby_condition to conditionally
include the "IS NULL" clause based on that flag; apply the same change to the
other to-many handling block referenced in the diff.
In `@query-compiler/schema/src/build/input_types/objects/order_by_objects.rs`:
- Around line 223-228: The code currently selects
IdentifierType::OrderByToManyScalarFieldsInput for model relations which changes
the public API; instead, always use IdentifierType::OrderByToManyAggregateInput
in the Identifier::new_prisma call (for both ParentContainer::Model and other
containers) and, when container is ParentContainer::Model, add the scalar-field
rendering into that same OrderByToManyAggregateInput implementation so scalar
fields are included without introducing a new identifier type; update usage in
the match at the identifier creation site (the container/ident logic) and adjust
identifier_type.rs to append scalar fields to OrderByToManyAggregateInput for
model relations.
---
Duplicate comments:
In `@query-compiler/query-builders/sql-query-builder/src/ordering.rs`:
- Around line 188-206: compute_subquery_for_to_many_field can pass a None
context_alias for root-level to-many ordering, causing unqualified parent
columns to be emitted and accidentally bind to inner tables; fix it by ensuring
context_alias is never None before calling build_m2m_correlated_subquery or
build_one2m_correlated_subquery: if intermediary_joins.last() and parent_alias
are both None, supply an explicit outer table qualifier (the same qualifier used
in the outer FROM — e.g., render the model/table identifier via the existing
helper such as db_name() or the code that renders the outer table) and pass that
Some(qualifier) into context_alias so the correlation predicates use the outer
table qualifier instead of leaving columns unqualified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 25f10e14-5f28-4834-b4e0-af1e3202772c
⛔ Files ignored due to path filters (9)
query-compiler/query-compiler/tests/snapshots/queries__queries@nested-distinct-incompatible-orderby-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@nested-distinct-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@nested-distinct-orderby-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@nested-distinct-pagination-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@nested-pagination-join.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@order-by-to-many-1m-field-nulls.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@order-by-to-many-1m-field.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@order-by-to-many-m2m-field.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@query-o2m-lateral.json.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
query-compiler/core/src/query_graph_builder/extractors/query_arguments.rsquery-compiler/query-builders/sql-query-builder/src/cursor_condition.rsquery-compiler/query-builders/sql-query-builder/src/ordering.rsquery-compiler/query-builders/sql-query-builder/src/select/mod.rsquery-compiler/query-compiler/tests/data/order-by-to-many-1m-field-nulls.jsonquery-compiler/query-compiler/tests/data/order-by-to-many-1m-field.jsonquery-compiler/query-compiler/tests/data/order-by-to-many-m2m-field.jsonquery-compiler/query-structure/src/order_by.rsquery-compiler/query-structure/src/record.rsquery-compiler/schema/src/build/input_types/objects/order_by_objects.rsquery-compiler/schema/src/identifier_type.rs
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
query-compiler/schema/src/build/input_types/objects/order_by_objects.rs (1)
223-228:⚠️ Potential issue | 🟠 MajorStill a breaking public DMMF/Client API change — and it contradicts the PR summary.
The
ParentContainer::Model(_)branch still selectsIdentifierType::OrderByToManyScalarFieldsInput, which peridentifier_type.rsrenders as{Model}OrderByRelationInputinstead of the previously published{Model}OrderByRelationAggregateInput. That renames the existing generated input type for every model with a to-many relation, which is observable via DMMF and generated client typings.Note this also diverges from this PR's own summary, which says the change "expose[s] scalar fields in *OrderByRelationAggregateInput DMMF type" — i.e. the intent was to extend the existing type, not rename it.
Preserve the public name by reusing
OrderByToManyAggregateInputfor both branches (or by havingOrderByToManyScalarFieldsInputrender to{Model}OrderByRelationAggregateInput) and just append the scalar-field inputs for the Model case.♻️ Suggested direction
- let ident = match container { - ParentContainer::Model(_) => { - Identifier::new_prisma(IdentifierType::OrderByToManyScalarFieldsInput(container.clone())) - } - _ => Identifier::new_prisma(IdentifierType::OrderByToManyAggregateInput(container.clone())), - }; + // Keep the public name `{Model}OrderByRelationAggregateInput` stable; for Model + // containers we just enrich its field set with per-scalar-field ordering below. + let ident = Identifier::new_prisma(IdentifierType::OrderByToManyAggregateInput(container.clone()));If the dedicated
OrderByToManyScalarFieldsInputvariant is kept for internal dispatch reasons, adjust itsDisplayimpl inidentifier_type.rsto also render as{}OrderByRelationAggregateInput.#!/bin/bash # Confirm the two identifier variants render to different public names and locate any snapshot/schema # tests that would capture the rename. rg -nP --type=rust -C2 'OrderByToManyScalarFieldsInput|OrderByToManyAggregateInput' echo '---' rg -nP -C2 'OrderByRelationAggregateInput|OrderByRelationInput' --glob '!**/target/**'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@query-compiler/schema/src/build/input_types/objects/order_by_objects.rs` around lines 223 - 228, The change accidentally renames the public DMMF/Client type by selecting IdentifierType::OrderByToManyScalarFieldsInput for ParentContainer::Model in the ParentContainer::Model branch, which renders to {Model}OrderByRelationInput instead of the existing {Model}OrderByRelationAggregateInput; to fix, preserve the public name by using IdentifierType::OrderByToManyAggregateInput for the Model branch (or keep OrderByToManyScalarFieldsInput but change its Display/formatting in identifier_type.rs to render as "{Model}OrderByRelationAggregateInput") and ensure the scalar-field inputs are appended to that aggregate type rather than replacing it (update the logic around ParentContainer::Model, Identifier::new_prisma, and the Display impl for OrderByToManyScalarFieldsInput accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@query-compiler/schema/src/build/input_types/objects/order_by_objects.rs`:
- Line 219: The function order_by_to_many_aggregate_object_type now does more
than aggregate inputs (it also emits per-scalar-field ordering for
ParentContainer::Model) so rename it to order_by_to_many_object_type to reflect
actual responsibility and update all call sites accordingly; optionally split
the implementation into two helpers (e.g., order_by_to_many_model_object_type
and order_by_to_many_composite_object_type) and call the appropriate helper
based on ParentContainer to keep code readable, ensuring function signatures and
any referenced symbols (order_by_to_many_aggregate_object_type,
ParentContainer::Model) are updated throughout the module.
---
Duplicate comments:
In `@query-compiler/schema/src/build/input_types/objects/order_by_objects.rs`:
- Around line 223-228: The change accidentally renames the public DMMF/Client
type by selecting IdentifierType::OrderByToManyScalarFieldsInput for
ParentContainer::Model in the ParentContainer::Model branch, which renders to
{Model}OrderByRelationInput instead of the existing
{Model}OrderByRelationAggregateInput; to fix, preserve the public name by using
IdentifierType::OrderByToManyAggregateInput for the Model branch (or keep
OrderByToManyScalarFieldsInput but change its Display/formatting in
identifier_type.rs to render as "{Model}OrderByRelationAggregateInput") and
ensure the scalar-field inputs are appended to that aggregate type rather than
replacing it (update the logic around ParentContainer::Model,
Identifier::new_prisma, and the Display impl for OrderByToManyScalarFieldsInput
accordingly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5d9acf10-1a16-4ee5-ad1f-f9fdfdea31e8
📒 Files selected for processing (1)
query-compiler/schema/src/build/input_types/objects/order_by_objects.rs
…_order in cursor, rename helper
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 (1)
query-compiler/query-builders/sql-query-builder/src/cursor_condition.rs (1)
481-487:⚠️ Potential issue | 🟠 MajorPropagate scalar
nulls_orderinto cursor definitions.Line 486 leaves scalar orderBy cursor handling in the conservative
Nonepath, so existing queries likeorderBy: { nullableField: { sort: "asc", nulls: "first" } }can still include NULL rows on the wrong side of the cursor. TheOrderByScalarstruct carriesnulls_order, and the parallelcursor_order_def_to_many_fieldfunction already propagates it at line 564. Apply the same pattern here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@query-compiler/query-builders/sql-query-builder/src/cursor_condition.rs` around lines 481 - 487, The CursorOrderDefinition currently sets nulls_order to None; instead propagate the scalar OrderBy nulls setting by assigning the same nulls_order used for scalar orderBys (e.g. use order_by.nulls_order or order_by.nulls_order.clone()) when building the CursorOrderDefinition in this branch so it matches the behavior of cursor_order_def_to_many_field; update the assignment of nulls_order in the CursorOrderDefinition construction to use the OrderByScalar's nulls_order and ensure any necessary cloning or mapping is applied to match types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@query-compiler/query-builders/sql-query-builder/src/cursor_condition.rs`:
- Around line 363-378: The current logic unconditionally ORs cloned_cmp_column
IS NULL (via cloned_cmp_column.is_null()) which makes a NULL cursor value match
every candidate row; change the construction in the order_expression branch
(where order_definition.on_nullable_fields, include_nulls, order_expr,
cloned_order_column and cloned_cmp_column are used) to handle three explicit
cases: (1) row IS NULL & cursor non-NULL — include the row only when NULLs are
on the paginated side (use cloned_order_column.is_null() combined with
include_nulls), (2) row non-NULL & cursor IS NULL — include only when non-NULLs
are on the paginated side (negate the include_nulls logic and ensure
cloned_cmp_column.is_null() does not short-circuit), and (3) both NULL — treat
as equality/prefix-match (include for equality/lenient comparisons and prefix
equality). Apply the same split-logic to the prefix-equality path referenced
around the 412-427 region so NULL = NULL can match only in the both-NULL branch
and NULL cursor does not match all rows.
---
Outside diff comments:
In `@query-compiler/query-builders/sql-query-builder/src/cursor_condition.rs`:
- Around line 481-487: The CursorOrderDefinition currently sets nulls_order to
None; instead propagate the scalar OrderBy nulls setting by assigning the same
nulls_order used for scalar orderBys (e.g. use order_by.nulls_order or
order_by.nulls_order.clone()) when building the CursorOrderDefinition in this
branch so it matches the behavior of cursor_order_def_to_many_field; update the
assignment of nulls_order in the CursorOrderDefinition construction to use the
OrderByScalar's nulls_order and ensure any necessary cloning or mapping is
applied to match types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2fee84f3-f658-4b9d-bd90-af3bec1410e1
📒 Files selected for processing (2)
query-compiler/query-builders/sql-query-builder/src/cursor_condition.rsquery-compiler/schema/src/build/input_types/objects/order_by_objects.rs
…te scalar nulls_order
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
query-compiler/query-builders/sql-query-builder/src/cursor_condition.rs (1)
417-429:⚠️ Potential issue | 🟠 MajorApply explicit NULL placement to nullable relation FKs too.
Line 524 propagates
nulls_orderfor scalar relation orderings, but Lines 417-429 still append nullable FKIS NULLpredicates unconditionally. FororderBy: { author: { name: { sort: "asc", nulls: "first" } } }on an optionalauthor, rows withauthor_id IS NULLcan be included after a non-NULL cursor even though they sort before it; a NULL cursor can also miss non-NULL rows when the leaf scalar is required becauseon_nullable_fieldsstays false.🐛 Proposed fix
fn map_orderby_condition( order_subquery: &Select<'static>, order_definition: &CursorOrderDefinition, reverse: bool, @@ - if let Some(fks) = &order_definition.order_fks { + if order_definition.nulls_order.is_some() { + return order_expr; + } + + if let Some(fks) = &order_definition.order_fks { fks.iter() .filter(|fk| !fk.field.is_required()) .fold(order_expr, |acc, fk| { let col = if let Some(alias) = &fk.alias { @@ fn cursor_order_def_scalar(order_by: &OrderByScalar, order_by_def: &OrderByDefinition) -> CursorOrderDefinition { // If there are any ordering hops, this finds the foreign key fields for the _last_ hop (we look for the last one because the ordering is done the last one). // These fk fields are needed to check whether they are nullable // cf: part `#2` of the SQL query above, when a field is nullable. let fks = foreign_keys_from_order_path(&order_by.path, &order_by_def.joins); + let has_nullable_fk = fks + .as_ref() + .map_or(false, |fks| fks.iter().any(|fk| !fk.field.is_required())); CursorOrderDefinition { sort_order: order_by.sort_order, order_column: order_by_def.order_column.clone(), order_fks: fks, - on_nullable_fields: !order_by.field.is_required(), + on_nullable_fields: !order_by.field.is_required() || (order_by.nulls_order.is_some() && has_nullable_fk), nulls_order: order_by.nulls_order.clone(), } }Also applies to: 513-525
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@query-compiler/query-builders/sql-query-builder/src/cursor_condition.rs` around lines 417 - 429, The code unconditionally appends nullable-FK "IS NULL" predicates when folding over order_definition.order_fks (the closure that builds order_expr using Column::from / fk.field.as_column(ctx).is_null()), which ignores the requested nulls_order and fails to set on_nullable_fields correctly; change the fold to check the propagated nulls_order/nulls placement for this ordering and only OR-in the .is_null() predicate when the nulls placement requires NULLs to be ordered separately (and set on_nullable_fields = true when fk.field.is_required() is false and you add that predicate); apply the same conditional logic to the analogous block that handles relation FKs at the second site (the block referenced around lines 513-525).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@query-compiler/query-builders/sql-query-builder/src/cursor_condition.rs`:
- Around line 417-429: The code unconditionally appends nullable-FK "IS NULL"
predicates when folding over order_definition.order_fks (the closure that builds
order_expr using Column::from / fk.field.as_column(ctx).is_null()), which
ignores the requested nulls_order and fails to set on_nullable_fields correctly;
change the fold to check the propagated nulls_order/nulls placement for this
ordering and only OR-in the .is_null() predicate when the nulls placement
requires NULLs to be ordered separately (and set on_nullable_fields = true when
fk.field.is_required() is false and you add that predicate); apply the same
conditional logic to the analogous block that handles relation FKs at the second
site (the block referenced around lines 513-525).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba277778-4a7f-4762-9b3f-b9a8f6052453
📒 Files selected for processing (1)
query-compiler/query-builders/sql-query-builder/src/cursor_condition.rs
Author
|
@coderabbitai review again please |
|
✅ Actions performedFull review triggered. |
Author
|
@coderabbitai are you reviewed? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2765
Please check the inline comments and the review summaries already posted on the PR. Let me know if you have any questions or want to discuss any of the findings! 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
query-compiler/query-builders/sql-query-builder/src/cursor_condition.rs (1)
403-409:⚠️ Potential issue | 🟠 MajorNULL-cursor bug persists in the
Nonebranch and now affectsToManyField.When
nulls_orderisNoneand the cursor row's ordering value is NULL, the trailing.or(Expression::from(cloned_cmp_column).is_null())evaluates TRUE for every candidate row (the comparison subquery is constant for the cursor row), so the cursor predicate degrades to "match all" and pagination returns duplicated/reordered results.The
Some(_)branch addresses this by splitting row-null and cursor-null cases, but theNonebranch retains the legacy conservative OR. This path is now easy to hit from the new feature:OrderByToManyField.nulls_orderisOption<NullsOrder>, soorderBy: { localization: { name: 'asc' } }(without explicitnulls) routes throughcursor_order_def_to_many_field→nulls_order: None→ this branch, and the correlated LIMIT-1 subquery legitimately returns NULL whenever the cursor's record has no related rows.The same issue applies to
map_equality_condition(lines 468-476): the prefix-equality OR-ing ofcmp IS NULL/order IS NULLmakesNULLprefixes match any row, breaking multi-field cursors.Consider applying the split-case treatment (row NULL / cursor NULL / both NULL) in the
Nonebranch too, picking a default placement based on connector semantics (e.g., Postgres: ASC ⇒ NULLS LAST, DESC ⇒ NULLS FIRST; MySQL/SQLite: opposite), or at minimum treating the cursor-NULL case symmetrically so a NULL cursor does not match every candidate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@query-compiler/query-builders/sql-query-builder/src/cursor_condition.rs` around lines 403 - 409, The None branch currently ORs on the candidate's cmp-column being NULL which makes a NULL cursor value match all rows; update the None branch in cursor_condition.rs to mirror the Some(_) split-case logic: explicitly handle (a) row-order NULL, (b) cursor-order NULL, and (c) both NULL instead of the single conservative OR, so that a NULL cursor value does not universally match candidates—use the same pattern used in the Some branch to produce separate predicates for row-null vs cursor-null (and combine them appropriately), and apply the same fix to map_equality_condition to remove the unconditional ".or(...cmp_column.is_null())" style OR and implement symmetric cursor-null handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@query-compiler/query-builders/sql-query-builder/src/cursor_condition.rs`:
- Around line 363-401: The code eagerly constructs row_null_cursor_not,
row_not_cursor_null, and both_null even when they may not be used; to avoid
unnecessary allocations, move the construction of each Expression into the
branch that needs it: build row_null_cursor_not only inside the if include_nulls
block, build row_not_cursor_null only inside the if !include_nulls block, and
build both_null only inside the if include_eq block, updating result (the
Expression<'static> named result) accordingly; keep the existing logic and
variable names (cloned_order_column, cloned_cmp_column, include_nulls,
include_eq, result) so the change is localized and types remain the same.
---
Duplicate comments:
In `@query-compiler/query-builders/sql-query-builder/src/cursor_condition.rs`:
- Around line 403-409: The None branch currently ORs on the candidate's
cmp-column being NULL which makes a NULL cursor value match all rows; update the
None branch in cursor_condition.rs to mirror the Some(_) split-case logic:
explicitly handle (a) row-order NULL, (b) cursor-order NULL, and (c) both NULL
instead of the single conservative OR, so that a NULL cursor value does not
universally match candidates—use the same pattern used in the Some branch to
produce separate predicates for row-null vs cursor-null (and combine them
appropriately), and apply the same fix to map_equality_condition to remove the
unconditional ".or(...cmp_column.is_null())" style OR and implement symmetric
cursor-null handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 70ddc299-c4b7-4f0d-ab9e-6b6fddbd70fa
📒 Files selected for processing (1)
query-compiler/query-builders/sql-query-builder/src/cursor_condition.rs
…locate expressions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implement ordering by scalar fields of to-many (1:m and m:n) relations using correlated subqueries. The correlated subquery selects the field value from the first matching related record (LIMIT 1), naturally producing NULL when no related records exist.
Given the following schema:
This PR enables:
prisma.item.findMany({ orderBy: { localization: { name: 'asc' } } }) prisma.item.findMany({ orderBy: { localization: { name: { sort: 'asc', nulls: 'first' } } } })which was previously limited to aggregate-only ordering (
_count).Changes:
OrderByToManyFieldvariant andOrderBy::to_many_fieldconstructor*OrderByRelationAggregateInputDMMF typeToManyFieldin cursor-based paginationToManyField(subquery is self-contained)ToManyFieldin sort record comparisonFixes prisma/prisma#5837