Allow overlapping route definitions

[david-crespo]

Whether we need this depends on some other architectural decisions. I'm writing this up so I can refer to it elsewhere.

Right now, defining both GET /abc/* and GET /abc/def is an error at startup because the route definitions overlap, and Dropshot would not know which to handler to call when a request matches both routes. There are a couple of ways we could break ties:

• Order of route registration
  • Simple to implement (I think? we're already using BTreeMap for the route nodes, so we know the insertion order) and to understand
  • Too easy to accidentally mess up the logic by moving routes around
• Specificity of matching
  • Request to /abc/def goes to the exact match because it ranks higher than a wildcard match
  • While the logic is hidden in the implementation, the idea of match specificity is pretty intuitive. You can only change matching logic by changing the route paths, which is less error-prone than changing it by shuffling things around. However, it has the similar problem that deleting a more specific route can cause a wildcard to start matching requests it didn't match before.
  • A little more work to implement, but not too bad. something in here
  • Example ranking algorithms
    • Rocket
    • React Router

[davepacheco]

Off the top of my head, I think the reasons we chose to disallow this early on were (1) we hoped this would encourage clearer code structure. I'd find it annoying to have to track down which handler was going to be run for a request by mentally evaluating the routing table's rules. and (2) it's easy to do this accidentally and be very confused by the behavior. I burned a bunch of time with Actix debugging why a different handler was being run than what I intended.

[ahl]

I'd suggest that this is an area where Dropshot may be opinionated: I view the absence of this as a feature of Dropshot.

[david-crespo]

That's fine with me as long as we have a nice way of solving the proximate issue (oxidecomputer/omicron#430), which is distinguishing console routes and API routes in Nexus, ideally without having an ugly contrived prefix like /c/ in front of the console routes. (I don't think of /orgs as contrived because it's meaningful.)

We can probably just stick with a slight modification of what we're already doing: use /api/* (not actually implemented with a wildcard) for API routes, and use some other prefix or set of prefixes that work for console routes in order to avoid the overlapping definitions. We currently have /orgs/* for console pages and /assets/* for console assets. When we want some console page route to start with something other than /orgs, all we have to do is add another handler to Nexus with the new prefix that does exactly the same thing as the existing /orgs/* handler does.

Right now the API routes don't actually start with /api (almost all of them start with /organizations) but for our purposes all that matters is that it is something other than what I'm using for the console route handler.

[ahl]

I'd suggest that this is an area where Dropshot may be opinionated: I view the absence of this as a feature of Dropshot.

Respectfully, I'd like to disagree. For example, having a catch-all /* route to serve a custom 404 page is pretty important to some consumers. Instead I'd suggest that we allow overlapping routes, and prefer specific routes over generic routes from left to right.

[jclulow]

Respectfully, I'd like to disagree. For example, having a catch-all /* route to serve a custom 404 page is pretty important to some consumers. Instead I'd suggest that we allow overlapping routes, and prefer specific routes over generic routes from left to right.

I agree with ahl′ -- I would in some places use a catch-all route myself, with the knowledge that it is on my own recognisance.

[davepacheco]

I think it's more of a footgun than it looks because adding any route in the catch-all space is a potentially breaking change, and you'd have no way to know. As a concrete example, suppose we went down this path to solve oxidecomputer/omicron#430, so that /* went to a console catch-all handler and specific /organizations, /projects, etc. went to specific API handlers. If someone happened to add an API route that happens to match a client-side console route (which you would never know, because those routes are client-side-only and in a different repo), wouldn't it break that console screen? Worse, I don't think you'd notice the breakage unless someone went directly to that route in the console or else navigated to it and refreshed the page.

Agreed on the 404 page use case, but I wonder if there are safer ways to provide that (see #39/#40)?

[jclulow]

So it's been two years and I would still really like to have this functionality. Would it be alright to add something like this if we can have it be off by default, thus requiring people to opt in?

[davepacheco]

In offline discussion I think we decided:

• could use customisation of error presentation #39 would probably address this specific use case, and seems good because it solves a bunch of problems, but is probably a fair bit of work.
• It still makes me nervous to have something that's easy to use that enables this behavior. IIRC something like what I described above already did happen in Omicron. But if we can make this option sufficiently off the golden path, that'd be okay. (I would really like to not use this in Omicron.)

[david-crespo]

For the console routes, the lack of overlapping routes is clearly not a big deal. Every once in a while we have to add a new top-level path here. That's it.

https://github.com/oxidecomputer/omicron/blob/aa7b087e28fadc79f1770f78bfb1f7f73ec82575/nexus/external-api/src/lib.rs#L2763-L2870

[steveklabnik]

I have found the lack of this annoying, because I can't do the rails-like URLs of /parts/:id and /parts/new. It's not the end of the world, as I can use /parts-new, but it's not my favorite.