Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alternate "syntax" for routing. #10

Open
sheepdreamofandroids opened this issue Feb 27, 2020 · 13 comments
Open

Alternate "syntax" for routing. #10

sheepdreamofandroids opened this issue Feb 27, 2020 · 13 comments
Labels
enhancement New feature or request

Comments

@sheepdreamofandroids
Copy link

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
Copy link
Collaborator

Wicpar commented Feb 27, 2020

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.

@Wicpar Wicpar added the enhancement New feature or request label Mar 2, 2020
@sheepdreamofandroids
Copy link
Author

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
Copy link
Collaborator

Wicpar commented Mar 2, 2020

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
Copy link
Author

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
Copy link
Collaborator

Wicpar commented Mar 4, 2020

I'll see what you come up with.

@Wicpar
Copy link
Collaborator

Wicpar commented Mar 10, 2020

@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:

  • Default object handler, generating a response from the return type
  • Per endpoint generated param: id
  • Reusable paged parameters, declared like:
@RequestParameters
data class PagedParameters(@QueryParam("Page number") val page: Int? = 0, @QueryParam("Items per page") val items: Int? = -1)

(please note that default parameters are not yet supported, and if they work it is by pure coincidence)

post ("resource") { body: @RequestBody SomeObject ->
    ResourceService.createSomeObject(body)
}

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
Copy link
Collaborator

Wicpar commented Mar 11, 2020

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
Copy link
Author

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
Copy link
Collaborator

Wicpar commented Mar 11, 2020

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
Copy link
Author

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
Copy link
Contributor

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
Copy link

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
Copy link
Collaborator

Wicpar commented Sep 25, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants