Skip to content

feat: add Edition 2023 Support #2051

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

Merged
merged 12 commits into from
Apr 14, 2025
Merged

Conversation

mkruskal-google
Copy link
Contributor

@mkruskal-google mkruskal-google commented Mar 21, 2025

Protobuf.js was audited for the following features:

  • Field presence - Full support of all 3 presence modes, although the behavior appears to be non-conformant in general. The feature values were hooked up to the existing behaviors, with new tests added for edition 2023.
  • Packed encoding - Both packed and expanded encoding are supported and conformant. Some opportunistic cleanup was done, but was behavior-preserving.
  • Delimited encoding - Proto2 groups are supported but mark a property on the message rather than the field. This information was moved to the field and the more general delimited logic was added (with tests).
  • Enum type - All enums are open, this was left untouched. CLOSED enum_type will be ignored in edition 2023 (which is consistent with our long-term goal anyway).
  • UTF8 validation - There doesn't seem to be any explicit validation of utf8, beyond what javascript may provide by default. This would treat proto2/proto3 and edition 2023 the same regardless of features.

The proto target CLI appeared to be broken in a number of edge cases related to editions features, so these were fixed and tested were added. It now supports proto2 and proto3 roundtrips, and conversions to proto2/proto3 (even from edition 2023) as long as they're valid. No extra validation layer was added, so impossible conversions will just produce invalid protos. A new target for edition 2023 wasn't added, but could be pretty easily if necessary in the future.

One major design flaw in protobuf.js, that's common in proto3-based code, is that the concept of files was largely dropped. The in-memory and JSON representation of descriptors puts all of the file metadata into the most-nested package namespace object, but this object may be shared by multiple files. This results in possible clobbering and merging, leading to malformed file metadata. In proto2/proto3, this largely doesn't matter outside of custom file options, but the effects can be seen in the hack that forces packed = false for proto2 descriptors to get the repeated field encoding correct. This problem was made worse in editions, where we store important metadata at the file level (edition + features). The solution implemented here collapses all of this information into the top-level objects instead of putting it on the namespace. This results in some minimal duplication, but has the ability to properly support proto2, proto3, and editions in addition to fixing some pre-existing bugs (demonstrated by some newly added tests).

Other changes:

  • Group decoding will now look for the proper matching end-group tag instead of any end-group tag
  • A bug in feature resolution for extensions was fixed
  • A bug in how unresolved features showed up in the options struct was fixed
  • Standard reflection helpers were added that are compatible with editions and proto2/proto3
  • Proto2 and proto3 are now treated as special editions (with full feature resolution). This substantially increases test coverage of all of the new editions logic
  • The JSON spec now fully supports proto2 descriptors, instead of the previous hack of forcing packed=false to make them behave properly

Migration Guide

  • Code must always ensure .resolveAll() is called on dynamic built protos. This is needed when building from constructors directly or individual fromJSON/addJSON methods. Top-level builders like Root.fromJSON, Root.load, and protobuf.parse do not need to explicitly call this

BEGIN_COMMIT_OVERRIDE
feat: add Edition 2023 Support

END_COMMIT_OVERRIDE

This will now work as long as all files sharing a package agree on file-level edition and options
@sofisl sofisl changed the title Add Edition 2023 Support feat!: add Edition 2023 Support Mar 28, 2025
Copy link
Contributor

@sofisl sofisl left a comment

Choose a reason for hiding this comment

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

Mostly questions to determine whether something is breaking but seems mostly not.

@sofisl
Copy link
Contributor

sofisl commented Apr 3, 2025

@dcodeIO can you take a look at this PR? Debating whether this should be a breaking change or not - would love your input, thanks!

@sofisl sofisl changed the title feat!: add Edition 2023 Support feat: add Edition 2023 Support Apr 14, 2025
@sofisl sofisl merged commit f04ded3 into protobufjs:master Apr 14, 2025
6 checks passed
@Tofandel
Copy link

Tofandel commented Apr 24, 2025

This broke presence detection for proto3 syntax, proto3 syntax only allows optional keyword, this means that anything not explicitely optional was semantically required in v7.4.0, but with this change everything is now considered optional and the keyword has no effect which breaks typescript definitions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants