Skip to content

Basic Error Handler #19

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

Closed
10 tasks done
curquiza opened this issue May 15, 2020 · 3 comments
Closed
10 tasks done

Basic Error Handler #19

curquiza opened this issue May 15, 2020 · 3 comments

Comments

@curquiza
Copy link
Member

curquiza commented May 15, 2020

I create this issue as a continuation of this one that is an investigation specifically about naming for error handling 🙂

The goal of this issue is to detail the basic requirements of the SDKs error handlers.
I would like to keep this discussion as clear as possible, it means if you think I forgot basic points you can continue the discussion here with really clear explanations. Or you can contact me directly (if you can) so that I can update this post 😊

Goal

The goal of an error handler in an SDK is to say to the user "this package (so MeiliSearch) you are using, among the multitude of packages, raises an error".
That's why our SDK should not return an Axios (JS client library) or Httparty (equivalent in ruby) error. Otherwise, it becomes complicated for the user to understand where exactly the error comes from.

What kind of error?

The name of the error is really important, and should give 2 pieces of information:

  • the error comes from MeiliSearch SDK, by adopting the prefix/namespace MeiliSearch
  • the general kind of the error, by giving an explicit error name.

Following the previous issue about error handler naming, here are the 3 minimum errors the SDKs should raise:

  • MeiliSearchApiError: MeiliSearch (HTTP) sends back a 4xx or a 5xx. There are most of the SDKs errors.
  • MeiliSearchCommunicationError: problem with communication, for example, the MeiliSearch instance is unreachable.

    the suggestion in the previous issue is CommunicationError but I really think it's important that the name of MeiliSearch appears in the error, otherwise it becomes complicated for the user to understand where the error comes from.

  • MeiliSearchError: for any other errors in the SDK (wrong parameter if the SDK checks the parameters...etc)

Remark

Some language allows namespacing. So the raised error could be MeiliSearch::ApiError instead of MeiliSearchApiError. It depends on the language. We should adopt the best practice of each language 🙂

For example, in the Ruby and PHP SDKs, we used the namespace MeiliSearch.
In the JS one, we use the prefix MeiliSearch instead, directly in the name of the error.

Also, depending on the language, the Error could be changed as Exception for example (ex: PHP).

Another suggestion:

Even if we define a generic MeiliSearchApiError we can also define more specific errors concerning the API. I particularly think about an error for This index already exists.
But I suggest we talk about that in another issue once all the SDKs will have their own error handler according to this issue 🙂
Just telling this is possible 😉

What kind of display with the language interpreter?

The name of the raised error is great, but not enough. We should display at least an error message.
In the case of the generic MeilISearchApiError, the expection message has to be the message of the MeiliSearch (HTTP) response.
Again for the generic MeiliSearchApiError, if it's possible, we should also display the HTTP code error. I say "if it's possible" because all the language interpreter does not use the same way to display exception raising. We should at least manage to display the error message 🙂

Capture d’écran 2020-05-15 à 11 10 37

(I know, the naming `MeiliSearch HTTPRequestException` should be `MeiliSearch ApiException`)

Which methods?

Depending on the good practices of each language, we should also provide methods in our custom errors. The goal of these methods is to help the user during his/her code implementation and for debugging.

Generally, a to_sring or message method has to be implemented.
For example, with the PHP example, the __toString method is the method called by the interpreter to display the raising exception.

For the generic MeiliSearchApiError, a method/attribute should be provided for the HTTP code (ex: httpCode) and for the HTTP message response (ex: httpMessage or message).

Final words

The goal of this issue is to stay basic, that's why I did not detail more what to do, especially because it depends a lot on each language.

If some methods or other kind of error implementation seems really essential to you, it will deserve its own issue on this repository 🙂 We should stay simple for the beginning with these 3 kinds of error.

Tell me if some parts are not clear to you, deserve more explanation, or if I forgot some problematic points in my logic.

TODO on different SDKs

  • meilisearch-dart
  • meilisearch-dotnet
  • meilisearch-go
  • meilisearch-java
  • meilisearch-js
  • meilisearch-php
  • meilisearch-python
  • meilisearch-ruby
  • meilisearch-rust
  • meilisearch-swift
@curquiza curquiza added integration guides need vote When several solutions are suggested labels May 15, 2020
@curquiza curquiza added need approval A final choice is suggested and removed need vote When several solutions are suggested labels May 18, 2020
@bidoubiwa
Copy link
Contributor

bidoubiwa commented May 28, 2020

Is the Http Error 400 not a little too much? Index already exists is enough information for the user. Since it is in the middle of a string it is not an information that the user can extract and act upon dynamically. The error message should contain the information he needs to understand the bug, which is Index already exists.

Of course, I agree that if there is no message the Http error code should be displayed.

Exemple with your suggestion in python with error message:

Traceback (most recent call last):
File "create/two_index.py", line 14, in <module>
    index = client.create_index(uid="movies1")
  File "/python_meili/meilisearch/client.py", line 52, in create_index
    index.create(self.config, uid=uid, primary_key=primary_key, name=name)
  File "/python_meili/meilisearch/index.py", line 113, in create
    return HttpRequests(config).post(config.paths.index, payload)
  File "/python_meili/meilisearch/_httprequests.py", line 37, in post
    return self.__validate(request)
  File "/python_meili/meilisearch/_httprequests.py", line 78, in__validate
    raise MeiliSearchApiError(err, request) from None
meilisearch.meilisearch_api_error.MeiliSearchApiError: MeiliSearchApiError, HTTP status: 400 -> Impossible to create index; index already exists

In case without error message :

Traceback (most recent call last):
  File "create/two_index.py", line 14, in <module>
    index = client.create_index(uid="movies1")
  File "/meili/python_meili/meilisearch/client.py", line 52, in create_index
    index.create(self.config, uid=uid, primary_key=primary_key, name=name)
  File "/meili/python_meili/meilisearch/index.py", line 113, in create
    return HttpRequests(config).post(config.paths.index, payload)
  File "/meili/python_meili/meilisearch/_httprequests.py", line 37, in post
    return self.__validate(request)
  File "/meili/python_meili/meilisearch/_httprequests.py", line 78, in __validate
    raise MeiliSearchApiError(err, request) from None
meilisearch.meilisearch_api_error.MeiliSearchApiError: MeiliSearchApiError, 400 Client Error: Bad Request for url: http://127.0.0.1:7700/indexes

@curquiza
Copy link
Member Author

Is the Http Error 400 not a little too much? Index already exists is enough information for the user.

After discussing we think (in the majority) that we should display as much information as possible, especially because we are doing a basic error handler. Plus, this information is display only with the interpreter, so for debugging.

@curquiza curquiza added ready to implement and removed need approval A final choice is suggested labels Jun 9, 2020
bors bot added a commit to meilisearch/meilisearch-php that referenced this issue Feb 24, 2021
143: Rename MeiliSearch\HTTPRequestException into MeiliSearch\ApiException r=bidoubiwa a=claudiunicolaa

Rename `MeiliSearch\HTTPRequestException` into `MeiliSearch\ApiException`

Explained here meilisearch/integration-guides#19 and discussed here #50.  

I hope I understood correctly your intention 😃 

Co-authored-by: Claudiu Nicola <[email protected]>
bors bot added a commit to meilisearch/meilisearch-php that referenced this issue Feb 24, 2021
144: Add MeiliSearch\Exceptions\CommunicationException r=bidoubiwa a=claudiunicolaa

Add a new exception. It will handle the connection errors as explained here meilisearch/integration-guides#19. 

`Http\Client\Exception\NetworkException` exceptions will be catch and resend as `MeiliSearch\Exceptions\CommunicationException` 


Co-authored-by: Claudiu Nicola <[email protected]>
Co-authored-by: claudiunicolaa <[email protected]>
bors bot added a commit to meilisearch/meilisearch-php that referenced this issue Feb 24, 2021
144: Add MeiliSearch\Exceptions\CommunicationException r=bidoubiwa a=claudiunicolaa

Add a new exception. It will handle the connection errors as explained here meilisearch/integration-guides#19. 

`Http\Client\Exception\NetworkException` exceptions will be catch and resend as `MeiliSearch\Exceptions\CommunicationException` 


Co-authored-by: Claudiu Nicola <[email protected]>
Co-authored-by: claudiunicolaa <[email protected]>
@curquiza
Copy link
Member Author

curquiza commented Oct 6, 2021

Closing this since it's finally implemented everywhere :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants