add show performance details field to related endpoints - #938
Conversation
📝 WalkthroughWalkthroughAdds support for requesting and returning raw performance traces: request types gain a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SDK as "SDK (Request classes)"
participant Meili as "Meilisearch Server"
participant Models as "Response models"
Client->>SDK: build request with showPerformanceDetails=true
SDK->>Meili: send search/multi-search/similar-docs request (flag included)
Meili-->>SDK: response including performanceDetails object
SDK->>Models: deserialize raw performanceDetails into HashMap<String,Object>
SDK-->>Client: return result containing performanceDetails
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/com/meilisearch/integration/SearchTest.java (1)
837-847:⚠️ Potential issue | 🔴 CriticalFederation test may fail —
showPerformanceDetailsis not serialized inMultiSearchFederation.toString().As flagged in
MultiSearchFederation.java, thetoString()method doesn't includeshowPerformanceDetails. This means line 837 sets the flag but it's never sent to the server, sogetPerformanceDetails()on line 847 will likely benull, causing an NPE on.size().Consider adding a null-safety assertion (
assertThat(results.getPerformanceDetails(), is(notNullValue()))) before checking size, and ensure the serialization bug is fixed first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/meilisearch/integration/SearchTest.java` around lines 837 - 847, The test sets federation.setShowPerformanceDetails(true) but MultiSearchFederation.toString() currently omits the showPerformanceDetails field so the flag never reaches the server and results.getPerformanceDetails() can be null; update MultiSearchFederation.toString() to serialize showPerformanceDetails (matching the other federation fields) and then make the test null-safe by asserting results.getPerformanceDetails() is not null (e.g., assertThat(results.getPerformanceDetails(), is(notNullValue()))) before asserting its size; reference the MultiSearchFederation.toString(), MultiSearchFederation.setShowPerformanceDetails(), and MultiSearchResult.getPerformanceDetails() symbols when applying the fixes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/meilisearch/sdk/model/MultiSearchResult.java`:
- Line 26: The performanceDetails field in MultiSearchResult is typed as
HashMap<String, String> but can contain non-string values; change its
declaration to HashMap<String, Object> and update any associated
accessor/mutator (e.g., getPerformanceDetails, setPerformanceDetails,
constructors, and any JSON (de)serialization annotations or usages) to use
HashMap<String, Object> so heterogeneous values are preserved.
In `@src/main/java/com/meilisearch/sdk/model/SearchResult.java`:
- Line 27: Update the performanceDetails field from HashMap<String, String> to
HashMap<String, Object> to allow non-string values during deserialization:
locate the performanceDetails declaration in the SearchResult class (field name
performanceDetails), and make the same change in MultiSearchResult
(performanceDetails) and SimilarDocumentsResults (performanceDetails); ensure
any getters/setters or usages (e.g., getPerformanceDetails(),
setPerformanceDetails(...)) and relevant imports are updated to use
HashMap<String, Object> (or Map<String, Object>) so deserialization won't fail
if the API returns numbers or nested objects.
In `@src/main/java/com/meilisearch/sdk/model/SimilarDocumentsResults.java`:
- Line 23: In SimilarDocumentsResults change the field declaration
performanceDetails from HashMap<String, String> to HashMap<String, Object> and
update any associated accessor/mutator methods, constructors, builders, and
(de)serialization logic that reference performanceDetails to use HashMap<String,
Object> instead; mirror the same adjustments done in SearchResult.java to ensure
type consistency across the models.
In `@src/test/java/com/meilisearch/integration/SearchTest.java`:
- Around line 1319-1325: In SearchTest, the assertion on searchResult hits uses
hasSize(11) but should expect the default page size of 20; update the assertion
for searchResult.getHits() from hasSize(11) to hasSize(20) so that
SearchRequest.builder().q("a").showPerformanceDetails(true).build() correctly
validates 20 returned hits; keep the other assertions (getEstimatedTotalHits and
getPerformanceDetails) unchanged.
---
Outside diff comments:
In `@src/test/java/com/meilisearch/integration/SearchTest.java`:
- Around line 837-847: The test sets federation.setShowPerformanceDetails(true)
but MultiSearchFederation.toString() currently omits the showPerformanceDetails
field so the flag never reaches the server and results.getPerformanceDetails()
can be null; update MultiSearchFederation.toString() to serialize
showPerformanceDetails (matching the other federation fields) and then make the
test null-safe by asserting results.getPerformanceDetails() is not null (e.g.,
assertThat(results.getPerformanceDetails(), is(notNullValue()))) before
asserting its size; reference the MultiSearchFederation.toString(),
MultiSearchFederation.setShowPerformanceDetails(), and
MultiSearchResult.getPerformanceDetails() symbols when applying the fixes.
4199f96 to
d323fc5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/com/meilisearch/sdk/SearchRequest.java (1)
51-57: Minor: Javadoc constructor comment omitsshowPerformanceDetails.The constructor doc lists defaults for all serialized fields but
showPerformanceDetailsis missing.📝 Suggested Javadoc update
* Constructor for SearchRequest for building search queries with the default values: offset: 0, * limit: 20, attributesToRetrieve: ["*"], attributesToCrop: null, cropLength: 200, * attributesToHighlight: null, filter: null, showMatchesPosition: false, facets: null, sort: - * null, showRankingScore: false, showRankingScoreDetails: false, rankingScoreThreshold: null, - * retrieveVectors: false + * null, showRankingScore: false, showRankingScoreDetails: false, rankingScoreThreshold: null, + * retrieveVectors: false, showPerformanceDetails: false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/meilisearch/sdk/SearchRequest.java` around lines 51 - 57, The Javadoc for the SearchRequest constructor omits the default for the showPerformanceDetails field; update the constructor comment in SearchRequest.java to include showPerformanceDetails (with its default value) alongside the other serialized field defaults so the documentation accurately reflects all fields initialized by the constructor—locate the constructor comment for SearchRequest and add the showPerformanceDetails default to the list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/java/com/meilisearch/integration/SearchTest.java`:
- Line 847: Replace the direct size() assertions on the possibly-null map with a
Hamcrest map matcher that handles nulls: change usages of
assertThat(results.getPerformanceDetails().size(), greaterThan(0)) to
assertThat(results.getPerformanceDetails(), aMapWithSize(greaterThan(0))) in
SearchTest (references to getPerformanceDetails), and apply the same replacement
at the other two locations flagged (the occurrences around lines 1114 and 1325).
---
Duplicate comments:
In `@src/main/java/com/meilisearch/sdk/model/MultiSearchResult.java`:
- Line 26: No change needed: the field declaration performanceDetails of type
HashMap<String, Object> in class MultiSearchResult is correct and matches the
fix applied in SearchResult.java; leave the declaration "HashMap<String, Object>
performanceDetails" as-is and no further modifications are required.
In `@src/test/java/com/meilisearch/integration/SearchTest.java`:
- Line 1323: The assertion expecting 11 hits is a copy-paste error; update the
test that asserts on searchResult.getHits() to expect 20 hits instead of 11 (the
test invokes q("a") with default offset and default limit 20, so
searchResult.getHits() should have size 20). Locate the failing assertion in
SearchTest.java referencing searchResult.getHits() (the hasSize(11) call) and
change it to hasSize(20).
---
Nitpick comments:
In `@src/main/java/com/meilisearch/sdk/SearchRequest.java`:
- Around line 51-57: The Javadoc for the SearchRequest constructor omits the
default for the showPerformanceDetails field; update the constructor comment in
SearchRequest.java to include showPerformanceDetails (with its default value)
alongside the other serialized field defaults so the documentation accurately
reflects all fields initialized by the constructor—locate the constructor
comment for SearchRequest and add the showPerformanceDetails default to the
list.
d323fc5 to
9a70d02
Compare
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/test/java/com/meilisearch/integration/SearchTest.java`:
- Around line 1319-1325: The assertion for hit count is wrong: update the test
in SearchTest where SearchRequest searchRequest =
SearchRequest.builder().q("a").showPerformanceDetails(true).build() and
SearchResult searchResult = (SearchResult) index.search(searchRequest) to expect
20 hits (change the hasSize matcher from 11 to 20); verify getHits() is asserted
with hasSize(20) to reflect the default limit of 20 when no offset/limit is
provided.
Co-authored-by: Cursor <cursoragent@cursor.com>
Strift
left a comment
There was a problem hiding this comment.
Hello @valkrypton and thanks for your contribution 🙌 LGTM!
Pull Request
Related issue
Fixes #933
What does this PR do?
showPerformanceDetailsto all related endpointsperformanceDetailsFieldPR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
New Features
Tests