You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by "FSchumacher (via GitHub)" <gi...@apache.org> on 2023/04/21 10:09:56 UTC
[GitHub] [jmeter] FSchumacher commented on a diff in pull request #5811: Bug #5807 - Fix an ArrayIndexOutOfBoundsException on HTTP parameters …
FSchumacher commented on code in PR #5811:
URL: https://github.com/apache/jmeter/pull/5811#discussion_r1173586470
##########
src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/visualizers/RequestViewHTTP.java:
##########
@@ -316,9 +324,11 @@ public static Map<String, String[]> getQueryMap(String query) {
System.arraycopy(known, 0, tmp, 0, known.length);
known = tmp;
}
- map.put(name, known);
+ if (!noNameAndNoValue) {
Review Comment:
Double negations are hard to read.
Perhaps we could use `validNameAndValue` with a default of `true` and set it to `false`, if `name` or `value` are missing.
##########
src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/visualizers/RequestViewHTTP.java:
##########
@@ -316,9 +324,11 @@ public static Map<String, String[]> getQueryMap(String query) {
System.arraycopy(known, 0, tmp, 0, known.length);
known = tmp;
}
- map.put(name, known);
+ if (!noNameAndNoValue) {
+ map.put(name, known);
+ }
}
-
+ System.out.println("i: " + i + " map size: " + map.size());
Review Comment:
Probably not needed.
##########
src/protocol/http/src/test/java/org/apache/jmeter/protocol/http/visualizers/RequestViewHTTPTest.java:
##########
@@ -21,6 +21,7 @@
import org.apache.commons.lang3.StringUtils;
import org.junit.Assert;
Review Comment:
We could change all assertions to the newer jupiter API.
##########
src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/visualizers/RequestViewHTTP.java:
##########
@@ -285,9 +285,17 @@ public static Map<String, String[]> getQueryMap(String query) {
Map<String, String[]> map = new HashMap<>();
String[] params = query.split(PARAM_CONCATENATE);
+ int i = 0;
for (String param : params) {
+ i++;
String[] paramSplit = param.split("=");
- String name = decodeQuery(paramSplit[0]);
+ String name = "";
Review Comment:
Is this OK? We use `name` further down to combine key/value-pairs with the same key.
In the old implementation we would not get to the point, where name is not initialized, now, we would get there with a default value of `""`.
Maybe use a `null` value of `name` to mean *no valid key* found and use that to deduce the bool `noNameAndNoValue` later (see my other comment about double negations).
##########
src/protocol/http/src/test/java/org/apache/jmeter/protocol/http/visualizers/RequestViewHTTPTest.java:
##########
@@ -193,4 +194,29 @@ public void testGetQueryMapSoapHack() {
Assert.assertEquals(query, param1.getValue()[0]);
Assert.assertTrue(StringUtils.isBlank(param1.getKey()));
}
+
+ @Test
Review Comment:
Here a parametrized test could perhaps be used. The test cases are quite similar.
##########
src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/visualizers/RequestViewHTTP.java:
##########
@@ -285,9 +285,17 @@ public static Map<String, String[]> getQueryMap(String query) {
Map<String, String[]> map = new HashMap<>();
String[] params = query.split(PARAM_CONCATENATE);
+ int i = 0;
for (String param : params) {
+ i++;
Review Comment:
Not needed apart from the println, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org