-
Notifications
You must be signed in to change notification settings - Fork 134
Add support for sorting on the documents API #929
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add support for sorting on the documents API #929
Conversation
📝 WalkthroughWalkthroughAdds sorting support to Documents: Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DocumentsSDK as Documents
participant HTTP as HTTPClient
participant Meili as MeiliSearch
Client->>DocumentsSDK: getDocuments(uid, param, targetClass)
alt param.getSort() != null OR param.getFilter() != null
DocumentsSDK->>HTTP: POST /indexes/{uid}/documents/fetch (body: param.toString())
else
DocumentsSDK->>HTTP: GET /indexes/{uid}/documents? + param.toQuery()
end
HTTP->>Meili: HTTP request
Meili-->>HTTP: HTTP response
HTTP-->>DocumentsSDK: parsed response
DocumentsSDK-->>Client: return documents (mapped to targetClass or raw)
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/test/java/com/meilisearch/integration/DocumentsTest.java`:
- Around line 544-549: The current assertion checks that compareTo(...) is not
equal to 1 which is incorrect; replace the equality check with an assertion that
the result of movies[i].getTitle().compareTo(movies[i + 1].getTitle()) is less
than or equal to 0 to verify ascending order. Locate the loop in DocumentsTest
(the for loop iterating over the movies array) and change the compareTo
assertion to use a lessThanOrEqualTo(0) matcher (or equivalent <= 0) so the test
verifies proper ascending order; ensure any required Hamcrest matcher imports
are present.
🧹 Nitpick comments (2)
src/main/java/com/meilisearch/sdk/model/DocumentsQuery.java (1)
28-36: Inconsistent withfilterhandling.Adding
sorttotoQuery()is inconsistent with howfilteris handled—filteris not included intoQuery()because it also triggers the POST path inDocuments.java. Whensortis non-null, the POST endpoint is used withtoString()for the body, so thissortparameter intoQuery()will never actually be used with a non-null value.Consider removing
sortfromtoQuery()to match thefilterpattern, or document why it's intentionally included.Suggested fix to align with filter handling
public String toQuery() { URLBuilder urlb = new URLBuilder() .addParameter("limit", this.getLimit()) .addParameter("offset", this.getOffset()) - .addParameter("fields", this.getFields()) - .addParameter("sort", this.getSort()); + .addParameter("fields", this.getFields()); return urlb.getURL(); }src/test/java/com/meilisearch/integration/DocumentsTest.java (1)
645-667: Test coverage is minimal but acceptable.This test verifies the
sortparameter doesn't cause errors and that the response structure is correct, but doesn't verify the actual descending sort order. Since this is raw JSON output, parsing to verify order would add complexity.Consider adding a basic order verification by checking that known titles appear in expected positions, or parsing the JSON to validate order—though this is optional given the typed test (
testGetDocumentsWithSort) already covers sort ordering.
Pull Request
Related issue
Fixes #884.
What does this PR do?
sortparameter toDocumentsQueryDocumentsclass to use POST/fetchendpoint when sort parameter is presentgetDocuments()andgetRawDocuments()methodsThe code in this pull request was generated by GitHub Copilot with the Claude Sonnet 4.5 model.
PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.