Skip to content

#1452 simplify MethodsCompletionException implementation #1453

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

#1452 simplify MethodsCompletionException implementation #1453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 3 additions & 17 deletions slack-api-client/src/main/java/com/slack/api/methods/MethodsCompletionException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,13 @@
package com.slack.api.methods;

import lombok.Data;
import lombok.EqualsAndHashCode;
import lombok.extern.slf4j.Slf4j;

import java.io.IOException;

/**
* Wrapped exception holding the cause exception occurred in AsyncMethodsClient
*/
@Data
@Slf4j
@EqualsAndHashCode(callSuper = false)
@EqualsAndHashCode(callSuper = true)
public class MethodsCompletionException extends RuntimeException {

private final IOException ioException;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let me share my thoughts as former maintainer of this project:

The underlying API client (MethodsClient) can throw any of these, and the current implementation provides getters to access them. So, it's not because of the breaking change, these data structure should not be eliminated. I hear that having three types here could be unnecessary for most cases, but this could be still beneficial for other users.

If your concern is the lack of easy-to-use getMessage() method, the internal code could be changed just to override getMessage() to return the underlying exception's message string data. Only one of these three usually exists, so checking the existence of them one by one within overridden getMessage() method should work for the purpose.

This could be a minor breaking change, so ideally it should be considered in the next major version. But in practice, having such a change with sufficient notice in release notes should be okay too.

private final SlackApiException slackApiException;
private final Exception otherException;

public MethodsCompletionException(IOException ioException, SlackApiException slackApiException, Exception otherException) {
this.ioException = ioException;
this.slackApiException = slackApiException;
this.otherException = otherException;
public MethodsCompletionException(Exception cause) {
super(cause);
}

}
8 changes: 4 additions & 4 deletions slack-api-client/src/main/java/com/slack/api/methods/impl/AsyncRateLimitExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ private <T extends SlackApiResponse> T runWithoutQueue(
return handleIOException(teamId, methodName, e);
} catch (SlackApiException e) {
logSlackApiException(teamId, methodName, e);
throw new MethodsCompletionException(null, e, null);
throw new MethodsCompletionException(e);
}
}

Expand Down Expand Up @@ -171,7 +171,7 @@ private <T extends SlackApiResponse> T enqueueThenRun(
if (e.getResponse().code() == 429) {
return enqueueThenRun(messageId, teamId, methodName, params, methodsSupplier);
}
throw new MethodsCompletionException(null, e, null);
throw new MethodsCompletionException(e);
} catch (InterruptedException e) {
log.error("Got an InterruptedException (error: {})", e.getMessage(), e);
throw new RuntimeException(e);
Expand All @@ -180,12 +180,12 @@ private <T extends SlackApiResponse> T enqueueThenRun(

private static <T extends SlackApiTextResponse> T handleRuntimeException(String teamId, String methodName, RuntimeException e) {
log.error("Got an exception while calling {} API (team: {}, error: {})", methodName, teamId, e.getMessage(), e);
throw new MethodsCompletionException(null, null, e);
throw new MethodsCompletionException(e);
}

private static <T extends SlackApiTextResponse> T handleIOException(String teamId, String methodName, IOException e) {
log.error("Failed to connect to {} API (team: {}, error: {})", methodName, teamId, e.getMessage(), e);
throw new MethodsCompletionException(e, null, null);
throw new MethodsCompletionException(e);
}

private static void logSlackApiException(String teamId, String methodName, SlackApiException e) {
Expand Down
3 changes: 1 addition & 2 deletions slack-api-client/src/test/java/test_locally/api/methods/EqualityTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ public void test() {
.message("")
.build();

assertEquals(new MethodsCompletionException(null, null, null), new MethodsCompletionException(null, null, null));
assertEquals(new SlackApiException(response, ""), new SlackApiException(response, ""));
assertEquals(new MethodsCompletionException(new SlackApiException(response, "")), new MethodsCompletionException(new SlackApiException(response, "")));
}
}
6 changes: 5 additions & 1 deletion slack-api-client/src/test/java/test_locally/api/methods/ExceptionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,17 @@
import com.slack.api.methods.MethodsCompletionException;
import org.junit.Test;

import java.io.IOException;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;

public class ExceptionsTest {

@Test
public void createMethodsCompletionException() {
MethodsCompletionException exception = new MethodsCompletionException(null, null, null);
MethodsCompletionException exception = new MethodsCompletionException(new IOException("IO error"));
assertNotNull(exception);
assertEquals("IO error", exception.getMessage());
}
}
Loading