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

[dennwc]

Can be reproduced with custom type:

package pt

//go:generate protoc --proto_path=$GOPATH/src:. --gogo_out=. types.proto

type Native struct{
    data string
}

// ... omiting methods to satisfy protobuf interfaces.

// types.proto
syntax = "proto3";

package pt;

// Contains custom type definition to bypass generation errors:
// message Native{}
import ".../native.proto";

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

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

message Item{
    Native sub = 1;
}

Auto-generated tests for serialization will fail with panic:

panic: reflect.Value.Interface: cannot return value obtained from unexported field or method

reflect.valueInterface(...)
    /usr/local/go/src/reflect/value.go:912 +0xe7
reflect.Value.Interface(...)
    /usr/local/go/src/reflect/value.go:901
github.com/gogo/protobuf/jsonpb.(*Marshaler).marshalValue(...)
    src/github.com/gogo/protobuf/jsonpb/jsonpb.go:371
github.com/gogo/protobuf/jsonpb.(*Marshaler).marshalField(...)
    src/github.com/gogo/protobuf/jsonpb/jsonpb.go:216
github.com/gogo/protobuf/jsonpb.(*Marshaler).marshalObject(...)
    src/github.com/gogo/protobuf/jsonpb/jsonpb.go:147
github.com/gogo/protobuf/jsonpb.(*Marshaler).marshalValue(...)
    src/github.com/gogo/protobuf/jsonpb/jsonpb.go:316
github.com/gogo/protobuf/jsonpb.(*Marshaler).marshalField(...)
    src/github.com/gogo/protobuf/jsonpb/jsonpb.go:216
github.com/gogo/protobuf/jsonpb.(*Marshaler).marshalObject(...)
    src/github.com/gogo/protobuf/jsonpb/jsonpb.go:147
github.com/gogo/protobuf/jsonpb.(*Marshaler).Marshal(...)
    src/github.com/gogo/protobuf/jsonpb/jsonpb.go:75
github.com/gogo/protobuf/jsonpb.(*Marshaler).MarshalToString(...)
    src/github.com/gogo/protobuf/jsonpb/jsonpb.go:81

Jsonpb definitely has no code to test for unexported fields.

But this can be fixed easier: jsonpb code should check if object implements json.Marshaler.

[dennwc]

This issue is even harder: nor protobuf-text, nor jsonpb checks for implementation of TextMarshaler/json.Marshaler for fields of a message. So tests can pass for custom type, but not for messages with fields with that type.

[awalterschulze]

I don't see you actually using the customtype extension in types.proto
Shouldn't it be something like this?

message Item {
    bytes sub = 1 [(gogoproto.customtype)="native.Native"];
}

Or am I missing something?

[dennwc]

No, it should not. Generated Go code will actually contain Native type references, and the type itself is already implemented in a separate file (code block at the beginning). This works because code generation bypasses all checks for Native message type because it is present in the import (you suggested this technique, by the way :) ).

[awalterschulze]

I don't think so. Please link my previous suggestion.

[dennwc]

#126
"Couldn't you achieve the same thing by simply putting MyType in a separate proto file which you can import, but not generate code for?"

[awalterschulze]

Ah yes context is very helpful thank you :)

[awalterschulze]

So why can't you export the fields?

[dennwc]

Because the type of the field is unknown to protobuf (interface{}). Custom marshaling/unmarshaling procedures allows us to preserve comparability guaranties for that field. This can't be achieved in any other way that is known to me.

[awalterschulze]

Yeah and if you want to create any custom type you don't intend to always have struct types.
Hmmm.
So basically jsonpb and prototext would need to be modified.
But is that all?

[dennwc]

Yes. Basically, we're actually not using jsonpb right now, but I thought that you should be aware of the problem.
And this is kind of annoying to comment out those JSON tests all the time :)

[awalterschulze]

So basically someone would need to fully explore this and make this a proper feature and not just a hack.
Almost like you suggested in #126, but it would be nice if it was possible to do without adding another extension.

[ericlagergren]

This happens with any unexported field. E.g., if I use this

message Foo {
    string bar = 1 [(gogoproto.customname) = "bar"];
}

The test "TestFooProtoText" panics with

--- FAIL: TestFooProtoText (0.00s)
panic: reflect.Value.Interface: cannot return value obtained from unexported field or method [recovered]
    panic: reflect.Value.Interface: cannot return value obtained from unexported field or method

[awalterschulze]

No you can't declare private customnames sorry.
This should probably be added to the doc.

[ericlagergren]

All good, thank you 🙂
On Fri, Aug 26, 2016 at 12:03 AM Walter Schulze notifications@github.com
wrote:

No you can't declare private customnames sorry.
This should probably be added to the doc.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#132 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AFnwZ4EBZOxgFqxCwxW-aISVMFGI8MWiks5qjo-9gaJpZM4G-5Mu
.

[awalterschulze]

Is this fixed by #239 ?