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

Use proto optional fields for non required primitive object attributes. #3162

Merged
merged 2 commits into from
Oct 9, 2022

Conversation

raphael
Copy link
Member

@raphael raphael commented Oct 9, 2022

This PR changes the proto files generated by Goa to take advantage of the optional keyword (re)introduced in proto 3.15.

In particular, this means that the generated transport data structure can make use of pointers for optional fields and therefore properly check for required fields.

Fixes #3159

for non required primitive object attributes.
@raphael raphael merged commit c1e1d75 into v3 Oct 9, 2022
@raphael raphael deleted the proto_optional_fields branch October 9, 2022 21:14
@MichaelUrman
Copy link
Contributor

Awesome update. Do you think it would be feasible and worthwhile to omit the 'optional' from fields that default to the zero value? That should avoid a pointer indirection in the pb struct and a branch in the goa server code that converts it.

@raphael
Copy link
Member Author

raphael commented Oct 12, 2022

Not sure I follow; would that be for fields that use Default in the design? If yes, then I believe we still want to specify them as optional and have Goa initialize them to their default value when unmarshalling if nil - that's been the behavior with HTTP.

@MichaelUrman
Copy link
Contributor

Yes. I've used Default(0) and Default(""), etc. to avoid pointers in the server type struct. I do this mostly for ergonomics in fields that I'm not ready to require, including that the server struct won't change when we switch from Default() to Required().

My code is still insulated, but with this PR the underlying pb field type becomes a *int or *string, etc., for no expressive or performance benefit. Fields excluded from the protobuf message can be unmarshaled as nil and replaced with their zero Default, or unmarshaled as zero and converted, with no difference in visible behavior.

@raphael
Copy link
Member Author

raphael commented Oct 12, 2022

Using Default to avoid pointers in the service layer makes sense, in a way the idea is that the user "shouldn't care" about the transport layer (obviously it's great to have contributors that do :) ). As described earlier the algorithm uses pointers at the transport layer for fields with default values so that it can check for nil and in that case initialize the field with the default value before passing it to the service layer (where it's not a pointer anymore). This happens for payloads server side and responses client side and has always been the behavior with HTTP. It seems to make sense to keep that consistent with the gRPC transport.

@MichaelUrman
Copy link
Contributor

Ok, thank you for considering. The symmetry with HTTP transports is nice, whereas I've used almost exclusively gRPC so I saw it as a change. I'll assume my performance concerns are premature until proven otherwise.

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.

gRPC: Support optional protobuf 3.15+ feature
2 participants