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

Getting 405 Method Not Allowed from combined routes #492

Open
hoshikon opened this issue Jan 25, 2021 · 3 comments
Open

Getting 405 Method Not Allowed from combined routes #492

hoshikon opened this issue Jan 25, 2021 · 3 comments
Labels

Comments

@hoshikon
Copy link

When I combine these routes and hit the second route, it returns 405 Method Not Allowed. I don't know if this is intended or a bug but I would prefer to have it aligned with http4s library.

    val routes1 = new RhoRoutes[IO] {
      GET / "path" |>> { () => Ok("good") }
    }.toRoutes()

    val routes2 = new RhoRoutes[IO] {
      POST / "path" |>> { () => Ok("good") }
    }.toRoutes()

    val combinedRoutes = routes1 <+> routes2

    val req = Request[IO](Method.POST, Uri(path = "/path"))
    val response = combinedRoutes.orNotFound.run(req).unsafeRunSync()
    response.status shouldBe Status.Ok   // fails because it returns MethodNotFound instead
@zarthross zarthross added the bug label Feb 4, 2021
@zarthross
Copy link
Member

zarthross commented Feb 21, 2021

This is a bit of a tricky one. We have 2 choice based on how the frameworks work.

  1. Let routes1 return None if the route matches but the method isn't found.
    • Pros:
      • The above example works and gives a 200 OK
      • This is the same as Http4s DSL
    • Cons: If there really are no other methods for that route, it would give a 404 Not Found instead of 405 Method Not Allowed
  2. Let routes return a Some(MethodNotAllowed) if the route matches but the method isn't found
    • Pros:
      • You can actually get a 405 Method Not Allowed
      • Follows the web standards I should think...
    • Cons:
      • The above isn't clear to users of row why routes2 is ignored.
      • It's different than Http4s DSL

After some thought, I'm inclined to leave the functionally as is. Rho was not designed for you to convert 2 sets of RhoRoutes to Http4s routes and then compose them using Http4s DSL <+> you really should compose all RhoRoutes together FIRST, and then convert them to Http4s.

Unless you have some edge case where you intend to create separate swagger documents within a single application, there shouldn't be any issues here.

@zarthross
Copy link
Member

I'd be curious if @rossabaker has any thoughts on this?

@hughlunt
Copy link

I've hit this same issue.
The scenario is similar to the above, except that the POST request requires auth. e.g.

object Auth extends org.http4s.rho.AuthedContext[IO, AuthInfo]

val routes1 = new RhoRoutes[IO] {
  GET / "path" |>> { () => Ok("good") }
}

val routes2 = new RhoRoutes[IO] {
  POST / "path" >>> Auth.auth() |>> { (_: AuthInfo) => Ok("good") }
}

val combinedRoutes = routes1.toRoutes() <+> routes2.toRoutes()

In order to auth, routes2 needs to be wrapped in authMiddleware. However, if I compose the RhoRoutes first, then routes1 will also be wrapped in middleware and then return a 401 when no header is supplied.

Whilst it's probably not technically correct, any thoughts on including a param to enable copying the http4s behaviour if desired?

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

No branches or pull requests

3 participants