Skip to content

fix: handle synthetic oneofs for proto3 optional fields#26

Closed
dsociative wants to merge 4 commits intolinka-cloud:mainfrom
dsociative:fix-optional-proto3
Closed

fix: handle synthetic oneofs for proto3 optional fields#26
dsociative wants to merge 4 commits intolinka-cloud:mainfrom
dsociative:fix-optional-proto3

Conversation

@dsociative
Copy link

Fix path generation for proto3 optional fields

Problem

Proto3 optional fields are implemented using synthetic oneofs (e.g., oneof _field { string field = 1; }). prost-validate was treating these as regular oneofs, generating incorrect paths like message._field.field instead of message.field.

Solution

Added containing_non_synthetic_oneof() helper that filters out synthetic oneofs using is_synthetic() check, ensuring correct path generation for optional fields.

Updated prost-reflect to the latest version and all its dependencies, as the is_synthetic() method on OneofDescriptor was only added in recent versions.

Changes

  • Synthetic oneofs (proto3 optional): message.field
  • Real oneofs: message.oneof_name.field
  • Regular fields: message.field
  • Updated prost-reflect and dependencies to support is_synthetic() API

@dsociative dsociative force-pushed the fix-optional-proto3 branch 2 times, most recently from 8e076ab to f6004cd Compare October 22, 2025 14:54
Add `containing_non_synthetic_oneof` helper to distinguish between
real oneofs and synthetic oneofs generated for proto3 optional fields.
This fixes incorrect path generation where optional fields were being
treated as regular oneof members (e.g., `message._field.field` instead
of `message.field`).
@Adphi
Copy link
Member

Adphi commented Nov 14, 2025

Thanks a lot for this contribution @dsociative!

When you get a chance, could you please rebase the PR on the latest main branch?
That should help us move it forward. Thanks again!

Signed-off-by: Adphi <philippe.adrien.nousse@gmail.com>
Signed-off-by: Adphi <philippe.adrien.nousse@gmail.com>
Signed-off-by: Adphi <philippe.adrien.nousse@gmail.com>
@dsociative
Copy link
Author

Thanks for the review! I've rebased on the latest main.

I've encountered an edge case with this test that's now failing due to the fix. The proto defines an optional field with message.required = true:

message MessageRequiredButOptional { optional TestMsg val = 1 [(validate.rules).message.required = true]; }

With Rust's type system, we can't distinguish between an unset optional field and one explicitly set to None. What should the expected validation behavior be in this case?

@Adphi
Copy link
Member

Adphi commented Nov 25, 2025

Thanks @dsociative !

Can you please run git rebase <main repo remote name>/main and resolve the Cargo.toml and Cargo.lock conflicts ?
There shouldn’t be any changes in them.

I’ll check the failing test.

@Adphi
Copy link
Member

Adphi commented Nov 25, 2025

Thanks @dsociative !
The fix in #29 resolves the tests, so I’m closing this.

@Adphi Adphi closed this Nov 25, 2025
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.

2 participants