feat: add support for sort parameter in documents API #2038
Conversation
WalkthroughAdds optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant SDK as SDK.getDocuments
participant MS as Meilisearch Server
App->>SDK: getDocuments(indexUid, params { limit, fields, sort?, ... })
rect rgba(220,235,245,0.35)
note right of SDK: normalize params into shallow copy
SDK->>SDK: normalizedParams = { ...params }
alt sort is empty array
SDK->>SDK: delete normalizedParams.sort
note right of SDK: omit empty sort to avoid server rejection
else
SDK->>SDK: keep normalizedParams.sort
end
end
alt POST /documents/fetch used
SDK->>MS: POST /indexes/:uid/documents/fetch body: normalizedParams
MS-->>SDK: 200 Documents (sorted if requested)
else GET /documents used
SDK->>MS: GET /indexes/:uid/documents?{normalizedParams}
MS-->>SDK: 200 Documents (sorted if requested)
end
SDK-->>App: Documents list
rect rgba(245,225,225,0.25)
note over MS,SDK: Error when sorting by non-sortable attribute
MS-->>SDK: 400 "Attribute `...` is not sortable"
SDK-->>App: Propagate error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (1)tests/utils/meilisearch-test-utils.ts (1)
🔇 Additional comments (1)
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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: 2
🧹 Nitpick comments (1)
tests/documents.test.ts (1)
292-301: Strengthen multi-field sort assertionWe’re aiming to verify forwarding of the entire
sortarray, yet this check only confirms the first entry (id:desc). Becauseidstays unique indataset, a regression that strips the secondary key (title:asc) would still pass. Please reshape the test data or expectations so the first sort key contains duplicates (e.g., sort primarily ongenre) and assert the order within those duplicates; that way the test actually fails if we drop the second key.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.code-samples.meilisearch.yaml(2 hunks)package.json(1 hunks)src/indexes.ts(1 hunks)src/types/types.ts(3 hunks)tests/client.test.ts(1 hunks)tests/documents.test.ts(1 hunks)tests/get_search.test.ts(0 hunks)tests/index.test.ts(1 hunks)tests/utils/meilisearch-test-utils.ts(4 hunks)
💤 Files with no reviewable changes (1)
- tests/get_search.test.ts
🧰 Additional context used
🧬 Code graph analysis (5)
tests/client.test.ts (3)
tests/utils/meilisearch-test-utils.ts (1)
getClient(369-369)src/meilisearch.ts (1)
index(92-94)src/types/task_and_batch.ts (1)
IndexSwap(109-112)
tests/index.test.ts (1)
src/types/types.ts (2)
ErrorStatusCode(864-1224)ErrorStatusCode(1226-1227)
tests/utils/meilisearch-test-utils.ts (1)
src/meilisearch.ts (1)
MeiliSearch(49-570)
tests/documents.test.ts (2)
tests/env/node/getting_started.cjs (1)
dataset(12-19)tests/utils/meilisearch-test-utils.ts (2)
dataset(372-372)Book(339-345)
src/indexes.ts (1)
src/types/types.ts (1)
ResourceResults(129-132)
.code-samples.meilisearch.yaml
Outdated
| rename_an_index_1: |- | ||
| client.updateIndex('INDEX_A', { indexUid: 'INDEX_B' }) |
There was a problem hiding this comment.
Fix the rename example to use the correct payload key
updateIndex expects the new identifier under uid, not indexUid. Shipping this sample as-is will make readers copy a snippet that the server rejects.(meilisearch.com)
- client.updateIndex('INDEX_A', { indexUid: 'INDEX_B' })
+ client.updateIndex('INDEX_A', { uid: 'INDEX_B' })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rename_an_index_1: |- | |
| client.updateIndex('INDEX_A', { indexUid: 'INDEX_B' }) | |
| rename_an_index_1: |- | |
| client.updateIndex('INDEX_A', { uid: 'INDEX_B' }) |
🤖 Prompt for AI Agents
In .code-samples.meilisearch.yaml around lines 34 to 35, the example uses an
incorrect payload key `indexUid` when calling client.updateIndex; the API
expects the new identifier under the `uid` key. Change the payload object to use
`uid: 'INDEX_B'` instead of `indexUid: 'INDEX_B'` so the sample matches the
server expectation.
| test(`${permission} key: Swap two indexes with rename`, async () => { | ||
| const client = await getClient(permission); | ||
| const originalUid1 = index.uid; | ||
| const originalUid2 = index2.uid; | ||
|
|
||
| await client | ||
| .index(originalUid1) | ||
| .addDocuments([{ id: 1, title: "index_1" }]) | ||
| .waitTask(); | ||
| await client | ||
| .index(originalUid2) | ||
| .addDocuments([{ id: 1, title: "index_2" }]) | ||
| .waitTask(); | ||
|
|
||
| const swaps: IndexSwap[] = [ | ||
| { indexes: [originalUid1, originalUid2], rename: true }, | ||
| ]; | ||
|
|
||
| const resolvedTask = await client.swapIndexes(swaps).waitTask(); | ||
|
|
||
| // Verify the swap task completed with expected details | ||
| expect(resolvedTask.type).toEqual("indexSwap"); | ||
| expect(resolvedTask.details?.swaps).toEqual(swaps); | ||
| }); |
There was a problem hiding this comment.
Rename swap test should assert a successful rename
We’re pre-populating both indexes and then calling swapIndexes with rename: true, even though the API expects the second UID to be unused when you opt into rename mode. If the server rejects the rename (e.g., with index_already_exists), the test still “passes” because we only read the task type/details and never assert success or the post-rename state. Let’s stop creating the destination index ahead of time and assert that the task succeeds and that the old UID is gone while the new UID now holds the payload.(meilisearch.com)
- await client
- .index(originalUid2)
- .addDocuments([{ id: 1, title: "index_2" }])
- .waitTask();
-
const swaps: IndexSwap[] = [
{ indexes: [originalUid1, originalUid2], rename: true },
];
const resolvedTask = await client.swapIndexes(swaps).waitTask();
- // Verify the swap task completed with expected details
- expect(resolvedTask.type).toEqual("indexSwap");
- expect(resolvedTask.details?.swaps).toEqual(swaps);
+ expect(resolvedTask.status).toEqual("succeeded");
+ expect(resolvedTask.type).toEqual("indexSwap");
+ expect(resolvedTask.details?.swaps).toEqual(swaps);
+ await expect(
+ client.index(originalUid1).getDocument(1),
+ ).rejects.toHaveProperty("cause.code", ErrorStatusCode.INDEX_NOT_FOUND);
+ const renamedDoc = await client.index(originalUid2).getDocument(1);
+ expect(renamedDoc.title).toEqual("index_1");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test(`${permission} key: Swap two indexes with rename`, async () => { | |
| const client = await getClient(permission); | |
| const originalUid1 = index.uid; | |
| const originalUid2 = index2.uid; | |
| await client | |
| .index(originalUid1) | |
| .addDocuments([{ id: 1, title: "index_1" }]) | |
| .waitTask(); | |
| await client | |
| .index(originalUid2) | |
| .addDocuments([{ id: 1, title: "index_2" }]) | |
| .waitTask(); | |
| const swaps: IndexSwap[] = [ | |
| { indexes: [originalUid1, originalUid2], rename: true }, | |
| ]; | |
| const resolvedTask = await client.swapIndexes(swaps).waitTask(); | |
| // Verify the swap task completed with expected details | |
| expect(resolvedTask.type).toEqual("indexSwap"); | |
| expect(resolvedTask.details?.swaps).toEqual(swaps); | |
| }); | |
| test(`${permission} key: Swap two indexes with rename`, async () => { | |
| const client = await getClient(permission); | |
| const originalUid1 = index.uid; | |
| const originalUid2 = index2.uid; | |
| // only populate the source index | |
| await client | |
| .index(originalUid1) | |
| .addDocuments([{ id: 1, title: "index_1" }]) | |
| .waitTask(); | |
| const swaps: IndexSwap[] = [ | |
| { indexes: [originalUid1, originalUid2], rename: true }, | |
| ]; | |
| const resolvedTask = await client.swapIndexes(swaps).waitTask(); | |
| expect(resolvedTask.status).toEqual("succeeded"); | |
| expect(resolvedTask.type).toEqual("indexSwap"); | |
| expect(resolvedTask.details?.swaps).toEqual(swaps); | |
| // originalUid1 should no longer exist | |
| await expect( | |
| client.index(originalUid1).getDocument(1), | |
| ).rejects.toHaveProperty("cause.code", ErrorStatusCode.INDEX_NOT_FOUND); | |
| // new index originalUid2 should now contain the documents from originalUid1 | |
| const renamedDoc = await client.index(originalUid2).getDocument(1); | |
| expect(renamedDoc.title).toEqual("index_1"); | |
| }); |
🤖 Prompt for AI Agents
In tests/client.test.ts around lines 570 to 593, the rename-swap test is
creating both source and destination indexes then calling swapIndexes with
rename:true which violates the API expectation and allows silent rejection;
remove the code that pre-creates/populates the destination index (originalUid2),
call swapIndexes with rename:true where originalUid2 is unused, await the task
and assert it succeeded (e.g., status 'succeeded'), then verify the old UID no
longer exists and the new UID contains the expected document (check the document
by id/title) to confirm the rename occurred.
Ritinpaul
force-pushed
the
feat/documents-api-sort
branch
from
7f161f0 to
1a1f07f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.code-samples.meilisearch.yaml(1 hunks)src/indexes.ts(1 hunks)src/types/types.ts(3 hunks)tests/documents.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .code-samples.meilisearch.yaml
- src/indexes.ts
- src/types/types.ts
🧰 Additional context used
🧬 Code graph analysis (1)
tests/documents.test.ts (2)
tests/env/node/getting_started.cjs (2)
client(4-7)dataset(12-19)tests/utils/meilisearch-test-utils.ts (3)
getClient(369-369)dataset(372-372)Book(339-345)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2038 +/- ##
=======================================
Coverage 98.83% 98.84%
=======================================
Files 18 18
Lines 1549 1557 +8
Branches 334 338 +4
=======================================
+ Hits 1531 1539 +8
Misses 18 18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Pull Request
Related issue
Fixes #1996
What does this PR do?
sortparameter in the documents API (index.search).SearchParamstype to includesort: string[].sortparameter is correctly forwarded to the Meilisearch server.sortoption.PR checklist
Please check if your PR fulfills the following requirements:
Thanks a lot for reviewing this PR!
Summary by CodeRabbit
New Features
Documentation
Tests
Chores