twitter / finatra

Fast, testable, Scala services built on TwitterServer and Finagle
https://twitter.github.io/finatra/
Apache License 2.0
2.27k stars 406 forks source link

Overloading a controller in admin server: routeToAdminServer short-circuit in jsonAwareHttpExecute? #490

Closed safi-manar closed 5 years ago

safi-manar commented 5 years ago

Overloading a route in a controller yields different results when testing a running server vs in a FeatureTest.

Expected behavior

If I create two routes, such as /ping, one of which is on the admin port server ( get("/ping", admin = true)) and the other is not (get("/ping")), I can run a local instance of the server, and run a GET request and depending on the port, it will route me to the correct controller. I expect to be able to replicate the same result in a FeatureTest.

Actual behavior

However, if we try to test this in a FeatureTest using routeToAdminServer, we will always get routed to the private controller, even if routeToAdminServer = false.

I believe this is because in EmbeddedHttpServer, we have the following line:

    if (routeToAdminServer || matchesAdminRoute(request.method, request.path)) {
      httpAdminClient(
        request,
        headers,
        suppress,
        andExpect,
        withLocation,
        withBody
      )

so that even if routeToAdminServer=false, if matchesAdminRoute() returns true, we will resolve the request using httpAdminClient. Should the condiiton here be routeToAdminServer && matchesAdminRoute() to match observed behavior on a real server?

cacoco commented 5 years ago

@safi-manar it is not necessarily expected to have the same route served on both the admin and external interfaces (since generally admin routes are expected to start with /admin to maintain conformance with the TwitterServer HTTP Admin interface -- so you'd have /ping and /admin/ping) hence this logic.

We are working to rewrite the embedded server logic and will take this case into consideration.

Thanks!

cacoco commented 5 years ago

@safi-manar this was fixed in 4653992cf0d14481aa3faeae4071c873b6a81a5d. Please take look!

safi-manar commented 5 years ago

Very cool-- thank you! 🙂