Skip to content

Conversation

@brunoocasali
Copy link
Member

  • Includes creation of a new implementation of PendingUpdate to be used inside the MeiliSearchClient instance.
  • All the tests are related directly to the updated version of the indexes.

Related to the v0.25.0 upgrade.

- Includes a creation of a new implementation of PendingUpdate to be used inside the MeiliSearchClient instance.
- All the tests related directly to the updated version of the indexes.

> This commit are not in a green state.
@brunoocasali brunoocasali added breaking-change The related changes are breaking for the users skip-changelog The PR will not appear in the release changelogs labels Jan 15, 2022
Copy link
Contributor

@alallema alallema left a comment

Choose a reason for hiding this comment

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

Great 💪 !
Just ask a few questions

ClientTaskImpl(client, map['uid'] as int);

@override
Future<UpdateStatus> getStatus() async {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this method will become getTask ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the name will be changed in the next PR but I think this getStatus method and getUpdateStatus should be renamed in getTask and getTasks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I just tried to reduce the scope of changes haha, but I'll do that, thanks for the reminder ;)


Future<UpdateStatus> getUpdateStatus(int updateId) async {
final response = await http.getMethod(('/indexes/$uid/updates/$updateId'));
final response = await http.getMethod(('/tasks/$updateId'));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this method should call '/indexes/$uid/tasks/$updateId' if this is index.getTask()

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this was one of my biggest nightmares when I was implementing this...

Since we have this extension defined here test/utils/client.dart:73
It causes problems for me when I updated this test:

   test('Delete an existing index', () async {
      final uid = randomUid();
      await client.createIndex(uid).waitFor();

      final index = await client.getIndex(uid);
      await index.delete().waitFor(); // waitFor() => this causes the issue...

      expect(client.getIndex(uid), throwsA(isA<MeiliSearchApiException>()));
    });

The problem here is because .waitFor(); Will loop until the task is over, but since the extension tries to get the currentStatus based on each implementation, it will make a request to indexes/:uid/tasks/:task_uid, but if you look closely... For the task to be completed, the index should be removed before the task is done, so imagine this timeline:

  • request to the server to delete the index
  • server add that request to the tasks queue
  • #try-1 to check if the delete is done but is not.
  • #try-2 to check if the delete is done but is not.
  • server get the task be processed
  • #try-3 to check if the delete is done but is not, currently is being processed.
  • server finished the task, the index is gone.
  • #try-4 to check if the delete is done and it is but we have a problem (💥) why?
    • /indexes/:uid/tasks is not accessible anymore because the :uid (meaning, the index was removed remember?)
    • that’s why we receive an error instead of a successful response telling us that the task is finished (btw the error is ).
    {
      "message": "Index `my_index` not found.",
      "code": "index_not_found",
      "type": "invalid_request",
      "link": "https://docs.meilisearch.com/errors#index_not_found"
    }

By the way, yes I could skip waitFor() here in this test, but in this case, I could not ensure the current behavior or have the exception when I try to access the index. (if you have some idea please tell me) - I thought about doing a mock, but idk if this will be possible.

That is the current situation here, I hope you understand if not, don’t hesitate to make a comment ;)

FYI @curquiza (maybe this is an API design situation that I need to be aware of)

Copy link
Contributor

@alallema alallema Jan 17, 2022

Choose a reason for hiding this comment

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

I had the same issue on another SDK, I rewrite the test to get the Task from the client. Indeed if the index doesn't exist you can't retrieve the task from it.
Also, the waitForTask function must be accessible by the client and the index too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had the same issue on another SDK, I rewrite the test to get the Task from the client.

Could you link here to this change you made?

Also, the waitForTask function must be accessible by the client and the index too.

*(on the dart integration you will have the waitFor for all the methods that respond with a Task object (formerly PendingUpdate)), so my waitForTask is already working for these both situations... (but I'm not sure if this is what you mean)

Copy link
Member

Choose a reason for hiding this comment

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

Hello, sorry I read this kind of quickly, so I hope I correctly understood the situation.

The index.waitForTask and client.waitForTask methods should definitely NOT call the /indexes/:uid/tasks/:uid method, but the /tasks/:uid one, for the reason you described @brunoocasali, even for the method you called from the Index instance
We anticipated this issue, see this comment meilisearch/integration-guides#157 (comment), le second point.

Sorry again if this answer does not solve your problem @brunoocasali, tell me!

@alallema
Copy link
Contributor

And a last question did these two new methods would be implemented after?

  • client.getTasks that calls /tasks
  • client.getTask that calls /tasks/:uid

@brunoocasali brunoocasali merged commit 4268f0c into bump-meilisearch-v0.25.0 Jan 17, 2022
@brunoocasali brunoocasali deleted the upgrade-tasks-api branch January 17, 2022 18:02
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 skip-changelog The PR will not appear in the release changelogs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants