papsign / Ktor-OpenAPI-Generator

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

Issue with testing post route with body #55

Closed plannigan closed 4 years ago

plannigan commented 4 years ago

I wanted to create a PR that would allow for not using Unit when there are no route params (should be ready soon). However, when I was writing some unit test cases for the new code, I ran into a weird issue with the post route not being handled when the body was specified. What makes this weird is that running the same code works fine when standing up the server and sending real post requests.

This commit has a minimal server that shows the post working (ktor only, post route with Any for body, post route with actually specifying body type) and a unit test cases that should represent those same three cases, but testPost_bodyType_expectedBodyAndResponse fails.

Can you take a look? It is possible that I am missing something. I don't know enough about the ktor internals to track down where the request is being rejected. And it is odd that only the test application fails.

Wicpar commented 4 years ago

I'll look at it later today if i have time. If you want to display the trace you can add the trace as described at the bottom of the ktor route documentation. https://ktor.io/servers/features/routing.html

Also be careful with authenticated routes, the definitions could clash, this was the reason i originally didn't create these variants...

Wicpar commented 4 years ago

Oh right, something similar happened to someone else also, make sure that the content type is specified in the tests, the routes are defined on specific content types to allow handling of multiple content types on the same route.

plannigan commented 4 years ago

Setting the content type resolved the issue and now all the test cases pass. So I'll close this issue.

I'll take a look at the auth routes. If the generics does cause a problem, would you be open to something like auth{METHOD}()? It's subjective, but I feel like

route("/some/route") {
    post<SomeResponse, SomeBody> {
            body ->
        println(body)
        respond(SomeResponse("hello"))
    }
    authPost<SomeResponse, SomeBody, SomeAuth> {
            body ->
        println(body)
        respond(SomeResponse("hello"))
    }
}

route("/some/route/{foo}") {
    post<SomeParam, SomeResponse, SomeBody> {
            param, body ->
        println(param)
        println(body)
        respond(SomeResponse("hello"))
    }
    authPost<SomeParam, SomeResponse, SomeBody, SomeAuth> {
            param, body ->
        println(param)
        println(body)
        respond(SomeResponse("hello"))
    }
}

looks better than

route("/some/route") {
    post<Unit, SomeResponse, SomeBody> {
            body ->
        println(body)
        respond(SomeResponse("hello"))
    }
    post<Unit, SomeResponse, SomeBody, SomeAuth> {
            body ->
        println(body)
        respond(SomeResponse("hello"))
    }
}

route("/some/route/{foo}") {
    post<SomeParam, SomeResponse, SomeBody> {
            param, body ->
        println(param)
        println(body)
        respond(SomeResponse("hello"))
    }
    post<SomeParam, SomeResponse, SomeBody, SomeAuth> {
            param, body ->
        println(param)
        println(body)
        respond(SomeResponse("hello"))
    }
}
Wicpar commented 4 years ago

The plan is to eventually transition to an injection based route configuration #10 It will still be computed during init for no runtime impact and allow for quite a bit more flexibility and a cleaner syntax.