arrow icon indicating copy to clipboard operation
arrow copied to clipboard

Alternative Ktor Raise DSL

Open tKe opened this issue 1 year ago • 8 comments

Not complete by any stretch but hopefully enough to start any discussion.

tKe avatar Feb 10 '25 23:02 tKe

@tKe please don't take the merge of #3576 as making the new Ktor module the "correct" one, the door is still open to the changes you propose.

Would you mind elaborating how this PR is different from the other one about Ktor + Raise, and the pros/cons of this implementation?

serras avatar Feb 12 '25 12:02 serras

At the moment, this PR is only addressing one "con" of the existing API, which is the reliance on OutgoingContent (or HttpStatusCode) as the only "raisable" responses.

Raising OutgoingContent

The downside of only allowing for OutgoingContent within Ktor is that it shortcircuits any ContentNegotiation or response transformer plugins.

An example where this is an issue would be if a service was intended to have a typed/structured error response, and desired to use the standard approach of using call.respond(HttpStatus.BadRequest, myErrorPayload) when an error response is desired. This wouldn't be possible via raise(OutgoingContent) without separately encoding the error payload into a TextContent or other OutgoingContent type.

@Serializable
data class ErrorPayload(val code: String, val detail: String)

private fun Raise<OutgoingContent>.handleError(domainError: DomainError): Nothing {
  val errorPayload = when(domainError) {
    is UserBanned -> ErrorPayload(
      code = "USER_BANNED", 
      detail = "user ${domainError.userId} was banned because ${domainError.reason}"
    )
    else -> TODO()
  }
  val outgoingContent = // convert via JSON to byte array?
  raise(outgoingContent)
}

fun Application.module(userService: UserService) {
  install(ContentNegotiation) { json() }
  routing {
    getOrRaise("/users/{userId}") {
      val userId = // get from path or raise
      
      val user = withError(::handleError) { 
        userService.lookup(userId) ?: raise(HttpStatusCode.NotFound)
      }

      call.respond(user)
    }
  }
}

The desirable API would allow for raising the errorPayload, potentially with a specific status code, and let the ktor plugins handle conversion to outgoing content as you would have if you had used call.respond, while still allowing for the short-circuiting of raise.

private fun Raise<RoutingResponse>.handleError(domainError: DomainError): Nothing {
  val errorPayload = when(domainError) {
    is UserBanned -> raise(HttpStatusCode.Conflict, ErrorPayload(
      code = "USER_BANNED", 
      detail = "user ${domainError.userId} was banned because ${domainError.reason}"
    ))
    else -> TODO()
  }
  val outgoingContent = // convert via JSON to byte array? how to determine status code?
  raise(outgoingContent)
}

I've also attempted to avoid context receivers for implementing this, and have instead defined extensions for a Raise<RoutingResponse> for handling HttpStatusCode and OutgoingContent responses - that said, this is just syntactic sugar and doesn't actually expose a Raise<HttpStatusCode> or Raise<OutgoingContent> - union types would be lovely here...

Composability with ktor APIs

This PR also attempts to implement error/raise response handling in a handleOrRaise as an analogue of ktor's handle generic route handler, rather than only specific ktor get or post style route handlers (which it admittedly could also do for ease-of-use).

handleOrRaise allows for "breaking out" of a handler with a response (either empty with HttpStatusCode, a pre-converted OutgoingContent, or a reified payload that would flow through the ktor plugins, e.g ContentNegotiation).

Two paths to a response

The handleOrRaise still relies on explicitly calling call.response for the "happy" response, as would be expected in a standard ktor route handler, whereas the raise response is automatically invoking call.respond for you.

To approach this, I've attempted to provide a mechanism in which the "happy-path" call.response is also achieved automatically with the successful result of the lambda/body - this would ensure that call.response is always called at least once, either through the raise path or the "happy" path.

It might be desirable for these respondOrRaise-style to discourage direct use of call.respond through compile-time warnings, although this is might prove difficult for the extension methods such as respondText.

A start on this is this is attempted in respondOrRaise.kt with an example (and alternative) raisingGet implementation.

There is also a RoutingContext.respondOrRaise that can be composed inside existing ktor get(...) { ... } or post(...) { ... } style handlers, e.g.

routing {
  get("/users/{userId}") {
    respondOrRaise {
      val userId = pathParameters["userId"]
      withError({ raise(convertToError(it)) }) {
        userService.lookupUser(userId) ?: raise(HttpStatusCode.NotFound)
      }
    }
  }
}

validate et al

I've not (yet) considered the validate DSL and/or pathOrRaise/queryOrRaise components of the original API, but they would likely warrant aligning to a similar approach. The only thing this PR does with those is remove the duplicate of the RaiseOrAccumulate that is now part of arrow core.

existing getOrRaise/putOrRaise extensions

I've currently left the existing getOrRaise/putOrRaise extensions in routing.kt for now.

I would intend to replace them with extensions that either:

  • require explicit happy-path call.response like the non-raising ktor API (equivalent to route(path, HttpMethod.Get) { handleOrRaise { body() } })
  • implicitly call call.respond with the happy-path result (equivalent to route(path, HttpMethod.Get) { respondOrRaise { body() } })

note: raisingGet is an example of the latter but with an alternative name to avoid clashing while this PR is elaborated

tKe avatar Feb 12 '25 14:02 tKe

I've now replaced the existing verbOrRaise helpers with ones that will implicitly respond with the result of the body lambda.

You could now achieve the above example with something like:

routing {
  getOrRaise("/users/{userId?}") {
    val userId = call.pathParameters["userId"] ?: raiseBadRequest("userId not specified")
    withError(::handleError) {
      userService.lookupUser(userId) ?: raise(HttpStatusCode.NotFound)
    }
  }
}

tKe avatar Feb 13 '25 19:02 tKe

I've included a delegate based access for query/path parameters allowing for:

val id: Int by pathRaising
val userId: Int by queryRaising("user-id")"
val nameUpper: String by pathRaising { it.uppercase() }

validate { ... } block using RaiseAccumulate has by pathAccumulating and by queryAccumulating forms and a receiveAccumulating<T>() helper.

This makes for a nice and succinct:

putOrRaise("/user/{name}") {
  val person = validate {
    val name: String by pathAccumulating
    val age: Int by queryAccumulating
    val info: Info by receiveAccumulating()
    Person(name, age, info)
  }
  Created(person)
}

There's also a Ktor Route-scoped plugin to provide the default error responses:

install(RaiseErrorResponse) {
  errorResponse = { validationError(it.nel()) }
  errorsResponse = { validationError(it) }
}

tKe avatar Feb 28 '25 21:02 tKe

@tKe is the implementation now ready to merge? it would be nice to have this in a 2.2-beta release (alongside the context parameter Raise)

serras avatar May 21 '25 09:05 serras

I've started playing with a context-parameters based alternative at a suggestion from @nomisRev : https://github.com/tKe/arrow/pull/2

As part of that I've reduced the API surface a little to remove some duplication between RoutingContext and RoutingCall - effectively requiring call.pathOrRaise rather than just pathOrRaise as it's more consistent with the Ktor API and avoids a lot of duplication. I was planning on applying that same simplification to this branch to avoid a larger initial api surface.

The downside of the context-parameters approach is my desire to provide a scope with both Raise<Response> and Raise<RequestError> and the contextual ambiguity that ensues.

tKe avatar May 21 '25 11:05 tKe

The downside of the context-parameters approach is my desire to provide a scope with both Raise<Response> and Raise<RequestError> and the contextual ambiguity that ensues.

I'd need to double check, but I don't think that ambiguity will arise very often. If you use raise(Response(...)), the compiler knows that it has to use the Raise<Response> context, and similarly for RequestError.

serras avatar May 21 '25 20:05 serras

I'd need to double check, but I don't think that ambiguity will arise very often. If you use raise(Response(...)), the compiler knows that it has to use the Raise<Response> context, and similarly for RequestError.

It's likely fine for raise(E) but it's ambiguous for the bridge functions being added in #3606 which is the case I was referring to on slack.

https://github.com/tKe/arrow/blob/1a891a3f89bdfbddeb9e1b88331337288f145165/arrow-libs/integrations/arrow-raise-ktor-server/src/jvmTest/kotlin/arrow/raise/ktor/server/RaiseRespondTest.kt#L124

tKe avatar May 21 '25 21:05 tKe

Bringing this one up again, is this ready to merge?

serras avatar Jun 24 '25 06:06 serras

Closing this for now, a total different experiment is taking place to bring most of this features to all Ktor users. This would only leave some utilities for Raise <~> Outgoing context, for short-circuiting with responses. An investigation will take place subsequent to the other work.

This was discussed with everyone offline. Thank you for your efforts @tKe 🙏

nomisRev avatar Jul 16 '25 17:07 nomisRev