Open-EO / openeo-r-backend

A reference implementation for the openEO core API as a proof-of-concept
Apache License 2.0
6 stars 3 forks source link

Endpoints shouldn't have a trailing slash #17

Closed christophfriedrich closed 5 years ago

christophfriedrich commented 5 years ago

Some endpoint URLs have a trailing slash, others do not have one (e.g. /services/ vs. /service_types). The latter version should be used consistently for all endpoints, because that's the way the spec defines them.

christophfriedrich commented 5 years ago

I just read the Notes section of the README and realised that the current situation didn't happen by accident, but is due to a technical limitation:

There are some minor variations to the openEO API, regarding naming of the endpoints. Due to the different access methods we use multiple plumber routes that run on a shared root route. By doing this we cannot leave an endpoint blank, which means that some enpoints require a trailing /. For example, you will need to query GET http://host:port/api/processes/ to fetch the list of offered processes. The basic rule of thump here, is that all the endpoints directly after /api/xxx need the trailing slash, but not the basic server endpoints like capabilities.

Is there really no way to fix this?! I don't see this as "minor variations" -- it's a MAJOR incompatibility!

flahn commented 5 years ago

Since the capabilities request contains the "path" to the endpoints, I don't see it as a "major incompatibility" since you simply need to check if there is a trailing slash (a simple .endsWith checking for a / will probably do it.

As background for this: I run plumber on multiple endpoint nodes, e.g. all the endpoints on /jobs, /process_graph, etc. That is for the reason that there a different filter in use, e.g. authorization, or simply for the reason of maintainability. Plumber has currently no implementation to allow an endpoint "" registered to such an endpoint node, that is the reason why the implementation is as it is right now.

christophfriedrich commented 5 years ago

Of course it is possible to work around this, but that's not what I want to do -- the very reason we do this whole openEO project is that every client should seamlessly communicate with every back-end, so (since we left POC state) I'm arguing against any custom behaviour.

I had a look at that plumber package. Is it not possible to use the annotation syntax for all the routes? Or use the full /api/... URL as the handle method's second argument, bypassing that mount? Maybe even get hacky and define /api/job instead of /api/jobs and then use s as the "endpoint" (and s/<job_id> etc.) because "" is not possible? I'm no expert at all, but I can't believe that it's not possible to mount the handler functions to the proper URLs.

flahn commented 5 years ago

I looked into the back-end code again and I was willing to give it a try to fix it. However, I realized that I had stopped fixing it prior due to various reasons. Also for backtracking I will simply list the insights.

  1. plumber uses router / endpoints and filters as core concepts. A router contains one or more endpoints and redirects the HTTP calls those endpoints. The router itself can be mounted on a certain path on a "root" router. The endpoints are defined to be functions which are executed by a router, when a HTTP call is resolved. Filters on the otherhand can be added to a router. They specify how the HTTP request payload might be updated before the endpoint function is called, e.g. authentication is often realized by a filter. First filters can only be added to a router and second not all endpoints are required to be used by an authenticated user, e.g. get capabilities or list data or list processes, those are the main reasons why I have realized the server configuration as multiple mounted router.

  2. When you use mounted router, then plumber automatically adds a slash "/" at the end of the path you have specified during the router mount. The second drawback is, that if you specify an empty path like "", when you create an endpoint and add this endpoint to a plumber router, then, sadly, the intended path can not be resolved. I tried the following for testing:

library(plumber)
root = plumber$new()
route1 = plumber$new(envir = root)
route1$handle(methods = "GET",path="",handler = function()print("Route 1"))

route2 = plumber$new(envir = root)
route2$handle(methods = "GET",path="",handler = function()print("Route 2"))

root$mount("/router/r1",route1)
root$mount("/router/r2",route2)

root$run(port=9876)

When you call http://localhost:9876/router/r1 or http://localhost:9876/router/r2 from a browser, then you will always get a 404 in return. Changing the code above so that path="/" is defined in the "handle" call, will work.

flahn commented 5 years ago

I fixed this by hacking the plumber code and implementing two R6 classes that inherit from the plumber classes plumber and PlumberEndpoint. I modified the Endpoint class so that we can now also register PlumberFilter on each endpoint.