Skip to content

Commit 45df2b0

Browse files
jpfennellyjpfennelly
committed
Fix AwsProxyRequestBuilder to be immutable when calling the alb() method
1 parent b0754f3 commit 45df2b0

File tree

2 files changed

+199
-34
lines changed

2 files changed

+199
-34
lines changed

‎aws-serverless-java-container-core/src/test/java/com/amazonaws/serverless/proxy/internal/testutils/AwsProxyRequestBuilder.java‎

Lines changed: 42 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import com.amazonaws.serverless.proxy.model.*;
1717

1818
import com.fasterxml.jackson.core.JsonProcessingException;
19+
import com.fasterxml.jackson.databind.ObjectMapper;
1920
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
2021
import org.apache.commons.io.IOUtils;
2122
import org.apache.hc.core5.http.ContentType;
@@ -49,7 +50,7 @@ public class AwsProxyRequestBuilder {
4950

5051
private AwsProxyRequest request;
5152
private MultipartEntityBuilder multipartBuilder;
52-
53+
5354
//-------------------------------------------------------------
5455
// Constructors
5556
//-------------------------------------------------------------
@@ -86,22 +87,41 @@ public AwsProxyRequestBuilder(String path, String httpMethod) {
8687
this.request.getRequestContext().setIdentity(identity);
8788
}
8889

89-
90-
//-------------------------------------------------------------
90+
//-------------------------------------------------------------
9191
// Methods - Public
9292
//-------------------------------------------------------------
9393

9494
public AwsProxyRequestBuilder alb() {
95-
this.request.setRequestContext(new AwsProxyRequestContext());
96-
this.request.getRequestContext().setElb(new AlbContext());
97-
this.request.getRequestContext().getElb().setTargetGroupArn(
95+
/*
96+
* This method sets up the requestContext to look like an ALB request and also
97+
* re-encodes URL query params, since ALBs do not decode them. This now returns
98+
* a new AwsProxyRequestBuilder with the new query param state, so the original
99+
* builder maintains the original configured state and can be then be reused in
100+
* further unit tests. For now the simplest way to accomplish a deep copy is by
101+
* serializing to JSON then deserializing.
102+
*/
103+
104+
ObjectMapper objectMapper = new ObjectMapper();
105+
AwsProxyRequest albRequest = null;
106+
try {
107+
String json = objectMapper.writeValueAsString(this.request);
108+
albRequest = objectMapper.readValue(json, AwsProxyRequest.class);
109+
} catch (JsonProcessingException jpe) {
110+
throw new RuntimeException(jpe);
111+
}
112+
113+
if (albRequest.getRequestContext() == null) {
114+
albRequest.setRequestContext(new AwsProxyRequestContext());
115+
}
116+
albRequest.getRequestContext().setElb(new AlbContext());
117+
albRequest.getRequestContext().getElb().setTargetGroupArn(
98118
"arn:aws:elasticloadbalancing:us-east-1:123456789012:targetgroup/lambda-target/d6190d154bc908a5"
99119
);
100120

101121
// ALB does not decode query string parameters so we re-encode them all
102-
if (request.getMultiValueQueryStringParameters() != null) {
122+
if (albRequest.getMultiValueQueryStringParameters() != null) {
103123
MultiValuedTreeMap<String, String> newQs = new MultiValuedTreeMap<>();
104-
for (Map.Entry<String, List<String>> e : request.getMultiValueQueryStringParameters().entrySet()) {
124+
for (Map.Entry<String, List<String>> e : albRequest.getMultiValueQueryStringParameters().entrySet()) {
105125
for (String v : e.getValue()) {
106126
try {
107127
// this is a terrible hack. In our Spring tests we use the comma as a control character for lists
@@ -114,9 +134,9 @@ public AwsProxyRequestBuilder alb() {
114134
}
115135
}
116136
}
117-
request.setMultiValueQueryStringParameters(newQs);
137+
albRequest.setMultiValueQueryStringParameters(newQs);
118138
}
119-
return this;
139+
return new AwsProxyRequestBuilder(albRequest);
120140
}
121141

122142
public AwsProxyRequestBuilder stage(String stageName) {
@@ -142,6 +162,9 @@ public AwsProxyRequestBuilder json() {
142162

143163

144164
public AwsProxyRequestBuilder form(String key, String value) {
165+
if (key == null || value == null) {
166+
throw new IllegalArgumentException("form() does not support null key or value");
167+
}
145168
if (request.getMultiValueHeaders() == null) {
146169
request.setMultiValueHeaders(new Headers());
147170
}
@@ -150,7 +173,12 @@ public AwsProxyRequestBuilder form(String key, String value) {
150173
if (body == null) {
151174
body = "";
152175
}
153-
body += (body.equals("")?"":"&") + key + "=" + value;
176+
// URL-encode key and value to form expected body of a form post
177+
try {
178+
body += (body.equals("") ? "" : "&") + URLEncoder.encode(key, "UTF-8") + "=" + URLEncoder.encode(value, "UTF-8");
179+
} catch (UnsupportedEncodingException ex) {
180+
throw new RuntimeException("Could not encode form parameter: " + key + "=" + value, ex);
181+
}
154182
request.setBody(body);
155183
return this;
156184
}
@@ -214,35 +242,15 @@ public AwsProxyRequestBuilder multiValueQueryString(MultiValuedTreeMap<String, S
214242
return this;
215243
}
216244

217-
218245
public AwsProxyRequestBuilder queryString(String key, String value) {
219246
if (this.request.getMultiValueQueryStringParameters() == null) {
220247
this.request.setMultiValueQueryStringParameters(new MultiValuedTreeMap<>());
221248
}
222249

223-
if (request.getRequestSource() == RequestSource.API_GATEWAY) {
224-
this.request.getMultiValueQueryStringParameters().add(key, value);
225-
}
226-
// ALB does not decode parameters automatically like API Gateway.
227-
if (request.getRequestSource() == RequestSource.ALB) {
228-
try {
229-
//if (URLDecoder.decode(value, ContainerConfig.DEFAULT_CONTENT_CHARSET).equals(value)) {
230-
// TODO: Assume we are always given an unencoded value, smarter check here to encode
231-
// only if necessary
232-
this.request.getMultiValueQueryStringParameters().add(
233-
key,
234-
URLEncoder.encode(value, ContainerConfig.DEFAULT_CONTENT_CHARSET)
235-
);
236-
//}
237-
} catch (UnsupportedEncodingException e) {
238-
throw new RuntimeException(e);
239-
}
240-
241-
}
250+
this.request.getMultiValueQueryStringParameters().add(key, value);
242251
return this;
243252
}
244253

245-
246254
public AwsProxyRequestBuilder body(String body) {
247255
this.request.setBody(body);
248256
return this;
@@ -475,11 +483,11 @@ public HttpApiV2ProxyRequest toHttpApiV2Request() {
475483
request.getMultiValueQueryStringParameters().forEach((k, v) -> {
476484
for (String s : v) {
477485
rawQueryString.append("&");
478-
rawQueryString.append(k);
479-
rawQueryString.append("=");
480486
try {
481487
// same terrible hack as the alb() method. Because our spring tests use commas as control characters
482488
// we do not encode it
489+
rawQueryString.append(URLEncoder.encode(k, "UTF-8").replaceAll("%2C", ","));
490+
rawQueryString.append("=");
483491
rawQueryString.append(URLEncoder.encode(s, "UTF-8").replaceAll("%2C", ","));
484492
} catch (UnsupportedEncodingException e) {
485493
throw new RuntimeException(e);

‎aws-serverless-java-container-core/src/test/java/com/amazonaws/serverless/proxy/internal/testutils/AwsProxyRequestBuilderTest.java‎

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
package com.amazonaws.serverless.proxy.internal.testutils;
2+
3+
import java.io.IOException;
4+
import java.net.URLEncoder;
5+
import jakarta.ws.rs.core.HttpHeaders;
6+
import jakarta.ws.rs.core.MediaType;
7+
8+
import com.amazonaws.serverless.proxy.model.*;
9+
import org.junit.jupiter.api.Test;
10+
11+
import static org.junit.jupiter.api.Assertions.*;
12+
13+
public class AwsProxyRequestBuilderTest {
14+
15+
private static final String TEST_KEY = "testkey";
16+
private static final String TEST_VALUE = "testvalue";
17+
private static final String TEST_KEY_FOR_ENCODING = "test@key 1";
18+
private static final String TEST_VALUE_FOR_ENCODING = "test value!!";
19+
20+
21+
void baseConstructorAsserts(AwsProxyRequest request) {
22+
assertEquals(0, request.getMultiValueHeaders().size());
23+
assertEquals(0, request.getHeaders().size());
24+
assertEquals(0, request.getMultiValueQueryStringParameters().size());
25+
assertNotNull(request.getRequestContext());
26+
assertNotNull(request.getRequestContext().getRequestId());
27+
assertNotNull(request.getRequestContext().getExtendedRequestId());
28+
assertEquals("test", request.getRequestContext().getStage());
29+
assertEquals("HTTP/1.1", request.getRequestContext().getProtocol());
30+
assertNotNull(request.getRequestContext().getRequestTimeEpoch());
31+
assertNotNull(request.getRequestContext().getIdentity());
32+
assertEquals("127.0.0.1", request.getRequestContext().getIdentity().getSourceIp());
33+
}
34+
35+
@Test
36+
void constructor_path_httpMethod() {
37+
38+
AwsProxyRequestBuilder builder = new AwsProxyRequestBuilder("/path", "GET");
39+
AwsProxyRequest request = builder.build();
40+
assertEquals("/path", request.getPath());
41+
assertEquals("GET", request.getHttpMethod());
42+
baseConstructorAsserts(request);
43+
}
44+
45+
@Test
46+
void constructor_path_nullHttpMethod() {
47+
AwsProxyRequestBuilder builder = new AwsProxyRequestBuilder("/path");
48+
AwsProxyRequest request = builder.build();
49+
assertNull(request.getHttpMethod());
50+
assertEquals("/path", request.getPath());
51+
baseConstructorAsserts(request);
52+
}
53+
54+
@Test
55+
void constructor_nullPath_nullHttpMethod() {
56+
AwsProxyRequestBuilder builder = new AwsProxyRequestBuilder();
57+
AwsProxyRequest request = builder.build();
58+
assertNull(request.getHttpMethod());
59+
assertNull(request.getPath());
60+
baseConstructorAsserts(request);
61+
}
62+
63+
@Test
64+
void form_key_value() {
65+
AwsProxyRequestBuilder builder = new AwsProxyRequestBuilder("/path", "POST");
66+
builder.form(TEST_KEY, TEST_VALUE);
67+
AwsProxyRequest request = builder.build();
68+
assertEquals(1, request.getMultiValueHeaders().get(HttpHeaders.CONTENT_TYPE).size());
69+
assertEquals(MediaType.APPLICATION_FORM_URLENCODED, request.getMultiValueHeaders().getFirst(HttpHeaders.CONTENT_TYPE));
70+
assertNull(request.getHeaders().get(HttpHeaders.CONTENT_TYPE));
71+
assertNotNull(request.getBody());
72+
assertEquals(TEST_KEY + "=" + TEST_VALUE, request.getBody());
73+
}
74+
75+
@Test
76+
void form_key_nullKey_nullValue() {
77+
AwsProxyRequestBuilder builder = new AwsProxyRequestBuilder("/path", "POST");
78+
assertThrows(IllegalArgumentException.class, () -> builder.form(null, TEST_VALUE));
79+
assertThrows(IllegalArgumentException.class, () -> builder.form(TEST_KEY, null));
80+
assertThrows(IllegalArgumentException.class, () -> builder.form(null, null));
81+
}
82+
83+
@Test
84+
void form_keyEncoded_valueEncoded() throws IOException {
85+
AwsProxyRequestBuilder builder = new AwsProxyRequestBuilder("/path", "POST");
86+
builder.form(TEST_KEY_FOR_ENCODING, TEST_VALUE_FOR_ENCODING);
87+
AwsProxyRequest request = builder.build();
88+
89+
assertEquals(1, request.getMultiValueHeaders().get(HttpHeaders.CONTENT_TYPE).size());
90+
assertEquals(MediaType.APPLICATION_FORM_URLENCODED, request.getMultiValueHeaders().getFirst(HttpHeaders.CONTENT_TYPE));
91+
assertNull(request.getHeaders().get(HttpHeaders.CONTENT_TYPE));
92+
assertNotNull(request.getBody());
93+
String expected = URLEncoder.encode(TEST_KEY_FOR_ENCODING, "UTF-8") + "=" + URLEncoder.encode(TEST_VALUE_FOR_ENCODING, "UTF-8");
94+
assertEquals(expected, request.getBody());
95+
}
96+
97+
@Test
98+
void queryString_key_value() {
99+
AwsProxyRequestBuilder builder = new AwsProxyRequestBuilder("/path", "POST");
100+
builder.queryString(TEST_KEY, TEST_VALUE);
101+
AwsProxyRequest request = builder.build();
102+
103+
assertNull(request.getQueryStringParameters());
104+
assertEquals(1, request.getMultiValueQueryStringParameters().size());
105+
assertEquals(TEST_KEY, request.getMultiValueQueryStringParameters().keySet().iterator().next());
106+
assertEquals(TEST_VALUE, request.getMultiValueQueryStringParameters().get(TEST_KEY).get(0));
107+
assertEquals(TEST_VALUE, request.getMultiValueQueryStringParameters().getFirst(TEST_KEY));
108+
}
109+
110+
@Test
111+
void queryString_keyNotEncoded_valueNotEncoded() {
112+
// builder should not URL encode key or value for query string
113+
// in the case of an ALB where values should be encoded, the builder alb() method will handle it
114+
AwsProxyRequestBuilder builder = new AwsProxyRequestBuilder("/path", "POST");
115+
builder.queryString(TEST_KEY_FOR_ENCODING, TEST_VALUE_FOR_ENCODING);
116+
AwsProxyRequest request = builder.build();
117+
118+
assertNull(request.getQueryStringParameters());
119+
assertEquals(1, request.getMultiValueQueryStringParameters().size());
120+
assertEquals(TEST_KEY_FOR_ENCODING, request.getMultiValueQueryStringParameters().keySet().iterator().next());
121+
assertEquals(TEST_VALUE_FOR_ENCODING, request.getMultiValueQueryStringParameters().get(TEST_KEY_FOR_ENCODING).get(0));
122+
assertEquals(TEST_VALUE_FOR_ENCODING, request.getMultiValueQueryStringParameters().getFirst(TEST_KEY_FOR_ENCODING));
123+
}
124+
125+
@Test
126+
void queryString_alb_key_value() {
127+
AwsProxyRequestBuilder builder = new AwsProxyRequestBuilder("/path", "POST");
128+
builder.queryString(TEST_KEY, TEST_VALUE);
129+
AwsProxyRequest request = builder.alb().build();
130+
131+
assertNull(request.getQueryStringParameters());
132+
assertEquals(1, request.getMultiValueQueryStringParameters().size());
133+
assertEquals(TEST_KEY, request.getMultiValueQueryStringParameters().keySet().iterator().next());
134+
assertEquals(TEST_VALUE, request.getMultiValueQueryStringParameters().get(TEST_KEY).get(0));
135+
assertEquals(TEST_VALUE, request.getMultiValueQueryStringParameters().getFirst(TEST_KEY));
136+
}
137+
138+
@Test
139+
void alb_keyEncoded_valueEncoded() throws IOException {
140+
AwsProxyRequestBuilder builder = new AwsProxyRequestBuilder("/path", "POST");
141+
MultiValuedTreeMap<String, String> map = new MultiValuedTreeMap<>();
142+
map.add(TEST_KEY_FOR_ENCODING, TEST_VALUE_FOR_ENCODING);
143+
builder.multiValueQueryString(map);
144+
AwsProxyRequest request = builder.alb().build();
145+
146+
String expectedKey = URLEncoder.encode(TEST_KEY_FOR_ENCODING, "UTF-8");
147+
String expectedValue = URLEncoder.encode(TEST_VALUE_FOR_ENCODING, "UTF-8");
148+
assertEquals(1, request.getMultiValueQueryStringParameters().size());
149+
assertEquals(expectedKey, request.getMultiValueQueryStringParameters().keySet().iterator().next());
150+
assertEquals(expectedValue, request.getMultiValueQueryStringParameters().get(expectedKey).get(0));
151+
assertEquals(expectedValue, request.getMultiValueQueryStringParameters().getFirst(expectedKey));
152+
assertEquals(expectedKey, request.getMultiValueQueryStringParameters().keySet().iterator().next());
153+
assertEquals(expectedValue, request.getMultiValueQueryStringParameters().get(expectedKey).get(0));
154+
assertEquals(expectedValue, request.getMultiValueQueryStringParameters().getFirst(expectedKey));
155+
}
156+
157+
}

0 commit comments

Comments
 (0)