More fine grained exceptions

[devrck]

Hi all,

The current implementation of HTTPRequestException is to vague and of course allows you to build on it but maybe it's nice to have the more fine grained exceptions already in the package https://github.com/meilisearch/MeiliSearch/blob/master/meilisearch-core/src/error.rs, https://github.com/meilisearch/MeiliSearch/blob/master/meilisearch-error/src/lib.rs .

Wanted to start with this contribution but wanted first to ask you guys if that is ok and if you agree with it?

[curquiza]

Hello @devrck!

That would be really great !! 😄

Can you quickly explain here what kind of errors you want to add? Like a list of the new exceptions? 🙂

FYI: we thought about renaming MeiliSearch\HTTPRequestException into MeiliSearch\ApiException. See the issue we detailed for a really basic error handler. If you can keep the naming we decided for the 3 generalistic errors, but that is not integrated into this package yet, it would be perfect 🙂

[devrck]

Hi @curquiza,

What i wanted to propose is to create more than the 3, but of course I can extend from there.

For example you say MeiliSearch\ApiException I wanted to create InvalidFacetFilter that extends it, following the concrete verbose errors raised in the core lib .

<?php

namespace MeiliSearch\Exceptions;

class InvalidFacetFilter extends ApiException {
   // ...
}

The only issue I see now is the 3 errors detailed in meilisearch/integration-guides#19 are maybe conflicting because for example MeiliSearch\Error (for any other errors in the SDK, wrong parameter if the SDK checks the parameters...etc) seems like MeiliSearch\ApiException because this returns the HTTP status code 400 which means bad request because of parameters 😄 .

Maybe we can start with the list from here https://github.com/meilisearch/MeiliSearch/blob/master/meilisearch-error/src/lib.rs#L87 and put them in categories. What do you think?

[curquiza]

For example you say MeiliSearch\ApiException I wanted to create InvalidFacetFilter that extends it, following the concrete verbose errors raised in the core lib .

Seems perfect! 😄

The only issue I see now is the 3 errors detailed in meilisearch/integration-guides#19 are maybe conflicting because for example MeiliSearch\Error (for any other errors in the SDK, wrong parameter if the SDK checks the parameters...etc) seems like MeiliSearch\ApiException because this returns the HTTP status code 400 which means bad request because of parameters 😄 .

I think we were not really clear in our issue, sorry. I don't think it will be conflicting. The MeiliSearch\Error was supposed to be a really generalist error for all the other errors that are not related to HTTP or communication. For example, we could send an error for the uid should be a string in createIndex before sending it to the MeiliSearch API. But as you said, it's not really useful because the MeiliSearch API does (and also PHP does) great parsing and sends detailed errors now. It was not the case before, that's why we thought about MeiliSearch\Error. But not sure this error class is useful now.
TLDR; don't implement MeiliSearch\Error 😂

Maybe we can start with the list from here https://github.com/meilisearch/MeiliSearch/blob/master/meilisearch-error/src/lib.rs#L87 and put them in categories. What do you think?

There are already in categories 😉 When the MeiliSearch API returns an error, there is a errorType which is the category. Sorry there is no docs for now about that, it will come soon. Here are the categories you will encounter.

FYI, but not specifically useful for development:

• the messages of MeiliSearch API are going to be improved: more detailed and more relevant.
• the link in errorLink does not work yet, it's coming soon

[claudiunicolaa]

hi @curquiza,

For example, we could send an error for the uid should be a string in createIndex before sending it to the MeiliSearch API. But as you said, it's not really useful because the MeiliSearch API does (and also PHP does) great parsing and sends detailed errors now. It was not the case before, that's why we thought about MeiliSearch\Error. But not sure this error class is useful now.
TLDR; don't implement MeiliSearch\Error

What do you think is the best to do with InvalidArgumentException? Should it be removed?

[curquiza]

Hum I don't know, it is currently used in the package. Do you think about better error handling?

[claudiunicolaa]

I guess will be easier to maintain the SDK if the errors are passed from MeiliSearch API directly to the client.

From the above-quoted message, my understanding was that MeiliSearch\Error purposes are the same as InvalidArgumentException and MeiliSearch\Error not present interest anymore to be implemented - at least in this SDK. This is why I asked. 😃

[curquiza]

From the above-quoted message, my understanding was that MeiliSearch\Error purposes are the same as InvalidArgumentException and MeiliSearch\Error not present interest anymore to be implemented - at least in this SDK.

You're right, they share the same purpose 🙂 MeiliSearch returns well-detailed errors, with a link to the docs, see the docs.
And you're right, returning the MeiliSearch errors instead of doing our owns exception for each case is better. So InvalidArgumentException does not make sense anymore

However, currently the interpreter does not display all the information when raising an exception:

You can only get the message and the HTTP code error but not the link to the docs, and the errorCode neither. We can only see this information if we catch the exception, which is not really convenient.
Maybe should we add this information when the exception is raised?

[claudiunicolaa]

wdyt about this format?

PHP Fatal error:  Uncaught MeiliSearch HTTPRequestException: Http Status: 404 - Message: Index abafefefefeca not found - Error code: index_not_found - Error type: invalid_request_error - Error link: https://docs.meilisearch.com/errors#index_not_found
  thrown in ~/oss/meilisearch/meilisearch-php/src/Http/Client.php on line 190

[curquiza]

Seems great! Do you want to open a PR? 🙂 Or do you want me to fix it instead?

[bidoubiwa]

Does #150 resolves this issue?

[curquiza]

Yes! :)