From 6cb1b31eb7ff5a3ba5418cdb4183001d5a8ae9ff Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Thu, 17 May 2018 17:31:06 +0900 Subject: [PATCH 01/37] assign UUIDs to all requests and their logs * For each incoming request, assign a unique ID * Include the UUID in all log messages logged by the thread handling the request. * Return the UUID in the response as the `X-Request-Id` header. --- styx-common/pom.xml | 5 ++ .../main/java/com/spotify/styx/api/Api.java | 4 +- .../com/spotify/styx/api/Middlewares.java | 25 +++++++--- .../com/spotify/styx/StringIsValidUUID.java | 50 +++++++++++++++++++ .../com/spotify/styx/api/MiddlewaresTest.java | 36 +++++++++++-- styx-common/src/test/resources/logback.xml | 10 ++++ .../src/test/resources/logback.xml | 2 +- .../src/main/resources/logback.xml | 2 +- 8 files changed, 119 insertions(+), 15 deletions(-) create mode 100644 styx-common/src/test/java/com/spotify/styx/StringIsValidUUID.java create mode 100644 styx-common/src/test/resources/logback.xml diff --git a/styx-common/pom.xml b/styx-common/pom.xml index 72fce53180..0513b197d2 100644 --- a/styx-common/pom.xml +++ b/styx-common/pom.xml @@ -137,5 +137,10 @@ JUnitParams test + + ch.qos.logback + logback-classic + test + diff --git a/styx-common/src/main/java/com/spotify/styx/api/Api.java b/styx-common/src/main/java/com/spotify/styx/api/Api.java index a05f0c874a..e6801489a7 100644 --- a/styx-common/src/main/java/com/spotify/styx/api/Api.java +++ b/styx-common/src/main/java/com/spotify/styx/api/Api.java @@ -21,7 +21,7 @@ package com.spotify.styx.api; import static com.spotify.styx.api.Middlewares.clientValidator; -import static com.spotify.styx.api.Middlewares.exceptionHandler; +import static com.spotify.styx.api.Middlewares.exceptionAndRequestIdHandler; import static com.spotify.styx.api.Middlewares.httpLogger; import com.spotify.apollo.Response; @@ -62,7 +62,7 @@ public static Stream>>> withCommonMiddle return routes.map(r -> r .withMiddleware(httpLogger()) .withMiddleware(clientValidator(clientBlacklistSupplier)) - .withMiddleware(exceptionHandler())); + .withMiddleware(exceptionAndRequestIdHandler())); } private Api() { } diff --git a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java index 9d247a8845..a72418dd96 100644 --- a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java +++ b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java @@ -45,13 +45,14 @@ import java.util.Map; import java.util.Optional; import java.util.Set; -import java.util.concurrent.CompletionException; +import java.util.UUID; import java.util.function.Function; import java.util.function.Supplier; import java.util.stream.Collectors; import okio.ByteString; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.slf4j.MDC; /** * A collection of static methods implementing the apollo Middleware interface, useful for @@ -66,6 +67,9 @@ public final class Middlewares { private static final Set BLACKLISTED_HEADERS = ImmutableSet.of(HttpHeaders.AUTHORIZATION); private static final GoogleIdTokenVerifier GOOGLE_ID_TOKEN_VERIFIER; + private static final String REQUEST_ID = "request-id"; + private static final String X_REQUEST_ID = "X-Request-Id"; + static { final NetHttpTransport transport; try { @@ -126,22 +130,31 @@ public static Middleware>, AsyncHandler }; } - public static Middleware>, AsyncHandler>> exceptionHandler() { + public static Middleware>, AsyncHandler>> exceptionAndRequestIdHandler() { return innerHandler -> requestContext -> { + final String requestId = UUID.randomUUID().toString(); + + MDC.put(REQUEST_ID, requestId); + try { return innerHandler.invoke(requestContext).handle((r, t) -> { + MDC.remove(REQUEST_ID); + + final Response response; if (t != null) { if (t instanceof ResponseException) { - return ((ResponseException) t).getResponse(); + response = ((ResponseException) t).getResponse(); } else { - throw new CompletionException(t); + response = Response.forStatus(Status.INTERNAL_SERVER_ERROR); } } else { - return r; + response = r; } + return response.withHeader(X_REQUEST_ID, requestId); }); } catch (ResponseException e) { - return completedFuture(e.getResponse()); + return completedFuture(e.getResponse() + .withHeader(X_REQUEST_ID, requestId)); } }; } diff --git a/styx-common/src/test/java/com/spotify/styx/StringIsValidUUID.java b/styx-common/src/test/java/com/spotify/styx/StringIsValidUUID.java new file mode 100644 index 0000000000..95d2c7ae12 --- /dev/null +++ b/styx-common/src/test/java/com/spotify/styx/StringIsValidUUID.java @@ -0,0 +1,50 @@ +/* + * -\-\- + * Spotify Styx Scheduler Service + * -- + * Copyright (C) 2018 Spotify AB + * -- + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * -/-/- + */ + +package com.spotify.styx; + +import java.util.UUID; +import org.hamcrest.Description; +import org.hamcrest.Factory; +import org.hamcrest.Matcher; +import org.hamcrest.TypeSafeMatcher; + +public class StringIsValidUUID extends TypeSafeMatcher { + + @Override + protected boolean matchesSafely(String s) { + try { + UUID.fromString(s); + return true; + } catch (IllegalArgumentException e) { + return false; + } + } + + @Override + public void describeTo(Description description) { + description.appendText("a string that is a valid UUID"); + } + + @Factory + public static Matcher isValidUUID() { + return new StringIsValidUUID(); + } +} diff --git a/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java b/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java index 4408483e6c..464ce7fe07 100644 --- a/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java +++ b/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java @@ -20,9 +20,11 @@ package com.spotify.styx.api; +import static com.spotify.apollo.test.unit.ResponseMatchers.hasHeader; import static com.spotify.apollo.test.unit.ResponseMatchers.hasStatus; import static com.spotify.apollo.test.unit.StatusTypeMatchers.belongsToFamily; import static com.spotify.apollo.test.unit.StatusTypeMatchers.withCode; +import static com.spotify.styx.StringIsValidUUID.isValidUUID; import static java.util.concurrent.CompletableFuture.completedFuture; import static java.util.concurrent.TimeUnit.SECONDS; import static org.hamcrest.MatcherAssert.assertThat; @@ -55,6 +57,7 @@ import java.util.function.Supplier; import okio.ByteString; import org.junit.Test; +import org.slf4j.LoggerFactory; /** * Tests Middlewares @@ -249,7 +252,7 @@ public void testAuditLoggingForPutWithBrokenAuthorization() .withPayload(ByteString.encodeUtf8("hello")); when(requestContext.request()).thenReturn(request); - Response response = Middlewares.httpLogger().and(Middlewares.exceptionHandler()) + Response response = Middlewares.httpLogger().and(Middlewares.exceptionAndRequestIdHandler()) .apply(mockInnerHandler(requestContext)) .invoke(requestContext) .toCompletableFuture().get(5, SECONDS); @@ -258,18 +261,41 @@ public void testAuditLoggingForPutWithBrokenAuthorization() } @Test - public void testExceptionHandler() + public void testExceptionAndRequestIdHandlerOnException() throws InterruptedException, ExecutionException, TimeoutException { RequestContext requestContext = mock(RequestContext.class); Request request = Request.forUri("/", "GET"); when(requestContext.request()).thenReturn(request); - Response response = Middlewares.exceptionHandler() - .apply(mockInnerHandler(requestContext, new ResponseException(Response.forStatus(Status.IM_A_TEAPOT)))) + Response response = Middlewares.exceptionAndRequestIdHandler() + .apply(rc -> { + LoggerFactory.getLogger(MiddlewaresTest.class).error("I'm a teapot!"); + throw new ResponseException(Response.forStatus(Status.IM_A_TEAPOT)); + }) .invoke(requestContext) .toCompletableFuture().get(5, SECONDS); assertThat(response, hasStatus(withCode(Status.IM_A_TEAPOT))); + assertThat(response, hasHeader("X-Request-Id", isValidUUID())); + } + + @Test + public void testExceptionAndRequestIdHandlerOnResponse() + throws InterruptedException, ExecutionException, TimeoutException { + RequestContext requestContext = mock(RequestContext.class); + Request request = Request.forUri("/", "GET"); + when(requestContext.request()).thenReturn(request); + + Response response = Middlewares.exceptionAndRequestIdHandler() + .apply(rc -> { + LoggerFactory.getLogger(MiddlewaresTest.class).info("I'm OK!"); + return completedFuture(Response.forStatus(Status.OK)); + }) + .invoke(requestContext) + .toCompletableFuture().get(5, SECONDS); + + assertThat(response, hasStatus(withCode(Status.OK))); + assertThat(response, hasHeader("X-Request-Id", isValidUUID())); } @Test @@ -282,7 +308,7 @@ public void testExceptionHandlerForAsyncException() CompletableFuture failedFuture = new CompletableFuture(); failedFuture.completeExceptionally(new ResponseException(Response.forStatus(Status.IM_A_TEAPOT))); - Response response = Middlewares.exceptionHandler() + Response response = Middlewares.exceptionAndRequestIdHandler() .apply(mockInnerHandler(requestContext, failedFuture)) .invoke(requestContext) .toCompletableFuture().get(5, SECONDS); diff --git a/styx-common/src/test/resources/logback.xml b/styx-common/src/test/resources/logback.xml new file mode 100644 index 0000000000..213cc00c98 --- /dev/null +++ b/styx-common/src/test/resources/logback.xml @@ -0,0 +1,10 @@ + + + + STDOUT + + %d{HH:mm:ss.SSS} [%thread] [%X{request-id}] %-5level %logger{36} - %msg%n + + + + diff --git a/styx-scheduler-service/src/test/resources/logback.xml b/styx-scheduler-service/src/test/resources/logback.xml index e4c5517101..36d0e471e5 100644 --- a/styx-scheduler-service/src/test/resources/logback.xml +++ b/styx-scheduler-service/src/test/resources/logback.xml @@ -3,7 +3,7 @@ STDOUT - %d{HH:mm:ss.SSS} [%thread] %-5level %logger{36} - %msg%n + %d{HH:mm:ss.SSS} [%thread] [%X{request-id}] %-5level %logger{36} - %msg%n diff --git a/styx-standalone-service/src/main/resources/logback.xml b/styx-standalone-service/src/main/resources/logback.xml index 550f137722..2d40afea8d 100644 --- a/styx-standalone-service/src/main/resources/logback.xml +++ b/styx-standalone-service/src/main/resources/logback.xml @@ -3,7 +3,7 @@ STDOUT - %d{HH:mm:ss.SSS} [%thread] %-5level %logger{36} - %msg%n + %d{HH:mm:ss.SSS} [%thread] [%X{request-id}] %-5level %logger{36} - %msg%n From 1347a2979d58fc13086245270a79972ac80767c2 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Fri, 18 May 2018 10:01:47 +0900 Subject: [PATCH 02/37] accept request id from incoming request Allow for a single request id that can be logged end-to-end, client->api->scheduler->api->client. --- .../com/spotify/styx/api/Middlewares.java | 7 ++++++- .../com/spotify/styx/api/MiddlewaresTest.java | 21 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java index a72418dd96..9cebddf944 100644 --- a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java +++ b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java @@ -132,7 +132,12 @@ public static Middleware>, AsyncHandler public static Middleware>, AsyncHandler>> exceptionAndRequestIdHandler() { return innerHandler -> requestContext -> { - final String requestId = UUID.randomUUID().toString(); + + // Accept the request id from the incoming request if present. Otherwise generate one. + final String requestIdHeader = requestContext.request().headers().get(X_REQUEST_ID); + final String requestId = (requestIdHeader != null) + ? requestIdHeader + : UUID.randomUUID().toString(); MDC.put(REQUEST_ID, requestId); diff --git a/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java b/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java index 464ce7fe07..85c06ba85f 100644 --- a/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java +++ b/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java @@ -50,6 +50,7 @@ import java.util.Collections; import java.util.List; import java.util.Optional; +import java.util.UUID; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionStage; import java.util.concurrent.ExecutionException; @@ -297,6 +298,26 @@ public void testExceptionAndRequestIdHandlerOnResponse() assertThat(response, hasStatus(withCode(Status.OK))); assertThat(response, hasHeader("X-Request-Id", isValidUUID())); } + @Test + public void testExceptionAndRequestIdHandlerAcceptsRequestIdHeader() + throws InterruptedException, ExecutionException, TimeoutException { + RequestContext requestContext = mock(RequestContext.class); + String requestId = UUID.randomUUID().toString(); + Request request = Request.forUri("/", "GET") + .withHeader("X-Request-Id", requestId); + when(requestContext.request()).thenReturn(request); + + Response response = Middlewares.exceptionAndRequestIdHandler() + .apply(rc -> { + LoggerFactory.getLogger(MiddlewaresTest.class).info("I'm OK!"); + return completedFuture(Response.forStatus(Status.OK)); + }) + .invoke(requestContext) + .toCompletableFuture().get(5, SECONDS); + + assertThat(response, hasStatus(withCode(Status.OK))); + assertThat(response, hasHeader("X-Request-Id", is(requestId))); + } @Test public void testExceptionHandlerForAsyncException() From 02f440a724e33b86f13cbc59eeca8b5021f7bba9 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Fri, 18 May 2018 10:24:26 +0900 Subject: [PATCH 03/37] cli: print request id on api error --- .../java/com/spotify/styx/cli/CliMain.java | 7 ++-- .../com/spotify/styx/cli/CliMainTest.java | 18 ++++++--- .../styx/client/ApiErrorException.java | 15 +++++++- .../spotify/styx/client/StyxApolloClient.java | 9 +++-- .../styx/client/ApiErrorExceptionTest.java | 38 +++++++++++++++++++ 5 files changed, 74 insertions(+), 13 deletions(-) create mode 100644 styx-client/src/test/java/com/spotify/styx/client/ApiErrorExceptionTest.java diff --git a/styx-cli/src/main/java/com/spotify/styx/cli/CliMain.java b/styx-cli/src/main/java/com/spotify/styx/cli/CliMain.java index c594b5f3b1..fc372a11b4 100644 --- a/styx-cli/src/main/java/com/spotify/styx/cli/CliMain.java +++ b/styx-cli/src/main/java/com/spotify/styx/cli/CliMain.java @@ -298,7 +298,8 @@ private void run() { if (apiError.getCode() == UNAUTHORIZED.code()) { if (!apiError.isAuthenticated()) { cliOutput.printError( - "API error: Unauthorized: Please set up Application Default Credentials or set the " + "API error: Unauthorized: " + apiError.getMessage() + "\n" + + "Please set up Application Default Credentials or set the " + "GOOGLE_APPLICATION_CREDENTIALS env var to point to a file defining the " + "credentials.\n" + "\n" @@ -306,11 +307,11 @@ private void run() { + "\n" + "\t$ gcloud auth application-default login"); } else { - cliOutput.printError("API error: Unauthorized"); + cliOutput.printError("API error: Unauthorized: " + apiError.getMessage()); } throw CliExitException.of(ExitStatus.AuthError); } else { - cliOutput.printError("API error: " + cause.getMessage()); + cliOutput.printError("API error: " + apiError.getMessage()); throw CliExitException.of(ExitStatus.ApiError); } } else if (cause instanceof ClientErrorException) { diff --git a/styx-cli/src/test/java/com/spotify/styx/cli/CliMainTest.java b/styx-cli/src/test/java/com/spotify/styx/cli/CliMainTest.java index f1e34470ed..5a78f528c0 100644 --- a/styx-cli/src/test/java/com/spotify/styx/cli/CliMainTest.java +++ b/styx-cli/src/test/java/com/spotify/styx/cli/CliMainTest.java @@ -64,6 +64,7 @@ import java.util.Collections; import java.util.List; import java.util.Optional; +import java.util.UUID; import java.util.concurrent.CompletableFuture; import junitparams.JUnitParamsRunner; import junitparams.Parameters; @@ -86,6 +87,8 @@ public class CliMainTest { @Mock CliOutput cliOutput; @Mock WorkflowValidator validator; + private String requestId = UUID.randomUUID().toString(); + @Before public void setUp() { MockitoAnnotations.initMocks(this); @@ -488,7 +491,7 @@ public void shouldHandleWorkflowNotFoundWhenEnabling() { .enabled(true) .build(); - final ApiErrorException exception = new ApiErrorException("not found", 404, true); + final ApiErrorException exception = new ApiErrorException("not found", 404, true, requestId); when(client.updateWorkflowState(any(), any(), eq(workflowState))) .thenReturn(exceptionallyCompletedFuture(exception)); @@ -504,7 +507,7 @@ public void shouldHandleWorkflowNotFoundWhenDisabling() { .enabled(false) .build(); - final ApiErrorException exception = new ApiErrorException("not found", 404, true); + final ApiErrorException exception = new ApiErrorException("not found", 404, true, requestId); when(client.updateWorkflowState(any(), any(), eq(workflowState))) .thenReturn(exceptionallyCompletedFuture(exception)); @@ -552,7 +555,7 @@ public void testClientErrorDebug() { @Test public void testApiError() { - final ApiErrorException exception = new ApiErrorException("bar failure", 500, true); + final ApiErrorException exception = new ApiErrorException("bar failure", 500, true, requestId); when(client.triggerWorkflowInstance(any(), any(), any())) .thenReturn(exceptionallyCompletedFuture(exception)); @@ -601,8 +604,9 @@ public void testUnknownError() { @Test public void testMissingCredentialsHelpMessage() { + final ApiErrorException apiError = new ApiErrorException("foo", 401, false, requestId); when(client.triggerWorkflowInstance(any(), any(), any())) - .thenReturn(exceptionallyCompletedFuture(new ApiErrorException("foo", 401, false))); + .thenReturn(exceptionallyCompletedFuture(apiError)); try { CliMain.run(cliContext, "t", "foo", "bar", "2017-01-02"); @@ -612,12 +616,14 @@ public void testMissingCredentialsHelpMessage() { } verify(cliOutput).printError(contains("gcloud auth application-default login")); + verify(cliOutput).printError(contains(apiError.getMessage())); } @Test public void testUnauthorizedMessage() { + final ApiErrorException apiError = new ApiErrorException("foo", 401, true, requestId); when(client.triggerWorkflowInstance(any(), any(), any())) - .thenReturn(exceptionallyCompletedFuture(new ApiErrorException("foo", 401, true))); + .thenReturn(exceptionallyCompletedFuture(apiError)); try { CliMain.run(cliContext, "t", "foo", "bar", "2017-01-02"); @@ -626,7 +632,7 @@ public void testUnauthorizedMessage() { assertThat(e.status(), is(ExitStatus.AuthError)); } - verify(cliOutput).printError("API error: Unauthorized"); + verify(cliOutput).printError("API error: Unauthorized: " + apiError.getMessage()); } @Test diff --git a/styx-client/src/main/java/com/spotify/styx/client/ApiErrorException.java b/styx-client/src/main/java/com/spotify/styx/client/ApiErrorException.java index e5edb11561..fdf3fea661 100644 --- a/styx-client/src/main/java/com/spotify/styx/client/ApiErrorException.java +++ b/styx-client/src/main/java/com/spotify/styx/client/ApiErrorException.java @@ -20,6 +20,8 @@ package com.spotify.styx.client; +import java.util.Objects; + /** * Exception used in case of API errors, i.e. API requests whose response has status code * different than 2xx. @@ -28,11 +30,13 @@ public class ApiErrorException extends RuntimeException { private final int code; private final boolean authenticated; + private final String requestId; - public ApiErrorException(String message, int code, boolean authenticated) { + public ApiErrorException(String message, int code, boolean authenticated, String requestId) { super(message); this.code = code; this.authenticated = authenticated; + this.requestId = Objects.requireNonNull(requestId, "requestId"); } public int getCode() { @@ -42,4 +46,13 @@ public int getCode() { public boolean isAuthenticated() { return authenticated; } + + public String getRequestId() { + return requestId; + } + + @Override + public String getMessage() { + return super.getMessage() + " (Request ID: " + requestId + ")"; + } } diff --git a/styx-client/src/main/java/com/spotify/styx/client/StyxApolloClient.java b/styx-client/src/main/java/com/spotify/styx/client/StyxApolloClient.java index 532099b053..3c94f93117 100644 --- a/styx-client/src/main/java/com/spotify/styx/client/StyxApolloClient.java +++ b/styx-client/src/main/java/com/spotify/styx/client/StyxApolloClient.java @@ -58,6 +58,7 @@ import java.util.List; import java.util.Objects; import java.util.Optional; +import java.util.UUID; import java.util.concurrent.CompletionStage; import okhttp3.HttpUrl; import okhttp3.HttpUrl.Builder; @@ -418,9 +419,10 @@ private CompletionStage executeRequest(final Request request, } private Request decorateRequest( - final Request request, final Optional authToken) { + final Request request, String requestId, final Optional authToken) { return request .withHeader("User-Agent", STYX_CLIENT_VERSION) + .withHeader("X-Request-Id", requestId) .withTtl(TTL) .withHeaders(authToken .map(t -> ImmutableMap.of("Authorization", "Bearer " + t)) @@ -436,7 +438,8 @@ private CompletionStage> executeRequest(final Request reque return CompletableFutures.exceptionallyCompletedFuture( new ClientErrorException("Authentication failure: " + e.getMessage(), e)); } - return client.send(decorateRequest(request, authToken)).handle((response, e) -> { + final String requestId = UUID.randomUUID().toString(); + return client.send(decorateRequest(request, requestId, authToken)).handle((response, e) -> { if (e != null) { throw new ClientErrorException("Request failed: " + request.method() + " " + request.uri(), e); } else { @@ -445,7 +448,7 @@ private CompletionStage> executeRequest(final Request reque return response; default: final String message = response.status().code() + " " + response.status().reasonPhrase(); - throw new ApiErrorException(message, response.status().code(), authToken.isPresent()); + throw new ApiErrorException(message, response.status().code(), authToken.isPresent(), requestId); } } }); diff --git a/styx-client/src/test/java/com/spotify/styx/client/ApiErrorExceptionTest.java b/styx-client/src/test/java/com/spotify/styx/client/ApiErrorExceptionTest.java new file mode 100644 index 0000000000..85a3590846 --- /dev/null +++ b/styx-client/src/test/java/com/spotify/styx/client/ApiErrorExceptionTest.java @@ -0,0 +1,38 @@ +/* + * -\-\- + * Spotify Styx Scheduler Service + * -- + * Copyright (C) 2018 Spotify AB + * -- + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * -/-/- + */ + +package com.spotify.styx.client; + +import static org.hamcrest.Matchers.is; +import static org.junit.Assert.assertThat; + +import java.util.UUID; +import org.junit.Test; + +public class ApiErrorExceptionTest { + + private static final String REQUEST_ID = UUID.randomUUID().toString(); + private static final ApiErrorException EXCEPTION = new ApiErrorException("fubared!", 4711, true, REQUEST_ID); + + @Test + public void getMessage() { + assertThat(EXCEPTION.getMessage(), is("fubared! (Request ID: " + REQUEST_ID + ")")); + } +} \ No newline at end of file From 77176b11c6765d63f6699860e353aa9c0283d1c6 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Fri, 18 May 2018 12:49:08 +0900 Subject: [PATCH 04/37] propagate request IDs (MDC) to tasks Ensure that logging happening in tasks running on thread pools has the request ID of the incoming request. --- .../styx/storage/DatastoreStorage.java | 10 +- .../java/com/spotify/styx/util/MDCUtil.java | 125 ++++++++++++++++++ .../com/spotify/styx/util/MDCUtilTest.java | 102 ++++++++++++++ .../styx/state/QueuedStateManager.java | 9 +- .../java/com/spotify/styx/state/Striping.java | 26 ++-- .../com/spotify/styx/state/StripingTest.java | 73 ++++++++++ 6 files changed, 331 insertions(+), 14 deletions(-) create mode 100644 styx-common/src/main/java/com/spotify/styx/util/MDCUtil.java create mode 100644 styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java create mode 100644 styx-scheduler-service/src/test/java/com/spotify/styx/state/StripingTest.java diff --git a/styx-common/src/main/java/com/spotify/styx/storage/DatastoreStorage.java b/styx-common/src/main/java/com/spotify/styx/storage/DatastoreStorage.java index 8bd204d02f..4e4558e11d 100644 --- a/styx-common/src/main/java/com/spotify/styx/storage/DatastoreStorage.java +++ b/styx-common/src/main/java/com/spotify/styx/storage/DatastoreStorage.java @@ -21,6 +21,7 @@ package com.spotify.styx.storage; import static com.spotify.styx.serialization.Json.OBJECT_MAPPER; +import static com.spotify.styx.util.MDCUtil.withMDC; import static com.spotify.styx.util.ShardedCounter.KIND_COUNTER_LIMIT; import static com.spotify.styx.util.ShardedCounter.KIND_COUNTER_SHARD; import static com.spotify.styx.util.ShardedCounter.NUM_SHARDS; @@ -73,6 +74,7 @@ import com.spotify.styx.state.RunState.State; import com.spotify.styx.state.StateData; import com.spotify.styx.util.FnWithException; +import com.spotify.styx.util.MDCUtil; import com.spotify.styx.util.ResourceNotFoundException; import com.spotify.styx.util.StreamUtil; import com.spotify.styx.util.TimeUtil; @@ -346,7 +348,7 @@ public Map workflows(Set workflowIds) { final Iterable> batches = Iterables.partition(workflowIds, MAX_NUMBER_OF_ENTITIES_IN_ONE_BATCH_READ); return StreamSupport.stream(batches.spliterator(), false) - .map(batch -> forkJoinPool.submit(() -> this.getBatchOfWorkflows(batch))) + .map(batch -> forkJoinPool.submit(withMDC(() -> this.getBatchOfWorkflows(batch)))) // `collect and stream` is crucial to make tasks running in parallel, otherwise they will // be processed sequentially. Without `collect`, it will try to submit and wait for each task // while iterating through the stream. This is somewhat subtle, so think twice. @@ -404,11 +406,11 @@ public List workflows(String componentId) throws IOException { Map readActiveStates() throws IOException { // Strongly read active state keys from index shards final List keys = activeWorkflowInstanceIndexShardKeys(datastore.newKeyFactory()).stream() - .map(key -> forkJoinPool.submit(() -> + .map(key -> forkJoinPool.submit(withMDC(() -> datastore.run(Query.newEntityQueryBuilder() .setFilter(PropertyFilter.hasAncestor(key)) .setKind(KIND_ACTIVE_WORKFLOW_INSTANCE_INDEX_SHARD_ENTRY) - .build()))) + .build())))) .collect(toList()).stream() // collect here to execute batch reads in parallel .flatMap(task -> StreamUtil.stream(task.join())) .map(entity -> entity.getKey().getName()) @@ -417,7 +419,7 @@ Map readActiveStates() throws IOException { // Strongly consistently read values for the above keys return Lists.partition(keys, MAX_NUMBER_OF_ENTITIES_IN_ONE_BATCH_READ).stream() - .map(batch -> forkJoinPool.submit(() -> this.readRunStateBatch(batch))) + .map(batch -> forkJoinPool.submit(withMDC(() -> this.readRunStateBatch(batch)))) .collect(toList()).stream() // collect here to execute batch reads in parallel .flatMap(task -> task.join().stream()) .collect(toMap(RunState::workflowInstance, Function.identity())); diff --git a/styx-common/src/main/java/com/spotify/styx/util/MDCUtil.java b/styx-common/src/main/java/com/spotify/styx/util/MDCUtil.java new file mode 100644 index 0000000000..27db2b1f58 --- /dev/null +++ b/styx-common/src/main/java/com/spotify/styx/util/MDCUtil.java @@ -0,0 +1,125 @@ +/* + * -\-\- + * Spotify Styx Scheduler Service + * -- + * Copyright (C) 2018 Spotify AB + * -- + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * -/-/- + */ + +package com.spotify.styx.util; + +import java.util.Map; +import java.util.concurrent.Callable; +import java.util.concurrent.Executor; +import java.util.concurrent.ForkJoinPool; +import java.util.function.Function; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.slf4j.MDC; + +/** + * Utilities for propagating a {@link MDC} to tasks running on thread pools. + */ +public class MDCUtil { + + private static final Logger log = LoggerFactory.getLogger(MDCUtil.class); + + private MDCUtil() { + throw new UnsupportedOperationException(); + } + + /** + * Runs tasks on the shared FJP, with MDC propagated from the calling thread. + */ + public static Executor withMDC() { + return runnable -> ForkJoinPool.commonPool().execute(withMDC(runnable)); + } + + /** + * Runs tasks on the supplied executor, with MDC propagated from the calling thread. + */ + public static Executor withMDC(Executor executor) { + return runnable -> executor.execute(withMDC(runnable)); + } + + /** + * Wrap a {@link Runnable} to use the {@link MDC} from the calling thread. + */ + public static Runnable withMDC(Runnable r) { + return new Runnable() { + + private Map contextMap = safeCopyOfContextMap(); + + @Override + public void run() { + safeSetContextMap(contextMap); + try { + r.run(); + } finally { + safeResetContextMap(); + } + } + }; + } + + /** + * Wrap a {@link Callable} to use the {@link MDC} from the calling thread. + */ + public static Callable withMDC(Callable r) { + return new Callable() { + + private Map contextMap = safeCopyOfContextMap(); + + @Override + public V call() throws Exception { + safeSetContextMap(contextMap); + try { + return r.call(); + } finally { + safeResetContextMap(); + } + } + }; + } + + private static Map safeCopyOfContextMap() { + try { + return MDC.getCopyOfContextMap(); + } catch (Exception e) { + log.warn("Failed to copy MDC"); + return null; + } + } + + private static void safeSetContextMap(Map contextMap) { + try { + if (contextMap != null) { + MDC.setContextMap(contextMap); + } else { + MDC.clear(); + } + } catch (Exception e) { + log.warn("Failed to set MDC"); + } + } + + private static void safeResetContextMap() { + try { + MDC.clear(); + } catch (Exception e) { + log.warn("Failed to reset MDC"); + } + } +} diff --git a/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java b/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java new file mode 100644 index 0000000000..464a46d44b --- /dev/null +++ b/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java @@ -0,0 +1,102 @@ +/* + * -\-\- + * Spotify Styx Scheduler Service + * -- + * Copyright (C) 2018 Spotify AB + * -- + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * -/-/- + */ + +package com.spotify.styx.util; + +import static com.spotify.styx.util.MDCUtil.withMDC; +import static java.util.Collections.emptyMap; +import static org.hamcrest.Matchers.anyOf; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.nullValue; +import static org.junit.Assert.assertThat; + +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.ForkJoinPool; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.slf4j.MDC; + +public class MDCUtilTest { + + @Before + public void setUp() throws Exception { + MDC.clear(); + } + + @After + public void tearDown() throws Exception { + MDC.clear(); + } + + @Test + public void withMDCRunnable() throws ExecutionException, InterruptedException { + MDC.put("foo", "bar"); + Executors.newSingleThreadExecutor() + .submit(withMDC(() -> assertThat(MDC.get("foo"), is("bar")))) + .get(); + } + + @Test + public void withMDCCallable() throws ExecutionException, InterruptedException { + MDC.put("foo", "bar"); + Executors.newSingleThreadExecutor() + .submit(withMDC(() -> { + assertThat(MDC.get("foo"), is("bar")); + return null; + })).get(); + } + + @Test + public void withMDCCommonPool() throws ExecutionException, InterruptedException { + MDC.put("foo", "bar"); + CompletableFuture.runAsync( + () -> assertThat(MDC.get("foo"), is("bar")), + withMDC()) + // Later stages should also have the MDC applied + .thenRun(() -> assertThat(MDC.get("foo"), is("bar"))) + .get(); + + // MDC should not leak + ForkJoinPool.commonPool() + .submit(() -> assertThat(MDC.getCopyOfContextMap(), is(anyOf(nullValue(), is(emptyMap()))))) + .get(); + } + + @Test + public void withMDCExecutor() throws ExecutionException, InterruptedException { + MDC.put("foo", "bar"); + final ExecutorService executor = Executors.newSingleThreadExecutor(); + CompletableFuture.runAsync( + () -> assertThat(MDC.get("foo"), is("bar")), + withMDC(executor)) + // Later stages should also have the MDC applied + .thenRun(() -> assertThat(MDC.get("foo"), is("bar"))) + .get(); + + // MDC should not leak + executor + .submit(() -> assertThat(MDC.getCopyOfContextMap(), is(anyOf(nullValue(), is(emptyMap()))))) + .get(); + } +} \ No newline at end of file diff --git a/styx-scheduler-service/src/main/java/com/spotify/styx/state/QueuedStateManager.java b/styx-scheduler-service/src/main/java/com/spotify/styx/state/QueuedStateManager.java index 5d49d75759..7394e9b604 100644 --- a/styx-scheduler-service/src/main/java/com/spotify/styx/state/QueuedStateManager.java +++ b/styx-scheduler-service/src/main/java/com/spotify/styx/state/QueuedStateManager.java @@ -21,6 +21,7 @@ package com.spotify.styx.state; import static com.spotify.styx.state.StateUtil.isConsumingResources; +import static com.spotify.styx.util.MDCUtil.withMDC; import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.stream.Collectors.toList; import static java.util.stream.Collectors.toSet; @@ -39,6 +40,7 @@ import com.spotify.styx.util.CounterCapacityException; import com.spotify.styx.util.EventUtil; import com.spotify.styx.util.IsClosedException; +import com.spotify.styx.util.MDCUtil; import com.spotify.styx.util.ShardedCounter; import com.spotify.styx.util.Time; import eu.javaspecialists.tjsn.concurrency.stripedexecutor.StripedExecutorService; @@ -49,9 +51,12 @@ import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.concurrent.AbstractExecutorService; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionStage; import java.util.concurrent.Executor; +import java.util.concurrent.ForkJoinPool; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.LongAdder; import java.util.function.BiConsumer; import javaslang.Tuple; @@ -116,7 +121,7 @@ public CompletionStage trigger(WorkflowInstance workflowInstance, Trigger // TODO: optional retry on transaction conflict - return CompletableFuture.runAsync(() -> initialize(workflowInstance)).thenCompose((ignore) -> { + return CompletableFuture.runAsync(() -> initialize(workflowInstance), withMDC()).thenCompose((ignore) -> { final Event event = Event.triggerExecution(workflowInstance, trigger); try { return receive(event); @@ -322,7 +327,7 @@ private void postTransition(SequenceEvent sequenceEvent, RunState runState) { // Publish event try { - eventConsumerExecutor.execute(() -> eventConsumer.accept(sequenceEvent, runState)); + eventConsumerExecutor.execute(withMDC(() -> eventConsumer.accept(sequenceEvent, runState))); } catch (Exception e) { LOG.warn("Error while consuming event {}", sequenceEvent, e); } diff --git a/styx-scheduler-service/src/main/java/com/spotify/styx/state/Striping.java b/styx-scheduler-service/src/main/java/com/spotify/styx/state/Striping.java index 3ae819258b..74ffc12976 100644 --- a/styx-scheduler-service/src/main/java/com/spotify/styx/state/Striping.java +++ b/styx-scheduler-service/src/main/java/com/spotify/styx/state/Striping.java @@ -20,6 +20,7 @@ package com.spotify.styx.state; +import com.spotify.styx.util.MDCUtil; import eu.javaspecialists.tjsn.concurrency.stripedexecutor.StripedExecutorService; import eu.javaspecialists.tjsn.concurrency.stripedexecutor.StripedRunnable; import java.util.concurrent.CompletableFuture; @@ -34,9 +35,21 @@ private Striping() { throw new UnsupportedOperationException(); } - public static CompletableFuture supplyAsyncStriped(Supplier supplier, Object stripe, + static CompletableFuture supplyAsyncStriped(Supplier supplier, Object stripe, StripedExecutorService executor) { - final CompletableFuture f = new CompletableFuture<>(); + + final CompletableFuture future = new CompletableFuture<>(); + + // This runnable must be created here on the calling thread + final Runnable runnable = MDCUtil.withMDC(() -> { + try { + future.complete(supplier.get()); + } catch (Throwable e) { + future.completeExceptionally(e); + } + }); + + // The StripedRunnable cannot be wrapped using withMDC as that would hide the stripe executor.execute(new StripedRunnable() { @Override public Object getStripe() { @@ -45,13 +58,10 @@ public Object getStripe() { @Override public void run() { - try { - f.complete(supplier.get()); - } catch (Throwable e) { - f.completeExceptionally(e); - } + runnable.run(); } }); - return f; + + return future; } } diff --git a/styx-scheduler-service/src/test/java/com/spotify/styx/state/StripingTest.java b/styx-scheduler-service/src/test/java/com/spotify/styx/state/StripingTest.java new file mode 100644 index 0000000000..5cea7a2742 --- /dev/null +++ b/styx-scheduler-service/src/test/java/com/spotify/styx/state/StripingTest.java @@ -0,0 +1,73 @@ +/* + * -\-\- + * Spotify Styx Scheduler Service + * -- + * Copyright (C) 2018 Spotify AB + * -- + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * -/-/- + */ + +package com.spotify.styx.state; + +import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.is; +import static org.junit.Assert.assertThat; +import static org.mockito.Mockito.doCallRealMethod; + +import eu.javaspecialists.tjsn.concurrency.stripedexecutor.StripedExecutorService; +import eu.javaspecialists.tjsn.concurrency.stripedexecutor.StripedRunnable; +import java.util.concurrent.ExecutionException; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; +import org.mockito.Spy; +import org.mockito.runners.MockitoJUnitRunner; +import org.slf4j.MDC; + +@RunWith(MockitoJUnitRunner.class) +public class StripingTest { + + @Spy StripedExecutorService stripedExecutorService = new StripedExecutorService(); + @Captor ArgumentCaptor runnableArgumentCaptor; + + @Before + public void setUp() throws Exception { + MDC.clear(); + } + + @After + public void tearDown() throws Exception { + stripedExecutorService.shutdownNow(); + MDC.clear(); + } + + @Test + public void supplyAsyncStriped() throws ExecutionException, InterruptedException { + doCallRealMethod().when(stripedExecutorService).execute(runnableArgumentCaptor.capture()); + + MDC.put("foo", "bar"); + + Striping.supplyAsyncStriped(() -> { + assertThat(MDC.get("foo"), is("bar")); + return "foo"; + }, "baz", stripedExecutorService).get(); + + final Runnable runnable = runnableArgumentCaptor.getValue(); + assertThat(runnable, instanceOf(StripedRunnable.class)); + assertThat(((StripedRunnable) runnable).getStripe(), is("baz")); + } +} \ No newline at end of file From 5b7219907dc225fa3a4424138aec08dc12b28a1f Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Fri, 18 May 2018 12:50:57 +0900 Subject: [PATCH 05/37] fix license headers --- .../java/com/spotify/styx/client/ApiErrorExceptionTest.java | 6 +++--- .../src/main/java/com/spotify/styx/util/MDCUtil.java | 4 ++-- .../src/test/java/com/spotify/styx/util/MDCUtilTest.java | 6 +++--- .../src/test/java/com/spotify/styx/state/StripingTest.java | 6 +++--- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/styx-client/src/test/java/com/spotify/styx/client/ApiErrorExceptionTest.java b/styx-client/src/test/java/com/spotify/styx/client/ApiErrorExceptionTest.java index 85a3590846..c5755b3049 100644 --- a/styx-client/src/test/java/com/spotify/styx/client/ApiErrorExceptionTest.java +++ b/styx-client/src/test/java/com/spotify/styx/client/ApiErrorExceptionTest.java @@ -7,9 +7,9 @@ * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -35,4 +35,4 @@ public class ApiErrorExceptionTest { public void getMessage() { assertThat(EXCEPTION.getMessage(), is("fubared! (Request ID: " + REQUEST_ID + ")")); } -} \ No newline at end of file +} diff --git a/styx-common/src/main/java/com/spotify/styx/util/MDCUtil.java b/styx-common/src/main/java/com/spotify/styx/util/MDCUtil.java index 27db2b1f58..1978b17193 100644 --- a/styx-common/src/main/java/com/spotify/styx/util/MDCUtil.java +++ b/styx-common/src/main/java/com/spotify/styx/util/MDCUtil.java @@ -7,9 +7,9 @@ * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. diff --git a/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java b/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java index 464a46d44b..0249de6969 100644 --- a/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java +++ b/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java @@ -7,9 +7,9 @@ * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -99,4 +99,4 @@ public void withMDCExecutor() throws ExecutionException, InterruptedException { .submit(() -> assertThat(MDC.getCopyOfContextMap(), is(anyOf(nullValue(), is(emptyMap()))))) .get(); } -} \ No newline at end of file +} diff --git a/styx-scheduler-service/src/test/java/com/spotify/styx/state/StripingTest.java b/styx-scheduler-service/src/test/java/com/spotify/styx/state/StripingTest.java index 5cea7a2742..b59d0a15e0 100644 --- a/styx-scheduler-service/src/test/java/com/spotify/styx/state/StripingTest.java +++ b/styx-scheduler-service/src/test/java/com/spotify/styx/state/StripingTest.java @@ -7,9 +7,9 @@ * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -70,4 +70,4 @@ public void supplyAsyncStriped() throws ExecutionException, InterruptedException assertThat(runnable, instanceOf(StripedRunnable.class)); assertThat(((StripedRunnable) runnable).getStripe(), is("baz")); } -} \ No newline at end of file +} From f5fde4edf83e7ed965da33c03c307aead0a71f06 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Fri, 18 May 2018 12:53:22 +0900 Subject: [PATCH 06/37] remove unused imports --- .../main/java/com/spotify/styx/storage/DatastoreStorage.java | 1 - styx-common/src/main/java/com/spotify/styx/util/MDCUtil.java | 1 - .../main/java/com/spotify/styx/state/QueuedStateManager.java | 4 ---- 3 files changed, 6 deletions(-) diff --git a/styx-common/src/main/java/com/spotify/styx/storage/DatastoreStorage.java b/styx-common/src/main/java/com/spotify/styx/storage/DatastoreStorage.java index 4e4558e11d..591c1ddcd1 100644 --- a/styx-common/src/main/java/com/spotify/styx/storage/DatastoreStorage.java +++ b/styx-common/src/main/java/com/spotify/styx/storage/DatastoreStorage.java @@ -74,7 +74,6 @@ import com.spotify.styx.state.RunState.State; import com.spotify.styx.state.StateData; import com.spotify.styx.util.FnWithException; -import com.spotify.styx.util.MDCUtil; import com.spotify.styx.util.ResourceNotFoundException; import com.spotify.styx.util.StreamUtil; import com.spotify.styx.util.TimeUtil; diff --git a/styx-common/src/main/java/com/spotify/styx/util/MDCUtil.java b/styx-common/src/main/java/com/spotify/styx/util/MDCUtil.java index 1978b17193..76fff2a0a3 100644 --- a/styx-common/src/main/java/com/spotify/styx/util/MDCUtil.java +++ b/styx-common/src/main/java/com/spotify/styx/util/MDCUtil.java @@ -24,7 +24,6 @@ import java.util.concurrent.Callable; import java.util.concurrent.Executor; import java.util.concurrent.ForkJoinPool; -import java.util.function.Function; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.slf4j.MDC; diff --git a/styx-scheduler-service/src/main/java/com/spotify/styx/state/QueuedStateManager.java b/styx-scheduler-service/src/main/java/com/spotify/styx/state/QueuedStateManager.java index 7394e9b604..2114707e6f 100644 --- a/styx-scheduler-service/src/main/java/com/spotify/styx/state/QueuedStateManager.java +++ b/styx-scheduler-service/src/main/java/com/spotify/styx/state/QueuedStateManager.java @@ -40,7 +40,6 @@ import com.spotify.styx.util.CounterCapacityException; import com.spotify.styx.util.EventUtil; import com.spotify.styx.util.IsClosedException; -import com.spotify.styx.util.MDCUtil; import com.spotify.styx.util.ShardedCounter; import com.spotify.styx.util.Time; import eu.javaspecialists.tjsn.concurrency.stripedexecutor.StripedExecutorService; @@ -51,12 +50,9 @@ import java.util.Objects; import java.util.Optional; import java.util.Set; -import java.util.concurrent.AbstractExecutorService; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionStage; import java.util.concurrent.Executor; -import java.util.concurrent.ForkJoinPool; -import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.LongAdder; import java.util.function.BiConsumer; import javaslang.Tuple; From 4f142e9f742b09c23a457b738a10441d159c259f Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Fri, 18 May 2018 14:30:55 +0900 Subject: [PATCH 07/37] make codacy happy --- .../com/spotify/styx/util/MDCUtilTest.java | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java b/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java index 0249de6969..63266cf435 100644 --- a/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java +++ b/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java @@ -39,6 +39,8 @@ public class MDCUtilTest { + private static final ExecutorService EXECUTOR = Executors.newSingleThreadExecutor(); + @Before public void setUp() throws Exception { MDC.clear(); @@ -52,19 +54,16 @@ public void tearDown() throws Exception { @Test public void withMDCRunnable() throws ExecutionException, InterruptedException { MDC.put("foo", "bar"); - Executors.newSingleThreadExecutor() - .submit(withMDC(() -> assertThat(MDC.get("foo"), is("bar")))) - .get(); + final CompletableFuture value = new CompletableFuture<>(); + EXECUTOR.submit(withMDC(() -> value.complete(MDC.get("foo")))); + assertThat(value.get(), is("bar")); } @Test public void withMDCCallable() throws ExecutionException, InterruptedException { MDC.put("foo", "bar"); - Executors.newSingleThreadExecutor() - .submit(withMDC(() -> { - assertThat(MDC.get("foo"), is("bar")); - return null; - })).get(); + final String value = EXECUTOR.submit(withMDC(() -> MDC.get("foo"))).get(); + assertThat(value, is("bar")); } @Test @@ -86,16 +85,15 @@ public void withMDCCommonPool() throws ExecutionException, InterruptedException @Test public void withMDCExecutor() throws ExecutionException, InterruptedException { MDC.put("foo", "bar"); - final ExecutorService executor = Executors.newSingleThreadExecutor(); CompletableFuture.runAsync( () -> assertThat(MDC.get("foo"), is("bar")), - withMDC(executor)) + withMDC(EXECUTOR)) // Later stages should also have the MDC applied .thenRun(() -> assertThat(MDC.get("foo"), is("bar"))) .get(); // MDC should not leak - executor + EXECUTOR .submit(() -> assertThat(MDC.getCopyOfContextMap(), is(anyOf(nullValue(), is(emptyMap()))))) .get(); } From a92f122d030df18f5860463e64931d18664382d4 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Fri, 18 May 2018 14:31:48 +0900 Subject: [PATCH 08/37] do not reuse executor between tests --- .../test/java/com/spotify/styx/util/MDCUtilTest.java | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java b/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java index 63266cf435..0b1384e198 100644 --- a/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java +++ b/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java @@ -39,15 +39,17 @@ public class MDCUtilTest { - private static final ExecutorService EXECUTOR = Executors.newSingleThreadExecutor(); + private ExecutorService executor; @Before public void setUp() throws Exception { MDC.clear(); + executor = Executors.newSingleThreadExecutor(); } @After public void tearDown() throws Exception { + executor.shutdownNow(); MDC.clear(); } @@ -55,14 +57,14 @@ public void tearDown() throws Exception { public void withMDCRunnable() throws ExecutionException, InterruptedException { MDC.put("foo", "bar"); final CompletableFuture value = new CompletableFuture<>(); - EXECUTOR.submit(withMDC(() -> value.complete(MDC.get("foo")))); + executor.submit(withMDC(() -> value.complete(MDC.get("foo")))); assertThat(value.get(), is("bar")); } @Test public void withMDCCallable() throws ExecutionException, InterruptedException { MDC.put("foo", "bar"); - final String value = EXECUTOR.submit(withMDC(() -> MDC.get("foo"))).get(); + final String value = executor.submit(withMDC(() -> MDC.get("foo"))).get(); assertThat(value, is("bar")); } @@ -87,13 +89,13 @@ public void withMDCExecutor() throws ExecutionException, InterruptedException { MDC.put("foo", "bar"); CompletableFuture.runAsync( () -> assertThat(MDC.get("foo"), is("bar")), - withMDC(EXECUTOR)) + withMDC(executor)) // Later stages should also have the MDC applied .thenRun(() -> assertThat(MDC.get("foo"), is("bar"))) .get(); // MDC should not leak - EXECUTOR + executor .submit(() -> assertThat(MDC.getCopyOfContextMap(), is(anyOf(nullValue(), is(emptyMap()))))) .get(); } From 0da247631a408794ee2569c58d1628404c46a408 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Fri, 18 May 2018 16:02:47 +0900 Subject: [PATCH 09/37] api/scheduler: return failure cause for 500 errors --- .../com/spotify/styx/api/Middlewares.java | 29 +++++++- .../com/spotify/styx/api/MiddlewaresTest.java | 72 ++++++++++++++++++- 2 files changed, 97 insertions(+), 4 deletions(-) diff --git a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java index 9cebddf944..2e0046d336 100644 --- a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java +++ b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java @@ -20,6 +20,7 @@ package com.spotify.styx.api; +import static com.spotify.apollo.Status.INTERNAL_SERVER_ERROR; import static com.spotify.styx.serialization.Json.OBJECT_MAPPER; import static java.util.concurrent.CompletableFuture.completedFuture; @@ -29,6 +30,9 @@ import com.google.api.client.googleapis.javanet.GoogleNetHttpTransport; import com.google.api.client.googleapis.util.Utils; import com.google.api.client.http.javanet.NetHttpTransport; +import com.google.common.base.CharMatcher; +import com.google.common.base.Strings; +import com.google.common.base.Throwables; import com.google.common.collect.ImmutableSet; import com.google.common.net.HttpHeaders; import com.spotify.apollo.Request; @@ -107,7 +111,7 @@ private Middlewares() { .withHeader("Content-Type", "application/json"); } catch (JsonProcessingException e) { return Response.forStatus( - Status.INTERNAL_SERVER_ERROR.withReasonPhrase( + INTERNAL_SERVER_ERROR.withReasonPhrase( "Failed to serialize response " + e.getMessage())); } }); @@ -150,7 +154,8 @@ public static Middleware>, AsyncHandler if (t instanceof ResponseException) { response = ((ResponseException) t).getResponse(); } else { - response = Response.forStatus(Status.INTERNAL_SERVER_ERROR); + response = Response.forStatus(INTERNAL_SERVER_ERROR + .withReasonPhrase(internalServerErrorReason(t))); } } else { response = r; @@ -160,10 +165,30 @@ public static Middleware>, AsyncHandler } catch (ResponseException e) { return completedFuture(e.getResponse() .withHeader(X_REQUEST_ID, requestId)); + } catch (Exception e) { + return completedFuture(Response.forStatus(INTERNAL_SERVER_ERROR + .withReasonPhrase(internalServerErrorReason(e))) + .withHeader(X_REQUEST_ID, requestId)); } }; } + private static String internalServerErrorReason(Throwable t) { + final StringBuilder reason = new StringBuilder(INTERNAL_SERVER_ERROR.reasonPhrase()) + .append(": ").append(escapeReason(t.getClass().getSimpleName())) + .append(": ").append(escapeReason(t.getMessage())); + final Throwable rootCause = Throwables.getRootCause(t); + if (rootCause != t) { + reason.append(": ").append(escapeReason(rootCause.getClass().getSimpleName())) + .append(": ").append(escapeReason(rootCause.getMessage())); + } + return reason.toString(); + } + + private static String escapeReason(String s) { + return CharMatcher.anyOf("\n\r").replaceFrom(!Strings.isNullOrEmpty(s) ? s : "", ' '); + } + private static GoogleIdToken verifyIdToken(String s) { try { return GOOGLE_ID_TOKEN_VERIFIER.verify(s); diff --git a/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java b/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java index 85c06ba85f..574a50936b 100644 --- a/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java +++ b/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java @@ -24,6 +24,7 @@ import static com.spotify.apollo.test.unit.ResponseMatchers.hasStatus; import static com.spotify.apollo.test.unit.StatusTypeMatchers.belongsToFamily; import static com.spotify.apollo.test.unit.StatusTypeMatchers.withCode; +import static com.spotify.apollo.test.unit.StatusTypeMatchers.withReasonPhrase; import static com.spotify.styx.StringIsValidUUID.isValidUUID; import static java.util.concurrent.CompletableFuture.completedFuture; import static java.util.concurrent.TimeUnit.SECONDS; @@ -46,6 +47,7 @@ import com.spotify.apollo.request.RequestContexts; import com.spotify.apollo.request.RequestMetadataImpl; import com.spotify.apollo.route.AsyncHandler; +import java.io.IOException; import java.time.Instant; import java.util.Collections; import java.util.List; @@ -262,7 +264,7 @@ public void testAuditLoggingForPutWithBrokenAuthorization() } @Test - public void testExceptionAndRequestIdHandlerOnException() + public void testExceptionAndRequestIdHandlerOnImmediateResponseException() throws InterruptedException, ExecutionException, TimeoutException { RequestContext requestContext = mock(RequestContext.class); Request request = Request.forUri("/", "GET"); @@ -276,7 +278,73 @@ public void testExceptionAndRequestIdHandlerOnException() .invoke(requestContext) .toCompletableFuture().get(5, SECONDS); - assertThat(response, hasStatus(withCode(Status.IM_A_TEAPOT))); + assertThat(response, hasStatus(is(Status.IM_A_TEAPOT))); + assertThat(response, hasHeader("X-Request-Id", isValidUUID())); + } + + @Test + public void testExceptionAndRequestIdHandlerOnFutureResponseException() + throws InterruptedException, ExecutionException, TimeoutException { + RequestContext requestContext = mock(RequestContext.class); + Request request = Request.forUri("/", "GET"); + when(requestContext.request()).thenReturn(request); + + Response response = Middlewares.exceptionAndRequestIdHandler() + .apply(rc -> { + LoggerFactory.getLogger(MiddlewaresTest.class).error("I'm a teapot!"); + final CompletableFuture> failure = new CompletableFuture<>(); + LoggerFactory.getLogger(MiddlewaresTest.class).error("deadbeef"); + failure.completeExceptionally(new ResponseException(Response.forStatus(Status.IM_A_TEAPOT))); + return failure; + }) + .invoke(requestContext) + .toCompletableFuture().get(5, SECONDS); + + assertThat(response, hasStatus(is(Status.IM_A_TEAPOT))); + assertThat(response, hasHeader("X-Request-Id", isValidUUID())); + } + + @Test + public void testExceptionAndRequestIdHandlerOnImmediateUnhandledException() + throws InterruptedException, ExecutionException, TimeoutException { + RequestContext requestContext = mock(RequestContext.class); + Request request = Request.forUri("/", "GET"); + when(requestContext.request()).thenReturn(request); + + Response response = Middlewares.exceptionAndRequestIdHandler() + .apply(rc -> { + LoggerFactory.getLogger(MiddlewaresTest.class).error("deadbeef"); + throw new RuntimeException("fubar", new IOException("deadbeef")); + }) + .invoke(requestContext) + .toCompletableFuture().get(5, SECONDS); + + assertThat(response, hasStatus(withCode(Status.INTERNAL_SERVER_ERROR))); + assertThat(response, hasStatus(withReasonPhrase(is( + "Internal Server Error: RuntimeException: fubar: IOException: deadbeef")))); + assertThat(response, hasHeader("X-Request-Id", isValidUUID())); + } + + @Test + public void testExceptionAndRequestIdHandlerOnFutureUnhandledException() + throws InterruptedException, ExecutionException, TimeoutException { + RequestContext requestContext = mock(RequestContext.class); + Request request = Request.forUri("/", "GET"); + when(requestContext.request()).thenReturn(request); + + Response response = Middlewares.exceptionAndRequestIdHandler() + .apply(rc -> { + final CompletableFuture> failure = new CompletableFuture<>(); + LoggerFactory.getLogger(MiddlewaresTest.class).error("deadbeef"); + failure.completeExceptionally(new RuntimeException("fubar", new IOException("deadbeef"))); + return failure; + }) + .invoke(requestContext) + .toCompletableFuture().get(5, SECONDS); + + assertThat(response, hasStatus(withCode(Status.INTERNAL_SERVER_ERROR))); + assertThat(response, hasStatus(withReasonPhrase(is( + "Internal Server Error: RuntimeException: fubar: IOException: deadbeef")))); assertThat(response, hasHeader("X-Request-Id", isValidUUID())); } From b65b8cd0f14ce459cb7de4b8b93b7b2a7c16a3af Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Fri, 18 May 2018 16:17:02 +0900 Subject: [PATCH 10/37] remove request ID from MDC on immediate failure --- styx-common/src/main/java/com/spotify/styx/api/Middlewares.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java index 2e0046d336..d8433c30bf 100644 --- a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java +++ b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java @@ -163,9 +163,11 @@ public static Middleware>, AsyncHandler return response.withHeader(X_REQUEST_ID, requestId); }); } catch (ResponseException e) { + MDC.remove(REQUEST_ID); return completedFuture(e.getResponse() .withHeader(X_REQUEST_ID, requestId)); } catch (Exception e) { + MDC.remove(REQUEST_ID); return completedFuture(Response.forStatus(INTERNAL_SERVER_ERROR .withReasonPhrase(internalServerErrorReason(e))) .withHeader(X_REQUEST_ID, requestId)); From 1c80d9c844d9c0b43ab6c5f95d8aad69b317a9f6 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Mon, 21 May 2018 14:33:11 +0900 Subject: [PATCH 11/37] make codacy happy --- styx-common/src/main/java/com/spotify/styx/api/Middlewares.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java index d8433c30bf..07ba925fd6 100644 --- a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java +++ b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java @@ -180,7 +180,7 @@ private static String internalServerErrorReason(Throwable t) { .append(": ").append(escapeReason(t.getClass().getSimpleName())) .append(": ").append(escapeReason(t.getMessage())); final Throwable rootCause = Throwables.getRootCause(t); - if (rootCause != t) { + if (!t.equals(rootCause)) { reason.append(": ").append(escapeReason(rootCause.getClass().getSimpleName())) .append(": ").append(escapeReason(rootCause.getMessage())); } From 8f2617e412d5b1c270fe69b32be11dfc96429564 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Mon, 21 May 2018 14:37:21 +0900 Subject: [PATCH 12/37] make codacy happy --- .../com/spotify/styx/util/MDCUtilTest.java | 27 +++++++++++++------ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java b/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java index 0b1384e198..a00da8a486 100644 --- a/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java +++ b/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java @@ -27,6 +27,7 @@ import static org.hamcrest.Matchers.nullValue; import static org.junit.Assert.assertThat; +import java.util.Map; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; @@ -71,32 +72,42 @@ public void withMDCCallable() throws ExecutionException, InterruptedException { @Test public void withMDCCommonPool() throws ExecutionException, InterruptedException { MDC.put("foo", "bar"); + final CompletableFuture value1 = new CompletableFuture<>(); + final CompletableFuture value2 = new CompletableFuture<>(); CompletableFuture.runAsync( - () -> assertThat(MDC.get("foo"), is("bar")), + () -> value1.complete(MDC.get("foo")), withMDC()) // Later stages should also have the MDC applied - .thenRun(() -> assertThat(MDC.get("foo"), is("bar"))) + .thenRun(() -> value2.complete(MDC.get("foo"))) .get(); + assertThat(value1.getNow(""), is("bar")); + assertThat(value2.getNow(""), is("bar")); // MDC should not leak - ForkJoinPool.commonPool() - .submit(() -> assertThat(MDC.getCopyOfContextMap(), is(anyOf(nullValue(), is(emptyMap()))))) + final Map mdc = ForkJoinPool.commonPool() + .submit(MDC::getCopyOfContextMap) .get(); + assertThat(mdc, is(anyOf(nullValue(), is(emptyMap())))); } @Test public void withMDCExecutor() throws ExecutionException, InterruptedException { MDC.put("foo", "bar"); + final CompletableFuture value1 = new CompletableFuture<>(); + final CompletableFuture value2 = new CompletableFuture<>(); CompletableFuture.runAsync( - () -> assertThat(MDC.get("foo"), is("bar")), + () -> value1.complete(MDC.get("foo")), withMDC(executor)) // Later stages should also have the MDC applied - .thenRun(() -> assertThat(MDC.get("foo"), is("bar"))) + .thenRun(() -> value2.complete(MDC.get("foo"))) .get(); + assertThat(value1.getNow(""), is("bar")); + assertThat(value2.getNow(""), is("bar")); // MDC should not leak - executor - .submit(() -> assertThat(MDC.getCopyOfContextMap(), is(anyOf(nullValue(), is(emptyMap()))))) + final Map mdc = executor + .submit(MDC::getCopyOfContextMap) .get(); + assertThat(mdc, is(anyOf(nullValue(), is(emptyMap())))); } } From c367b88f60f04c1dd02e27bd3723b46904230f47 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Mon, 21 May 2018 14:50:20 +0900 Subject: [PATCH 13/37] include request id in 500 reason --- .../styx/client/ApiErrorException.java | 9 +++-- .../spotify/styx/client/StyxApolloClient.java | 3 +- .../styx/client/ApiErrorExceptionTest.java | 17 +++++++- .../com/spotify/styx/api/Middlewares.java | 11 ++++-- .../com/spotify/styx/StringIsValidUUID.java | 3 ++ .../spotify/styx/StringIsValidUUIDTest.java | 39 +++++++++++++++++++ .../com/spotify/styx/api/MiddlewaresTest.java | 22 +++++++---- 7 files changed, 85 insertions(+), 19 deletions(-) create mode 100644 styx-common/src/test/java/com/spotify/styx/StringIsValidUUIDTest.java diff --git a/styx-client/src/main/java/com/spotify/styx/client/ApiErrorException.java b/styx-client/src/main/java/com/spotify/styx/client/ApiErrorException.java index fdf3fea661..df5a823139 100644 --- a/styx-client/src/main/java/com/spotify/styx/client/ApiErrorException.java +++ b/styx-client/src/main/java/com/spotify/styx/client/ApiErrorException.java @@ -20,8 +20,6 @@ package com.spotify.styx.client; -import java.util.Objects; - /** * Exception used in case of API errors, i.e. API requests whose response has status code * different than 2xx. @@ -36,7 +34,7 @@ public ApiErrorException(String message, int code, boolean authenticated, String super(message); this.code = code; this.authenticated = authenticated; - this.requestId = Objects.requireNonNull(requestId, "requestId"); + this.requestId = requestId; } public int getCode() { @@ -53,6 +51,9 @@ public String getRequestId() { @Override public String getMessage() { - return super.getMessage() + " (Request ID: " + requestId + ")"; + final String message = super.getMessage(); + return requestId == null || message.contains(requestId) + ? message + : message + " (Request ID: " + requestId + ")"; } } diff --git a/styx-client/src/main/java/com/spotify/styx/client/StyxApolloClient.java b/styx-client/src/main/java/com/spotify/styx/client/StyxApolloClient.java index 3c94f93117..86bf9c31c9 100644 --- a/styx-client/src/main/java/com/spotify/styx/client/StyxApolloClient.java +++ b/styx-client/src/main/java/com/spotify/styx/client/StyxApolloClient.java @@ -448,7 +448,8 @@ private CompletionStage> executeRequest(final Request reque return response; default: final String message = response.status().code() + " " + response.status().reasonPhrase(); - throw new ApiErrorException(message, response.status().code(), authToken.isPresent(), requestId); + final String responseRequestId = response.headers().getOrDefault("X-Request-Id", requestId); + throw new ApiErrorException(message, response.status().code(), authToken.isPresent(), responseRequestId); } } }); diff --git a/styx-client/src/test/java/com/spotify/styx/client/ApiErrorExceptionTest.java b/styx-client/src/test/java/com/spotify/styx/client/ApiErrorExceptionTest.java index c5755b3049..316c3dfee2 100644 --- a/styx-client/src/test/java/com/spotify/styx/client/ApiErrorExceptionTest.java +++ b/styx-client/src/test/java/com/spotify/styx/client/ApiErrorExceptionTest.java @@ -29,10 +29,23 @@ public class ApiErrorExceptionTest { private static final String REQUEST_ID = UUID.randomUUID().toString(); - private static final ApiErrorException EXCEPTION = new ApiErrorException("fubared!", 4711, true, REQUEST_ID); @Test public void getMessage() { - assertThat(EXCEPTION.getMessage(), is("fubared! (Request ID: " + REQUEST_ID + ")")); + final String message = "fubared: rid=" + REQUEST_ID; + ApiErrorException exception = new ApiErrorException(message, 4711, true, REQUEST_ID); + assertThat(exception.getMessage(), is(message)); + } + + @Test + public void getMessageNoRequestIdInMessage() { + ApiErrorException exception = new ApiErrorException("fubared!", 4711, true, REQUEST_ID); + assertThat(exception.getMessage(), is("fubared! (Request ID: " + REQUEST_ID + ")")); + } + + @Test + public void getMessageNoRequestId() { + ApiErrorException exception = new ApiErrorException("fubared!", 4711, true, null); + assertThat(exception.getMessage(), is("fubared!")); } } diff --git a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java index 07ba925fd6..788292ae5d 100644 --- a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java +++ b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java @@ -141,7 +141,7 @@ public static Middleware>, AsyncHandler final String requestIdHeader = requestContext.request().headers().get(X_REQUEST_ID); final String requestId = (requestIdHeader != null) ? requestIdHeader - : UUID.randomUUID().toString(); + : UUID.randomUUID().toString().replace("-",""); // UUID with no dashes, easier to deal with MDC.put(REQUEST_ID, requestId); @@ -155,7 +155,7 @@ public static Middleware>, AsyncHandler response = ((ResponseException) t).getResponse(); } else { response = Response.forStatus(INTERNAL_SERVER_ERROR - .withReasonPhrase(internalServerErrorReason(t))); + .withReasonPhrase(internalServerErrorReason(requestId, t))); } } else { response = r; @@ -169,14 +169,17 @@ public static Middleware>, AsyncHandler } catch (Exception e) { MDC.remove(REQUEST_ID); return completedFuture(Response.forStatus(INTERNAL_SERVER_ERROR - .withReasonPhrase(internalServerErrorReason(e))) + .withReasonPhrase(internalServerErrorReason(requestId, e))) .withHeader(X_REQUEST_ID, requestId)); } }; } - private static String internalServerErrorReason(Throwable t) { + private static String internalServerErrorReason(String requestId, Throwable t) { final StringBuilder reason = new StringBuilder(INTERNAL_SERVER_ERROR.reasonPhrase()) + .append(" (") + .append("Request ID: ") + .append(requestId).append(")") .append(": ").append(escapeReason(t.getClass().getSimpleName())) .append(": ").append(escapeReason(t.getMessage())); final Throwable rootCause = Throwables.getRootCause(t); diff --git a/styx-common/src/test/java/com/spotify/styx/StringIsValidUUID.java b/styx-common/src/test/java/com/spotify/styx/StringIsValidUUID.java index 95d2c7ae12..79abf5f48b 100644 --- a/styx-common/src/test/java/com/spotify/styx/StringIsValidUUID.java +++ b/styx-common/src/test/java/com/spotify/styx/StringIsValidUUID.java @@ -30,6 +30,9 @@ public class StringIsValidUUID extends TypeSafeMatcher { @Override protected boolean matchesSafely(String s) { + if (!s.contains("-")) { + s = s.replaceFirst("(\\w{8})(\\w{4})(\\w{4})(\\w{4})(\\w{12})", "$1-$2-$3-$4-$5"); + } try { UUID.fromString(s); return true; diff --git a/styx-common/src/test/java/com/spotify/styx/StringIsValidUUIDTest.java b/styx-common/src/test/java/com/spotify/styx/StringIsValidUUIDTest.java new file mode 100644 index 0000000000..83c4402ff6 --- /dev/null +++ b/styx-common/src/test/java/com/spotify/styx/StringIsValidUUIDTest.java @@ -0,0 +1,39 @@ +/* + * -\-\- + * Spotify Styx Scheduler Service + * -- + * Copyright (C) 2018 Spotify AB + * -- + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * -/-/- + */ + +package com.spotify.styx; + +import static org.hamcrest.Matchers.is; +import static org.junit.Assert.*; + +import java.util.UUID; +import org.junit.Test; + +public class StringIsValidUUIDTest { + + @Test + public void matchesSafely() { + assertThat(new StringIsValidUUID().matchesSafely(UUID.randomUUID().toString()), is(true)); + assertThat(new StringIsValidUUID().matchesSafely(UUID.randomUUID().toString().replace("-", "")), is(true)); + assertThat(new StringIsValidUUID().matchesSafely("foobar"), is(false)); + assertThat(new StringIsValidUUID().matchesSafely("foo-bar"), is(false)); + assertThat(new StringIsValidUUID().matchesSafely(""), is(false)); + } +} \ No newline at end of file diff --git a/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java b/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java index 574a50936b..631effd82a 100644 --- a/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java +++ b/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java @@ -57,10 +57,12 @@ import java.util.concurrent.CompletionStage; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Supplier; import okio.ByteString; import org.junit.Test; import org.slf4j.LoggerFactory; +import org.slf4j.MDC; /** * Tests Middlewares @@ -307,12 +309,14 @@ public void testExceptionAndRequestIdHandlerOnFutureResponseException() @Test public void testExceptionAndRequestIdHandlerOnImmediateUnhandledException() throws InterruptedException, ExecutionException, TimeoutException { - RequestContext requestContext = mock(RequestContext.class); - Request request = Request.forUri("/", "GET"); + final RequestContext requestContext = mock(RequestContext.class); + final Request request = Request.forUri("/", "GET"); + final AtomicReference requestId = new AtomicReference<>(); when(requestContext.request()).thenReturn(request); Response response = Middlewares.exceptionAndRequestIdHandler() .apply(rc -> { + requestId.set(MDC.get("request-id")); LoggerFactory.getLogger(MiddlewaresTest.class).error("deadbeef"); throw new RuntimeException("fubar", new IOException("deadbeef")); }) @@ -321,19 +325,21 @@ public void testExceptionAndRequestIdHandlerOnImmediateUnhandledException() assertThat(response, hasStatus(withCode(Status.INTERNAL_SERVER_ERROR))); assertThat(response, hasStatus(withReasonPhrase(is( - "Internal Server Error: RuntimeException: fubar: IOException: deadbeef")))); - assertThat(response, hasHeader("X-Request-Id", isValidUUID())); + "Internal Server Error (Request ID: " + requestId.get() + "): RuntimeException: fubar: IOException: deadbeef")))); + assertThat(response, hasHeader("X-Request-Id", is(requestId.get()))); } @Test public void testExceptionAndRequestIdHandlerOnFutureUnhandledException() throws InterruptedException, ExecutionException, TimeoutException { - RequestContext requestContext = mock(RequestContext.class); - Request request = Request.forUri("/", "GET"); + final RequestContext requestContext = mock(RequestContext.class); + final Request request = Request.forUri("/", "GET"); + final AtomicReference requestId = new AtomicReference<>(); when(requestContext.request()).thenReturn(request); Response response = Middlewares.exceptionAndRequestIdHandler() .apply(rc -> { + requestId.set(MDC.get("request-id")); final CompletableFuture> failure = new CompletableFuture<>(); LoggerFactory.getLogger(MiddlewaresTest.class).error("deadbeef"); failure.completeExceptionally(new RuntimeException("fubar", new IOException("deadbeef"))); @@ -344,8 +350,8 @@ public void testExceptionAndRequestIdHandlerOnFutureUnhandledException() assertThat(response, hasStatus(withCode(Status.INTERNAL_SERVER_ERROR))); assertThat(response, hasStatus(withReasonPhrase(is( - "Internal Server Error: RuntimeException: fubar: IOException: deadbeef")))); - assertThat(response, hasHeader("X-Request-Id", isValidUUID())); + "Internal Server Error (Request ID: " + requestId.get() + "): RuntimeException: fubar: IOException: deadbeef")))); + assertThat(response, hasHeader("X-Request-Id", is(requestId.get()))); } @Test From bbfb0608b122924d4765f0c9fcb38a456ddcdb61 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Mon, 21 May 2018 14:52:06 +0900 Subject: [PATCH 14/37] cleanup --- .../main/java/com/spotify/styx/client/StyxApolloClient.java | 4 +++- .../src/main/java/com/spotify/styx/api/Middlewares.java | 4 +--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/styx-client/src/main/java/com/spotify/styx/client/StyxApolloClient.java b/styx-client/src/main/java/com/spotify/styx/client/StyxApolloClient.java index 86bf9c31c9..3c51818310 100644 --- a/styx-client/src/main/java/com/spotify/styx/client/StyxApolloClient.java +++ b/styx-client/src/main/java/com/spotify/styx/client/StyxApolloClient.java @@ -70,10 +70,12 @@ * as {@link RuntimeException} instead. */ class StyxApolloClient implements StyxClient { + private static final String STYX_API_VERSION = V3.name().toLowerCase(); private static final String STYX_CLIENT_VERSION = "Styx Client " + StyxApolloClient.class.getPackage().getImplementationVersion(); private static final Duration TTL = Duration.ofSeconds(90); + private static final String X_REQUEST_ID = "X-Request-Id"; private final URI apiHost; private final Client client; @@ -448,7 +450,7 @@ private CompletionStage> executeRequest(final Request reque return response; default: final String message = response.status().code() + " " + response.status().reasonPhrase(); - final String responseRequestId = response.headers().getOrDefault("X-Request-Id", requestId); + final String responseRequestId = response.headers().getOrDefault(X_REQUEST_ID, requestId); throw new ApiErrorException(message, response.status().code(), authToken.isPresent(), responseRequestId); } } diff --git a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java index 788292ae5d..b6ce2dfb86 100644 --- a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java +++ b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java @@ -177,9 +177,7 @@ public static Middleware>, AsyncHandler private static String internalServerErrorReason(String requestId, Throwable t) { final StringBuilder reason = new StringBuilder(INTERNAL_SERVER_ERROR.reasonPhrase()) - .append(" (") - .append("Request ID: ") - .append(requestId).append(")") + .append(" (").append("Request ID: ").append(requestId).append(")") .append(": ").append(escapeReason(t.getClass().getSimpleName())) .append(": ").append(escapeReason(t.getMessage())); final Throwable rootCause = Throwables.getRootCause(t); From 11185fe7457e705673830a297d5199d45b9dbb0a Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Mon, 21 May 2018 14:53:16 +0900 Subject: [PATCH 15/37] escape entire reason string --- .../main/java/com/spotify/styx/api/Middlewares.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java index b6ce2dfb86..44b6fa2169 100644 --- a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java +++ b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java @@ -178,14 +178,14 @@ public static Middleware>, AsyncHandler private static String internalServerErrorReason(String requestId, Throwable t) { final StringBuilder reason = new StringBuilder(INTERNAL_SERVER_ERROR.reasonPhrase()) .append(" (").append("Request ID: ").append(requestId).append(")") - .append(": ").append(escapeReason(t.getClass().getSimpleName())) - .append(": ").append(escapeReason(t.getMessage())); + .append(": ").append(t.getClass().getSimpleName()) + .append(": ").append(t.getMessage()); final Throwable rootCause = Throwables.getRootCause(t); if (!t.equals(rootCause)) { - reason.append(": ").append(escapeReason(rootCause.getClass().getSimpleName())) - .append(": ").append(escapeReason(rootCause.getMessage())); + reason.append(": ").append(rootCause.getClass().getSimpleName()) + .append(": ").append(rootCause.getMessage()); } - return reason.toString(); + return escapeReason(reason.toString()); } private static String escapeReason(String s) { From c2bc28638e21477b1d0c9f89db23dfa70fd75172 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Mon, 21 May 2018 14:57:00 +0900 Subject: [PATCH 16/37] simplify --- .../src/main/java/com/spotify/styx/api/Middlewares.java | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java index 44b6fa2169..c97fff316c 100644 --- a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java +++ b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java @@ -31,7 +31,6 @@ import com.google.api.client.googleapis.util.Utils; import com.google.api.client.http.javanet.NetHttpTransport; import com.google.common.base.CharMatcher; -import com.google.common.base.Strings; import com.google.common.base.Throwables; import com.google.common.collect.ImmutableSet; import com.google.common.net.HttpHeaders; @@ -185,11 +184,8 @@ private static String internalServerErrorReason(String requestId, Throwable t) { reason.append(": ").append(rootCause.getClass().getSimpleName()) .append(": ").append(rootCause.getMessage()); } - return escapeReason(reason.toString()); - } - - private static String escapeReason(String s) { - return CharMatcher.anyOf("\n\r").replaceFrom(!Strings.isNullOrEmpty(s) ? s : "", ' '); + // Remove any line breaks + return CharMatcher.anyOf("\n\r").replaceFrom(reason.toString(), ' '); } private static GoogleIdToken verifyIdToken(String s) { From 0dc066fb8865e62b38d18d00193d87a108ef6b3a Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Mon, 21 May 2018 14:58:11 +0900 Subject: [PATCH 17/37] todo --- styx-common/src/main/java/com/spotify/styx/api/Middlewares.java | 1 + 1 file changed, 1 insertion(+) diff --git a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java index c97fff316c..722616854d 100644 --- a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java +++ b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java @@ -175,6 +175,7 @@ public static Middleware>, AsyncHandler } private static String internalServerErrorReason(String requestId, Throwable t) { + // TODO: returning internal error messages in reason phrase might be a security issue. Make configurable? final StringBuilder reason = new StringBuilder(INTERNAL_SERVER_ERROR.reasonPhrase()) .append(" (").append("Request ID: ").append(requestId).append(")") .append(": ").append(t.getClass().getSimpleName()) From 9bbd295d5729421813f71545738bea9a57571db4 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Mon, 21 May 2018 15:01:09 +0900 Subject: [PATCH 18/37] fix license header --- .../test/java/com/spotify/styx/StringIsValidUUIDTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/styx-common/src/test/java/com/spotify/styx/StringIsValidUUIDTest.java b/styx-common/src/test/java/com/spotify/styx/StringIsValidUUIDTest.java index 83c4402ff6..e50f331aaf 100644 --- a/styx-common/src/test/java/com/spotify/styx/StringIsValidUUIDTest.java +++ b/styx-common/src/test/java/com/spotify/styx/StringIsValidUUIDTest.java @@ -7,9 +7,9 @@ * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -36,4 +36,4 @@ public void matchesSafely() { assertThat(new StringIsValidUUID().matchesSafely("foo-bar"), is(false)); assertThat(new StringIsValidUUID().matchesSafely(""), is(false)); } -} \ No newline at end of file +} From d64dc1cdeac2c219da1abe55fadbfca3a5fff384 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Mon, 21 May 2018 15:04:29 +0900 Subject: [PATCH 19/37] cleanup --- .../src/test/java/com/spotify/styx/StringIsValidUUIDTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/styx-common/src/test/java/com/spotify/styx/StringIsValidUUIDTest.java b/styx-common/src/test/java/com/spotify/styx/StringIsValidUUIDTest.java index e50f331aaf..2c217be571 100644 --- a/styx-common/src/test/java/com/spotify/styx/StringIsValidUUIDTest.java +++ b/styx-common/src/test/java/com/spotify/styx/StringIsValidUUIDTest.java @@ -21,7 +21,7 @@ package com.spotify.styx; import static org.hamcrest.Matchers.is; -import static org.junit.Assert.*; +import static org.junit.Assert.assertThat; import java.util.UUID; import org.junit.Test; From eeed832333365d235c8040f659277261f9e96805 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Mon, 21 May 2018 17:11:29 +0900 Subject: [PATCH 20/37] make codacy happy --- .../src/test/java/com/spotify/styx/StringIsValidUUID.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/styx-common/src/test/java/com/spotify/styx/StringIsValidUUID.java b/styx-common/src/test/java/com/spotify/styx/StringIsValidUUID.java index 79abf5f48b..8c15599d26 100644 --- a/styx-common/src/test/java/com/spotify/styx/StringIsValidUUID.java +++ b/styx-common/src/test/java/com/spotify/styx/StringIsValidUUID.java @@ -30,11 +30,11 @@ public class StringIsValidUUID extends TypeSafeMatcher { @Override protected boolean matchesSafely(String s) { - if (!s.contains("-")) { - s = s.replaceFirst("(\\w{8})(\\w{4})(\\w{4})(\\w{4})(\\w{12})", "$1-$2-$3-$4-$5"); - } + final String uuid = !s.contains("-") + ? s.replaceFirst("(\\w{8})(\\w{4})(\\w{4})(\\w{4})(\\w{12})", "$1-$2-$3-$4-$5") + : s; try { - UUID.fromString(s); + UUID.fromString(uuid); return true; } catch (IllegalArgumentException e) { return false; From c466d6f6c9c460f884c7fbb4192001e6a037450e Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Tue, 22 May 2018 12:49:07 +0900 Subject: [PATCH 21/37] use try-with-resources MDC And remove misplaced MDC cleanup. --- .../main/java/com/spotify/styx/api/Middlewares.java | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java index 722616854d..276be60c1a 100644 --- a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java +++ b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java @@ -140,14 +140,10 @@ public static Middleware>, AsyncHandler final String requestIdHeader = requestContext.request().headers().get(X_REQUEST_ID); final String requestId = (requestIdHeader != null) ? requestIdHeader - : UUID.randomUUID().toString().replace("-",""); // UUID with no dashes, easier to deal with + : UUID.randomUUID().toString().replace("-", ""); // UUID with no dashes, easier to deal with - MDC.put(REQUEST_ID, requestId); - - try { + try (MDC.MDCCloseable mdc = MDC.putCloseable(REQUEST_ID, requestId)) { return innerHandler.invoke(requestContext).handle((r, t) -> { - MDC.remove(REQUEST_ID); - final Response response; if (t != null) { if (t instanceof ResponseException) { @@ -162,11 +158,9 @@ public static Middleware>, AsyncHandler return response.withHeader(X_REQUEST_ID, requestId); }); } catch (ResponseException e) { - MDC.remove(REQUEST_ID); return completedFuture(e.getResponse() .withHeader(X_REQUEST_ID, requestId)); } catch (Exception e) { - MDC.remove(REQUEST_ID); return completedFuture(Response.forStatus(INTERNAL_SERVER_ERROR .withReasonPhrase(internalServerErrorReason(requestId, e))) .withHeader(X_REQUEST_ID, requestId)); From b48fff8c4ce58817409e43bd70e0d34f834c5f43 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Tue, 22 May 2018 12:57:32 +0900 Subject: [PATCH 22/37] verify request id propagation --- .../com/spotify/styx/api/MiddlewaresTest.java | 38 +++++++++++++------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java b/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java index 631effd82a..dc48422142 100644 --- a/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java +++ b/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java @@ -268,12 +268,14 @@ public void testAuditLoggingForPutWithBrokenAuthorization() @Test public void testExceptionAndRequestIdHandlerOnImmediateResponseException() throws InterruptedException, ExecutionException, TimeoutException { - RequestContext requestContext = mock(RequestContext.class); - Request request = Request.forUri("/", "GET"); + final RequestContext requestContext = mock(RequestContext.class); + final Request request = Request.forUri("/", "GET"); + final AtomicReference requestId = new AtomicReference<>(); when(requestContext.request()).thenReturn(request); Response response = Middlewares.exceptionAndRequestIdHandler() .apply(rc -> { + requestId.set(MDC.get("request-id")); LoggerFactory.getLogger(MiddlewaresTest.class).error("I'm a teapot!"); throw new ResponseException(Response.forStatus(Status.IM_A_TEAPOT)); }) @@ -281,18 +283,21 @@ public void testExceptionAndRequestIdHandlerOnImmediateResponseException() .toCompletableFuture().get(5, SECONDS); assertThat(response, hasStatus(is(Status.IM_A_TEAPOT))); - assertThat(response, hasHeader("X-Request-Id", isValidUUID())); + assertThat(response, hasHeader("X-Request-Id", is(requestId.get()))); + assertThat(requestId.get(), isValidUUID()); } @Test public void testExceptionAndRequestIdHandlerOnFutureResponseException() throws InterruptedException, ExecutionException, TimeoutException { - RequestContext requestContext = mock(RequestContext.class); - Request request = Request.forUri("/", "GET"); + final RequestContext requestContext = mock(RequestContext.class); + final Request request = Request.forUri("/", "GET"); + final AtomicReference requestId = new AtomicReference<>(); when(requestContext.request()).thenReturn(request); Response response = Middlewares.exceptionAndRequestIdHandler() .apply(rc -> { + requestId.set(MDC.get("request-id")); LoggerFactory.getLogger(MiddlewaresTest.class).error("I'm a teapot!"); final CompletableFuture> failure = new CompletableFuture<>(); LoggerFactory.getLogger(MiddlewaresTest.class).error("deadbeef"); @@ -303,7 +308,8 @@ public void testExceptionAndRequestIdHandlerOnFutureResponseException() .toCompletableFuture().get(5, SECONDS); assertThat(response, hasStatus(is(Status.IM_A_TEAPOT))); - assertThat(response, hasHeader("X-Request-Id", isValidUUID())); + assertThat(response, hasHeader("X-Request-Id", is(requestId.get()))); + assertThat(requestId.get(), isValidUUID()); } @Test @@ -327,6 +333,7 @@ public void testExceptionAndRequestIdHandlerOnImmediateUnhandledException() assertThat(response, hasStatus(withReasonPhrase(is( "Internal Server Error (Request ID: " + requestId.get() + "): RuntimeException: fubar: IOException: deadbeef")))); assertThat(response, hasHeader("X-Request-Id", is(requestId.get()))); + assertThat(requestId.get(), isValidUUID()); } @Test @@ -352,17 +359,20 @@ public void testExceptionAndRequestIdHandlerOnFutureUnhandledException() assertThat(response, hasStatus(withReasonPhrase(is( "Internal Server Error (Request ID: " + requestId.get() + "): RuntimeException: fubar: IOException: deadbeef")))); assertThat(response, hasHeader("X-Request-Id", is(requestId.get()))); + assertThat(requestId.get(), isValidUUID()); } @Test public void testExceptionAndRequestIdHandlerOnResponse() throws InterruptedException, ExecutionException, TimeoutException { - RequestContext requestContext = mock(RequestContext.class); - Request request = Request.forUri("/", "GET"); + final RequestContext requestContext = mock(RequestContext.class); + final Request request = Request.forUri("/", "GET"); + final AtomicReference requestId = new AtomicReference<>(); when(requestContext.request()).thenReturn(request); Response response = Middlewares.exceptionAndRequestIdHandler() .apply(rc -> { + requestId.set(MDC.get("request-id")); LoggerFactory.getLogger(MiddlewaresTest.class).info("I'm OK!"); return completedFuture(Response.forStatus(Status.OK)); }) @@ -370,19 +380,22 @@ public void testExceptionAndRequestIdHandlerOnResponse() .toCompletableFuture().get(5, SECONDS); assertThat(response, hasStatus(withCode(Status.OK))); - assertThat(response, hasHeader("X-Request-Id", isValidUUID())); + assertThat(response, hasHeader("X-Request-Id", is(requestId.get()))); + assertThat(requestId.get(), isValidUUID()); } @Test public void testExceptionAndRequestIdHandlerAcceptsRequestIdHeader() throws InterruptedException, ExecutionException, TimeoutException { - RequestContext requestContext = mock(RequestContext.class); - String requestId = UUID.randomUUID().toString(); - Request request = Request.forUri("/", "GET") + final RequestContext requestContext = mock(RequestContext.class); + final String requestId = UUID.randomUUID().toString(); + final Request request = Request.forUri("/", "GET") .withHeader("X-Request-Id", requestId); + final AtomicReference propagatedRequestId = new AtomicReference<>(); when(requestContext.request()).thenReturn(request); Response response = Middlewares.exceptionAndRequestIdHandler() .apply(rc -> { + propagatedRequestId.set(MDC.get("request-id")); LoggerFactory.getLogger(MiddlewaresTest.class).info("I'm OK!"); return completedFuture(Response.forStatus(Status.OK)); }) @@ -391,6 +404,7 @@ public void testExceptionAndRequestIdHandlerAcceptsRequestIdHeader() assertThat(response, hasStatus(withCode(Status.OK))); assertThat(response, hasHeader("X-Request-Id", is(requestId))); + assertThat(propagatedRequestId.get(), is(requestId)); } @Test From 642e4df589ed665f605df890994c27729bc03ea0 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Tue, 22 May 2018 13:13:41 +0900 Subject: [PATCH 23/37] log error on MDC failures --- .../src/main/java/com/spotify/styx/util/MDCUtil.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/styx-common/src/main/java/com/spotify/styx/util/MDCUtil.java b/styx-common/src/main/java/com/spotify/styx/util/MDCUtil.java index 76fff2a0a3..27c0473f81 100644 --- a/styx-common/src/main/java/com/spotify/styx/util/MDCUtil.java +++ b/styx-common/src/main/java/com/spotify/styx/util/MDCUtil.java @@ -97,7 +97,7 @@ private static Map safeCopyOfContextMap() { try { return MDC.getCopyOfContextMap(); } catch (Exception e) { - log.warn("Failed to copy MDC"); + log.error("Failed to copy MDC", e); return null; } } @@ -110,7 +110,7 @@ private static void safeSetContextMap(Map contextMap) { MDC.clear(); } } catch (Exception e) { - log.warn("Failed to set MDC"); + log.error("Failed to set MDC", e); } } @@ -118,7 +118,7 @@ private static void safeResetContextMap() { try { MDC.clear(); } catch (Exception e) { - log.warn("Failed to reset MDC"); + log.error("Failed to reset MDC", e); } } } From 76e46fc5abe80d710406959e2f258179f52bfc87 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Tue, 22 May 2018 13:15:43 +0900 Subject: [PATCH 24/37] handle MDC failure in middleware --- .../main/java/com/spotify/styx/api/Middlewares.java | 3 ++- .../src/main/java/com/spotify/styx/util/MDCUtil.java | 10 ++++++++++ .../test/java/com/spotify/styx/util/MDCUtilTest.java | 9 +++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java index 276be60c1a..c1355b7950 100644 --- a/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java +++ b/styx-common/src/main/java/com/spotify/styx/api/Middlewares.java @@ -41,6 +41,7 @@ import com.spotify.apollo.route.AsyncHandler; import com.spotify.apollo.route.Middleware; import com.spotify.apollo.route.SyncHandler; +import com.spotify.styx.util.MDCUtil; import io.norberg.automatter.AutoMatter; import java.io.IOException; import java.security.GeneralSecurityException; @@ -142,7 +143,7 @@ public static Middleware>, AsyncHandler ? requestIdHeader : UUID.randomUUID().toString().replace("-", ""); // UUID with no dashes, easier to deal with - try (MDC.MDCCloseable mdc = MDC.putCloseable(REQUEST_ID, requestId)) { + try (MDC.MDCCloseable mdc = MDCUtil.safePutCloseable(REQUEST_ID, requestId)) { return innerHandler.invoke(requestContext).handle((r, t) -> { final Response response; if (t != null) { diff --git a/styx-common/src/main/java/com/spotify/styx/util/MDCUtil.java b/styx-common/src/main/java/com/spotify/styx/util/MDCUtil.java index 27c0473f81..50f4b309a2 100644 --- a/styx-common/src/main/java/com/spotify/styx/util/MDCUtil.java +++ b/styx-common/src/main/java/com/spotify/styx/util/MDCUtil.java @@ -121,4 +121,14 @@ private static void safeResetContextMap() { log.error("Failed to reset MDC", e); } } + + public static MDC.MDCCloseable safePutCloseable(String key, String value) { + try { + return MDC.putCloseable(key, value); + } catch (Exception e) { + log.error("Failed to put MDC", e); + // Returning null here is ok as try-with-resources ignores null resources. + return null; + } + } } diff --git a/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java b/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java index a00da8a486..c93a785055 100644 --- a/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java +++ b/styx-common/src/test/java/com/spotify/styx/util/MDCUtilTest.java @@ -37,6 +37,7 @@ import org.junit.Before; import org.junit.Test; import org.slf4j.MDC; +import org.slf4j.MDC.MDCCloseable; public class MDCUtilTest { @@ -110,4 +111,12 @@ public void withMDCExecutor() throws ExecutionException, InterruptedException { .get(); assertThat(mdc, is(anyOf(nullValue(), is(emptyMap())))); } + + @Test + public void safePutCloseable() { + try (MDCCloseable mdc = MDCUtil.safePutCloseable("foo", "bar")) { + assertThat(MDC.get("foo"), is("bar")); + } + assertThat(MDC.get("foo"), is(nullValue())); + } } From 4a96e2d6d440703ac288bc6a21c8301c531a1fb9 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Tue, 22 May 2018 13:17:19 +0900 Subject: [PATCH 25/37] remove dashes in client request ID --- .../src/main/java/com/spotify/styx/client/StyxApolloClient.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/styx-client/src/main/java/com/spotify/styx/client/StyxApolloClient.java b/styx-client/src/main/java/com/spotify/styx/client/StyxApolloClient.java index 3c51818310..16b674b768 100644 --- a/styx-client/src/main/java/com/spotify/styx/client/StyxApolloClient.java +++ b/styx-client/src/main/java/com/spotify/styx/client/StyxApolloClient.java @@ -440,7 +440,7 @@ private CompletionStage> executeRequest(final Request reque return CompletableFutures.exceptionallyCompletedFuture( new ClientErrorException("Authentication failure: " + e.getMessage(), e)); } - final String requestId = UUID.randomUUID().toString(); + final String requestId = UUID.randomUUID().toString().replace("-", ""); // UUID with no dashes, easier to deal with return client.send(decorateRequest(request, requestId, authToken)).handle((response, e) -> { if (e != null) { throw new ClientErrorException("Request failed: " + request.method() + " " + request.uri(), e); From 67e116ceb3b3127f93cca1bdbc48b4d9d7bd9fe1 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Tue, 22 May 2018 13:25:00 +0900 Subject: [PATCH 26/37] verify that client sends request id --- styx-client/pom.xml | 7 +++++++ .../spotify/styx/client/StyxApolloClientTest.java | 14 ++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/styx-client/pom.xml b/styx-client/pom.xml index 985e1fbfaf..d6bc32a118 100644 --- a/styx-client/pom.xml +++ b/styx-client/pom.xml @@ -36,6 +36,13 @@ google-api-services-iam + + com.spotify + styx-common + test-jar + ${project.version} + test + junit junit diff --git a/styx-client/src/test/java/com/spotify/styx/client/StyxApolloClientTest.java b/styx-client/src/test/java/com/spotify/styx/client/StyxApolloClientTest.java index f0fee97f91..60cbb5fcf6 100644 --- a/styx-client/src/test/java/com/spotify/styx/client/StyxApolloClientTest.java +++ b/styx-client/src/test/java/com/spotify/styx/client/StyxApolloClientTest.java @@ -21,6 +21,7 @@ package com.spotify.styx.client; import static com.google.common.collect.Iterables.getLast; +import static com.spotify.styx.StringIsValidUUID.isValidUUID; import static java.util.Arrays.asList; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; @@ -33,6 +34,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import com.fasterxml.jackson.core.JsonProcessingException; import com.google.common.collect.ImmutableList; import com.spotify.apollo.Client; import com.spotify.apollo.Request; @@ -50,10 +52,12 @@ import com.spotify.styx.serialization.Json; import java.io.IOException; import java.time.Instant; +import java.util.Collections; import java.util.List; import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; import junitparams.JUnitParamsRunner; import junitparams.Parameters; import okhttp3.HttpUrl; @@ -322,4 +326,14 @@ public void testUnauthorizedWithCredentialsApiError() throws Exception { assertThat(apiErrorException.isAuthenticated(), is(true)); } } + + @Test + public void testSendsRequestId() throws JsonProcessingException { + final StyxApolloClient styx = new StyxApolloClient(client, CLIENT_HOST, auth); + when(client.send(requestCaptor.capture())).thenReturn(CompletableFuture.completedFuture( + Response.forStatus(Status.OK).withPayload(Json.serialize(Collections.emptyList())))); + styx.workflows(); + final Request request = requestCaptor.getValue(); + assertThat(request.header("X-Request-Id").get(), isValidUUID()); + } } From 885058ef263a4fbefb83b9527e5c18b0911628f7 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Tue, 22 May 2018 13:43:24 +0900 Subject: [PATCH 27/37] client: error on request id mismatch --- .../styx/client/ClientErrorException.java | 4 ++++ .../spotify/styx/client/StyxApolloClient.java | 5 ++++- .../styx/client/StyxApolloClientTest.java | 21 +++++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/styx-client/src/main/java/com/spotify/styx/client/ClientErrorException.java b/styx-client/src/main/java/com/spotify/styx/client/ClientErrorException.java index 829e84ddfa..b5c3cbcabd 100644 --- a/styx-client/src/main/java/com/spotify/styx/client/ClientErrorException.java +++ b/styx-client/src/main/java/com/spotify/styx/client/ClientErrorException.java @@ -25,4 +25,8 @@ public class ClientErrorException extends RuntimeException { public ClientErrorException(String message, Throwable cause) { super(message, cause); } + + public ClientErrorException(String message) { + super(message); + } } diff --git a/styx-client/src/main/java/com/spotify/styx/client/StyxApolloClient.java b/styx-client/src/main/java/com/spotify/styx/client/StyxApolloClient.java index 16b674b768..ed347ac2ca 100644 --- a/styx-client/src/main/java/com/spotify/styx/client/StyxApolloClient.java +++ b/styx-client/src/main/java/com/spotify/styx/client/StyxApolloClient.java @@ -445,12 +445,15 @@ private CompletionStage> executeRequest(final Request reque if (e != null) { throw new ClientErrorException("Request failed: " + request.method() + " " + request.uri(), e); } else { + final String responseRequestId = response.headers().get(X_REQUEST_ID); + if (responseRequestId != null && !responseRequestId.equals(requestId)) { + throw new ClientErrorException("Request ID mismatch: '" + requestId + "' != '" + responseRequestId + "'"); + } switch (response.status().family()) { case SUCCESSFUL: return response; default: final String message = response.status().code() + " " + response.status().reasonPhrase(); - final String responseRequestId = response.headers().getOrDefault(X_REQUEST_ID, requestId); throw new ApiErrorException(message, response.status().code(), authToken.isPresent(), responseRequestId); } } diff --git a/styx-client/src/test/java/com/spotify/styx/client/StyxApolloClientTest.java b/styx-client/src/test/java/com/spotify/styx/client/StyxApolloClientTest.java index 60cbb5fcf6..26ef5efff9 100644 --- a/styx-client/src/test/java/com/spotify/styx/client/StyxApolloClientTest.java +++ b/styx-client/src/test/java/com/spotify/styx/client/StyxApolloClientTest.java @@ -62,6 +62,7 @@ import junitparams.Parameters; import okhttp3.HttpUrl; import okio.ByteString; +import org.hamcrest.Matchers; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -336,4 +337,24 @@ public void testSendsRequestId() throws JsonProcessingException { final Request request = requestCaptor.getValue(); assertThat(request.header("X-Request-Id").get(), isValidUUID()); } + + @Test + public void testErrorOnRequestIdMismatch() throws JsonProcessingException, InterruptedException { + final StyxApolloClient styx = new StyxApolloClient(client, CLIENT_HOST, auth); + when(client.send(requestCaptor.capture())).thenReturn(CompletableFuture.completedFuture( + Response.forStatus(Status.OK) + .withHeader("X-Request-Id", "foobar") + .withPayload(Json.serialize(Collections.emptyList())))); + + try { + styx.workflows().toCompletableFuture().get(); + fail(); + } catch (ExecutionException e) { + final Request request = requestCaptor.getValue(); + final String requestId = request.header("X-Request-Id").get(); + final Throwable cause = e.getCause(); + assertThat(cause, instanceOf(ClientErrorException.class)); + assertThat(cause.getMessage(), is("Request ID mismatch: '" + requestId + "' != 'foobar'")); + } + } } From f049f3f6e913cf743e8c0ec88f57498cad2c9aa1 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Tue, 22 May 2018 14:12:45 +0900 Subject: [PATCH 28/37] remove unused imports --- .../test/java/com/spotify/styx/client/StyxApolloClientTest.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/styx-client/src/test/java/com/spotify/styx/client/StyxApolloClientTest.java b/styx-client/src/test/java/com/spotify/styx/client/StyxApolloClientTest.java index 26ef5efff9..6e0b724fe1 100644 --- a/styx-client/src/test/java/com/spotify/styx/client/StyxApolloClientTest.java +++ b/styx-client/src/test/java/com/spotify/styx/client/StyxApolloClientTest.java @@ -57,12 +57,10 @@ import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; -import java.util.concurrent.TimeUnit; import junitparams.JUnitParamsRunner; import junitparams.Parameters; import okhttp3.HttpUrl; import okio.ByteString; -import org.hamcrest.Matchers; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; From 7224c30b63132b45616b216b968fe20bd3498d4d Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Tue, 22 May 2018 15:11:23 +0900 Subject: [PATCH 29/37] fix test-jar dependency --- styx-common/pom.xml | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/styx-common/pom.xml b/styx-common/pom.xml index 0513b197d2..3e6d03ebde 100644 --- a/styx-common/pom.xml +++ b/styx-common/pom.xml @@ -143,4 +143,21 @@ test + + + + + org.apache.maven.plugins + maven-jar-plugin + 3.1.0 + + + + test-jar + + + + + + From 7b5ab2358b0b0765b1a36b96876bbe96c60bb2a9 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Tue, 22 May 2018 15:33:34 +0900 Subject: [PATCH 30/37] fix build --- .../src/test/java/com/spotify/styx/api/MiddlewaresTest.java | 2 +- .../main/java/com/spotify/styx/util}/StringIsValidUUID.java | 6 +++--- .../java/com/spotify/styx/util}/StringIsValidUUIDTest.java | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) rename {styx-common/src/test/java/com/spotify/styx => styx-test/src/main/java/com/spotify/styx/util}/StringIsValidUUID.java (97%) rename {styx-common/src/test/java/com/spotify/styx => styx-test/src/test/java/com/spotify/styx/util}/StringIsValidUUIDTest.java (97%) diff --git a/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java b/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java index dc48422142..97daa15126 100644 --- a/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java +++ b/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java @@ -25,7 +25,7 @@ import static com.spotify.apollo.test.unit.StatusTypeMatchers.belongsToFamily; import static com.spotify.apollo.test.unit.StatusTypeMatchers.withCode; import static com.spotify.apollo.test.unit.StatusTypeMatchers.withReasonPhrase; -import static com.spotify.styx.StringIsValidUUID.isValidUUID; +import static com.spotify.styx.util.StringIsValidUUID.isValidUUID; import static java.util.concurrent.CompletableFuture.completedFuture; import static java.util.concurrent.TimeUnit.SECONDS; import static org.hamcrest.MatcherAssert.assertThat; diff --git a/styx-common/src/test/java/com/spotify/styx/StringIsValidUUID.java b/styx-test/src/main/java/com/spotify/styx/util/StringIsValidUUID.java similarity index 97% rename from styx-common/src/test/java/com/spotify/styx/StringIsValidUUID.java rename to styx-test/src/main/java/com/spotify/styx/util/StringIsValidUUID.java index 8c15599d26..42fcc47f6b 100644 --- a/styx-common/src/test/java/com/spotify/styx/StringIsValidUUID.java +++ b/styx-test/src/main/java/com/spotify/styx/util/StringIsValidUUID.java @@ -7,9 +7,9 @@ * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -18,7 +18,7 @@ * -/-/- */ -package com.spotify.styx; +package com.spotify.styx.util; import java.util.UUID; import org.hamcrest.Description; diff --git a/styx-common/src/test/java/com/spotify/styx/StringIsValidUUIDTest.java b/styx-test/src/test/java/com/spotify/styx/util/StringIsValidUUIDTest.java similarity index 97% rename from styx-common/src/test/java/com/spotify/styx/StringIsValidUUIDTest.java rename to styx-test/src/test/java/com/spotify/styx/util/StringIsValidUUIDTest.java index 2c217be571..5ef81144a5 100644 --- a/styx-common/src/test/java/com/spotify/styx/StringIsValidUUIDTest.java +++ b/styx-test/src/test/java/com/spotify/styx/util/StringIsValidUUIDTest.java @@ -7,9 +7,9 @@ * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -18,7 +18,7 @@ * -/-/- */ -package com.spotify.styx; +package com.spotify.styx.util; import static org.hamcrest.Matchers.is; import static org.junit.Assert.assertThat; From 77ad597cf61878d571cd8aefab2addc6ff5459cd Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Tue, 22 May 2018 15:33:50 +0900 Subject: [PATCH 31/37] Revert "fix test-jar dependency" This reverts commit 7224c30b63132b45616b216b968fe20bd3498d4d. --- styx-common/pom.xml | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/styx-common/pom.xml b/styx-common/pom.xml index 3e6d03ebde..0513b197d2 100644 --- a/styx-common/pom.xml +++ b/styx-common/pom.xml @@ -143,21 +143,4 @@ test - - - - - org.apache.maven.plugins - maven-jar-plugin - 3.1.0 - - - - test-jar - - - - - - From 8a4ebfafcaaa72beb44057494c999cc8cc1cd935 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Tue, 22 May 2018 15:34:14 +0900 Subject: [PATCH 32/37] fix build --- styx-client/pom.xml | 7 ------- 1 file changed, 7 deletions(-) diff --git a/styx-client/pom.xml b/styx-client/pom.xml index d6bc32a118..985e1fbfaf 100644 --- a/styx-client/pom.xml +++ b/styx-client/pom.xml @@ -36,13 +36,6 @@ google-api-services-iam - - com.spotify - styx-common - test-jar - ${project.version} - test - junit junit From c8a7ced81debc0201fe0e014ba0681ebf99b81e6 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Tue, 22 May 2018 15:36:58 +0900 Subject: [PATCH 33/37] make checkstyle happy --- .../java/com/spotify/styx/api/MiddlewaresTest.java | 12 ++++++------ .../com/spotify/styx/util/StringIsValidUUID.java | 6 +++--- .../com/spotify/styx/util/StringIsValidUUIDTest.java | 12 ++++++------ 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java b/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java index 97daa15126..75c372d737 100644 --- a/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java +++ b/styx-common/src/test/java/com/spotify/styx/api/MiddlewaresTest.java @@ -25,7 +25,7 @@ import static com.spotify.apollo.test.unit.StatusTypeMatchers.belongsToFamily; import static com.spotify.apollo.test.unit.StatusTypeMatchers.withCode; import static com.spotify.apollo.test.unit.StatusTypeMatchers.withReasonPhrase; -import static com.spotify.styx.util.StringIsValidUUID.isValidUUID; +import static com.spotify.styx.util.StringIsValidUuid.isValidUuid; import static java.util.concurrent.CompletableFuture.completedFuture; import static java.util.concurrent.TimeUnit.SECONDS; import static org.hamcrest.MatcherAssert.assertThat; @@ -284,7 +284,7 @@ public void testExceptionAndRequestIdHandlerOnImmediateResponseException() assertThat(response, hasStatus(is(Status.IM_A_TEAPOT))); assertThat(response, hasHeader("X-Request-Id", is(requestId.get()))); - assertThat(requestId.get(), isValidUUID()); + assertThat(requestId.get(), isValidUuid()); } @Test @@ -309,7 +309,7 @@ public void testExceptionAndRequestIdHandlerOnFutureResponseException() assertThat(response, hasStatus(is(Status.IM_A_TEAPOT))); assertThat(response, hasHeader("X-Request-Id", is(requestId.get()))); - assertThat(requestId.get(), isValidUUID()); + assertThat(requestId.get(), isValidUuid()); } @Test @@ -333,7 +333,7 @@ public void testExceptionAndRequestIdHandlerOnImmediateUnhandledException() assertThat(response, hasStatus(withReasonPhrase(is( "Internal Server Error (Request ID: " + requestId.get() + "): RuntimeException: fubar: IOException: deadbeef")))); assertThat(response, hasHeader("X-Request-Id", is(requestId.get()))); - assertThat(requestId.get(), isValidUUID()); + assertThat(requestId.get(), isValidUuid()); } @Test @@ -359,7 +359,7 @@ public void testExceptionAndRequestIdHandlerOnFutureUnhandledException() assertThat(response, hasStatus(withReasonPhrase(is( "Internal Server Error (Request ID: " + requestId.get() + "): RuntimeException: fubar: IOException: deadbeef")))); assertThat(response, hasHeader("X-Request-Id", is(requestId.get()))); - assertThat(requestId.get(), isValidUUID()); + assertThat(requestId.get(), isValidUuid()); } @Test @@ -381,7 +381,7 @@ public void testExceptionAndRequestIdHandlerOnResponse() assertThat(response, hasStatus(withCode(Status.OK))); assertThat(response, hasHeader("X-Request-Id", is(requestId.get()))); - assertThat(requestId.get(), isValidUUID()); + assertThat(requestId.get(), isValidUuid()); } @Test public void testExceptionAndRequestIdHandlerAcceptsRequestIdHeader() diff --git a/styx-test/src/main/java/com/spotify/styx/util/StringIsValidUUID.java b/styx-test/src/main/java/com/spotify/styx/util/StringIsValidUUID.java index 42fcc47f6b..947481ee02 100644 --- a/styx-test/src/main/java/com/spotify/styx/util/StringIsValidUUID.java +++ b/styx-test/src/main/java/com/spotify/styx/util/StringIsValidUUID.java @@ -26,7 +26,7 @@ import org.hamcrest.Matcher; import org.hamcrest.TypeSafeMatcher; -public class StringIsValidUUID extends TypeSafeMatcher { +public class StringIsValidUuid extends TypeSafeMatcher { @Override protected boolean matchesSafely(String s) { @@ -47,7 +47,7 @@ public void describeTo(Description description) { } @Factory - public static Matcher isValidUUID() { - return new StringIsValidUUID(); + public static Matcher isValidUuid() { + return new StringIsValidUuid(); } } diff --git a/styx-test/src/test/java/com/spotify/styx/util/StringIsValidUUIDTest.java b/styx-test/src/test/java/com/spotify/styx/util/StringIsValidUUIDTest.java index 5ef81144a5..25299b2556 100644 --- a/styx-test/src/test/java/com/spotify/styx/util/StringIsValidUUIDTest.java +++ b/styx-test/src/test/java/com/spotify/styx/util/StringIsValidUUIDTest.java @@ -26,14 +26,14 @@ import java.util.UUID; import org.junit.Test; -public class StringIsValidUUIDTest { +public class StringIsValidUuidTest { @Test public void matchesSafely() { - assertThat(new StringIsValidUUID().matchesSafely(UUID.randomUUID().toString()), is(true)); - assertThat(new StringIsValidUUID().matchesSafely(UUID.randomUUID().toString().replace("-", "")), is(true)); - assertThat(new StringIsValidUUID().matchesSafely("foobar"), is(false)); - assertThat(new StringIsValidUUID().matchesSafely("foo-bar"), is(false)); - assertThat(new StringIsValidUUID().matchesSafely(""), is(false)); + assertThat(new StringIsValidUuid().matchesSafely(UUID.randomUUID().toString()), is(true)); + assertThat(new StringIsValidUuid().matchesSafely(UUID.randomUUID().toString().replace("-", "")), is(true)); + assertThat(new StringIsValidUuid().matchesSafely("foobar"), is(false)); + assertThat(new StringIsValidUuid().matchesSafely("foo-bar"), is(false)); + assertThat(new StringIsValidUuid().matchesSafely(""), is(false)); } } From 8db7d38eae942f6917c7587b260f0edd2b57f0c1 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Tue, 22 May 2018 15:38:00 +0900 Subject: [PATCH 34/37] fix license header --- .../main/java/com/spotify/styx/util/StringIsValidUUID.java | 4 ++-- .../java/com/spotify/styx/util/StringIsValidUUIDTest.java | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/styx-test/src/main/java/com/spotify/styx/util/StringIsValidUUID.java b/styx-test/src/main/java/com/spotify/styx/util/StringIsValidUUID.java index 947481ee02..af851a2e44 100644 --- a/styx-test/src/main/java/com/spotify/styx/util/StringIsValidUUID.java +++ b/styx-test/src/main/java/com/spotify/styx/util/StringIsValidUUID.java @@ -7,9 +7,9 @@ * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. diff --git a/styx-test/src/test/java/com/spotify/styx/util/StringIsValidUUIDTest.java b/styx-test/src/test/java/com/spotify/styx/util/StringIsValidUUIDTest.java index 25299b2556..44c3b800e5 100644 --- a/styx-test/src/test/java/com/spotify/styx/util/StringIsValidUUIDTest.java +++ b/styx-test/src/test/java/com/spotify/styx/util/StringIsValidUUIDTest.java @@ -7,9 +7,9 @@ * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. From 0348b856fa8287f45d59d146b7f2a31ef117bcdc Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Tue, 22 May 2018 15:55:44 +0900 Subject: [PATCH 35/37] fix filename case --- .../styx/util/{StringIsValidUUID.java => StringIsValidUuid.java} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename styx-test/src/main/java/com/spotify/styx/util/{StringIsValidUUID.java => StringIsValidUuid.java} (100%) diff --git a/styx-test/src/main/java/com/spotify/styx/util/StringIsValidUUID.java b/styx-test/src/main/java/com/spotify/styx/util/StringIsValidUuid.java similarity index 100% rename from styx-test/src/main/java/com/spotify/styx/util/StringIsValidUUID.java rename to styx-test/src/main/java/com/spotify/styx/util/StringIsValidUuid.java From 4b52244a8715ecfd88b5218f44f892f8c347bcb3 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Tue, 22 May 2018 16:05:23 +0900 Subject: [PATCH 36/37] fix build --- styx-client/pom.xml | 6 ++++++ .../java/com/spotify/styx/client/StyxApolloClientTest.java | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/styx-client/pom.xml b/styx-client/pom.xml index 985e1fbfaf..a95a9cfdcb 100644 --- a/styx-client/pom.xml +++ b/styx-client/pom.xml @@ -36,6 +36,12 @@ google-api-services-iam + + com.spotify + styx-test + ${project.version} + test + junit junit diff --git a/styx-client/src/test/java/com/spotify/styx/client/StyxApolloClientTest.java b/styx-client/src/test/java/com/spotify/styx/client/StyxApolloClientTest.java index 6e0b724fe1..b43b198eb0 100644 --- a/styx-client/src/test/java/com/spotify/styx/client/StyxApolloClientTest.java +++ b/styx-client/src/test/java/com/spotify/styx/client/StyxApolloClientTest.java @@ -21,7 +21,6 @@ package com.spotify.styx.client; import static com.google.common.collect.Iterables.getLast; -import static com.spotify.styx.StringIsValidUUID.isValidUUID; import static java.util.Arrays.asList; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; @@ -34,6 +33,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static com.spotify.styx.util.StringIsValidUuid.isValidUuid; import com.fasterxml.jackson.core.JsonProcessingException; import com.google.common.collect.ImmutableList; import com.spotify.apollo.Client; @@ -333,7 +333,7 @@ public void testSendsRequestId() throws JsonProcessingException { Response.forStatus(Status.OK).withPayload(Json.serialize(Collections.emptyList())))); styx.workflows(); final Request request = requestCaptor.getValue(); - assertThat(request.header("X-Request-Id").get(), isValidUUID()); + assertThat(request.header("X-Request-Id").get(), isValidUuid()); } @Test From 9c218daf78291611613b98edab0cda4e0c18d2f1 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Tue, 22 May 2018 16:09:41 +0900 Subject: [PATCH 37/37] fix filename case --- .../{StringIsValidUUIDTest.java => StringIsValidUuidTest.java} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename styx-test/src/test/java/com/spotify/styx/util/{StringIsValidUUIDTest.java => StringIsValidUuidTest.java} (100%) diff --git a/styx-test/src/test/java/com/spotify/styx/util/StringIsValidUUIDTest.java b/styx-test/src/test/java/com/spotify/styx/util/StringIsValidUuidTest.java similarity index 100% rename from styx-test/src/test/java/com/spotify/styx/util/StringIsValidUUIDTest.java rename to styx-test/src/test/java/com/spotify/styx/util/StringIsValidUuidTest.java