Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

JSONPb marshaler panics if input is nil interface #639

Merged

Conversation

jhump
Copy link
Contributor

@jhump jhump commented May 2, 2018

The JSONBuiltin marshaler emits "null" when marshaling a nil interface, but JSONPb panics:

panic: reflect: call of reflect.Value.Interface on zero Value

goroutine 6 [running]:
testing.tRunner.func1(0xc4201982d0)
  /usr/local/go/src/testing/testing.go:711 +0x2d2
panic(0x1506500, 0xc4201bee20)
  /usr/local/go/src/runtime/panic.go:491 +0x283
reflect.valueInterface(0x0, 0x0, 0x0, 0x1, 0x0, 0xc4201cf340)
  /usr/local/go/src/reflect/value.go:936 +0x1bf
reflect.Value.Interface(0x0, 0x0, 0x0, 0x0, 0x0)
  /usr/local/go/src/reflect/value.go:931 +0x44
github.com/grpc-ecosystem/grpc-gateway/runtime.(*JSONPb).marshalNonProtoField(0xc420173f80, 0x0, 0x0, 0x0, 0x0, 0xc4201ccd00, 0xc4201cf368, 0x1)
  /Users/jh/go/src/github.com/grpc-ecosystem/grpc-gateway/runtime/marshal_jsonpb.go:81 +0x59e
github.com/grpc-ecosystem/grpc-gateway/runtime.(*JSONPb).Marshal(0xc420173f80, 0x0, 0x0, 0x1, 0xc4201ccd01, 0x1, 0x0, 0x0)
  /Users/jh/go/src/github.com/grpc-ecosystem/grpc-gateway/runtime/marshal_jsonpb.go:27 +0x16f
github.com/grpc-ecosystem/grpc-gateway/runtime_test.TestJSONPbMarshalFields(0xc4201982d0)
  /Users/jh/go/src/github.com/grpc-ecosystem/grpc-gateway/runtime/marshal_jsonpb_test.go:126 +0xe6
testing.tRunner(0xc4201982d0, 0x15da798)
  /usr/local/go/src/testing/testing.go:746 +0xd0
created by testing.(*T).Run
  /usr/local/go/src/testing/testing.go:789 +0x2de

@jhump
Copy link
Contributor Author

jhump commented May 2, 2018

@achew22, @yugui: we're in the process of converting some REST APIs to use protos, not all of which actually go through grpc-gateway generated handlers, but manual handlers that already return non-proto objects to serialize to JSON. So we're starting to use grpc-gateway's marshaler to aid in the transition, since it can marshal both protos and non-protos.

But we have some code that uses nil as a sentinel response, expecting it to be serialized as "null". That works with encoding/json, but caused grpc-gateway's JSONPb marshaler to panic. This fixes the panic.

@jhump jhump force-pushed the jh/jsonpb-marshaler-should-handle-nil branch from 72a162a to 3ea4cd3 Compare May 2, 2018 19:31
@codecov-io
Copy link

Codecov Report

Merging #639 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #639      +/-   ##
==========================================
+ Coverage   58.88%   58.91%   +0.02%     
==========================================
  Files          30       30              
  Lines        2853     2855       +2     
==========================================
+ Hits         1680     1682       +2     
  Misses       1010     1010              
  Partials      163      163
Impacted Files Coverage Δ
runtime/marshal_jsonpb.go 70% <100%> (+0.61%) ⬆️

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 c2b051d...3ea4cd3. Read the comment docs.

@achew22 achew22 merged commit 31596c4 into grpc-ecosystem:master May 3, 2018
adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants