Merge branch '17361-redirect-renamed-paths' into 'master'

Resolve "Redirect to new project link after a rename"

Closes #17361 and #30317

See merge request !11136

4 files changed, 83 insertions, 17 deletions

diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb
index b28e05d0c28..e351dbb45dd 100644
--- a/app/models/concerns/routable.rb
+++ b/app/models/concerns/routable.rb
@@ -5,6 +5,7 @@ module Routable
 
   included do
     has_one :route, as: :source, autosave: true, dependent: :destroy
+    has_many :redirect_routes, as: :source, autosave: true, dependent: :destroy
 
     validates_associated :route
     validates :route, presence: true
@@ -26,16 +27,31 @@ module Routable
     #     Klass.find_by_full_path('gitlab-org/gitlab-ce')
     #
     # Returns a single object, or nil.
-    def find_by_full_path(path)
+    def find_by_full_path(path, follow_redirects: false)
       # On MySQL we want to ensure the ORDER BY uses a case-sensitive match so
       # any literal matches come first, for this we have to use "BINARY".
       # Without this there's still no guarantee in what order MySQL will return
       # rows.
+      #
+      # Why do we do this?
+      #
+      # Even though we have Rails validation on Route for unique paths
+      # (case-insensitive), there are old projects in our DB (and possibly
+      # clients' DBs) that have the same path with different cases.
+      # See https://gitlab.com/gitlab-org/gitlab-ce/issues/18603. Also note that
+      # our unique index is case-sensitive in Postgres.
       binary = Gitlab::Database.mysql? ? 'BINARY' : ''
-
       order_sql = "(CASE WHEN #{binary} routes.path = #{connection.quote(path)} THEN 0 ELSE 1 END)"
-
-      where_full_path_in([path]).reorder(order_sql).take
+      found = where_full_path_in([path]).reorder(order_sql).take
+      return found if found
+
+      if follow_redirects
+        if Gitlab::Database.postgresql?
+          joins(:redirect_routes).find_by("LOWER(redirect_routes.path) = LOWER(?)", path)
+        else
+          joins(:redirect_routes).find_by(path: path)
+        end
+      end
     end
 
     # Builds a relation to find multiple objects by their full paths.
diff --git a/app/models/redirect_route.rb b/app/models/redirect_route.rb
new file mode 100644
index 00000000000..99812bcde53
--- /dev/null
+++ b/app/models/redirect_route.rb
@@ -0,0 +1,12 @@
+class RedirectRoute < ActiveRecord::Base
+  belongs_to :source, polymorphic: true
+
+  validates :source, presence: true
+
+  validates :path,
+    length: { within: 1..255 },
+    presence: true,
+    uniqueness: { case_sensitive: false }
+
+  scope :matching_path_and_descendants, -> (path) { where('redirect_routes.path = ? OR redirect_routes.path LIKE ?', path, "#{sanitize_sql_like(path)}/%") }
+end
diff --git a/app/models/route.rb b/app/models/route.rb
index 4b3efab5c3c..12a7fa3d01b 100644
--- a/app/models/route.rb
+++ b/app/models/route.rb
@@ -8,29 +8,58 @@ class Route < ActiveRecord::Base
     presence: true,
     uniqueness: { case_sensitive: false }
 
+  after_create :delete_conflicting_redirects
+  after_update :delete_conflicting_redirects, if: :path_changed?
+  after_update :create_redirect_for_old_path
   after_update :rename_descendants
 
   scope :inside_path, -> (path) { where('routes.path LIKE ?', "#{sanitize_sql_like(path)}/%") }
 
   def rename_descendants
-    if path_changed? || name_changed?
-      descendants = self.class.inside_path(path_was)
+    return unless path_changed? || name_changed?
 
-      descendants.each do |route|
-        attributes = {}
+    descendant_routes = self.class.inside_path(path_was)
 
-        if path_changed? && route.path.present?
-          attributes[:path] = route.path.sub(path_was, path)
-        end
+    descendant_routes.each do |route|
+      attributes = {}
 
-        if name_changed? && name_was.present? && route.name.present?
-          attributes[:name] = route.name.sub(name_was, name)
-        end
+      if path_changed? && route.path.present?
+        attributes[:path] = route.path.sub(path_was, path)
+      end
 
-        # Note that update_columns skips validation and callbacks.
-        # We need this to avoid recursive call of rename_descendants method
-        route.update_columns(attributes) unless attributes.empty?
+      if name_changed? && name_was.present? && route.name.present?
+        attributes[:name] = route.name.sub(name_was, name)
+      end
+
+      if attributes.present?
+        old_path = route.path
+
+        # Callbacks must be run manually
+        route.update_columns(attributes)
+
+        # We are not calling route.delete_conflicting_redirects here, in hopes
+        # of avoiding deadlocks. The parent (self, in this method) already
+        # called it, which deletes conflicts for all descendants.
+        route.create_redirect(old_path) if attributes[:path]
       end
     end
   end
+
+  def delete_conflicting_redirects
+    conflicting_redirects.delete_all
+  end
+
+  def conflicting_redirects
+    RedirectRoute.matching_path_and_descendants(path)
+  end
+
+  def create_redirect(path)
+    RedirectRoute.create(source: source, path: path)
+  end
+
+  private
+
+  def create_redirect_for_old_path
+    create_redirect(path_was) if path_changed?
+  end
 end
diff --git a/app/models/user.rb b/app/models/user.rb
index 59f2be3ba9d..accaa91b805 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -337,6 +337,11 @@ class User < ActiveRecord::Base
       find_by(id: Key.unscoped.select(:user_id).where(id: key_id))
     end
 
+    def find_by_full_path(path, follow_redirects: false)
+      namespace = Namespace.find_by_full_path(path, follow_redirects: follow_redirects)
+      namespace&.owner
+    end
+
     def reference_prefix
       '@'
     end
@@ -359,6 +364,10 @@ class User < ActiveRecord::Base
     end
   end
 
+  def full_path
+    username
+  end
+
   def self.internal_attributes
     [:ghost]
   end