https://kotlinlang.org logo
Join Slack
Powered by
I'm converting a jaxrs system which heavily depend...
# graphql-kotlin
t
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
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
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
toSpecification
creates just a Map representation of an object -> personally I'd just do
Copy code
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
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
Copy code
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
well your response is
Copy code
{
  "data": {
    "myQuery": {
      "error": {...}
    }
  }
}
t
sorry, yes
Very new to graphql
d
in 
DataFetcherExceptionHandler
you can create custom response but you still would need to have some logic there, i.e.
Copy code
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
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
it could be as simple as
Copy code
val myResult = mapOf("error" to CommonErrorClass())
val completeResult = mapOf("myQuery" to myResult) // <- you need to know about `myQuery`
DataFetcherExceptionHandlerResult.newResult().build()
t
yes
that's what I finally got at "right..."
d
since you have source location and path probably you could do it
t
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
you could wrap that common logic in some function
t
Then the
Copy code
DataFetcher.get
checks if it's the error-type and returns null for everything but the 
error
field
d
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
I try/catch in both properly. They both look like
Copy code
override fun runBlockingFunction(parameterValues: Map<KParameter, Any?>): Any? =
    try {
        kFunction.callBy(parameterValues)
    } catch (exception: InvocationTargetException) {
        mapException(exception)
    }
and
Copy code
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
Copy code
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
you could avoid that by moving this common logic to target functions
t
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
yeah doing try/catch in every method would give you type safety
Copy code
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
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
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
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
I found being very explicit in the schema/return types was a good way to do this. For example:
Copy code
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
Yes, that's similar to what I was initially aiming for, although my idea was
Copy code
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:
Copy code
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)
Copy code
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
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
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
• 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:
Copy code
... 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
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
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
hmm.. ok it does seems to hold together
👍 1
7 Views
Open in Slack
PreviousNext
Powered by