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.

Sign up for GitHub

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

Jump to bottom

Feature/add optional support #537

Merged
merged 12 commits into from
Jan 12, 2022
Merged

Feature/add optional support #537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5eca99b
pgs: support proto3 presence & bump go.mod (#431)
snqk May 22, 2021
e736bcf
bump lyft/protoc-gen-start to v0.6.0
mgebundy Nov 18, 2021
ef139b2
bump protoc-gen-star to 0.6.0 in bazel dependencies
mgebundy Nov 29, 2021
06aa17b
add 'optional.proto' file to test harness
mgebundy Nov 29, 2021
af6f8b4
add supported feature optional to go init
mgebundy Nov 30, 2021
c6772be
add types/pluginpb to list of deps in BUILD
mgebundy Nov 30, 2021
544bcc8
bump protobuf in bazel deps to match go.mod version, add deps to BUILD
mgebundy Nov 30, 2021
ae5977c
bump com_google_protobuf to v3.15.3, matches go-protobuf v1.27 depend…
mgebundy Dec 1, 2021
cdc0715
Merge branch 'main' into feature/add-optional-support
rmichela Dec 13, 2021
2f52871
update optional test to include optional int64 field within message
mgebundy Dec 13, 2021
c5b310d
add some test cases to try out
mgebundy Jan 7, 2022
173c5d0
remove optionalCases from cases
mgebundy Jan 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
Expand Down
6 changes: 3 additions & 3 deletions bazel/repositories.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions dependencies.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down
6 changes: 4 additions & 2 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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()
Expand Down
2 changes: 1 addition & 1 deletion templates/cc/msg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ bool Validate(const {{ class . }}& m, pgv::ValidationMsg* err) {
{{ range .NonOneOfFields }}
{{- render (context .) -}}
{{ end -}}
{{ range .OneOfs }}
{{ range .RealOneOfs }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this going to cause problems for messages with optional fields with validations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generating gofiles with a message like this works just fine now.

message AddEntityEntitlementRequest {
    optional string name= 1 [(validate.rules).string = {
        pattern:   "^[^[0-9]A-Za-z]+( [^[0-9]A-Za-z]+)*$",
        max_bytes: 256,
    }]; /** name */
}

the regex fails when expected and if no value is provided, no error is thrown, since it's marked as optional.

I don't know the ins and outs of generation, especially since that piece comes from the original PR, so I'm not sure what the halo effect is like with this change.

switch (m.{{ .Name }}_case()) {
{{ range .Fields -}}
case {{ oneof . }}:
Expand Down
9 changes: 8 additions & 1 deletion templates/goshared/msg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 . }}:
Expand All @@ -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 -}}
}
Expand Down
4 changes: 2 additions & 2 deletions templates/java/msg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const msgInnerTpl = `
{{- range .NonOneOfFields }}
{{ renderConstants (context .) }}
{{ end }}
{{ range .OneOfs }}
{{ range .RealOneOfs }}
{{ template "oneOfConst" . }}
{{ end }}

Expand All @@ -27,7 +27,7 @@ const msgInnerTpl = `
{{ range .NonOneOfFields -}}
{{ render (context .) }}
{{ end -}}
{{ range .OneOfs }}
{{ range .RealOneOfs }}
{{ template "oneOf" . }}
{{- end -}}
{{- end }}
Expand Down
1 change: 1 addition & 0 deletions tests/harness/cases/messages.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 2 additions & 0 deletions tests/harness/cases/numbers.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]; }
7 changes: 7 additions & 0 deletions tests/harness/executor/cases.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand Down