fix(orm): fixed orderBy in nested queries (includes/selects)#2518
fix(orm): fixed orderBy in nested queries (includes/selects)#2518eredzik wants to merge 4 commits intozenstackhq:devfrom
Conversation
…y aggregation - Updated `buildArrayAgg` method to accept an optional `orderBy` parameter for sorting. - Introduced `buildRelationOrderByExpressions` to handle orderBy logic for relations. - Added tests for stable ordering in nested includes and selects for both SQLite and PostgreSQL.
📝 WalkthroughWalkthroughThis PR threads relation-level ordering into lateral-join aggregation: it normalizes relation Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
tests/e2e/orm/client-api/relation/order-by-nested-includes.test.ts (1)
46-57: Consider adding orderBy test for nested comments.The
makeCommentsDatafunction creates comments linked to posts, but the test doesn't verify ordering of the nestedcommentsarray within each post. While the PR focuses on fixingpostsordering when nested includes are present, adding a test case for multi-level ordering (e.g.,comments: { orderBy: { content: 'asc' } }) could strengthen the regression coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/client-api/relation/order-by-nested-includes.test.ts` around lines 46 - 57, Add a new assertion or test case in the same spec that uses makeCommentsData to verify ordering of the nested comments include: when querying posts with include: { comments: { orderBy: { content: 'asc' } } } (or similar nested include used in the existing test), ensure each returned post's comments array is sorted by comment.content ascending; update or add expectations in the test file tests/e2e/orm/client-api/relation/order-by-nested-includes.test.ts referencing the query where posts are fetched (the test that exercises nested includes) and assert that for each post the comments list is ordered (e.g., compare sequence of comment.content or ids) to cover the regression for multi-level ordering while keeping makeCommentsData as the comment fixture.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/orm/src/client/crud/dialects/lateral-join-dialect-base.ts`:
- Around line 189-217: The MySQL dialect's buildArrayAgg implementation must be
updated to match the abstract signature in LateralJoinDialectBase: change the
method signature of buildArrayAgg in
packages/orm/src/client/crud/dialects/mysql.ts to accept the additional optional
orderBy parameter (orderBy?: { expr: Expression<any>; sort: SortOrder; nulls?:
NullsOrder }[]), and modify the method body to incorporate that orderBy when
building the ARRAY_AGG/JSON_ARRAYAGG expression (use orderBy to produce an ORDER
BY clause passed into the aggregate expression if provided); keep the return
type AliasableExpression<any> and ensure any internal references to
buildArrayAgg or calls to it still compile with the new optional parameter.
---
Nitpick comments:
In `@tests/e2e/orm/client-api/relation/order-by-nested-includes.test.ts`:
- Around line 46-57: Add a new assertion or test case in the same spec that uses
makeCommentsData to verify ordering of the nested comments include: when
querying posts with include: { comments: { orderBy: { content: 'asc' } } } (or
similar nested include used in the existing test), ensure each returned post's
comments array is sorted by comment.content ascending; update or add
expectations in the test file
tests/e2e/orm/client-api/relation/order-by-nested-includes.test.ts referencing
the query where posts are fetched (the test that exercises nested includes) and
assert that for each post the comments list is ordered (e.g., compare sequence
of comment.content or ids) to cover the regression for multi-level ordering
while keeping makeCommentsData as the comment fixture.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf40d0dc-2ece-4b23-a39a-3d0df92b3fae
📒 Files selected for processing (3)
packages/orm/src/client/crud/dialects/lateral-join-dialect-base.tspackages/orm/src/client/crud/dialects/postgresql.tstests/e2e/orm/client-api/relation/order-by-nested-includes.test.ts
ymc9
left a comment
ymc9
left a comment
There was a problem hiding this comment.
Thanks @eredzik .
I believe mysql needs some more fixes since the JSON_ARRAYAGG doesn't preserve input order either. I'll make a separate change for that.
I've also simplified the tests and removed the multi-provider matrix since the CI automatically do multiple runs for each provider. Will merge once CI passes and publish a patch release.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/e2e/orm/client-api/relation/order-by-nested-includes.test.ts (2)
64-71: Consider extracting common setup to reduce duplication.Both tests repeat identical setup: creating the client, user, posts, and comments. This could be extracted into a
beforeEachor a shared setup helper to reduce duplication while maintaining test isolation.♻️ Optional refactor to extract common setup
describe('Relation orderBy with nested includes', () => { let db: any; + const count = 2000; + + async function seedDatabase() { + db = await createTestClient(schema); + await db.user.create({ data: { id: 'u1', email: 'u1@example.com' } }); + await db.post.createMany({ data: makePostsData(count) }); + await db.comment.createMany({ data: makeCommentsData(count) }); + } afterEach(async () => { await db?.$disconnect(); }); it('keeps stable order for to-many include with nested includes', async () => { - const count = 2000; - - db = await createTestClient(schema); - - await db.user.create({ data: { id: 'u1', email: 'u1@example.com' } }); - await db.post.createMany({ data: makePostsData(count) }); - await db.comment.createMany({ data: makeCommentsData(count) }); + await seedDatabase(); const user = await db.user.findFirst({Also applies to: 100-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/client-api/relation/order-by-nested-includes.test.ts` around lines 64 - 71, Extract the repeated test setup (creating the TestClient via createTestClient(schema), creating user with id 'u1', and creating posts/comments via makePostsData and makeCommentsData) into a shared setup routine called from a beforeEach in the test file so each test begins with the same isolated state; specifically move the db = await createTestClient(schema) and the three create/createMany calls into a beforeEach block (or into a helper function invoked from beforeEach) and update the two tests (the one using count = 2000 and the other at lines ~100-107) to rely on that shared setup while keeping their local variations (like different count values) as needed.
77-77: Consider adding a test case for multi-column orderBy.The PostgreSQL dialect supports multi-column ordering in
buildArrayAgg(seepackages/orm/src/client/crud/dialects/postgresql.ts), but the current tests only exercise single-columnorderBy: { sequence: 'asc' | 'desc' }. Adding a test with multi-column ordering (e.g.,orderBy: [{ title: 'asc' }, { sequence: 'desc' }]) would improve coverage.Also applies to: 90-90, 114-114, 132-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/client-api/relation/order-by-nested-includes.test.ts` at line 77, Add new e2e test cases that exercise multi-column ordering where the current tests only use a single-column orderBy (the places using orderBy: { sequence: 'asc' }). For each of those occurrences, add a variant that passes an array for orderBy (for example orderBy: [{ title: 'asc' }, { sequence: 'desc' }]) and assert the resulting nested include ordering matches the combined sort; update the tests that reference the single-column orderBy occurrences so they include both the original single-column check and the new multi-column check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/orm/client-api/relation/order-by-nested-includes.test.ts`:
- Around line 64-71: Extract the repeated test setup (creating the TestClient
via createTestClient(schema), creating user with id 'u1', and creating
posts/comments via makePostsData and makeCommentsData) into a shared setup
routine called from a beforeEach in the test file so each test begins with the
same isolated state; specifically move the db = await createTestClient(schema)
and the three create/createMany calls into a beforeEach block (or into a helper
function invoked from beforeEach) and update the two tests (the one using count
= 2000 and the other at lines ~100-107) to rely on that shared setup while
keeping their local variations (like different count values) as needed.
- Line 77: Add new e2e test cases that exercise multi-column ordering where the
current tests only use a single-column orderBy (the places using orderBy: {
sequence: 'asc' }). For each of those occurrences, add a variant that passes an
array for orderBy (for example orderBy: [{ title: 'asc' }, { sequence: 'desc'
}]) and assert the resulting nested include ordering matches the combined sort;
update the tests that reference the single-column orderBy occurrences so they
include both the original single-column check and the new multi-column check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db2a44f1-ca9d-4d36-8d70-fd346acc7667
📒 Files selected for processing (1)
tests/e2e/orm/client-api/relation/order-by-nested-includes.test.ts
|
Thanks for cleaning tests up, I was not aware that this is set up in this way for CI. As for MySQL, when I ran MySQL tests on my PC with db in docker, tests returned correct ordering in both asc and desc cases with current version. I haven't ever used MySQL though so my knowledge there is limited unfortunately |
Same here, tests pass consistently, however mysql documentation seems to imply the order is not guaranteed, so may be one day someone will see a mismatch 🤷🏻♂️. I think it's safer to sort again post-read. |
2 participants
Summary
Fixes #2464
Test plan
keeps stable order for to-many include with nested includes — creates 10 (SQLite) / 2000 (PostgreSQL) posts with descending insert order, queries with orderBy: { sequence: 'asc' } and 'desc' while including nested author and comments.author relations. Asserts the returned sequence arrays match the expected sorted order.
keeps stable order for to-many select with nested selects — same scenario using select instead of include, verifying ordering is preserved through nested select clauses as well.
Summary by CodeRabbit
New Features
Compatibility
Tests