atlassian-labs · gnawf · May 24, 2024
diff --git a/lib/src/main/java/graphql/nadel/NadelUncaughtExecutionError.kt b/lib/src/main/java/graphql/nadel/NadelUncaughtExecutionError.kt
@@ -0,0 +1,12 @@
+package graphql.nadel
+
+import graphql.nadel.error.NadelGraphQLError
+
+internal class NadelUncaughtExecutionError(
+    message: String,
+    extensions: Map<String, Any?>,
+    val cause: Throwable,
+) : NadelGraphQLError(
+    message = message,
+    extensions = extensions,
+)
diff --git a/lib/src/main/java/graphql/nadel/NextgenEngine.kt b/lib/src/main/java/graphql/nadel/NextgenEngine.kt
@@ -1,6 +1,5 @@
 package graphql.nadel
 
-import graphql.ErrorType
 import graphql.ExecutionInput
 import graphql.ExecutionResult
 import graphql.GraphQLError
@@ -27,17 +26,15 @@ import graphql.nadel.engine.util.compileToDocument
 import graphql.nadel.engine.util.copy
 import graphql.nadel.engine.util.getOperationKind
 import graphql.nadel.engine.util.newExecutionResult
-import graphql.nadel.engine.util.newGraphQLError
 import graphql.nadel.engine.util.newServiceExecutionErrorResult
 import graphql.nadel.engine.util.newServiceExecutionResult
 import graphql.nadel.engine.util.provide
 import graphql.nadel.engine.util.singleOfType
 import graphql.nadel.engine.util.strictAssociateBy
 import graphql.nadel.hooks.NadelExecutionHooks
 import graphql.nadel.instrumentation.NadelInstrumentation
-import graphql.nadel.instrumentation.parameters.ErrorData
-import graphql.nadel.instrumentation.parameters.ErrorType.ServiceExecutionError
-import graphql.nadel.instrumentation.parameters.NadelInstrumentationOnErrorParameters
+import graphql.nadel.instrumentation.parameters.NadelInstrumentationOnExceptionParameters
+import graphql.nadel.instrumentation.parameters.NadelInstrumentationOnGraphQLErrorsParameters
 import graphql.nadel.instrumentation.parameters.NadelInstrumentationTimingParameters.ChildStep.Companion.DocumentCompilation
 import graphql.nadel.instrumentation.parameters.NadelInstrumentationTimingParameters.RootStep
 import graphql.nadel.instrumentation.parameters.child
@@ -160,6 +157,7 @@ internal class NextgenEngine(
                 executionHooks,
                 executionHints,
                 instrumentationState,
+                executionInput.executionId ?: executionIdProvider.provide(executionInput),
                 timer,
                 incrementalResultSupport,
             )
@@ -187,7 +185,17 @@ internal class NextgenEngine(
                                     )
                                 } catch (e: Throwable) {
                                     when (e) {
-                                        is GraphQLError -> newServiceExecutionErrorResult(field, error = e)
+                                        is GraphQLError -> {
+                                            instrumentation.onException(
+                                                NadelInstrumentationOnExceptionParameters(
+                                                    exception = e,
+                                                    instrumentationState = instrumentationState,
+                                                    serviceName = service.name,
+                                                ),
+                                            )
+
+                                            newServiceExecutionErrorResult(field, error = e)
+                                        }
                                         else -> throw e
                                     }
                                 }
@@ -218,7 +226,17 @@ internal class NextgenEngine(
             }
         } catch (e: Throwable) {
             when (e) {
-                is GraphQLError -> return newExecutionResult(error = e)
+                is GraphQLError -> {
+                    instrumentation.onException(
+                        NadelInstrumentationOnExceptionParameters(
+                            exception = e,
+                            instrumentationState = instrumentationState,
+                            serviceName = null,
+                        ),
+                    )
+
+                    return newExecutionResult(error = e)
+                }
                 else -> throw e
             }
         }
@@ -266,10 +284,10 @@ internal class NextgenEngine(
                 executionHydrationDetails = executionContext.hydrationDetails,
             )
         }
-        val transformedResult: ServiceExecutionResult = when {
-            topLevelField.name.startsWith("__") -> result
-            else -> timer.time(step = RootStep.ResultTransforming) {
-                resultTransformer.transform(
+
+        if (!topLevelField.name.startsWith("__")) {
+            val transformResult = timer.time(step = RootStep.ResultTransforming) {
+                resultTransformer.mutate(
                     executionContext = executionContext,
                     executionPlan = executionPlan,
                     artificialFields = queryTransform.artificialFields,
@@ -278,9 +296,19 @@ internal class NextgenEngine(
                     result = result,
                 )
             }
+
+            if (transformResult.errorsAdded.isNotEmpty()) {
+                instrumentation.onGraphQLErrors(
+                    NadelInstrumentationOnGraphQLErrorsParameters(
+                        errors = transformResult.errorsAdded,
+                        instrumentationState = executionContext.instrumentationState,
+                        serviceName = service.name,
+                    ),
+                )
+            }
         }
 
-        return transformedResult
+        return result
     }
 
     private suspend fun executeService(
@@ -310,7 +338,7 @@ internal class NextgenEngine(
             query = compileResult.document,
             context = executionInput.context,
             graphQLContext = executionInput.graphQLContext,
-            executionId = executionInput.executionId ?: executionIdProvider.provide(executionInput),
+            executionId = executionContext.executionId,
             variables = compileResult.variables,
             operationDefinition = compileResult.document.definitions.singleOfType(),
             serviceContext = executionContext.getContextForService(service).await(),
@@ -324,34 +352,30 @@ internal class NextgenEngine(
                 .asDeferred()
                 .await()
         } catch (e: Exception) {
-            val errorMessage = "An exception occurred invoking the service '${service.name}'"
-            val errorMessageNotSafe = "$errorMessage: ${e.message}"
-            val executionId = serviceExecParams.executionId.toString()
+            val serviceName = service.name
 
-            instrumentation.onError(
-                NadelInstrumentationOnErrorParameters(
-                    message = errorMessage,
+            instrumentation.onException(
+                NadelInstrumentationOnExceptionParameters(
                     exception = e,
                     instrumentationState = executionContext.instrumentationState,
-                    errorType = ServiceExecutionError,
-                    errorData = ErrorData.ServiceExecutionErrorData(
-                        executionId = executionId,
-                        serviceName = service.name
-                    )
-                )
+                    serviceName = serviceName,
+                ),
             )
 
-            newServiceExecutionResult(
-                errors = mutableListOf(
-                    newGraphQLError(
-                        message = errorMessageNotSafe, // End user can receive not safe message
-                        errorType = ErrorType.DataFetchingException,
+            if (e is GraphQLError) {
+                newServiceExecutionResult(e)
+            } else {
+                val exceptionClass = e.javaClass.simpleName
+                newServiceExecutionResult(
+                    NadelUncaughtExecutionError(
+                        message = "An $exceptionClass occurred invoking the service $serviceName",
+                        cause = e,
                         extensions = mutableMapOf(
-                            "executionId" to executionId,
+                            "executionId" to serviceExecParams.executionId.toString(),
                         ),
-                    ).toSpecification(),
-                ),
-            )
+                    ),
+                )
+            }
         }
 
         return serviceExecResult.copy(

diff --git a/lib/src/main/java/graphql/nadel/engine/NadelExecutionContext.kt b/lib/src/main/java/graphql/nadel/engine/NadelExecutionContext.kt
@@ -2,6 +2,7 @@ package graphql.nadel.engine
 
 import graphql.ExecutionInput
 import graphql.GraphQLContext
+import graphql.execution.ExecutionId
 import graphql.execution.instrumentation.InstrumentationState
 import graphql.nadel.NadelExecutionHints
 import graphql.nadel.Service
@@ -19,6 +20,7 @@ data class NadelExecutionContext internal constructor(
     internal val hooks: NadelExecutionHooks,
     val hints: NadelExecutionHints,
     val instrumentationState: InstrumentationState?,
+    val executionId: ExecutionId,
     internal val timer: NadelInstrumentationTimer,
     internal val incrementalResultSupport: NadelIncrementalResultSupport,
     internal val hydrationDetails: ServiceExecutionHydrationDetails? = null,

diff --git a/lib/src/main/java/graphql/nadel/engine/transform/result/NadelResultTransformer.kt b/lib/src/main/java/graphql/nadel/engine/transform/result/NadelResultTransformer.kt
@@ -1,5 +1,6 @@
 package graphql.nadel.engine.transform.result
 
+import graphql.GraphQLError
 import graphql.nadel.Service
 import graphql.nadel.ServiceExecutionResult
 import graphql.nadel.engine.NadelExecutionContext
@@ -15,15 +16,21 @@ import kotlinx.coroutines.async
 import kotlinx.coroutines.awaitAll
 import kotlinx.coroutines.coroutineScope
 
-internal class NadelResultTransformer(private val executionBlueprint: NadelOverallExecutionBlueprint) {
-    suspend fun transform(
+internal class NadelResultTransformer(
+    private val executionBlueprint: NadelOverallExecutionBlueprint,
+) {
+    internal data class MutateResult(
+        val errorsAdded: List<GraphQLError>,
+    )
+
+    suspend fun mutate(
         executionContext: NadelExecutionContext,
         executionPlan: NadelExecutionPlan,
         artificialFields: List<ExecutableNormalizedField>,
         overallToUnderlyingFields: Map<ExecutableNormalizedField, List<ExecutableNormalizedField>>,
         service: Service,
         result: ServiceExecutionResult,
-    ): ServiceExecutionResult {
+    ): MutateResult {
         val nodes = JsonNodes(result.data)
 
         val deferredInstructions = ArrayList<Deferred<List<NadelResultInstruction>>>()
@@ -63,11 +70,16 @@ internal class NadelResultTransformer(private val executionBlueprint: NadelOvera
 
         val instructions = deferredInstructions
             .awaitAll()
-            .flatten()
+            .flatten() // todo: why do we even flatten here? we can just iterate
 
         mutate(result, instructions)
 
-        return result
+        val errors = instructions
+            .mapNotNull {
+                (it as? NadelResultInstruction.AddError)?.error
+            }
+
+        return MutateResult(errors)
     }
 
     private fun mutate(result: ServiceExecutionResult, instructions: List<NadelResultInstruction>) {

diff --git a/lib/src/main/java/graphql/nadel/error/NadelGraphQLError.kt b/lib/src/main/java/graphql/nadel/error/NadelGraphQLError.kt
@@ -0,0 +1,32 @@
+package graphql.nadel.error
+
+import graphql.ErrorClassification
+import graphql.GraphQLError
+import graphql.language.SourceLocation
+
+internal abstract class NadelGraphQLError(
+    private val message: String,
+    private val extensions: Map<String, Any?>,
+    private val path: List<Any>? = null,
+    private val errorClassification: ErrorClassification? = null,
+) : GraphQLError {
+    override fun getMessage(): String {
+        return message
+    }
+
+    override fun getLocations(): MutableList<SourceLocation>? {
+        return null
+    }
+
+    override fun getPath(): List<Any>? {
+        return path
+    }
+
+    override fun getErrorType(): ErrorClassification {
+        return errorClassification ?: ErrorClassification.errorClassification(javaClass.simpleName)
+    }
+
+    override fun getExtensions(): Map<String, Any?> {
+        return extensions
+    }
+}
diff --git a/lib/src/main/java/graphql/nadel/instrumentation/ChainedNadelInstrumentation.kt b/lib/src/main/java/graphql/nadel/instrumentation/ChainedNadelInstrumentation.kt
@@ -7,7 +7,8 @@ import graphql.execution.instrumentation.InstrumentationState
 import graphql.language.Document
 import graphql.nadel.instrumentation.parameters.NadelInstrumentationCreateStateParameters
 import graphql.nadel.instrumentation.parameters.NadelInstrumentationExecuteOperationParameters
-import graphql.nadel.instrumentation.parameters.NadelInstrumentationOnErrorParameters
+import graphql.nadel.instrumentation.parameters.NadelInstrumentationOnExceptionParameters
+import graphql.nadel.instrumentation.parameters.NadelInstrumentationOnGraphQLErrorsParameters
 import graphql.nadel.instrumentation.parameters.NadelInstrumentationQueryExecutionParameters
 import graphql.nadel.instrumentation.parameters.NadelInstrumentationQueryValidationParameters
 import graphql.nadel.instrumentation.parameters.NadelInstrumentationTimingParameters
@@ -50,10 +51,17 @@ class ChainedNadelInstrumentation(
         }
     }
 
-    override fun onError(parameters: NadelInstrumentationOnErrorParameters) {
+    override fun onException(parameters: NadelInstrumentationOnExceptionParameters) {
         instrumentations.forEach { instrumentation: NadelInstrumentation ->
             val state = getStateFor(instrumentation, parameters.getInstrumentationState()!!)
-            instrumentation.onError(parameters.copy(instrumentationState = state))
+            instrumentation.onException(parameters.copy(instrumentationState = state))
+        }
+    }
+
+    override fun onGraphQLErrors(parameters: NadelInstrumentationOnGraphQLErrorsParameters) {
+        instrumentations.forEach { instrumentation: NadelInstrumentation ->
+            val state = getStateFor(instrumentation, parameters.getInstrumentationState()!!)
+            instrumentation.onGraphQLErrors(parameters.copy(instrumentationState = state))
         }
     }
 

diff --git a/lib/src/main/java/graphql/nadel/instrumentation/NadelInstrumentation.kt b/lib/src/main/java/graphql/nadel/instrumentation/NadelInstrumentation.kt
@@ -1,13 +1,15 @@
 package graphql.nadel.instrumentation
 
 import graphql.ExecutionResult
+import graphql.GraphQLError
 import graphql.execution.instrumentation.InstrumentationContext
 import graphql.execution.instrumentation.InstrumentationState
 import graphql.execution.instrumentation.SimpleInstrumentationContext.noOp
 import graphql.language.Document
 import graphql.nadel.instrumentation.parameters.NadelInstrumentationCreateStateParameters
 import graphql.nadel.instrumentation.parameters.NadelInstrumentationExecuteOperationParameters
-import graphql.nadel.instrumentation.parameters.NadelInstrumentationOnErrorParameters
+import graphql.nadel.instrumentation.parameters.NadelInstrumentationOnExceptionParameters
+import graphql.nadel.instrumentation.parameters.NadelInstrumentationOnGraphQLErrorsParameters
 import graphql.nadel.instrumentation.parameters.NadelInstrumentationQueryExecutionParameters
 import graphql.nadel.instrumentation.parameters.NadelInstrumentationQueryValidationParameters
 import graphql.nadel.instrumentation.parameters.NadelInstrumentationTimingParameters
@@ -103,12 +105,18 @@ interface NadelInstrumentation {
     }
 
     /**
-     * Called when Nadel encounters an error that can be useful for the caller code - for logging or metrics, for example.
+     * Called when Nadel receives [GraphQLError]s to inject into the response.
      *
-     * The nature of the error can be obtained by looking at the value of [NadelInstrumentationOnErrorParameters.errorType].
+     * @param parameters to this step
+     */
+    fun onGraphQLErrors(parameters: NadelInstrumentationOnGraphQLErrorsParameters) {
+    }
+
+    /**
+     * Called when Nadel catches an [Exception] so the caller can perform extra handling.
      *
-     *  @param parameters to this step
+     * @param parameters to this step
      */
-    fun onError(parameters: NadelInstrumentationOnErrorParameters) {
+    fun onException(parameters: NadelInstrumentationOnExceptionParameters) {
     }
 }
diff --git a/.../NadelInstrumentationOnErrorParameters.kt → ...elInstrumentationOnExceptionParameters.kt b/.../NadelInstrumentationOnErrorParameters.kt → ...elInstrumentationOnExceptionParameters.kt
@@ -2,11 +2,12 @@ package graphql.nadel.instrumentation.parameters
 
 import graphql.execution.instrumentation.InstrumentationState
 
-data class NadelInstrumentationOnErrorParameters(
-    val message: String,
+data class NadelInstrumentationOnExceptionParameters(
     val exception: Throwable,
-    val errorType: ErrorType,
-    val errorData: ErrorData,
+    /**
+     * Which service was being executed, if known.
+     */
+    val serviceName: String?,
     private val instrumentationState: InstrumentationState?,
 ) {
     fun <T : InstrumentationState?> getInstrumentationState(): T? {
@@ -15,13 +16,3 @@ data class NadelInstrumentationOnErrorParameters(
     }
 }
 
-sealed interface ErrorData {
-    data class ServiceExecutionErrorData(
-        val executionId: String,
-        val serviceName: String,
-    ) : ErrorData
-}
-
-enum class ErrorType {
-    ServiceExecutionError
-}
diff --git a/...graphql/nadel/instrumentation/parameters/NadelInstrumentationOnGraphQLErrorsParameters.kt b/...graphql/nadel/instrumentation/parameters/NadelInstrumentationOnGraphQLErrorsParameters.kt
@@ -0,0 +1,15 @@
+package graphql.nadel.instrumentation.parameters
+
+import graphql.GraphQLError
+import graphql.execution.instrumentation.InstrumentationState
+
+data class NadelInstrumentationOnGraphQLErrorsParameters(
+    val errors: List<GraphQLError>,
+    val serviceName: String,
+    private val instrumentationState: InstrumentationState?,
+) {
+    fun <T : InstrumentationState?> getInstrumentationState(): T? {
+        @Suppress("UNCHECKED_CAST") // trust the caller
+        return instrumentationState as T?
+    }
+}