Allow usage of message types with customtype override #239

jvshahid · 2016-12-09T16:02:19Z

this pr tries to fix an issue with gogoproto when a customtype is used in conjuction with a message type, e.g.:

message DesiredLRPSchedulingInfo {                                                                                                                                                              
  ...                                                                                                                                                                                           
  optional ProtoRoutes routes = 1 [(gogoproto.nullable) = false, (gogoproto.customtype) = "Routes"];                                                                                            
  ...                                                                                                                                                                                           
}                                                                                                                                                                                               
                                                                                                                                                                                                
message ProtoRoutes {                                                                                                                                                                           
  map<string, bytes> routes = 1;                                                                                                                                                                
}

The idea is to decorate the map in golang but allow non Golang (e.g. Ruby)
clients to be generated with reasonable types. The alternative of using bytes
as the type of routes in this case generate non Golang marshal methods that
are dumb and won't do any serialization/checking on the values of the routes
field.

note this pr is just a demonstration and we don't expect it to be merged as
is. we are happy for feedback and will do the necessary updates to get it
merged in.

fyi, the code sample mentioned above is from the diego bbs models that is used in cloudfoundry

jvshahid · 2016-12-09T16:23:28Z

Looks like the build failed for protoc v3.1.0, not sure why. it is hard to understand the output. If someone clarify the reason, we can look into it.

awalterschulze · 2016-12-09T16:53:15Z

Doesn't seem like you regenerated the code.
Its just doing a git diff

jvshahid · 2016-12-09T17:03:39Z

sweet, regenerated the code using protoc 3.1.0 and golang 1.7.1

jvshahid · 2016-12-09T17:03:50Z

@awalterschulze what do you think of the change ?

awalterschulze · 2016-12-10T10:34:01Z

I think we have to keep these issues in mind:

defining a customtype as a fake proto message causes problems with jsonpb, prototext and possibly some others #132
Known Issue: customtype that is another protocol buffer message with gogoprotobuf generated functions and methods is not supported #125
customtype of type struct cannot jsonpb unmarshal #201

awalterschulze · 2016-12-10T10:37:23Z

Ok 125 does not seem to apply here, since it is trying to have a proto message as the customtype and is still keeping the proto field type as type bytes.

awalterschulze · 2016-12-10T10:42:58Z

Ok maybe none of them are applicable.

awalterschulze · 2016-12-10T10:46:40Z

Basically you just want to be able to have a custom type where the proto field is of some message type and not only of type bytes.
Even though the serialization would look the same, someone from another language could deserialize these bytes into the defined proto message, but the gogo user will be able to use the custom go coded struct.
Am I understanding this correctly?

If so I am pretty sure you'll need more tests.
Would be nice to add a totally separate test struct in thetest.proto
https://github.com/gogo/protobuf/blob/master/test/thetest.proto
This should then generate a combination of tests for you, which use and don't generated code for marshaling and unmarshaling.

awalterschulze · 2016-12-10T10:47:24Z

So yes I am interested in adding this feature :)
But lets see how the tests cope first.

jvshahid · 2016-12-10T14:57:39Z

Basically you just want to be able to have a custom type where the proto field is of some message type and not only of type bytes.
Even though the serialization would look the same, someone from another language could deserialize these bytes into the defined proto message, but the gogo user will be able to use the custom go coded struct.
Am I understanding this correctly?

yes, that's right

If so I am pretty sure you'll need more tests.
Would be nice to add a totally separate test struct in thetest.proto
https://github.com/gogo/protobuf/blob/master/test/thetest.proto
This should then generate a combination of tests for you, which use and don't generated code for marshaling and unmarshaling.

sweet, i'll work on the tests and get back to you

jvshahid · 2016-12-12T16:29:36Z

so i added the tests as requested. currently there are two failres. one related to the text representation of
the proto message, which i think need to be fixed to use the right struct, it is probably doing reflection on the wrong type. the other is related to the nullability of the go custom type, which i'm not sure
about.

awalterschulze · 2016-12-12T16:54:10Z

No both are just reflection woes in the jsonpb and prototext libraries respectively.
They are always lots of "fun"

jvshahid · 2016-12-14T17:29:02Z

So I got to fixing the tests for both json and text marshaling. I'm less comfortable with the json marshaling change, since it requires the custom type to implement {Un,}MarshalJSON and will fail with a weird error if the user doesn't provide those. BTW, the same is true for text marshaling, all the weird errors i was getting previously were caused by an invalid Marshal() signature, where I had a pointer receiver but the code expected a value receiver. I think there are two examples (Uuid & Uint128) that demonstrate how to write a custom type but it's not obvious that these functions are required and must conform to a given signature. I think documenting those requirement will be beneficial. Thoughts ?

awalterschulze

Overall super positive about this :)

awalterschulze · 2016-12-14T19:18:44Z

test/example/example.proto

@@ -55,7 +55,11 @@ message A {
 message B {
 	option (gogoproto.description) = true;
 	optional A A = 1 [(gogoproto.nullable) = false, (gogoproto.embed) = true];
-	repeated bytes G = 2 [(gogoproto.customtype) = "github.com/gogo/protobuf/test/custom.Uint128", (gogoproto.nullable) = false];
+	optional G G = 2 [(gogoproto.customtype) = "github.com/gogo/protobuf/test/custom.Uint128"];


Lets not change this example or add an extra one

awalterschulze · 2016-12-14T19:19:52Z

test/thetest.proto

@@ -624,3 +624,11 @@ message Node {
 	repeated Node Children = 2;
 }

+message NonByteCustomType {
+  option (gogoproto.compare) = false;


Is compare not supported?

awalterschulze · 2016-12-14T19:20:04Z

jsonpb/jsonpb.go

 		pm, ok := iface.(proto.Message)
 		if !ok {
 			if prop.CustomType == "" {
 				return fmt.Errorf("%v does not implement proto.Message", v.Type())
 			}
 			t := proto.MessageType(prop.CustomType)
+			// fmt.Printf("################ %T, %T, %s\n", i, t, prop.CustomType)


remove comment

awalterschulze · 2016-12-14T19:20:09Z

jsonpb/jsonpb.go

@@ -725,6 +738,15 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe

 	// Handle nested messages.
 	if targetType.Kind() == reflect.Struct {
+		if target.CanAddr() {
+			// fmt.Printf("################ %T\n", target.Interface())


remove comment

awalterschulze · 2016-12-14T19:24:11Z

If you want to add documentation as you mentioned for customtype that will be really really awesome.
I think it might even warrant its own .md file which we can link to from the other docs, if you are up for it?

jvshahid · 2016-12-15T15:23:04Z

Removed the comments, made the message comparable and added a new document for custom types. With hindsight, I should not have squashed the commits to make it easier to review, sorry about that. I'm also not sure if the new documentation fixes #201 ?

awalterschulze · 2016-12-17T10:01:22Z

test/combos/unmarshaler/thetest.proto

@@ -624,3 +624,10 @@ message Node {
 	repeated Node Children = 2;
 }

+message NonByteCustomType {
+  optional ProtoType Field1 = 1 [(gogoproto.customtype) = "GoType"];


Just thought of this the other day, but can we also have structs which use nullable=false and repeated custom types, similar to NidOptCustom, NinOptCustom, NidRepCustom and NinRepCustom

awalterschulze · 2016-12-17T10:03:13Z

extensions.md

@@ -59,6 +59,7 @@ Issues with customtype include:
  * <a href="https://github.com/gogo/protobuf/issues/125">Using a proto message as a customtype is not allowed.</a>
  * <a href="https://github.com/gogo/protobuf/issues/200">cusomtype of type map can not UnmarshalText</a>
  * <a href="https://github.com/gogo/protobuf/issues/201">customtype of type struct cannot jsonpb unmarshal</a>
+  * Custom types are required to implement a handful of methods. See [CustomTypes](custom_types.md) for more information.


I think we can move all these issues to the customtypes.md file and then simply reference the customtypes.md file in the table above.

awalterschulze · 2016-12-17T10:04:10Z

custom_types.md

+  optional bytes Field = 1 [(gogoproto.customtype) = "GoType"];
+}
+
+message ProtoType {


You can remove ProtoType from the bytes example

awalterschulze · 2016-12-17T10:06:27Z

custom_types.md

+func (gt *GoType) UnmarshalJSON(data []byte) error
+
+func (gt GoType) Compare(other GoType) int // only required if the comparable option is set
+```


What about Equal and NewPopulated, etc.
I think the comment about a method being optional can be added above the method.
You can also get the highlighting to keep on working by simply adding curly braces to the function definitions.

func (r GoType) Marshal() ([]byte, error) {}

awalterschulze · 2016-12-17T10:06:48Z

custom_types.md

+or alternatively you can declare the field type in the protocol message to be
+`bytes`:
+
+``` protocol-buffer


I think its simply ```proto

awalterschulze · 2016-12-17T10:08:13Z

custom_types.md

+The custom type must define the following methods with the given
+signatures. Assuming the custom type is called `GoType`:
+
+``` go


You can also add a link to your test file go_type.go

awalterschulze · 2016-12-17T10:08:48Z

Also you are welcome to add yourself to the AUTHORS or CONTRIBUTORS file as appropriate.

awalterschulze · 2016-12-17T10:10:34Z

Yes I think might fix #201 @ericlagergren what do you think?

awalterschulze · 2016-12-17T10:11:45Z

I really like the docs, I think its really clear and will result in less confusion from users in future.
Thanks a lot for you work on this, I really appreciate it :)

ericlagergren · 2016-12-17T19:15:04Z

Docs look great! I'd suggest using T instead of GoType since I think it's more idiomatic and well known.

I could give a more in-depth look over on Sunday or Monday, I'm on my phone right now 😉

jvshahid · 2016-12-20T16:03:27Z

Sorry didn't get a chance to look into the comments until now. They all sound reasonable. I'll try to go through them and push the appropriate fixes as soon as I can. Thanks!

jvshahid · 2017-01-04T16:39:56Z

I think I covered all the feedback y'all gave me. I didn't force push to make it easier to review the changes. Once everything is okay I can squash everything and rebase on master. Let me know what you think

awalterschulze

Looks good

awalterschulze · 2017-01-04T16:54:21Z

custom_types.md

 ```
+
+Check [t.go](test/t.go) for a full example


Does this link actually work?

awalterschulze · 2017-01-04T17:01:06Z

Really good work. Thank you very much.
Please resolve the conflicts and submit again.

awalterschulze · 2017-01-04T19:03:09Z

To resolve conflicts I think you simply need to regenerate

jvshahid · 2017-01-04T20:06:10Z

Sweet. Force pushed the squashed changes. Thanks for the thorough review and accepting this pr. BTW, there were minor changes to the proto3 test suite generated files. I'm not sure if that's ok or not. Let me know if they shouldn't have changed and i can look into it.

awalterschulze · 2017-01-04T20:21:33Z

Looks fine.
Thanks again, this is really cool.

jvshahid force-pushed the customtype-with-message-type branch from 8fc2c1e to 472f7d8 Compare December 9, 2016 17:03

awalterschulze requested changes Dec 14, 2016

View reviewed changes

jvshahid force-pushed the customtype-with-message-type branch from 6a681e2 to 8bb6cdf Compare December 15, 2016 15:20

awalterschulze reviewed Dec 17, 2016

View reviewed changes

awalterschulze mentioned this pull request Dec 26, 2016

Custom Go fields in generated struct? #242

Closed

awalterschulze reviewed Jan 4, 2017

View reviewed changes

custom_types.md

```

Check [t.go](test/t.go) for a full example

Copy link

Member

awalterschulze Jan 4, 2017

Choose a reason for hiding this comment

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

Does this link actually work?

awalterschulze approved these changes Jan 4, 2017

View reviewed changes

Allow usage of message types with customtype override

4e62173

jvshahid force-pushed the customtype-with-message-type branch from 4bab499 to 4e62173 Compare January 4, 2017 20:04

awalterschulze merged commit f9114da into gogo:master Jan 4, 2017

awalterschulze mentioned this pull request Jan 4, 2017

customtype of type struct cannot jsonpb unmarshal #201

Closed

awalterschulze mentioned this pull request Sep 13, 2018

defining a customtype as a fake proto message causes problems with jsonpb, prototext and possibly some others #132

Open

Allow usage of message types with customtype override #239

Allow usage of message types with customtype override #239

Conversation

jvshahid commented Dec 9, 2016

jvshahid commented Dec 9, 2016

awalterschulze commented Dec 9, 2016

jvshahid commented Dec 9, 2016

jvshahid commented Dec 9, 2016

awalterschulze commented Dec 10, 2016

awalterschulze commented Dec 10, 2016

awalterschulze commented Dec 10, 2016

awalterschulze commented Dec 10, 2016

awalterschulze commented Dec 10, 2016

jvshahid commented Dec 10, 2016

jvshahid commented Dec 12, 2016

awalterschulze commented Dec 12, 2016

jvshahid commented Dec 14, 2016 • edited Loading

awalterschulze 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

awalterschulze commented Dec 14, 2016

jvshahid commented Dec 15, 2016

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

Choose a reason for hiding this comment

awalterschulze commented Dec 17, 2016

awalterschulze commented Dec 17, 2016

awalterschulze commented Dec 17, 2016

ericlagergren commented Dec 17, 2016

jvshahid commented Dec 20, 2016

jvshahid commented Jan 4, 2017

awalterschulze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awalterschulze commented Jan 4, 2017

awalterschulze commented Jan 4, 2017

jvshahid commented Jan 4, 2017 • edited Loading

awalterschulze commented Jan 4, 2017

jvshahid commented Dec 14, 2016 •

edited

Loading

jvshahid commented Jan 4, 2017 •

edited

Loading