Skip to content

text: reject unknown fields by default - #371

Open
fallintoplace wants to merge 1 commit into
anthropics:mainfrom
fallintoplace:fix/textproto-strict-unknown-fields
Open

text: reject unknown fields by default#371
fallintoplace wants to merge 1 commit into
anthropics:mainfrom
fallintoplace:fix/textproto-strict-unknown-fields

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

What changed

  • Generated textproto parsers now return UnknownField for unknown field names.
  • Nested messages, map entries, the built-in Any parser, and empty messages use the same strict behavior.
  • Regenerated the checked-in WKT and descriptor output.
  • Added a changelog entry and regression tests.

Why

  • A typo like user_nmae was previously ignored.
  • This matches the default behavior of upstream protobuf text parsers.
  • Hand-written TextFormat implementations can still call skip_value() when lenient parsing is wanted.

Checks

  • task lint
  • task doc
  • task test

@github-actions

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@iainmcgin

Copy link
Copy Markdown
Collaborator

[claude code]

Thanks for this. The direction matches upstream, and I'd like to take it for the next release with a bold changelog line, but CI's conformance job shows a contract the strict default breaks: 40 Required.*.TextFormatInput.ReservedFieldName.* cases fail with "Failed to parse input or produce output" (run: https://github.com/anthropics/buffa/actions/runs/32824819559/job/99136761718). Those tests feed a field name the message declares as reserved "..." and expect the parser to accept and discard it, which is what the C++ TextFormat::Parser does (it warns and skips a reserved name rather than erroring). So the rule upstream implements is "unknown names are an error, reserved names are skipped", and this PR implements only the first half.

The fix belongs in codegen: the message descriptor carries reserved_name, so the generated parser's name match can emit one arm per reserved name that calls dec.skip_value() before the catch-all UnknownField arm. The Any and map-entry parsers have no reserved names, so they need nothing. A regression test in buffa-test/src/tests/textproto.rs on a message with a reserved "old_name"; declaration would pin it, and task conformance (Docker) or task conformance-local reproduces the suite locally.

Two smaller things while you're in there: the changelog fragment should say explicitly that reserved names are still skipped, and please keep skip_value() public since hand-written TextFormat impls rely on it (the fragment already says so, just confirming that stays).

@fallintoplace

Copy link
Copy Markdown
Contributor Author

@iainmcgin Thanks, let me fix this one.

@fallintoplace
fallintoplace force-pushed the fix/textproto-strict-unknown-fields branch from 88876f1 to c3d71e6 Compare August 29, 2026 18:11
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