Ktor server routes and controller interfaces

[ulrikandersen]

Design choices

Inspired by #214, Ktor server code is generated as a controller interface with methods for the routes along with a companion object which is used for mounting the routes on the Ktor server (see example further down).

Ktor is by design very flexible, and thus in order for us to be able to generate code we need to make certain design decisions. The approach with routing function + an interface should work well for most cases. The users are still free to configure Ktor however they like, including where they mount the routes and how the implement the controller interface.

Ktor's ApplicationCall is passed to the controller methods to allow them to perform additional logic on the incoming/outgoing request. One might desire to create an extension function for resolving the current user or respond directly with a redirect which requires having access to call.

Controller methods responsible for responding to the request (call.respond()). If no respond is sent, Ktor will respond with 404 per default.

Example

public interface InternalEventsController {

    public suspend fun post(bulkEntityDetails: BulkEntityDetails, call: ApplicationCall)

    public companion object {
        public fun Route.internalEventsRoutes(controller: InternalEventsController) {
            authenticate("oauth2") {
                post("/internal/events") {
                    val bulkEntityDetails = call.receive<BulkEntityDetails>()
                    controller.post(bulkEntityDetails, call)
                }
            }
        }
    }
}

Usage example

fun main() {
    embeddedServer(Netty, port = 8080) {
        routing {
            internalEventsRoutes(object : InternalEventsController {
                override suspend fun post(
                    bulkEntityDetails: BulkEntityDetails,
                    call: ApplicationCall
                ) {
                    call.respond(HttpStatusCode.OK, EventResults(emptyList()))
                }
            })
        }
    }.start(wait = true)
}

Here the controller interface is implemented as an object, but one could also implement is as a regular class which would make things like dependency injection and modularisation easier.

Changes

1. Add Ktor route & controller interface generation.
2. Split controller interface generator abstract class in two; one for annotation based libraries (Spring, Micronaut) and one common for all.

Known limitations

1. Headers are always handled as strings. Should be OK for now, but an improvement would be to also handle those in a typed way (including conversion from string).

[atollk]

It seems that schema information is not used here, unlike it is for the Spring controllers. Wouldn't it make sense to add checks here as well?

[ulrikandersen]

I assume you mean the validation constraints added as annotations to Spring and Micronaut controllers.

Unfortunately Ktor does not have such thing built in, and we would have to either rely on an external library or implement the checks ourselves.

In the past I have used the RequestValidation plugin combined Hibernate Validator with to validate the incoming body, perhaps something similar could be done for query, path and header parameters, but it would have to be custom coded.

[atollk]

Yeah, that's what I meant. At the moment, the supported validations are numeric min/max, nullability, regex patterns and sequence length, all of which I think could be implemented in a few lines. But as @cjbooms said themselves below, not everything needs to be perfect from the start, so if you feel like this addition would be too much for this PR, I also won't mind if it gets merged without this feature. We should probably just create another issue and a note in the documentation in that case.

[ulrikandersen]

Yes, let's add that as an issue 👍 It would be a good improvement.

[atollk]

I think this is too limited of a response struct, considering what Ktor offers in general: https://ktor.io/docs/server-responses.html

[ulrikandersen]

Yeah I agree it is too limited to cover all use cases, and this is why I have chosen to pass ApplicationCall to the controllers which allows them to both read from the request, but also respond in the various ways Ktor supports.

Unfortunately this leads to an ambiguous API where the controller method is expected to return a ControllerResult, but is also free to "close" the call with e.g. call.respondRedirect.

In practise this leads to the following implementation:

call.respondRedirect("https://some.other.domain/path")
return ControllerResult(HttpStatusCode.OK, Unit)

Ktor will respond with the first respond and the ControllerResult will be ignored.

In the initial version I had simply skipped the ControllerResult in cases where the spec does not mention a return model. This would avoid the awkward code mentioned above.

Possible alternatives would be to

1. Not force a return type and let the controller call call.respondX. Here we loose the type safety of the return type, which is not desirable.
2. Provide a return struct that supports all the same respond as Ktor. This would have to be updated whenever Ktor changes.
3. Other alternatives?

[atollk]

That is a dilemma :)

What about creating two functions for each endpoint; a "simple" and a "powerful" one.

interface MyController {
    // Only returns 200 with the returned body; or 500 on an exception.
    public suspend fun post(
        querySomeObject: String,
        bodySomeObject: SomeObject,
    ): Unit
    
    // Can use arbitrary returns.
    public suspend fun post(
        call: ApplicationCall,
        querySomeObject: String,
        bodySomeObject: SomeObject,
    ) {
        val responseBody = post(querySomeObject, bodySomeObject)
        call.respond(responseBody)
        // ...
    }
    
    // Routing ...
}

The only issue I see is if users override both functions for one endpoint, but I feel like that would be a rare mistake and not that difficult to catch.

Of course, it is arguable whether the first method actually serves any purpose or if 99% of use cases require the second one anyway.

[ulrikandersen]

Having looked more into this, I would like to go with simply not requiring the implementation to return ControllerResult in cases where the route has no return content.

In that case Ktor will respond with 200 OK by default, but we still allow the implementation to respond with a e.g. a redirect if needed.

This change has been added in 35c3195.

[ulrikandersen]

Turns out I was wrong about the default: if no response is provided Ktor will respond with 404 Not Found 😬

@atollk Would you be OK with making routes without response content return HttpStatusCode? If the implementation choses to perform an early call.respond the code is a little ambiguous, but that is already the case with the endpoints that return content. Here the implementation would still be required to return a ControllerResult even though it performs an early respond.

interface Controller {
  suspend fun testPath(call: ApplicationCall): HttpStatusCode
  
  companion object {
    get("/test") {
      val resultStatus = controller.testPath(call)
      call.respond(resultStatus)
    }
  }
}

// respond with 200 OK

override suspend fun testPath(call: ApplicationCall): HttpStatusCode {
  return HttpStatusCode.OK
}

// or redirect

override suspend fun testPath(call: ApplicationCall): HttpStatusCode {
  call.respondRedirect("/someOtherPath")
  return HttpStatusCode.InternalServerError // <-- should never be reached
}

[ulrikandersen]

I agree that it is not nice to force a return that will never be reached.

I like the design where we provide a simple and an advanced option, and I think it could cover all use cases. Any time the user wishes to deal directly with call (either to read additional information from the request, or to provide a custom response) he would implement the advanced function.

I am thinking it could look like this, where the default implementation of the advanced function can be overriden by the user:

public interface InternalEventsController {

  // simple version
  public suspend fun post(bulkEntityDetails: BulkEntityDetails): ControllerResult<EventResults>

  // advanced version; user is responsible for returning a response
  public suspend fun post(bulkEntityDetails: BulkEntityDetails, call: ApplicationCall) {
    // default implementation delegates to the simple version
    val result = post(bulkEntityDetails)
    call.respond(result.status, result.message)
  }

  public companion object {
    public fun Route.internalEventsRoutes(controller: InternalEventsController) {
      post("/internal/events") {
        val bulkEntityDetails = call.receive<BulkEntityDetails>()
        controller.post(bulkEntityDetails, call)
      }
    }
}

In cases where no model is returned by the endpoint we could simply return HttpStatusCode from the simple function:

public interface InternalEventsController {

  public suspend fun post(bulkEntityDetails: BulkEntityDetails): HttpStatusCode // <---

  public suspend fun post(bulkEntityDetails: BulkEntityDetails, call: ApplicationCall) {
    val status = post(bulkEntityDetails)
    call.respond(status)
  }

  public companion object {
    public fun Route.internalEventsRoutes(controller: InternalEventsController) {
      post("/internal/events") {
        val bulkEntityDetails = call.receive<BulkEntityDetails>()
        controller.post(bulkEntityDetails, call)
      }
    }
}

Does that resonate with you?

[ulrikandersen]

I just realised that the above will not cut it: The user will be forced to always implement the simple version as it is abstract 🤔

We could however provide a default implementation which throws in case it has not been overridden. This will happen in cases where neither the advanced nor the simple version has been implemented.

Downside is that the user will not get warned at compile time if they have forgotten to implement one of the routes.

public suspend fun post(bulkEntityDetails: BulkEntityDetails): ControllerResult<EventResults> {
  throw NotImplementedError("Not implemented")
}

public suspend fun post(bulkEntityDetails: BulkEntityDetails, call: ApplicationCall) {
  // default implementation delegates to the simple version
  val result = post(bulkEntityDetails)
  call.respond(result.status, result.message)
}

We could also keep it simple and always rely on the implementation to respond (the "advanced" version). We would loose the typecheck on the return, but provide maximum flexibility.

I am all ears if you have alternative solutions i mind.

[atollk]

I just realised that the above will not cut it: The user will be forced to always implement the simple version as it is abstract 🤔

True, I didn't think about that either.

The NotImplementedError is not great, as you said, because it's lacking compile checks but that would also have been my idea at the moment. Maybe also respond a 501 status code to the call.

I'm still not to keen on the ControllerResult custom class. Maybe we should create a feature request on the Ktor tracker to see if Jetbrains would be willing to add a Response-like type that we could use. Fabrikt isn't the first time I wish something like that existed :)

But for the time being, if you want to merge the MR, it might be the right choice to just offer the "advanced" version for now and potentially add convenience overloads in the future? We could add a comment about the excepted return type to make it slightly less annoying to the user.

[cjbooms]

Would having an extension property help resolve this in any way? Eg, allow the user to specify the return type desired for each operation

x-ktor-response-type: blah

I'm not fully following this PR, so feel free to disregard this comment if not helpful

[ulrikandersen]

Let us stick with offering a function which delegates responding to the implementation (aka the advanced version) 👍

I have added extra documentation to the generated methods which informs the user about the responsibility to respond to the call.

public interface InternalEventsController {
    /**
     * Generate change events for a list of entities
     *
     * Route is expected to respond with [examples.githubApi.models.EventResults].
     * Use [io.ktor.server.response.respond] to send the response.
     *
     * @param bulkEntityDetails
     * @param call The Ktor application call
     */
    public suspend fun post(bulkEntityDetails: BulkEntityDetails, call: ApplicationCall)

    public companion object {
        /**
         * Mounts all routes for the InternalEvents resource
         *
         * - POST /internal/events Generate change events for a list of entities
         */
        public fun Route.internalEventsRoutes(controller: InternalEventsController) {
            post("/internal/events") {
                val bulkEntityDetails = call.receive<BulkEntityDetails>()
                controller.post(bulkEntityDetails, call)
            }
        }
        // ...
    }
}

With this change the ControllerResult class as well as the generateLibrary function on the controller generator has been made obsolete, which is why I have removed them again.

Following this decision I believe the PR is ready to be merged, @cjbooms 🚀

[atollk]

I mostly looked at the examples in the test cases but from that it looks close to what I imaged 👍

[cjbooms]

Let me know when this is ready for review. I should be able to cut a release for you today if you like.
I won't have any input on the generated code, as I have never used Ktor, so will leave that up to you two to decide on. It does not have to be perfect first time around, we can cut a few different releases until you are satisfied it handles your local cases

[cjbooms]

LGTM - Let me know if you are happy to merge

[atollk]

lgtm @cjbooms

[cjbooms]

Great news.
@ulrikandersen is the PR description still accurate if I use it as the squashed commit message?

[ulrikandersen]

Great news. @ulrikandersen is the PR description still accurate if I use it as the squashed commit message?

Yes, I would say so! I updated it following the decision on how to handle returns.

[cjbooms]

Cutting you a release now