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

merged 12 commits into from
Jan 12, 2022

Conversation

mgebundy
Copy link
Contributor

closes #431
spin off of #474

@snqk @mgebundy
pgs: support proto3 presence & bump go.mod (bufbuild#431)
5eca99b 
Signed-off-by: Sarthak Gupta <signed@sarthak.sh>
@akonradi
Copy link
Contributor

Looks like you're running afoul of the DCO. See https://github.com/envoyproxy/envoy/blob/main/CONTRIBUTING.md#fixing-dco

@akonradi
Copy link
Contributor

The build action runs with Bazel, which uses a separate dependencies list than the Go build. Can you update the PGS dep here?

@mgebundy
bump lyft/protoc-gen-start to v0.6.0
e736bcf 
Signed-off-by: Mitchell Bundy <mitchell.bundy@unity3d.com>
@mgebundy
Copy link
Contributor Author

@akonradi got it! updated the PR. thanks for the rapid response :)

@mgebundy
bump protoc-gen-star to 0.6.0 in bazel dependencies
ef139b2 
Signed-off-by: Mitchell Bundy <mitchell.bundy@unity3d.com>
akonradi
akonradi suggested changes Nov 29, 2021
View reviewed changes
Copy link
Contributor

@akonradi akonradi left a comment

Choose a reason for hiding this comment

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

This is definitely going to need some tests.

templates/cc/msg.go
{{ 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.

@mgebundy
add 'optional.proto' file to test harness
06aa17b 
Signed-off-by: Mitchell Bundy <mitchell.bundy@unity3d.com>
@mgebundy mgebundy force-pushed the feature/add-optional-support branch 3 times, most recently from f9188cf to ad94922 Compare November 30, 2021 22:53
@mgebundy
add supported feature optional to go init
af6f8b4 
Signed-off-by: Mitchell Bundy <mitchell.bundy@unity3d.com>
@mgebundy
add types/pluginpb to list of deps in BUILD
c6772be 
Signed-off-by: Mitchell Bundy <mitchell.bundy@unity3d.com>
@mgebundy
bump protobuf in bazel deps to match go.mod version, add deps to BUILD
544bcc8 
Signed-off-by: Mitchell Bundy <mitchell.bundy@unity3d.com>
@mgebundy
bump com_google_protobuf to v3.15.3, matches go-protobuf v1.27 depend…
ae5977c 
…ency

Signed-off-by: Mitchell Bundy <mitchell.bundy@unity3d.com>
@mgebundy
Copy link
Contributor Author

mgebundy commented Dec 6, 2021

@akonradi I go things working for golang, but the java version seems to have some sort of dependency mismatch. There's an error, but that's not expected on the more recent version of protobuf that the golang library is correctly depending on.

I don't have any maven or bazel experience, and I'm struggling to find where the issue is. Any help here is appreciated.

@rodaine
Copy link
Member

rodaine commented Dec 8, 2021

@rmichela, if you have a chance to take a look?

@mgebundy mgebundy mentioned this pull request Dec 13, 2021
Closed
2 tasks
@rmichela
Copy link
Contributor

I figured out what was going on. The Java protobuf and grpc dependencies were for versions that predated proto optional fields. I updated the dependencies in #545, which will need to be merged before this PR will compile.

@loeffel-io
Copy link

Any update here? ✌️

@ryanrhee
Copy link

is this awaiting final review? @rodaine would you know who can approve this?

@loeffel-io
Copy link

✌️

@eroshiva
Copy link

eroshiva commented Jan 3, 2022

@akonradi @rodaine just a gentle reminder to catch up on this

@molinjun
Copy link

molinjun commented Jan 4, 2022

When will this feature be merged in? @akonradi

rodaine
rodaine reviewed Jan 6, 2022
View reviewed changes
Copy link
Member

@rodaine rodaine left a comment

Choose a reason for hiding this comment

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

Thanks for the hard work on this issue, and your patience on the review. Once the test cases are added, we can go ahead and merge this.

tests/harness/cases/optional.proto Outdated
}

message MessageRequiredButOptional { optional TestOptionalMsg val = 1 [(validate.rules).message.required = true]; }
message Int64LTEOptional { optional int64 val = 1 [(validate.rules).int64.lte = 64]; }
Copy link
Member

Choose a reason for hiding this comment

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

Please add some test cases to the executor, otherwise this won't actually test the behavior of these: https://github.com/envoyproxy/protoc-gen-validate/blob/main/tests/harness/executor/cases.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i moved the tests to existing proto files since creating new ones resulted in a level of complexity i didn't want to deal with.

@mgebundy mgebundy force-pushed the feature/add-optional-support branch 5 times, most recently from cd3e7a6 to 43718f8 Compare January 11, 2022 01:26
@mgebundy
add some test cases to try out
c5b310d 
Signed-off-by: Mitchell Bundy <mitchell.bundy@unity3d.com>
@ryanrhee
Copy link

tests passed! 🎉

@mgebundy mgebundy requested a review from rodaine January 11, 2022 17:54
@mgebundy
remove optionalCases from cases
173c5d0 
Signed-off-by: Mitchell Bundy <mitchell.bundy@unity3d.com>
@rodaine
Copy link
Member

rodaine commented Jan 12, 2022

Awesome job! Thank you for all the hard work to get this over the finish line 😁

@rodaine rodaine merged commit dc51e59 into bufbuild:main Jan 12, 2022
@rodaine rodaine mentioned this pull request Jan 12, 2022
Closed
@molinjun molinjun mentioned this pull request Jan 14, 2022
Closed
@mgebundy mgebundy deleted the feature/add-optional-support branch January 14, 2022 15:56
@glerchundi glerchundi mentioned this pull request Jan 20, 2022
Closed
mattklein123 pushed a commit that referenced this pull request Feb 26, 2022
Various small fixes
9a93c57 
1) Fix regression from
   #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 #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 <mklein@lyft.com>
@mattklein123 mattklein123 mentioned this pull request Feb 26, 2022
Merged
rodaine pushed a commit that referenced this pull request Feb 28, 2022
Various small fixes (#573)
3898097 
* Various small fixes

1) Fix regression from
   #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 #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 <mklein@lyft.com>

* fix

Signed-off-by: Matt Klein <mklein@lyft.com>

* fix

Signed-off-by: Matt Klein <mklein@lyft.com>
@acrazing acrazing mentioned this pull request Jul 6, 2022
Closed
@acrazing acrazing mentioned this pull request Sep 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsynesael-unity vsynesael-unity vsynesael-unity left review comments

@rodaine rodaine rodaine approved these changes

@akonradi akonradi Awaiting requested review from akonradi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

*: add proto3 optional support
10 participants
@mgebundy @akonradi @rodaine @rmichela @loeffel-io @ryanrhee @eroshiva @molinjun @vsynesael-unity @snqk