https://kotlinlang.org logo
Title
t

Tobias

08/11/2021, 5:58 AM
I'm converting a jaxrs system which heavily depends on converting exceptions thrown in the code to status codes. I'm trying to do something similar in Graphql by returning a specific specification and having all queries/mutations return class that "implements" that specification. Basically, all classes get an
error: CommonErrorClass?
. It will be a bit hackish, but with some automatic schema validation it should work reliably. The problem is, I can figure out how to turn an
Exception
into a custom specification?
I've subclassed
SimpleKotlinDataFetcherFactoryProvider
and
FunctionDataFetcher
so I can catch the exceptions. I've tried to different solutions: 1. if I return another type, java reflections becomes angry because I return a different type than declared fromt he get emthod 2. If I throw an
Exception
(
GraphqlError
), I can't get the
toSpecification()
to be invoked
s

Shane Myrick

08/11/2021, 6:22 PM
You can implement
DataFetcherExceptionHandler
from graphql-java, which will allow you to get the exception thrown and run custom logic to create the
GraphQLError
and
DataFetcherExceptionHandlerResult
See an example here: https://github.com/ExpediaGroup/graphql-kotlin/blob/3832f7fc67f1a1b020071585f49f88[…]s/server/spring/exceptions/CustomDataFetcherExceptionHandler.kt
t

Tobias

08/11/2021, 7:46 PM
Thanks. I didn't know I could change that class in graphql-java. I'm not using spring, so it's a bit different, but that looks like it. Thanks again! 🙇
d

Dariusz Kuc

08/11/2021, 7:51 PM
toSpecification
creates just a Map representation of an object -> personally I'd just do
suspend fun foo(): MyResult = try {
   // do whatever
} catch (e: Exception) {
   MyResult(
      error = e.toCommonError()
   )
}

internal fun Exception.toCommonError(): CommonErrorClass {
   // convert exception to your object here
}
otherwise in this exception handler you would have to know how to construct
MyResult
object
t

Tobias

08/11/2021, 8:08 PM
Yes, but there are literally hundreds of methods and that boiler plate code would be necessary in all of them. I've made similar constructs with Grpc, but I'd prefer not to. MyResult is not really what I'm creating. The typing would be something like
class MyResult(
    override val error: CommonErrorClass?,
    val data: Foo?
) : CommonResponseClass
So all that is necessary is that I return
{ "error": {....}}
Ofc, there's a few implied contracts here, most notably that all queries & mutations must request the error field, and that all responses really do implement CommonResponseClass. The good thing is that the people consuming the API will also notice if a response isn't implementing it, because they will in turn make an error-handling construct on their side that does the same thing in inverse (ie, checks that
error
is null or else it's an error)
d

Dariusz Kuc

08/11/2021, 8:11 PM
well your response is
{
  "data": {
    "myQuery": {
      "error": {...}
    }
  }
}
t

Tobias

08/11/2021, 8:11 PM
sorry, yes
Very new to graphql
d

Dariusz Kuc

08/11/2021, 8:13 PM
in
DataFetcherExceptionHandler
you can create custom response but you still would need to have some logic there, i.e.
DataFetcherExceptionHandlerResult.newResult().error(error).build(). // returns no data and just errors
so your logic would have to know how to construct
myQuery: MyResult
response
t

Tobias

08/11/2021, 8:14 PM
right.. Maybe I should just stick to the old slightly icky solution where I fake all attributes if I get an error
At least that works
d

Dariusz Kuc

08/11/2021, 8:15 PM
it could be as simple as
val myResult = mapOf("error" to CommonErrorClass())
val completeResult = mapOf("myQuery" to myResult) // <- you need to know about `myQuery`
DataFetcherExceptionHandlerResult.newResult().build()
t

Tobias

08/11/2021, 8:15 PM
yes
that's what I finally got at "right..."
d

Dariusz Kuc

08/11/2021, 8:15 PM
since you have source location and path probably you could do it
t

Tobias

08/11/2021, 8:16 PM
still feels worse than the currrent solution
Current solution is having the try/catch in
FunctionDataFetcher.runBlockingFunction
returning a type that just implements the
CommonResponse
(which contains the
error: ..
)
d

Dariusz Kuc

08/11/2021, 8:17 PM
you could wrap that common logic in some function
t

Tobias

08/11/2021, 8:18 PM
Then the
DataFetcher.get
checks if it's the error-type and returns null for everything but the
error
field
d

Dariusz Kuc

08/11/2021, 8:18 PM
in general I'd try to use suspendable functions over the blocking ones (but i guess it should work if you use proper execution strategy to correctly parallelize the calls)
t

Tobias

08/11/2021, 8:19 PM
I try/catch in both properly. They both look like
override fun runBlockingFunction(parameterValues: Map<KParameter, Any?>): Any? =
    try {
        kFunction.callBy(parameterValues)
    } catch (exception: InvocationTargetException) {
        mapException(exception)
    }
and
override fun runSuspendingFunction(
    parameterValues: Map<KParameter, Any?>,
    coroutineContext: CoroutineContext,
    coroutineStart: CoroutineStart
): CompletableFuture<Any?> = ourScope.future(context = coroutineContext, start = coroutineStart) {
    try {
        kFunction.callSuspendBy(parameterValues)
    } catch (exception: InvocationTargetException) {
        mapException(exception)
    }
}
👍 1
Like I said, tested and works well. Benefit is that even if the client doesn't ask for the error, it still works
override fun get(environment: DataFetchingEnvironment): Any? = environment.getSource<Any?>()?.let { instance ->
                if (instance is CommonResponseClass) {
                    if (kProperty.name == "error") {
                        instance.error
                    } else {
                        null
                    }
                } else {
                    kProperty.call(instance)
                }
            }
One thing that will really blow up everything is if the response neither implements CommonResponseClass nor has every property nullable. But to do that it's left the "mistake" and entered the "willfully evil" territory, and thankfully I have no such colleagues
d

Dariusz Kuc

08/11/2021, 8:26 PM
you could avoid that by moving this common logic to target functions
t

Tobias

08/11/2021, 8:27 PM
I'm not sure I understand? Do you mean by having the try/catch in every graphql method?
Btw, we're using Armeria and have enabled blockingTaskExecutor to avoid blocking the event thread.
d

Dariusz Kuc

08/11/2021, 8:30 PM
yeah doing try/catch in every method would give you type safety
suspend fun foo(): Foo = withCommonError {
   // whatever
}

suspend fun inline reified<T: CommonType> CoroutineScope.withCommonError(block: () -> T) = try {
  block.apply()
} catch (e: Exception) {
  // need to create T somehow -> guess using reflections could work
}
*block should be suspendable as well
t

Tobias

08/11/2021, 8:36 PM
Tbh, I'm still not convinced. I have no problem using reflections, but it makes the code much harder to read and often more fragile. I still would get the problem with that just forgetting to have
= withCommonError {
and just putting a
{
at the end will break things silently in runtime.
I think it would be easier to just further alter
SchemaGeneratorHooks
to make sure all function return classes extend the error interface
Btw, lest I forget. Thanks for a very good framework!
👍 1
d

Dariusz Kuc

08/11/2021, 8:49 PM
you could always do
withCommonError(Foo()) { foo -> ... }
(pass an instance to the lambda to avoid reflections)
since that is runtime logic don't think it would be easy to detect missing this wrapper generically
guess unit tests could handle some of it
handling this logic in datafetcher makes it more centralized and easier to test
t

Tobias

08/11/2021, 8:57 PM
That's where I'm doing it now
Either way, it's getting late here, so I wish you a pleasant evening and I'm off to bed. Thanks for all your help and input. Now I at least know what the proper solution is, even if don't use it 😉
👋 1
r

rocketraman

08/16/2021, 10:23 PM
I found being very explicit in the schema/return types was a good way to do this. For example:
fun myResolver(): MyPayload

sealed interface MyPayload

data class MyPayloadSuccess(
  ...non-null fields here...
) : MyPayload

sealed class BaseError(
  // base error fields here
)

// error classes all extend BaseError and 
// implement as many payload types as
// necessary
class SomeError(...error fields)
  : BaseError(...)
  , MyPayload
  , OtherPayload
  , Etc
Its a bit of work to set up all the types, but provides fully type-safe success and error responses. I find it works out very nicely. See also this issue/post (by me) for background: https://github.com/graphql-rules/graphql-rules/issues/62.
👍 1
t

Tobias

08/17/2021, 6:02 AM
Yes, that's similar to what I was initially aiming for, although my idea was
union LikePostPayload = LikePostSuccess | LikePostError
where the error contains the data. What you're doing otherwise is possibly making clients silently get incorrect data, as the client would (without recompiling with new schema) have no way of knowing how to handle the following correctly:
union LikePostPayload = LikePostSuccess | LikePostError | LikePostMyNewError
That would break forward compatibility.
The reason I refrained from doing that is that unlike what I hoped, I found it impossible to get more advanced inheritance working in graphql so it would just flood the Graphql schema with interfaces and subclasses. Not sure if it's a limitation in graphql or kotlin-graphql, but it looks like the former. I was hoping for something like this: (the following does NOT work)
abstract class BaseResponse(
    val error: ErrorType?,
    val data: Any
) {
    init {
        check((error == null) != (data == null))
    }
}

class LikePostResponse(
    error: ErrorType? = null,
    data: LikePostPayload? = null
) : BaseResponse(error, data)
r

rocketraman

08/17/2021, 6:14 AM
he client would (without recompiling with new schema) have no way of knowing how to handle the following correctly:
Because both LikePostError and LikePostMyNewError extend from a common base error class, I don't believe LikePostMyNewError breaks forward compatibility.
I was hoping for something like this
Why? I see no obvious advantages to this approach at all.
t

Tobias

08/17/2021, 6:22 AM
One base class with validation logic, presenting a clear common contract to the client, and no extra boilerplate classes cluttering neither the Kotlin side, nor the graphql schema aside from the necessary Response + Payload
That's what I was aiming for anyways.
afaik union is not inherianace. Tbh, I have no idea how graphql transfers type information when it comes to unions over the wire, but it's seems something similar to
"__typename": "LikePostSuccess"
, and you can basically get null back then (as the client doesn't ask for the extra "LikePostMyNewError") At least if my skimming of this link is correct https://graphql.org/learn/schema/#union-types
r

rocketraman

08/17/2021, 6:30 AM
• The validation logic is unnecessary with the typed approach, as the sealed payload types enforce either success or error. • The payload types present a very clear common contract to clients as union types are native to graphql, and handled well by every client I've seen. • I don't actually see a lot of boilerplate with the approach I outlined -- if you want typed success and error responses, you've got to have the types. The only "boilerplate" is some base sealed marker interfaces and, for errors, a base error class.
For the union, the client can ask for:
... on BaseError {
  whatever base error fields
}
without any knowledge of the specific error types.
kotlin-graphql exposes the
sealed class BaseError
as a GraphQL interface type (https://graphql.org/learn/schema/#interfaces).
t

Tobias

08/17/2021, 6:37 AM
hmm.. ok. The boilerplate is all the different errors and the sealed error class (per mutation). Adding another error requires redoing all queries in addition to actually handling it.
r

rocketraman

08/17/2021, 6:39 AM
Adding error types is optional and only necessary if you actually have useful additional fields to send in the error, or the error type itself is useful to the client. If you have that case, you'd need to redo queries / handle it client-side no matter what.
If a generic error with a message and code or whatever is good enough, you can use that everywhere: union WhateverPayload = WhateverPayloadSuccess | GenericError
I'd still have GenericError extend BaseError (and on the client side always look for BaseError) just in case you do want to add additional error types later.
t

Tobias

08/17/2021, 6:45 AM
hmm.. ok it does seems to hold together
👍 1