support custom JSON (de)serialization by delegating to message if it implements json.Marshaler/json.Unmarshaler #325

jhump · 2017-03-31T01:41:46Z

This is achieved by delegating to the message if it implements json.Marshaler or json.Unmarshaler.

FWIW, this could simplify how the well-known types are handled (e.g. no need to special-case them in this package).

Both the binary and text encodings support the same concept, where a message can implement a particular interface (proto.Marshaler/proto.Unmarshaler, encoding.TextMarshaler/encoding.TextUnmarshaler) to customize serialization/de-serialization. This just extends that same concept to JSON encoding/decoding.

cybrcodr · 2017-05-19T21:09:07Z

I had some discussion with @zombiezen on the approach for this. We have a concern with the use of json.Marshaler or json.Unmarshaler as interfaces for doing custom serialization or deserialization because the jsonpb.Marshaler (or Unmarshaler) options would not be passed on and hence ignored. This means that if a dynamic message contains other non-dynamic proto messages or WKT, it would not be able to consistently use the same options in (de)serializing those fields/subfields.

How about defining the following interfaces?

type JSONPBMarshaler interface {
    MarshalJSONPB(Marshaler) ([]byte, error)
}

type JSONPBUnmarshaler interface {
    UnmarshalJSONPB(Unmarshaler, []byte) error
}

Let me know if you're amenable to our suggestion. Thanks!

jhump · 2017-05-19T23:03:15Z

Let me know if you're amenable to our suggestion. Thanks!

Sure, that sounds good. I can probably make the changes this weekend. I'll ping you after I've pushed them.

…implements json.Marshaler/json.Unmarshaler

…nmarshaler settings

jhump · 2017-05-22T14:57:01Z

@cybrcodr, the suggested changes were quite minimal. So the patch is still nice and small.

FYI, when I tried to run the tests, one of them failed. But it's not my change as it also fails when I roll my commits back (e.g. it fails on master). Maybe some sort of CI tests should be configured to run against every pull request, to prevent such regressions? Here's the test output:

jsonpb_test.go:570: Any with message: can't unmarshal nested Any proto: json: cannot unmarshal string into Go value of type bool
jsonpb_test.go:570: Any with message and indent: can't unmarshal nested Any proto: json: cannot unmarshal string into Go value of type bool
FAIL	github.com/golang/protobuf/jsonpb	0.012s

awalterschulze · 2017-05-22T14:59:55Z

I suspect you are using go 1.7 and not go 1.8?

…

On Mon, 22 May 2017, 16:57 Joshua Humphries, ***@***.***> wrote: @cybrcodr <https://github.com/cybrcodr>, the suggested changes were quite minimal. So the patch is still nice and small. FYI, when I tried to run the tests, one of them failed. But it's not my change as it also fails when I roll my commits back (e.g. it fails on master). Maybe some sort of CI tests should be configured to run against every pull request, to prevent such regressions? Here's the test output: jsonpb_test.go:570: Any with message: can't unmarshal nested Any proto: json: cannot unmarshal string into Go value of type bool jsonpb_test.go:570: Any with message and indent: can't unmarshal nested Any proto: json: cannot unmarshal string into Go value of type bool FAIL github.com/golang/protobuf/jsonpb 0.012s — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#325 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvsLaUt2ubYbJigtMqCGklSyaP6bcu6ks5r8aI_gaJpZM4MvH7M> .

jhump · 2017-05-22T15:10:33Z

@awalterschulze correct. Should this project require 1.8? We are still using 1.7 in our prod environments (and we also have GAE code [standard runtime] code that uses protobufs and runs on Go 1.6).

awalterschulze · 2017-05-22T15:25:12Z

Thats a question for the maintainers of this library. I am also interested in the answer.

jhump · 2017-05-22T18:02:55Z

jsonpb/jsonpb.go

+// also implement JSONPBUnmarshaler so that the custom format can be
+// parsed.
+type JSONPBMarshaler interface {
+	MarshalJSONPB(*Marshaler) ([]byte, error)


@cybrcodr, I wasn't sure if this struct could ever become stateful, so I have it passed by ref. Should this instead be pass-by-value? I know it currently just represents marshaling options; but it seemed like, from the name, it could at some point change to such that a pointer is the correct way to pass it.

(same goes for passing Unmarshaler by pointer in the other interface)

Pass by ref sounds good.

cybrcodr · 2017-05-22T18:27:19Z

I've discussed with @dsnet regarding Go versions. We'd like to support at least the latest 1.6 for now (till Go AppEngine default has been upgraded). I've filed issue #356 to fix these tests. I don't know why you're seeing different errors though, but we'll investigate further.

And yes, we agree that we need to set up CI for this project #357.

cybrcodr

Please add unit tests for these.

Current TestUnmarshaling uses comparison of text proto, which should be changed to use proto.Equal instead. Anyways, I'm not sure if you can use that for testing your changes, if not, either change the test to use proto.Equal (which I'm thinking of doing anyways) or write a separate test for it and I'll clean up later.

Thanks!

cybrcodr · 2017-05-22T20:01:49Z

jsonpb/jsonpb.go

+// also implement JSONPBUnmarshaler so that the custom format can be
+// parsed.
+type JSONPBMarshaler interface {
+	MarshalJSONPB(*Marshaler) ([]byte, error)


Pass by ref sounds good.

cybrcodr · 2017-05-22T20:08:15Z

jsonpb/jsonpb.go

@@ -528,6 +553,9 @@ func (u *Unmarshaler) UnmarshalNext(dec *json.Decoder, pb proto.Message) error {
 	if err := dec.Decode(&inputValue); err != nil {
 		return err
 	}
+	if jsu, ok := pb.(JSONPBUnmarshaler); ok {


How about moving this inside unmarshalValue instead? And I think it may need to be after block that allocates memory for pointer fields, i.e. 597-600.

You can do type assertion using ...
if jsu, ok := target.Addr().Interface().(JSONPBUnmarshaler); ok { ... }

jhump · 2017-05-22T20:48:44Z

@cybrcodr: I addressed your comments. I did not change the existing unmarshal test: simplest way to test customized marshaling/unmarshaling is a hand-written message, which won't work with proto.Equals anyway (its implementation relies on struct tags in generated messages).

(Also, my current environment is Go 1.7, in which the TestUnmarshaling case fails. So it felt safest for me to leave it alone.)

PTAL

cybrcodr

Looks good. Just minor nits. Thanks!

cybrcodr · 2017-05-22T21:16:48Z

jsonpb/jsonpb_test.go

+
+// Implements protobuf.Message but is not a normal generated message type. Provides
+// implementations of JSONPBMarshaler and JSONPBUnmarshaler for JSON support.
+type notGeneratedMessage struct {


Optional. How about just naming this as dynamicMessage?

cybrcodr · 2017-05-22T21:23:52Z

jsonpb/jsonpb_test.go

+		t.Errorf("an unexpected error occurred when marshalling JSONPBMarshaler: %v", err)
+	}
+	if str != rawJson {
+		t.Errorf("Marshalling JSON produced incorrect output\nExpected: %s\nGot: %s", rawJson, str)


A couple of nits.
(1) Start with lowercase for error message for consistency, i.e. "marshalling ...".
(2) display got first before want.
"marshalling JSON produced incorrect output: got %s want %s"

jhump · 2017-05-22T22:21:48Z

@cybrcodr - hopefully it's ready to merge now

Thanks for reviewing!

cybrcodr · 2017-05-26T03:40:34Z

Fyi. I just discovered issue #360 related to this. Let me know if you're interested in providing a fix, else, I'll try to give it a shot next week. Thanks.

This was referenced Apr 3, 2017

types/dynamicpb: add support for dynamic messages #199

Closed

jsonpb should support custom JSON serialization #331

Closed

dsnet added the jsonpb label May 19, 2017

jhump added 2 commits May 22, 2017 10:43

support custom JSON (de)serialization by delegating to message if it …

e7d161e

…implements json.Marshaler/json.Unmarshaler

use custom marshal/unmarshal interfaces that can preserve marshaler/u…

e8c65fd

…nmarshaler settings

jhump force-pushed the jh/support-custom-json-marshalling-unmarshalling branch from 4ba9ac5 to e8c65fd Compare May 22, 2017 14:53

jhump commented May 22, 2017

View reviewed changes

cybrcodr reviewed May 22, 2017

View reviewed changes

add tests; move JSONPBUnmarshaler check into unmarshalValue

cbc0d77

cybrcodr approved these changes May 22, 2017

View reviewed changes

address nits

a26be53

cybrcodr merged commit a4e8f93 into golang:master May 22, 2017

jhump mentioned this pull request May 23, 2017

dynamic message implements JSONPBMarshaler/JSONPBUnmarshaler jhump/protoreflect#27

Merged

kyessenov mentioned this pull request Aug 18, 2017

Unified config api istio/api#152

Merged

jhump mentioned this pull request Feb 2, 2018

jsonpb: change Marshal/Unmarshal to return error if any required field is not set #472

Merged

cybrcodr mentioned this pull request Jan 16, 2020

APIv2: Support magic methods on messages in MarshalJSONPB, etc. #1017

Closed

golang locked and limited conversation to collaborators Jun 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support custom JSON (de)serialization by delegating to message if it implements json.Marshaler/json.Unmarshaler #325

support custom JSON (de)serialization by delegating to message if it implements json.Marshaler/json.Unmarshaler #325

jhump commented Mar 31, 2017

cybrcodr commented May 19, 2017

jhump commented May 19, 2017

jhump commented May 22, 2017

awalterschulze commented May 22, 2017 via email

jhump commented May 22, 2017

awalterschulze commented May 22, 2017

jhump May 22, 2017

jhump May 22, 2017

cybrcodr May 22, 2017

cybrcodr commented May 22, 2017

cybrcodr left a comment

cybrcodr May 22, 2017

cybrcodr May 22, 2017

jhump May 22, 2017

jhump commented May 22, 2017 •

edited

Loading

cybrcodr left a comment

cybrcodr May 22, 2017

jhump May 22, 2017

cybrcodr May 22, 2017

jhump May 22, 2017

jhump commented May 22, 2017

cybrcodr commented May 26, 2017

support custom JSON (de)serialization by delegating to message if it implements json.Marshaler/json.Unmarshaler #325

support custom JSON (de)serialization by delegating to message if it implements json.Marshaler/json.Unmarshaler #325

Conversation

jhump commented Mar 31, 2017

cybrcodr commented May 19, 2017

jhump commented May 19, 2017

jhump commented May 22, 2017

awalterschulze commented May 22, 2017 via email

jhump commented May 22, 2017

awalterschulze commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cybrcodr commented May 22, 2017

cybrcodr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhump commented May 22, 2017 • edited Loading

cybrcodr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhump commented May 22, 2017

cybrcodr commented May 26, 2017

jhump commented May 22, 2017 •

edited

Loading