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.

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

5113. Validation of oneof nested objects #5211

Conversation

optician
Copy link
Contributor

@optician optician commented Feb 6, 2025

References to other Issues or PRs

Fixes #5113

Have you read the Contributing Guidelines?

Yes.

Brief description of what is fixed or changed

The fix follows the suggestion from the issue. The oneOf check is moved before msgValue.Mutable() resets other option of this oneOf.
Also, previous behavior didn't validate nested messages correctly. After the cycle "fieldDescriptor" points to a leaf of a path tree and it's not a member of a oneOf field. For example, the piece of a proto

 ...
  oneof nested_oneof_value {
    int32 nested_oneof_int32_value = 50;
    Proto3Message nested_oneof_value_one = 46;
  }
 ...

and the path ["nested_oneof_value_one", "int32Value"]. WhichOneof(nested_oneof_value_one) returns nested_oneof_value, but `WhichOneof(int32Value) returns nil.

Other comments

I struggled with codegen refreshing and spent quite a lot of time on a simple task. The contribution instruction does not regenerate *.pb.go files when protos are changed (and runs very slowly because it recompiles protoc). Other documentation also lacks instructions. I hope I found the correct way and it will save time for others.

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

Minus the contribution guide changes, this looks great, thank you so much!

CONTRIBUTING.md Outdated Show resolved Hide resolved
@johanbrandhorst
Copy link
Collaborator

Seems like some swagger files still need to be regenerated, could you try running it again? See the failures in the generate CI job.

@optician
Copy link
Contributor Author

optician commented Feb 7, 2025

As I understand the file shouldn't change. It seems ok now. I can't explain what happened in the previous iteration 🤷

@johanbrandhorst johanbrandhorst enabled auto-merge (squash) February 7, 2025 19:09
@johanbrandhorst johanbrandhorst merged commit 3b8ac9f into grpc-ecosystem:main Feb 7, 2025
14 checks passed
@johanbrandhorst
Copy link
Collaborator

Thanks for your contribution!

@optician optician deleted the 5113_validation_of_oneof_nested_objects branch February 7, 2025 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expecting a oneof validation error but only getting it intermittently
2 participants