Skip to content

Resolve "Redirect to new project link after a rename"

Michael Kozono requested to merge mkozono/gitlab-ce:17361-redirect-renamed-paths into master

What does this MR do?

When the path of a routable object (Group, Namespace, Project) is changed, a redirect is created from the old path to the new. This also applies to route descendants, e.g. renaming a group affects the paths of subgroups and projects.

Furthermore, new or updated routes override redirects with conflicting paths.

Are there points in the code the reviewer needs to double check?

  • I've introduced the concept of a redirect route vs a canonical route, but I am leveraging the name field to determine route type (null name is a redirect). This violates the principle of least surprise, but I am unsure if it is worth adding a column e.g. a boolean called "redirect" or "canonical" at this time.
  • Any routable that is found by find_by_full_path will “work”, but any action that doesn’t implement a redirect to the canonical path will work with old paths as if they are the canonical path. I’ve implemented a number of redirects in this MR, but as I’m not familiar with the whole codebase, it’s possible I’ve missed a case where this is needed.
  • Non-GET requests are not redirected, which in some cases means that you can, for example, POST to the old path and it will work until the redirect is overridden by a new canonical route. I’m not familiar with HTTP code 307, and am unsure if that would be appropriate. http://softwareengineering.stackexchange.com/questions/99894/why-doesnt-http-have-post-redirect

Edit 2017-05-03 (after the addition of the redirect_routes table)

We already redirect wrong-cased URLs when GET-requesting projects paths. I added this behavior for users and groups. E.g. If my username was MKozono and you visit gitlab.com/mkozono, you'll be redirected to gitlab.com/MKozono. It seems like good behavior, and as a bonus, it kept my changes a few lines simpler.

I copied/adapted the indexes on routes for redirect_routes since this table is used in similar ways.

Questions and things that I haven't gotten around to

  • Do we need to look at git protocol stuff?
  • Do we need to redirect from old paths of renamed users? The user’s projects will properly redirect with my implementation as-is. But we don’t use Routable for users, so e.g. the old user show path won't redirect.
  • Routable’s with_route may not always work properly if used during a request to a redirect route. This needs to be checked. Using order(name: :asc) instead of where.not(name: nil) in Routable's has_one :route scope may fix this.
  • Namespace#ancestors may not always work properly if used during a request to a redirect route. This needs to be checked. Using order(name: :asc) instead of where.not(name: nil) in Routable's has_one :route scope may fix this.
  • Warning copy in the UI may need to change to indicate the new redirect behavior.
  • Add many tests

Edit 2017-05-03 (after the addition of the redirect_routes table)

  • I still need to double check what happens to git requests Edit: See the later commits.
  • I still need to look at warning copy in the UI Edit: looks good enough to me.

Why was this MR needed?

Renaming groups and projects is a valuable feature, but the user is forced to accept that they are breaking links. This redirect behavior will make renaming less impactful (in a good way). See #17361 (closed).

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Closes #17361 (closed)

Closes #30317 (closed)

Merge request reports