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

Unable to get the http response details by Dapr HttpBinding if the state code is not 200 #783

Closed
ParrySMS opened this issue Sep 19, 2022 · 12 comments
Assignees
Labels
good first issue Good for newcomers kind/bug Something isn't working P1
Milestone

Comments

@ParrySMS
Copy link

Expected Behavior

I have service A, and service A will call service B by the DaprClient. All thing works when service B returns the response with state code 200.

    @Autowired
    private DaprClient client;
   
    @Override
    public Mono<BBBResponse> getUserData(String realms, String accessToken) {
        var metadata = setUpHeaderParams(accessToken);
        var queryParams = setUpQueryParams("m_getUserData");
        var path = toQueryParamsPath(String.format(AUTHENTICATE_PATH, realms), queryParams);
        metadata.put("path", path);
        return client.invokeBinding(properties.getHttpBindingName(), DaprBindingOperation.POST.getValue(), null, metadata, BBBResponse.class);
    }

If the state code is not 200, I want to get the original response details. The service B will return a response with a 401 state code like this one:

{
    "code": "BS_0002"
    "message": "The access token is expired."
}

Actual Behavior

I will get an error like this one if I get the 401 from Service B.

"error when invoke output binding aaa-service-http-binding: received status code 401."

There should be a way to catch this HTTP binding exception or somehow get the original response. I need to get the original response to do something.

Steps to Reproduce the Problem

Use a DaprClient to call other APIs by function invokeBinding().

Get a response but the state code is not 200.

No way to get the orginal response but a dapr error like "error when invoke output binding XXXX : received status code YYY."

Release Note

RELEASE NOTE:

@ParrySMS ParrySMS added the kind/bug Something isn't working label Sep 19, 2022
@ParrySMS
Copy link
Author

ParrySMS commented Oct 22, 2022

The root cause here but I am not sure how to fix it.

You can check this function doInvokeApi in the DaprHttp.java.

  /**
   * Invokes an API that returns a text payload.
   *
   * @param method        HTTP method.
   * @param pathSegments  Array of path segments (/a/b/c -> ["a", "b", "c"]).
   * @param urlParameters Parameters in the URL
   * @param content       payload to be posted.
   * @param headers       HTTP headers.
   * @param context       OpenTelemetry's Context.
   * @return CompletableFuture for Response.
   */
  private CompletableFuture<Response> doInvokeApi(String method,
                               String[] pathSegments,
                               Map<String, List<String>> urlParameters,
                               byte[] content, Map<String, String> headers,
                               Context context)

In the line 320, it enqueue a ResponseFutureCallback. It converts the okhttp3 response into the response object expected internally by the SDK.

this.httpClient.newCall(request).enqueue(new ResponseFutureCallback(future));

You can check the function onResponse in the ResponseFutureCallback.

  /**
   * Converts the okhttp3 response into the response object expected internally by the SDK.
   */
  private static class ResponseFutureCallback implements Callback { 
   ... 
   @Override
    public void onResponse(@NotNull Call call, @NotNull okhttp3.Response response) throws IOException {
      if (!response.isSuccessful()) {
        try {
          DaprError error = parseDaprError(getBodyBytesOrEmptyArray(response));
          if ((error != null) && (error.getErrorCode() != null)) {
            if (error.getMessage() != null) {
              future.completeExceptionally(new DaprException(error));
            } else {
              future.completeExceptionally(
                  new DaprException(error.getErrorCode(), "HTTP status code: " + response.code()));
            }
            return;
          }

          future.completeExceptionally(new DaprException("UNKNOWN", "HTTP status code: " + response.code()));
          return;
        } catch (DaprException e) {
          future.completeExceptionally(e);
          return;
        }
      }

      Map<String, String> mapHeaders = new HashMap<>();
      byte[] result = getBodyBytesOrEmptyArray(response);
      response.headers().forEach(pair -> {
        mapHeaders.put(pair.getFirst(), pair.getSecond());
      });
      future.complete(new Response(result, mapHeaders, response.code()));
    }   
  }

line 372

DaprError error = parseDaprError(getBodyBytesOrEmptyArray(response));

It converts the okhttp3.Response to DaprError, then it will throw DaprException.

The DaprException is just an exception so we can not get the original reponse details.

It seem that we need to add a params like Function<byte[],? extends DaprError> daprErrorParser or Function<okhttp3.Response,? extends DaprError> daprErrorParser to let people get the original reponse details.

DaprError error = daprErrorParser.apply(getBodyBytesOrEmptyArray(response));

Then we may need to add other field in the DaprError and DaprException, to save some original reponse details(such as the original body). I am not sure about how to make it work for this part.

If we fix this issue in this way, here is how it works in the services to use invokeBinding

    @Autowired
    private DaprClient client;
   
    @Override
    public Mono<BBBResponse> getUserData(String realms, String accessToken) {
        var metadata = setUpHeaderParams(accessToken);
        var queryParams = setUpQueryParams("m_getUserData");
        var path = toQueryParamsPath(String.format(AUTHENTICATE_PATH, realms), queryParams);
        Function<byte[],MyCustomDaprError> daprErrorParser = ( json -> { .... } );
        metadata.put("path", path);
        return client.invokeBinding(
                       properties.getHttpBindingName(), 
                       DaprBindingOperation.POST.getValue(), 
                       null,
                       metadata,
                       BBBResponse.class,
                       daprErrorParser
                  ).onErrorMap(DaprException.class, e -> {
                       if(e.getDaprError() instanceof MyCustomDaprError){
                           var myDaprError = (MyCustomDaprError) e.getDaprError();
                           var response = parseToBBBResponse(myDaprError.getOriginalBody());
                           return new MyCustomException(response);
                       }
                       return e;
               });   

    }

@ParrySMS
Copy link
Author

@yaron2 @artursouza @skyao @CrazyHZM Could you please have a look? Please correct me if anything wrong.

@johnewart
Copy link
Contributor

Part of the underlying issue is that the HTTP binding explicitly treats non-200 status codes in the response as an error and returns an error object. This translates to the response data being lost in both the HTTP API and the gRPC API because the error condition causes the API to return no data (other than the error). I opened a PR in components-contrib to allow for setting a metadata field in the request to suppress this behavior (keeping the default behavior unless explicitly changed).

The binding could also be changed to support setting this flag on the binding component itself but it seems like this behavior may be accidentally depended on and so changing it at the binding level might cause unintended errors whereas this allows per-invocation granularity.

This would allow you to prevent an exception from being raised by doing something like:

public Mono<BBBResponse> getUserData(String realms, String accessToken) {
    var metadata = setUpHeaderParams(accessToken);
    var queryParams = setUpQueryParams("m_getUserData");
    var path = toQueryParamsPath(String.format(AUTHENTICATE_PATH, realms), queryParams);
    metadata.put("path", path);
    // Add new metadata flag here
    metadata.put("treatNon200AsError", "false");
    return client.invokeBinding(properties.getHttpBindingName(), DaprBindingOperation.POST.getValue(), null, metadata, BBBResponse.class);
}

@ParrySMS
Copy link
Author

Thank @johnewart.

According to the latest code and test(https://github.com/dapr/components-contrib/blob/master/bindings/http/http_test.go), the metadata name should be "errorIfNot2XX" but not "treatNon200AsError".

    metadata.put("errorIfNot2XX", "false");

I will try to use this flag as a work around.

Excepting a better solution in SDK v1.8.

@artursouza artursouza modified the milestones: v1.8, v1.9 Feb 1, 2023
@ParrySMS
Copy link
Author

ParrySMS commented Feb 7, 2023

Here is another idea to get the original response refer to the WebClientResponseException(org.springframework.web.reactive.function.client)

There are some API to get more details when you meet error.

image

@artursouza @johnewart

@ParrySMS
Copy link
Author

Hi, @johnewart

Thank you for your sharing, but I still get an exception using the "errorIfNot2XX" metedata. Here is my impl:


 private Map<String, String> setUpHeaderParams(String accessToken) {
        var headerParams = new HashMap<String, String>();
        headerParams.put(HttpHeaders.AUTHORIZATION, accessToken);
        headerParams.put(HttpHeaders.CONTENT_TYPE, "application/json");
        headerParams.put(HttpHeaders.ACCEPT, MediaType.ALL_VALUE);
        headerParams.put("accept-language", "en");
        // work around flag for https://github.com/dapr/java-sdk/issues/783
        headerParams.put("errorIfNot2XX", "false");
        return headerParams;
    }

public Mono<BBBResponse> getUserData(String realms, String accessToken) {
    var metadata = setUpHeaderParams(accessToken);
    var queryParams = setUpQueryParams("m_getUserData");
    var path = toQueryParamsPath(String.format(AUTHENTICATE_PATH, realms), queryParams);
    metadata.put("path", path);
    // try to use Map to get the response when http 200 and 401
    return client.invokeBinding(properties.getHttpBindingName(), DaprBindingOperation.POST.getValue(), null, metadata, Map.class);
                .map(responseMap -> {
                    // temp test dapr client for non-200 error case
                    var json = JsonUtils.toJSONString(responseMap);
                    log.debug("responseMap:{}", json);
                    try {
                        return JsonUtils.parseObject(json, parameterizedTypeReferenceClazz);
                    } catch (Exception e1) {
                        log.debug("JsonUtils.parseObject error");
                        throw InternalException.unexpectedException();
                    }
                })
                .onErrorMap(Throwable.class, e -> {
                    // temp test dapr client for non-200 error case
                    var msg = "";
                    if (e instanceof DaprException) {
                        msg = "DaprException, message:" + e.getMessage()
                                + ", suppressed:" + JsonUtils.toJSONString(e.getSuppressed())
                                + ", stack:" + JsonUtils.toJSONString(e.getStackTrace());
                        log.error(msg);
                        return InternalException.unexpectedException(msg);
                    }
                    msg = "Unknown exception: " + e.getClass().getName() + ", message: " + e.getMessage();
                    log.error(msg);
                    return InternalException.unexpectedException(msg);
                });
}

And the invokeBinding still return the onError signal and get a DaprException. The response is as follow:

{
    "code": "BBB_500_0000",
    "message": "DaprException, message:INTERNAL: error when invoke output binding bbb-http-binding: received status code 401, suppressed:[], stack:[{\"classLoaderName\":null,\"moduleName\":null,\"moduleVersion\":null,\"methodName\":\"propagate\",\"fileName\":\"DaprException.java\",\"lineNumber\":194,\"className\":\"io.dapr.exceptions.DaprException\",\"nativeMethod\":false},{\"classLoaderName\":null,\"moduleName\":null,\"moduleVersion\":null,\"methodName\":\"onError\",\"fileName\":\"DaprClientGrpc.java\",\"lineNumber\":964,\"className\":\"io.dapr.client.DaprClientGrpc$2\",\"nativeMethod\":false},{\"classLoaderName\":null,\"moduleName\":null,\"moduleVersion\":null,\"methodName\":\"onClose\",\"fileName\":\"ClientCalls.java\",\"lineNumber\":478,\"className\":\"io.grpc.stub.ClientCalls$StreamObserverToCallListenerAdapter\",\"nativeMethod\":false},{\"classLoaderName\":null,\"moduleName\":null,\"moduleVersion\":null,\"methodName\":\"onClose\",\"fileName\":\"TracingClientInterceptor.java\",\"lineNumber\":159,\"className\":\"io.opentelemetry.javaagent.shaded.instrumentation.grpc.v1_6.TracingClientInterceptor$TracingClientCall$TracingClientCallListener\",\"nativeMethod\":false},{\"classLoaderName\":null,\"moduleName\":null,\"moduleVersion\":null,\"methodName\":\"closeObserver\",\"fileName\":\"ClientCallImpl.java\",\"lineNumber\":562,\"className\":\"io.grpc.internal.ClientCallImpl\",\"nativeMethod\":false},{\"classLoaderName\":null,\"moduleName\":null,\"moduleVersion\":null,\"methodName\":\"access$300\",\"fileName\":\"ClientCallImpl.java\",\"lineNumber\":70,\"className\":\"io.grpc.internal.ClientCallImpl\",\"nativeMethod\":false},{\"classLoaderName\":null,\"moduleName\":null,\"moduleVersion\":null,\"methodName\":\"runInternal\",\"fileName\":\"ClientCallImpl.java\",\"lineNumber\":743,\"className\":\"io.grpc.internal.ClientCallImpl$ClientStreamListenerImpl$1StreamClosed\",\"nativeMethod\":false},{\"classLoaderName\":null,\"moduleName\":null,\"moduleVersion\":null,\"methodName\":\"runInContext\",\"fileName\":\"ClientCallImpl.java\",\"lineNumber\":722,\"className\":\"io.grpc.internal.ClientCallImpl$ClientStreamListenerImpl$1StreamClosed\",\"nativeMethod\":false},{\"classLoaderName\":null,\"moduleName\":null,\"moduleVersion\":null,\"methodName\":\"run\",\"fileName\":\"ContextRunnable.java\",\"lineNumber\":37,\"className\":\"io.grpc.internal.ContextRunnable\",\"nativeMethod\":false},{\"classLoaderName\":null,\"moduleName\":null,\"moduleVersion\":null,\"methodName\":\"run\",\"fileName\":\"SerializingExecutor.java\",\"lineNumber\":133,\"className\":\"io.grpc.internal.SerializingExecutor\",\"nativeMethod\":false},{\"classLoaderName\":null,\"moduleName\":\"java.base\",\"moduleVersion\":\"11.0.18\",\"methodName\":\"runWorker\",\"fileName\":\"ThreadPoolExecutor.java\",\"lineNumber\":1128,\"className\":\"java.util.concurrent.ThreadPoolExecutor\",\"nativeMethod\":false},{\"classLoaderName\":null,\"moduleName\":\"java.base\",\"moduleVersion\":\"11.0.18\",\"methodName\":\"run\",\"fileName\":\"ThreadPoolExecutor.java\",\"lineNumber\":628,\"className\":\"java.util.concurrent.ThreadPoolExecutor$Worker\",\"nativeMethod\":false},{\"classLoaderName\":null,\"moduleName\":\"java.base\",\"moduleVersion\":\"11.0.18\",\"methodName\":\"run\",\"fileName\":\"Thread.java\",\"lineNumber\":829,\"className\":\"java.lang.Thread\",\"nativeMethod\":false}]",
    "data": null
}

@maciek047
Copy link

Hi, will it be addressed in the 1.8 release? I'm having the same issue; because any response with status code >= 300 is treated as error and is attempted to be mapped to DaprError. Will it be possible to pass the original error in the response, without the necessity of using the DaprError ?
image

@artursouza
Copy link
Member

We will try. We need volunteers in the Java SDK.

@MatejNedic
Copy link
Contributor

@artursouza assign me please.

@ParrySMS
Copy link
Author

ParrySMS commented Jul 3, 2023

@artursouza @MatejNedic @maciek047 is there any updated?

@artursouza artursouza modified the milestones: v1.10, v1.11 Oct 3, 2023
@artursouza
Copy link
Member

This PR should allow the raw payload to be parsed by the application's code: #1009

@artursouza artursouza self-assigned this Feb 16, 2024
@artursouza
Copy link
Member

Reopening as the other PR did not fix this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers kind/bug Something isn't working P1
Projects
None yet
Development

No branches or pull requests

5 participants