Custom non-2xx response handling - DaprErrorResponseParser - Issue 783 #842

maciek047 · 2023-03-29T06:34:50Z

Description

This PR aims to address the problems described in issue #783. It introduces the DaprErrorResponseParser interface, which allows for setting a custom parser for the DaprHttpClient. This enables the deserialization of responses with structures different from DaprError. The changes are backward compatible, as the current logic is wrapped in the DefaultDaprErrorResponseParser.

Issue reference

#783

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Created/updated tests
Extended the documentation

Signed-off-by: Maciej <m.szymeczko@dac.digital>

* Bump from spring boot 2.3.5.RELEASE to 2.7.8 Signed-off-by: Sergio <champel@gmail.com> (cherry picked from commit 9152c91) * Ensure old versions of spring boot are still compatible Signed-off-by: Sergio <champel@gmail.com> --------- Signed-off-by: champel <champel@gmail.com> Signed-off-by: Sergio <champel@gmail.com> Signed-off-by: Maciej <m.szymeczko@dac.digital>

Signed-off-by: Maciej Szymeczko <maciek.047@gmail.com>

artursouza · 2023-04-01T07:45:41Z

sdk/src/main/java/io/dapr/client/DefaultDaprErrorResponseParser.java

+    DaprError error;
+
+    @Override
+    public DaprException parse(@NotNull okhttp3.Response response) throws IOException {


Two points here. First, can we make this interface not depend on Okhttp3? This way we can migrate to any http client in the near future.

Second point is the the http implementation will be deprecated and removed soon and gRPC will be the only implementation. Users of the SDK should not care about the protocol, except for service invocation.

So, I see two ways to proceed with this PR: make this interface protocol agnostic, making it work on with gRPC and http; Or create a new method for HTTP service invocation only and make this be used only in HTTP service invocation.

Happy to get other idea on this that would address my first two concerns.

Thanks for your input, I've added changes to address the concerns raised, the interface works now both with HTTP and gRPC and is passed via the DaprClientBuilder. Let me know if this is a good direction.

Sorry, I was out of vacation. I will review this in the next couple of days.

Signed-off-by: Maciej <m.szymeczko@dac.digital>

…o issue_783 # Conflicts: # sdk/src/main/java/io/dapr/client/DaprClientGrpc.java

Signed-off-by: Maciej <m.szymeczko@dac.digital>

artursouza

I like where this is going. Thanks a lot.

artursouza · 2023-04-24T04:54:08Z

sdk/src/main/java/io/dapr/client/DaprClientGrpc.java

+            StatusRuntimeException statusException = (StatusRuntimeException) t;
+            int statusCode = statusException.getStatus().getCode().value();
+            byte[] errorDetails = statusException.getStatus().getDescription() != null
+                    ? statusException.getStatus().getDescription().getBytes(StandardCharsets.UTF_8)


Get encoding from our properties. See the Properties.java file.

artursouza · 2023-04-24T04:55:19Z

sdk/src/main/java/io/dapr/client/DaprErrorResponseParser.java

+import java.io.IOException;
+
+public interface DaprErrorResponseParser {
+    DaprException parse(int statusCode, byte[] response) throws IOException;


What is statusCode? These are different for gRPC or HTTP. Maybe add a new param for the protocol, so the implementation can handle both. Another option would be to have one method for gRPC and one for HTTP.

My intention with the DaprErrorResponseParser interface was to provide separate implementations for handling different protocols (as demonstrated by DefaultDaprHttpErrorResponseParser and DefaultDaprGrpcErrorResponseParser). I believe this approach maintains a clean separation of concerns between gRPC and HTTP error handling, which is why I opted not to include a protocol parameter in the interface. Ofc if you think there is a strong reason for having a single implementation handling both protocols, then I can change it

artursouza · 2023-04-24T04:56:08Z

sdk/src/main/java/io/dapr/client/DefaultDaprErrorResponseParser.java

+
+    @Override
+    public DaprException parse(int statusCode, byte[] response) {
+        String errorMessage = (response == null || new String(response).isEmpty()) ? "HTTP status code: " + statusCode : new String(response, StandardCharsets.UTF_8);


Please, reference the encoding from the Properties.java file.

artursouza · 2023-04-24T04:56:18Z

sdk/src/main/java/io/dapr/client/DefaultDaprGrpcErrorResponseParser.java

+public class DefaultDaprGrpcErrorResponseParser implements DaprErrorResponseParser {
+    @Override
+    public DaprException parse(int statusCode, byte[] errorDetails) {
+        String errorMessage = new String(errorDetails, StandardCharsets.UTF_8);


artursouza · 2023-04-24T04:56:23Z

sdk/src/main/java/io/dapr/client/DefaultDaprErrorResponseParser.java

+            try {
+                error = OBJECT_MAPPER.readValue(response, DaprError.class);
+            } catch (IOException e) {
+                return new DaprException("UNKNOWN", new String(response, StandardCharsets.UTF_8));


artursouza · 2023-04-24T04:57:00Z

sdk/src/test/java/io/dapr/client/DaprHttpTest.java

@@ -139,7 +144,7 @@ public void invokeGetMethod() throws IOException {
    mockInterceptor.addRule()
      .get("http://127.0.0.1:3500/v1.0/get")
      .respond(serializer.serialize(EXPECTED_RESULT));
-    DaprHttp daprHttp = new DaprHttp(Properties.SIDECAR_IP.get(), 3500, okHttpClient);
+    DaprHttp daprHttp = new DaprHttp(Properties.SIDECAR_IP.get(), 3500, okHttpClient, DEFAULT_PARSER);


Use an overloaded version of the constructor instead. So, there will not be changes needed everywhere.

Signed-off-by: Maciej <m.szymeczko@dac.digital>

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

codecov · 2023-05-25T22:44:09Z

Codecov Report

Merging #842 (5e36001) into master (e03cb15) will decrease coverage by 0.34%.
The diff coverage is 69.87%.

@@             Coverage Diff              @@
##             master     #842      +/-   ##
============================================
- Coverage     77.82%   77.48%   -0.34%     
- Complexity     1267     1282      +15     
============================================
  Files           116      119       +3     
  Lines          3892     3936      +44     
  Branches        458      465       +7     
============================================
+ Hits           3029     3050      +21     
- Misses          630      650      +20     
- Partials        233      236       +3

Impacted Files	Coverage Δ
...ain/java/io/dapr/exceptions/DaprHttpException.java	`0.00% <0.00%> (ø)`
...apr/client/DefaultDaprGrpcErrorResponseParser.java	`33.33% <33.33%> (ø)`
...k/src/main/java/io/dapr/client/DaprClientGrpc.java	`87.25% <54.83%> (-1.82%)`	⬇️
...rc/main/java/io/dapr/client/DaprClientBuilder.java	`63.04% <66.66%> (-1.25%)`	⬇️
...apr/client/DefaultDaprHttpErrorResponseParser.java	`76.47% <76.47%> (ø)`
sdk/src/main/java/io/dapr/client/DaprHttp.java	`92.59% <100.00%> (+0.28%)`	⬆️
.../src/main/java/io/dapr/client/DaprHttpBuilder.java	`95.23% <100.00%> (+0.79%)`	⬆️

... and 1 file with indirect coverage changes

ParrySMS · 2023-07-04T03:38:36Z

@maciek047 Hi, is there any updated?

ParrySMS · 2023-11-17T08:49:38Z

Hi @maciek047 , Could you please help to add more unit test to fix the coverage issue? It will not take a long time with GPT.

cicoyle · 2023-12-21T15:02:21Z

gentle ping @maciek047 - mind updating this PR?

artursouza · 2024-02-16T20:34:26Z

Issue #783 is now resolved by #1009 - it is a different approach but makes the raw error payload available.

maciek047 requested review from a team as code owners March 29, 2023 06:34

Maciej and others added 3 commits March 29, 2023 08:53

added DaprException for custom error handling

5416573

Signed-off-by: Maciej <m.szymeczko@dac.digital>

added DaprErrorResponseParser

b341df6

Signed-off-by: Maciej <m.szymeczko@dac.digital>

maciek047 force-pushed the issue_783 branch from faa7ee8 to 6bc4c56 Compare March 29, 2023 06:54

Merge branch 'master' into issue_783

90a14b7

Signed-off-by: Maciej Szymeczko <maciek.047@gmail.com>

artursouza requested changes Apr 1, 2023

View reviewed changes

maciek047 and others added 4 commits April 5, 2023 08:48

added DaprErrorResponseParser support for Grpc client

759d028

Signed-off-by: Maciej <m.szymeczko@dac.digital>

Merge branch 'issue_783' of https://github.com/maciek047/java-sdk int…

987ac1b

…o issue_783 # Conflicts: # sdk/src/main/java/io/dapr/client/DaprClientGrpc.java

added DaprErrorResponseParser support for Grpc client

0f26f71

Signed-off-by: Maciej <m.szymeczko@dac.digital>

added DaprErrorResponseParser support for Grpc client

040d889

Signed-off-by: Maciej <m.szymeczko@dac.digital>

artursouza requested changes Apr 24, 2023

View reviewed changes

maciek047 and others added 5 commits May 2, 2023 12:02

added cr refactor

2b0edd7

Signed-off-by: Maciej <m.szymeczko@dac.digital>

added cr refactor

6ccb4f9

Signed-off-by: Maciej <m.szymeczko@dac.digital>

Merge branch 'master' into issue_783

fa7ffac

Merge branch 'master' into issue_783

83c2891

Fixes fingbus, stylecheck + some improvements.

5e36001

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

artursouza closed this Feb 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom non-2xx response handling - DaprErrorResponseParser - Issue 783 #842

Custom non-2xx response handling - DaprErrorResponseParser - Issue 783 #842

maciek047 commented Mar 29, 2023 •

edited

Loading

artursouza Apr 1, 2023

maciek047 Apr 5, 2023

artursouza Apr 18, 2023

artursouza left a comment

artursouza Apr 24, 2023

artursouza Apr 24, 2023

maciek047 May 2, 2023

artursouza Apr 24, 2023

artursouza Apr 24, 2023

artursouza Apr 24, 2023

artursouza Apr 24, 2023

codecov bot commented May 25, 2023

ParrySMS commented Jul 4, 2023

ParrySMS commented Nov 17, 2023

cicoyle commented Dec 21, 2023

artursouza commented Feb 16, 2024

Custom non-2xx response handling - DaprErrorResponseParser - Issue 783 #842

Custom non-2xx response handling - DaprErrorResponseParser - Issue 783 #842

Conversation

maciek047 commented Mar 29, 2023 • edited Loading

Description

Issue reference

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artursouza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 25, 2023

Codecov Report

ParrySMS commented Jul 4, 2023

ParrySMS commented Nov 17, 2023

cicoyle commented Dec 21, 2023

artursouza commented Feb 16, 2024

maciek047 commented Mar 29, 2023 •

edited

Loading