js-sdk icon indicating copy to clipboard operation
js-sdk copied to clipboard

[WIP] Correct HTTP response status for subscription

Open DeepanshuA opened this issue 3 years ago • 3 comments

Signed-off-by: Deepanshu Agarwal [email protected]

Description

This intends to fix n/ack of http response status correctly during subscription, as per https://docs.dapr.io/reference/api/pubsub_api/#expected-http-response

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: #333

Checklist

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

  • [x] Code compiles correctly
  • [ ] Created/updated tests
  • [ ] Extended the documentation

DeepanshuA avatar Aug 02 '22 20:08 DeepanshuA

Codecov Report

Merging #334 (ce8416b) into main (d3f2bc0) will increase coverage by 9.34%. The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main     #334      +/-   ##
==========================================
+ Coverage   25.43%   34.78%   +9.34%     
==========================================
  Files          76       77       +1     
  Lines        6164     6170       +6     
  Branches      205      205              
==========================================
+ Hits         1568     2146     +578     
+ Misses       4592     4005     -587     
- Partials        4       19      +15     
Impacted Files Coverage Δ
src/implementation/Server/HTTPServer/pubsub.ts 17.64% <25.00%> (+5.14%) :arrow_up:
src/enum/SubscribedMessageHttpResponse.enum.ts 100.00% <100.00%> (ø)
src/implementation/Client/GRPCClient/state.ts 5.00% <0.00%> (+1.00%) :arrow_up:
.../implementation/Client/GRPCClient/configuration.ts 7.27% <0.00%> (+1.81%) :arrow_up:
src/proto/dapr/proto/runtime/v1/dapr_pb.js 30.68% <0.00%> (+2.57%) :arrow_up:
src/implementation/Client/GRPCClient/lock.ts 16.00% <0.00%> (+4.00%) :arrow_up:
src/utils/Settings.util.ts 70.83% <0.00%> (+4.16%) :arrow_up:
src/implementation/Client/GRPCClient/metadata.ts 16.66% <0.00%> (+4.16%) :arrow_up:
src/implementation/Client/GRPCClient/secret.ts 13.63% <0.00%> (+4.54%) :arrow_up:
src/proto/dapr/proto/runtime/v1/dapr_grpc_pb.js 10.85% <0.00%> (+6.85%) :arrow_up:
... and 22 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar Aug 02 '22 20:08 codecov[bot]

Thank you @DeepanshuA, this was really fast!

Quick question - were you able to test the existing behaviour? Also it would be great to add e2e tests for this if possible, or we can create an issue to track it separately.

shubham1172 avatar Aug 03 '22 05:08 shubham1172

Thank you @DeepanshuA, this was really fast!

Quick question - were you able to test the existing behaviour? Also it would be great to add e2e tests for this if possible, or we can create an issue to track it separately.

Yeah, I was able to test the existing behavior and after this change. Earlier, it always landed in runtime with appResponse.Status being blank ("") https://github.com/dapr/dapr/blob/91af1367f29d77f16f8acee1cfff53830bc42b63/pkg/runtime/runtime.go#L1719. But, now it lands either with status: "SUCCESS" or status: "RETRY".

I have created a separate issue for adding e2e test: https://github.com/dapr/js-sdk/issues/335

DeepanshuA avatar Aug 03 '22 08:08 DeepanshuA