Skip to content
GitLab
Menu
Projects
Groups
Snippets
/
Help
Help
Support
Community forum
Keyboard shortcuts
?
Submit feedback
Contribute to GitLab
Sign in / Register
Toggle navigation
Menu
Open sidebar
heptapod
heptapod
Commits
744ac38e166e
Commit
744ac38e
authored
Jan 31, 2018
by
Douwe Maan
Browse files
Validate path uniqueness only on Route, and bubble up appropriately
parent
56e950071da4
Changes
12
Hide whitespace changes
Inline
Side-by-side
app/models/concerns/routable.rb
View file @
744ac38e
...
...
@@ -7,8 +7,7 @@
has_one
:route
,
as: :source
,
autosave:
true
,
dependent: :destroy
# rubocop:disable Cop/ActiveRecordDependent
has_many
:redirect_routes
,
as: :source
,
autosave:
true
,
dependent: :destroy
# rubocop:disable Cop/ActiveRecordDependent
validates_associated
:route
validates
:route
,
presence:
true
scope
:with_route
,
->
{
includes
(
:route
)
}
...
...
@@ -11,7 +10,9 @@
validates
:route
,
presence:
true
scope
:with_route
,
->
{
includes
(
:route
)
}
after_validation
:set_path_errors
before_validation
do
if
full_path_changed?
||
full_name_changed?
prepare_route
...
...
@@ -125,6 +126,11 @@
private
def
set_path_errors
route_path_errors
=
self
.
errors
.
delete
(
:"route.path"
)
self
.
errors
[
:path
].
concat
(
route_path_errors
)
if
route_path_errors
end
def
uncached_full_path
if
route
&&
route
.
path
.
present?
@full_path
||=
route
.
path
# rubocop:disable Gitlab/ModuleWithInstanceVariables
...
...
app/models/namespace.rb
View file @
744ac38e
...
...
@@ -32,7 +32,6 @@
validates
:owner
,
presence:
true
,
unless:
->
(
n
)
{
n
.
type
==
"Group"
}
validates
:name
,
presence:
true
,
uniqueness:
{
scope: :parent_id
},
length:
{
maximum:
255
},
namespace_name:
true
...
...
app/models/project.rb
View file @
744ac38e
...
...
@@ -245,8 +245,7 @@
validates
:path
,
presence:
true
,
project_path:
true
,
length:
{
maximum:
255
},
uniqueness:
{
scope: :namespace_id
}
length:
{
maximum:
255
}
validates
:namespace
,
presence:
true
validates
:name
,
uniqueness:
{
scope: :namespace_id
}
...
...
app/models/route.rb
View file @
744ac38e
...
...
@@ -75,7 +75,7 @@
def
ensure_permanent_paths
return
if
path
.
nil?
errors
.
add
(
:path
,
"
#{
path
}
has been taken before
. Please use another one
"
)
if
conflicting_redirect_exists?
errors
.
add
(
:path
,
"has been taken before"
)
if
conflicting_redirect_exists?
end
def
conflicting_redirect_exists?
...
...
app/models/user.rb
View file @
744ac38e
...
...
@@ -153,6 +153,5 @@
numericality:
{
greater_than_or_equal_to:
0
,
less_than_or_equal_to:
Gitlab
::
Database
::
MAX_INT_VALUE
}
validates
:username
,
user_path:
true
,
presence:
true
,
uniqueness:
{
case_sensitive:
false
}
presence:
true
...
...
@@ -158,5 +157,5 @@
validate
:namespace
_uniq
,
if: :username_changed?
validate
s
:namespace
,
presence:
true
validate
:namespace_move_dir_allowed
,
if: :username_changed?
validate
:unique_email
,
if: :email_changed?
...
...
@@ -172,6 +171,7 @@
before_save
:skip_reconfirmation!
,
if:
->
(
user
)
{
user
.
email_changed?
&&
user
.
read_only_attribute?
(
:email
)
}
before_save
:check_for_verified_email
,
if:
->
(
user
)
{
user
.
email_changed?
&&
!
user
.
new_record?
}
before_validation
:ensure_namespace_correct
after_validation
:set_username_errors
after_update
:username_changed_hook
,
if: :username_changed?
after_destroy
:post_destroy_hook
after_destroy
:remove_key_cache
...
...
@@ -505,17 +505,6 @@
end
end
def
namespace_uniq
# Return early if username already failed the first uniqueness validation
return
if
errors
.
key?
(
:username
)
&&
errors
[
:username
].
include?
(
'has already been taken'
)
existing_namespace
=
Namespace
.
by_path
(
username
)
if
existing_namespace
&&
existing_namespace
!=
namespace
errors
.
add
(
:username
,
'has already been taken'
)
end
end
def
namespace_move_dir_allowed
if
namespace
&
.
any_project_has_container_registry_tags?
errors
.
add
(
:username
,
'cannot be changed if a personal project has container registry tags.'
)
...
...
@@ -891,6 +880,11 @@
end
end
def
set_username_errors
namespace_path_errors
=
self
.
errors
.
delete
(
:"namespace.path"
)
self
.
errors
[
:username
].
concat
(
namespace_path_errors
)
if
namespace_path_errors
end
def
username_changed_hook
system_hook_service
.
execute_hooks_for
(
self
,
:rename
)
end
...
...
spec/models/concerns/routable_spec.rb
View file @
744ac38e
...
...
@@ -39,7 +39,7 @@
create
(
:group
,
parent:
group
,
path:
'xyz'
)
duplicate
=
build
(
:project
,
namespace:
group
,
path:
'xyz'
)
expect
{
duplicate
.
save!
}.
to
raise_error
(
ActiveRecord
::
RecordInvalid
,
'Validation failed:
Route p
ath has already been taken
, Route is invalid
'
)
expect
{
duplicate
.
save!
}.
to
raise_error
(
ActiveRecord
::
RecordInvalid
,
'Validation failed:
P
ath has already been taken'
)
end
end
...
...
spec/models/group_spec.rb
View file @
744ac38e
...
...
@@ -41,7 +41,6 @@
describe
'validations'
do
it
{
is_expected
.
to
validate_presence_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
.
not_to
validate_presence_of
:owner
}
it
{
is_expected
.
to
validate_presence_of
:two_factor_grace_period
}
...
...
spec/models/namespace_spec.rb
View file @
744ac38e
...
...
@@ -15,7 +15,6 @@
describe
'validations'
do
it
{
is_expected
.
to
validate_presence_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
(
:description
).
is_at_most
(
255
)
}
it
{
is_expected
.
to
validate_presence_of
(
:path
)
}
...
...
spec/models/project_spec.rb
View file @
744ac38e
...
...
@@ -129,7 +129,6 @@
it
{
is_expected
.
to
validate_length_of
(
:name
).
is_at_most
(
255
)
}
it
{
is_expected
.
to
validate_presence_of
(
:path
)
}
it
{
is_expected
.
to
validate_uniqueness_of
(
:path
).
scoped_to
(
:namespace_id
)
}
it
{
is_expected
.
to
validate_length_of
(
:path
).
is_at_most
(
255
)
}
it
{
is_expected
.
to
validate_length_of
(
:description
).
is_at_most
(
2000
)
}
...
...
spec/models/route_spec.rb
View file @
744ac38e
...
...
@@ -27,7 +27,7 @@
redirect
.
save!
(
validate:
false
)
expect
(
new_route
.
valid?
).
to
be_falsey
expect
(
new_route
.
errors
.
first
[
1
]).
to
eq
(
'
foo
has been taken before
. Please use another one
'
)
expect
(
new_route
.
errors
.
first
[
1
]).
to
eq
(
'has been taken before'
)
end
end
...
...
@@ -49,7 +49,7 @@
redirect
.
save!
(
validate:
false
)
expect
(
route
.
valid?
).
to
be_falsey
expect
(
route
.
errors
.
first
[
1
]).
to
eq
(
'
foo
has been taken before
. Please use another one
'
)
expect
(
route
.
errors
.
first
[
1
]).
to
eq
(
'has been taken before'
)
end
end
...
...
@@ -368,7 +368,7 @@
group2
.
path
=
'foo'
group2
.
valid?
expect
(
group2
.
errors
[
"route.path"
].
first
).
to
eq
(
'
foo
has been taken before
. Please use another one
'
)
expect
(
group2
.
errors
[
:path
]
).
to
eq
(
[
'has been taken before'
]
)
end
end
...
...
spec/models/user_spec.rb
View file @
744ac38e
...
...
@@ -116,12 +116,6 @@
expect
(
user
).
to
be_valid
end
it
'validates uniqueness'
do
user
=
build
(
:user
)
expect
(
user
).
to
validate_uniqueness_of
(
:username
).
case_insensitive
end
context
'when username is changed'
do
let
(
:user
)
{
build_stubbed
(
:user
,
username:
'old_path'
,
namespace:
build_stubbed
(
:namespace
))
}
...
...
@@ -2287,7 +2281,7 @@
end
context
'when there is a validation error (namespace name taken) while updating namespace'
do
let!
(
:conflicting_namespace
)
{
create
(
:group
,
name
:
new_username
,
path:
'quz'
)
}
let!
(
:conflicting_namespace
)
{
create
(
:group
,
path
:
new_username
)
}
it
'causes the user save to fail'
do
expect
(
user
.
update_attributes
(
username:
new_username
)).
to
be_falsey
...
...
@@ -2291,9 +2285,9 @@
it
'causes the user save to fail'
do
expect
(
user
.
update_attributes
(
username:
new_username
)).
to
be_falsey
expect
(
user
.
namespace
.
errors
.
messages
[
:
name
].
first
).
to
eq
(
'has already been taken'
)
expect
(
user
.
namespace
.
errors
.
messages
[
:
path
].
first
).
to
eq
(
'has already been taken'
)
end
it
'adds the namespace errors to the user'
do
user
.
update_attributes
(
username:
new_username
)
...
...
@@ -2295,9 +2289,9 @@
end
it
'adds the namespace errors to the user'
do
user
.
update_attributes
(
username:
new_username
)
expect
(
user
.
errors
.
full_messages
.
first
).
to
eq
(
'
Namespace
name has already been taken'
)
expect
(
user
.
errors
.
full_messages
.
first
).
to
eq
(
'
User
name has already been taken'
)
end
end
end
...
...
spec/services/users/update_service_spec.rb
View file @
744ac38e
...
...
@@ -21,9 +21,9 @@
end
it
'includes namespace error messages'
do
create
(
:group
,
name:
'taken'
,
path:
'something_else
'
)
create
(
:group
,
path:
'taken
'
)
result
=
{}
expect
do
result
=
update_user
(
user
,
{
username:
'taken'
})
end
.
not_to
change
{
user
.
reload
.
username
}
expect
(
result
[
:status
]).
to
eq
(
:error
)
...
...
@@ -25,9 +25,9 @@
result
=
{}
expect
do
result
=
update_user
(
user
,
{
username:
'taken'
})
end
.
not_to
change
{
user
.
reload
.
username
}
expect
(
result
[
:status
]).
to
eq
(
:error
)
expect
(
result
[
:message
]).
to
eq
(
'
Namespace
name has already been taken'
)
expect
(
result
[
:message
]).
to
eq
(
'
User
name has already been taken'
)
end
def
update_user
(
user
,
opts
)
...
...
Write
Preview
Supports
Markdown
0%
Try again
or
attach a new file
.
Attach a file
Cancel
You are about to add
0
people
to the discussion. Proceed with caution.
Finish editing this message first!
Cancel
Please
register
or
sign in
to comment