feat(http): optimize parameter parsing #5483
Conversation
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## release_v4.7.3 #5483 +/- ##
====================================================
+ Coverage 64.58% 64.62% +0.04%
Complexity 9684 9684
====================================================
Files 867 867
Lines 51726 51724 -2
Branches 5755 5754 -1
====================================================
+ Hits 33407 33428 +21
+ Misses 15714 15693 -21
+ Partials 2605 2603 -2
... and 8 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -535,7 +534,9 @@ private static String checkGetParam(HttpServletRequest request, String key) thro | |||
| if (StringUtils.isBlank(contentType)) { | |||
| return null; | |||
| } | |||
| if (APPLICATION_JSON.toLowerCase().contains(contentType)) { | |||
| if (contentType.contains(MimeTypes.Type.APPLICATION_JSON.asString()) | |||
There was a problem hiding this comment.
if contentType.contains(MimeTypes.Type.APPLICATION_JSON_UTF_8.asString()) == ture
then contentType.contains(MimeTypes.Type.APPLICATION_JSON.asString() must be true.
There was a problem hiding this comment.
The content-type will be true if the condition is met.
There was a problem hiding this comment.
I mean the last two rulings are not necessary, if you insist on doing this, all the scenes need to be added
|
@forfreeday Please add test for validation. |
| @@ -545,7 +546,7 @@ private static String checkGetParam(HttpServletRequest request, String key) thro | |||
| if (jsonObject != null) { | |||
| return jsonObject.getString(key); | |||
| } | |||
| } else if (APPLICATION_FORM_URLENCODED.toLowerCase().contains(contentType)) { | |||
| } else if (contentType.contains(MimeTypes.Type.FORM_ENCODED.asString())) { | |||
There was a problem hiding this comment.
Why reverse the contains()?
There was a problem hiding this comment.
The characteristics of String.contains() dictate that when using contains(), you need to pay attention to its their containment relationship; longer characters can contain shorter characters, and conversely, shorter characters cannot contain longer characters.
String str1 = "abc";
String str2 = "ab";
str1.contains(str2);is valid, the reverse is not.
The content-type of the request could be any of the following:
application/json;
application/json;charset=utf-8;
application/json; charset=utf-8;
So you need to compare the constants with the content-type of the request.
There was a problem hiding this comment.
charset=gb2312;application/JSON is also legal.
There was a problem hiding this comment.
Actually contentType.contains(MimeTypes.Type.APPLICATION_JSON.asString() is compatible with all scenarios, more judgment seems redundant
| @@ -78,8 +79,6 @@ public class Util { | |||
| public static final String FUNCTION_SELECTOR = "function_selector"; | |||
| public static final String FUNCTION_PARAMETER = "parameter"; | |||
| public static final String CALL_DATA = "data"; | |||
| public static final String APPLICATION_FORM_URLENCODED = "application/x-www-form-urlencoded"; | |||
| public static final String APPLICATION_JSON = "application/json"; | |||
|
|
|||
| public static String printTransactionFee(String transactionFee) { | |||
There was a problem hiding this comment.
Better add unit test
| @@ -535,7 +534,9 @@ private static String checkGetParam(HttpServletRequest request, String key) thro | |||
| if (StringUtils.isBlank(contentType)) { | |||
| return null; | |||
| } | |||
| if (APPLICATION_JSON.toLowerCase().contains(contentType)) { | |||
There was a problem hiding this comment.
Should the contentType to lower case too ?
There was a problem hiding this comment.
defaults to lowercase.
halibobo1205
added
area:http
topic: gRPC/HTTP api
halibobo1205
changed the title
| @@ -535,8 +533,10 @@ private static String checkGetParam(HttpServletRequest request, String key) thro | |||
| if (StringUtils.isBlank(contentType)) { | |||
| return null; | |||
| } | |||
| if (APPLICATION_JSON.toLowerCase().contains(contentType)) { | |||
| value = getRequestValue(request); | |||
| if (contentType.contains(MimeTypes.Type.FORM_ENCODED.asString())) { | |||
There was a problem hiding this comment.
Shall we add some explanation about this change?
What does this PR do?
optimize incorrect content-type comparisons.
Why are these changes required?
When a client requests an interface, if it is an application/json request, it may be written in a variety of ways, including:
application/jsonapplication/json;charset=iso-8859-1application/json;charset=utf-8When determining the content-type of a request, the interface needs to be compatible with the variable writing ways of the requests.
This PR has been tested by:
Follow up
Extra details