fix(http): fix java.lang.IllegalStateException: STREAMED #5367

forfreeday · 2023-07-20T16:47:06Z

What does this PR do?

Optimized Util getAddress method
add GetBrokerageServletTest test case
add GetRewardServletTest test case

Why are these changes required?
Fix incorrect interface returns, add test cases

This PR has been tested by:

Unit Tests
Manual Testing

Follow up

Extra details

1. Optimized Util getAddress method 2. add `GetBrokerageServletTest` test case 3. add `GetRewardServletTest` test case

halibobo1205 · 2023-07-21T10:30:32Z

framework/src/main/java/org/tron/core/services/http/Util.java

@@ -499,6 +500,12 @@ public static byte[] getAddress(HttpServletRequest request) throws Exception {
    byte[] address = null;
    String addressParam = "address";
    String addressStr = request.getParameter(addressParam);
+
+    if (APPLICATION_FORM_URLENCODED.equals(request.getContentType())


It's not clear. Is this the only scene?

It is recommended to make a clear distinction between GET, POST (application/json, application/x-www-form-urlencoded)

There is no need to add other types of content types, and these types are supported even if other types of judgments are not explicitly added.

forfreeday · 2023-07-24T08:12:18Z

@halibobo1205

With the addition of parameter checks, if the uploaded parameter is an illegal parameter, an error will be returned.

request:

curl --location 'http://127.0.0.1:8090/wallet/getReward' \
--header 'Content-Type: application/x-www-form-urlencoded' \
--data-urlencode 'address='

expected Result:

{
    "Error": "INVALID address, invalid request parameter"
}

Otherwise, normal parameter requests for JSON, GET, and POST are not affected.

1. add parameter checking methods

halibobo1205 · 2023-07-28T09:12:13Z

framework/src/main/java/org/tron/core/services/http/Util.java

+    if (HttpMethod.GET.toString().toUpperCase() .equalsIgnoreCase(method)) {
+      value = request.getParameter(key);
+      if (StringUtils.isBlank(value)) {
+        throw new IllegalArgumentException("Invalid request parameter");


Need to keep compatibility

halibobo1205 · 2023-07-28T09:17:02Z

framework/src/main/java/org/tron/core/services/http/Util.java

+        }
+        if (keyValue[0].equals("address")) {
+          return keyValue[1];
+        }


Need to keep compatibility

halibobo1205 · 2023-08-07T09:10:46Z

@forfreeday Is there any progress on this PR?

forfreeday · 2023-08-09T06:19:12Z

@halibobo1205
Adjusted the level of interface validation, keeping the original checksums

halibobo1205 · 2023-08-09T08:06:59Z

framework/src/main/java/org/tron/core/services/http/Util.java

+          return jsonObject.getString(key);
+        }
+      } else if (APPLICATION_FORM_URLENCODED.toLowerCase().contains(contentType)) {
+        return getParam(getRequestValue(request));


Why not just use request.getParameter(addressParam)?

halibobo1205 · 2023-08-09T08:11:25Z

framework/src/main/java/org/tron/core/services/http/Util.java

+        if (keyValue.length == 1) {
+          continue;
+        }
+        if (keyValue[0].equals("address")) {


BTW, Why is it limited to address ? getParam seems to be a generic method.

forfreeday · 2023-08-09T08:18:31Z

@halibobo1205
getParam doesn't need to be a generic method, it's just that the name of the method looks generic, and the only correct entry for this method is address, while all the others are incorrect.

halibobo1205 · 2023-08-09T08:31:19Z

@halibobo1205 getParam doesn't need to be a generic method, it's just that the name of the method looks generic, and the only correct entry for this method is address, while all the others are incorrect.

@forfreeday Why does getParam need an input?

forfreeday · 2023-08-09T08:36:43Z

@halibobo1205
Do you understand what this method does?
getParam is the input parameter of the POST method.
What would happen if you didn't use getParam?

halibobo1205 · 2023-08-09T08:42:29Z

@halibobo1205 Do you understand what this method does? getParam is the input parameter of the POST method. What would happen if you didn't use getParam?

@forfreeday I really don't get it. @tomatoishealthy Please take a look.

tomatoishealthy · 2023-08-09T09:14:13Z

framework/src/test/java/org/tron/core/services/http/GetBrokerageServletTest.java

For historical reasons, this logic does not have a unit test.

Better add comprehensive unit tests for all situations after changing the whole logic of the param extraction to make sure all the new logic works well, such as: good requests for both form&json

framework/src/main/java/org/tron/core/services/http/Util.java

codecov-commenter · 2023-08-09T14:43:33Z

Codecov Report

Merging #5367 (1d3036a) into develop (83151aa) will increase coverage by 0.00%.
The diff coverage is 64.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             develop    #5367   +/-   ##
==========================================
  Coverage      60.95%   60.96%           
- Complexity      9247     9253    +6     
==========================================
  Files            840      840           
  Lines          50064    50081   +17     
  Branches        5578     5584    +6     
==========================================
+ Hits           30517    30530   +13     
  Misses         17146    17146           
- Partials        2401     2405    +4

Files Changed	Coverage Δ
...rc/main/java/org/tron/core/services/http/Util.java	`40.65% <64.00%> (+2.11%)`	⬆️

... and 8 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

halibobo1205 · 2023-08-10T10:17:35Z

framework/src/test/java/org/tron/core/services/http/GetBrokerageServletTest.java

+      String content = (String) result.get("Error");
+      Assert.assertNull(content);
+    } catch (UnsupportedEncodingException e) {
+      e.printStackTrace();


Assert.fail(); is a good choice

I'm not expecting an exception to be thrown; the end result of this use case is a normal return of the

Does it mean that even if an UnsupportedEncodingException is thrown, it's still normal?

Using a statement when pointing out a problem is a good way to respond; if you use a rhetorical question, I can't be sure of your intentions and it doesn't help to accurately describe the problem.

The point of this use case is not to test for the UnsupportedEncodingException, but rather to determine the value of null.

Assert.fail() is added where it is not possible to reach in the expectation, once it is reached, indicates that the test has failed, the result is not as expected.

halibobo1205 · 2023-08-10T10:19:47Z

framework/src/test/java/org/tron/core/services/http/GetRewardServletTest.java

+      String content = (String) result.get("Error");
+      Assert.assertNull(content);
+    } catch (UnsupportedEncodingException e) {
+      e.printStackTrace();


Please test all cases of checkGetParam.

checkGetParam is business-independent and does not need to be validated.

halibobo1205 · 2023-08-17T09:48:39Z

framework/src/main/java/org/tron/core/services/http/Util.java

-        addressStr = jsonObject.getString(addressParam);
-      }
-    }
+    String addressStr = checkGetParam(request, addressParam);


getParam or checkAndGetParam ?

Use checkGetParam as the method name, because there are some checking judgments

halibobo1205 · 2023-08-17T09:49:52Z

framework/src/test/java/org/tron/core/services/http/GetRewardServletTest.java

+  }
+
+  @After
+  public void deleteDatabase() {


BaseTest has already done the release.

Yes, it can be dispensed with here

halibobo1205

LGTM

feat(Servlet): Optimize Http Servlet

029c040

1. Optimized Util getAddress method 2. add `GetBrokerageServletTest` test case 3. add `GetRewardServletTest` test case

forfreeday changed the title ~~feat(Servlet): Optimize Http Servlet~~ feat(Servlet): optimize the http request interface Jul 20, 2023

forfreeday mentioned this pull request Jul 21, 2023

java.lang.IllegalStateException: STREAMED: (GetRewardServlet.java:36) #5356

Closed

lvs007 requested review from xxo1shine and halibobo1205 July 21, 2023 09:09

halibobo1205 linked an issue Jul 21, 2023 that may be closed by this pull request

java.lang.IllegalStateException: STREAMED: (GetRewardServlet.java:36) #5356

Closed

halibobo1205 reviewed Jul 21, 2023

View reviewed changes

feat(util): optimization parameter check

9003b0e

1. add parameter checking methods

halibobo1205 reviewed Jul 28, 2023

View reviewed changes

halibobo1205 added this to the GreatVoyage-v4.7.3 milestone Aug 9, 2023

liukai added 2 commits August 9, 2023 15:16

feat(Servlet): Optimize request parameter fetching

0bcaaad

test(Servlet): modify assertion conditions

0664c4a

halibobo1205 reviewed Aug 9, 2023

View reviewed changes

halibobo1205 requested a review from tomatoishealthy August 9, 2023 08:40

feat(Util): Optimize of the method into the parameter

e404b18

tomatoishealthy reviewed Aug 9, 2023

View reviewed changes

feat(Util): simplified return logic

b1a2870

halibobo1205 reviewed Aug 9, 2023

View reviewed changes

framework/src/main/java/org/tron/core/services/http/Util.java Show resolved Hide resolved

liukai added 2 commits August 9, 2023 18:01

feat(Util): remove Content-type checksums

8bad304

feat(Util): fix codeStyle

b3400df

halibobo1205 reviewed Aug 10, 2023

View reviewed changes

halibobo1205 assigned forfreeday Aug 10, 2023

liukai added 3 commits August 14, 2023 19:32

test(case): add routine use cases

75d832e

test(case): add json case

304dba4

test(case): fix case

c8a0cc5

halibobo1205 reviewed Aug 17, 2023

View reviewed changes

liukai added 2 commits August 17, 2023 18:02

test(case): remove useless methods

9cc6b5e

Merge remote-tracking branch 'upstream/develop' into feat/fix_servlet

1d3036a

halibobo1205 approved these changes Aug 18, 2023

View reviewed changes

halibobo1205 merged commit f18864c into tronprotocol:develop Aug 18, 2023

halibobo1205 changed the title ~~feat(Servlet): optimize the http request interface~~ fix(http): fix java.lang.IllegalStateException: STREAMED Oct 18, 2023

halibobo1205 added the area:http label Oct 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(http): fix java.lang.IllegalStateException: STREAMED #5367

fix(http): fix java.lang.IllegalStateException: STREAMED #5367

forfreeday commented Jul 20, 2023

halibobo1205 Jul 21, 2023

halibobo1205 Jul 27, 2023

forfreeday Jul 27, 2023

forfreeday commented Jul 24, 2023

halibobo1205 Jul 28, 2023

halibobo1205 Jul 28, 2023

halibobo1205 commented Aug 7, 2023

forfreeday commented Aug 9, 2023

halibobo1205 Aug 9, 2023

halibobo1205 Aug 9, 2023 •

edited

Loading

forfreeday commented Aug 9, 2023 •

edited

Loading

halibobo1205 commented Aug 9, 2023

forfreeday commented Aug 9, 2023 •

edited

Loading

halibobo1205 commented Aug 9, 2023

tomatoishealthy Aug 9, 2023

codecov-commenter commented Aug 9, 2023 •

edited

Loading

halibobo1205 Aug 10, 2023

forfreeday Aug 14, 2023

halibobo1205 Aug 14, 2023

forfreeday Aug 14, 2023

forfreeday Aug 14, 2023

halibobo1205 Aug 14, 2023

halibobo1205 Aug 10, 2023

forfreeday Aug 14, 2023 •

edited

Loading

halibobo1205 Aug 17, 2023

forfreeday Aug 17, 2023

halibobo1205 Aug 17, 2023

forfreeday Aug 17, 2023

halibobo1205 left a comment

fix(http): fix java.lang.IllegalStateException: STREAMED #5367

fix(http): fix java.lang.IllegalStateException: STREAMED #5367

Conversation

forfreeday commented Jul 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forfreeday commented Jul 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halibobo1205 commented Aug 7, 2023

forfreeday commented Aug 9, 2023

Choose a reason for hiding this comment

halibobo1205 Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

forfreeday commented Aug 9, 2023 • edited Loading

halibobo1205 commented Aug 9, 2023

forfreeday commented Aug 9, 2023 • edited Loading

halibobo1205 commented Aug 9, 2023

Choose a reason for hiding this comment

codecov-commenter commented Aug 9, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forfreeday Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halibobo1205 left a comment

Choose a reason for hiding this comment

halibobo1205 Aug 9, 2023 •

edited

Loading

forfreeday commented Aug 9, 2023 •

edited

Loading

forfreeday commented Aug 9, 2023 •

edited

Loading

codecov-commenter commented Aug 9, 2023 •

edited

Loading

forfreeday Aug 14, 2023 •

edited

Loading