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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
public static class ObjectToStringMap extends HashMap<String, String> { | ||
private Map<String, Object> attachments; | ||
|
||
@Deprecated |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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();
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
Now branch 3.0 is the default, if you want 3.0 need this change also, pick it to 3.0 |
This change has broken compatibility |
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 :( |
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. |
yep, the code has breaking change, It will influence the user who modify the attachments by operate |
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