Add features of meilisearch 1.18 (queryVector and index renaming) #906
Conversation
WalkthroughAdds support for Meilisearch 1.18 features: introduces a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IndexesHandler
participant API
participant MockServer
rect rgb(235, 245, 255)
Note over Client,IndexesHandler: Rename index via updateIndex overload
Client->>Client: updateIndex("indexA", null, "indexB")
Client->>IndexesHandler: updateIndexUid("indexA","indexB")
IndexesHandler->>API: PATCH /indexes/indexA {"uid":"indexB"}
API->>MockServer: deliver request
MockServer-->>API: 202 { "taskUid":123 }
API-->>IndexesHandler: TaskInfo
IndexesHandler-->>Client: TaskInfo
end
rect rgb(255, 245, 235)
Note over Client,API: Rename via swap-indexes with rename=true
Client->>API: POST /swap-indexes [{"indexes":["A","B"],"rename":true}]
API->>MockServer: deliver request
MockServer-->>API: 202 { "taskUid":456 }
API-->>Client: TaskInfo
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🔇 Additional comments (3)
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: 3
🧹 Nitpick comments (5)
src/main/java/com/meilisearch/sdk/model/SearchResultPaginated.java (1)
26-26: Consider adding test coverage forqueryVectorin paginated resultsThe
queryVectorfield is consistent withSearchResultand will be exposed via Lombok getters, which is good. To avoid regressions, I’d add a small test that deserializes a paginated search response includingqueryVectorand asserts the values, similar toSearchResultQueryVectorTest.src/main/java/com/meilisearch/sdk/model/SwapIndexesParams.java (1)
13-13: Document the semantics of therenameflag on swap-indexes paramsThe
renameflag integrates nicely with the existing chained params and will serialize as"rename": truewhen set. To help users discover it, consider expanding the class-level or method-level docs (and any public docs) to briefly explain that whenrenameis true, the swap also renames the indexes according to Meilisearch 1.18 semantics.src/main/java/com/meilisearch/sdk/IndexesHandler.java (1)
124-136: Index rename handler is correct; consider a small guard for invalid input
updateIndexUidcorrectly constructs a{"indexUid": "<newUid>"}body and PATCHes/indexes/{uid}, matching the intended rename flow and the pattern used byupdatePrimaryKey.You might optionally add a simple precondition (e.g., reject null/blank
indexUid) to fail fast if this method is ever called directly with invalid input, instead of relying solely on theClientoverload to guard it.src/main/java/com/meilisearch/sdk/Client.java (1)
168-175: OverloadedupdateIndexworks; consider clarifying behavior and API shapeThe new overload cleanly routes:
- to
updateIndexUidwhenindexUid != null(rename), and- to
updatePrimaryKeyotherwise (primary key update),so behavior is correct and non-breaking for existing callers.
Two optional improvements to consider:
- Clarify the Javadoc to explicitly describe the precedence (“if
indexUidis non-null, the primary key argument is ignored”) so callers don’t assume both can be updated in one call.- Long term, a dedicated
renameIndex(String uid, String indexUid)entry point could make the API more explicit than a tri-parameter overload that multiplexes two concerns.src/test/java/com/meilisearch/sdk/SearchResultQueryVectorTest.java (1)
12-31: Consider adding edge case tests.The test validates the happy path for
queryVectordeserialization. Consider adding test cases for edge scenarios:
queryVectorfield is absent (should be null or empty)queryVectoris null in JSONqueryVectoris an empty arrayThis ensures robust handling of various API response formats.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.code-samples.meilisearch.yaml(1 hunks)data.ms/VERSION(1 hunks)data.ms/instance-uid(1 hunks)src/main/java/com/meilisearch/sdk/Client.java(2 hunks)src/main/java/com/meilisearch/sdk/IndexesHandler.java(1 hunks)src/main/java/com/meilisearch/sdk/model/SearchResult.java(1 hunks)src/main/java/com/meilisearch/sdk/model/SearchResultPaginated.java(1 hunks)src/main/java/com/meilisearch/sdk/model/SwapIndexesParams.java(1 hunks)src/test/java/com/meilisearch/sdk/IndexRenameTest.java(1 hunks)src/test/java/com/meilisearch/sdk/SearchResultQueryVectorTest.java(1 hunks)src/test/java/com/meilisearch/sdk/SwapIndexRenameTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/test/java/com/meilisearch/sdk/SearchResultQueryVectorTest.java (1)
src/main/java/com/meilisearch/sdk/json/GsonJsonHandler.java (1)
GsonJsonHandler(12-56)
src/test/java/com/meilisearch/sdk/SwapIndexRenameTest.java (1)
src/main/java/com/meilisearch/sdk/http/CustomOkHttpClient.java (1)
CustomOkHttpClient(18-126)
src/test/java/com/meilisearch/sdk/IndexRenameTest.java (1)
src/main/java/com/meilisearch/sdk/http/CustomOkHttpClient.java (1)
CustomOkHttpClient(18-126)
🔇 Additional comments (5)
data.ms/VERSION (1)
1-1: Ensure VERSION value stays in sync with published SDK version
1.26.0looks fine as a data marker, but please double-check it matches the actual SDK version used in packaging (e.g., Maven coordinates, release notes) so tests and metadata don’t drift.data.ms/instance-uid (1)
1-1: UUID fixture looks fineStatic instance UID value is acceptable as a deterministic fixture; no functional issues from this change.
.code-samples.meilisearch.yaml (1)
857-858: Rename sample correctly targets the newupdateIndexoverloadThe
rename_an_index_1snippet usesclient.updateIndex("indexA", null, "indexB");, which cleanly maps to “rename indexA to indexB” while leaving the primary key untouched. This aligns with the new overload semantics and fits well with the surrounding samples.src/main/java/com/meilisearch/sdk/model/SearchResult.java (1)
21-21:queryVectorfield addition is consistent and testableAdding
ArrayList<Float> queryVector;alongside existing response fields is consistent with the current model style and should deserialize correctly given the existing JSON mapping and the dedicated test.src/main/java/com/meilisearch/sdk/Client.java (1)
13-13: Wildcardjava.utilimport is acceptable given multiple usagesSwitching to
import java.util.*;is harmless here, since the class already depends on severaljava.utiltypes (Map, HashMap, Date, TimeZone, UUID). No action needed unless your style guide prefers explicit imports.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/test/java/com/meilisearch/sdk/IndexRenameTest.java (1)
62-80: Optional: align second test’s HTTP assertions with the first.The second test correctly verifies path and payload for different names. For symmetry and slightly stronger coverage, you could also assert the HTTP method and
Authorizationheader as in the first test.- RecordedRequest request = mockServer.takeRequest(); - assertThat(request.getPath(), equalTo("/indexes/oldIndex")); - - String requestBody = request.getBody().readUtf8(); + RecordedRequest request = mockServer.takeRequest(); + assertThat(request.getMethod(), equalTo("PATCH")); + assertThat(request.getPath(), equalTo("/indexes/oldIndex")); + assertThat(request.getHeader("Authorization"), equalTo("Bearer masterKey")); + + String requestBody = request.getBody().readUtf8(); assertThat(requestBody, containsString("\"indexUid\":\"newIndex\""));src/test/java/com/meilisearch/sdk/SwapIndexesRenameTest.java (2)
72-99: Non-rename swap test is good; header assertion is an optional enhancement.The test correctly covers
rename=false, index names, andtaskUid. For parity with the first test, you might also assert theAuthorizationheader, but the current coverage is acceptable.- RecordedRequest request = mockServer.takeRequest(); - assertThat(request.getMethod(), equalTo("POST")); - assertThat(request.getPath(), equalTo("/swap-indexes")); - - String requestBody = request.getBody().readUtf8(); + RecordedRequest request = mockServer.takeRequest(); + assertThat(request.getMethod(), equalTo("POST")); + assertThat(request.getPath(), equalTo("/swap-indexes")); + assertThat(request.getHeader("Authorization"), equalTo("Bearer masterKey")); + + String requestBody = request.getBody().readUtf8(); assertThat(requestBody, containsString("\"rename\":false"));
101-132: Consider slightly strengthening multi-pair swap assertions and reducing JSON-shape brittleness.The multi-pair test correctly verifies both index pairs and that the payload is a JSON array. To make it a bit more robust and explicit you could:
- Trim the body before
startsWith/endsWithso a trailing newline won’t break the test.- Optionally assert both
"rename":trueand"rename":falseare present to ensure per-pair flags are serialized.- String requestBody = request.getBody().readUtf8(); - - assertThat(requestBody, containsString("\"indexA\"")); - assertThat(requestBody, containsString("\"indexB\"")); - assertThat(requestBody, containsString("\"indexC\"")); - assertThat(requestBody, containsString("\"indexD\"")); - assertThat(requestBody, startsWith("[")); - assertThat(requestBody, endsWith("]")); + String requestBody = request.getBody().readUtf8(); + + assertThat(requestBody, containsString("\"indexA\"")); + assertThat(requestBody, containsString("\"indexB\"")); + assertThat(requestBody, containsString("\"indexC\"")); + assertThat(requestBody, containsString("\"indexD\"")); + assertThat(requestBody, containsString("\"rename\":true")); + assertThat(requestBody, containsString("\"rename\":false")); + + String trimmedBody = requestBody.trim(); + assertThat(trimmedBody, startsWith("[")); + assertThat(trimmedBody, endsWith("]"));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
build.gradle(1 hunks)src/test/java/com/meilisearch/sdk/IndexRenameTest.java(1 hunks)src/test/java/com/meilisearch/sdk/SwapIndexesRenameTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- build.gradle
🔇 Additional comments (4)
src/test/java/com/meilisearch/sdk/IndexRenameTest.java (2)
18-38: MockWebServer setup/teardown and Config wiring look correct.Creating a per-test MockWebServer, trimming the trailing slash from the base URL, and shutting the server down in
@AfterEachis clean and consistent; no issues here.
40-60: Rename test cleanly exercisesIndexesHandler.updateIndexUidover HTTP.The test validates:
- Response mapping via
TaskInfo.getTaskUid()- HTTP verb (
PATCH), path (/indexes/indexA), andAuthorizationheader- JSON payload including
"indexUid":"indexB"This gives good coverage of the new rename behavior in the handler.
src/test/java/com/meilisearch/sdk/SwapIndexesRenameTest.java (2)
19-39: Client + MockWebServer wiring is straightforward and consistent.Using
Configwith the trimmed mock base URL and constructing aClientper test, plus proper MockWebServer lifecycle, looks good.
41-70: Swap-with-rename test validates all critical request/response aspects.You’re asserting:
POST /swap-indexesAuthorizationheader- JSON body including
"rename":trueand both index namesTaskInfodeserialization viataskUidThis is solid coverage for the new
rename=truebehavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/test/java/com/meilisearch/sdk/IndexRenameTest.java (2)
40-82: Consider consolidating with @ParameterizedTest.Both test methods have identical structure and differ only in the index names and expected taskUid. Using
@ParameterizedTestwith@MethodSourceor@CsvSourcewould reduce duplication and make it easier to add more test cases.Example refactor:
@ParameterizedTest @CsvSource({ "indexA, indexB, 123", "oldIndex, newIndex, 456" }) void testRenameIndex(String oldUid, String newUid, int expectedTaskUid) throws Exception { String responseJson = String.format( "{\"taskUid\":%d,\"indexUid\":\"%s\",\"status\":\"enqueued\",\"type\":\"indexUpdate\",\"enqueuedAt\":\"2024-01-01T00:00:00Z\"}", expectedTaskUid, newUid); mockServer.enqueue(new MockResponse() .setResponseCode(202) .setBody(responseJson) .addHeader("Content-Type", "application/json")); TaskInfo result = handler.updateIndexUid(oldUid, newUid); assertThat(result, is(notNullValue())); assertThat(result.getTaskUid(), is(equalTo(expectedTaskUid))); RecordedRequest request = mockServer.takeRequest(); assertThat(request.getMethod(), equalTo("PATCH")); assertThat(request.getPath(), equalTo("/indexes/" + oldUid)); assertThat(request.getHeader("Authorization"), equalTo("Bearer masterKey")); String requestBody = request.getBody().readUtf8(); assertThat(requestBody, containsString("\"indexUid\":\"" + newUid + "\"")); }
40-82: Consider adding error case tests.The tests cover happy path scenarios well but don't verify error handling. Consider adding tests for:
- 404 response when the source index doesn't exist
- 400 response for invalid index names
- 409 response when the target index name already exists
- Null or empty string handling
Example error case test:
@Test void testRenameIndexNotFound() throws Exception { String errorJson = "{\"message\":\"Index `nonexistent` not found.\",\"code\":\"index_not_found\",\"type\":\"invalid_request\",\"link\":\"https://docs.meilisearch.com/errors#index_not_found\"}"; mockServer.enqueue(new MockResponse() .setResponseCode(404) .setBody(errorJson) .addHeader("Content-Type", "application/json")); // Assert that appropriate exception is thrown assertThrows(MeilisearchApiException.class, () -> { handler.updateIndexUid("nonexistent", "newName"); }); }src/test/java/com/meilisearch/sdk/SwapIndexesRenameTest.java (2)
41-100: Consider consolidating with @ParameterizedTest.The first two test methods have nearly identical structure and differ only in the
renameflag value and expected taskUid. Using@ParameterizedTestwould reduce duplication.Example refactor:
@ParameterizedTest @CsvSource({ "true, indexA, indexB, 789", "false, indexC, indexD, 790" }) void testSwapIndexesWithRenameFlag(boolean rename, String indexA, String indexB, int expectedTaskUid) throws Exception { String responseJson = String.format( "{\"taskUid\":%d,\"status\":\"enqueued\",\"type\":\"indexSwap\",\"enqueuedAt\":\"2024-01-01T00:00:00Z\"}", expectedTaskUid); mockServer.enqueue(new MockResponse() .setResponseCode(202) .setBody(responseJson) .addHeader("Content-Type", "application/json")); SwapIndexesParams[] params = { new SwapIndexesParams() .setIndexes(new String[]{indexA, indexB}) .setRename(rename) }; TaskInfo result = client.swapIndexes(params); assertThat(result, is(notNullValue())); assertThat(result.getTaskUid(), is(equalTo(expectedTaskUid))); RecordedRequest request = mockServer.takeRequest(); assertThat(request.getMethod(), equalTo("POST")); assertThat(request.getPath(), equalTo("/swap-indexes")); String requestBody = request.getBody().readUtf8(); assertThat(requestBody, containsString("\"rename\":" + rename)); assertThat(requestBody, containsString(indexA)); assertThat(requestBody, containsString(indexB)); }
41-137: Consider adding error and edge case tests.The tests cover happy path scenarios effectively but don't verify error handling or edge cases. Consider adding tests for:
- 404 response when one of the indexes doesn't exist
- 400 response for invalid payloads (e.g., empty
indexesarray, single index, mismatched array lengths)- Empty
SwapIndexesParams[]array- Null values in the params
Example edge case test:
@Test void testSwapIndexesWithEmptyArray() throws Exception { SwapIndexesParams[] params = {}; // Verify behavior - should it throw an exception or send empty array? // This depends on the API specification } @Test void testSwapIndexesNotFound() throws Exception { String errorJson = "{\"message\":\"Index `nonexistent` not found.\",\"code\":\"index_not_found\",\"type\":\"invalid_request\"}"; mockServer.enqueue(new MockResponse() .setResponseCode(404) .setBody(errorJson) .addHeader("Content-Type", "application/json")); SwapIndexesParams[] params = { new SwapIndexesParams() .setIndexes(new String[]{"nonexistent", "indexB"}) .setRename(true) }; assertThrows(MeilisearchApiException.class, () -> { client.swapIndexes(params); }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/java/com/meilisearch/sdk/IndexRenameTest.java(1 hunks)src/test/java/com/meilisearch/sdk/SwapIndexesRenameTest.java(1 hunks)
🔇 Additional comments (1)
src/test/java/com/meilisearch/sdk/SwapIndexesRenameTest.java (1)
102-137: Excellent test coverage for multiple index pairs.This test effectively validates that multiple swap operations are serialized correctly as a JSON array. The array structure validation (lines 134-136) is particularly valuable, ensuring the request body is properly formatted as an array rather than individual objects.
curquiza
changed the title
There was a problem hiding this comment.
Hey @yamiSukehiro2907 can you remove the data.ms?
Also, can you fix the tests in the CI?
Thanks for your pR
|
@curquiza |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/java/com/meilisearch/sdk/SwapIndexRenameTest.java (1)
33-58: Relax path assertion to avoid coupling to double‑slash quirkThe comment notes the double slash in the recorded path; asserting
equalTo("//swap-indexes")hard‑codes that behavior into the test. To keep the test stable if the base URL handling is later cleaned up, consider matching only the tail of the path:- assertThat(req.getMethod(), equalTo("POST")); - // FIX: Actual path contains double slash - assertThat(req.getPath(), equalTo("//swap-indexes")); + assertThat(req.getMethod(), equalTo("POST")); + assertThat(req.getPath(), endsWith("/swap-indexes"));This still validates the correct endpoint while tolerating either
/swap-indexesor//swap-indexes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
build.gradle(8 hunks)src/main/java/com/meilisearch/sdk/IndexesHandler.java(2 hunks)src/test/java/com/meilisearch/sdk/IndexRenameTest.java(1 hunks)src/test/java/com/meilisearch/sdk/SearchResultQueryVectorTest.java(1 hunks)src/test/java/com/meilisearch/sdk/SwapIndexRenameTest.java(1 hunks)
🔇 Additional comments (3)
src/test/java/com/meilisearch/sdk/SearchResultQueryVectorTest.java (1)
31-55: Good coverage ofqueryVectordeserialization through the public search APIThis test cleanly validates the new
queryVectorfield viaClient.index(...).search(...)and asserts both size and contents; setup/teardown withMockWebServerand JUnit 5 look correct.build.gradle (1)
48-77: Upgrade legacy MockWebServer to align with JUnit 6The OkHttp version alignment (okhttp 5.3.0 and mockwebserver 5.3.0) is correct, but the build has a version mismatch: the legacy okhttp3.mockwebserver artifact is considered obsolete and depends on JUnit 4, yet your project uses JUnit 6 (junit-jupiter 6.0.1).
This pairing risks package incompatibilities and misses improvements in the newer API. Use the new mockwebserver3 artifacts, with mockwebserver3-junit5 for JUnit 5 integrations.
Verify that mockwebserver3-junit5 supports JUnit 6, or upgrade accordingly. You can also remove the redundant
testImplementation 'com.squareup.okhttp3:okhttp:5.3.0'since tests already see it via theapidependency.src/test/java/com/meilisearch/sdk/IndexRenameTest.java (1)
32-52: Align rename test with Meilisearch PATCH/indexescontractTo match the Meilisearch index update API and the
updateIndexUidimplementation fix, this test should assert theuidfield in the payload, notindexUid. You can also make the path assertion less brittle withendsWith:- assertThat(req.getMethod(), equalTo("PATCH")); - // FIX: Actual path includes double slash - assertThat(req.getPath(), equalTo("//indexes/oldIndex")); + assertThat(req.getMethod(), equalTo("PATCH")); + assertThat(req.getPath(), endsWith("/indexes/oldIndex")); @@ - String body = req.getBody().readUtf8(); - assertThat(body, containsString("\"indexUid\":\"newIndex\"")); + String body = req.getBody().readUtf8(); + assertThat(body, containsString("\"uid\":\"newIndex\"")); assertThat(req.getHeader("Authorization"), equalTo("Bearer masterKey"));This way the test validates the correct field name expected by the server while remaining tolerant to minor base‑URL formatting differences.
2 participants
Fixes #888
🚀 Meilisearch Java SDK – Support for Meilisearch 1.18
What does this PR do?
This PR adds full support for Meilisearch 1.18 features in the Java SDK.
It implements:
1.
queryVectorsupport in search responsesretrieveVectors=true, Meilisearch now returns a newqueryVectorfield.SearchResult.getQueryVector().SearchResultQueryVectorTest2. Index renaming using
PATCH /indexes/:uidIndexesHandler.updateIndex()to accept a newindexUidparameter for renaming.{ "indexUid": "indexB" }Added test:
IndexRenameTest3. Index renaming using swap-indexes
SwapIndexesParamsto includerename.IndexesHandler.swapIndexes()to send therenameflag.[ { "indexes": ["indexA", "indexB"], "rename": true } ]Added test:
SwapIndexRenameTest4. Code sample update
rename_an_index_1key in.code-samples.meilisearch.yamlSummary of changes
Source
SearchResult.javaIndexesHandler.javaSwapIndexesParams.javaIndex.java(if updated method exposed)Tests
SearchResultQueryVectorTest.javaIndexRenameTest.javaSwapIndexRenameTest.javaDocumentation
.code-samples.meilisearch.yamlPR Checklist
./gradlew clean build)spotlessApply)Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.