Merge branch 'fix-219658-add-route-to-ghost-lost-and-found-group' into 'master'

Fix - Update routes for lost-and-found group and projects

See merge request gitlab-org/gitlab!34285

changelogs/unreleased/219658-add-route-to-ghost-lost-and-found-group.yml

+---
+title: Add route for the lost-and-found group and update the route of orphaned projects
+merge_request: 34285
+author:
+type: fixed

db/post_migrate/20200602143020_update_routes_for_lost_and_found_group_and_orphaned_projects.rb

+# frozen_string_literal: true
+
+# This migration adds or updates the routes for all the entities affected by
+#  post-migration '20200511083541_cleanup_projects_with_missing_namespace'
+# - A route is added for the 'lost-and-found' group
+# - A route is added for the Ghost user (if not already defined)
+# - The routes for all the orphaned projects that were moved under the 'lost-and-found'
+#   group are updated to reflect the new path
+class UpdateRoutesForLostAndFoundGroupAndOrphanedProjects < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  class User < ActiveRecord::Base
+    self.table_name = 'users'
+
+    LOST_AND_FOUND_GROUP = 'lost-and-found'
+    USER_TYPE_GHOST = 5
+    ACCESS_LEVEL_OWNER = 50
+
+    has_one :namespace, -> { where(type: nil) },
+            foreign_key: :owner_id, inverse_of: :owner, autosave: true,
+            class_name: 'UpdateRoutesForLostAndFoundGroupAndOrphanedProjects::Namespace'
+
+    def lost_and_found_group
+      # Find the 'lost-and-found' group
+      # There should only be one Group owned by the Ghost user starting with 'lost-and-found'
+      Group
+        .joins('INNER JOIN members ON namespaces.id = members.source_id')
+        .where('namespaces.type = ?', 'Group')
+        .where('members.type = ?', 'GroupMember')
+        .where('members.source_type = ?', 'Namespace')
+        .where('members.user_id = ?', self.id)
+        .where('members.access_level = ?', ACCESS_LEVEL_OWNER)
+        .find_by(Group.arel_table[:name].matches("#{LOST_AND_FOUND_GROUP}%"))
+    end
+
+    class << self
+      # Return the ghost user
+      def ghost
+        User.find_by(user_type: USER_TYPE_GHOST)
+      end
+    end
+  end
+
+  # Temporary Concern to not repeat the same methods twice
+  module HasPath
+    extend ActiveSupport::Concern
+
+    def full_path
+      if parent && path
+        parent.full_path + '/' + path
+      else
+        path
+      end
+    end
+
+    def full_name
+      if parent && name
+        parent.full_name + ' / ' + name
+      else
+        name
+      end
+    end
+  end
+
+  class Namespace < ActiveRecord::Base
+    include HasPath
+
+    self.table_name = 'namespaces'
+
+    belongs_to :owner, class_name: 'UpdateRoutesForLostAndFoundGroupAndOrphanedProjects::User'
+    belongs_to :parent, class_name: "UpdateRoutesForLostAndFoundGroupAndOrphanedProjects::Namespace"
+    has_many :children, foreign_key: :parent_id,
+             class_name: "UpdateRoutesForLostAndFoundGroupAndOrphanedProjects::Namespace"
+    has_many :projects, class_name: "UpdateRoutesForLostAndFoundGroupAndOrphanedProjects::Project"
+
+    def ensure_route!
+      unless Route.for_source('Namespace', self.id)
+        Route.create!(
+          source_id: self.id,
+          source_type: 'Namespace',
+          path: self.full_path,
+          name: self.full_name
+        )
+      end
+    end
+
+    def generate_unique_path
+      # Generate a unique path if there is no route for the namespace
+      # (an existing route guarantees that the path is already unique)
+      unless Route.for_source('Namespace', self.id)
+        self.path = Uniquify.new.string(self.path) do |str|
+          Route.where(path: str).exists?
+        end
+      end
+    end
+  end
+
+  class Group < Namespace
+    # Disable STI to allow us to manually set "type = 'Group'"
+    # Otherwise rails forces "type = CleanupProjectsWithMissingNamespace::Group"
+    self.inheritance_column = :_type_disabled
+  end
+
+  class Route < ActiveRecord::Base
+    self.table_name = 'routes'
+
+    def self.for_source(source_type, source_id)
+      Route.find_by(source_type: source_type, source_id: source_id)
+    end
+  end
+
+  class Project < ActiveRecord::Base
+    include HasPath
+
+    self.table_name = 'projects'
+
+    belongs_to :group, -> { where(type: 'Group') }, foreign_key: 'namespace_id',
+      class_name: "UpdateRoutesForLostAndFoundGroupAndOrphanedProjects::Group"
+    belongs_to :namespace,
+      class_name: "UpdateRoutesForLostAndFoundGroupAndOrphanedProjects::Namespace"
+
+    alias_method :parent, :namespace
+    alias_attribute :parent_id, :namespace_id
+
+    def ensure_route!
+      Route.find_or_initialize_by(source_type: 'Project', source_id: self.id).tap do |record|
+        record.path = self.full_path
+        record.name = self.full_name
+        record.save!
+      end
+    end
+  end
+
+  def up
+    # Reset the column information of all the models that update the database
+    # to ensure the Active Record's knowledge of the table structure is current
+    Namespace.reset_column_information
+    Route.reset_column_information
+
+    # Find the ghost user, its namespace and the "lost and found" group
+    ghost_user = User.ghost
+    return unless ghost_user # No reason to continue if there is no Ghost user
+
+    ghost_namespace = ghost_user.namespace
+    lost_and_found_group = ghost_user.lost_and_found_group
+
+    # No reason to continue if there is no 'lost-and-found' group
+    # 1. No orphaned projects were found in this instance, or
+    # 2. The 'lost-and-found' group and the orphaned projects have been already deleted
+    return unless lost_and_found_group
+
+    # Update the 'lost-and-found' group description to be more self-explanatory
+    lost_and_found_group.generate_unique_path
+    lost_and_found_group.description =
+      'Group for storing projects that were not properly deleted. '\
+      'It should be considered as a system level Group with non-working '\
+      'projects inside it. The contents may be deleted with a future update. '\
+      'More info: gitlab.com/gitlab-org/gitlab/-/issues/198603'
+    lost_and_found_group.save!
+
+    # Update the routes for the Ghost user, the "lost and found" group
+    # and all the orphaned projects
+    ghost_namespace.ensure_route!
+    lost_and_found_group.ensure_route!
+
+    # The following does a fast index scan by namespace_id
+    # No reason to process in batches:
+    # - 66 projects in GitLab.com, less than 1ms execution time to fetch them
+    #   with a constant update time for each
+    lost_and_found_group.projects.each do |project|
+      project.ensure_route!
+    end
+  end
+
+  def down
+    # no-op
+  end
+end

db/structure.sql

@@ -13974,6 +13974,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200601210148
 20200602013900
 20200602013901
+20200602143020
 20200603073101
 20200603180338
 20200604143628

spec/migrations/cleanup_projects_with_missing_namespace_spec.rb

@@ -5,10 +5,6 @@ require 'spec_helper'
 require Rails.root.join('db', 'post_migrate', '20200511080113_add_projects_foreign_key_to_namespaces.rb')
 require Rails.root.join('db', 'post_migrate', '20200511083541_cleanup_projects_with_missing_namespace.rb')
 
-LOST_AND_FOUND_GROUP = 'lost-and-found'
-USER_TYPE_GHOST = 5
-ACCESS_LEVEL_OWNER = 50
-
 # In order to test the CleanupProjectsWithMissingNamespace migration, we need
 #  to first create an orphaned project (one with an invalid namespace_id)
 #  and then run the migration to check that the project was properly cleaned up
@@ -77,31 +73,39 @@ describe CleanupProjectsWithMissingNamespace, :migration, schema: SchemaVersionF
   end
 
   it 'creates the ghost user' do
-    expect(users.where(user_type: USER_TYPE_GHOST).count).to eq(0)
+    expect(users.where(user_type: described_class::User::USER_TYPE_GHOST).count).to eq(0)
 
     disable_migrations_output { migrate! }
 
-    expect(users.where(user_type: USER_TYPE_GHOST).count).to eq(1)
+    expect(users.where(user_type: described_class::User::USER_TYPE_GHOST).count).to eq(1)
   end
 
   it 'creates the lost-and-found group, owned by the ghost user' do
     expect(
-      Group.where(Group.arel_table[:name].matches("#{LOST_AND_FOUND_GROUP}%")).count
+      described_class::Group.where(
+        described_class::Group
+        .arel_table[:name]
+        .matches("#{described_class::User::LOST_AND_FOUND_GROUP}%")
+      ).count
     ).to eq(0)
 
     disable_migrations_output { migrate! }
 
-    ghost_user = users.find_by(user_type: USER_TYPE_GHOST)
+    ghost_user = users.find_by(user_type: described_class::User::USER_TYPE_GHOST)
     expect(
-      Group
+      described_class::Group
         .joins('INNER JOIN members ON namespaces.id = members.source_id')
         .where('namespaces.type = ?', 'Group')
         .where('members.type = ?', 'GroupMember')
         .where('members.source_type = ?', 'Namespace')
         .where('members.user_id = ?', ghost_user.id)
         .where('members.requested_at IS NULL')
-        .where('members.access_level = ?', ACCESS_LEVEL_OWNER)
-        .where(Group.arel_table[:name].matches("#{LOST_AND_FOUND_GROUP}%"))
+        .where('members.access_level = ?', described_class::ACCESS_LEVEL_OWNER)
+        .where(
+          described_class::Group
+          .arel_table[:name]
+          .matches("#{described_class::User::LOST_AND_FOUND_GROUP}%")
+        )
         .count
     ).to eq(1)
   end
@@ -114,7 +118,11 @@ describe CleanupProjectsWithMissingNamespace, :migration, schema: SchemaVersionF
 
     disable_migrations_output { migrate! }
 
-    lost_and_found_group = Group.find_by(Group.arel_table[:name].matches("#{LOST_AND_FOUND_GROUP}%"))
+    lost_and_found_group = described_class::Group.find_by(
+      described_class::Group
+      .arel_table[:name]
+      .matches("#{described_class::User::LOST_AND_FOUND_GROUP}%")
+    )
     orphaned_project = projects.find_by(id: orphaned_project.id)
 
     expect(orphaned_project.visibility_level).to eq(0)

spec/migrations/update_routes_for_lost_and_found_group_and_orphaned_projects_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require Rails.root.join('db', 'post_migrate', '20200602143020_update_routes_for_lost_and_found_group_and_orphaned_projects.rb')
+
+describe UpdateRoutesForLostAndFoundGroupAndOrphanedProjects, :migration do
+  let(:users) { table(:users) }
+  let(:namespaces) { table(:namespaces) }
+  let(:members) { table(:members) }
+  let(:projects) { table(:projects) }
+  let(:routes) { table(:routes) }
+
+  before do
+    # Create a Ghost User and its namnespace, but skip the route
+    ghost_user = users.create!(
+      name: 'Ghost User',
+      username: 'ghost',
+      email: 'ghost@example.com',
+      user_type: described_class::User::USER_TYPE_GHOST,
+      projects_limit: 100,
+      state: :active,
+      bio: 'This is a "Ghost User"'
+    )
+
+    namespaces.create!(
+      name: 'Ghost User',
+      path: 'ghost',
+      owner_id: ghost_user.id,
+      visibility_level: 20
+    )
+
+    # Create the 'lost-and-found', owned by the Ghost user, but with no route
+    lost_and_found_group = namespaces.create!(
+      name: described_class::User::LOST_AND_FOUND_GROUP,
+      path: described_class::User::LOST_AND_FOUND_GROUP,
+      type: 'Group',
+      description: 'Group to store orphaned projects',
+      visibility_level: 0
+    )
+
+    members.create!(
+      type: 'GroupMember',
+      source_id: lost_and_found_group.id,
+      user_id: ghost_user.id,
+      source_type: 'Namespace',
+      access_level: described_class::User::ACCESS_LEVEL_OWNER,
+      notification_level: 3
+    )
+
+    # Add an orphaned project under 'lost-and-found' but with the wrong path in its route
+    orphaned_project = projects.create!(
+      name: 'orphaned_project',
+      path: 'orphaned_project',
+      visibility_level: 20,
+      archived: false,
+      namespace_id: lost_and_found_group.id
+    )
+
+    routes.create!(
+      source_id: orphaned_project.id,
+      source_type: 'Project',
+      path: 'orphaned_project',
+      name: 'orphaned_project',
+      created_at: Time.zone.now,
+      updated_at: Time.zone.now
+    )
+
+    # Create another user named ghost which is not the Ghost User
+    # Also create a 'lost-and-found' group for them and add projects to it
+    # Purpose: test that the routes added for the 'lost-and-found' group and
+    #  its projects are unique
+    fake_ghost_user = users.create!(
+      name: 'Ghost User',
+      username: 'ghost1',
+      email: 'ghost1@example.com',
+      user_type: nil,
+      projects_limit: 100,
+      state: :active,
+      bio: 'This is NOT a "Ghost User"'
+    )
+
+    fake_ghost_user_namespace = namespaces.create!(
+      name: 'Ghost User',
+      path: 'ghost1',
+      owner_id: fake_ghost_user.id,
+      visibility_level: 20
+    )
+
+    routes.create!(
+      source_id: fake_ghost_user_namespace.id,
+      source_type: 'Namespace',
+      path: 'Ghost User',
+      name: 'ghost1',
+      created_at: Time.zone.now,
+      updated_at: Time.zone.now
+    )
+
+    fake_lost_and_found_group = namespaces.create!(
+      name: 'Lost and Found',
+      path: described_class::User::LOST_AND_FOUND_GROUP, # same path as the lost-and-found group
+      type: 'Group',
+      description: 'Fake lost and found group with the same path as the real one',
+      visibility_level: 20
+    )
+
+    routes.create!(
+      source_id: fake_lost_and_found_group.id,
+      source_type: 'Namespace',
+      path: described_class::User::LOST_AND_FOUND_GROUP, # same path as the lost-and-found group
+      name: 'Lost and Found',
+      created_at: Time.zone.now,
+      updated_at: Time.zone.now
+    )
+
+    members.create!(
+      type: 'GroupMember',
+      source_id: fake_lost_and_found_group.id,
+      user_id: fake_ghost_user.id,
+      source_type: 'Namespace',
+      access_level: described_class::User::ACCESS_LEVEL_OWNER,
+      notification_level: 3
+    )
+
+    normal_project = projects.create!(
+      name: 'normal_project',
+      path: 'normal_project',
+      visibility_level: 20,
+      archived: false,
+      namespace_id: fake_lost_and_found_group.id
+    )
+
+    routes.create!(
+      source_id: normal_project.id,
+      source_type: 'Project',
+      path: "#{described_class::User::LOST_AND_FOUND_GROUP}/normal_project",
+      name: 'Lost and Found / normal_project',
+      created_at: Time.zone.now,
+      updated_at: Time.zone.now
+    )
+  end
+
+  it 'creates the route for the ghost user namespace' do
+    expect(routes.where(path: 'ghost').count).to eq(0)
+
+    disable_migrations_output { migrate! }
+
+    expect(routes.where(path: 'ghost').count).to eq(1)
+  end
+
+  it 'fixes the path for the lost-and-found group by generating a unique one' do
+    expect(namespaces.where(path: described_class::User::LOST_AND_FOUND_GROUP).count).to eq(2)
+
+    disable_migrations_output { migrate! }
+
+    expect(namespaces.where(path: described_class::User::LOST_AND_FOUND_GROUP).count).to eq(1)
+
+    lost_and_found_group = namespaces.find_by(name: described_class::User::LOST_AND_FOUND_GROUP)
+    expect(lost_and_found_group.path).to eq('lost-and-found1')
+  end
+
+  it 'creates the route for the lost-and-found group' do
+    expect(routes.where(path: described_class::User::LOST_AND_FOUND_GROUP).count).to eq(1)
+    expect(routes.where(path: 'lost-and-found1').count).to eq(0)
+
+    disable_migrations_output { migrate! }
+
+    expect(routes.where(path: described_class::User::LOST_AND_FOUND_GROUP).count).to eq(1)
+    expect(routes.where(path: 'lost-and-found1').count).to eq(1)
+  end
+
+  it 'updates the route for the orphaned project' do
+    orphaned_project_route = routes.find_by(path: 'orphaned_project')
+    expect(orphaned_project_route.name).to eq('orphaned_project')
+
+    disable_migrations_output { migrate! }
+
+    updated_route = routes.find_by(id: orphaned_project_route.id)
+    expect(updated_route.path).to eq('lost-and-found1/orphaned_project')
+    expect(updated_route.name).to eq("#{described_class::User::LOST_AND_FOUND_GROUP} / orphaned_project")
+  end
+end