[Bug] Fix RpcContext compatibility between Object and String attachments #15934

CuSO41108 · 2025-12-28T09:04:15Z

What is the purpose of the change?

This PR fixes the compatibility issue in RpcContext where non-String objects stored via setObjectAttachment could not be retrieved via the legacy getAttachment(String) method.

Brief changelog

Modified RpcContext#setObjectAttachment to automatically store a String representation of the object for legacy API compatibility.
Addressed the TODO: "compatible with previous" in RpcContext.java.

Verifying this change

I have added a unit test RpcContextCompatibilityTest to verify that:

getObjectAttachment returns the original object (e.g., Long).
getAttachment returns the String representation (e.g., "12345") instead of null.

I have already sent my ICLA to [email protected]

Checklist

Make sure there is a GitHub_issue field for the change.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Write necessary unit-test to verify your logic correction. If the new feature or significant change is committed, please remember to add sample in dubbo samples project.
Make sure gitHub actions can pass. Why the workflow is failing and how to fix it?

…acy getAttachment API

Copilot

Pull request overview

This PR attempts to fix a compatibility issue in RpcContext where non-String objects stored via setObjectAttachment could not be retrieved via the legacy getAttachment(String) method. The fix addresses a TODO comment about maintaining backward compatibility with the legacy API.

Key changes:

Modified RpcContext#setObjectAttachment to store a String representation alongside non-String objects
Added unit test RpcContextCompatibilityTest to verify the compatibility behavior

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
dubbo-rpc/dubbo-rpc-api/src/main/java/org/apache/dubbo/rpc/RpcContext.java	Added logic to store String representation of non-String objects for legacy API compatibility
dubbo-rpc/dubbo-rpc-api/src/test/java/org/apache/dubbo/rpc/RpcContextCompatibilityTest.java	New test file to verify attachment compatibility between object and string APIs

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-28T09:14:21Z

dubbo-rpc/dubbo-rpc-api/src/test/java/org/apache/dubbo/rpc/RpcContextCompatibilityTest.java

+        Object legacyValue = context.getAttachment(key);
+        System.out.println("Legacy getAttachment result: " + legacyValue);


The test uses System.out.println for debugging instead of proper assertions. The test should verify that the legacy getAttachment method returns the expected String value "12345" using an assertion like Assertions.assertEquals("12345", legacyValue) to ensure the compatibility fix is working correctly.

Copilot · 2025-12-28T09:14:21Z

dubbo-rpc/dubbo-rpc-api/src/test/java/org/apache/dubbo/rpc/RpcContextCompatibilityTest.java

+        try {
+            for (Map.Entry<String, String> entry : allAttachments.entrySet()) {
+                String k = entry.getKey();
+                String v = entry.getValue(); // 如果 entry.getValue() 实际是 Long，这里可能会崩
+                System.out.println(k + " = " + v);
+            }
+        } catch (ClassCastException e) {
+            System.err.println("发现不兼容 Bug！由于底层存了非 String 对象，导致老接口遍历崩溃: " + e.getMessage());
+        }


The test relies on System.out.println instead of proper assertions. The try-catch block should assert that no ClassCastException occurs when iterating over attachments, and verify that the String representation exists in the map. Consider using Assertions.assertDoesNotThrow and validating the map contents.

Copilot · 2025-12-28T09:14:21Z

dubbo-rpc/dubbo-rpc-api/src/main/java/org/apache/dubbo/rpc/RpcContext.java

+        if (value != null && !(value instanceof String)) {
+            CLIENT_ATTACHMENT.get().setAttachment(key, String.valueOf(value));
+        }


This fix has a critical bug. When a non-String object is stored, the code first calls setObjectAttachment(key, value) which stores the object in the attachments map. Then it calls setAttachment(key, String.valueOf(value)), which delegates to setObjectAttachment and overwrites the original object with its String representation. This means getObjectAttachment(key) will return the String "12345" instead of the original Long object, breaking the intended behavior. The fix should store both representations without overwriting, or use a different storage mechanism.

codecov-commenter · 2025-12-28T09:28:13Z

Codecov Report

❌ Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 60.76%. Comparing base (6a70f6b) to head (c88c0fa).

Files with missing lines	Patch %	Lines
...src/main/java/org/apache/dubbo/rpc/RpcContext.java	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##                3.3   #15934      +/-   ##
============================================
+ Coverage     60.74%   60.76%   +0.02%     
+ Complexity    11713    11697      -16     
============================================
  Files          1938     1938              
  Lines         88698    88700       +2     
  Branches      13387    13388       +1     
============================================
+ Hits          53876    53900      +24     
+ Misses        29290    29272      -18     
+ Partials       5532     5528       -4

Flag	Coverage Δ
integration-tests-java21	`32.45% <0.00%> (+0.05%)`	⬆️
integration-tests-java8	`32.52% <50.00%> (-0.01%)`	⬇️
samples-tests-java21	`31.98% <66.66%> (-0.05%)`	⬇️
samples-tests-java8	`29.73% <66.66%> (+<0.01%)`	⬆️
unit-tests-java11	`59.06% <33.33%> (+0.03%)`	⬆️
unit-tests-java17	`58.52% <33.33%> (+<0.01%)`	⬆️
unit-tests-java21	`58.53% <33.33%> (+0.01%)`	⬆️
unit-tests-java25	`58.49% <33.33%> (+0.02%)`	⬆️
unit-tests-java8	`59.05% <33.33%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

wangchengming666

LGTM

RainYuY · 2026-01-06T09:40:17Z

I’m a bit concerned that this PR will alter the existing behavior. Additionally, it’s odd that when I set an Integer value as an attachment and retrieve it using getAttachment (instead of getStringAttachment), I end up getting a String.

CuSO41108 · 2026-01-06T13:59:25Z

I’m a bit concerned that this PR will alter the existing behavior. Additionally, it’s odd that when I set an Integer value as an attachment and retrieve it using getAttachment (instead of getStringAttachment), I end up getting a String.

Hi @RainYuY, thank you for your feedback!

Regarding your concern, this PR is specifically intended to address the // TODO: compatible with previous comment in RpcContext.java.

Currently, the legacy getAttachment(String key) method returns null or may cause type conflicts when a non-String object is stored via the new setObjectAttachment API. This breaks backward compatibility for users who still rely on the legacy String-based API but interact with services using the newer Object-based attachments.

By performing a "read-time conversion" in getAttachment:

Backward Compatibility: Legacy callers get a safe String representation instead of null.
Type Safety: The new getObjectAttachment still returns the original type (e.g., Integer/Long) without any loss of information, as the underlying storage remains untouched.
Consistency: It aligns the behavior of getAttachment with the existing AttachmentsAdapter.ObjectToStringMap logic.

Do you think we should maintain the current behavior (returning null for non-String objects in the legacy API), or is this dynamic conversion a more robust way to fulfill the "compatible with previous" TODO?

[RPC] Fix RpcContext.setObjectAttachment compatibility issue with leg…

2404f50

…acy getAttachment API

Copilot AI review requested due to automatic review settings December 28, 2025 09:04

Copilot started reviewing on behalf of CuSO41108 December 28, 2025 09:04 View session

Copilot AI reviewed Dec 28, 2025

View reviewed changes

CuSO41108 added 3 commits December 28, 2025 17:33

[RPC] Fix RpcContext compatibility and cleanup imports

d46d433

Merge branch '3.3' into fix-typo-demo

ec53537

[Test] Replace println with Assertions

c88c0fa

wangchengming666 approved these changes Jan 6, 2026

View reviewed changes

songxiaosheng added the status/wait for another approve label Jan 6, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Bug] Fix RpcContext compatibility between Object and String attachments #15934

[Bug] Fix RpcContext compatibility between Object and String attachments #15934

CuSO41108 commented Dec 28, 2025 •

edited by zrlw

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 28, 2025

Uh oh!

Copilot AI Dec 28, 2025

Uh oh!

Copilot AI Dec 28, 2025

Uh oh!

codecov-commenter commented Dec 28, 2025 •

edited

Loading

Uh oh!

wangchengming666 left a comment

Uh oh!

RainYuY commented Jan 6, 2026

Uh oh!

CuSO41108 commented Jan 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		Object legacyValue = context.getAttachment(key);
		System.out.println("Legacy getAttachment result: " + legacyValue);

[Bug] Fix RpcContext compatibility between Object and String attachments #15934

Are you sure you want to change the base?

[Bug] Fix RpcContext compatibility between Object and String attachments #15934

Conversation

CuSO41108 commented Dec 28, 2025 • edited by zrlw Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the purpose of the change?

Brief changelog

Verifying this change

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Dec 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

wangchengming666 left a comment

Choose a reason for hiding this comment

Uh oh!

RainYuY commented Jan 6, 2026

Uh oh!

CuSO41108 commented Jan 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CuSO41108 commented Dec 28, 2025 •

edited by zrlw

Loading

codecov-commenter commented Dec 28, 2025 •

edited

Loading