Commit abad9a9b authored by Dmitriy Zaporozhets's avatar Dmitriy Zaporozhets

Merge branch 'dz-remove-namespaces-path-uniq' into 'master'

Modify namespace name and path validation

## What does this MR do?

* Allow same namespace name with different parent_id
* Allow same namespace path. Uniq validation should be handled by routes table

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

migrations

## Why was this MR needed?

So we can use same name in different nesting like: 

```
gitlab-ce/foo
gitlab-com/foo
foo/gitlab-ce
```

## Screenshots (if relevant)

no

## Does this MR meet the acceptance criteria?

- ~~[Changelog entry](https://docs.gitlab.com/ce/development/changelog.html) 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 !8013
parents b70b9628 4bdfb039
...@@ -17,14 +17,13 @@ class Namespace < ActiveRecord::Base ...@@ -17,14 +17,13 @@ class Namespace < ActiveRecord::Base
validates :owner, presence: true, unless: ->(n) { n.type == "Group" } validates :owner, presence: true, unless: ->(n) { n.type == "Group" }
validates :name, validates :name,
presence: true, presence: true,
uniqueness: true, uniqueness: { scope: :parent_id },
length: { maximum: 255 }, length: { maximum: 255 },
namespace_name: true namespace_name: true
validates :description, length: { maximum: 255 } validates :description, length: { maximum: 255 }
validates :path, validates :path,
presence: true, presence: true,
uniqueness: { case_sensitive: false },
length: { maximum: 255 }, length: { maximum: 255 },
namespace: true namespace: true
......
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class RemoveUniqPathIndexFromNamespace < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
disable_ddl_transaction!
DOWNTIME = false
def up
constraint_name = 'namespaces_path_key'
transaction do
if index_exists?(:namespaces, :path)
remove_index(:namespaces, :path)
end
# In some bizarre cases PostgreSQL might have a separate unique constraint
# that we'll need to drop.
if constraint_exists?(constraint_name) && Gitlab::Database.postgresql?
execute("ALTER TABLE namespaces DROP CONSTRAINT IF EXISTS #{constraint_name};")
end
end
end
def down
unless index_exists?(:namespaces, :path)
add_concurrent_index(:namespaces, :path, unique: true)
end
end
def constraint_exists?(name)
indexes(:namespaces).map(&:name).include?(name)
end
end
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AddPathIndexToNamespace < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
disable_ddl_transaction!
DOWNTIME = false
def up
add_concurrent_index :namespaces, :path
end
def down
if index_exists?(:namespaces, :path)
remove_index :namespaces, :path
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 RemoveUniqNameIndexFromNamespace < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
disable_ddl_transaction!
DOWNTIME = false
def up
constraint_name = 'namespaces_name_key'
transaction do
if index_exists?(:namespaces, :name)
remove_index(:namespaces, :name)
end
# In some bizarre cases PostgreSQL might have a separate unique constraint
# that we'll need to drop.
if constraint_exists?(constraint_name) && Gitlab::Database.postgresql?
execute("ALTER TABLE namespaces DROP CONSTRAINT IF EXISTS #{constraint_name};")
end
end
end
def down
unless index_exists?(:namespaces, :name)
add_concurrent_index(:namespaces, :name, unique: true)
end
end
def constraint_exists?(name)
indexes(:namespaces).map(&:name).include?(name)
end
end
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AddNameIndexToNamespace < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
disable_ddl_transaction!
DOWNTIME = false
def up
add_concurrent_index(:namespaces, [:name, :parent_id], unique: true)
end
def down
if index_exists?(:namespaces, :name)
remove_index :namespaces, [:name, :parent_id]
end
end
end
...@@ -744,11 +744,11 @@ ActiveRecord::Schema.define(version: 20161212142807) do ...@@ -744,11 +744,11 @@ ActiveRecord::Schema.define(version: 20161212142807) do
add_index "namespaces", ["created_at"], name: "index_namespaces_on_created_at", using: :btree add_index "namespaces", ["created_at"], name: "index_namespaces_on_created_at", using: :btree
add_index "namespaces", ["deleted_at"], name: "index_namespaces_on_deleted_at", using: :btree add_index "namespaces", ["deleted_at"], name: "index_namespaces_on_deleted_at", using: :btree
add_index "namespaces", ["name"], name: "index_namespaces_on_name", unique: true, using: :btree add_index "namespaces", ["name", "parent_id"], name: "index_namespaces_on_name_and_parent_id", unique: true, using: :btree
add_index "namespaces", ["name"], name: "index_namespaces_on_name_trigram", using: :gin, opclasses: {"name"=>"gin_trgm_ops"} 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", ["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", ["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", using: :btree
add_index "namespaces", ["path"], name: "index_namespaces_on_path_trigram", using: :gin, opclasses: {"path"=>"gin_trgm_ops"} 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 add_index "namespaces", ["type"], name: "index_namespaces_on_type", using: :btree
......
...@@ -50,9 +50,8 @@ describe Group, models: true do ...@@ -50,9 +50,8 @@ describe Group, models: true do
describe 'validations' do describe 'validations' do
it { is_expected.to validate_presence_of :name } it { is_expected.to validate_presence_of :name }
it { is_expected.to validate_uniqueness_of(:name) } it { is_expected.to validate_uniqueness_of(:name).scoped_to(:parent_id) }
it { is_expected.to validate_presence_of :path } it { is_expected.to validate_presence_of :path }
it { is_expected.to validate_uniqueness_of(:path) }
it { is_expected.not_to validate_presence_of :owner } it { is_expected.not_to validate_presence_of :owner }
end end
......
...@@ -6,20 +6,16 @@ describe Namespace, models: true do ...@@ -6,20 +6,16 @@ describe Namespace, models: true do
it { is_expected.to have_many :projects } it { is_expected.to have_many :projects }
it { is_expected.to validate_presence_of(:name) } it { is_expected.to validate_presence_of(:name) }
it { is_expected.to validate_uniqueness_of(:name) } it { is_expected.to validate_uniqueness_of(:name).scoped_to(:parent_id) }
it { is_expected.to validate_length_of(:name).is_at_most(255) } it { is_expected.to validate_length_of(:name).is_at_most(255) }
it { is_expected.to validate_length_of(:description).is_at_most(255) } it { is_expected.to validate_length_of(:description).is_at_most(255) }
it { is_expected.to validate_presence_of(:path) } it { is_expected.to validate_presence_of(:path) }
it { is_expected.to validate_uniqueness_of(:path) }
it { is_expected.to validate_length_of(:path).is_at_most(255) } it { is_expected.to validate_length_of(:path).is_at_most(255) }
it { is_expected.to validate_presence_of(:owner) } it { is_expected.to validate_presence_of(:owner) }
describe "Mass assignment" do
end
describe "Respond to" do describe "Respond to" do
it { is_expected.to respond_to(:human_name) } it { is_expected.to respond_to(:human_name) }
it { is_expected.to respond_to(:to_param) } it { is_expected.to respond_to(:to_param) }
......
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