Skip to content

Add option to generate proto3 optionals as swift optionals #1793

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TheMerski
Copy link

Motivation

As discussed in #1114 the swift-protobuf generator does not generate swift optionals. Although I feel this makes complete sense from an wholistic view of an protobuf generator it does introduce tradeoffs mainly when using code generated for proto3.

Our main usage of swift-protobuf is done in combination with gRPC, which recommends using proto3 for describing the API.
Within our API Specifications we rely on marking fields as optional to be able to communicate to the client when to ignore/not handle specific fields because they are not present.

Not having generated swift optionals is not ideal since it's easy for an developer to forget checking the hazzer method that is generated which can cause bugs because the client than loses the information if the fields was never set and thus incorrectly interpreting the default value of an message as the "set" value.

As an example we have an message that includes an title and optional subtitle to display in the client

message Foo {
  string title = 1;
  optional string subtitle = 2;
}

Mapping these to domain models in Swift requires checking the has functions that are generated:

init(_ foo: Client.Foo) {
        self.init(
            title: foo.title,
            subtitle: foo.hasSubtitle ? foo.subtitle : nil,
        )
    }

Not checking the hasSubtitle method would not result in an compiler error since accessing foo.subtitle will return de default (empty string) value, which could cause the client to allocate space to display an empty string instead of not allocating space at all. This issue becomes more apparent with larger and more complex models returned by the API.

Thoughts behind the change

  • While it does not make sense to generate Swift optionals by default I feel it would be nice to be able to have an option to generate swift optionals for proto3 optionals so this change adds the Proto3OptionalAsSwiftOptional option that will enable generating swift optionals only for proto3 optionals.
    • This will give users the ability to decide for themselves wether they want to accept the complications that come with having swift optionals generated.
  • I decided to not touch the hazzer methods to enable a more seamless transition for existing codebases to use or stop using generated swift optionals.

The deprecated descriptor.hasOptionalKeyword api had to be used to make sure swift optionals are only generated for messages explicitly marked as optional in the proto file.

It would be great to know if this change is something that can be included here or if there are objections with introducing this option and we should look at other ways to achieve this.

@thomasvl
Copy link
Collaborator

fyi - I've reached out to the protobuf-team about the grpc page; I didn't think the protobuf-team really wants to encourage proto3 over editions going forward, so hopefully that page will be updated in the future.

As far as generating differently based on the syntax/edition vs. on features - I think this is a likely not a good choice. Within google, they have been converting files to editions syntax while preserving the generation behavior via features as a path to then migrate code to updated api. So rolling out something knew that forces the choice of specific editions/syntaxes seems likely to be the opposite direction from what the protobuf-team would be encouraging for the ecosystem.

@TheMerski
Copy link
Author

fyi - I've reached out to the protobuf-team about the grpc page; I didn't think the protobuf-team really wants to encourage proto3 over editions going forward, so hopefully that page will be updated in the future.

As far as generating differently based on the syntax/edition vs. on features - I think this is a likely not a good choice. Within google, they have been converting files to editions syntax while preserving the generation behavior via features as a path to then migrate code to updated api. So rolling out something knew that forces the choice of specific editions/syntaxes seems likely to be the opposite direction from what the protobuf-team would be encouraging for the ecosystem.

First of all thanks a lot for taking the time to take a look and respond!
I have taken a bit of time to dive into protobuf editions to see if this change could be updated to also (or only) support editions. However I feel like i've hit a roadblock with protobuf editions which would also prevent us from adopting it in our gRPC api's.

With proto3 the optional keyword was also allowed on message type fields even though this would not result in a difference for presence tracking it allows for communicating to the API clients & devs which messages they can expect to not be present and which they shouldn't expect. Something I think is important for communicating about API specifications specifically.

With protobuf editions this option is lost so all messages should either be generated as an optional or no messages should be generated as an optional. With the added complexity that the client devs now don't have any way of knowing wether they can use the "default" value when accessing a message or wether they should map the hazzer to a nullable domain object.

In my opinion it feels like proto3 currently serves the need of an API description better than editions does, so I would be interested wether gRPC would encourage using editions over proto3.

For this change specifically the only option I see to make this compatible with the new editions syntax is by generating swift optionals for all messages, this would however introduce a change in generated code when moving from proto3 to editions, so I am not sure wether that would be the best approach. And not taking the optional keyword for messages into account on proto3 will greatly reduce the usability of this change.

Although I fully understand if you don't want to support this at this time, I would love to know your view on this change specifically, and whether it would be an option to merge and support this, either as is, or with changes made to support editions.

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