Skip to content

Update Index API for Meilisearch v.28 #516

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

Merged
merged 14 commits into from
Jan 5, 2023

Conversation

alallema
Copy link
Contributor

@alallema alallema commented Dec 27, 2022

Pull Request

Related to: meilisearch/meilisearch#2373

What does this PR do?

Changes

  • client.getIndexes() accept pagination metadata, added limit (default: 20), offset (default: 0), total
  • Wrap the indexes objects in a Results object
  • Remove client.getRawIndex() method
  • client.getRawIndexes() method return a String
  • Creation of IndexesQuery class
  • Rename the methods of the "IndexesHandler" to match the name of the Client methods
    client.getIndexes()

returns for example

com.meilisearch.sdk.model.Results@2a742aa2
    client.getRawIndexes()

returns for example

{"results":[],"offset":0,"limit":20,"total":0}

@@ -105,7 +105,8 @@ public static void deleteAllIndexes() {
Client ms = new Client(new Config(getMeilisearchHost(), "masterKey"));
Results<Index> indexes = ms.getIndexes();
for (Index index : indexes.getResults()) {
ms.deleteIndex(index.getUid());
TaskInfo task = ms.deleteIndex(index.getUid());
Copy link
Member

Choose a reason for hiding this comment

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

Nice change! 👯

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.

I don't see the updateIndex method.

Can you also change the name of the PR with more context? For example

Update Index API for Meilisearch v.28

@alallema
Copy link
Contributor Author

alallema commented Jan 4, 2023

I don't see the updateIndex method.

Can you also change the name of the PR with more context? For example

Update Index API for Meilisearch v.28

It's on line 151 on Client file and this method is called updatePrimaryKey on the IndexesHandler file because it updates the primary key.

@alallema alallema changed the title Apply indexes changes Update Index API for Meilisearch v.28 Jan 4, 2023
@alallema alallema requested a review from bidoubiwa January 4, 2023 11:43
@alallema alallema requested a review from bidoubiwa January 4, 2023 13:41
* @throws MeilisearchException if an error occurs
*/
String getAll() throws MeilisearchException {
Results<Index> getIndexes() throws MeilisearchException {
return httpClient.get("/indexes", Results.class, Index.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

It could be nice to use IndexesQuery everywhere so that we only hardcode /indexes in toQuery. Nonetheless, maybe it is worth creating an issue for that change to avoid making this PR more complex

Copy link
Contributor Author

@alallema alallema Jan 5, 2023

Choose a reason for hiding this comment

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

We can indeed do this but it means creating a new function that will require resources and complicate the general call such as:
return httpClient.get("/indexes", Results.class, Index.class);
will became:
return httpClient.get(new IndexesQuery().toQuery(), Results.class, Index.class);
Not sure it worth it but we can also just create a variable in the IndexesHandler class:
String indexesPath = "/indexes";

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the second option for consistency, but you are maybe right. You can ignore this for the moment but I would like the opinion of @brunoocasali on that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Me too !

Copy link
Member

Choose a reason for hiding this comment

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

My initial conception around IndexesQuery, KeysQuery, and so on, where to handle query parameters. If we are very strict, we can say that the /indexes path is related to the URL, not the query parameters, so it shouldn't be inside.

In any case, I don't think /indexes should be treated as handled inside of IndexesQuery, but if we use a private constant inside of IndexesHandler is enough.

If the path change in the future, we will have two places to change the tests, IndexesQuery and IndexesHandler.

@alallema alallema requested a review from bidoubiwa January 5, 2023 13:39
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.

LGTM 🔥🔥🔥🔥🔥

Nice work ✨✨✨✨

@alallema alallema merged commit 4a0fec6 into bump-meilisearch-v0.28.0 Jan 5, 2023
@alallema alallema deleted the indexes_changes branch January 5, 2023 15:32
@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