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.

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

Fix Error handling and add test coverage for Thrift2ProtoAdapter #950

Merged
merged 1 commit into from
Nov 8, 2024
Merged

Fix Error handling and add test coverage for Thrift2ProtoAdapter #950

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
26 changes: 18 additions & 8 deletions src/main/java/com/uber/cadence/internal/compatibility/Thrift2ProtoAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@
import io.grpc.StatusRuntimeException;
import java.util.UUID;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ForkJoinPool;
import java.util.concurrent.TimeUnit;
import org.apache.thrift.TException;
Expand Down Expand Up @@ -203,7 +204,7 @@
RestartWorkflowExecutionRequest restartRequest)
throws BadRequestError, ServiceBusyError, DomainNotActiveError, LimitExceededError,
EntityNotExistsError, ClientVersionNotSupportedError, TException {
throw new IllegalArgumentException("unimplemented");
throw new UnsupportedOperationException("unimplemented");
}

@Override
Expand Down Expand Up @@ -851,7 +852,7 @@
public void RestartWorkflowExecution(
RestartWorkflowExecutionRequest restartRequest, AsyncMethodCallback resultHandler)
throws TException {
throw new IllegalArgumentException("unimplemented");
throw new UnsupportedOperationException("unimplemented");
}

@Override
Expand Down Expand Up @@ -880,7 +881,7 @@
resultHandler.onComplete(
ResponseMapper.startWorkflowExecutionAsyncResponse(response));
} catch (Exception e) {
resultHandler.onError(e);
handleAsyncException(resultHandler, e);
}
},
ForkJoinPool.commonPool());
Expand Down Expand Up @@ -1003,7 +1004,7 @@
com.uber.cadence.api.v1.SignalWorkflowExecutionResponse response = resultFuture.get();
resultHandler.onComplete(null);
} catch (Exception e) {
resultHandler.onError(e);
handleAsyncException(resultHandler, e);
}
},
ForkJoinPool.commonPool());
Expand All @@ -1025,7 +1026,7 @@
SignalWithStartWorkflowExecutionAsyncRequest signalWithStartRequest,
AsyncMethodCallback resultHandler)
throws TException {
throw new IllegalArgumentException("unimplemented");
throw new UnsupportedOperationException("unimplemented");
}

@Override
Expand Down Expand Up @@ -1199,7 +1200,7 @@
com.uber.cadence.api.v1.StartWorkflowExecutionResponse response = resultFuture.get();
resultHandler.onComplete(ResponseMapper.startWorkflowExecutionResponse(response));
} catch (Exception e) {
resultHandler.onError(e);
handleAsyncException(resultHandler, e);
}
},
ForkJoinPool.commonPool());
Expand Down Expand Up @@ -1230,7 +1231,7 @@
resultHandler.onComplete(
ResponseMapper.startWorkflowExecutionAsyncResponse(response));
} catch (Exception e) {
resultHandler.onError(e);
handleAsyncException(resultHandler, e);
}
},
ForkJoinPool.commonPool());
Expand Down Expand Up @@ -1276,7 +1277,7 @@
resultHandler.onComplete(
ResponseMapper.getWorkflowExecutionHistoryResponse(response));
} catch (Exception e) {
resultHandler.onError(e);
handleAsyncException(resultHandler, e);
}
},
ForkJoinPool.commonPool());
Expand All @@ -1293,4 +1294,13 @@
throws TException {
throw new UnsupportedOperationException("not implemented");
}

private void handleAsyncException(AsyncMethodCallback callback, Exception exception) {
if (exception instanceof ExecutionException
&& exception.getCause() instanceof StatusRuntimeException) {
callback.onError(ErrorMapper.Error(((StatusRuntimeException) exception.getCause())));
} else {
callback.onError(exception);

Check warning on line 1303 in src/main/java/com/uber/cadence/internal/compatibility/Thrift2ProtoAdapter.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/com/uber/cadence/internal/compatibility/Thrift2ProtoAdapter.java#L1303 

Added line #L1303 was not covered by tests
}
}
}
59 changes: 59 additions & 0 deletions src/test/java/com/uber/cadence/internal/compatibility/ProtoObjects.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1324,6 +1324,65 @@ public final class ProtoObjects {
public static final UpdateDomainResponse UPDATE_DOMAIN_RESPONSE =
UpdateDomainResponse.newBuilder().setDomain(DOMAIN).build();

public static final GetSearchAttributesRequest GET_SEARCH_ATTRIBUTES_REQUEST =
GetSearchAttributesRequest.getDefaultInstance();

public static final RegisterDomainResponse REGISTER_DOMAIN_RESPONSE =
RegisterDomainResponse.getDefaultInstance();

public static final DeprecateDomainResponse DEPRECATE_DOMAIN_RESPONSE =
DeprecateDomainResponse.getDefaultInstance();

public static final SignalWorkflowExecutionResponse SIGNAL_WORKFLOW_EXECUTION_RESPONSE =
SignalWorkflowExecutionResponse.getDefaultInstance();

public static final RequestCancelWorkflowExecutionResponse
REQUEST_CANCEL_WORKFLOW_EXECUTION_RESPONSE =
RequestCancelWorkflowExecutionResponse.getDefaultInstance();

public static final TerminateWorkflowExecutionResponse TERMINATE_WORKFLOW_EXECUTION_RESPONSE =
TerminateWorkflowExecutionResponse.getDefaultInstance();

public static final GetClusterInfoRequest GET_CLUSTER_INFO_REQUEST =
GetClusterInfoRequest.getDefaultInstance();

public static final RespondDecisionTaskFailedResponse RESPOND_DECISION_TASK_FAILED_RESPONSE =
RespondDecisionTaskFailedResponse.getDefaultInstance();

public static final RespondActivityTaskCompletedResponse
RESPOND_ACTIVITY_TASK_COMPLETED_RESPONSE =
RespondActivityTaskCompletedResponse.getDefaultInstance();

public static final RespondActivityTaskCompletedByIDResponse
RESPOND_ACTIVITY_TASK_COMPLETED_BY_ID_RESPONSE =
RespondActivityTaskCompletedByIDResponse.getDefaultInstance();

public static final RespondActivityTaskFailedResponse RESPOND_ACTIVITY_TASK_FAILED_RESPONSE =
RespondActivityTaskFailedResponse.getDefaultInstance();

public static final RespondActivityTaskFailedByIDResponse
RESPOND_ACTIVITY_TASK_FAILED_BY_ID_RESPONSE =
RespondActivityTaskFailedByIDResponse.getDefaultInstance();

public static final RespondActivityTaskCanceledResponse RESPOND_ACTIVITY_TASK_CANCELED_RESPONSE =
RespondActivityTaskCanceledResponse.getDefaultInstance();

public static final RespondActivityTaskCanceledByIDResponse
RESPOND_ACTIVITY_TASK_CANCELED_BY_ID_RESPONSE =
RespondActivityTaskCanceledByIDResponse.getDefaultInstance();

public static final RespondQueryTaskCompletedResponse RESPOND_QUERY_TASK_COMPLETED_RESPONSE =
RespondQueryTaskCompletedResponse.getDefaultInstance();

public static final ResetStickyTaskListResponse RESET_STICKY_TASK_LIST_RESPONSE =
ResetStickyTaskListResponse.getDefaultInstance();

public static final RefreshWorkflowTasksRequest REFRESH_WORKFLOW_TASKS_REQUEST =
RefreshWorkflowTasksRequest.getDefaultInstance();

public static final RefreshWorkflowTasksResponse REFRESH_WORKFLOW_TASKS_RESPONSE =
RefreshWorkflowTasksResponse.getDefaultInstance();

Comment on lines +1384 to +1385
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure how much we care about these, but if we do care about them, is there some kind of assertion we can add to ensure that they're mapped for all future fields too? (maybe reflection?)

Copy link
Member Author

@natemort natemort Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For any type that we have a mapper for we use MapperTestUtil to assert that all fields are set (or a select set are missing if we haven't updated the mappers for them). We do similar things with some of the types contained within the requests(decisions and history events) as well as enums.

These types don't actually exist in thrift (the thrift APIs return void, something we never do in gRPC) so there aren't any mappers. It seems very unlikely we'd add fields to these types until after we completely remove thrift, at which point this Thrift2Proto layer won't exist at all. So I don't think it's worth testing these ones the way all the others are tested.

private ProtoObjects() {}

private static Payload payload(String value) {
Expand Down
Loading