papsign / Ktor-OpenAPI-Generator

Ktor OpenAPI/Swagger 3 Generator
Apache License 2.0
240 stars 42 forks source link

Alternate "syntax" for routing. #10

Open sheepdreamofandroids opened 4 years ago

sheepdreamofandroids commented 4 years ago

Just a wild idea. How about allowing routing like get("path", ::someMethod) and the parameters of that method are annotated the same way the fields of parameter classes are now annotated.

That way you don't have to declare a class for just one or two simple parameters and you get a very natural looking routing style.

Also this would be quite familiar to users of springfox.

Wicpar commented 4 years ago

I am open to any useful contributions. This seems to have its uses, and it would allow for more flexible parameter definitions. It may even be possible to directly use reflection on a noinline lambda, that would make the syntax basically identical to what we have now except that you can define the types in the lambda parameters. If that is the case i might even favor this system.

sheepdreamofandroids commented 4 years ago

I want to look into this. A quick look revealed that it will probably touch some of the same files as you are working on now for parameters. I'll see how far I can get without getting in too much of a merge conflict. Or maybe I'll just work off of your branch and keep up to date. Are you planning on merging that one soon?

Wicpar commented 4 years ago

You should be able to do this without interfering with the current work. Look at the handle and preHandle functions that do the conversion logic for the current functions. https://github.com/papsign/Ktor-OpenAPI-Generator/blob/master/src/main/kotlin/com/papsign/ktor/openapigen/route/OpenAPIRoute.kt You will probably need to create a function that takes a KFunction<Unit>, and then use reflection to get the invoke method, and use the KParameters to get the KType and the relevant annotations.

sheepdreamofandroids commented 4 years ago

You will probably need to create a function that takes a KFunction, and then use reflection to get the invoke method, and use the KParameters to get the KType and the relevant annotations. All that is already there. The only difference between the current syntax and the proposed one is that currently a KFunction (the primary constructor) is called to create a model and then a lambda is called to process it while in the new one, the first KFunction also does the processing. So I think this can be a quite superficial change where simply the execution of the last lambda is skipped and the difference between calling a lambda vs a constructor is abstracted away asap.

Wicpar commented 4 years ago

I'll see what you come up with.

Wicpar commented 4 years ago

@sheepdreamofandroids so i tinkered around and came up with this syntax:

import org.junit.Test
import kotlin.reflect.KFunction
import kotlin.reflect.jvm.reflect

class ReflectionSandbox {

    @Target(AnnotationTarget.TYPE)
    @Retention(AnnotationRetention.RUNTIME)
    annotation class TestParam

    fun <R> handleReflect(kfun: KFunction<R>) {
        println(kfun)
        println(kfun.parameters.associateWith { it.type.annotations }.mapKeys { it.key.type })
        println(kfun.returnType)
    }

    inline fun <reified R> hanldleWithReflect(noinline test: () -> R) {
        handleReflect(test.reflect()!!)
    }

    inline fun <reified R, reified A> hanldleWithReflect(noinline test: (a: A) -> R) {
        handleReflect(test.reflect()!!)
    }

    inline fun <reified R, reified A, reified B> hanldleWithReflect(noinline test: (a: A, b: B) -> R) {
        handleReflect(test.reflect()!!)
    }

    inline fun <reified R, reified A, reified B, reified C> hanldleWithReflect(noinline test: (a: A, b: B, c: C) -> R) {
        handleReflect(test.reflect()!!)
    }

    inline fun <reified R, reified A, reified B, reified C, reified D> hanldleWithReflect(noinline test: (a: A, b: B, c: C, d: D) -> R) {
        handleReflect(test.reflect()!!)
    }

    // etc...

    data class BodyResponse<T>(val payload: T)

    @Test
    fun testNoInlineParameterReflection() {
        hanldleWithReflect { a: @TestParam String ->
            BodyResponse(a)
        }
    }
}

So it is possible to annotate properties directly in the lambda. No vararg type parameters yet sadly

Theoretical syntax could be:

get ("{id}/pagedResources") { id: @PathParam("ID of resources") UUID, paged: PagedParameters ->
    ResourceService.getPaged(id, paged)
}

this would use:

Is this to your liking ?

To me it even seems preferable to the current system as it is a lot clearer, flexible, and reusable

Wicpar commented 4 years ago

I have pondered further on the question. I think it would be beneficial to completely change the system to this. It would drastically change the structure of the library and won't be backwards compatible. The benefits would be huge, as an injection based system will allow for greater modularity and flexibility as compared to the current system.

I estimate the workload to be about two office weeks, or as i like to call it: 4 caffeine fuelled all-nighters.

I will also try to reduce the usage of the reflections library. it is slow as hell.

sheepdreamofandroids commented 4 years ago

I love it. Much easier on the eye than the current way that forces you to create a data class for each parameter list.

I think you would even be able to directly use a method reference. Like

get ("{id}/pagedResources", ::pagedResources) // id is path parameter
get ("/pagedResources", ::pagedResources) // id is query parameter
post ("/pagedResources^paged", ::pagedResources) // id is query parameter, paged is body

fun pagedResources { id: @Param("ID of resources") UUID, paged: PagedParameters ->
    ResourceService.getPaged(id, paged)
}

This way all the info about the syntax of the endpoint is in the routing and all the reusable meaning in the method.

Wicpar commented 4 years ago

alright, let's do this. Just one note, i will keep @PathParam, @QueryParam, @HeaderParam, @CookieParam separate as it is not possible (i.e. a lot more work, confusion and less reliability) to build the correct OpenAPI descriptor without this distinction. It will be possible to add such a handler nevertheless.

sheepdreamofandroids commented 4 years ago

Yeah, I see how putting that info in the path string is suboptimal. Yet I think that the idea of keeping the location of parameters in the route and type, documentation and default values in the method is sound. I'll try to come up with a better way of expressing that info.

davidhiendl commented 4 years ago

It would also be interesting to have a way to pass in the KClass instances for typed Parameters, Response and Body into a generic .handle(...) method instead of relying on reified types entirely. This would allow for more customization when generating the routes from legacy applications. As it stands right now I cannot do this and have to manually register the routes with each controller. I have a project with hundreds of endpoints which are all methods like this, which are register with reflection and have all sorts of automatic functionally:

@AutoDiscover
class ManageUsersController(directDI: DirectDI) : AnnotationController(directDI) {

    override val path = "$PATH_UI/auth/manage/users"
    override val permissionPath = "auth/manage/users"

    @Route(HttpMethod.Post, "list")
    suspend fun routeList(
        uiCtx: UiRequestContext,
        params: ListParams
    ): ListResponse? {
        // ...
    }
    // ...
}

A method like this would allow for flexible implementation of "custom" route detection with reflection:

@ContextDsl
inline fun <P : Any, R : Any, B : Any> NormalOpenAPIRoute.handle(
    classParams: KClass<P>,
    classResponse: KClass<R>,
    classBody: KClass<B>
    exampleResponse: R? = null,
    exampleRequest: B? = null,
    crossinline body: suspend OpenAPIPipelineResponseContext<R>.(P, B) -> Unit
) {
    preHandle<P, R, B, NormalOpenAPIRoute>(classParams, classResponse, classBody, exampleResponse, exampleRequest) {
        handle(body)
    }
}
leeaaron629 commented 3 years ago

Hi everyone, I love the suggested syntax, but it doesn't look like it's implemented. Wondering what's the status and if I can do anything to help out

Wicpar commented 3 years ago

I haven't gotten around to start beyond the initial proof of feasibility. I am currently on other projects. If you think you can get something clean on the rails feel free to contribute.