Skip to content
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

Refining exception throws #678

Open
junghoon-vans opened this issue Oct 29, 2023 · 6 comments
Open

Refining exception throws #678

junghoon-vans opened this issue Oct 29, 2023 · 6 comments
Labels
enhancement New feature or request

Comments

@junghoon-vans
Copy link
Contributor

junghoon-vans commented Oct 29, 2023

Description

Currently, the exceptions thrown are all MeilisearchException.
This makes it difficult for the user to specify which exception was thrown.

Basic example

The way to fix this is to throw all exceptions as-is.
Instead, the practice of creating and throwing MeilsearchException directly should be eliminated.

AS-IS

public <T> HttpResponse<T> execute(HttpRequest request) throws MeilisearchException {
    try {
        Request okRequest = buildRequest(request);
        Response response = client.newCall(okRequest).execute();

        return buildResponse(response);
    } catch (MalformedURLException e) {
        throw new MeilisearchException(e); // do not throw MeilisearchException directly!
    } catch (SocketTimeoutException e) {
        throw new MeilisearchTimeoutException(e);
    } catch (IOException e) {
        throw new MeilisearchCommunicationException(e);
    }
}

If the user wants to get a specific exception, they have to compare if it is castable and perform the cast.

TO-BE

public <T> HttpResponse<T> execute(HttpRequest request)
    throws MeilisearchURLException, MeilisearchTimeoutException, MeilisearchCommunicationException {
    try {
        Request okRequest = buildRequest(request);
        Response response = client.newCall(okRequest).execute();

        return buildResponse(response);
    } catch (MalformedURLException e) {
        throw new MeilisearchURLException(e); // for example
    } catch (SocketTimeoutException e) {
        throw new MeilisearchTimeoutException(e);
    } catch (IOException e) {
        throw new MeilisearchCommunicationException(e);
    }
}

With this change, users will be able to receive verbose exceptions(e.g. MeilisearchURLException..) as they are,
or as a MeilisearchException as they do now.

Other
This issue has great synergy when applied with #677.
If all exceptions were RuntimeException, we wouldn't need any code to re-throw.

public <T> HttpResponse<T> execute(HttpRequest request) { // we don't have to re-throw!
    try {
        Request okRequest = buildRequest(request);
        Response response = client.newCall(okRequest).execute();

        return buildResponse(response);
    } catch (MalformedURLException e) {
        throw new MeilisearchURLException(e); // for example
    } catch (SocketTimeoutException e) {
        throw new MeilisearchTimeoutException(e);
    } catch (IOException e) {
        throw new MeilisearchCommunicationException(e);
    }
}
@curquiza
Copy link
Member

Hello @junghoon-vans

thanks for the suggestion

Feel free to open PRs so that we better see what you mean. I'm not sure I like the MeilisearchURLException because the meaning is not explicit 😅 but I trust you for the global change!

Feel free to tackle #677 too

@curquiza curquiza added the enhancement New feature or request label Oct 30, 2023
@junghoon-vans
Copy link
Contributor Author

@curquiza Thank you for believing in me.
I've been trying to figure out how to make this change, and it's been a while.
For now, I've only raised a PR for issue #677.

@239yash
Copy link

239yash commented Mar 28, 2024

@curquiza Hi, Can I take a hit on this. I was thinking to revamp all the exception throws in the code.

@curquiza
Copy link
Member

Hello @239yash

Feel free to take this 😊 thank you very much!

For your information, we prefer not assigning people to our issues because sometimes people ask to be assigned and never come back, which discourages the volunteer contributors from opening a PR to fix this issue.
We will accept and merge the first PR that fixes correctly and well implements the issue following our contributing guidelines.

@239yash
Copy link

239yash commented Mar 28, 2024

@curquiza I am implementing this. Thanks!

@239yash
Copy link

239yash commented Mar 28, 2024

@curquiza Please review the PR & do let me know of any changes to be made. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants