Skip to content

add decorator for optional fields in protobuf emitter #4312

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

Conversation

w01fgang
Copy link

@w01fgang w01fgang commented Sep 1, 2024

Add optional field decorator.

Fixes #4311

Signed-off-by: Denis Sumin <sumin@unix-center.ru>
@microsoft-github-policy-service microsoft-github-policy-service bot added the stale Mark a PR that hasn't been recently updated and will be closed. label Dec 16, 2024
@witemple-msft
Copy link
Member

Hey @w01fgang, I'm sorry I haven't been able to catch up on this in a while. I've given this some thought, and I think we can just use TypeSpec's built-in optionality for this.

My understanding of the optional modifier is that it only controls the presence discipline of fields where the type is not a message. This modifier did not exist in the proto3 language guide when @typespec/protobuf was created, but it clearly does now. I think we can just use TypeSpec optionality to control the presence of this modifier and there won't be any risk to that, even though the concept of TypeSpec optionality and proto3 optional are not the same concept. The TypeSpec concept of optional is a stronger concept, but it makes sense to me that any field that is optional in TypeSpec should also be optional in Protobuf.

In the future, code generators that implement Protobuf from TypeSpec directly (we don't have plans for this currently, but it should be possible) could use TypeSpec optionality to do more extended validation of message-typed fields as well.

What do you think?

@witemple-msft witemple-msft removed the stale Mark a PR that hasn't been recently updated and will be closed. label Jan 9, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot added the stale Mark a PR that hasn't been recently updated and will be closed. label Feb 9, 2025
@Stanzilla
Copy link

Hey @w01fgang, I'm sorry I haven't been able to catch up on this in a while. I've given this some thought, and I think we can just use TypeSpec's built-in optionality for this.

My understanding of the optional modifier is that it only controls the presence discipline of fields where the type is not a message. This modifier did not exist in the proto3 language guide when @typespec/protobuf was created, but it clearly does now. I think we can just use TypeSpec optionality to control the presence of this modifier and there won't be any risk to that, even though the concept of TypeSpec optionality and proto3 optional are not the same concept. The TypeSpec concept of optional is a stronger concept, but it makes sense to me that any field that is optional in TypeSpec should also be optional in Protobuf.

In the future, code generators that implement Protobuf from TypeSpec directly (we don't have plans for this currently, but it should be possible) could use TypeSpec optionality to do more extended validation of message-typed fields as well.

What do you think?

Hey there, how would that look like, syntax wise?

@w01fgang
Copy link
Author

Hey @witemple-msft!
Sorry for the radio silence, I'm stuck in traveling mode for a few months already, writing from Bali 😅

The behaviour of optional keyword is librery dependant and doesn't always match between the protobuf and openapi.
Our use case of typespec is to define openapi and protobuf definitions in a single place.
We generate TypeScript definitions and Go clients as the final outcome of the single typespec definition.
Golang's library optional keyword is configurable and can create two cases:

  1. optional means that field is optional and can be omitted
  2. optional means that field is nullable and the library can assign default value depending on the type like 0 for int, false for boolean, and etc.
    So in the first case optional matches with field?: string and in the second case optional matches with field: string.
    I would add a setting to the config of the protobuf emitter to configure this behaviour.

I wasn't sure about all the cases that could be so I went with a "brute force" solution, just added a decorator for the optional keyword.
Adding optional as a setting in the emitter options would require less code and no breaking changes.

@Stanzilla
Copy link

@witemple-msft thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Mark a PR that hasn't been recently updated and will be closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature request] [protobuf] add optional keyword
3 participants