From 9a93c578f669b8b02384797cbd0594c0fa64eb04 Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Fri, 25 Feb 2022 00:19:12 +0000 Subject: [PATCH] Various small fixes 1) Fix regression from https://github.com/envoyproxy/protoc-gen-validate/pull/529. The code needs to handle enums at file scope and in a message. At the same time also handle arbitrary nesting. 2) Fix regression from https://github.com/envoyproxy/protoc-gen-validate/pull/537 when dealing with single element string oneofs (and possibly other types being returned as pointer types vs. value types and breaking in queries. This fixes that for both C++ and Go. It's possible this fix shoudl be in PG* but doing it here for simplicity. 3) Modify the C++ validator registry to have polymorphic lookup capability which I want to use in Envoy. Signed-off-by: Matt Klein --- templates/cc/register.go | 3 ++- templates/go/file.go | 2 +- templates/goshared/register.go | 23 ++++++++++++++++++- tests/harness/cases/enums.proto | 1 + tests/harness/cases/other_package/embed.proto | 4 ++++ tests/harness/cases/strings.proto | 6 +++++ validate/validate.h | 3 ++- 7 files changed, 38 insertions(+), 4 deletions(-) diff --git a/templates/cc/register.go b/templates/cc/register.go index 615932c4c..7792e77f9 100644 --- a/templates/cc/register.go +++ b/templates/cc/register.go @@ -318,7 +318,8 @@ func (fns CCFuncs) cType(t pgs.FieldType) string { return fns.cTypeOfString(fns.Type(t.Field()).Element().String()) } - return fns.cTypeOfString(fns.Type(t.Field()).String()) + // Use Value() to strip any potential pointer type. + return fns.cTypeOfString(fns.Type(t.Field()).Value().String()) } func (fns CCFuncs) cTypeOfString(s string) string { diff --git a/templates/go/file.go b/templates/go/file.go index fdb359add..8e382a373 100644 --- a/templates/go/file.go +++ b/templates/go/file.go @@ -41,7 +41,7 @@ var ( _ = sort.Sort {{ range $pkg, $path := enumPackages (externalEnums .) }} - _ = {{ $pkg }}.{{ (index (externalEnums $) 0).Parent.Name }}_{{ (index (externalEnums $) 0).Name }}(0) + _ = {{ $pkg }}.{{ enumName (index (externalEnums $) 0) }}(0) {{ end }} ) diff --git a/templates/goshared/register.go b/templates/goshared/register.go index a6a039ff3..e342fb7c8 100644 --- a/templates/goshared/register.go +++ b/templates/goshared/register.go @@ -48,6 +48,7 @@ func Register(tpl *template.Template, params pgs.Parameters) { "typ": fns.Type, "unwrap": fns.unwrap, "externalEnums": fns.externalEnums, + "enumName": fns.enumName, "enumPackages": fns.enumPackages, }) @@ -197,6 +198,11 @@ func (fns goSharedFuncs) isBytes(f interface { return f.ProtoType() == pgs.BytesT } +func (fns goSharedFuncs) isMessage(i interface{}) bool { + _, ok := i.(pgs.Message) + return ok +} + func (fns goSharedFuncs) byteStr(x []byte) string { elms := make([]string, len(x)) for i, b := range x { @@ -228,7 +234,8 @@ func (fns goSharedFuncs) inType(f pgs.Field, x interface{}) string { return fns.Type(f).String() } default: - return fns.Type(f).String() + // Use Value() to strip any potential pointer type. + return fns.Type(f).Value().String() } } @@ -326,6 +333,20 @@ func (fns goSharedFuncs) externalEnums(file pgs.File) []pgs.Enum { return out } +func (fns goSharedFuncs) enumName(enum pgs.Enum) string { + out := string(enum.Name()) + parent := enum.Parent() + for { + message, ok := parent.(pgs.Message) + if ok { + out = string(message.Name()) + "_" + out + parent = message.Parent() + } else { + return out + } + } +} + func (fns goSharedFuncs) enumPackages(enums []pgs.Enum) map[pgs.Name]pgs.FilePath { out := make(map[pgs.Name]pgs.FilePath, len(enums)) diff --git a/tests/harness/cases/enums.proto b/tests/harness/cases/enums.proto index 010c6b1fe..ea46b08b5 100644 --- a/tests/harness/cases/enums.proto +++ b/tests/harness/cases/enums.proto @@ -39,6 +39,7 @@ message EnumNotIn { TestEnum val = 1 [(validate.rules).enum = { not_in: [1] message EnumAliasNotIn { TestEnumAlias val = 1 [(validate.rules).enum = { not_in: [1]}]; } message EnumExternal { other_package.Embed.Enumerated val = 1 [(validate.rules).enum.defined_only = true]; } +message EnumExternal2 { other_package.Embed.DoubleEmbed.DoubleEnumerated val = 1 [(validate.rules).enum.defined_only = true]; } message RepeatedEnumDefined { repeated TestEnum val = 1 [(validate.rules).repeated.items.enum.defined_only = true]; } message RepeatedExternalEnumDefined { repeated other_package.Embed.Enumerated val = 1 [(validate.rules).repeated.items.enum.defined_only = true]; } diff --git a/tests/harness/cases/other_package/embed.proto b/tests/harness/cases/other_package/embed.proto index 59cefe002..22d1f1497 100644 --- a/tests/harness/cases/other_package/embed.proto +++ b/tests/harness/cases/other_package/embed.proto @@ -7,6 +7,10 @@ import "validate/validate.proto"; // Validate message embedding across packages. message Embed { + message DoubleEmbed { + enum DoubleEnumerated { VALUE = 0; } + } + int64 val = 1 [(validate.rules).int64.gt = 0]; enum Enumerated { VALUE = 0; } diff --git a/tests/harness/cases/strings.proto b/tests/harness/cases/strings.proto index 2efc9371a..19fe29e61 100644 --- a/tests/harness/cases/strings.proto +++ b/tests/harness/cases/strings.proto @@ -37,3 +37,9 @@ message StringHttpHeaderName { string val = 1 [(validate.rules).string.well_know message StringHttpHeaderValue { string val = 1 [(validate.rules).string.well_known_regex = HTTP_HEADER_VALUE]; } message StringValidHeader { string val = 1 [(validate.rules).string = {well_known_regex: HTTP_HEADER_VALUE, strict: false}]; } message StringUUIDIgnore { string val = 1 [(validate.rules).string = {uuid: true, ignore_empty: true}]; } + +message StringInOneOf { + oneof foo { + string bar = 1 [(validate.rules).string = {in: "a" in: "b"}]; + } + } diff --git a/validate/validate.h b/validate/validate.h index 15e2c4bff..8ad4c8b8a 100644 --- a/validate/validate.h +++ b/validate/validate.h @@ -59,7 +59,8 @@ class Validator : public BaseValidator { static bool CheckMessage(const T& m, ValidationMsg* err) { - auto val = static_cast*>(validators()[std::type_index(typeid(T))]); + // Run typeid() on the variable vs. the type to allow polymorphic lookup of the validator. + auto val = static_cast*>(validators()[std::type_index(typeid(m))]); if (val) { return val->check_(m, err); }