Skip to content

Fix input encoding and error parsing for HTTP calls.#428

Merged
artursouza merged 3 commits into
dapr:masterfrom
artursouza:fix_http_encoding
Dec 30, 2020
Merged

Fix input encoding and error parsing for HTTP calls.#428
artursouza merged 3 commits into
dapr:masterfrom
artursouza:fix_http_encoding

Conversation

@artursouza

Copy link
Copy Markdown
Contributor

Description

Fix input encoding and error parsing for HTTP calls.

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #427

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

Comment thread sdk/src/main/java/io/dapr/client/DaprHttp.java Outdated

@wcs1only wcs1only left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changes look good, but do you have a test that reproduces #427 that is now passing?

return OBJECT_MAPPER.readValue(json, DaprError.class);
} catch (JsonParseException e) {
throw new DaprException("UNKNOWN", new String(json, StandardCharsets.UTF_8));
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is this supposed to be here? What are the cases where this is called?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This can happen if Dapr does not unescape correctly and returns a bad response at the HTTP server prior to the request being processed by dapr logic. I reproduced this with daprd with a bad patch from another PR - so I thought it would be good to have a better experience in the SDK if that kind of corner case happens.

String urlDeleteState = DaprClientHttp.STATE_PATH + "/" + deletedStateKey;
String urlExistingState = DaprClientHttp.STATE_PATH + "/" + existingState;
String urlDeleteState = STATE_PATH + "/" + deletedStateKey;
String urlExistingState = STATE_PATH + "/" + existingState;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It seems little wierd to do the work in the tests of building a path and then splitting it again because the API takes string[]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It was to minimize changes.

Comment thread sdk-actors/src/main/java/io/dapr/actors/client/DaprHttpClient.java Outdated

@wcs1only wcs1only left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for adding a test.

@codecov

codecov Bot commented Dec 30, 2020

Copy link
Copy Markdown

Codecov Report

Merging #428 (d2e434a) into master (4c1773c) will decrease coverage by 0.10%.
The diff coverage is 95.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #428      +/-   ##
============================================
- Coverage     83.40%   83.29%   -0.11%     
- Complexity      839      840       +1     
============================================
  Files            72       72              
  Lines          2489     2485       -4     
  Branches        259      260       +1     
============================================
- Hits           2076     2070       -6     
- Misses          287      289       +2     
  Partials        126      126              
Impacted Files Coverage Δ Complexity Δ
sdk/src/main/java/io/dapr/client/DaprHttp.java 89.36% <77.77%> (-1.85%) 26.00 <2.00> (+1.00) ⬇️
...ain/java/io/dapr/actors/client/DaprHttpClient.java 100.00% <100.00%> (ø) 3.00 <1.00> (ø)
...in/java/io/dapr/actors/runtime/DaprHttpClient.java 80.70% <100.00%> (ø) 13.00 <6.00> (ø)
...k/src/main/java/io/dapr/client/DaprClientHttp.java 92.68% <100.00%> (-0.21%) 78.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c1773c...9e7cc0b. Read the comment docs.

@artursouza artursouza merged commit 2714402 into dapr:master Dec 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HTTP input is not properly encoded.

3 participants