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

panic with proto.Equal and gogoproto.stdtime = true #685

Open
ben0x539 opened this issue Apr 24, 2020 · 0 comments
Open

panic with proto.Equal and gogoproto.stdtime = true #685

ben0x539 opened this issue Apr 24, 2020 · 0 comments

Comments

@ben0x539
Copy link

I think this should work, but it panics (go 1.14).

syntax = "proto3";

package foo;
option go_package = "main";

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

message Foo {
  google.protobuf.Timestamp some_time = 1 [(gogoproto.stdtime) = true];
}
package main

import (
    "time"

    "github.com/gogo/protobuf/proto"
)

func main() {
    now := time.Now()
    f1 := &Foo{SomeTime: &now}
    f2 := &Foo{SomeTime: &now}
    proto.Equal(f1, f2)
}
panic: reflect.Value.Interface: cannot return value obtained from unexported field or method

goroutine 1 [running]:
reflect.valueInterface(0x625420, 0x85bb60, 0x1b8, 0x6bbc01, 0x625420, 0x627a40)
	/usr/share/go/src/reflect/value.go:1004 +0x161
reflect.Value.Interface(...)
	/usr/share/go/src/reflect/value.go:993
[...]/vendor/github.com/gogo/protobuf/proto.equalAny(0x625420, 0x85bb60, 0x1b8, 0x625420, 0x85bb60, 0x1b8, 0xc0001c6400, 0x625420)
	[...]/vendor/github.com/gogo/protobuf/proto/equal.go:218 +0xe05
[...]/vendor/github.com/gogo/protobuf/proto.equalStruct(0x658480, 0x85bb60, 0x1b9, 0x658480, 0x85bb60, 0x1b9, 0xc000190078)
	[...]/vendor/github.com/gogo/protobuf/proto/equal.go:114 +0x3cc
[...]/vendor/github.com/gogo/protobuf/proto.equalAny(0x658480, 0x85bb60, 0x1b9, 0x658480, 0x85bb60, 0x1b9, 0xc0001c6300, 0x649c60)
	[...]/vendor/github.com/gogo/protobuf/proto/equal.go:220 +0xdc1
[...]/vendor/github.com/gogo/protobuf/proto.equalStruct(0x66a880, 0xc0001c4040, 0x199, 0x66a880, 0xc0001c4040, 0x199, 0xc000190038)
	[...]/vendor/github.com/gogo/protobuf/proto/equal.go:114 +0x3cc
[...]/vendor/github.com/gogo/protobuf/proto.equalAny(0x66a880, 0xc0001c4040, 0x199, 0x66a880, 0xc0001c4040, 0x199, 0xc0001c6000, 0x66b580)
	[...]/vendor/github.com/gogo/protobuf/proto/equal.go:220 +0xdc1
[...]/vendor/github.com/gogo/protobuf/proto.equalStruct(0x63db60, 0xc000182020, 0x199, 0x63db60, 0xc000182028, 0x199, 0xc000182028)
	/[...]vendor/github.com/gogo/protobuf/proto/equal.go:114 +0x3cc
[...]/vendor/github.com/gogo/protobuf/proto.Equal(0x6b9480, 0xc000182020, 0x6b9480, 0xc000182028, 0x0)
	[...]/vendor/github.com/gogo/protobuf/proto/equal.go:92 +0x331
main.main()
	[...]/main.go:13 +0xe0
exit status 2

The problem appears to be that when proto.Equal uses reflection to recursively extract all struct fields, it doesn't stop recursing when it reaches the time.Time field. That struct has unexported fields, in particular a string, which reflect.Value.Interface() refuses to give to us. I think this could be avoided by checking if a value we're looking at has StdTime and doing something smarter in that case.

#682 incidentally fixes this, because it avoids reflect.Value.Interface() in favor of using reflect.Value.String() to compare strings. That seems like a reasonable thing to do, and fixes my particular use case (using proto.Equal to compare messages in unit tests), but may not be the best way to compare timestamps in general.

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

1 participant