Skip to content

[Java] Improve ProtoCoder decode error for raw Message type - #40904

Open
Brijesh-Thakkar wants to merge 1 commit into
apache:masterfrom
Brijesh-Thakkar:fix-19713-protocoder-raw-message
Open

Brijesh-Thakkar wants to merge 1 commit into
apache:masterfrom
Brijesh-Thakkar:fix-19713-protocoder-raw-message

Conversation

@Brijesh-Thakkar

Copy link
Copy Markdown

Addresses #19713.

ProtoCoder stores only the message class and finds its Parser lazily through getDefaultInstance(). For a generic T extends Message the type is erased to the com.google.protobuf.Message interface, which has no getDefaultInstance(). A coder for it can never decode, and the failure on the worker was a bare NoSuchMethodException: com.google.protobuf.Message.getDefaultInstance(), as reported in the issue.

The concrete type is not recoverable at decode time (the wire format carries no type information), so this PR does not try to make that coder work and does not change what succeeds or fails. It only makes the failure understandable: ProtoCoder.getParser() now throws an IllegalArgumentException (the same exception type as before, with the original NoSuchMethodException as the cause) saying why the parser cannot be found and to use a concrete generated message class, DynamicProtoCoder, or setCoder(). The existing DynamicMessage path is untouched.

Earlier attempts (#34194, #37005) added an instantiation fallback. That cannot work for an interface, and their tests did not fail before the change. The decode test here fails on master (old message, no guidance) and passes with the change.

Behaviour change: none beyond the error message. Encoding with a raw-Message coder still works and is covered by a test.

Testing:

  • ./gradlew :sdks:java:extensions:protobuf:spotlessCheck :sdks:java:extensions:protobuf:test : 196 tests, all pass.
  • testDecodeWithRawMessageClass fails on master (old NoSuchMethodException message), passes with the change. testEncodeWithRawMessageClass passes before and after.
  • GCP IO (users of ProtoCoder): PubsubIOTest, PubsubUnboundedSinkTest, PubsubMessageWith*CoderTest, BigQueryIOStorageReadTest : 133 tests, all pass.
  • CHANGES.md updated under Bugfixes.

Possible follow-up (for the maintainer to decide)

Coder inference (CoderRegistry) still returns a ProtoCoder<Message> silently for the raw Message type, so the failure only appears at decode time on the worker. A small follow-up would make ProtoCoderProvider.coderFor throw CannotProvideCoderException for it, so the failure moves to pipeline construction (test: testRegistryRejectsRawMessageType). I held it back because it is a behaviour change: a pipeline that relied on inference for an erased T extends Message and only ever encoded (never decoded, shuffled or checkpointed) works today and would then need an explicit setCoder(). No Beam code constructs a raw-Message coder (grep of main and test code), and the protobuf and GCP IO tests pass with it. It is ready as a separate local commit if wanted.

🤖 Generated with Claude Code

…9713)

A ProtoCoder for the raw com.google.protobuf.Message interface (for
example inferred from a generic T extends Message) cannot find a Parser,
and failed with a bare NoSuchMethodException on the worker. Wrap it in an
IllegalArgumentException that explains the cause and suggests a concrete
message class, DynamicProtoCoder, or setCoder().

No behaviour change: the same inputs fail, with a clearer message.
Encoding with such a coder is unchanged and covered by a test.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 30, 2026 16:20

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📋 Review Summary

This PR improves the error message in ProtoCoder.getParser() when a NoSuchMethodException occurs while looking up getDefaultInstance() (such as when a ProtoCoder is created for the raw com.google.protobuf.Message interface due to type erasure). It preserves existing behavior, exception types, and causes while giving actionable guidance to users.

🔍 General Feedback

  • The implementation is clean, minimal, and backwards-compatible (preserving IllegalArgumentException with the underlying NoSuchMethodException as its cause).
  • Unit tests cover both the improved error message on decode and the existing behavior on encode.
  • Regarding the follow-up mentioned in the PR description about rejecting raw Message.class in ProtoCoderProvider.coderFor: keeping this PR scoped to the decode error message is a safe choice, and rejecting raw Message / abstract classes during coder inference in ProtoCoderProvider would indeed be a good candidate for a separate follow-up discussion/PR.

💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.

@github-actions

Copy link
Copy Markdown
Contributor

Assigning reviewers:

R: @damccorm for label java.
Backup reviewer: Abacn (if the main reviewer is unresponsive, you can escalate to them by tagging their username)

Note: If you would like to opt out of this review, comment assign to next reviewer.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

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.

2 participants