ktorio / ktor

Framework for quickly creating connected applications in Kotlin with minimal effort
https://ktor.io
Apache License 2.0
13.06k stars 1.07k forks source link

[Evolution] Ktor locations nested classes #1660

Open cy6erGn0m opened 4 years ago

cy6erGn0m commented 4 years ago

Subsystem ktor-locations

Description We are going to develop and expand ktor Locations (Type-Safe routing https://ktor.io/servers/features/locations.html). One of the directions to improve is Kotlin Multiplatform. The most obvious way of migration to MPP is to use kotlinx.serialization in locations. Due to the limited reflection capabilities of non-JVM targets, there are things that not so easy to implement. On the other hand, there are questionable features that may lead to issues. One of the problematic features is nested location classes and nested location objects.

What we are thinking of to change:

The motivation for the first point is the fact that a location class nested to another, makes no sense without the ability to refer to the outer class. Consider the following example:

@Location("/api/{version}")
class Api(val version: Int) {
    @Location("/user/{id}")
    class User(val id: String, val api: Api = Api(1)) {
        // here API presence looks required, 
        // otherwise, there is no way to find a version value
        // so now we demand the api parameter to exist
    }
}

The other point is that one can't just move a nested class outside without additional manual changes:

@Location("/root")
class Outer {
    @Location("/child")
    class Child  // it is tied to /root/child path
}

@Location("/child")
class Child  // this is tied to just /child

@Location("/child2")
class Child2(val outer: Outer) // -> /root/child2

The cause of the second change is that objects should be symmetric with classes, and one can't add a constructor property to an object. The unfortunate consequence is that one can't write like this anymore:

@Location("/api")
object Api {
    @Location("/info")
    object Info 
}

Should be migrated to the following:

@Location("/api")
object Api {
    @Location("/info")
    class Info(val api: Api = Api)
    // the api parameter is required, weird isn't it
}
spand commented 4 years ago

An asymmetric case I stumbled upon with the current design is that the current builders ie. get<Location>(){ .. } also work nested: path("/fooo/") { get<Location> {} }. This context isnt preserved so one cannot always assume href(location) will give the routeable path that lead to the location. I am not sure what to do about it but it bit me.

One solution is obviously to disallow nested location routing.

spand commented 4 years ago

What is the technical reason this cannot be special cased:

@Location("/api")
object Api {
    @Location("/info")
    object Info 
}
cy6erGn0m commented 4 years ago

@spand the technical part of the problem with the sample is that we can't collect the required information (location annotations and arguments and class hierarchy information) without reflections by using only kotlinx.serialization facilities. We could improve kotlinx.serialization, but introducing a class structure info just for a very special ktor case doesn't look good from the serialization pov. We see no other use-cases except ktor.

philipbel commented 4 years ago

Hello.

I am having an issue with Ktor locations—nesting them inside route() and using ApplicationCall.url() to obtain their URLs. I am not sure whether my issue is related, but I thought I'd share here, so that can be taken into account (assuming my usage is correct and according to the design).

I have generated a minimal project with the following routes:

routing {
        trace { application.log.trace(it.buildText()) }

        // Register nested routes
        get<Type.Edit> {
            val myUrl = call.url(it)
            call.respondText("Inside $it: $myUrl\n")
        }
        get<Type.List> {
            val myUrl = call.url(it)
            call.respondText("Inside $it: $myUrl\n")
        }

        route("/rootroute1/rootchild1") {
            get<RootLoc1.ChildLoc1> { location ->
                val myUrl = call.url(location)
                call.respondText("ChildLoc1: $myUrl\n")
            }
        }
    }
% curl http://localhost:8080/rootroute1/rootchild1/rootloc1/childloc1
ChildLoc1: http://localhost:8080/rootloc1/childloc1

I would expect to see http://localhost:8080/rootroute1/rootchild1/rootloc1/childloc1 from ApplicationCall.url(). The path given, /rootroute1/rootchild1 to route(), is not picked up.

Without using route(), everything works as expected:

% curl http://localhost:8080/type/foo/edit
Inside Edit(type=Type(name=foo)): http://localhost:8080/type/foo/edit
oleg-larshin commented 4 years ago

Please check the following ticket on YouTrack for follow-ups to this issue. GitHub issues will be closed in the coming weeks.