-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
marshal_jsonpb: Added nil slice default value #854
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this 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.
runtime/marshal_jsonpb.go
Outdated
if rv.Kind() == reflect.Slice { | ||
if rv.IsNil() && j.EmitDefaults { | ||
return []byte("[]"), nil | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
👍
Thanks for the contribution! |
* marshal_jsonpb: Added nil slice default value * marshal_jsonpb: reduced indentation
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 theproto.Message
interface, and therefore is marshalled locally in theruntime.JSONPb
implementation asnull
even ifEmitDefaults
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!