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

feat(genaip): add support for Protobuf Editions #328

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

Conversation

vallahaye
Copy link
Contributor

Hello again 👋

These changes are enough to add support for Protobuf Editions (and proto3 optional fields) in protoc-gen-aip-go.

Let me know if this works for you and if I should add simple tests (although it doesn't affect the codegen).

Closes #327

@vallahaye vallahaye requested a review from a team as a code owner January 26, 2025 18:52
@vallahaye vallahaye force-pushed the 327-add-support-for-protobuf-editions branch from 7986460 to 5300659 Compare January 26, 2025 18:55
@vallahaye vallahaye marked this pull request as draft January 26, 2025 18:55
@vallahaye vallahaye marked this pull request as ready for review January 26, 2025 18:57
Copy link
Contributor

@alethenorio alethenorio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi

Thank you for the PR. Can you detail the use case for introducing this change?

While I agree that it would be nice to have support for editions for a more future-safe setup, the AIP syntax spec explicitly states that APIs should be written in proto3 format so maybe we should start there and get a better understanding on whether there are any plans supporting editions?

@vallahaye
Copy link
Contributor Author

vallahaye commented Jan 27, 2025

Hello @alethenorio

Thank you for the quick reply. To be totally honest, I overlooked that part of the spec and, after reviewing it, you're absolutely right:

APIs defined in protocol buffers must use proto3 syntax.

So, at the bare minimum, the changes I committed should not enable proto2 in protoc-gen-aip-go. Regarding Editions, I did a little research on the AIPs' GitHub repository and couldn't find anything mentioning upcoming support or the opposite. No issue, no PR, no discussion, nothing.

In summary:

  • proto2 should not be supported (I'll bump the SupportedEditionsMinimum property in this regard).
  • proto3 optional fields, although not explicitly mentioned, should be supported in my opinion. I found one Google Cloud specific AIP making use of the keyword.
  • It's up to you to decide if you want to deviate a bit from the specification and support Editions now for the sake of future compatibility. If it were me, I'd do it, as I can definitely see new projects embracing the Connect RPCs stack, Protobuf Editions (which is now mature) to later discover AIPs and get stuck with protoc-gen-aip-go not being able to handle their Protobuf files.

Protobuf Editions introduces a new feature flag system aimed at unifying proto2 and
proto3 functionality and semantics.

Closes einride#327
@vallahaye vallahaye force-pushed the 327-add-support-for-protobuf-editions branch from 5300659 to 1a24723 Compare January 27, 2025 13:09
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.

Add support for Protobuf Editions
2 participants