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

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

Merged
merged 13 commits into from
Aug 18, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
029c040
feat(Servlet): Optimize Http Servlet
forfreeday Jul 14, 2023
9003b0e
feat(util): optimization parameter check
Jul 28, 2023
0bcaaad
feat(Servlet): Optimize request parameter fetching
Aug 9, 2023
0664c4a
test(Servlet): modify assertion conditions
Aug 9, 2023
e404b18
feat(Util): Optimize of the method into the parameter
Aug 9, 2023
b1a2870
feat(Util): simplified return logic
Aug 9, 2023
8bad304
feat(Util): remove Content-type checksums
Aug 9, 2023
b3400df
feat(Util): fix codeStyle
Aug 9, 2023
75d832e
test(case): add routine use cases
Aug 14, 2023
304dba4
test(case): add json case
Aug 15, 2023
c8a0cc5
test(case): fix case
Aug 16, 2023
9cc6b5e
test(case): remove useless methods
Aug 17, 2023
1d3036a
Merge remote-tracking branch 'upstream/develop' into feat/fix_servlet
Aug 18, 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
1 change: 1 addition & 0 deletions framework/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ dependencies {
compile group: 'org.pf4j', name: 'pf4j', version: '2.5.0'

testImplementation group: 'org.springframework', name: 'spring-test', version: '5.2.0.RELEASE'
testImplementation group: 'org.springframework', name: 'spring-web', version: '5.2.0.RELEASE'

compile group: 'org.zeromq', name: 'jeromq', version: '0.5.3'
compile project(":chainbase")
Expand Down
7 changes: 7 additions & 0 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 @@ -70,6 +70,7 @@ 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 String printTransactionFee(String transactionFee) {
JSONObject jsonObject = new JSONObject();
Expand Down Expand Up @@ -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())
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

&& StringUtils.isBlank(addressStr)) {
throw new IllegalArgumentException("invalid request parameter");
}

if (StringUtils.isBlank(addressStr)) {
String input = request.getReader().lines()
.collect(Collectors.joining(System.lineSeparator()));
Expand Down
43 changes: 43 additions & 0 deletions framework/src/test/java/org/tron/core/services/http/GetBrokerageServletTest.java
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
package org.tron.core.services.http;

import static org.junit.Assert.assertEquals;

import com.alibaba.fastjson.JSONObject;

import java.io.UnsupportedEncodingException;

import org.junit.Before;
import org.junit.Test;
import org.springframework.mock.web.MockHttpServletRequest;
import org.springframework.mock.web.MockHttpServletResponse;

public class GetBrokerageServletTest {

private MockHttpServletRequest request;
private MockHttpServletResponse response;

@Before
public void setUp() {
request = new MockHttpServletRequest();
request.setMethod("POST");
request.setContentType("application/x-www-form-urlencoded");
request.setCharacterEncoding("UTF-8");

response = new MockHttpServletResponse();
}

@Test
public void getBrokerageTest() {
request.addParameter("address", "");
GetRewardServlet getRewardServlet = new GetRewardServlet();
getRewardServlet.doPost(request, response);
try {
String contentAsString = response.getContentAsString();
JSONObject result = JSONObject.parseObject(contentAsString);
String content = (String) result.get("Error");
assertEquals(content, "INVALID address, invalid request parameter");
} catch (UnsupportedEncodingException e) {
e.printStackTrace();
Copy link
Contributor

Choose a reason for hiding this comment

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

Assert.fail(); is a good choice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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 point of this use case is not to test for the UnsupportedEncodingException, but rather to determine the value of null.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
}
}
43 changes: 43 additions & 0 deletions framework/src/test/java/org/tron/core/services/http/GetRewardServletTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
package org.tron.core.services.http;

import static org.junit.Assert.assertEquals;

import com.alibaba.fastjson.JSONObject;

import java.io.UnsupportedEncodingException;

import org.junit.Before;
import org.junit.Test;
import org.springframework.mock.web.MockHttpServletRequest;
import org.springframework.mock.web.MockHttpServletResponse;

public class GetRewardServletTest {

private MockHttpServletRequest request;
private MockHttpServletResponse response;

@Before
public void setUp() {
request = new MockHttpServletRequest();
request.setMethod("POST");
request.setContentType("application/x-www-form-urlencoded");
request.setCharacterEncoding("UTF-8");

response = new MockHttpServletResponse();
}

@Test
public void getRewardTest() {
request.addParameter("address", "");
GetRewardServlet getRewardServlet = new GetRewardServlet();
getRewardServlet.doPost(request, response);
try {
String contentAsString = response.getContentAsString();
JSONObject result = JSONObject.parseObject(contentAsString);
String content = (String) result.get("Error");
assertEquals(content, "INVALID address, invalid request parameter");
} catch (UnsupportedEncodingException e) {
e.printStackTrace();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please test all cases of checkGetParam.

Copy link
Contributor Author

@forfreeday forfreeday Aug 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

}
}
}