feat: support Meilisearch v1.18.0 (queryVector + index renaming) #2020
Conversation
|
Hello @Ritinpaul and thanks for your contribution 🙌 It appears the tests are failing. Can you look into it? |
WalkthroughAdds index renaming support and related examples/tests: updateIndex now accepts indexUid for renaming, swap-indexes supports rename operations, types gain optional uid in IndexOptions and queryVector in SearchResponse, tests cover rename flows, wait timeouts are configured in utils, and test runner timeouts are increased. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant SDK
participant Server as Meilisearch
rect rgb(236,248,255)
note right of App: Rename via updateIndex (PATCH)
App->>SDK: updateIndex('indexA', { indexUid: 'indexB' })
SDK->>Server: PATCH /indexes/indexA { indexUid: "indexB" }
Server-->>SDK: Task enqueued (type: indexUpdate/rename)
SDK-->>App: Task info
end
rect rgb(245,240,255)
note right of App: Rename via swap-indexes (POST) with rename=true
App->>SDK: swapIndexes([{ indexes: ['indexA','indexB'], rename: true }])
SDK->>Server: POST /swap-indexes [{ indexes: [...], rename: true }]
Server-->>SDK: Task enqueued (type: indexSwap)
SDK-->>App: Task info
end
sequenceDiagram
autonumber
participant App
participant SDK
participant Server as Meilisearch
rect rgb(240,255,244)
note right of App: Search with retrieveVectors
App->>SDK: index.search(q, { retrieveVectors: true })
SDK->>Server: POST /indexes/:uid/search { retrieveVectors: true }
Server-->>SDK: SearchResponse { hits, queryVector?, ... }
SDK-->>App: SearchResponse including optional queryVector
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 (2)
.code-samples.meilisearch.yaml (1)
34-36: Rename example is correct; add swapIndexes rename sample
ConfirmedupdateIndexusesindexUid(per types and tests). Add a sample illustratingclient.swapIndexes([{ indexes: ['INDEX_A','INDEX_B'], rename: true }])src/types/types.ts (1)
412-413: AddqueryVectorto SearchResponse: LGTMMatches the new v1.18 behavior (vector retrieval). Optional typing is appropriate and propagates through multi-search via
SearchResponse.Consider a brief JSDoc on
queryVectorindicating it is only present whenretrieveVectorsis true and the query can be embedded.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.code-samples.meilisearch.yaml(1 hunks)src/types/types.ts(2 hunks)tests/client.test.ts(1 hunks)tests/get_search.test.ts(2 hunks)tests/index.test.ts(1 hunks)tests/search.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/client.test.ts (4)
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)src/types/types.ts (2)
ErrorStatusCode(863-1223)ErrorStatusCode(1225-1226)
tests/index.test.ts (2)
src/types/types.ts (2)
ErrorStatusCode(863-1223)ErrorStatusCode(1225-1226)src/meilisearch.ts (1)
index(92-94)
🔇 Additional comments (6)
src/types/types.ts (1)
142-145: Index rename payload key: confirm server contract usesindexUidChange looks correct for the SDK surface, but the Meilisearch PATCH payload for index rename must accept
indexUid. If the server expectsuidinstead, this will break at runtime.
- Please confirm against v1.18 API that the field name is
indexUid.- If both
uidandindexUidmight exist across versions, consider accepting both and normalizing before sending.tests/search.test.ts (2)
1196-1200: AssertingqueryVectorwhenretrieveVectorsis true: looks goodMatches the new API surface. Ensure the test environment runs against Meilisearch v1.18 and that the index embeds queries so
queryVectoris actually returned.
1207-1211: Absence ofqueryVectorwithoutretrieveVectors: looks goodThis aligns with the conditional exposure of
queryVector.tests/index.test.ts (1)
295-317: Index rename via update: LGTMSolid coverage: rename via
updateIndex(..., { indexUid }), then 404 on old UID and success on new UID.tests/get_search.test.ts (2)
520-524: Presence ofqueryVectoron GET whenretrieveVectorsis true: looks goodConsistent with the types change. Verify CI uses v1.18 and the index is configured so queries are embeddable.
531-535: Absence ofqueryVectorwithoutretrieveVectors: looks goodMatches expected behavior.
| 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 old indexes no longer exist | ||
| await expect(client.getIndex(originalUid1)).rejects.toHaveProperty( | ||
| "cause.code", | ||
| ErrorStatusCode.INDEX_NOT_FOUND, | ||
| ); | ||
| await expect(client.getIndex(originalUid2)).rejects.toHaveProperty( | ||
| "cause.code", | ||
| ErrorStatusCode.INDEX_NOT_FOUND, | ||
| ); | ||
|
|
||
| // Verify the new indexes exist with swapped content | ||
| const docIndex1 = await client.index(originalUid1).getDocument(1); | ||
| const docIndex2 = await client.index(originalUid2).getDocument(1); | ||
|
|
||
| expect(docIndex1.title).toEqual("index_2"); | ||
| expect(docIndex2.title).toEqual("index_1"); | ||
| expect(resolvedTask.type).toEqual("indexSwap"); | ||
| expect(resolvedTask.details?.swaps).toEqual(swaps); | ||
| }); |
There was a problem hiding this comment.
Test contradiction: asserting non-existence then reading from the same indexes
The test first asserts both original UIDs no longer exist, then immediately fetches documents from those same UIDs. Both cannot be true. With rename: true, UIDs are swapped, not deleted; the original UIDs should remain addressable, with swapped content.
Apply this diff to remove the invalid assertions and clarify the intent:
const resolvedTask = await client.swapIndexes(swaps).waitTask();
- // Verify the old indexes no longer exist
- await expect(client.getIndex(originalUid1)).rejects.toHaveProperty(
- "cause.code",
- ErrorStatusCode.INDEX_NOT_FOUND,
- );
- await expect(client.getIndex(originalUid2)).rejects.toHaveProperty(
- "cause.code",
- ErrorStatusCode.INDEX_NOT_FOUND,
- );
-
- // Verify the new indexes exist with swapped content
+ // Verify the indexes are accessible and content has been swapped under the same UIDs
const docIndex1 = await client.index(originalUid1).getDocument(1);
const docIndex2 = await client.index(originalUid2).getDocument(1);
expect(docIndex1.title).toEqual("index_2");
expect(docIndex2.title).toEqual("index_1");
expect(resolvedTask.type).toEqual("indexSwap");
expect(resolvedTask.details?.swaps).toEqual(swaps);📝 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 old indexes no longer exist | |
| await expect(client.getIndex(originalUid1)).rejects.toHaveProperty( | |
| "cause.code", | |
| ErrorStatusCode.INDEX_NOT_FOUND, | |
| ); | |
| await expect(client.getIndex(originalUid2)).rejects.toHaveProperty( | |
| "cause.code", | |
| ErrorStatusCode.INDEX_NOT_FOUND, | |
| ); | |
| // Verify the new indexes exist with swapped content | |
| const docIndex1 = await client.index(originalUid1).getDocument(1); | |
| const docIndex2 = await client.index(originalUid2).getDocument(1); | |
| expect(docIndex1.title).toEqual("index_2"); | |
| expect(docIndex2.title).toEqual("index_1"); | |
| expect(resolvedTask.type).toEqual("indexSwap"); | |
| expect(resolvedTask.details?.swaps).toEqual(swaps); | |
| }); | |
| const resolvedTask = await client.swapIndexes(swaps).waitTask(); | |
| // Verify the indexes are accessible and content has been swapped under the same UIDs | |
| const docIndex1 = await client.index(originalUid1).getDocument(1); | |
| const docIndex2 = await client.index(originalUid2).getDocument(1); | |
| expect(docIndex1.title).toEqual("index_2"); | |
| expect(docIndex2.title).toEqual("index_1"); | |
| expect(resolvedTask.type).toEqual("indexSwap"); | |
| expect(resolvedTask.details?.swaps).toEqual(swaps); |
🤖 Prompt for AI Agents
In tests/client.test.ts around lines 570 to 608, the test incorrectly asserts
that the original UIDs no longer exist then tries to read documents from those
same UIDs; with rename: true the UIDs remain addressable and their contents are
swapped. Remove the two expect(...) checks that assert INDEX_NOT_FOUND for
originalUid1 and originalUid2, keep the document fetches and assertions that
docIndex1.title === "index_2" and docIndex2.title === "index_1", and update the
surrounding comments to state that the UIDs remain and their contents are
swapped rather than deleted.
There was a problem hiding this comment.
@Ritinpaul what do you think of this?
I agree with the feedback; the intent is unclear. That being said, the tests are passing
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/utils/meilisearch-test-utils.ts (1)
19-20: DRY the repeated defaultWaitOptions into a constantReduces repetition and keeps future tweaks in one place.
Apply this diff for the highlighted lines, introducing a shared constant:
@@ -const config: Config = { +const DEFAULT_WAIT_OPTIONS = { interval: 10, timeout: 60_000 }; + +const config: Config = { host: HOST, apiKey: MASTER_KEY, - defaultWaitOptions: { interval: 10, timeout: 60_000 }, + defaultWaitOptions: DEFAULT_WAIT_OPTIONS, }; @@ const masterClient = new MeiliSearch({ host: HOST, apiKey: MASTER_KEY, - defaultWaitOptions: { interval: 10, timeout: 60_000 }, + defaultWaitOptions: DEFAULT_WAIT_OPTIONS, }); @@ const anonymousClient = new MeiliSearch({ host: HOST, - defaultWaitOptions: { interval: 10, timeout: 60_000 }, + defaultWaitOptions: DEFAULT_WAIT_OPTIONS, }); @@ const searchClient = new MeiliSearch({ host: HOST, apiKey: searchKey, - defaultWaitOptions: { interval: 10, timeout: 60_000 }, + defaultWaitOptions: DEFAULT_WAIT_OPTIONS, }); @@ const adminClient = new MeiliSearch({ host: HOST, apiKey: adminKey, - defaultWaitOptions: { interval: 10, timeout: 60_000 }, + defaultWaitOptions: DEFAULT_WAIT_OPTIONS, });Optionally also use the constant for the "No" permission client to keep behavior uniform.
Also applies to: 28-29, 33-34, 73-74, 84-85
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
package.json(1 hunks)src/types/types.ts(2 hunks)tests/client.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
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/client.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
tests/index.test.ts (2)
tests/utils/meilisearch-test-utils.ts (1)
getClient(369-369)src/types/types.ts (2)
ErrorStatusCode(863-1223)ErrorStatusCode(1225-1226)
🔇 Additional comments (3)
package.json (1)
46-46: Longer Vitest timeouts for hooks/tests: LGTMThis should reduce flakiness for rename/swap tests.
tests/utils/meilisearch-test-utils.ts (1)
19-20: Adding defaultWaitOptions with a 60s timeout: LGTMConsistent wait behavior across clients improves test reliability.
Also applies to: 28-29, 33-34, 73-74, 84-85
tests/index.test.ts (1)
295-314: Rename test via update: LGTMCovers happy path: create → rename → verify old uid 404 and new uid exists.
|
Hi @Strift , thanks for the review. I've addressed the failing tests. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2020 +/- ##
=======================================
Coverage 98.83% 98.83%
=======================================
Files 18 18
Lines 1549 1549
Branches 334 334
=======================================
Hits 1531 1531
Misses 18 18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Pull Request
Related issue
Fixes #2013
What does this PR do?
This PR updates the SDK to support the new features in Meilisearch v1.18:
queryVectorfield in search responses whenretrieveVectorsis usedindexUidparameter inupdateIndexqueryVectorand index renaming.code-samples.meilisearch.yamlThese changes keep everything backward compatible.
PR checklist
Please check if your PR fulfills the following requirements:
Thanks a lot for reviewing this PR!
Summary by CodeRabbit