Repository navigation
descriptor: reject a field or extension typed as a map entry it does not own - #720
Open
veljjanoski wants to merge 4 commits into
Open
veljjanoski wants to merge 4 commits into
veljjanoski wants to merge 4 commits into
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
… expected entry An extension whose type is a map entry now fails with PoolError::ExtensionWithMapEntryType, whatever its shape. protoc accepts one, a repeated extension of the entry's containing message that is named for the entry, and treats it as a map. The pool has no map extension, linking it as a list left the entry's own rules unchecked, and the code buffa-codegen emits for it names an entry struct that is not generated. InvalidMapEntryUse gains expected_entry, the full name a map field of that name requires of its entry, and its Display text gives the remedy for the case at hand: the field's shape when the name is right, the entry's name otherwise. The docs say which rules are stricter than protoc 33.5 (the extension, and a repeated group named for the entry), map_entry_name cites both protoc functions it mirrors, and the fragment names the one shape that linked as a working map before.
iainmcgin
reviewed
Oct 16, 2026
iainmcgin
left a comment
Collaborator
There was a problem hiding this comment.
[claude code] Thanks. The field rules are a faithful port of protoc, the pass-1 options change holds on every path traced, and the no_std fix is in. Pushed one maintainer commit on top (a2bc0a0), with one maintainer decision and the review fixes:
- Every extension whose type is a map entry is rejected, with the new
PoolError::ExtensionWithMapEntryType, including the one shape protoc compiles. The pool has no map extension, linking it as a list left the entry shape unchecked, and the codebuffa-codegenemits for it names an entry struct that is not generated. The docs name this as a rule stricter than protoc. InvalidMapEntryUsegainsexpected_entry, the full name a map field of that name requires of its entry. ItsDisplaytext gives the remedy for the case at hand: the field shape when the name is already right, the entry name otherwise. Both texts are asserted in tests.- Parity wording: the line numbers in the description are from protobuf
main. At v33.5,ValidateMapEntry(descriptor.cc:9135) has no group rule, so a repeated group named for the entry is accepted there. The pool still rejects it, and the variant doc says so. - Tests: the extension cases are one table of seven shapes, through
newanddecode; a rejection for a name that differs only in case;a_1bandFOO_BARnaming cases; the linked entry keeps its options. - The fragment is rewritten: the exact naming rule, the one shape that linked as a working map before and what to do about it, and the extension rule.
cargo test --workspace passes on this head (4279 passed, 0 failed), as do the no_std check and tests. The description will be updated to match.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #637.
Problem
find_map_entrytreated a repeated message field as a map when its type was amap_entrymessage nested in the same message, without comparing the entry's name with the field's. Every field that failed that test linked as an ordinary message field. So a second repeated field of an entry's type linked as a second map, a field of another message (or a singular field) of an entry type linked as a message field, and so did an extension of an entry type.Fix
This follows protoc's checks (
descriptor.cc:7915-7965at90229ff): a group may not have a map entry type, and a message field of a map entry type must be repeated, must share the entry's containing message, and must be named so thatToCamelCase(name, false)followed byEntryis the entry's name.find_map_entryalso requires that name. The new privatemap_entry_nameis a port of it.link_fieldreturns the newPoolError::InvalidMapEntryUse { field, entry }when the resolved message type hasmap_entry = trueandfind_map_entrydid not accept the field.link_extensionapplies the same rule to an extension, with the extendee as its message. protoc's check accepts exactly one shape there: protoc 33.5 compilesextend Holder { repeated Holder.EntriesEntry entries = 100; }for aHolderwithmap<string, int32> entries, and rejects a singular one. That one shape still links, as a list of entries, as it did before (the map's wire form). Every other extension of a map entry type is rejected. protobuf-go rejects that shape too (desc_validate.go:254-256); if you would rather follow it, the check is one condition.take(), so it is still one clone per message). Pass 2 links a message's fields before its nested messages, and can link another message's field before the entry, so without thislink_fieldwould see no options for the entry. With only this part reverted, 6 of the 7 rejection tests below fail (the extension one passes, since extensions link in pass 3).tests/dynamic_e2e.rs: the hand-built maps inmap_wire_type_poolused fieldsf1,f2, … with entriesE1,E2, …, which protoc rejects. The entries are nowF1Entry,F2Entry, …; the test is about wire types and is otherwise unchanged.Net change outside tests:
pool.rs+109/-13, including the new variant's rustdoc andDisplaytext and the comments.Not changed here: codegen's own
find_map_entry(buffa-codegen/src/message.rs) does not check the entry name either, so for a hand-built set with a misnamed entry codegen still emits a map field. protoc output cannot reach this.Tests
New in
tests/map_entry_fields.rs. Each rejection is checked through bothDescriptorPool::newandDescriptorPool::decode:string_to_int,_leading,double__under,trailing_,v2_items,already_UpperWith both checks and the name test disabled, all 7 rejection tests fail; with the fix, all 16 tests in the file pass.
Run locally on Windows (
x86_64-pc-windows-gnu, toolchain 1.95.0, protoc 33.5):cargo fmt --all --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspace: all pass except twobuffa-typestests that fail on Windows only, becauseSystemTimethere has 100 ns resolutioncargo test -p buffa-descriptor --all-featurestask doc,task test-arbitrary, markdownlinttask conformance(Docker, tools image built locally withtask tools-image-local): all 16 suites passed, 0 unexpected failuresAI-usage disclosure: Claude (Anthropic) was used as assistant.