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.

Sign up for GitHub

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

Jump to bottom

feat(http): optimize parameter parsing #5483

Merged
merged 6 commits into from
Sep 14, 2023
Merged

feat(http): optimize parameter parsing #5483

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
48585eb
feat(Util): fix util
Sep 12, 2023
be8f186
feat(Util): optimize judgment logic
Sep 13, 2023
e6a5c68
test(case): add GetRewardServletTest and GetBrokerageServletTest test…
Sep 13, 2023
5292497
Merge branch 'release_v4.7.3' into feature/fix_utils
Sep 13, 2023
a241aa3
test(case): fix code
Sep 13, 2023
54884dc
feature(Util): remove 'content-type' judgment
Sep 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions framework/src/main/java/org/tron/core/services/http/Util.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.apache.commons.lang3.StringUtils;
import org.bouncycastle.util.encoders.Hex;
import org.eclipse.jetty.http.HttpMethod;
import org.eclipse.jetty.http.MimeTypes;
import org.eclipse.jetty.util.MultiMap;
import org.eclipse.jetty.util.StringUtil;
import org.eclipse.jetty.util.UrlEncoded;
Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Better add unit test

JSONObject jsonObject = new JSONObject();
Expand Down Expand Up @@ -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)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the contentType to lower case too ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

defaults to lowercase.

if (contentType.contains(MimeTypes.Type.APPLICATION_JSON.asString())
Copy link
Contributor

@halibobo1205 halibobo1205 Sep 13, 2023 •
edited by forfreeday
Loading

Choose a reason for hiding this comment

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

if contentType.contains(MimeTypes.Type.APPLICATION_JSON_UTF_8.asString()) == ture
then contentType.contains(MimeTypes.Type.APPLICATION_JSON.asString() must be true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The content-type will be true if the condition is met.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean the last two rulings are not necessary, if you insist on doing this, all the scenes need to be added

|| contentType.contains(MimeTypes.Type.APPLICATION_JSON_UTF_8.asString())
|| contentType.contains(MimeTypes.Type.APPLICATION_JSON_8859_1.asString())) {
value = getRequestValue(request);
if (StringUtils.isBlank(value)) {
return null;
Expand All @@ -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())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why reverse the contains()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@halibobo1205 halibobo1205 Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

charset=gb2312;application/JSON is also legal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually contentType.contains(MimeTypes.Type.APPLICATION_JSON.asString() is compatible with all scenarios, more judgment seems redundant

return request.getParameter(key);
} else {
return null;
Expand Down