In/NotIn and const rules show enum values for enum type (#699)

## Problem The error message for enum `in/not_in` rules show numbers instead of enum values Sample case **Input:** ```protobuf ... enum Enum { UNKNOWN_ENUM = 0; OPTION_1 = 1; OPTION_2 = 2; } message Book { Enum enum1 = 1 [(validate.rules).enum = {in: [1, 2]}]; Enum enum2 = 2 [(validate.rules).enum = {not_in: [1]}]; Enum enum3 = 3 [(validate.rules).enum = {const: 1}]; } ``` **Current output:** ```go func (m *Book) validate(all bool) error { ... if _, ok := _Book_Enum1_InLookup[m.GetEnum1()]; !ok { err := BookValidationError{ field: "Enum1", reason: "value must be in list [1 2]", } ... } if _, ok := _Book_Enum2_NotInLookup[m.GetEnum2()]; ok { err := BookValidationError{ field: "Enum2", reason: "value must not be in list [1]", } ... } if m.GetEnum3() != 1 { err := BookValidationError{ field: "Enum3", reason: "value must equal 1", } ... } } ``` **Desired output:** ```go func (m *Book) validate(all bool) error { ... if _, ok := _Book_Enum1_InLookup[m.GetEnum1()]; !ok { err := BookValidationError{ field: "Enum1", reason: "value must be in list [OPTION_1 OPTION_2]", } ... } if _, ok := _Book_Enum2_NotInLookup[m.GetEnum2()]; ok { err := BookValidationError{ field: "Enum2", reason: "value must not be in list [OPTION_1]", } ... } if m.GetEnum3() != 1 { err := BookValidationError{ field: "Enum3", reason: "value must equal OPTION_1", } .... } } ``` ## Changes - Added `isEnum` function to check if the field type is enum - Added `enumList` function to return options string (e.g. `[OPTION_1 OPTION_2]` ) for given field and `in/notIn` list - Added `enumVal` function to return option (for const) - Change `cpp`, `goshared` templates ### TODO - [x] Add test cases - [x] Update [compatibility set](https://github.com/bufbuild/protoc-gen-validate/blob/main/rule_comparison.md) Co-authored-by: Elliot Jackson <[email protected]>
bufbuild · Jan 10, 2023 · a658586 · a658586
1 parent 9513a03
commit a658586
Show file tree

Hide file tree

Showing 8 changed files with 116 additions and 10 deletions.
diff --git a/python/protoc_gen_validate/validator.py b/python/protoc_gen_validate/validator.py
@@ -308,9 +308,6 @@ def const_template(option_value, name):
  {%- elif str(o.bool) and o.bool['const'] != "" -%}
  if {{ name }} != {{ o.bool['const'] }}:
  raise ValidationFailed(\"{{ name }} not equal to {{ o.bool['const'] }}\")
- {%- elif str(o.enum) and o.enum['const'] -%}
- if {{ name }} != {{ o.enum['const'] }}:
- raise ValidationFailed(\"{{ name }} not equal to {{ o.enum['const'] }}\")
  {%- elif str(o.bytes) and o.bytes.HasField('const') -%}
  {% if sys.version_info[0] >= 3 %}
  if {{ name }} != {{ o.bytes['const'] }}:
@@ -828,17 +825,52 @@ def enum_values(field):
  return [x.number for x in field.enum_type.values]
 
 
+def enum_name(field, number):
+ for x in field.enum_type.values:
+ if x.number == number:
+ return x.name
+ return ""
+
+
+def enum_names(field, numbers):
+ m = {x.number: x.name for x in field.enum_type.values}
+ return "[" + "".join([m[n] for n in numbers]) + "]"
+
+
+def enum_const_template(value, name, field):
+ const_tmpl = """{%- if str(value) and value['const'] -%}
+ if {{ name }} != {{ value['const'] }}:
+ raise ValidationFailed(\"{{ name }} not equal to {{ enum_name(field, value['const']) }}\")
+ {%- endif -%}
+ """
+ return Template(const_tmpl).render(value=value, name=name, field=field, enum_name=enum_name, str=str)
+
+
+def enum_in_template(value, name, field):
+ in_tmpl = """
+ {%- if value['in'] %}
+ if {{ name }} not in {{ value['in'] }}:
+ raise ValidationFailed(\"{{ name }} not in {{ enum_names(field, value['in']) }}\")
+ {%- endif -%}
+ {%- if value['not_in'] %}
+ if {{ name }} in {{ value['not_in'] }}:
+ raise ValidationFailed(\"{{ name }} in {{ enum_names(field, value['not_in']) }}\")
+ {%- endif -%}
+ """
+ return Template(in_tmpl).render(value=value, name=name, field=field, enum_names=enum_names)
+
+
 def enum_template(option_value, name, field):
  enum_tmpl = """
- {{ const_template(option_value, name) -}}
- {{ in_template(option_value.enum, name) -}}
+ {{ enum_const_template(option_value.enum, name, field) -}}
+ {{ enum_in_template(option_value.enum, name, field) -}}
  {% if option_value.enum['defined_only'] %}
  if {{ name }} not in {{ enum_values(field) }}:
  raise ValidationFailed(\"{{ name }} is not defined\")
  {% endif %}
  """
- return Template(enum_tmpl).render(option_value=option_value, name=name, const_template=const_template,
- in_template=in_template, field=field, enum_values=enum_values)
+ return Template(enum_tmpl).render(option_value=option_value, name=name, enum_const_template=enum_const_template,
+ enum_in_template=enum_in_template, field=field, enum_values=enum_values)
 
 
 def any_template(option_value, name, repeated=False):

diff --git a/templates/cc/const.go b/templates/cc/const.go
@@ -1,9 +1,13 @@
 package cc
 
-const constTpl = `{{ $r := .Rules }}
+const constTpl = `{{ $f := .Field }}{{ $r := .Rules }}
  {{ if $r.Const }}
  if ({{ accessor . }} != {{ lit $r.GetConst }}) {
+ {{- if isEnum $f }}
+ {{ err . "value must equal " (enumVal $f $r.GetConst) }}
+ {{- else }}
  {{ err . "value must equal " (lit $r.GetConst) }}
+ {{- end }}
  }
  {{ end }}
 `
diff --git a/templates/cc/in.go b/templates/cc/in.go
@@ -3,11 +3,19 @@ package cc
 const inTpl = `{{ $f := .Field -}}{{ $r := .Rules -}}
  {{- if $r.In }}
  if ({{ lookup $f "InLookup" }}.find(static_cast<decltype({{ lookup $f "InLookup" }})::key_type>({{ accessor . }})) == {{ lookup $f "InLookup" }}.end()) {
+ {{- if isEnum $f }}
+ {{ err . "value must be in list " (enumList $f $r.In) }}
+ {{- else }}
  {{ err . "value must be in list " $r.In }}
+ {{- end }}
  }
  {{- else if $r.NotIn }}
  if ({{ lookup $f "NotInLookup" }}.find(static_cast<decltype({{ lookup $f "NotInLookup" }})::key_type>({{ accessor . }})) != {{ lookup $f "NotInLookup" }}.end()) {
+ {{- if isEnum $f }}
+ {{ err . "value must not be in list " (enumList $f $r.NotIn) }}
+ {{- else }}
  {{ err . "value must not be in list " $r.NotIn }}
+ {{- end }}
  }
  {{- end }}
 `
diff --git a/templates/goshared/const.go b/templates/goshared/const.go
@@ -1,9 +1,13 @@
 package goshared
 
-const constTpl = `{{ $r := .Rules }}
+const constTpl = `{{ $f := .Field }}{{ $r := .Rules }}
  {{ if $r.Const }}
  if {{ accessor . }} != {{ lit $r.GetConst }} {
+ {{- if isEnum $f }}
+ err := {{ err . "value must equal " (enumVal $f $r.GetConst) }}
+ {{- else }}
  err := {{ err . "value must equal " $r.GetConst }}
+ {{- end }}
  if !all { return err }
  errors = append(errors, err)
  }

diff --git a/templates/goshared/in.go b/templates/goshared/in.go
@@ -3,13 +3,21 @@ package goshared
 const inTpl = `{{ $f := .Field }}{{ $r := .Rules }}
  {{ if $r.In }}
  if _, ok := {{ lookup $f "InLookup" }}[{{ accessor . }}]; !ok {
+ {{- if isEnum $f }}
+ err := {{ err . "value must be in list " (enumList $f $r.In) }}
+ {{- else }}
  err := {{ err . "value must be in list " $r.In }}
+ {{- end }}
  if !all { return err }
  errors = append(errors, err)
  }
  {{ else if $r.NotIn }}
  if _, ok := {{ lookup $f "NotInLookup" }}[{{ accessor . }}]; ok {
+ {{- if isEnum $f }}
+ err := {{ err . "value must not be in list " (enumList $f $r.NotIn) }}
+ {{- else }}
  err := {{ err . "value must not be in list " $r.NotIn }}
+ {{- end }}
  if !all { return err }
  errors = append(errors, err)
  }

diff --git a/templates/shared/BUILD.bazel b/templates/shared/BUILD.bazel
@@ -8,6 +8,7 @@ go_library(
  "functions.go",
  "reflection.go",
  "well_known.go",
+ "enums.go"
  ],
  importpath = "github.com/envoyproxy/protoc-gen-validate/templates/shared",
  visibility = ["//visibility:public"],

diff --git a/templates/shared/enums.go b/templates/shared/enums.go
@@ -0,0 +1,46 @@
+package shared
+
+import (
+ "fmt"
+ "strings"
+
+ pgs "github.com/lyft/protoc-gen-star"
+)
+
+func isEnum(f pgs.Field) bool {
+ return f.Type().IsEnum()
+}
+
+func enumNamesMap(values []pgs.EnumValue) (m map[int32]string) {
+ m = make(map[int32]string)
+ for _, v := range values {
+ if _, exists := m[v.Value()]; !exists {
+ m[v.Value()] = v.Name().String()
+ }
+ }
+ return m
+}
+
+// enumList - if type is ENUM, enum values are returned
+func enumList(f pgs.Field, list []int32) string {
+ stringList := make([]string, 0, len(list))
+ if enum := f.Type().Enum(); enum != nil {
+ names := enumNamesMap(enum.Values())
+ for _, n := range list {
+ stringList = append(stringList, names[n])
+ }
+ } else {
+ for _, n := range list {
+ stringList = append(stringList, fmt.Sprint(n))
+ }
+ }
+ return "[" + strings.Join(stringList, " ") + "]"
+}
+
+// enumVal - if type is ENUM, enum value is returned
+func enumVal(f pgs.Field, val int32) string {
+ if enum := f.Type().Enum(); enum != nil {
+ return enumNamesMap(enum.Values())[val]
+ }
+ return fmt.Sprint(val)
+}
diff --git a/templates/shared/functions.go b/templates/shared/functions.go
@@ -3,7 +3,7 @@ package shared
 import (
  "text/template"
 
- "github.com/lyft/protoc-gen-star"
+ pgs "github.com/lyft/protoc-gen-star"
 )
 
 func RegisterFunctions(tpl *template.Template, params pgs.Parameters) {
@@ -16,5 +16,8 @@ func RegisterFunctions(tpl *template.Template, params pgs.Parameters) {
  "has": Has,
  "needs": Needs,
  "fileneeds": FileNeeds,
+ "isEnum": isEnum,
+ "enumList": enumList,
+ "enumVal": enumVal,
  })
 }