Skip to content
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

Use MapUtils instead of AttachmentsAdapter #8772

Merged
merged 8 commits into from Sep 14, 2021

Conversation

manzhizhen
Copy link
Contributor

@manzhizhen manzhizhen commented Sep 11, 2021

What is the purpose of the change

In order to realize the conversion from Map<String, Object> to Map<String, String>, the implementation of AttachmentsAdapter is too complicated, and the inner class of the class is also used. For the sake of simplicity, the tool class MapUtils with less code is used to achieve the same function.

为了实现Map<String, Object>到Map<String, String>的转换,AttachmentsAdapter实现的太复杂,还使用类内部类。为了简单起见,使用更少代码的工具类MapUtils来实现一样的功能。

Brief changelog

Verifying this change

Checklist

  • Make sure there is a GitHub_issue field for the change (usually before you start working on it). Trivial changes like typos do not require a GitHub issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Check if is necessary to patch to Dubbo 3 if you are work on Dubbo 2.7
  • Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add sample in dubbo samples project.
  • Add some description to dubbo-website project if you are requesting to add a feature.
  • GitHub Actions works fine on your own branch.
  • If this contribution is large, please follow the Software Donation Guide.

@manzhizhen manzhizhen changed the title Rpccontext opt Use MapUtils instead of AttachmentsAdapter Sep 11, 2021
@codecov-commenter
Copy link

@codecov-commenter codecov-commenter commented Sep 11, 2021

Codecov Report

Merging #8772 (49a3b06) into master (abb2109) will decrease coverage by 0.63%.
The diff coverage is 57.89%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #8772      +/-   ##
============================================
- Coverage     61.04%   60.40%   -0.64%     
+ Complexity      448      447       -1     
============================================
  Files          1097     1100       +3     
  Lines         44383    44422      +39     
  Branches       6460     6469       +9     
============================================
- Hits          27093    26835     -258     
- Misses        14309    14605     +296     
- Partials       2981     2982       +1     
Impacted Files Coverage Δ
...rc/main/java/org/apache/dubbo/rpc/AppResponse.java 55.69% <0.00%> (ø)
.../java/org/apache/dubbo/rpc/AttachmentsAdapter.java 0.00% <0.00%> (-50.00%) ⬇️
...src/main/java/org/apache/dubbo/rpc/RpcContext.java 66.33% <0.00%> (ø)
...n/java/org/apache/dubbo/common/utils/MapUtils.java 76.92% <76.92%> (ø)
.../main/java/org/apache/dubbo/rpc/RpcInvocation.java 64.73% <100.00%> (ø)
...gcenter/wrapper/CompositeDynamicConfiguration.java 0.00% <0.00%> (-73.34%) ⬇️
...ookeeper/ZookeeperDynamicConfigurationFactory.java 0.00% <0.00%> (-66.67%) ⬇️
...pport/zookeeper/ZookeeperDynamicConfiguration.java 0.00% <0.00%> (-62.86%) ⬇️
...fig/configcenter/TreePathDynamicConfiguration.java 31.42% <0.00%> (-54.29%) ⬇️
.../dubbo/metadata/report/MetadataReportInstance.java 12.50% <0.00%> (-40.63%) ⬇️
... and 49 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abb2109...49a3b06. Read the comment docs.

public static class ObjectToStringMap extends HashMap<String, String> {
private Map<String, Object> attachments;

@Deprecated
Copy link
Member

@horizonzy horizonzy Sep 12, 2021

Choose a reason for hiding this comment

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

this file method convert has problem, if object type is not string, it will return null, cound you fix it conveniently

Copy link
Contributor Author

@manzhizhen manzhizhen Sep 12, 2021

Choose a reason for hiding this comment

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

Hmm, the new method takes this into consideration, calling Object#toString() directly。Using JSON here may not be the best choice.

/**
 * use {@link Object#toString()} switch Obj to String
 *
 * @param obj
 * @return
 */
private static String convertToString(Object obj) {
    if (obj == null) {
        return null;
    } else {
        return obj.toString();
    }
}

@manzhizhen manzhizhen requested a review from horizonzy Sep 12, 2021
Copy link
Member

@horizonzy horizonzy left a comment

lgtm.

@horizonzy horizonzy added this to the 2.7.14 milestone Sep 13, 2021
@horizonzy horizonzy merged commit f8f6995 into apache:master Sep 14, 2021
10 checks passed
@horizonzy
Copy link
Member

@horizonzy horizonzy commented Sep 14, 2021

Now branch 3.0 is the default, if you want 3.0 need this change also, pick it to 3.0

@wuwen5
Copy link
Contributor

@wuwen5 wuwen5 commented Jan 12, 2022

This change has broken compatibility ☹️

@wu-sheng
Copy link
Member

@wu-sheng wu-sheng commented Jan 12, 2022

We are surprised to receive a compatibility bug due to a minor version change, and include an implementation level change.

I am following changes at apache/skywalking-java#88, but it is better Dubbo community could provide some suggestions. And hope this kind of change would not be in patch version in the future :(

@wuwen5
Copy link
Contributor

@wuwen5 wuwen5 commented Jan 12, 2022

openzipkin/brave is also affected

Map<String, String> attachments = RpcContext.getContext().getAttachments();
DubboClientRequest clientRequest = new DubboClientRequest(invoker, invocation, attachments);

If the user's business code is used, it may be fatal.

@chickenlj @horizonzy @CrazyHZM @manzhizhen

@horizonzy
Copy link
Member

@horizonzy horizonzy commented Jan 12, 2022

openzipkin/brave is also affected

Map<String, String> attachments = RpcContext.getContext().getAttachments();
DubboClientRequest clientRequest = new DubboClientRequest(invoker, invocation, attachments);

If the user's business code is used, it may be fatal.

@chickenlj @horizonzy @CrazyHZM @manzhizhen

yep, the code has breaking change, It will influence the user who modify the attachments by operate RpcContext.getContext().getAttachments().
It's a problem indeed, it shouldn't change original logic.
sorry for my careless review.

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.

None yet

5 participants