From 93fe19637ac33c3a79f321e79ba75a2dfcc41bde Mon Sep 17 00:00:00 2001 From: Ronald Holshausen Date: Thu, 24 Oct 2024 11:41:59 +1100 Subject: [PATCH] fix: Log the error response bodies from the Pact Broker #1830 --- .../dius/pact/core/pactbroker/HalClient.kt | 30 +++++++++++++++++-- .../pact/core/pactbroker/PactBrokerClient.kt | 4 +++ .../pact/core/pactbroker/HalClientSpec.groovy | 25 ++++++++++++++-- .../junitsupport/loader/PactBrokerLoader.kt | 20 ++++++++++++- .../loader/PactBrokerLoaderSpec.groovy | 12 ++++++++ 5 files changed, 84 insertions(+), 7 deletions(-) diff --git a/core/pactbroker/src/main/kotlin/au/com/dius/pact/core/pactbroker/HalClient.kt b/core/pactbroker/src/main/kotlin/au/com/dius/pact/core/pactbroker/HalClient.kt index 11b77ce2d..3bacd1585 100644 --- a/core/pactbroker/src/main/kotlin/au/com/dius/pact/core/pactbroker/HalClient.kt +++ b/core/pactbroker/src/main/kotlin/au/com/dius/pact/core/pactbroker/HalClient.kt @@ -210,7 +210,7 @@ open class HalClient @JvmOverloads constructor( if (handler != null) { handler(it.code, it) } else if (it.code >= 300) { - logger.error { "PUT JSON request failed with status ${it.code} ${it.reasonPhrase}" } + logger.error { "POST JSON request failed with status ${it.code} ${it.reasonPhrase}" } Result.Err(RequestFailedException(it.code, if (it.entity != null) EntityUtils.toString(it.entity) else null)) } else { true @@ -343,14 +343,38 @@ open class HalClient @JvmOverloads constructor( when (response.code) { 404 -> Result.Err(NotFoundHalResponse("No HAL document found at path '$path'")) else -> { - val body = if (response.entity != null) EntityUtils.toString(response.entity) else null + val body = handleResponseBody(response, path) Result.Err(RequestFailedException(response.code, body, - "Request to path '$path' failed with response ${response.code}")) + "Request to path '$path' failed with HTTP response ${response.code}")) } } } } + private fun handleResponseBody(response: ClassicHttpResponse, path: String): String? { + var body: String? = null + if (response.entity != null) { + body = EntityUtils.toString(response.entity) + val contentType = ContentType.parseLenient(response.entity.contentType) + if (isJsonResponse(contentType)) { + val json = handleWith { JsonParser.parseString(body) } + when (json) { + is Result.Ok -> { + logger.error { "Request to path '$path' failed with HTTP response ${response.code}" } + logger.error { "JSON Response:\n${json.value.prettyPrint()}" } + } + + is Result.Err -> { + logger.error { "Request to path '$path' failed with HTTP response ${response.code}: $body" } + } + } + } else { + logger.error { "Request to path '$path' failed with HTTP response ${response.code}: $body" } + } + } + return body + } + private fun fetchLink(link: String, options: Map): JsonValue.Object { val href = hrefForLink(link, options) return this.fetch(href, false).unwrap() diff --git a/core/pactbroker/src/main/kotlin/au/com/dius/pact/core/pactbroker/PactBrokerClient.kt b/core/pactbroker/src/main/kotlin/au/com/dius/pact/core/pactbroker/PactBrokerClient.kt index 70d77e735..b57abc8ae 100644 --- a/core/pactbroker/src/main/kotlin/au/com/dius/pact/core/pactbroker/PactBrokerClient.kt +++ b/core/pactbroker/src/main/kotlin/au/com/dius/pact/core/pactbroker/PactBrokerClient.kt @@ -473,6 +473,10 @@ open class PactBrokerClient( } } + @Deprecated( + "use version that takes a list of ConsumerVersionSelectors", + replaceWith = ReplaceWith("fetchConsumersWithSelectorsV2") + ) override fun fetchConsumersWithSelectors( providerName: String, selectors: List, diff --git a/core/pactbroker/src/test/groovy/au/com/dius/pact/core/pactbroker/HalClientSpec.groovy b/core/pactbroker/src/test/groovy/au/com/dius/pact/core/pactbroker/HalClientSpec.groovy index 8c9b915ec..9c2e28591 100644 --- a/core/pactbroker/src/test/groovy/au/com/dius/pact/core/pactbroker/HalClientSpec.groovy +++ b/core/pactbroker/src/test/groovy/au/com/dius/pact/core/pactbroker/HalClientSpec.groovy @@ -289,7 +289,7 @@ class HalClientSpec extends Specification { @Unroll @SuppressWarnings('UnnecessaryGetter') - def 'post URL returns #success if the response is #status'() { + def 'post to URL returns #success if the response is #status'() { given: def mockClient = Mock(CloseableHttpClient) client.httpClient = mockClient @@ -308,7 +308,7 @@ class HalClientSpec extends Specification { 'failure' | 400 | Result.Err } - def 'post URL returns a failure result if an exception is thrown'() { + def 'post to URL returns a failure result if an exception is thrown'() { given: def mockClient = Mock(CloseableHttpClient) client.httpClient = mockClient @@ -322,7 +322,7 @@ class HalClientSpec extends Specification { } @SuppressWarnings('UnnecessaryGetter') - def 'post URL delegates to a handler if one is supplied'() { + def 'post to URL delegates to a handler if one is supplied'() { given: def mockClient = Mock(CloseableHttpClient) client.httpClient = mockClient @@ -523,4 +523,23 @@ class HalClientSpec extends Specification { handler.handleResponse(mockResponse) } } + + @Issue('1830') + def 'post to URL handles any error responses'() { + given: + def mockClient = Mock(CloseableHttpClient) + client.httpClient = mockClient + def mockResponse = Mock(ClassicHttpResponse) { + getCode() >> 400 + getEntity() >> new StringEntity('{"error": ["it went bang!"]}', ContentType.APPLICATION_JSON) + } + client.pathInfo = JsonParser.INSTANCE.parseString('{"_links":{"path":{"href":"http://localhost:8080/"}}}') + + when: + def result = client.postJson('path', [:], '"body"') + + then: + 1 * mockClient.execute(_, _, _) >> { req, c, handler -> handler.handleResponse(mockResponse) } + result instanceof Result.Err + } } diff --git a/provider/src/main/kotlin/au/com/dius/pact/provider/junitsupport/loader/PactBrokerLoader.kt b/provider/src/main/kotlin/au/com/dius/pact/provider/junitsupport/loader/PactBrokerLoader.kt index 16d02ab4b..b3d18d0ce 100644 --- a/provider/src/main/kotlin/au/com/dius/pact/provider/junitsupport/loader/PactBrokerLoader.kt +++ b/provider/src/main/kotlin/au/com/dius/pact/provider/junitsupport/loader/PactBrokerLoader.kt @@ -12,6 +12,7 @@ import au.com.dius.pact.core.pactbroker.ConsumerVersionSelectors import au.com.dius.pact.core.pactbroker.IPactBrokerClient import au.com.dius.pact.core.pactbroker.PactBrokerClient import au.com.dius.pact.core.pactbroker.PactBrokerClientConfig +import au.com.dius.pact.core.pactbroker.RequestFailedException import au.com.dius.pact.core.support.Result import au.com.dius.pact.core.support.Utils.permutations import au.com.dius.pact.core.support.expressions.DataType @@ -249,7 +250,24 @@ open class PactBrokerLoader( providerBranch, pending, wipSinceDate) var consumers = when (result) { is Result.Ok -> result.value - is Result.Err -> throw result.error + is Result.Err -> { + when (val exception = result.error) { + is RequestFailedException -> { + logger.error(exception) { + if (exception.body.isNotEmpty()) { + "Failed to load Pacts from the Pact broker: ${exception.message} (HTTP Status ${exception.status})" + + "\nResponse: ${exception.body}" + } else { + "Failed to load Pacts from the Pact broker: ${exception.message} (HTTP Status ${exception.status})" + } + } + } + else -> { + logger.error(exception) { "Failed to load Pacts from the Pact broker " } + } + } + throw result.error + } } if (failIfNoPactsFound && consumers.isEmpty()) { diff --git a/provider/src/test/groovy/au/com/dius/pact/provider/junitsupport/loader/PactBrokerLoaderSpec.groovy b/provider/src/test/groovy/au/com/dius/pact/provider/junitsupport/loader/PactBrokerLoaderSpec.groovy index 11c436dcd..0a7d1c9b6 100644 --- a/provider/src/test/groovy/au/com/dius/pact/provider/junitsupport/loader/PactBrokerLoaderSpec.groovy +++ b/provider/src/test/groovy/au/com/dius/pact/provider/junitsupport/loader/PactBrokerLoaderSpec.groovy @@ -8,6 +8,7 @@ import au.com.dius.pact.core.pactbroker.IPactBrokerClient import au.com.dius.pact.core.pactbroker.InvalidHalResponse import au.com.dius.pact.core.pactbroker.InvalidNavigationRequest import au.com.dius.pact.core.pactbroker.PactBrokerResult +import au.com.dius.pact.core.pactbroker.RequestFailedException import au.com.dius.pact.core.support.expressions.DataType import au.com.dius.pact.core.support.expressions.ExpressionParser import au.com.dius.pact.core.support.expressions.SystemPropertyResolver @@ -1368,6 +1369,17 @@ class PactBrokerLoaderSpec extends Specification { thrown(InvalidNavigationRequest) } + @Issue('#1830') + def 'Handles error responses from the broker'() { + when: + pactBrokerLoader().load('test') + + then: + 1 * brokerClient.fetchConsumersWithSelectorsV2('test', [], [], '', false, '') >> + new Result.Err(new RequestFailedException(400, '{"error": "selectors are invalid"}', 'Request to broker failed')) + thrown(RequestFailedException) + } + void 'Does not enable insecure TLS when not set in PactBroker annotation and not using the fallback system property'() { given: pactBrokerLoader = {