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

[AIP-203] Clarify that oneof fields should not be annotated with field_behavior REQUIRED #299

Closed
noahdietz opened this issue Sep 17, 2019 · 3 comments

Comments

@noahdietz
Copy link
Collaborator

I have seen this a few times where API producers add (or request one be added in review) a REQUIRED field_behavior annotation to a oneof field. While it is straight forward to deduce that a oneof field should not be REQUIRED, the effect the annotation has on downstream tooling is not necessarily known, thus the AIP should be explicit.

@FyiurAmron
Copy link

FyiurAmron commented Jun 22, 2023

@noahdietz @lukesneeringer TBH, I'm not sure that is really the case. oneof implies the optional code generation etc., as it may be not set at all, but the actual contract may require exactly one of the values to be present (with the default contract providing just "at most one" guarantee). As such, while it is mostly equal to the 3.15-ish optional keyword in this matter, it still doesn't cover the expectation vs semantic and behavior. As such, IMVHO, it's completely reasonable to expect a REQUIRED oneof (exactly one) as well as OPTIONAL oneof (at most one, i.e. 0 or 1). Remember, that by default all the fields are loosely "optional" in proto3.

I would expect the AIP to be amended to explicitly cover this case, or, even better - cover both the optional and oneof keywords and how they should be handled w.r.t. field_behavior. My gut feeling is that those concepts are somewhat orthogonal nowadays, but I'm not an expert or language lawyer here :D

@noahdietz
Copy link
Collaborator Author

Yeah this issue is quite out of date at this point. We actually have a few scenarios where we have field_behavior = REQUIRED allowed in oneof if all fields are annotated as such (for that scenario where least one should be set). But after more discussion we are not sure if this is really a good approach and we've considered adding a new extension or FieldBehavior value to express the semantic requirements aside from the wire format. We don't have a good answer yet.

The keyword optional in proto3 is a different case, but related. It's easier to answer that one. I've already got #1143 open to differentiate proto3_optional and field_behavior annotations.

oneof is still a bit of a thorny one. I think this issue doesn't really make sense any more as is, so I will open another to track the discussion. Thanks @FyiurAmron for reigniting this. We should probably clean up the issues in here 😅

@noahdietz
Copy link
Collaborator Author

noahdietz commented Jun 22, 2023

See #1147

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

No branches or pull requests

3 participants