Feature/add optional support (#537)

* pgs: support proto3 presence & bump go.mod (#431) Signed-off-by: Sarthak Gupta <[email protected]> * bump lyft/protoc-gen-start to v0.6.0 Signed-off-by: Mitchell Bundy <[email protected]> * bump protoc-gen-star to 0.6.0 in bazel dependencies Signed-off-by: Mitchell Bundy <[email protected]> * add 'optional.proto' file to test harness Signed-off-by: Mitchell Bundy <[email protected]> * add supported feature optional to go init Signed-off-by: Mitchell Bundy <[email protected]> * add types/pluginpb to list of deps in BUILD Signed-off-by: Mitchell Bundy <[email protected]> * bump protobuf in bazel deps to match go.mod version, add deps to BUILD Signed-off-by: Mitchell Bundy <[email protected]> * bump com_google_protobuf to v3.15.3, matches go-protobuf v1.27 dependency Signed-off-by: Mitchell Bundy <[email protected]> * update optional test to include optional int64 field within message Signed-off-by: Mitchell Bundy <[email protected]> * add some test cases to try out Signed-off-by: Mitchell Bundy <[email protected]> * remove optionalCases from cases Signed-off-by: Mitchell Bundy <[email protected]> Co-authored-by: Sarthak Gupta <[email protected]> Co-authored-by: Ryan Michela <[email protected]>
bufbuild · Jan 12, 2022 · dc51e59 · dc51e59
1 parent 6a061a0
commit dc51e59
Show file tree

Hide file tree

Showing 12 changed files with 35 additions and 15 deletions.
diff --git a/BUILD b/BUILD
@@ -22,6 +22,7 @@ go_library(
     visibility = ["//visibility:private"],
     deps = [
         "//module",
+        "@org_golang_google_protobuf//types/pluginpb",
         "@com_github_lyft_protoc_gen_star//:protoc-gen-star",
         "@com_github_lyft_protoc_gen_star//lang/go",
     ],

diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl
@@ -28,9 +28,9 @@ def pgv_dependencies(maven_repos = _DEFAULT_REPOSITORIES):
     if not native.existing_rule("com_google_protobuf"):
         http_archive(
             name = "com_google_protobuf",
-            url = "https://github.com/protocolbuffers/protobuf/archive/v3.14.0.tar.gz",
-            sha256 = "d0f5f605d0d656007ce6c8b5a82df3037e1d8fe8b121ed42e536f569dec16113",
-            strip_prefix = "protobuf-3.14.0",
+            url = "https://github.com/protocolbuffers/protobuf/archive/v3.15.3.tar.gz",
+            sha256 = "b10bf4e2d1a7586f54e64a5d9e7837e5188fc75ae69e36f215eb01def4f9721b",
+            strip_prefix = "protobuf-3.15.3",
         )
 
     # TODO(akonradi): This shouldn't be necesary since the same http_archive block is imported by

diff --git a/dependencies.bzl b/dependencies.bzl
@@ -35,8 +35,8 @@ def go_third_party():
     go_repository(
         name = "com_github_lyft_protoc_gen_star",
         importpath = "github.com/lyft/protoc-gen-star",
-        sum = "h1:zSGLzsUew8RT+ZKPHc3jnf8XLaVyHzTcAFBzHtCNR20=",
-        version = "v0.5.3",
+        sum = "h1:xOpFu4vwmIoUeUrRuAtdCrZZymT/6AkW/bsUWA506Fo=",
+        version = "v0.6.0",
     )
     go_repository(
         name = "com_github_pkg_errors",
@@ -105,7 +105,7 @@ def go_third_party():
         name = "org_golang_google_protobuf",
         importpath = "google.golang.org/protobuf",
         sum = "h1:bxAC2xTBsZGibn2RTntX0oH50xLsqy1OxA9tTL3p/lk=",
-        version = "v1.26.0",
+        version = "v1.27.1",
     )
     go_repository(
         name = "org_golang_x_crypto",

diff --git a/go.mod b/go.mod
@@ -4,7 +4,7 @@ go 1.12
 
 require (
 	github.com/iancoleman/strcase v0.2.0
-	github.com/lyft/protoc-gen-star v0.5.3
+	github.com/lyft/protoc-gen-star v0.6.0
 	github.com/spf13/afero v1.6.0 // indirect
 	golang.org/x/lint v0.0.0-20210508222113-6edffad5e616
 	golang.org/x/mod v0.5.0 // indirect

diff --git a/go.sum b/go.sum
@@ -8,8 +8,8 @@ github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/
 github.com/iancoleman/strcase v0.2.0 h1:05I4QRnGpI0m37iZQRuskXh+w77mr6Z41lwQzuHLwW0=
 github.com/iancoleman/strcase v0.2.0/go.mod h1:iwCmte+B7n89clKwxIoIXy/HfoL7AsD47ZCWhYzw7ho=
 github.com/kr/fs v0.1.0/go.mod h1:FFnZGqtBN9Gxj7eW1uZ42v5BccTP0vu6NEaFoC2HwRg=
-github.com/lyft/protoc-gen-star v0.5.3 h1:zSGLzsUew8RT+ZKPHc3jnf8XLaVyHzTcAFBzHtCNR20=
-github.com/lyft/protoc-gen-star v0.5.3/go.mod h1:V0xaHgaf5oCCqmcxYcWiDfTiKsZsRc87/1qhoTACD8w=
+github.com/lyft/protoc-gen-star v0.6.0 h1:xOpFu4vwmIoUeUrRuAtdCrZZymT/6AkW/bsUWA506Fo=
+github.com/lyft/protoc-gen-star v0.6.0/go.mod h1:TGAoBVkt8w7MPG72TrKIu85MIdXwDuzJYeZuUPFPNwA=
 github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
 github.com/pkg/sftp v1.10.1/go.mod h1:lYOWFsE0bwd1+KfKJaKeuokY15vzFx25BLbzYYoAxZI=
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=

diff --git a/main.go b/main.go
@@ -1,14 +1,16 @@
 package main
 
 import (
+	"github.com/envoyproxy/protoc-gen-validate/module"
 	"github.com/lyft/protoc-gen-star"
 	"github.com/lyft/protoc-gen-star/lang/go"
-	"github.com/envoyproxy/protoc-gen-validate/module"
+	"google.golang.org/protobuf/types/pluginpb"
 )
 
 func main() {
+	optional := uint64(pluginpb.CodeGeneratorResponse_FEATURE_PROTO3_OPTIONAL)
 	pgs.
-		Init(pgs.DebugEnv("DEBUG_PGV")).
+		Init(pgs.DebugEnv("DEBUG_PGV"), pgs.SupportedFeatures(&optional)).
 		RegisterModule(module.Validator()).
 		RegisterPostProcessor(pgsgo.GoFmt()).
 		Render()

diff --git a/templates/cc/msg.go b/templates/cc/msg.go
@@ -102,7 +102,7 @@ bool Validate(const {{ class . }}& m, pgv::ValidationMsg* err) {
 		{{ range .NonOneOfFields }}
 			{{- render (context .) -}}
 		{{ end -}}
-		{{ range .OneOfs }}
+		{{ range .RealOneOfs }}
 			switch (m.{{ .Name }}_case()) {
 				{{ range .Fields -}}
 					case {{ oneof . }}:

diff --git a/templates/goshared/msg.go b/templates/goshared/msg.go
@@ -33,7 +33,7 @@ func (m {{ (msgTyp .).Pointer }}) validate(all bool) error {
 			{{ render (context .) }}
 		{{ end }}
 
-		{{ range .OneOfs }}
+		{{ range .RealOneOfs }}
 			switch m.{{ name . }}.(type) {
 				{{ range .Fields }}
 					case {{ oneof . }}:
@@ -51,9 +51,16 @@ func (m {{ (msgTyp .).Pointer }}) validate(all bool) error {
 			}
 		{{ end }}
 
+		{{ range .SyntheticOneOfFields }}
+			if m.{{ name . }} != nil {
+				{{ render (context .) }}
+			}
+		{{ end }}
+
 		if len(errors) > 0 {
 			return {{ multierrname . }}(errors)
 		}
+
 		return nil
 	{{ end -}}
 }

diff --git a/templates/java/msg.go b/templates/java/msg.go
@@ -15,7 +15,7 @@ const msgInnerTpl = `
 	{{- range .NonOneOfFields }}
 		{{ renderConstants (context .) }}
 	{{ end }}
-	{{ range .OneOfs }}
+	{{ range .RealOneOfs }}
 		{{ template "oneOfConst" . }}
 	{{ end }}
 
@@ -27,7 +27,7 @@ const msgInnerTpl = `
 	{{ range .NonOneOfFields -}}
 		{{ render (context .) }}
 	{{ end -}}
-	{{ range .OneOfs }}
+	{{ range .RealOneOfs }}
 		{{ template "oneOf" . }}
 	{{- end -}}
 	{{- end }}

diff --git a/tests/harness/cases/messages.proto b/tests/harness/cases/messages.proto
@@ -30,6 +30,7 @@ message Message { TestMsg val = 1; }
 message MessageCrossPackage { tests.harness.cases.other_package.Embed val = 1; }
 message MessageSkip { TestMsg val = 1 [(validate.rules).message.skip = true];}
 message MessageRequired { TestMsg val = 1 [(validate.rules).message.required = true]; }
+message MessageRequiredButOptional { optional TestMsg val = 1 [(validate.rules).message.required = true]; }
 
 message MessageRequiredOneof {
     oneof one {

diff --git a/tests/harness/cases/numbers.proto b/tests/harness/cases/numbers.proto
@@ -171,3 +171,5 @@ message SFixed64ExLTGT   { sfixed64 val = 1 [(validate.rules).sfixed64 = {lt: 0,
 message SFixed64GTELTE   { sfixed64 val = 1 [(validate.rules).sfixed64 = {gte: 128, lte: 256}]; }
 message SFixed64ExGTELTE { sfixed64 val = 1 [(validate.rules).sfixed64 = {lte: 128, gte: 256}]; }
 message SFixed64Ignore   { sfixed64 val = 1 [(validate.rules).sfixed64 = {lte: 128, gte: 256, ignore_empty: true}]; }
+
+message Int64LTEOptional      { optional int64 val = 1 [(validate.rules).int64.lte = 64]; }
diff --git a/tests/harness/executor/cases.go b/tests/harness/executor/cases.go
@@ -279,6 +279,10 @@ var int64Cases = []TestCase{
 	{"int64 - exclusive gte & lte - invalid", &cases.Int64ExGTELTE{Val: 200}, 1},
 
 	{"int64 - ignore_empty gte & lte - valid", &cases.Int64Ignore{Val: 0}, 0},
+
+	{"int64 optional - lte - valid", &cases.Int64LTEOptional{Val: &wrapperspb.Int64(63).Value}, 0},
+	{"int64 optional - lte - valid (equal)", &cases.Int64LTEOptional{Val: &wrapperspb.Int64(64).Value}, 0},
+	{"int64 optional - lte - valid (unset)", &cases.Int64LTEOptional{}, 0},
 }
 
 var uint32Cases = []TestCase{
@@ -1059,6 +1063,9 @@ var messageCases = []TestCase{
 	{"message - cross-package embed none - valid (nil)", &cases.MessageCrossPackage{}, 0},
 	{"message - cross-package embed none - valid (empty)", &cases.MessageCrossPackage{Val: &other_package.Embed{}}, 1},
 	{"message - cross-package embed none - invalid", &cases.MessageCrossPackage{Val: &other_package.Embed{Val: -1}}, 1},
+
+	{"message - required - valid", &cases.MessageRequiredButOptional{Val: &cases.TestMsg{Const: "foo"}}, 0},
+	{"message - required - valid (unset)", &cases.MessageRequiredButOptional{}, 0},
 }
 
 var repeatedCases = []TestCase{