athena-framework / athena

An ecosystem of reusable, independent components
https://athenaframework.org
MIT License
211 stars 17 forks source link

Inconsistent routing destinations for similar paths #286

Closed yanecc closed 1 year ago

yanecc commented 1 year ago

When I have similar routes such as "/path" and "/path/", I expect them to have the same routing destination. Many popular frameworks do as what athena do with "" and "/". refering

Besides, when the route has optional params, it doesn't work as expected. With "/foo/{bar?}/", requests to "/foo/" will lead to a 404 Not Found status.

Blacksmoke16 commented 1 year ago

So /path and /path/ being different is the correct behavior because they have different semantics and are treated as unique URLs by caches and such. Plus I think as you pointed out, it also leaves room for ambiguity when it comes to matching optional path parameters. E.g. given you have a /foo/ and /foo/{slug} routes, should /foo/ match the first one, or should it match the second with an empty slug?

However, while I was looking into this to confirm, I realized there is a key part of it missing. It should be automatically redirecting GET/HEAD requests to /path to /path/ and vice versa. This is something that has yet to be implemented. But I'm thinking shouldn't be too hard to do for the next minor release. For methods other than those, the solution would be just use the correct path. Or once #198 is implemented, define both variants on the action.

yanecc commented 1 year ago

There exists a routing-related problem yet. With the current approach, routes with such optional parameters not at the last as /{name?}/{ssid} have the same effect as /{name}/{ssid}. The optional parameters can not be ignored indeed.

Blacksmoke16 commented 1 year ago

That's kind of by design. It doesn't really make sense to allow having a route like /{name?}/{ssid}, but being able to just omit the name and have it match something as if it was just /{ssid}. There can be multiple optional parameters, but all the ones after an optional parameter need to also be optional.

This is also called out in the API docs:

image

Blacksmoke16 commented 1 year ago

@18183883296 This will be somewhat alleviated by #307. In that GET/HEAD requests made to /path/ for a route that expects /path will be gracefully handled via a redirect.

I got that merged the other day, and will do a release in the coming weeks once I wrap up some additional console component changes.