Removing trailing slashes #80
Merged
rand
merged 27 commits from 20220208_removing_trailing_slashes
into main
10 months ago
Loading…
Reference in new issue
There is no content yet.
Delete Branch '20220208_removing_trailing_slashes'
Deleting a branch is permanent. It CANNOT be undone. Continue?
This PR solves the issue of there being both "/" and "" paths, that in the builder-web context shouldn't mean different things.
The slashes are now removed using a
Dream
middleware, and the request is redirected using a permanent redirect (that doesn't change the method used): https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/LocationTrailing slashes from the hardcoded link urls were removed, as unneccesary redirects are then avoided.
I wonder, since albatross-client and builder-server interact with the HTTP server whether it would be more suitable to keep the
/
and redirect e.g./job/:job
to/job/:job
- so add a trailin slash if it is not present. The advantage would be that the diff is smaller, and the URL structure would be preserved.Any thoughts @reynir?
I wrote https://github.com/reynir/dream-normalize-route which defines a function
normalize
that takes e.g.Dream.get
, a pattern and a handler and then appliesDream.get
on the pattern and handler and makes a permanent redirect to the url with a trailing slash if the pattern does not end in a trailing slash or otherwise a redirect to the url without the trailing slash.That would be the behavior I would want, but that solution is somewhat complex and relies on the current behavior of
Dream.get
patterns etc. :/I think always redirecting to the trailing slash version is acceptable, too. Although I it would be nicest to keep e.g.
/job/:job/build/:build/script
without the trailing slash.To begin with I don't think it's too much of an issue that
/job/:job
leads to an error page.Here is a list of reasons for why I chose to default to removing slashes instead of appending them:
Reason 1
A slash normally means that you list something, like with directories. If you access the thing itself, I don't think it should have a slash in the end.
E.g.
https://builds-staging.robur.coop/job/albatross/build/a75a5f8c-61f8-4d06-9ff0-f7244a986e46/
vs.
https://builds-staging.robur.coop/job/albatross/build/a75a5f8c-61f8-4d06-9ff0-f7244a986e46
Reason 2
The existing url-structure of builder-web already defines "list-pages" as named urls on top of using a strailing slash, e.g.:
.. so the usage of slash in these urls is double-information - the url without the slash already communicates that it's a list.
Reason 3
The root of the site is a list, and it already removes the trailing slash.
Reason 4
I think it feels confusing and messy to use both slash and non-slash urls. The user doesn't really have any way of guessing a correct url, as he doesn't know the existing list vs non-list semantics of the site.
Reason 5
I think that query parameters looks less aesthetic when given to a slash-adress (in the context of there not being a need for the address to have a trailing slash).
E.g.:
https://builds-staging.robur.coop/job/albatross/?platform=freebsd-12
vs.
https://builds-staging.robur.coop/job/albatross?platform=freebsd-12
.. also, most of the time, a page is both for a single entity of something, and a list of something else at the same time - as is the case for /job/:job
Pages often also change semantics over time, e.g. starting as a single thing, and then becoming a list of things. I don't think one should switch from no trailing slash to slash in this case.
So overall my ideals for this is simplification, consistency and avoiding deprecation of urls.
@reynir I looked at the implementation - it needs to pass the query params on to the redirect too, else a page like https://builds-staging.robur.coop/job/albatross/?platform=ubuntu-20.04 breaks
I pushed some minor changes. Looks fine to merge now I think.
let path_matches_dreamroute ~path dreamroute =
let is_match path_elem dpath_elem =
(dpath_elem |> String.starts_with ~prefix:":")
String.starts_with
was introduced in 4.13.0, so we need to update the constraint.let matches_dreamroute ~path dreamroute =
let is_match path_elem dpath_elem =
(dpath_elem |> String.starts_with ~prefix:":")
We need to bump ocaml >= 4.13.0 for
String.split_on_char
0f493e9b47
into main 10 months agoReviewers
0f493e9b47
.