Skip to content

Set etag to be null if it's empty#414

Merged
artursouza merged 1 commit into
dapr:masterfrom
arghya88:fix-etag
Dec 18, 2020
Merged

Set etag to be null if it's empty#414
artursouza merged 1 commit into
dapr:masterfrom
arghya88:fix-etag

Conversation

@arghya88

@arghya88 arghya88 commented Dec 15, 2020

Copy link
Copy Markdown
Contributor

Signed-off-by: Arghya Sadhu arghya88@gmail.com

Description

Convert empty string to null for GRPC implementation to keep it consistent with HTTP implementation

Issue reference

Fixes #405

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

@arghya88 arghya88 force-pushed the fix-etag branch 2 times, most recently from a0e1733 to 95f8d0e Compare December 15, 2020 07:29

@mukundansundar mukundansundar 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.

lgtm.
@artursouza The issue linked to this should be a breaking change since the response is changing.

@artursouza

Copy link
Copy Markdown
Contributor

Yes, LGTM. Let's mark it as breaking change - even though it is minor.

Comment thread sdk/src/main/java/io/dapr/client/DaprClientGrpc.java
@arghya88 arghya88 force-pushed the fix-etag branch 10 times, most recently from 1f80e1c to 5e0dcd7 Compare December 16, 2020 14:55
Comment thread sdk-tests/src/test/java/io/dapr/it/state/AbstractStateClientIT.java Outdated
@arghya88 arghya88 force-pushed the fix-etag branch 11 times, most recently from 4c8de50 to 52cb9b7 Compare December 17, 2020 09:38
@arghya88

arghya88 commented Dec 17, 2020

Copy link
Copy Markdown
Contributor Author

@artursouza I debugged why saveAndGetBulkState test in AbstractStateClientIT is failing. It turns out

  1. For the second state object both GRPC and HTTP client returns 1 for Etag even though we set null in the object.
  2. For the third object where we don't even save it in state store HTTP client returns empty and GRPC client returns null for Etag.

I am not sure if 1 is expected to be this way. If this is expected then we can keep it this way and let the test pass.
For 2 the tests used to pass before my change because GRPC client used to return empty before my change. With my change it returns null instead of empty and hence test for GRPC client is failing.

2 is not happening in unit tests because units tests don't cover a scenario of trying to retrieve a state object which is not saved at all.

@arghya88 arghya88 force-pushed the fix-etag branch 6 times, most recently from 02d4a19 to f58fee3 Compare December 17, 2020 13:08
Signed-off-by: Arghya Sadhu <arghya88@gmail.com>
@codecov

codecov Bot commented Dec 17, 2020

Copy link
Copy Markdown

Codecov Report

Merging #414 (7c437ad) into master (7f1ce0b) will decrease coverage by 0.03%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #414      +/-   ##
============================================
- Coverage     83.27%   83.23%   -0.04%     
- Complexity      835      838       +3     
============================================
  Files            72       72              
  Lines          2487     2493       +6     
  Branches        255      258       +3     
============================================
+ Hits           2071     2075       +4     
- Misses          293      294       +1     
- Partials        123      124       +1     
Impacted Files Coverage Δ Complexity Δ
...k/src/main/java/io/dapr/client/DaprClientHttp.java 92.80% <0.00%> (-0.71%) 80.00 <0.00> (ø)
...k/src/main/java/io/dapr/client/DaprClientGrpc.java 85.81% <100.00%> (+0.19%) 86.00 <0.00> (+2.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 7f1ce0b...7c437ad. Read the comment docs.

@arghya88

Copy link
Copy Markdown
Contributor Author

@artursouza Pls review

@artursouza

Copy link
Copy Markdown
Contributor

@artursouza I debugged why saveAndGetBulkState test in AbstractStateClientIT is failing. It turns out

  1. For the second state object both GRPC and HTTP client returns 1 for Etag even though we set null in the object.
  2. For the third object where we don't even save it in state store HTTP client returns empty and GRPC client returns null for Etag.

I am not sure if 1 is expected to be this way. If this is expected then we can keep it this way and let the test pass.
For 2 the tests used to pass before my change because GRPC client used to return empty before my change. With my change it returns null instead of empty and hence test for GRPC client is failing.

2 is not happening in unit tests because units tests don't cover a scenario of trying to retrieve a state object which is not saved at all.

The reason why the etag is "1" and not null even when we set it to null is because Dapr maps etag to version in Redis. Version is always present in Redis, so it defaults to "1" when we set a value: https://github.com/dapr/components-contrib/blob/master/state/redis/redis.go

This is why the integration tests asserts "1" for the null case.

@artursouza artursouza merged commit a35e414 into dapr:master Dec 18, 2020
@arghya88 arghya88 deleted the fix-etag branch December 23, 2020 12:25
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.

States without etag is handled differently in gRPC and HTTP APIs

3 participants