Simple error handler

[bidoubiwa]

Error handling in python

Custom error created

• MeiliSearchApiError: Throws when API returns bad status code
• MeiliSearchCommunicationError: Throws when problem accessing the API

Usage

# test.py
import meilisearch


client = meilisearch.Client("http://127.0.0.1:7700")
try:
    index = client.create_index(uid="movies1")
    index = client.create_index(uid="movies1")
except meilisearch.meilisearch_api_error.MeiliSearchApiError as err:
    print(err)
except meilisearch.meilisearch_communication_error.MeiliSearchCommunicationError as err:
    print(err)

MeiliSearchApiError will output

MeiliSearchApiError, HTTP status: 400 -> Impossible to create index; index already exists

Implementation

MeiliSearchApiError

import json

class MeiliSearchApiError(Exception):
    """Error from MeiliSearch API"""
    def __init__(self, error, request):
        self.status_code = request.status_code
        if request.text:
            self.message = f'HTTP status: {self.status_code} -> {json.loads(request.text)["message"]}'
        else:
            self.message = error
        super().__init__(self.message)

    def __str__(self):
        return f'MeiliSearchApiError, {self.message}'

This error will use the default HTTP error message if MeiliSearch did not return a message.

It is called in the __validate method of the httpRequests class:

try:
     request.raise_for_status()
     return HttpRequests.__to_json(request)
except requests.exceptions.HTTPError as err:
     raise MeiliSearchApiError(err, request) from None

By adding from None we prevent the chaining of errors.

MeiliSearchCommunicationError

class MeiliSearchCommunicationError(Exception):
    """Error when communicating with MeiliSearch"""
    def __init__(self, message):
        self.message = message
        super().__init__(self.message)

    def __str__(self):
        return f'MeiliSearchCommunicationError, {self.message}'

This error will use the default error thrown by the HTTP request library to display the communication problem.

Mentionned in: /meilisearch/integration-guides#19

[curquiza]

Too bad for the code duplication with MeiliSearchCommunicationError, but I don't find any other way to do that! What do you think @eskombro?

Can you add tests for your exceptions? 🙂 Especially testing the Communication error (you can put localhost:8800 if I'm not wrong). I know you have to wait for the @eskombro merge 😉

Are these changes breaking for you guys? @eskombro @bidoubiwa
For me, they are.
If they are, can you rename the apikey parameter into api_key please? :)

[curquiza]

Suggested change

[curquiza]

Suggested change

	raise MeiliSearchCommunicationError(err) from None
	raise MeiliSearchCommunicationError(err) from None

[curquiza]

What happens if the is no "message" in the body? (it should not happen but it would be bad if there is an exception in the exception ^^ There was a time where MeiliSearch did not return any message for some HTTP error = 4XX or 5XX)

[eskombro]

Right, should check if there's a message before setting it to the internat attribute. And have a default value like "No message received" for example

[curquiza]

Don't you think that the user would believe that there is an error because "there is no message" with this default value? @eskombro

[eskombro]

Maybe a simple unknown can solve that?

[curquiza]

@bidoubiwa I need confirmation about usage.

With the MeiliSearchCommunicationError, if can write:

except meilisearch.meilisearch_communication_error.MeiliSearchCommunicationError as err:
    print(err.message)

And with MeiliSearchApiError:

except meilisearch.meilisearch_api_error.MeiliSearchApiError as err:
    print(err.status_code)
    print(err.message)

Can you confirm that?
Because the __str__ method is here for debugging (= display in the terminal) but http_status and message are mandatory for programming.

[curquiza]

Like your work about Error handler anyway! It was needed!!

[eskombro]

This error handler is great, and message is way better that returning the http error. I really like it.

I think that those error classes should be internal classes to HttpRequests class, and not separate classes in the package, what do you think @curquiza and @bidoubiwa ?

Or, we can have a MeiliSearchException (or MeiliSearchError) class and those classes inherit from the Super Error class, modifying just what is specific

:)

[eskombro]

Right, should check if there's a message before setting it to the internat attribute. And have a default value like "No message received" for example

[curquiza]

I think that those error classes should be internal classes to HttpRequests class, and not separate classes in the package, what do you think @curquiza and @bidoubiwa ?

Not sure doing internal class error for the HTTPRequest class would be a good practice. I found these both separated files in Algolia and stripe packages.

Or, we can have a MeiliSearchException (or MeiliSearchError) class and those classes inherit from the Super Error class, modifying just what is specific

Why not! I tried to start a thing like that in our ruby SDK but completely useless for the moment (because of the lazy "we'll see later" implementation, not the idea 😂)

[eskombro]

We checked with @bidoubiwa how several 'famous' packages handle errors. Seemds like conclusions are:

1. Validate the Super error class idea, with inheriting for every specific error.
2. Unvalidating the "internal class" idea. Nevertheless, errors are in one single file at the 'root' (same level as Index, Client)

We also agreed on adding tests, @bidoubiwa waits for #80 to be merged to implement the tests

Too bad for the code duplication with MeiliSearchCommunicationError, but I don't find any other way to do that! What do you think @eskombro?

I agree, but every way of implementing DRY I can think of makes me feel like we would be adding unnecessary complexity and loosing in readability, so I would prefer to accpt @bidoubiwa 's proposition

Are these changes breaking for you guys? @eskombro @bidoubiwa
For me, they are.

I think we should consider them as breaking. This has an impact an almost every return value

[eskombro]

Ah, PS: Also, removing from None after raising an error, to have all the chain of errors in the console

[curquiza]

👍