Unverified Commit 4e249d5b authored by Rémy Coutable's avatar Rémy Coutable
Browse files

Use :maximum instead of :within for length validators with a 0..N range


Signed-off-by: default avatarRémy Coutable <remy@rymai.me>
parent 1dd886de60e1
...@@ -4,10 +4,10 @@ class Variable < ActiveRecord::Base ...@@ -4,10 +4,10 @@ class Variable < ActiveRecord::Base
belongs_to :project, foreign_key: :gl_project_id belongs_to :project, foreign_key: :gl_project_id
validates_uniqueness_of :key, scope: :gl_project_id
validates :key, validates :key,
presence: true, presence: true,
length: { within: 0..255 }, uniqueness: { scope: :gl_project_id },
length: { maximum: 255 },
format: { with: /\A[a-zA-Z0-9_]+\z/, format: { with: /\A[a-zA-Z0-9_]+\z/,
message: "can contain only letters, digits and '_'." } message: "can contain only letters, digits and '_'." }
......
...@@ -41,7 +41,7 @@ def award_emojis_loaded? ...@@ -41,7 +41,7 @@ def award_emojis_loaded?
has_one :metrics has_one :metrics
validates :author, presence: true validates :author, presence: true
validates :title, presence: true, length: { within: 0..255 } validates :title, presence: true, length: { maximum: 255 }
scope :authored, ->(user) { where(author_id: user) } scope :authored, ->(user) { where(author_id: user) }
scope :assigned_to, ->(u) { where(assignee_id: u.id)} scope :assigned_to, ->(u) { where(assignee_id: u.id)}
......
...@@ -9,7 +9,7 @@ class Environment < ActiveRecord::Base ...@@ -9,7 +9,7 @@ class Environment < ActiveRecord::Base
validates :name, validates :name,
presence: true, presence: true,
uniqueness: { scope: :project_id }, uniqueness: { scope: :project_id },
length: { within: 0..255 }, length: { maximum: 255 },
format: { with: Gitlab::Regex.environment_name_regex, format: { with: Gitlab::Regex.environment_name_regex,
message: Gitlab::Regex.environment_name_regex_message } message: Gitlab::Regex.environment_name_regex_message }
......
...@@ -8,10 +8,18 @@ class Key < ActiveRecord::Base ...@@ -8,10 +8,18 @@ class Key < ActiveRecord::Base
before_validation :generate_fingerprint before_validation :generate_fingerprint
validates :title, presence: true, length: { within: 0..255 } validates :title,
validates :key, presence: true, length: { within: 0..5000 }, format: { with: /\A(ssh|ecdsa)-.*\Z/ } presence: true,
validates :key, format: { without: /\n|\r/, message: 'should be a single line' } length: { maximum: 255 }
validates :fingerprint, uniqueness: true, presence: { message: 'cannot be generated' } validates :key,
presence: true,
length: { maximum: 5000 },
format: { with: /\A(ssh|ecdsa)-.*\Z/ }
validates :key,
format: { without: /\n|\r/, message: 'should be a single line' }
validates :fingerprint,
uniqueness: true,
presence: { message: 'cannot be generated' }
delegate :name, :email, to: :user, prefix: true delegate :name, :email, to: :user, prefix: true
......
...@@ -12,17 +12,17 @@ class Namespace < ActiveRecord::Base ...@@ -12,17 +12,17 @@ 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,
length: { within: 0..255 },
namespace_name: true,
presence: true, presence: true,
uniqueness: true uniqueness: true,
length: { maximum: 255 },
namespace_name: true
validates :description, length: { within: 0..255 } validates :description, length: { maximum: 255 }
validates :path, validates :path,
length: { within: 1..255 },
namespace: true,
presence: true, presence: true,
uniqueness: { case_sensitive: false } uniqueness: { case_sensitive: false },
length: { maximum: 255 },
namespace: true
delegate :name, to: :owner, allow_nil: true, prefix: true delegate :name, to: :owner, allow_nil: true, prefix: true
......
...@@ -172,13 +172,13 @@ def update_forks_visibility_level ...@@ -172,13 +172,13 @@ def update_forks_visibility_level
validates :description, length: { maximum: 2000 }, allow_blank: true validates :description, length: { maximum: 2000 }, allow_blank: true
validates :name, validates :name,
presence: true, presence: true,
length: { within: 0..255 }, length: { maximum: 255 },
format: { with: Gitlab::Regex.project_name_regex, format: { with: Gitlab::Regex.project_name_regex,
message: Gitlab::Regex.project_name_regex_message } message: Gitlab::Regex.project_name_regex_message }
validates :path, validates :path,
presence: true, presence: true,
project_path: true, project_path: true,
length: { within: 0..255 }, length: { maximum: 255 },
format: { with: Gitlab::Regex.project_path_regex, format: { with: Gitlab::Regex.project_path_regex,
message: Gitlab::Regex.project_path_regex_message } message: Gitlab::Regex.project_path_regex_message }
validates :namespace, presence: true validates :namespace, presence: true
......
...@@ -27,9 +27,9 @@ def content_html_invalidated? ...@@ -27,9 +27,9 @@ def content_html_invalidated?
delegate :name, :email, to: :author, prefix: true, allow_nil: true delegate :name, :email, to: :author, prefix: true, allow_nil: true
validates :author, presence: true validates :author, presence: true
validates :title, presence: true, length: { within: 0..255 } validates :title, presence: true, length: { maximum: 255 }
validates :file_name, validates :file_name,
length: { within: 0..255 }, length: { maximum: 255 },
format: { with: Gitlab::Regex.file_name_regex, format: { with: Gitlab::Regex.file_name_regex,
message: Gitlab::Regex.file_name_regex_message } message: Gitlab::Regex.file_name_regex_message }
...@@ -94,6 +94,10 @@ def size ...@@ -94,6 +94,10 @@ def size
0 0
end end
def file_name
super.to_s
end
# alias for compatibility with blobs and highlighting # alias for compatibility with blobs and highlighting
def path def path
file_name file_name
......
...@@ -155,7 +155,7 @@ ...@@ -155,7 +155,7 @@
message: 'The remote data could not be fully imported.', message: 'The remote data could not be fully imported.',
errors: [ errors: [
{ type: :label, url: "https://api.github.com/repos/octocat/Hello-World/labels/bug", errors: "Validation failed: Title can't be blank, Title is invalid" }, { type: :label, url: "https://api.github.com/repos/octocat/Hello-World/labels/bug", errors: "Validation failed: Title can't be blank, Title is invalid" },
{ type: :issue, url: "https://api.github.com/repos/octocat/Hello-World/issues/1348", errors: "Validation failed: Title can't be blank, Title is too short (minimum is 0 characters)" }, { type: :issue, url: "https://api.github.com/repos/octocat/Hello-World/issues/1348", errors: "Validation failed: Title can't be blank" },
{ type: :wiki, errors: "Gitlab::Shell::Error" }, { type: :wiki, errors: "Gitlab::Shell::Error" },
{ type: :release, url: 'https://api.github.com/repos/octocat/Hello-World/releases/2', errors: "Validation failed: Description can't be blank" } { type: :release, url: 'https://api.github.com/repos/octocat/Hello-World/releases/2', errors: "Validation failed: Description can't be blank" }
] ]
......
...@@ -5,6 +5,13 @@ ...@@ -5,6 +5,13 @@
let(:secret_value) { 'secret' } let(:secret_value) { 'secret' }
it { is_expected.to validate_presence_of(:key) }
it { is_expected.to validate_uniqueness_of(:key).scoped_to(:gl_project_id) }
it { is_expected.to validate_length_of(:key).is_at_most(255) }
it { is_expected.to allow_value('foo').for(:key) }
it { is_expected.not_to allow_value('foo bar').for(:key) }
it { is_expected.not_to allow_value('foo/bar').for(:key) }
before :each do before :each do
subject.value = secret_value subject.value = secret_value
end end
......
...@@ -35,7 +35,7 @@ ...@@ -35,7 +35,7 @@
it { is_expected.to validate_presence_of(:iid) } it { is_expected.to validate_presence_of(:iid) }
it { is_expected.to validate_presence_of(:author) } it { is_expected.to validate_presence_of(:author) }
it { is_expected.to validate_presence_of(:title) } it { is_expected.to validate_presence_of(:title) }
it { is_expected.to validate_length_of(:title).is_at_least(0).is_at_most(255) } it { is_expected.to validate_length_of(:title).is_at_most(255) }
end end
describe "Scope" do describe "Scope" do
......
...@@ -13,9 +13,9 @@ ...@@ -13,9 +13,9 @@
it { is_expected.to validate_presence_of(:name) } it { is_expected.to validate_presence_of(:name) }
it { is_expected.to validate_uniqueness_of(:name).scoped_to(:project_id) } it { is_expected.to validate_uniqueness_of(:name).scoped_to(:project_id) }
it { is_expected.to validate_length_of(:name).is_within(0..255) } it { is_expected.to validate_length_of(:name).is_at_most(255) }
it { is_expected.to validate_length_of(:external_url).is_within(0..255) } it { is_expected.to validate_length_of(:external_url).is_at_most(255) }
# To circumvent a not null violation of the name column: # To circumvent a not null violation of the name column:
# https://github.com/thoughtbot/shoulda-matchers/issues/336 # https://github.com/thoughtbot/shoulda-matchers/issues/336
......
...@@ -7,9 +7,13 @@ ...@@ -7,9 +7,13 @@
describe "Validation" do describe "Validation" do
it { is_expected.to validate_presence_of(:title) } it { is_expected.to validate_presence_of(:title) }
it { is_expected.to validate_length_of(:title).is_at_most(255) }
it { is_expected.to validate_presence_of(:key) } it { is_expected.to validate_presence_of(:key) }
it { is_expected.to validate_length_of(:title).is_within(0..255) } it { is_expected.to validate_length_of(:key).is_at_most(5000) }
it { is_expected.to validate_length_of(:key).is_within(0..5000) } it { is_expected.to allow_value('ssh-foo').for(:key) }
it { is_expected.to allow_value('ecdsa-foo').for(:key) }
it { is_expected.not_to allow_value('foo-bar').for(:key) }
end end
describe "Methods" do describe "Methods" do
......
...@@ -4,11 +4,18 @@ ...@@ -4,11 +4,18 @@
let!(:namespace) { create(:namespace) } let!(:namespace) { create(:namespace) }
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) }
it { is_expected.to validate_presence_of :path } 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) }
it { is_expected.to validate_uniqueness_of(:path) } it { is_expected.to validate_uniqueness_of(:path) }
it { is_expected.to validate_presence_of :owner } it { is_expected.to validate_length_of(:path).is_at_most(255) }
it { is_expected.to validate_presence_of(:owner) }
describe "Mass assignment" do describe "Mass assignment" do
end end
......
...@@ -131,14 +131,18 @@ ...@@ -131,14 +131,18 @@
it { is_expected.to validate_presence_of(:name) } it { is_expected.to validate_presence_of(:name) }
it { is_expected.to validate_uniqueness_of(:name).scoped_to(:namespace_id) } it { is_expected.to validate_uniqueness_of(:name).scoped_to(:namespace_id) }
it { is_expected.to validate_length_of(:name).is_within(0..255) } 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_presence_of(:path) }
it { is_expected.to validate_uniqueness_of(:path).scoped_to(:namespace_id) } it { is_expected.to validate_uniqueness_of(:path).scoped_to(:namespace_id) }
it { is_expected.to validate_length_of(:path).is_within(0..255) } it { is_expected.to validate_length_of(:path).is_at_most(255) }
it { is_expected.to validate_length_of(:description).is_within(0..2000) }
it { is_expected.to validate_length_of(:description).is_at_most(2000) }
it { is_expected.to validate_presence_of(:creator) } it { is_expected.to validate_presence_of(:creator) }
it { is_expected.to validate_presence_of(:namespace) } it { is_expected.to validate_presence_of(:namespace) }
it { is_expected.to validate_presence_of(:repository_storage) } it { is_expected.to validate_presence_of(:repository_storage) }
it 'does not allow new projects beyond user limits' do it 'does not allow new projects beyond user limits' do
......
...@@ -23,9 +23,9 @@ ...@@ -23,9 +23,9 @@
it { is_expected.to validate_presence_of(:author) } it { is_expected.to validate_presence_of(:author) }
it { is_expected.to validate_presence_of(:title) } it { is_expected.to validate_presence_of(:title) }
it { is_expected.to validate_length_of(:title).is_within(0..255) } it { is_expected.to validate_length_of(:title).is_at_most(255) }
it { is_expected.to validate_length_of(:file_name).is_within(0..255) } it { is_expected.to validate_length_of(:file_name).is_at_most(255) }
it { is_expected.to validate_presence_of(:content) } it { is_expected.to validate_presence_of(:content) }
...@@ -46,6 +46,26 @@ ...@@ -46,6 +46,26 @@
end end
end end
describe '#file_name' do
let(:project) { create(:empty_project) }
context 'file_name is nil' do
let(:snippet) { create(:snippet, project: project, file_name: nil) }
it 'returns an empty string' do
expect(snippet.file_name).to eq ''
end
end
context 'file_name is not nil' do
let(:snippet) { create(:snippet, project: project, file_name: 'foo.txt') }
it 'returns the file_name' do
expect(snippet.file_name).to eq 'foo.txt'
end
end
end
describe "#content_html_invalidated?" do describe "#content_html_invalidated?" do
let(:snippet) { create(:snippet, content: "md", content_html: "html", file_name: "foo.md") } let(:snippet) { create(:snippet, content: "md", content_html: "html", file_name: "foo.md") }
it "invalidates the HTML cache of content when the filename changes" do it "invalidates the HTML cache of content when the filename changes" do
......
...@@ -79,7 +79,7 @@ ...@@ -79,7 +79,7 @@
it { is_expected.to allow_value(0).for(:projects_limit) } it { is_expected.to allow_value(0).for(:projects_limit) }
it { is_expected.not_to allow_value(-1).for(:projects_limit) } it { is_expected.not_to allow_value(-1).for(:projects_limit) }
it { is_expected.to validate_length_of(:bio).is_within(0..255) } it { is_expected.to validate_length_of(:bio).is_at_most(255) }
it_behaves_like 'an object with email-formated attributes', :email do it_behaves_like 'an object with email-formated attributes', :email do
subject { build(:user) } subject { build(:user) }
......
...@@ -75,7 +75,6 @@ ...@@ -75,7 +75,6 @@
expect(response).to have_http_status(400) expect(response).to have_http_status(400)
expect(json_response['message']['key']).to eq([ expect(json_response['message']['key']).to eq([
'can\'t be blank', 'can\'t be blank',
'is too short (minimum is 0 characters)',
'is invalid' 'is invalid'
]) ])
end end
...@@ -85,8 +84,7 @@ ...@@ -85,8 +84,7 @@
expect(response).to have_http_status(400) expect(response).to have_http_status(400)
expect(json_response['message']['title']).to eq([ expect(json_response['message']['title']).to eq([
'can\'t be blank', 'can\'t be blank'
'is too short (minimum is 0 characters)'
]) ])
end end
......
# Extend shoulda-matchers
module Shoulda::Matchers::ActiveModel
class ValidateLengthOfMatcher
# Shortcut for is_at_least and is_at_most
def is_within(range)
is_at_least(range.min) && is_at_most(range.max)
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