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

marshal_jsonpb: Added nil slice default value #854

Merged
merged 2 commits into from
Jan 18, 2019

Conversation

abice
Copy link
Contributor

@abice abice commented Jan 18, 2019

I noticed that when using the repeated response body, that when an empty array is returned from the grpc server, it is translated to a nil object ([]*examplepb.RepeatedResponseBodyOut_Response)(nil), which does not implement the proto.Message interface, and therefore is marshalled locally in the runtime.JSONPb implementation as null even if EmitDefaults is set on the marshaller.

I updated the marshaller to check for the case mentioned above and to make sure it behaves according to the EmitDefaults option correctly. Let me know if I missed anything on it.

Thanks!

@codecov-io
Copy link

codecov-io commented Jan 18, 2019

Codecov Report

Merging #854 into master will decrease coverage by 0.02%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #854      +/-   ##
==========================================
- Coverage   51.66%   51.63%   -0.03%     
==========================================
  Files          39       39              
  Lines        3782     3788       +6     
==========================================
+ Hits         1954     1956       +2     
- Misses       1643     1647       +4     
  Partials      185      185
Impacted Files Coverage Δ
examples/server/responsebody.go 0% <0%> (ø) ⬆️
runtime/marshal_jsonpb.go 71.15% <100%> (+0.56%) ⬆️

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 a0500cb...7098b14. Read the comment docs.

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

Great find! One small suggestion.

if rv.Kind() == reflect.Slice {
if rv.IsNil() && j.EmitDefaults {
return []byte("[]"), nil
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose we could put this all in one big if instead to reduce indentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That works for me, honestly wasn't sure if you wanted me to return "null" if j.EmitDefaults wasn't set. I can make it all one big if.

👍

@johanbrandhorst johanbrandhorst merged commit ff4dc68 into grpc-ecosystem:master Jan 18, 2019
@johanbrandhorst
Copy link
Collaborator

Thanks for the contribution!

@abice abice deleted the emit_default_slice branch January 18, 2019 23:05
adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
* marshal_jsonpb: Added nil slice default value

* marshal_jsonpb: reduced indentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants