Commit 05efd19e authored by Douwe Maan's avatar Douwe Maan

Merge branch 'dz-nested-groups' into 'master'

Add nested groups support on data level

## What does this MR do?

- [x] Add  `parent_id` field to `Namespace`model. 
- [x] Create new database table `routes` that keeps information about full path to each group or project
- [x] Remove uniq index from `namespaces.path`
- [x] Add uniq index on `routes.path`
- [x] Fill routes table with path data from namespaces and projects
- [x] Change Namespace/Project URL lookup by routes table
- [x] Rename related routes (nested groups, projects) when parent path changes

This is solely backend preparation. UI, Permissions and API support will be added in separate merge request.   
 
## Are there points in the code the reviewer needs to double check?

migrations, Route model, Routable concern

Will require downtime. See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7121#note_19490281 discussion

## Why was this MR needed?

One step further to full nested groups support

## Screenshots (if relevant)

No UI changes in this merge request so far

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG.md) entry added~~
- ~~[Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)~~
- ~~API support added~~
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if it does - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?

https://gitlab.com/gitlab-org/gitlab-ce/issues/2772

See merge request !7121
parents f23b1cb4 83232be0
......@@ -56,7 +56,7 @@ class Admin::GroupsController < Admin::ApplicationController
private
def group
@group ||= Group.find_by(path: params[:id])
@group ||= Group.find_by_full_path(params[:id])
end
def group_params
......
......@@ -9,7 +9,7 @@ class Groups::ApplicationController < ApplicationController
def group
unless @group
id = params[:group_id] || params[:id]
@group = Group.find_by(path: id)
@group = Group.find_by_full_path(id)
unless @group && can?(current_user, :read_group, @group)
@group = nil
......
......@@ -5,7 +5,7 @@ module GroupsHelper
def group_icon(group)
if group.is_a?(String)
group = Group.find_by(path: group)
group = Group.find_by_full_path(group)
end
group.try(:avatar_url) || image_path('no_group_avatar.png')
......
# Store object full path in separate table for easy lookup and uniq validation
# Object must have path db field and respond to full_path and full_path_changed? methods.
module Routable
extend ActiveSupport::Concern
included do
has_one :route, as: :source, autosave: true, dependent: :destroy
validates_associated :route
before_validation :update_route_path, if: :full_path_changed?
end
class_methods do
# Finds a single object by full path match in routes table.
#
# Usage:
#
# Klass.find_by_full_path('gitlab-org/gitlab-ce')
#
# Returns a single object, or nil.
def find_by_full_path(path)
# 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.
binary = Gitlab::Database.mysql? ? 'BINARY' : ''
order_sql = "(CASE WHEN #{binary} routes.path = #{connection.quote(path)} THEN 0 ELSE 1 END)"
where_paths_in([path]).reorder(order_sql).take
end
# Builds a relation to find multiple objects by their full paths.
#
# Usage:
#
# Klass.where_paths_in(%w{gitlab-org/gitlab-ce gitlab-org/gitlab-ee})
#
# Returns an ActiveRecord::Relation.
def where_paths_in(paths)
wheres = []
cast_lower = Gitlab::Database.postgresql?
paths.each do |path|
path = connection.quote(path)
where = "(routes.path = #{path})"
if cast_lower
where = "(#{where} OR (LOWER(routes.path) = LOWER(#{path})))"
end
wheres << where
end
if wheres.empty?
none
else
joins(:route).where(wheres.join(' OR '))
end
end
end
private
def update_route_path
route || build_route(source: self)
route.path = full_path
end
end
......@@ -4,12 +4,16 @@ class Namespace < ActiveRecord::Base
include CacheMarkdownField
include Sortable
include Gitlab::ShellAdapter
include Routable
cache_markdown_field :description, pipeline: :description
has_many :projects, dependent: :destroy
belongs_to :owner, class_name: "User"
belongs_to :parent, class_name: "Namespace"
has_many :children, class_name: "Namespace", foreign_key: :parent_id
validates :owner, presence: true, unless: ->(n) { n.type == "Group" }
validates :name,
presence: true,
......@@ -86,7 +90,7 @@ class Namespace < ActiveRecord::Base
end
def to_param
path
full_path
end
def human_name
......@@ -150,6 +154,14 @@ class Namespace < ActiveRecord::Base
Gitlab.config.lfs.enabled
end
def full_path
if parent
parent.full_path + '/' + path
else
path
end
end
private
def repository_storage_paths
......@@ -185,4 +197,8 @@ class Namespace < ActiveRecord::Base
where(projects: { namespace_id: id }).
find_each(&:refresh_members_authorized_projects)
end
def full_path_changed?
path_changed? || parent_id_changed?
end
end
......@@ -14,6 +14,7 @@ class Project < ActiveRecord::Base
include TokenAuthenticatable
include ProjectFeaturesCompatibility
include SelectForProjectAuthorization
include Routable
extend Gitlab::ConfigHelper
......@@ -324,87 +325,6 @@ class Project < ActiveRecord::Base
non_archived.where(table[:name].matches(pattern))
end
# Finds a single project for the given path.
#
# path - The full project path (including namespace path).
#
# Returns a Project, or nil if no project could be found.
def find_with_namespace(path)
namespace_path, project_path = path.split('/', 2)
return unless namespace_path && project_path
namespace_path = connection.quote(namespace_path)
project_path = connection.quote(project_path)
# 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.
binary = Gitlab::Database.mysql? ? 'BINARY' : ''
order_sql = "(CASE WHEN #{binary} namespaces.path = #{namespace_path} " \
"AND #{binary} projects.path = #{project_path} THEN 0 ELSE 1 END)"
where_paths_in([path]).reorder(order_sql).take
end
# Builds a relation to find multiple projects by their full paths.
#
# Each path must be in the following format:
#
# namespace_path/project_path
#
# For example:
#
# gitlab-org/gitlab-ce
#
# Usage:
#
# Project.where_paths_in(%w{gitlab-org/gitlab-ce gitlab-org/gitlab-ee})
#
# This would return the projects with the full paths matching the values
# given.
#
# paths - An Array of full paths (namespace path + project path) for which
# to find the projects.
#
# Returns an ActiveRecord::Relation.
def where_paths_in(paths)
wheres = []
cast_lower = Gitlab::Database.postgresql?
paths.each do |path|
namespace_path, project_path = path.split('/', 2)
next unless namespace_path && project_path
namespace_path = connection.quote(namespace_path)
project_path = connection.quote(project_path)
where = "(namespaces.path = #{namespace_path}
AND projects.path = #{project_path})"
if cast_lower
where = "(
#{where}
OR (
LOWER(namespaces.path) = LOWER(#{namespace_path})
AND LOWER(projects.path) = LOWER(#{project_path})
)
)"
end
wheres << where
end
if wheres.empty?
none
else
joins(:namespace).where(wheres.join(' OR '))
end
end
def visibility_levels
Gitlab::VisibilityLevel.options
end
......@@ -440,6 +360,10 @@ class Project < ActiveRecord::Base
def group_ids
joins(:namespace).where(namespaces: { type: 'Group' }).select(:namespace_id)
end
# Add alias for Routable method for compatibility with old code.
# In future all calls `find_with_namespace` should be replaced with `find_by_full_path`
alias_method :find_with_namespace, :find_by_full_path
end
def lfs_enabled?
......@@ -879,13 +803,14 @@ class Project < ActiveRecord::Base
end
alias_method :human_name, :name_with_namespace
def path_with_namespace
if namespace
namespace.path + '/' + path
def full_path
if namespace && path
namespace.full_path + '/' + path
else
path
end
end
alias_method :path_with_namespace, :full_path
def execute_hooks(data, hooks_scope = :push_hooks)
hooks.send(hooks_scope).each do |hook|
......@@ -1373,4 +1298,8 @@ class Project < ActiveRecord::Base
def validate_board_limit(board)
raise BoardLimitExceeded, 'Number of permitted boards exceeded' if boards.size >= NUMBER_OF_PERMITTED_BOARDS
end
def full_path_changed?
path_changed? || namespace_id_changed?
end
end
class Route < ActiveRecord::Base
belongs_to :source, polymorphic: true
validates :source, presence: true
validates :path,
length: { within: 1..255 },
presence: true,
uniqueness: { case_sensitive: false }
after_update :rename_children, if: :path_changed?
def rename_children
# We update each row separately because MySQL does not have regexp_replace.
# rubocop:disable Rails/FindEach
Route.where('path LIKE ?', "#{path_was}%").each do |route|
# Note that update column skips validation and callbacks.
# We need this to avoid recursive call of rename_children method
route.update_column(:path, route.path.sub(path_was, path))
end
end
end
......@@ -20,6 +20,10 @@ class DestroyGroupService
::Projects::DestroyService.new(project, current_user, skip_repo: true).execute
end
group.children.each do |group|
DestroyGroupService.new(group, current_user).async_execute
end
group.really_destroy!
end
end
......@@ -13,7 +13,11 @@
- if @forked_project && @forked_project.errors.any?
%p
&ndash;
= @forked_project.errors.full_messages.first
- error = @forked_project.errors.full_messages.first
- if error.include?("already been taken")
Name has already been taken
- else
= error
%p
= link_to new_namespace_project_fork_path(@project.namespace, @project), title: "Fork", class: "btn" do
......
---
title: Add nested groups support on data level
merge_request:
author:
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AddParentIdToNamespace < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def change
add_column(:namespaces, :parent_id, :integer)
end
end
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AddIndexToParentId < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
def change
add_concurrent_index(:namespaces, [:parent_id, :id], unique: true)
end
end
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AddRoutesTable < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def change
create_table :routes do |t|
t.integer :source_id, null: false
t.string :source_type, null: false
t.string :path, null: false
t.timestamps
end
end
end
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class FillRoutesTable < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = true
DOWNTIME_REASON = 'No new namespaces should be created during data copy'
def up
execute <<-EOF
INSERT INTO routes
(source_id, source_type, path)
(SELECT id, 'Namespace', path FROM namespaces)
EOF
end
def down
Route.delete_all(source_type: 'Namespace')
end
end
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class FillProjectsRoutesTable < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = true
DOWNTIME_REASON = 'No new projects should be created during data copy'
def up
execute <<-EOF
INSERT INTO routes
(source_id, source_type, path)
(SELECT projects.id, 'Project', concat(namespaces.path, '/', projects.path) FROM projects
INNER JOIN namespaces ON projects.namespace_id = namespaces.id)
EOF
end
def down
Route.delete_all(source_type: 'Project')
end
end
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class RemoveDuplicatesFromRoutes < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def up
select_all("SELECT path FROM #{quote_table_name(:routes)} GROUP BY path HAVING COUNT(*) > 1").each do |row|
path = connection.quote(row['path'])
execute(%Q{
DELETE FROM #{quote_table_name(:routes)}
WHERE path = #{path}
AND id != (
SELECT id FROM (
SELECT max(id) AS id
FROM #{quote_table_name(:routes)}
WHERE path = #{path}
) max_ids
)
})
end
end
def down
end
end
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AddIndexToRoutes < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
# Set this constant to true if this migration requires downtime.
DOWNTIME = false
disable_ddl_transaction!
def change
add_concurrent_index(:routes, :path, unique: true)
add_concurrent_index(:routes, [:source_type, :source_id], unique: true)
end
end
......@@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20161128161412) do
ActiveRecord::Schema.define(version: 20161202152035) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
......@@ -98,14 +98,14 @@ ActiveRecord::Schema.define(version: 20161128161412) do
t.text "help_page_text_html"
t.text "shared_runners_text_html"
t.text "after_sign_up_text_html"
t.boolean "sidekiq_throttling_enabled", default: false
t.string "sidekiq_throttling_queues"
t.decimal "sidekiq_throttling_factor"
t.boolean "housekeeping_enabled", default: true, null: false
t.boolean "housekeeping_bitmaps_enabled", default: true, null: false
t.integer "housekeeping_incremental_repack_period", default: 10, null: false
t.integer "housekeeping_full_repack_period", default: 50, null: false
t.integer "housekeeping_gc_period", default: 200, null: false
t.boolean "sidekiq_throttling_enabled", default: false
t.string "sidekiq_throttling_queues"
t.decimal "sidekiq_throttling_factor"
t.boolean "html_emails_enabled", default: true
end
......@@ -737,8 +737,9 @@ ActiveRecord::Schema.define(version: 20161128161412) do
t.integer "visibility_level", default: 20, null: false
t.boolean "request_access_enabled", default: false, null: false
t.datetime "deleted_at"
t.boolean "lfs_enabled"
t.text "description_html"
t.boolean "lfs_enabled"
t.integer "parent_id"
end
add_index "namespaces", ["created_at"], name: "index_namespaces_on_created_at", using: :btree
......@@ -746,6 +747,7 @@ ActiveRecord::Schema.define(version: 20161128161412) do
add_index "namespaces", ["name"], name: "index_namespaces_on_name", unique: true, using: :btree
add_index "namespaces", ["name"], name: "index_namespaces_on_name_trigram", using: :gin, opclasses: {"name"=>"gin_trgm_ops"}
add_index "namespaces", ["owner_id"], name: "index_namespaces_on_owner_id", using: :btree
add_index "namespaces", ["parent_id", "id"], name: "index_namespaces_on_parent_id_and_id", unique: true, using: :btree
add_index "namespaces", ["path"], name: "index_namespaces_on_path", unique: true, using: :btree
add_index "namespaces", ["path"], name: "index_namespaces_on_path_trigram", using: :gin, opclasses: {"path"=>"gin_trgm_ops"}
add_index "namespaces", ["type"], name: "index_namespaces_on_type", using: :btree
......@@ -991,6 +993,17 @@ ActiveRecord::Schema.define(version: 20161128161412) do
add_index "releases", ["project_id", "tag"], name: "index_releases_on_project_id_and_tag", using: :btree
add_index "releases", ["project_id"], name: "index_releases_on_project_id", using: :btree
create_table "routes", force: :cascade do |t|
t.integer "source_id", null: false
t.string "source_type", null: false
t.string "path", null: false
t.datetime "created_at"
t.datetime "updated_at"
end
add_index "routes", ["path"], name: "index_routes_on_path", unique: true, using: :btree
add_index "routes", ["source_type", "source_id"], name: "index_routes_on_source_type_and_source_id", unique: true, using: :btree
create_table "sent_notifications", force: :cascade do |t|
t.integer "project_id"
t.integer "noteable_id"
......@@ -1206,8 +1219,8 @@ ActiveRecord::Schema.define(version: 20161128161412) do
t.datetime "otp_grace_period_started_at"
t.boolean "ldap_email", default: false, null: false
t.boolean "external", default: false
t.string "organization"
t.string "incoming_email_token"
t.string "organization"
t.boolean "authorized_projects_populated"
end
......
......@@ -111,7 +111,7 @@ module API
if id =~ /^\d+$/
Group.find_by(id: id)
else
Group.find_by(path: id)
Group.find_by_full_path(id)
end
end
......
......@@ -4,7 +4,7 @@ class GroupUrlConstrainer
return false unless valid?(id)
Group.find_by(path: id).present?
Group.find_by_full_path(id).present?
end
private
......
......@@ -10,6 +10,13 @@ describe GroupUrlConstrainer, lib: true do
it { expect(subject.matches?(request)).to be_truthy }
end
context 'valid request for nested group' do
let!(:nested_group) { create(:group, path: 'nested', parent: group) }
let!(:request) { build_request('gitlab/nested') }
it { expect(subject.matches?(request)).to be_truthy }
end
context 'invalid request' do
let(:request) { build_request('foo') }
......
......@@ -188,6 +188,7 @@ project:
- project_feature
- authorized_users
- project_authorizations
- route
award_emoji:
- awardable
- user
......
require 'spec_helper'
describe Group, 'Routable' do
let!(:group) { create(:group) }
describe 'Associations' do
it { is_expected.to have_one(:route).dependent(:destroy) }
end
describe 'Callbacks' do
it 'creates route record on create' do
expect(group.route.path).to eq(group.path)
end
it 'updates route record on path change' do
group.update_attributes(path: 'wow')
expect(group.route.path).to eq('wow')
end
it 'ensure route path uniqueness across different objects' do
create(:group, parent: group, path: 'xyz')
duplicate = build(:project, namespace: group, path: 'xyz')
expect { duplicate.save! }.to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: Route path has already been taken, Route is invalid')
end
end
describe '.find_by_full_path' do
let!(:nested_group) { create(:group, parent: group) }
it { expect(described_class.find_by_full_path(group.to_param)).to eq(group) }
it { expect(described_class.find_by_full_path(group.to_param.upcase)).to eq(group) }
it { expect(described_class.find_by_full_path(nested_group.to_param)).to eq(nested_group) }
it { expect(described_class.find_by_full_path('unknown')).to eq(nil) }
end
describe '.where_paths_in' do
context 'without any paths' do
it 'returns an empty relation' do
expect(described_class.where_paths_in([])).to eq([])
end
end
context 'without any valid paths' do
it 'returns an empty relation' do
expect(described_class.where_paths_in(%w[unknown])).to eq([])
end
end
context 'with valid paths' do
let!(:nested_group) { create(:group, parent: group) }
it 'returns the projects matching the paths' do
result = described_class.where_paths_in([group.to_param, nested_group.to_param])
expect(result).to contain_exactly(group, nested_group)
end
it 'returns projects regardless of the casing of paths' do
result = described_class.where_paths_in([group.to_param.upcase, nested_group.to_param.upcase])
expect(result).to contain_exactly(group, nested_group)
end
end
end
end
......@@ -124,4 +124,12 @@ describe Namespace, models: true do
expect(Namespace.clean_path("--%+--valid_*&%name=.git.%.atom.atom.@email.com")).to eq("valid_name")
end
end
describe '#full_path' do
let(:group) { create(:group) }
let(:nested_group) { create(:group, parent: group) }
it { expect(group.full_path).to eq(group.path) }
it { expect(nested_group.full_path).to eq("#{group.path}/#{nested_group.path}") }
end
end
......@@ -478,35 +478,6 @@ describe Project, models: true do
end
end
describe '.find_with_namespace' do
context 'with namespace' do
before do
@group = create :group, name: 'gitlab'
@project = create(:project, name: 'gitlabhq', namespace: @group)
end
it { expect(Project.find_with_namespace('gitlab/gitlabhq')).to eq(@project) }
it { expect(Project.find_with_namespace('GitLab/GitlabHQ')).to eq(@project) }
it { expect(Project.find_with_namespace('gitlab-ci')).to be_nil }
end
context 'when multiple projects using a similar name exist' do
let(:group) { create(:group, name: 'gitlab') }
let!(:project1) do
create(:empty_project, name: 'gitlab1', path: 'gitlab', namespace: group)
end
let!(:project2) do
create(:empty_project, name: 'gitlab2', path: 'GITLAB', namespace: group)
end
it 'returns the row where the path matches literally' do
expect(Project.find_with_namespace('gitlab/GITLAB')).to eq(project2)
end
end
end
describe '#to_param' do
context 'with namespace' do
before do
......@@ -1548,39 +1519,6 @@ describe Project, models: true do
end
end
describe '.where_paths_in' do
context 'without any paths' do
it 'returns an empty relation' do
expect(Project.where_paths_in([])).to eq([])
end
end
context 'without any valid paths' do
it 'returns an empty relation' do
expect(Project.where_paths_in(%w[foo])).to eq([])
end
end
context 'with valid paths' do
let!(:project1) { create(:project) }
let!(:project2) { create(:project) }
it 'returns the projects matching the paths' do
projects = Project.where_paths_in([project1.path_with_namespace,
project2.path_with_namespace])
expect(projects).to contain_exactly(project1, project2)
end
it 'returns projects regardless of the casing of paths' do
projects = Project.where_paths_in([project1.path_with_namespace.upcase,
project2.path_with_namespace.upcase])
expect(projects).to contain_exactly(project1, project2)
end
end
end
describe 'change_head' do
let(:project) { create(:project) }
......
require 'spec_helper'
describe Route, models: true do
let!(:group) { create(:group) }
let!(:route) { group.route }
describe 'relationships' do
it { is_expected.to belong_to(:source) }
end
describe 'validations' do
it { is_expected.to validate_presence_of(:source) }
it { is_expected.to validate_presence_of(:path) }
it { is_expected.to validate_uniqueness_of(:path) }
end
describe '#rename_children' do
let!(:nested_group) { create(:group, path: "test", parent: group) }
let!(:deep_nested_group) { create(:group, path: "foo", parent: nested_group) }
it "updates children routes with new path" do
route.update_attributes(path: 'bar')
expect(described_class.exists?(path: 'bar')).to be_truthy
expect(described_class.exists?(path: 'bar/test')).to be_truthy
expect(described_class.exists?(path: 'bar/test/foo')).to be_truthy
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment