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

Issue with testing post route with body #55

Closed
plannigan opened this issue May 22, 2020 · 4 comments
Closed

Issue with testing post route with body #55

plannigan opened this issue May 22, 2020 · 4 comments

Comments

@plannigan
Copy link

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

Wicpar commented May 22, 2020

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

Wicpar commented May 22, 2020

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

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

Wicpar commented May 22, 2020

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.

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

No branches or pull requests

2 participants