From 87b35011c0f1fd2b01d8300193e3e27d1b0ca4d9 Mon Sep 17 00:00:00 2001 From: Bernardo Gomez Palacio Date: Fri, 28 Oct 2022 17:36:25 -0700 Subject: [PATCH] Rework the API expressed by DgsExecutionResult (#1298) * We are adopting a Builder pattern instead of depending on Kotlin delegation for the ExecutionResult. This was done to expose a cleaner API for Java Consumers. * We are also moving the `DgsExecutionResult` to not be _internal_, since at is clear from the `MyInstrumentation` example developers will be using it explicitly. --- .../datafetcher/MyInstrumentation.java | 18 ++- .../handlers/DefaultDgsWebfluxHttpHandler.kt | 6 +- .../graphql/dgs/mvc/DgsRestController.kt | 4 +- .../graphql/dgs/mvc/DgsRestControllerTest.kt | 5 +- .../dgs/{internal => }/DgsExecutionResult.kt | 137 ++++++++++-------- .../dgs/internal/BaseDgsQueryExecutor.kt | 23 ++- .../dgs/internal/DgsExecutionResultTest.kt | 32 ++-- 7 files changed, 130 insertions(+), 95 deletions(-) rename graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/{internal => }/DgsExecutionResult.kt (62%) diff --git a/graphql-dgs-example-java/src/main/java/com/netflix/graphql/dgs/example/datafetcher/MyInstrumentation.java b/graphql-dgs-example-java/src/main/java/com/netflix/graphql/dgs/example/datafetcher/MyInstrumentation.java index 4b4c7db30..39668c43f 100644 --- a/graphql-dgs-example-java/src/main/java/com/netflix/graphql/dgs/example/datafetcher/MyInstrumentation.java +++ b/graphql-dgs-example-java/src/main/java/com/netflix/graphql/dgs/example/datafetcher/MyInstrumentation.java @@ -16,10 +16,12 @@ package com.netflix.graphql.dgs.example.datafetcher; -import com.netflix.graphql.dgs.internal.DgsExecutionResult; +import com.netflix.graphql.dgs.DgsExecutionResult; import graphql.ExecutionResult; +import graphql.execution.instrumentation.InstrumentationState; import graphql.execution.instrumentation.SimpleInstrumentation; import graphql.execution.instrumentation.parameters.InstrumentationExecutionParameters; +import org.jetbrains.annotations.NotNull; import org.springframework.http.HttpHeaders; import org.springframework.stereotype.Component; @@ -27,14 +29,18 @@ @Component public class MyInstrumentation extends SimpleInstrumentation { + @NotNull @Override - public CompletableFuture instrumentExecutionResult(ExecutionResult executionResult, InstrumentationExecutionParameters parameters) { + public CompletableFuture instrumentExecutionResult(ExecutionResult executionResult, + InstrumentationExecutionParameters parameters, + InstrumentationState state) { HttpHeaders responseHeaders = new HttpHeaders(); responseHeaders.add("myHeader", "hello"); - return super.instrumentExecutionResult(new DgsExecutionResult( - executionResult, - responseHeaders - ), parameters); + return super.instrumentExecutionResult( + DgsExecutionResult.builder().executionResult(executionResult).headers(responseHeaders).build(), + parameters, + state + ); } } diff --git a/graphql-dgs-spring-webflux-autoconfigure/src/main/kotlin/com/netflix/graphql/dgs/webflux/handlers/DefaultDgsWebfluxHttpHandler.kt b/graphql-dgs-spring-webflux-autoconfigure/src/main/kotlin/com/netflix/graphql/dgs/webflux/handlers/DefaultDgsWebfluxHttpHandler.kt index e43181e97..b70b945cd 100644 --- a/graphql-dgs-spring-webflux-autoconfigure/src/main/kotlin/com/netflix/graphql/dgs/webflux/handlers/DefaultDgsWebfluxHttpHandler.kt +++ b/graphql-dgs-spring-webflux-autoconfigure/src/main/kotlin/com/netflix/graphql/dgs/webflux/handlers/DefaultDgsWebfluxHttpHandler.kt @@ -20,7 +20,7 @@ import com.fasterxml.jackson.core.JsonParseException import com.fasterxml.jackson.databind.ObjectMapper import com.fasterxml.jackson.databind.exc.MismatchedInputException import com.fasterxml.jackson.module.kotlin.readValue -import com.netflix.graphql.dgs.internal.DgsExecutionResult +import com.netflix.graphql.dgs.DgsExecutionResult import com.netflix.graphql.dgs.reactive.DgsReactiveQueryExecutor import graphql.ExecutionResult import org.slf4j.Logger @@ -77,12 +77,12 @@ class DefaultDgsWebfluxHttpHandler( return executionResult.flatMap { result -> val dgsExecutionResult = when (result) { is DgsExecutionResult -> result - else -> DgsExecutionResult(result) + else -> DgsExecutionResult.builder().executionResult(result).build() } ServerResponse .status(dgsExecutionResult.status) - .headers { it.addAll(dgsExecutionResult.headers) } + .headers { it.addAll(dgsExecutionResult.headers()) } .bodyValue(dgsExecutionResult.toSpecification()) }.onErrorResume { ex -> when (ex) { diff --git a/graphql-dgs-spring-webmvc/src/main/kotlin/com/netflix/graphql/dgs/mvc/DgsRestController.kt b/graphql-dgs-spring-webmvc/src/main/kotlin/com/netflix/graphql/dgs/mvc/DgsRestController.kt index 13e149009..024bdd259 100644 --- a/graphql-dgs-spring-webmvc/src/main/kotlin/com/netflix/graphql/dgs/mvc/DgsRestController.kt +++ b/graphql-dgs-spring-webmvc/src/main/kotlin/com/netflix/graphql/dgs/mvc/DgsRestController.kt @@ -21,8 +21,8 @@ import com.fasterxml.jackson.databind.ObjectMapper import com.fasterxml.jackson.databind.exc.MismatchedInputException import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper import com.fasterxml.jackson.module.kotlin.readValue +import com.netflix.graphql.dgs.DgsExecutionResult import com.netflix.graphql.dgs.DgsQueryExecutor -import com.netflix.graphql.dgs.internal.DgsExecutionResult import com.netflix.graphql.dgs.internal.utils.MultipartVariableMapper import com.netflix.graphql.dgs.internal.utils.TimeTracer import graphql.execution.reactive.SubscriptionPublisher @@ -228,7 +228,7 @@ open class DgsRestController( return when (executionResult) { is DgsExecutionResult -> executionResult.toSpringResponse() - else -> DgsExecutionResult(executionResult).toSpringResponse() + else -> DgsExecutionResult.builder().executionResult(executionResult).build().toSpringResponse() } } } diff --git a/graphql-dgs-spring-webmvc/src/test/kotlin/com/netflix/graphql/dgs/mvc/DgsRestControllerTest.kt b/graphql-dgs-spring-webmvc/src/test/kotlin/com/netflix/graphql/dgs/mvc/DgsRestControllerTest.kt index 859e19dca..c5ac88978 100644 --- a/graphql-dgs-spring-webmvc/src/test/kotlin/com/netflix/graphql/dgs/mvc/DgsRestControllerTest.kt +++ b/graphql-dgs-spring-webmvc/src/test/kotlin/com/netflix/graphql/dgs/mvc/DgsRestControllerTest.kt @@ -199,7 +199,10 @@ class DgsRestControllerTest { any(), any() ) - } returns ExecutionResultImpl.newExecutionResult().data(mapOf(Pair("hello", "hello"))).extensions(mutableMapOf(Pair(DgsRestController.DGS_RESPONSE_HEADERS_KEY, mapOf(Pair("myHeader", "hello")))) as Map?).build() + } returns ExecutionResultImpl.newExecutionResult() + .data(mapOf("hello" to "hello")) + .extensions(mutableMapOf(DgsRestController.DGS_RESPONSE_HEADERS_KEY to mapOf("myHeader" to "hello")) as Map?) + .build() val result = DgsRestController(dgsQueryExecutor).graphql(requestBody.toByteArray(), null, null, null, httpHeaders, webRequest) assertThat(result.headers["myHeader"]).contains("hello") diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResult.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/DgsExecutionResult.kt similarity index 62% rename from graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResult.kt rename to graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/DgsExecutionResult.kt index e2177c107..ac5bd9b74 100644 --- a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResult.kt +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/DgsExecutionResult.kt @@ -14,7 +14,7 @@ * limitations under the License. */ -package com.netflix.graphql.dgs.internal +package com.netflix.graphql.dgs import com.fasterxml.jackson.databind.ObjectMapper import com.fasterxml.jackson.databind.exc.InvalidDefinitionException @@ -22,7 +22,6 @@ import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper import com.netflix.graphql.dgs.internal.utils.TimeTracer import graphql.ExecutionResult import graphql.ExecutionResultImpl -import graphql.GraphQLError import graphql.GraphqlErrorBuilder import org.slf4j.Logger import org.slf4j.LoggerFactory @@ -30,68 +29,19 @@ import org.springframework.http.HttpHeaders import org.springframework.http.HttpStatus import org.springframework.http.ResponseEntity -class DgsExecutionResult @JvmOverloads constructor( - val executionResult: ExecutionResult, - val headers: HttpHeaders = HttpHeaders(), - val status: HttpStatus = HttpStatus.OK +class DgsExecutionResult constructor( + private val executionResult: ExecutionResult, + private var headers: HttpHeaders, + val status: HttpStatus ) : ExecutionResult by executionResult { - companion object { - // defined in here and DgsRestController, for backwards compatibility. - // keep these two variables synced. - const val DGS_RESPONSE_HEADERS_KEY = "dgs-response-headers" - private val sentinelObject = Any() - private val logger: Logger = LoggerFactory.getLogger(DgsExecutionResult::class.java) - } init { addExtensionsHeaderKeyToHeader() } - constructor( - status: HttpStatus = HttpStatus.OK, - headers: HttpHeaders = HttpHeaders.EMPTY, - errors: List = listOf(), - extensions: Map? = null, - - // By default, assign data as a sentinel object. - // If we were to default to null here, this constructor - // would be unable to discriminate between an intentionally null - // response and one that the user left default. - data: Any? = sentinelObject - ) : this( - headers = headers, - status = status, - executionResult = ExecutionResultImpl - .newExecutionResult() - .errors(errors) - .extensions(extensions) - .apply { - if (data != sentinelObject) { - data(data) - } - } - .build() - ) - - // for backwards compat with https://github.com/Netflix/dgs-framework/pull/1261. - private fun addExtensionsHeaderKeyToHeader() { - if (executionResult.extensions?.containsKey(DGS_RESPONSE_HEADERS_KEY) == true) { - val dgsResponseHeaders = executionResult.extensions[DGS_RESPONSE_HEADERS_KEY] - - if (dgsResponseHeaders is Map<*, *>) { - dgsResponseHeaders.forEach { - if (it.key != null) { - headers.add(it.key.toString(), it.value?.toString()) - } - } - } else { - logger.warn( - "{} must be of type java.util.Map, but was {}", - DGS_RESPONSE_HEADERS_KEY, - dgsResponseHeaders?.javaClass?.name - ) - } - } + /** Read-Only reference to the HTTP Headers. */ + fun headers(): HttpHeaders { + return HttpHeaders.readOnlyHttpHeaders(headers) } fun toSpringResponse( @@ -117,7 +67,7 @@ class DgsExecutionResult @JvmOverloads constructor( ) } - // overridden for compatibility with https://github.com/Netflix/dgs-framework/pull/1261. + // Refer to https://github.com/Netflix/dgs-framework/pull/1261 for further details. override fun toSpecification(): MutableMap { val spec = executionResult.toSpecification() @@ -133,4 +83,73 @@ class DgsExecutionResult @JvmOverloads constructor( return spec } + + // Refer to https://github.com/Netflix/dgs-framework/pull/1261 for further details. + private fun addExtensionsHeaderKeyToHeader() { + if (executionResult.extensions?.containsKey(DGS_RESPONSE_HEADERS_KEY) == true) { + val dgsResponseHeaders = executionResult.extensions[DGS_RESPONSE_HEADERS_KEY] + if (dgsResponseHeaders is Map<*, *> && dgsResponseHeaders.isNotEmpty()) { + // If the HttpHeaders are empty/read-only we need to switch to a new instance that allows us + // to store the headers that are part of the GraphQL response _extensions_. + if (headers == HttpHeaders.EMPTY) { + headers = HttpHeaders() + } + + dgsResponseHeaders.forEach { + if (it.key != null) { + headers.add(it.key.toString(), it.value?.toString()) + } + } + } else { + logger.warn( + "{} must be of type java.util.Map, but was {}", + DGS_RESPONSE_HEADERS_KEY, + dgsResponseHeaders?.javaClass?.name + ) + } + } + } + + /** + * Facilitate the construction of a [DgsExecutionResult] instance. + */ + class Builder { + var executionResult: ExecutionResult = DEFAULT_EXECUTION_RESULT + private set + + fun executionResult(executionResult: ExecutionResult) = + apply { this.executionResult = executionResult } + + fun executionResult(executionResultBuilder: ExecutionResultImpl.Builder) = + apply { this.executionResult = executionResultBuilder.build() } + + var headers: HttpHeaders = HttpHeaders.EMPTY + private set + + fun headers(headers: HttpHeaders) = apply { this.headers = headers } + + var status: HttpStatus = HttpStatus.OK + private set + + fun status(status: HttpStatus) = apply { this.status = status } + + fun build() = DgsExecutionResult( + executionResult = checkNotNull(executionResult), + headers = headers, + status = status + ) + + companion object { + private val DEFAULT_EXECUTION_RESULT = ExecutionResultImpl.newExecutionResult().build() + } + } + + companion object { + // defined in here and DgsRestController, for backwards compatibility. Keep these two variables synced. + const val DGS_RESPONSE_HEADERS_KEY = "dgs-response-headers" + private val logger: Logger = LoggerFactory.getLogger(DgsExecutionResult::class.java) + + @JvmStatic + fun builder(): Builder = Builder() + } } diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/BaseDgsQueryExecutor.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/BaseDgsQueryExecutor.kt index 79e6e3811..0f6084a1e 100644 --- a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/BaseDgsQueryExecutor.kt +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/BaseDgsQueryExecutor.kt @@ -26,6 +26,7 @@ import com.jayway.jsonpath.Option import com.jayway.jsonpath.ParseContext import com.jayway.jsonpath.spi.json.JacksonJsonProvider import com.jayway.jsonpath.spi.mapper.JacksonMappingProvider +import com.netflix.graphql.dgs.DgsExecutionResult import com.netflix.graphql.dgs.context.DgsContext import com.netflix.graphql.dgs.exceptions.DgsBadRequestException import graphql.ExecutionInput @@ -80,14 +81,20 @@ object BaseDgsQueryExecutor { if (!StringUtils.hasText(query)) { return CompletableFuture.completedFuture( - DgsExecutionResult( - status = HttpStatus.BAD_REQUEST, - errors = listOf( - DgsBadRequestException - .NULL_OR_EMPTY_QUERY_EXCEPTION - .toGraphQlError() - ) - ) + DgsExecutionResult + .builder() + .status(HttpStatus.BAD_REQUEST) + .executionResult( + ExecutionResultImpl + .newExecutionResult() + .errors( + listOf( + DgsBadRequestException + .NULL_OR_EMPTY_QUERY_EXCEPTION + .toGraphQlError() + ) + ) + ).build() ) } diff --git a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResultTest.kt b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResultTest.kt index 5a00741ac..a5525ab61 100644 --- a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResultTest.kt +++ b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResultTest.kt @@ -16,6 +16,8 @@ package com.netflix.graphql.dgs.internal +import com.netflix.graphql.dgs.DgsExecutionResult +import graphql.ExecutionResultImpl import org.assertj.core.api.Assertions.assertThat import org.assertj.core.api.Assertions.entry import org.junit.jupiter.api.Nested @@ -27,17 +29,18 @@ class DgsExecutionResultTest { @Test fun `should be able to pass null for data`() { assertThat( - DgsExecutionResult( - data = null - ).toSpecification() + DgsExecutionResult + .builder() + .executionResult(ExecutionResultImpl.newExecutionResult().data(null)) + .build() + .toSpecification() ).contains(entry("data", null)) } @Test fun `should default to not having data`() { assertThat( - DgsExecutionResult() - .toSpecification() + DgsExecutionResult.builder().build().toSpecification() ).doesNotContainKey("data") } @@ -46,7 +49,10 @@ class DgsExecutionResultTest { val data = "Check under your chair" assertThat( - DgsExecutionResult(data = data) + DgsExecutionResult + .builder() + .executionResult(ExecutionResultImpl.newExecutionResult().data(data)) + .build() .toSpecification() ).contains(entry("data", data)) } @@ -59,11 +65,8 @@ class DgsExecutionResultTest { headers.add("We can add headers now??", "Yes we can") assertThat( - DgsExecutionResult( - headers = headers - ).toSpringResponse() - .headers - .toMap() + DgsExecutionResult.builder().headers(headers).build().toSpringResponse() + .headers.toMap() ).containsAllEntriesOf(headers.toMap()) } @@ -72,11 +75,8 @@ class DgsExecutionResultTest { val httpStatusCode = HttpStatus.ALREADY_REPORTED assertThat( - DgsExecutionResult( - status = httpStatusCode - ).toSpringResponse() - .statusCode - .value() + DgsExecutionResult.builder().status(httpStatusCode).build().toSpringResponse() + .statusCode.value() ).isEqualTo(httpStatusCode.value()) } }