gogo / protobuf

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

casttype fails if type has custom String() method #330

Closed rubenv closed 7 years ago

rubenv commented 7 years ago

Consider the following:

syntax = "proto3";

package main;

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

option (gogoproto.benchgen_all)    = true;
option (gogoproto.equal_all)       = true;
option (gogoproto.marshaler_all)   = true;
option (gogoproto.populate_all)    = true;
option (gogoproto.sizer_all)       = true;
option (gogoproto.testgen_all)     = true;
option (gogoproto.unmarshaler_all) = true;

message Object {
    uint32 type         = 1 [(gogoproto.casttype) = "TypeIdentifier"];
}

With TypeIdentifier defined like this:

package main

type TypeIdentifier uint32

const (
        UnknownType TypeIdentifier = 0
        UserType    TypeIdentifier = 20
)

func (t TypeIdentifier) String() string {
        switch t {
        case 20:
                return "User"
        default:
                return "Unknown"
        }
}

The generated tests for this fail:

=== RUN   TestObjectProto
--- PASS: TestObjectProto (0.00s)
=== RUN   TestObjectMarshalTo
--- PASS: TestObjectMarshalTo (0.00s)
=== RUN   TestObjectJSON
--- PASS: TestObjectJSON (0.00s)
=== RUN   TestObjectProtoText
--- FAIL: TestObjectProtoText (0.00s)
        typespb_test.go:152: seed = 1504170216371383417, err = line 1.6: invalid main.TypeIdentifier: Unknown
=== RUN   TestObjectProtoCompactText
--- FAIL: TestObjectProtoCompactText (0.00s)
        typespb_test.go:166: seed = 1504170216371512384, err = line 1.5: invalid main.TypeIdentifier: Unknown
=== RUN   TestObjectSize
--- PASS: TestObjectSize (0.00s)

Obviously this is related to the String() method on TypeIdentifier. Is this a bug or am I doing something wrong?

awalterschulze commented 7 years ago

Do you care about prototext support?

rubenv commented 7 years ago

@awalterschulze Not really to be honest. Any way to disable that?

awalterschulze commented 7 years ago

The only way is to disable all of testgen unfortunately.

This looks like our culprit https://github.com/gogo/protobuf/blob/master/proto/text_parser.go#L995

awalterschulze commented 7 years ago

But thats weird, since reflect.Uint32 is right there in the switch statement above it, hmmm

awalterschulze commented 7 years ago

I'll look at this later again

rubenv commented 7 years ago

Perhaps it sees TypeIdentifier rather than uint32?

awalterschulze commented 7 years ago

That I will check later, but that is my hope as well. Because that might be fixable.

awalterschulze commented 7 years ago

So the problem is in text.go

default:
        _, err := fmt.Fprint(w, v.Interface())
        return err
    }

This will cause the String method to be called.
To get around this will require a lot of special case handling.

awalterschulze commented 7 years ago

Fixed https://github.com/gogo/protobuf/commit/2adc21fd136931e0388e278825291678e1d98309

rubenv commented 7 years ago

To get around this will require a lot of special case handling.

Looking at 2adc21f you have a very special definition of "a lot" :-)

Thanks a lot for the fix and the awesome, awesome project that gogoprotobuf is.

awalterschulze commented 7 years ago

Yes after I made that statement I got a better plan 😀

On Thu, 31 Aug 2017, 21:01 Ruben Vermeersch, notifications@github.com wrote:

To get around this will require a lot of special case handling.

Looking at 2adc21f https://github.com/gogo/protobuf/commit/2adc21fd136931e0388e278825291678e1d98309 you have a very special definition of "a lot" :-)

Thanks a lot for the fix and the awesome, awesome project that gogoprotobuf is.

— You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub https://github.com/gogo/protobuf/issues/330#issuecomment-326390806, or mute the thread https://github.com/notifications/unsubscribe-auth/ABvsLfBE0V3cAJAKZjtd3GhMMWiw_bqYks5sdwLugaJpZM4PIh_b .

awalterschulze commented 7 years ago

My pleasure. I am glad you enjoy it.

On Thu, 31 Aug 2017, 22:13 Walter Schulze, awalterschulze@gmail.com wrote:

Yes after I made that statement I got a better plan 😀

On Thu, 31 Aug 2017, 21:01 Ruben Vermeersch, notifications@github.com wrote:

To get around this will require a lot of special case handling.

Looking at 2adc21f https://github.com/gogo/protobuf/commit/2adc21fd136931e0388e278825291678e1d98309 you have a very special definition of "a lot" :-)

Thanks a lot for the fix and the awesome, awesome project that gogoprotobuf is.

— You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub https://github.com/gogo/protobuf/issues/330#issuecomment-326390806, or mute the thread https://github.com/notifications/unsubscribe-auth/ABvsLfBE0V3cAJAKZjtd3GhMMWiw_bqYks5sdwLugaJpZM4PIh_b .