diff --git a/README.md b/README.md index a446e18..c776ec1 100644 --- a/README.md +++ b/README.md @@ -175,23 +175,9 @@ Our existing public APIs are expected to be stable even before the v1.0 release. ## Known issues -### TypeError with the same type names +### gogo/protobuf support -E.g.: -``` -panic: TypeError: value (type `google.protobuf.UInt32Value') can't be assigned to type `google.protobuf.UInt32Value'. -``` - -On the face of it this looks confusing, and it is. It's an effect of a -project that use `gogo_protobuf`, where some fundamental types are -wrapped in messages that shadow the same messages defined by -`google.protobuf`. Even though the messages seem to say that they are -of the same type, they are actually two different types with the same -name. - -For projects that use `gogo_protobuf`, e.g. envoy, this can be very confusing. - -To resolve this, use the package `github.com/stripe/skycfg/gogocompat` -in your go code. This will allow skycfg code to load -`proto.package("gogo:google.protobuf") and the internal registry it -creates will handle the conversion of these types. +Skycfg has dropped support for +[gogo/protobuf](https://github.com/gogo/protobuf), an alternative protobuf go +code generator, when upgrading to using the go-protobuf v2 api. For using gogo +support, see `Skycfg@v0.1.0` (Skycfg prior to switching to v2). \ No newline at end of file diff --git a/build/go_dependencies.bzl b/build/go_dependencies.bzl index 7af0ff1..666ffa3 100644 --- a/build/go_dependencies.bzl +++ b/build/go_dependencies.bzl @@ -1,12 +1,6 @@ load("@bazel_gazelle//:deps.bzl", "go_repository") def go_dependencies(): - go_repository( - name = "com_github_gogo_protobuf", - importpath = "github.com/gogo/protobuf", - sum = "h1:DqDEcV5aeaTmdFBePNpYsp3FlcVH/2ISVVM9Qf8PSls=", - version = "v1.3.1", - ) go_repository( name = "com_github_golang_protobuf", importpath = "github.com/golang/protobuf", diff --git a/go.mod b/go.mod index 54d34df..27e4179 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,6 @@ module github.com/stripe/skycfg go 1.16 require ( - github.com/gogo/protobuf v1.3.1 github.com/golang/protobuf v1.4.1 github.com/kylelemons/godebug v0.0.0-20170820004349-d65d576e9348 go.starlark.net v0.0.0-20201204201740-42d4f566359b diff --git a/go.sum b/go.sum index 927d325..d655108 100644 --- a/go.sum +++ b/go.sum @@ -1,8 +1,6 @@ github.com/chzyer/logex v1.1.10/go.mod h1:+Ywpsq7O8HXn0nuIou7OrIPyXbp3wmkHB+jjWRnGsAI= github.com/chzyer/readline v0.0.0-20180603132655-2972be24d48e/go.mod h1:nSuG5e5PlCu98SY8svDHJxuZscDgtXS6KTTbou5AhLI= github.com/chzyer/test v0.0.0-20180213035817-a1ea475d72b1/go.mod h1:Q3SI9o4m/ZMnBNeIyt5eFwwo7qiLfzFZmjNmxjkiQlU= -github.com/gogo/protobuf v1.3.1 h1:DqDEcV5aeaTmdFBePNpYsp3FlcVH/2ISVVM9Qf8PSls= -github.com/gogo/protobuf v1.3.1/go.mod h1:SlYgWuQ5SjCEi6WLHjHCa1yvBfUnHcTbrrZtXPKa29o= github.com/golang/protobuf v1.4.0-rc.1/go.mod h1:ceaxUfeHdC40wWswd/P6IGgMaK3YpKi5j83Wpe3EHw8= github.com/golang/protobuf v1.4.0-rc.1.0.20200221234624-67d41d38c208/go.mod h1:xKAWHe0F5eneWXFV3EuXVDTCmh+JuBKY0li0aMyXATA= github.com/golang/protobuf v1.4.0-rc.2/go.mod h1:LlEzMj4AhA7rCAGe4KMBDvJI+AwstrUpVNzEA03Pprs= @@ -17,13 +15,10 @@ github.com/google/go-cmp v0.5.0 h1:/QaMHBdZ26BB3SSst0Iwl10Epc+xhTquomWX0oZEB6w= github.com/google/go-cmp v0.5.0/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/jmillikin-stripe/godebug v0.0.0-20180620173319-8279e1966bc1 h1:JgsVrDAUy59N248f3l4RGZ0hij5u1HTit8iJr1mFSBY= github.com/jmillikin-stripe/godebug v0.0.0-20180620173319-8279e1966bc1/go.mod h1:gFqr/IKD8P+Hluq9gThCR944BAu6jUqd5H/R3PrPfuM= -github.com/kisielk/errcheck v1.2.0/go.mod h1:/BMXB+zMLi60iA8Vv6Ksmxu/1UDYcXs4uQLJ+jE2L00= -github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck= go.starlark.net v0.0.0-20201204201740-42d4f566359b h1:yHUzJ1WfcdR1oOafytJ6K1/ntYwnEIXICNVzHb+FzbA= go.starlark.net v0.0.0-20201204201740-42d4f566359b/go.mod h1:5YFcFnRptTN+41758c2bMPiqpGg4zBfYji1IQz8wNFk= golang.org/x/sys v0.0.0-20200803210538-64077c9b5642 h1:B6caxRw+hozq68X2MY7jEpZh/cr4/aHLv9xU8Kkadrw= golang.org/x/sys v0.0.0-20200803210538-64077c9b5642/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= -golang.org/x/tools v0.0.0-20181030221726-6c7e314b6563/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= google.golang.org/protobuf v0.0.0-20200109180630-ec00e32a8dfd/go.mod h1:DFci5gLYBciE7Vtevhsrf46CRTquxDuWsQurQQe4oz8= diff --git a/gogocompat/BUILD b/gogocompat/BUILD deleted file mode 100644 index 2a1b77b..0000000 --- a/gogocompat/BUILD +++ /dev/null @@ -1,26 +0,0 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") - -go_library( - name = "gogocompat", - srcs = ["gogocompat.go"], - importpath = "github.com/stripe/skycfg/gogocompat", - visibility = ["//visibility:public"], - deps = [ - "//internal/go/skycfg", - "@com_github_gogo_protobuf//proto", - "@com_github_golang_protobuf//proto:go_default_library", - ], -) - -go_test( - name = "gogocompat_test", - size = "small", - srcs = ["gogocompat_test.go"], - embed = [":gogocompat"], - deps = [ - "//internal/go/skycfg", - "@com_github_gogo_protobuf//types", - "@io_bazel_rules_go//proto/wkt:wrappers_go_proto", - "@net_starlark_go//starlark", - ], -) diff --git a/gogocompat/gogocompat.go b/gogocompat/gogocompat.go deleted file mode 100644 index 1d68eab..0000000 --- a/gogocompat/gogocompat.go +++ /dev/null @@ -1,71 +0,0 @@ -// Copyright 2019 The Skycfg Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// -// SPDX-License-Identifier: Apache-2.0 - -// Package gogocompat is a compatibility shim for GoGo. -package gogocompat - -import ( - "reflect" - "strings" - - gogo_proto "github.com/gogo/protobuf/proto" - "github.com/golang/protobuf/proto" - - impl "github.com/stripe/skycfg/internal/go/skycfg" -) - -type unstableProtoRegistry interface { - impl.ProtoRegistry -} - -type protoRegistry struct{} - -func (*protoRegistry) UnstableProtoMessageType(name string) (reflect.Type, error) { - if t := proto.MessageType(name); t != nil { - return t, nil - } - name = strings.TrimPrefix(name, "gogo:") - if t := gogo_proto.MessageType(name); t != nil { - return t, nil - } - return nil, nil -} - -func (*protoRegistry) UnstableEnumValueMap(name string) map[string]int32 { - if ev := proto.EnumValueMap(name); ev != nil { - return ev - } - if ev := gogo_proto.EnumValueMap(name); ev != nil { - return ev - } - return nil -} - -// ProtoRegistry returns a Protobuf message registry that falls back to GoGo. -// -// To support types that might differ between Protobuf and GoGo registrations, -// the special prefix "gogo:" can be used to skip looking up messages in the -// standard Protobuf registry. -// -// pb = proto.package("google.protobuf") -// gogo_pb = proto.package("gogo:google.protobuf") -// # pb.Timestamp and gogo_pb.Timestamp are distinct types. -// -// The exact type of the return value is not yet stabilized, but the result -// is guaranteed to be accepted by the skycfg.WithProtoRegistry() load option. -func ProtoRegistry() unstableProtoRegistry { - return &protoRegistry{} -} diff --git a/gogocompat/gogocompat_test.go b/gogocompat/gogocompat_test.go deleted file mode 100644 index d47c4ec..0000000 --- a/gogocompat/gogocompat_test.go +++ /dev/null @@ -1,51 +0,0 @@ -// Copyright 2019 The Skycfg Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// -// SPDX-License-Identifier: Apache-2.0 - -// Package gogocompat is a compatibility shim for GoGo. -package gogocompat - -import ( - "testing" - - _ "github.com/gogo/protobuf/types" - _ "github.com/golang/protobuf/ptypes/wrappers" - "go.starlark.net/starlark" - - impl "github.com/stripe/skycfg/internal/go/skycfg" -) - -func skyEval(t *testing.T, src string) starlark.Value { - t.Helper() - globals := starlark.StringDict{ - "proto": impl.NewProtoModule(ProtoRegistry()), - } - val, err := starlark.Eval(&starlark.Thread{}, "", src, globals) - if err != nil { - t.Fatalf("eval(%q): %v", src, err) - } - return val -} - -func TestGogoRegistry(t *testing.T) { - protoTimestamp := skyEval(t, `proto.package("google.protobuf").Timestamp()`) - gogoTimestamp := skyEval(t, `proto.package("gogo:google.protobuf").Timestamp()`) - - protoType := protoTimestamp.Type() - gogoType := gogoTimestamp.Type() - if protoType != gogoType { - t.Fatalf("Expected same type name for Protobuf and GoGo, got proto=%q, gogo=%q)", protoType, gogoType) - } -} diff --git a/internal/go/skycfg/BUILD b/internal/go/skycfg/BUILD index 84c8d52..196080a 100644 --- a/internal/go/skycfg/BUILD +++ b/internal/go/skycfg/BUILD @@ -39,9 +39,6 @@ go_test( deps = [ "//:skycfg", "//internal/testdata/test_proto:test_proto_go_proto", - "//internal/testdata/test_proto_gogo:test_proto_gogo_go_proto", - "@com_github_gogo_protobuf//proto", - "@com_github_gogo_protobuf//types", "@com_github_golang_protobuf//proto:go_default_library", "@com_github_kylelemons_godebug//pretty", "@net_starlark_go//resolve", diff --git a/internal/go/skycfg/proto_test.go b/internal/go/skycfg/proto_test.go index 704c1f7..3bdca64 100644 --- a/internal/go/skycfg/proto_test.go +++ b/internal/go/skycfg/proto_test.go @@ -26,9 +26,7 @@ import ( "sort" "strings" "testing" - "time" - gogo_proto "github.com/gogo/protobuf/proto" "github.com/golang/protobuf/proto" "github.com/kylelemons/godebug/pretty" "go.starlark.net/resolve" @@ -37,31 +35,15 @@ import ( "google.golang.org/protobuf/types/descriptorpb" any "google.golang.org/protobuf/types/known/anypb" - _ "github.com/gogo/protobuf/types" - "github.com/stripe/skycfg" impl "github.com/stripe/skycfg/internal/go/skycfg" pb "github.com/stripe/skycfg/internal/testdata/test_proto" - pb_gogo "github.com/stripe/skycfg/internal/testdata/test_proto_gogo" ) func init() { resolve.AllowFloat = true } -type gogoRegistry struct{} - -func (*gogoRegistry) UnstableProtoMessageType(name string) (reflect.Type, error) { - if t := gogo_proto.MessageType(name); t != nil { - return t, nil - } - return nil, nil -} - -func (*gogoRegistry) UnstableEnumValueMap(name string) map[string]int32 { - return gogo_proto.EnumValueMap(name) -} - func mustProtoMessage(v starlark.Value) proto.Message { if msg, ok := impl.ToProtoMessage(v); ok { return msg @@ -72,8 +54,7 @@ func mustProtoMessage(v starlark.Value) proto.Message { func skyEval(t *testing.T, src string) starlark.Value { t.Helper() globals := starlark.StringDict{ - "proto": skycfg.UnstableProtoModule(nil), - "gogo_proto": skycfg.UnstableProtoModule(&gogoRegistry{}), + "proto": skycfg.UnstableProtoModule(nil), } val, err := starlark.Eval(&starlark.Thread{}, "", src, globals) if err != nil { @@ -85,8 +66,7 @@ func skyEval(t *testing.T, src string) starlark.Value { func skyExecFun(t *testing.T, src string) starlark.Value { t.Helper() globals := starlark.StringDict{ - "proto": skycfg.UnstableProtoModule(nil), - "gogo_proto": skycfg.UnstableProtoModule(&gogoRegistry{}), + "proto": skycfg.UnstableProtoModule(nil), } globals, err := starlark.ExecFile(&starlark.Thread{}, "", src, globals) if err != nil { @@ -140,7 +120,6 @@ func TestProtoMessageString(t *testing.T) { func TestNestedMessages(t *testing.T) { testPb := `proto.package("skycfg.test_proto").` - gogoPb := `gogo_proto.package("skycfg.test_proto").` tests := []struct { src string @@ -163,15 +142,6 @@ func TestNestedMessages(t *testing.T) { src: testPb + `MessageV3.NestedMessage.DoubleNestedMessage()`, wantVal: ``, }, - - { - src: gogoPb + `MessageGogo.NestedMessage()`, - wantVal: ``, - }, - { - src: gogoPb + `MessageGogo.NestedMessage.DoubleNestedMessage()`, - wantVal: ``, - }, } for _, test := range tests { gotVal := skyEval(t, test.src).String() @@ -681,115 +651,6 @@ func TestMessageV3(t *testing.T) { } } -func TestMessageGogo(t *testing.T) { - val := skyEval(t, `gogo_proto.package("skycfg.test_proto").MessageGogo( - f_int32 = 1010, - f_int64 = 1020, - f_uint32 = 1030, - f_uint64 = 1040, - f_float32 = 10.50, - f_float64 = 10.60, - f_string = "some string", - f_bool = True, - f_submsg = proto.package("skycfg.test_proto").MessageV2( - f_string = "string in submsg", - ), - r_string = ["r_string1", "r_string2"], - r_submsg = [ - proto.package("skycfg.test_proto").MessageV2( - f_string = "string in r_submsg", - ), - ], - map_string = { - "map_string key": "map_string val", - }, - map_submsg = { - "map_submsg key": proto.package("skycfg.test_proto").MessageV2( - f_string = "map_submsg val", - ), - }, - f_nested_submsg = gogo_proto.package("skycfg.test_proto").MessageGogo.NestedMessage( - f_string = "nested_submsg val", - ), - f_toplevel_enum = proto.package("skycfg.test_proto").ToplevelEnumV2.TOPLEVEL_ENUM_V2_B, - f_nested_enum = gogo_proto.package("skycfg.test_proto").MessageGogo.NestedEnum.NESTED_ENUM_B, - f_oneof_a = "string in oneof", - f_bytes = "also some string", - f_duration = proto.package("google.protobuf").Duration(seconds = 1), - )`) - gotMsg := mustProtoMessage(val) - wantMsg := &pb_gogo.MessageGogo{ - FInt32: proto.Int32(1010), - FInt64: proto.Int64(1020), - FUint32: proto.Uint32(1030), - FUint64: proto.Uint64(1040), - FFloat32: proto.Float32(10.50), - FFloat64: proto.Float64(10.60), - FString: proto.String("some string"), - FBool: proto.Bool(true), - FSubmsg: pb.MessageV2{ - FString: proto.String("string in submsg"), - }, - RString: []string{"r_string1", "r_string2"}, - RSubmsg: []pb.MessageV2{{ - FString: proto.String("string in r_submsg"), - }}, - MapString: map[string]string{ - "map_string key": "map_string val", - }, - MapSubmsg: map[string]*pb.MessageV2{ - "map_submsg key": &pb.MessageV2{ - FString: proto.String("map_submsg val"), - }, - }, - FNestedSubmsg: &pb_gogo.MessageGogo_NestedMessage{ - FString: proto.String("nested_submsg val"), - }, - FToplevelEnum: pb.ToplevelEnumV2_TOPLEVEL_ENUM_V2_B.Enum(), - FNestedEnum: pb_gogo.MessageGogo_NESTED_ENUM_B.Enum(), - FOneof: &pb_gogo.MessageGogo_FOneofA{"string in oneof"}, - FBytes: []byte("also some string"), - FDuration: time.Second, - } - if diff := ProtoDiff(wantMsg, gotMsg); diff != "" { - t.Fatalf("diff from expected message:\n%s", diff) - } - - wantAttrs := map[string]string{ - "f_int32": "1010", - "f_int64": "1020", - "f_uint32": "1030", - "f_uint64": "1040", - "f_float32": "10.5", - "f_float64": "10.6", - "f_string": `"some string"`, - "f_bool": "True", - "f_submsg": ``, - "r_string": `["r_string1", "r_string2"]`, - "r_submsg": `[]`, - "map_string": `{"map_string key": "map_string val"}`, - "map_submsg": `{"map_submsg key": }`, - "f_nested_submsg": ``, - "f_toplevel_enum": ``, - "f_nested_enum": ``, - "f_oneof_a": `"string in oneof"`, - "f_oneof_b": `None`, - "f_bytes": `"also some string"`, - "f_duration": ``, - } - attrs := val.(starlark.HasAttrs) - for attrName, wantAttr := range wantAttrs { - attr, err := attrs.Attr(attrName) - if err != nil { - t.Fatalf("val.Attr(%q): %v", attrName, err) - } - gotAttr := attr.String() - if wantAttr != gotAttr { - t.Errorf("val.Attr(%q): wanted %q, got %q", attrName, wantAttr, gotAttr) - } - } -} - func TestAttrValidation(t *testing.T) { globals := starlark.StringDict{ "proto": skycfg.UnstableProtoModule(nil), diff --git a/internal/testdata/test_proto_gogo/BUILD b/internal/testdata/test_proto_gogo/BUILD deleted file mode 100644 index da64f43..0000000 --- a/internal/testdata/test_proto_gogo/BUILD +++ /dev/null @@ -1,30 +0,0 @@ -load("@rules_proto//proto:defs.bzl", "proto_library") -load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library") - -# gazelle:exclude package.go -# gazelle:go_proto_compilers @io_bazel_rules_go//proto:gogofast_proto -# gazelle:resolve proto github.com/stripe/skycfg/internal/testdata/test_proto/test_proto_v2.proto //internal/testdata/test_proto -# gazelle:resolve proto github.com/gogo/protobuf/gogoproto/gogo.proto @gogo_special_proto//github.com/gogo/protobuf/gogoproto - -proto_library( - name = "test_proto_gogo", - srcs = ["test_proto_gogo.proto"], - visibility = ["//:__subpackages__"], - deps = [ - "//internal/testdata/test_proto", - "@com_google_protobuf//:duration_proto", - "@gogo_special_proto//github.com/gogo/protobuf/gogoproto", - ], -) - -go_proto_library( - name = "test_proto_gogo_go_proto", - compilers = ["@io_bazel_rules_go//proto:gogofast_proto"], - importpath = "github.com/stripe/skycfg/internal/testdata/test_proto_gogo", - proto = ":test_proto_gogo", - visibility = ["//:__subpackages__"], - # keep - deps = [ - "//internal/testdata/test_proto:test_proto_go_proto", - ], -) diff --git a/internal/testdata/test_proto_gogo/package.go b/internal/testdata/test_proto_gogo/package.go deleted file mode 100644 index b424809..0000000 --- a/internal/testdata/test_proto_gogo/package.go +++ /dev/null @@ -1 +0,0 @@ -package test_proto_gogo diff --git a/internal/testdata/test_proto_gogo/test_proto_gogo.proto b/internal/testdata/test_proto_gogo/test_proto_gogo.proto deleted file mode 100644 index aac5a98..0000000 --- a/internal/testdata/test_proto_gogo/test_proto_gogo.proto +++ /dev/null @@ -1,82 +0,0 @@ -// Copyright 2018 The Skycfg Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// -// SPDX-License-Identifier: Apache-2.0 - -syntax = "proto2"; - -package skycfg.test_proto; - -import "github.com/gogo/protobuf/gogoproto/gogo.proto"; -import "github.com/stripe/skycfg/internal/testdata/test_proto/test_proto_v2.proto"; -import "google/protobuf/duration.proto"; - -option go_package = "github.com/stripe/skycfg/internal/testdata/test_proto_gogo"; - -message MessageGogo { - option (gogoproto.sizer) = false; - option (gogoproto.marshaler) = false; - option (gogoproto.unmarshaler) = false; - - optional int32 f_int32 = 1; - optional int64 f_int64 = 2; - optional uint32 f_uint32 = 3; - optional uint64 f_uint64 = 4; - optional float f_float32 = 5; - optional double f_float64 = 6; - optional string f_string = 7 [default="default_str"];; - optional bool f_bool = 8; - - optional MessageV2 f_submsg = 9 [ - (gogoproto.nullable) = false - ]; - - repeated string r_string = 10; - repeated MessageV2 r_submsg = 11 [ - (gogoproto.nullable) = false - ]; - - map map_string = 12; - map map_submsg = 13; - - message NestedMessage { - optional string f_string = 1; - - message DoubleNestedMessage { - optional string f_string = 1; - } - } - optional NestedMessage f_nested_submsg = 16; - - enum NestedEnum { - NESTED_ENUM_A = 0; - NESTED_ENUM_B = 1; - } - optional ToplevelEnumV2 f_toplevel_enum = 14; - optional NestedEnum f_nested_enum = 15; - - oneof f_oneof { - string f_oneof_a = 17; - string f_oneof_b = 18; - } - - optional bytes f_bytes = 19; - - optional google.protobuf.Duration f_duration = 20 [ - (gogoproto.stdduration) = true, - (gogoproto.nullable) = false - ]; - - // NEXT: 21 -}