Skip to content
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

Update documents API for Meilisearch v.28 #518

Merged
merged 14 commits into from
Jan 5, 2023

Conversation

alallema
Copy link
Contributor

@alallema alallema commented Dec 28, 2022

Pull Request

Related to: meilisearch/integration-guides#205

What does this PR do?

Breaking because enforces the users to use Meilisearch v0.28.0

  • GET /documents/:uid, GET /documents Add the possibility to reduce the body payload by using a query parameter called fields (previously called attributesToRetrieve), check the issue and the spec for the entire behavior.
  • The documents objects now return wrap in a Results
  • Create two separate methods for GET /documents:
    • <T> Results<T> getDocuments(Class<T> targetClass) throws MeilisearchException -> return an Object
    • String getRawDocuments() -> return a String
  • Create two separate methods for GET /documents/:uid:
    • <T> Results<T> getDocument(String identifier, Class<T> targetClass) throws MeilisearchException -> return an Object
    • String getRawDocument(String identifier) -> return a String
  • Adapt DocumentsQuery naming following all the routes
  • Adapt and fix tests
  • Add test for getRawDocument
  • Add test for getRawDocuments

@alallema alallema marked this pull request as ready for review December 28, 2022 16:42
Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a comment, but it applies to every occurrence of the queries :)

I really think it will improve the complexity of the code :)

src/main/java/com/meilisearch/sdk/Documents.java Outdated Show resolved Hide resolved
* @return Object containing the requested document
* @throws MeilisearchException if the client request causes an error
*/
<T> T getDocument(String uid, String identifier, Class<T> targetClass)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to also add support to fields, which allows the user to reduce the payload meilisearch will return like we have here: https://github.com/meilisearch/meilisearch-python/blob/a6dc0631103a495d109ff6b69e288193d1b22b73/tests/index/test_index_document_meilisearch.py#L71-L78.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be used in the getDocuments, getDocument, getRawDocuments, and getRawDocument

@alallema alallema changed the title Apply documents changes Update documents API for Meilisearch v.28 Jan 5, 2023
Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you go over the different method documentations to update

  • The explanations
  • The @param param
  • The return statement

I started suggesting on a lot of them

src/main/java/com/meilisearch/sdk/Documents.java Outdated Show resolved Hide resolved
src/main/java/com/meilisearch/sdk/Documents.java Outdated Show resolved Hide resolved
Comment on lines +169 to +170
URLBuilder urlb = new URLBuilder();
urlb.addSubroute("indexes").addSubroute(uid).addSubroute("documents");
if (primaryKey != null) {
urlPath += "?primaryKey=" + primaryKey;
urlb.addParameter("primaryKey", primaryKey);
}
String urlPath = urlb.getURL();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you use toQuery here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There already is an overload of toQuery with 2 String as parameters:
public String toQuery(String uid, String identifier)
So I can create a new one but with another name like toQueryWithPrimaryKey ...

Copy link
Contributor

@bidoubiwa bidoubiwa Jan 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be:

    public String toQuery(String uid, String identifier, String primaryKey) {

Which I don't think exists yet in the toQuery

But yes, it becomes to specific... so you are right. Until we find something more generic your solution is maybe better.

@brunoocasali what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be:
public String toQuery(String uid, String identifier, String primaryKey) {

It could but we don't use identifier

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brunoocasali what do you think?
Same

src/main/java/com/meilisearch/sdk/Documents.java Outdated Show resolved Hide resolved
src/main/java/com/meilisearch/sdk/Index.java Outdated Show resolved Hide resolved
src/main/java/com/meilisearch/sdk/Index.java Outdated Show resolved Hide resolved
src/main/java/com/meilisearch/sdk/Index.java Outdated Show resolved Hide resolved
@alallema alallema requested a review from bidoubiwa January 5, 2023 14:29
@alallema alallema merged commit 1a8704d into bump-meilisearch-v0.28.0 Jan 5, 2023
@alallema alallema deleted the documents_changes branch January 5, 2023 16:04
@alallema alallema added the breaking-change The related changes are breaking for the users label Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change The related changes are breaking for the users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants