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

customtype: no encoder for custom.Uint128 #166

Closed
dghubble opened this issue Apr 5, 2016 · 23 comments
Closed

customtype: no encoder for custom.Uint128 #166

dghubble opened this issue Apr 5, 2016 · 23 comments

Comments

@dghubble
Copy link

dghubble commented Apr 5, 2016

I've been trying out customtype and can't get it to work with the custom.Uint128 or any of my own types either.

Take a working proto3 message type and add a customtype.

syntax = "proto3";
package storagepb;
import "github.com/gogo/protobuf/gogoproto/gogo.proto";
type MyMessage struct {
      ...
      bytes right = 6 [(gogoproto.customtype) = "github.com/gogo/protobuf/test/custom.Uint128"];
}

Generation with protoc --gogo_out=plugins=grpc:. -I=my/paths *.proto

import _ "github.com/gogo/protobuf/gogoproto"
import github_com_gogo_protobuf_test_custom "github.com/gogo/protobuf/test/custom"
type MyMessage struct {
     ...
     Right    *github_com_gogo_protobuf_test_custom.Uint128 `protobuf:"bytes,6,opt,name=right,proto3,customtype=github.com/gogo/protobuf/test/custom.Uint128" json:"right,omitempty"`
}

Now whenever this message is sent across the wire, I get log output

proto: no encoder function for *custom.Uint128 -> custom.Uint128
proto: no encoder for Right *custom.Uint128 [GetProperties]

Same issue with or without nullable.

I haven't used this library before so maybe I'm doing something basic wrong, but the cause is opaque to me. I'm testing the Uint128 here (instead of my own type) to ensure I haven't done the JSON Marshal and Unmarshal incorrectly. The generated .pb.go file contains no generated Marhsal or Unmarshal methods. How is this supposed to work? What's the support status with proto3 and customtype?

I see the tests use protoc-gen-combo (missing docs?) but haven't been able to get that to work either. Why is it invoked that way?

@awalterschulze
Copy link
Member

protoc-gen-combo is only really useful for tests. It generates a bunch of folders for each combination of marshalers and unmarshalers, true and false.

It sounds like its supposed to work.
I do have a test for exactly this situation in test/theproto3/theproto3.proto

message Uint128Pair {
  bytes left = 1 [(gogoproto.nullable) = false, (gogoproto.customtype) = "github.com/gogo/protobuf/test/custom.Uint128"];
  bytes right = 2 [(gogoproto.customtype) = "github.com/gogo/protobuf/test/custom.Uint128"];
}

So I think I must be missing something.

Are you using the github.com/gogo/protobuf/proto library to Marshal instead of the github.com/golang/protobuf/proto library?

Marshalers and Unmarshalers are only generated if they are specifically turned on.

import "github.com/gogo/protobuf/gogoproto/gogo.proto";
option (gogoproto.unmarshaler_all) = false;
option (gogoproto.marshaler_all) = false;
option (gogoproto.sizer_all) = true;

Or you could use the protco-gen-gogofast binary.

protoc --gogofast_out=plugins=grpc:. -I=my/paths *.proto

But again everything should work even though you haven't turned on marshalers. It should just be slower.

Are you maybe trying to use it on appengine?

@dghubble
Copy link
Author

dghubble commented Apr 5, 2016

I'm not on appengine, not using any options, and not importing anything to Marshal since the custom.Uint128 is imported from gogo protobuf and should handle marshaling I believe. According to one line in the website, just the Marshal and Unmarshal methods are needed and your Uint128 has those.

If I get some time, I'll put an example repo up to reproduce.

@awalterschulze
Copy link
Member

You should be importing gogo.proto

import "github.com/gogo/protobuf/gogoproto/gogo.proto";

But protoc should give a compile error if you don't.

This is very weird.

@dghubble
Copy link
Author

dghubble commented Jun 14, 2016

I ended up using the regular protobuf generator and converting between structs instead of this. I didn't ever put together a repro example so I'm closing.

@awalterschulze
Copy link
Member

That is not really helping me to solve the issue :( but fair enough.

@abursavich
Copy link

abursavich commented Feb 20, 2018

I came across this old issue when hitting something similar with a gogoproto.stdtime field and grpc-gateway. It was a frustrating problem to solve, so for the benefit of anyone else who may hit it in the future:

I ended up extracting the runtime.JSONPb code, changing the jsonpb and proto imports to the gogo versions, and then setting it as a custom Marshaler on the runtime.ServeMux.

mux := runtime.NewServeMux(runtime.WithMarshalerOption(runtime.MIMEWildcard, &gogomarshal.JSONPb{}))

Here's my gogomarshal package which is 99.9% lifted from the github.com/grpc-ecosystem/grpc-gateway/runtime package: https://github.com/abursavich/gogomarshal

@johanbrandhorst
Copy link
Member

@abursavich note that this kind of solution has been discussed before, going back to #178. CockroachDB provided a similar solution to yours. I've written more about this an other gogo compatibility issues with the gRPC-Gateway in a post on my blog: https://jbrandhorst.com/post/gogoproto/.

@awalterschulze
Copy link
Member

Thank you very much both of you :)

@abursavich How do you use the Proto type that you have also included in https://github.com/abursavich/gogomarshal ?

@awalterschulze
Copy link
Member

@tamird What do you think about https://github.com/abursavich/gogomarshal
when compared to the cockroachdb solution?

@tamird
Copy link
Contributor

tamird commented Feb 20, 2018

@awalterschulze looks the same with a few minor differences:

Other than that, the two are identical AFAICT.

@awalterschulze
Copy link
Member

awalterschulze commented Feb 20, 2018

Maybe we can standardize this marshaler and unmarshaler into a github.com/gogo repo, because this something many people use these days.

  1. What do you think?
  2. What would be a good name for this repo?
  3. Who wants to volunteer as a maintainer, since I don't have enough expertise in this area?

@tamird
Copy link
Contributor

tamird commented Feb 20, 2018

@awalterschulze seems fine to me. I don't think there's any reason to include a proto marshaler (i.e. https://github.com/abursavich/gogomarshal/blob/master/protopb.go) since upstream's is just fine (and was added in (grpc-ecosystem/grpc-gateway#459). We would switch to that as well, but it has the wrong content type for us. The important part is to use gogo/protobuf.jsonpb.

@abursavich
Copy link

I'm new to both gogoproto and grpc-gateway. I'm happy to contribute code and it would be nice to have it in github.com/gogo, but I'm not certain that I should be the owner as I'm still ramping up at the moment. If you want to adopt my package, I agree that the naming should be changed but I don't have any good suggestions yet.

I would caution that grpc-gateway's ProtoMarshaler uses github.com/golang/protobuf/proto, while cockroachdb's and mine use github.com/gogo/protobuf/proto. I went ahead and slapped a CustomContentType field on mine to allow application/x-protobuf.

@awalterschulze
Copy link
Member

How about the name github.com/gogo/gateway?

Then we can put both marshalers in there.
Sounds like we need the ProtoMarshaler as well.

And if we need something else to help with grpc-gateway compatibility we can put in there are well?

Regarding maintenance. Well I will also be there to help, but I would prefer if a user, even a new user :) is a maintainer, over me, an unfortunate non user :(

@awalterschulze
Copy link
Member

@johanbrandhorst
Copy link
Member

Sounds good to me :)

@awalterschulze
Copy link
Member

Are you also volunteering? :)

@abursavich
Copy link

I'll volunteer.

@awalterschulze
Copy link
Member

Cool I made you admin of github.com/gogo/gateway
Please let me know if you haven't received an invite

@awalterschulze
Copy link
Member

@abursavich No rush. I just want to make sure. Did you receive the invite to the repo?

@abursavich
Copy link

Yes, I did. I have a new version of the code ready, but I'm waiting to find out about a licensing issue before pushing it.

@awalterschulze
Copy link
Member

awalterschulze commented Feb 28, 2018 via email

@awalterschulze
Copy link
Member

@abursavich no rush, just checking in. Have you had any luck yet?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants