Skip to content

Commit f78acbd

Browse files
gregwgregw
committed
Passing all tests
1 parent fc2a637 commit f78acbd

File tree

9 files changed

+91
-62
lines changed

9 files changed

+91
-62
lines changed

‎invoker/core/pom.xml‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -97,15 +97,10 @@
9797
<version>${jetty.version}</version>
9898
</dependency>
9999
<dependency>
100-
<groupId>org.eclipse.jetty</groupId>
101-
<artifactId>jetty-slf4j-impl</artifactId>
102-
<version>${jetty.version}</version>
103-
</dependency>
104-
<!--dependency>
105100
<groupId>org.slf4j</groupId>
106101
<artifactId>slf4j-jdk14</artifactId>
107102
<version>2.0.9</version>
108-
</dependency-->
103+
</dependency>
109104
<dependency>
110105
<groupId>com.beust</groupId>
111106
<artifactId>jcommander</artifactId>

‎invoker/core/src/main/java/com/google/cloud/functions/invoker/BackgroundFunctionExecutor.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -341,8 +341,9 @@ public boolean handle(Request req, Response res, Callback callback) throws Excep
341341
res.setStatus(HttpStatus.OK_200);
342342
callback.succeeded();
343343
} catch (Throwable t) {
344-
Response.writeError(req, res, callback, t);
345344
logger.log(Level.SEVERE, "Failed to execute " + functionExecutor.functionName(), t);
345+
res.setStatus(HttpStatus.INTERNAL_SERVER_ERROR_500);
346+
callback.succeeded();
346347
}
347348
return true;
348349
}

‎invoker/core/src/main/java/com/google/cloud/functions/invoker/HttpFunctionExecutor.java‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import com.google.cloud.functions.invoker.http.HttpResponseImpl;
2020
import java.util.logging.Level;
2121
import java.util.logging.Logger;
22+
import org.eclipse.jetty.http.HttpStatus;
2223
import org.eclipse.jetty.server.Handler;
2324
import org.eclipse.jetty.server.Request;
2425
import org.eclipse.jetty.server.Response;
@@ -72,7 +73,13 @@ public boolean handle(Request request, Response response, Callback callback) thr
7273
callback.succeeded();
7374
} catch (Throwable t) {
7475
logger.log(Level.SEVERE, "Failed to execute " + function.getClass().getName(), t);
75-
Response.writeError(request, response, callback, t);
76+
if (response.isCommitted()) {
77+
callback.failed(t);
78+
} else {
79+
response.reset();
80+
response.setStatus(HttpStatus.INTERNAL_SERVER_ERROR_500);
81+
callback.succeeded();
82+
}
7683
} finally {
7784
Thread.currentThread().setContextClassLoader(oldContextLoader);
7885
}

‎invoker/core/src/main/java/com/google/cloud/functions/invoker/TypedFunctionExecutor.java‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
import java.util.Optional;
1616
import java.util.logging.Level;
1717
import java.util.logging.Logger;
18-
import org.eclipse.jetty.http.BadMessageException;
1918
import org.eclipse.jetty.http.HttpStatus;
2019
import org.eclipse.jetty.server.Handler;
2120
import org.eclipse.jetty.server.Request;
@@ -108,7 +107,13 @@ public boolean handle(Request req, Response res, Callback callback) throws Excep
108107
resImpl.close();
109108
callback.succeeded();
110109
} catch (Throwable t) {
111-
Response.writeError(req, res, callback, t);
110+
if (res.isCommitted()) {
111+
callback.failed(t);
112+
} else {
113+
res.reset();
114+
res.setStatus(HttpStatus.INTERNAL_SERVER_ERROR_500);
115+
callback.succeeded();
116+
}
112117
} finally {
113118
Thread.currentThread().setContextClassLoader(oldContextClassLoader);
114119
}
@@ -121,23 +126,26 @@ private void handleRequest(HttpRequest req, HttpResponse res) {
121126
reqObj = format.deserialize(req, argType);
122127
} catch (Throwable t) {
123128
logger.log(Level.SEVERE, "Failed to parse request for " + function.getClass().getName(), t);
124-
throw new BadMessageException(HttpStatus.BAD_REQUEST_400);
129+
res.setStatusCode(HttpStatus.BAD_REQUEST_400);
130+
return;
125131
}
126132

127133
Object resObj;
128134
try {
129135
resObj = function.apply(reqObj);
130136
} catch (Throwable t) {
131137
logger.log(Level.SEVERE, "Failed to execute " + function.getClass().getName(), t);
132-
throw new BadMessageException(HttpStatus.INTERNAL_SERVER_ERROR_500);
138+
res.setStatusCode(HttpStatus.INTERNAL_SERVER_ERROR_500);
139+
return;
133140
}
134141

135142
try {
136143
format.serialize(resObj, res);
137144
} catch (Throwable t) {
138145
logger.log(
139146
Level.SEVERE, "Failed to serialize response for " + function.getClass().getName(), t);
140-
throw new BadMessageException(HttpStatus.INTERNAL_SERVER_ERROR_500);
147+
res.setStatusCode(HttpStatus.INTERNAL_SERVER_ERROR_500);
148+
return;
141149
}
142150
}
143151

‎invoker/core/src/main/java/com/google/cloud/functions/invoker/http/HttpRequestImpl.java‎

Lines changed: 28 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@
2525
import java.nio.charset.StandardCharsets;
2626
import java.util.ArrayList;
2727
import java.util.Collection;
28-
import java.util.Collections;
2928
import java.util.List;
3029
import java.util.Map;
3130
import java.util.Optional;
@@ -35,6 +34,7 @@
3534
import java.util.regex.Pattern;
3635
import java.util.stream.StreamSupport;
3736
import org.eclipse.jetty.http.HttpField;
37+
import org.eclipse.jetty.http.HttpFields;
3838
import org.eclipse.jetty.http.HttpHeader;
3939
import org.eclipse.jetty.http.MultiPart;
4040
import org.eclipse.jetty.http.MultiPart.Part;
@@ -81,19 +81,25 @@ public Map<String, List<String>> getQueryParameters() {
8181
public Map<String, HttpPart> getParts() {
8282
// TODO initiate reading the parts asynchronously before invocation
8383
String contentType = request.getHeaders().get(HttpHeader.CONTENT_TYPE);
84+
if (contentType == null || !contentType.startsWith("multipart/form-data")) {
85+
throw new IllegalStateException("Content-Type must be multipart/form-data: " + contentType);
86+
}
8487
String boundary = MultiPart.extractBoundary(contentType);
85-
if (boundary != null) {
86-
try {
87-
MultiPartFormData.Parts parts =
88-
MultiPartFormData.from(request, boundary, parser -> parser.parse(request)).get();
89-
return StreamSupport.stream(parts.spliterator(), false)
90-
.map(HttpPartImpl::new)
91-
.collect(toMap(HttpPartImpl::getName, p -> p));
92-
} catch (InterruptedException | ExecutionException e) {
93-
throw new RuntimeException(e);
94-
}
88+
if (boundary == null) {
89+
throw new IllegalStateException("No boundary in content-type: " + contentType);
90+
}
91+
try {
92+
MultiPartFormData.Parts parts =
93+
MultiPartFormData.from(request, boundary, parser -> {
94+
parser.setMaxMemoryFileSize(-1);
95+
return parser.parse(request);
96+
}).get();
97+
return StreamSupport.stream(parts.spliterator(), false)
98+
.map(HttpPartImpl::new)
99+
.collect(toMap(HttpPartImpl::getName, p -> p));
100+
} catch (InterruptedException | ExecutionException e) {
101+
throw new RuntimeException(e);
95102
}
96-
return Collections.emptyMap();
97103
}
98104

99105
@Override
@@ -141,13 +147,15 @@ public BufferedReader getReader() throws IOException {
141147

142148
@Override
143149
public Map<String, List<String>> getHeaders() {
144-
return request.getHeaders().stream()
145-
.collect(
146-
toMap(
147-
HttpField::getName,
148-
HttpField::getValueList,
149-
(a, b) -> b,
150-
() -> new TreeMap<>(String.CASE_INSENSITIVE_ORDER)));
150+
return toStringListMap(request.getHeaders());
151+
}
152+
153+
static Map<String, List<String>> toStringListMap(HttpFields headers) {
154+
Map<String, List<String>> map = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
155+
for (HttpField field : headers) {
156+
map.computeIfAbsent(field.getName(), key -> new ArrayList<>()).add(field.getValue());
157+
}
158+
return map;
151159
}
152160

153161
private static class HttpPartImpl implements HttpPart {
@@ -200,7 +208,7 @@ public BufferedReader getReader() throws IOException {
200208

201209
@Override
202210
public Map<String, List<String>> getHeaders() {
203-
return part.getHeaders().stream().collect(toMap(HttpField::getName, HttpField::getValueList));
211+
return HttpRequestImpl.toStringListMap(part.getHeaders());
204212
}
205213

206214
private static <T> List<T> list(Collection<T> collection) {

‎invoker/core/src/main/java/com/google/cloud/functions/invoker/http/HttpResponseImpl.java‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,6 @@
1414

1515
package com.google.cloud.functions.invoker.http;
1616

17-
import static java.util.stream.Collectors.toMap;
18-
1917
import com.google.cloud.functions.HttpResponse;
2018
import java.io.BufferedOutputStream;
2119
import java.io.BufferedWriter;
@@ -29,8 +27,6 @@
2927
import java.util.Map;
3028
import java.util.Objects;
3129
import java.util.Optional;
32-
import java.util.TreeMap;
33-
import org.eclipse.jetty.http.HttpField;
3430
import org.eclipse.jetty.http.HttpHeader;
3531
import org.eclipse.jetty.io.Content;
3632
import org.eclipse.jetty.io.WriteThroughWriter;
@@ -71,13 +67,7 @@ public void appendHeader(String key, String value) {
7167

7268
@Override
7369
public Map<String, List<String>> getHeaders() {
74-
return response.getHeaders().stream()
75-
.collect(
76-
toMap(
77-
HttpField::getName,
78-
HttpField::getValueList,
79-
(a, b) -> b,
80-
() -> new TreeMap<>(String.CASE_INSENSITIVE_ORDER)));
70+
return HttpRequestImpl.toStringListMap(response.getHeaders());
8171
}
8272

8373
private static <T> List<T> list(Collection<T> collection) {

‎invoker/core/src/main/java/com/google/cloud/functions/invoker/runner/Invoker.java‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@
5555
import org.eclipse.jetty.server.Server;
5656
import org.eclipse.jetty.server.ServerConnector;
5757
import org.eclipse.jetty.server.handler.ContextHandler;
58+
import org.eclipse.jetty.server.handler.ErrorHandler;
5859
import org.eclipse.jetty.util.Callback;
5960
import org.eclipse.jetty.util.thread.QueuedThreadPool;
6061

@@ -278,6 +279,14 @@ private void startServer(boolean join) throws Exception {
278279

279280
QueuedThreadPool pool = new QueuedThreadPool(1024);
280281
server = new Server(pool);
282+
server.setErrorHandler(new ErrorHandler() {
283+
@Override
284+
public boolean handle(Request request, Response response, Callback callback) {
285+
// Suppress error body
286+
callback.succeeded();
287+
return true;
288+
}
289+
});
281290
ServerConnector connector = new ServerConnector(server);
282291
connector.setPort(port);
283292
connector.setReuseAddress(true);
@@ -464,7 +473,8 @@ static NotFoundHandler forServlet(ContextHandler servletHandler) {
464473
@Override
465474
public boolean handle(Request request, Response response, Callback callback) throws Exception {
466475
if (NOT_FOUND_PATHS.contains(request.getHttpURI().getCanonicalPath())) {
467-
Response.writeError(request, response, callback, HttpStatus.NOT_FOUND_404);
476+
response.setStatus(HttpStatus.NOT_FOUND_404);
477+
callback.succeeded();
468478
return true;
469479
}
470480

‎invoker/core/src/test/java/com/google/cloud/functions/invoker/IntegrationTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -259,7 +259,7 @@ public void exceptionHttp() throws Exception {
259259
String exceptionExpectedOutput =
260260
"\"severity\": \"ERROR\", \"logging.googleapis.com/sourceLocation\": {\"file\":"
261261
+ " \"com/google/cloud/functions/invoker/HttpFunctionExecutor.java\", \"method\":"
262-
+ " \"service\"}, \"message\": \"Failed to execute"
262+
+ " \"handle\"}, \"message\": \"Failed to execute"
263263
+ " com.google.cloud.functions.invoker.testfunctions.ExceptionHttp\\n"
264264
+ "java.lang.RuntimeException: exception thrown for test";
265265
testHttpFunction(
@@ -276,7 +276,7 @@ public void exceptionBackground() throws Exception {
276276
String exceptionExpectedOutput =
277277
"\"severity\": \"ERROR\", \"logging.googleapis.com/sourceLocation\": {\"file\":"
278278
+ " \"com/google/cloud/functions/invoker/BackgroundFunctionExecutor.java\", \"method\":"
279-
+ " \"service\"}, \"message\": \"Failed to execute"
279+
+ " \"handle\"}, \"message\": \"Failed to execute"
280280
+ " com.google.cloud.functions.invoker.testfunctions.ExceptionBackground\\n"
281281
+ "java.lang.RuntimeException: exception thrown for test";
282282

‎invoker/core/src/test/java/com/google/cloud/functions/invoker/http/HttpTest.java‎

Lines changed: 25 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import java.util.TreeMap;
3737
import java.util.concurrent.atomic.AtomicReference;
3838
import java.util.stream.Collectors;
39+
import org.eclipse.jetty.client.ByteBufferRequestContent;
3940
import org.eclipse.jetty.client.ContentResponse;
4041
import org.eclipse.jetty.client.HttpClient;
4142
import org.eclipse.jetty.client.MultiPartRequestContent;
@@ -44,8 +45,8 @@
4445
import org.eclipse.jetty.http.HttpHeader;
4546
import org.eclipse.jetty.http.HttpMethod;
4647
import org.eclipse.jetty.http.HttpStatus;
48+
import org.eclipse.jetty.http.HttpStatus.Code;
4749
import org.eclipse.jetty.http.MultiPart;
48-
import org.eclipse.jetty.http.MultiPart.ByteBufferPart;
4950
import org.eclipse.jetty.server.Handler;
5051
import org.eclipse.jetty.server.Request;
5152
import org.eclipse.jetty.server.Response;
@@ -193,7 +194,7 @@ private void httpRequestMethods(
193194
httpClient
194195
.POST(uri)
195196
.headers(m -> {
196-
m.put(HttpHeader.CONTENT_TYPE, "text/plain; charset=utf-8");
197+
m.add(HttpHeader.CONTENT_TYPE, "text/plain; charset=utf-8");
197198
m.add("foo", "bar");
198199
m.add("foo", "baz");
199200
m.add("CaSe-SeNsItIvE", "VaLuE");
@@ -243,14 +244,17 @@ public void multiPartRequest() throws Exception {
243244
httpClient.start();
244245
String uri = "http://localhost:" + serverPort + "/";
245246
MultiPartRequestContent multiPart = new MultiPartRequestContent();
246-
HttpFields.Mutable textHttpFields = HttpFields.build();
247-
textHttpFields.add("foo", "bar");
248-
multiPart.addPart(new MultiPart.ContentSourcePart("test", null, textHttpFields, new StringRequestContent(TEST_BODY)));
249-
HttpFields.Mutable bytesHttpFields = HttpFields.build();
250-
bytesHttpFields.add("foo", "baz");
251-
bytesHttpFields.add("foo", "buh");
247+
HttpFields textHttpFields = HttpFields.build()
248+
.add("foo", "bar");
249+
multiPart.addPart(new MultiPart.ContentSourcePart("text", null, textHttpFields,
250+
new StringRequestContent(TEST_BODY)));
251+
HttpFields.Mutable bytesHttpFields = HttpFields.build()
252+
.add("foo", "baz")
253+
.add("foo", "buh");
252254
assertThat(bytesHttpFields.getValuesList("foo")).containsExactly("baz", "buh");
253-
multiPart.addPart(new ByteBufferPart("binary", "/tmp/binary.x", bytesHttpFields, ByteBuffer.wrap(RANDOM_BYTES)));
255+
multiPart.addPart(new MultiPart.ContentSourcePart("binary", "/tmp/binary.x",
256+
bytesHttpFields, new ByteBufferRequestContent(ByteBuffer.wrap(RANDOM_BYTES))));
257+
multiPart.close();
254258
HttpRequestTest test =
255259
request -> {
256260
// The Content-Type header will also have a boundary=something attribute.
@@ -269,10 +273,9 @@ public void multiPartRequest() throws Exception {
269273
assertThat(bytesPart.getFileName()).hasValue("/tmp/binary.x");
270274
assertThat(bytesPart.getContentLength()).isEqualTo(RANDOM_BYTES.length);
271275
assertThat(bytesPart.getContentType()).hasValue("application/octet-stream");
272-
// We only see ["buh"] here, not ["baz", "buh"], apparently due to a Jetty bug.
273-
// Repeated headers on multi-part content are not a big problem anyway.
274276
List<String> foos = bytesPart.getHeaders().get("foo");
275-
assertThat(foos).contains("buh");
277+
assertThat(foos).containsExactly("baz", "buh");
278+
276279
byte[] bytes = new byte[RANDOM_BYTES.length];
277280
try (InputStream inputStream = bytesPart.getInputStream()) {
278281
assertThat(inputStream.read(bytes)).isEqualTo(bytes.length);
@@ -305,12 +308,18 @@ private HttpRequestHandler(
305308
@Override
306309
public boolean handle(Request request, Response response, Callback callback) {
307310
try {
311+
if (!HttpMethod.POST.is(request.getMethod())) {
312+
response.setStatus(HttpStatus.METHOD_NOT_ALLOWED_405);
313+
callback.succeeded();
314+
return true;
315+
}
316+
308317
testReference.get().test(new HttpRequestImpl(request));
309-
callback.succeeded();
310318
} catch (Throwable t) {
319+
t.printStackTrace();
311320
exceptionReference.set(t);
312-
Response.writeError(request, response, callback, t);
313321
}
322+
callback.succeeded();
314323
return true;
315324
}
316325
}
@@ -452,10 +461,11 @@ private void httpResponseEffects(
452461
response -> response.setStatusCode(HttpStatus.IM_A_TEAPOT_418),
453462
response -> assertThat(response.getStatus()).isEqualTo(HttpStatus.IM_A_TEAPOT_418)),
454463
responseTest(
464+
// reason string cannot be set by application
455465
response -> response.setStatusCode(HttpStatus.IM_A_TEAPOT_418, "Je suis une théière"),
456466
response -> {
457467
assertThat(response.getStatus()).isEqualTo(HttpStatus.IM_A_TEAPOT_418);
458-
assertThat(response.getReason()).isEqualTo("Je suis une théière");
468+
assertThat(response.getReason()).isEqualTo(Code.IM_A_TEAPOT.getMessage());
459469
}),
460470
responseTest(
461471
response -> response.setContentType("application/noddy"),

0 commit comments

Comments
 (0)