Removing trailing slashes #80

Merged
rand merged 27 commits from 20220208_removing_trailing_slashes into main 4 months ago
rand commented 6 months ago
Owner

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/Location

Trailing slashes from the hardcoded link urls were removed, as unneccesary redirects are then avoided.

This PR solves the issue of there being both "<url>/" and "<url>" 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/Location Trailing slashes from the hardcoded link urls were removed, as unneccesary redirects are then avoided.
rand added 5 commits 6 months ago
rand added 1 commit 6 months ago
rand added 1 commit 6 months ago
rand added 1 commit 6 months ago
Owner

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 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?
Owner

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 applies Dream.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.

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 applies `Dream.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.
Poster
Owner

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.:

  • /job/:job/failed/
  • /failed-builds/

.. 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

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.: * /job/:job/failed/ * /failed-builds/ .. 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
Poster
Owner

.. 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.

.. 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.
Poster
Owner

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 applies Dream.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.

@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 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 applies `Dream.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. > @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
rand added 1 commit 6 months ago
rand added 1 commit 5 months ago
reynir added 1 commit 4 months ago
c17b588efb Use Dream.to_form_urlencoded for query parameters
rand added 1 commit 4 months ago
rand added 1 commit 4 months ago
rand added 3 commits 4 months ago
rand added 4 commits 4 months ago
d25d8023e0 Builder_web: Mixed:
rand added 1 commit 4 months ago
rand added 1 commit 4 months ago
rand added 1 commit 4 months ago
reynir added 4 commits 4 months ago
d665cb2943 Bump ocaml >= 4.13.0
870acf3c4a Redirect HEAD as well as GET, comments
reynir approved these changes 4 months ago
reynir left a comment

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:":")
Poster
Owner

String.starts_with was introduced in 4.13.0, so we need to update the constraint.

`String.starts_with` was introduced in 4.13.0, so we need to update the constraint.
reynir marked this conversation as resolved
let matches_dreamroute ~path dreamroute =
let is_match path_elem dpath_elem =
(dpath_elem |> String.starts_with ~prefix:":")
Poster
Owner

We need to bump ocaml >= 4.13.0 for String.split_on_char

We need to bump ocaml >= 4.13.0 for `String.split_on_char`
reynir marked this conversation as resolved
rand merged commit 0f493e9b47 into main 4 months ago
rand deleted branch 20220208_removing_trailing_slashes 4 months ago
rand referenced this issue from a commit 4 months ago

Reviewers

reynir approved these changes 4 months ago
The pull request has been merged as 0f493e9b47.
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.