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

allow use of jsonpb for marshaling #79

Closed
ericchiang opened this issue Dec 29, 2015 · 13 comments
Closed

allow use of jsonpb for marshaling #79

ericchiang opened this issue Dec 29, 2015 · 13 comments

Comments

@ericchiang
Copy link

github.com/golang/protobuf/jsonpb is a more compliant protobuf -> JSON package than encoding/json, and is the only way github.com/golang/protobuf plans to support JSON encoding in the future.

See this comment

The newly added jsonpb package (see 67cbcad) is the official vehicle for JSON support...

It would be great to be able to using this package for marshaling. It might even be wise to switch to it entirely.

@bufdev
Copy link
Contributor

bufdev commented Dec 29, 2015

Using it would be nice, but note that in my benchmarks, jsonpb is
approximately 6x slower at marshaling then the standard library package

On Tuesday, December 29, 2015, Eric Chiang [email protected] wrote:

github.com/golang/protobuf/jsonpb
https://godoc.org/github.com/golang/protobuf/jsonpb is a more compliant
protobuf -> JSON package than encoding/json, and is the only way
github.com/golang/protobuf plans to support JSON encoding in the future.

See this comment
golang/protobuf#44 (comment)

The newly added jsonpb package (see 67cbcad) is the official vehicle for
JSON support...

It would be great to be able to using this package for marshaling. It
might even be wise to switch to it entirely.


Reply to this email directly or view it on GitHub
#79.

@ericchiang
Copy link
Author

A lot of the overhead comes from checking to ensure the result is actually compliant with protobuf's defined JSON mapping. It's slower, but it's correct :)

@bufdev
Copy link
Contributor

bufdev commented Dec 29, 2015

Ya, I know :) Ugh. Programming is no fun :)

On Tuesday, December 29, 2015, Eric Chiang [email protected] wrote:

A lot of the overhead comes from checking to ensure the result is actually
compliant with protobuf's defined JSON mapping
https://developers.google.com/protocol-buffers/docs/proto3#json. It's
slower, but it's correct :)


Reply to this email directly or view it on GitHub
#79 (comment).

@philips
Copy link

philips commented Dec 30, 2015

So, someone needs to build a codegen ala ffjson

On Tue, Dec 29, 2015 at 11:24 PM Peter Edge [email protected]
wrote:

Ya, I know :) Ugh. Programming is no fun :)

On Tuesday, December 29, 2015, Eric Chiang [email protected]
wrote:

A lot of the overhead comes from checking to ensure the result is
actually
compliant with protobuf's defined JSON mapping
https://developers.google.com/protocol-buffers/docs/proto3#json. It's
slower, but it's correct :)


Reply to this email directly or view it on GitHub
<#79 (comment)
.


Reply to this email directly or view it on GitHub
#79 (comment).

@yugui
Copy link
Member

yugui commented Jan 8, 2016

@peter-edge Thank you for your information. I didn't know that it is that slower.
Then, it sounds better to have an option to keep using encoding/json, although jsonpb is mandatory for users who wants Any, Timestamp types, oneof fields and others.

I am thinking of a mechanism which allows users to register/overwrite marshaler per MIME type.
With this mechanism, jsonpb would be a good default marshaler for application/json but you could keep using encoding/json if you don't need new proto3 types of fields.

@tmc
Copy link
Collaborator

tmc commented Mar 18, 2016

@yugui can you sketch out what sort of approach you were thinking? I may hack on this soon.

@hbchai
Copy link
Contributor

hbchai commented Mar 19, 2016

The jsonpb.Marshaler recently changed its default behavior to output camelCase, so not using jsonpb within grpc-gateway is now a compatibility issue. jsonpb users will have to set OrigName: true when creating the Marshaler to work around this, which took me a while to figure out and is less than ideal. I would like to see support land soon.

@bufdev
Copy link
Contributor

bufdev commented Mar 19, 2016

@yugui @tmc @hbchai don't want to repeat work, but I'd propose what looks like (based on an overly-simple grep) a very small PR:

For code generation:

--grpc-gateway_out=jsonlib={std,jsonpb}:/path/to/etc/etc...

For library code:

var (
  // DefaultJSONAdapter would wrap stdlib JSON functions
  DefaultJSONAdapter = &defaultJSONAdapter{}
  // JSONPBAdapter would wrap jsonpb
  JSONPBAdapter = &jsonpbAdapter{}
  globalJSONAdapter = DefaultJSONAdapter
)

// not trying too hard here with naming or interface definitions, just trying to show the concept
type JSONAdapter interface {
  Marshal(proto.Message) ([]byte, error)
  Unmarshal([]byte) (proto.Message, error)
}

func SetJSONAdapter(jsonAdapter JSONAdapter) {
  globalJSONAdapter = jsonAdapter
}

func jsonMarshal(message proto.Message) ([]byte, error) {
  return globalJSONAdapter.Marshal(message)
}

func jsonUnmarshal(data []byte) (proto.Message, error) {
  return globalJSONAdapter.Unmarshal(data)
}

@hbchai
Copy link
Contributor

hbchai commented Mar 21, 2016

@peter-edge It might make more sense for the choice of library to be a new option to the generated RegisterXYZHandler function.

@bufdev
Copy link
Contributor

bufdev commented Mar 21, 2016

Ya actually that might be better :)

@tmc
Copy link
Collaborator

tmc commented Mar 26, 2016

this has implications for swagger specs doesn't it? Should this be described via a custom annotation?

@zackangelo
Copy link

Is this completed? Can it be closed?

@tamird
Copy link
Contributor

tamird commented Jun 21, 2016

Yep, see #144.

@yugui yugui closed this as completed Jun 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants