Skip to content

Conversation

@w01fgang
Copy link

@w01fgang w01fgang commented Sep 1, 2024

Add optional field decorator.

Fixes #4311

@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