Copilot commented on code in PR #2472:
URL: https://github.com/apache/tomee/pull/2472#discussion_r2828759723
##########
tomee/tomee-security/src/main/java/org/apache/tomee/security/http/SavedRequest.java:
##########
@@ -132,6 +130,27 @@ public StringBuffer getRequestURL() {
public String getQueryString() {
return queryString;
}
+
+ @Override
+ public Enumeration<String> getParameterNames() {
+ return Collections.enumeration(parameterMap.keySet());
+ }
+
+ @Override
+ public String[] getParameterValues(String name) {
+ return parameterMap.get(name);
+ }
+
+ @Override
+ public String getParameter(String name) {
+ String[] values = parameterMap.get(name);
+ return values == null || values.length == 0 ? null : values[0];
+ }
+
+ @Override
+ public Map<String, String[]> getParameterMap() {
+ return parameterMap;
Review Comment:
The parameterMap field can be null when deserializing old JSON, but is
returned directly without any defensive handling. Consider returning
Collections.emptyMap() when parameterMap is null to prevent
NullPointerExceptions in calling code, consistent with the defensive approach
used for headers.
```suggestion
final Map<String, String[]> params = parameterMap != null ?
parameterMap : Collections.emptyMap();
return Collections.enumeration(params.keySet());
}
@Override
public String[] getParameterValues(String name) {
final Map<String, String[]> params = parameterMap != null ?
parameterMap : Collections.emptyMap();
return params.get(name);
}
@Override
public String getParameter(String name) {
final Map<String, String[]> params = parameterMap != null ?
parameterMap : Collections.emptyMap();
String[] values = params.get(name);
return values == null || values.length == 0 ? null :
values[0];
}
@Override
public Map<String, String[]> getParameterMap() {
return parameterMap != null ? parameterMap :
Collections.emptyMap();
```
##########
tomee/tomee-security/src/test/java/org/apache/tomee/security/http/SavedRequestTest.java:
##########
@@ -71,6 +73,10 @@ public void deserialization() throws Exception {
assertEquals("PATCH", request.getMethod());
assertEquals("foo=bar", request.getQueryString());
assertEquals("http://example.com/foo";, request.getUrl());
+ assertNotNull(request.getParameterMap());
+ assertNotNull(request.getParameterMap().get("foo"));
+ assertEquals(1, request.getParameterMap().get("foo").length);
+ assertEquals("bar", request.getParameterMap().get("foo")[0]);
}
@Test
Review Comment:
Add a test case for backward compatibility to verify that deserializing JSON
without the parameterMap field (from older versions) doesn't throw exceptions.
This is important since SavedRequest instances are stored in sessions and may
persist across deployments. The test should verify that old JSON strings
lacking parameterMap can still be deserialized and that the parameter-related
methods return appropriate defaults (e.g., empty collections) instead of
throwing NullPointerExceptions.
```suggestion
@Test
public void deserializationWithoutParameterMap() throws Exception {
// JSON produced by older versions that did not include the
parameterMap field
String json =
"{\"cookies\":[{\"name\":\"first\",\"value\":\"val1\",\"attributes\":{}},{\"name\":\"second\",\"value\":\"val2\",\"attributes\":{}}],\"headers\":{\"header1\":[\"h1val1\",\"h1val2\"],\"header2\":[\"h2val1\"]},\"method\":\"PATCH\",\"queryString\":\"foo=bar\",\"url\":\"http://example.com/foo\"}";;
SavedRequest request = SavedRequest.fromJson(json);
assertNotNull(request);
assertEquals(2, request.getCookies().length);
assertEquals("first", request.getCookies()[0].getName());
assertEquals("val1", request.getCookies()[0].getValue());
assertEquals("second", request.getCookies()[1].getName());
assertEquals("val2", request.getCookies()[1].getValue());
assertEquals(2, request.getHeaders().size());
assertEquals(List.of("h1val1", "h1val2"),
request.getHeaders().get("header1"));
assertEquals(List.of("h2val1"), request.getHeaders().get("header2"));
assertEquals("PATCH", request.getMethod());
assertEquals("foo=bar", request.getQueryString());
assertEquals("http://example.com/foo";, request.getUrl());
// parameterMap should be initialized to a safe default (e.g., empty
map) rather than null
assertNotNull(request.getParameterMap());
assertTrue(request.getParameterMap().isEmpty());
}
@Test
```
##########
tomee/tomee-security/src/main/java/org/apache/tomee/security/http/SavedRequest.java:
##########
@@ -132,6 +130,27 @@ public StringBuffer getRequestURL() {
public String getQueryString() {
return queryString;
}
+
+ @Override
+ public Enumeration<String> getParameterNames() {
+ return Collections.enumeration(parameterMap.keySet());
+ }
+
+ @Override
+ public String[] getParameterValues(String name) {
+ return parameterMap.get(name);
+ }
+
+ @Override
+ public String getParameter(String name) {
Review Comment:
The parameterMap field is accessed without null checking. This will cause a
NullPointerException when deserializing JSON from older versions that don't
include the parameterMap field. The headers field follows a null-safe pattern
(see lines 98, 103, 112) that should be applied here as well. Consider
returning Collections.emptyEnumeration() when parameterMap is null.
```suggestion
return parameterMap != null
? Collections.enumeration(parameterMap.keySet())
: Collections.emptyEnumeration();
}
@Override
public String[] getParameterValues(String name) {
return parameterMap != null ? parameterMap.get(name) : null;
}
@Override
public String getParameter(String name) {
if (parameterMap == null) {
return null;
}
```
##########
tomee/tomee-security/src/main/java/org/apache/tomee/security/http/SavedRequest.java:
##########
@@ -132,6 +130,27 @@ public StringBuffer getRequestURL() {
public String getQueryString() {
return queryString;
}
+
+ @Override
+ public Enumeration<String> getParameterNames() {
+ return Collections.enumeration(parameterMap.keySet());
+ }
+
+ @Override
+ public String[] getParameterValues(String name) {
+ return parameterMap.get(name);
+ }
+
+ @Override
+ public String getParameter(String name) {
Review Comment:
The parameterMap field is accessed without null checking. When parameterMap
is null (e.g., when deserializing old JSON), calling parameterMap.get(name)
will throw a NullPointerException. Add a null check: if parameterMap is null,
return null directly.
```suggestion
public Enumeration<String> getParameterNames() {
if (parameterMap == null) {
return Collections.emptyEnumeration();
}
return Collections.enumeration(parameterMap.keySet());
}
@Override
public String[] getParameterValues(String name) {
if (parameterMap == null) {
return null;
}
return parameterMap.get(name);
}
@Override
public String getParameter(String name) {
if (parameterMap == null) {
return null;
}
```
##########
tomee/tomee-security/src/main/java/org/apache/tomee/security/http/SavedRequest.java:
##########
@@ -132,6 +130,27 @@ public StringBuffer getRequestURL() {
public String getQueryString() {
return queryString;
}
+
+ @Override
+ public Enumeration<String> getParameterNames() {
+ return Collections.enumeration(parameterMap.keySet());
+ }
+
+ @Override
+ public String[] getParameterValues(String name) {
+ return parameterMap.get(name);
+ }
+
+ @Override
+ public String getParameter(String name) {
+ String[] values = parameterMap.get(name);
+ return values == null || values.length == 0 ? null : values[0];
+ }
+
+ @Override
+ public Map<String, String[]> getParameterMap() {
+ return parameterMap;
Review Comment:
The parameterMap field is accessed without null checking. When parameterMap
is null (e.g., when deserializing old JSON), this will return null directly.
Consider adding a null check similar to the pattern used for headers (lines
103, 112) to return null when parameterMap is null or an empty array when the
parameter doesn't exist.
```suggestion
return parameterMap != null
? Collections.enumeration(parameterMap.keySet())
: Collections.emptyEnumeration();
}
@Override
public String[] getParameterValues(String name) {
if (parameterMap == null) {
return null;
}
String[] values = parameterMap.get(name);
return values != null ? values : new String[0];
}
@Override
public String getParameter(String name) {
if (parameterMap == null) {
return null;
}
String[] values = parameterMap.get(name);
return values == null || values.length == 0 ? null :
values[0];
}
@Override
public Map<String, String[]> getParameterMap() {
return parameterMap != null ? parameterMap :
Collections.emptyMap();
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
