gogo / protobuf

[Deprecated] Protocol Buffers for Go with Gadgets
Other
5.67k stars 807 forks source link

Undefined ProtoSize() method when using google.protobuf.Any #438

Closed jwilander closed 6 years ago

jwilander commented 6 years ago

As part of a larger project, I have some messages defined like so:

message API_LoadPluginConfigurationRequest {
    google.protobuf.Any arg1 = 1;
}

The MarshalTo method that gets generated is:

func (m *API_LoadPluginConfigurationRequest) MarshalTo(dAtA []byte) (int, error) {
    var i int
    _ = i
    var l int
    _ = l
    if m.Arg1 != nil {
        dAtA[i] = 0xa
        i++
        i = encodeVarintPlugin(dAtA, i, uint64(m.Arg1.ProtoSize()))
        n45, err := m.Arg1.MarshalTo(dAtA[i:])
        if err != nil {
            return 0, err
        }
        i += n45
    }
    if m.XXX_unrecognized != nil {
        i += copy(dAtA[i:], m.XXX_unrecognized)
    }
    return i, nil
}

Giving me the error:

m.Arg1.ProtoSize undefined (type *types.Any has no field or method ProtoSize)

I'm including the following options when running protoc:

--gofast_out=plugins=grpc,Mgoogle/protobuf/any.proto=github.com/gogo/protobuf/types,Mgoogle/protobuf/duration.proto=github.com/gogo/protobuf/types,Mgoogle/protobuf/timestamp.proto=github.com/gogo/protobuf/types:$(GOPATH)/src

This happens for other messages as well. Some with repeated Any types or Any types used as values in maps.

I'm not sure if this is a bug or something I'm doing wrong. If it is a bug I'd be glad to help to track down and fix it, though I'm still new to protocol buffers. Any ideas?

awalterschulze commented 6 years ago

Could you also include a small example of a proto file that uses any, that reproduces this problem?

Sounds like this might be a bug on our side.

On Wed, 25 Jul 2018, 23:42 Joram Wilander, notifications@github.com wrote:

As part of a larger project, I have some messages defined like so:

message API_LoadPluginConfigurationRequest { google.protobuf.Any arg1 = 1; }

The MarshalTo method that gets generated is:

func (m *APILoadPluginConfigurationRequest) MarshalTo(dAtA []byte) (int, error) { var i int = i var l int _ = l if m.Arg1 != nil { dAtA[i] = 0xa i++ i = encodeVarintPlugin(dAtA, i, uint64(m.Arg1.ProtoSize())) n45, err := m.Arg1.MarshalTo(dAtA[i:]) if err != nil { return 0, err } i += n45 } if m.XXX_unrecognized != nil { i += copy(dAtA[i:], m.XXX_unrecognized) } return i, nil }

Giving me the error:

m.Arg1.ProtoSize undefined (type *types.Any has no field or method ProtoSize)

I'm including the following options when running protoc:

--gofast_out=plugins=grpc,Mgoogle/protobuf/any.proto=github.com/gogo/protobuf/types,Mgoogle/protobuf/duration.proto=github.com/gogo/protobuf/types,Mgoogle/protobuf/timestamp.proto=github.com/gogo/protobuf/types:$(GOPATH)/src

This happens for other messages as well. Some with repeated Any types or Any types used as values in maps.

I'm not sure if this is a bug or something I'm doing wrong. If it is a bug I'd be glad to help to track down and fix it, though I'm still new to protocol buffers. Any ideas?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/gogo/protobuf/issues/438, or mute the thread https://github.com/notifications/unsubscribe-auth/ABvsLc-hK5osHCRN1t7ZkXlINjTEfFSjks5uKPQ8gaJpZM4Vg6P1 .

jwilander commented 6 years ago

Sure, this subset of my .proto file will reproduce it. I imagine it has something to do with the options being specified, though I (possibly incorrectly) assume it should work regardless of how those are set

syntax = "proto3";
package mmproto;

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

option (gogoproto.protosizer_all) = true;
option (gogoproto.sizer_all) = false;
option go_package = "mmproto";

message API_LoadPluginConfigurationRequest {
    google.protobuf.Any arg1 = 1;
}

message API_LoadPluginConfigurationResponse {
}

service APIService {
    rpc API_LoadPluginConfiguration (mmproto.API_LoadPluginConfigurationRequest) returns (mmproto.API_LoadPluginConfigurationResponse);
}
awalterschulze commented 6 years ago

@jmarais would mind taking a look at this?

jmarais commented 6 years ago

@awalterschulze Yes. I will take a look.

jmarais commented 6 years ago

@jwilander Thanks for the example. I could reduce it to:

syntax = "proto3";

package issue427;

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

option (gogoproto.protosizer_all) = true;
option (gogoproto.sizer_all) = false;

message Foo {
    google.protobuf.Any bar = 1;
}

And generated with:

protoc --gogo_out=Mgoogle/protobuf/duration.proto=github.com/gogo/protobuf/types:. --proto_path=../../../../../:../../protobuf/:. issue438.proto

It seems to break when you use any of the types in 'google/protobuf.proto'.

@awalterschulze it seems like our /types/<>.pb.go is not generate with the: option (gogoproto.protosizer_all) = true;

Option. Thus, currently you cannot use any of those types with the protosizer option. Is that option supported on the /types/ types?

awalterschulze commented 6 years ago

I think we should make the well known types like any support both Size and ProtoSize. Maybe we could enable this option, but maybe this causes a conflict in the code generator with sizer_all=true If so, maybe we could write these ProtoSize methods manually, or rather copy them after they have been generated once. What do you think? @jmarais

jmarais commented 6 years ago

but maybe this causes a conflict in the code generator with sizer_all=true Yes, unfortunately one cannot have both options true

If so, maybe we could write these ProtoSize methods manually, or rather copy them after they have been generated once.

If we say we support ProtoSize on well known types then we have to make work.

Edit: @awalterschulze I tried to look at the available documentation, but what is the difference between the Size and the ProtoSize methods?

awalterschulze commented 6 years ago

The methods are exactly the same, except for the name :) Size was my first attempt, but this conflicted with a lot of users' actual fieldnames. So after much discussion, I finally gave in, for this very reasonable use case. But I couldn't just remove the Size method, since this was already used. So that is why both exist.

jmarais commented 6 years ago

Ah ok. That confirms what I read in the sizer plugin. So do we want to proceed with this? We can either use a wrapper function or copy + rename the current Size methods?

awalterschulze commented 6 years ago

Oh yes a manually written wrapper function would be much better, great idea :)

awalterschulze commented 6 years ago

I think we can close this, given the commit https://github.com/gogo/protobuf/commit/baf2ea5cdb7ce2d399176f925052690c3dad1f56 by @jmarais

What do you think @jwilander ?

jwilander commented 6 years ago

Sounds good to me.

On Wed, Aug 1, 2018, 16:44 Walter Schulze, notifications@github.com wrote:

I think we can close this, given the commit baf2ea5 https://github.com/gogo/protobuf/commit/baf2ea5cdb7ce2d399176f925052690c3dad1f56 by @jmarais https://github.com/jmarais

What do you think @jwilander https://github.com/jwilander ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/gogo/protobuf/issues/438#issuecomment-409579543, or mute the thread https://github.com/notifications/unsubscribe-auth/ACjF4pz9reZrUXKnPEAHd-PFM2eUfgbmks5uMbDJgaJpZM4Vg6P1 .

awalterschulze commented 6 years ago

Thank you :)