Merge branch '25209-improve-length-validators' into 'master'

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

Closes #25209

See merge request !7894

app/models/ci/variable.rb

@@ -4,10 +4,10 @@ module Ci
 
     belongs_to :project, foreign_key: :gl_project_id
 
-    validates_uniqueness_of :key, scope: :gl_project_id
     validates :key,
       presence: true,
-      length: { within: 0..255 },
+      uniqueness: { scope: :gl_project_id },
+      length: { maximum: 255 },
       format: { with: /\A[a-zA-Z0-9_]+\z/,
                 message: "can contain only letters, digits and '_'." }
 

app/models/concerns/issuable.rb

@@ -41,7 +41,7 @@ module Issuable
     has_one :metrics
 
     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 :assigned_to, ->(u) { where(assignee_id: u.id)}

app/models/environment.rb

@@ -9,7 +9,7 @@ class Environment < ActiveRecord::Base
   validates :name,
             presence: true,
             uniqueness: { scope: :project_id },
-            length: { within: 0..255 },
+            length: { maximum: 255 },
             format: { with: Gitlab::Regex.environment_name_regex,
                       message: Gitlab::Regex.environment_name_regex_message }
 

app/models/key.rb

@@ -8,10 +8,18 @@ class Key < ActiveRecord::Base
 
   before_validation :generate_fingerprint
 
-  validates :title, presence: true, length: { within: 0..255 }
-  validates :key, presence: true, length: { within: 0..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' }
+  validates :title,
+    presence: true,
+    length: { maximum: 255 }
+  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
 

app/models/namespace.rb

@@ -12,17 +12,17 @@ class Namespace < ActiveRecord::Base
 
   validates :owner, presence: true, unless: ->(n) { n.type == "Group" }
   validates :name,
-    length: { within: 0..255 },
-    namespace_name: 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,
-    length: { within: 1..255 },
-    namespace: 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
 

app/models/project.rb

@@ -172,13 +172,13 @@ class Project < ActiveRecord::Base
   validates :description, length: { maximum: 2000 }, allow_blank: true
   validates :name,
     presence: true,
-    length: { within: 0..255 },
+    length: { maximum: 255 },
     format: { with: Gitlab::Regex.project_name_regex,
               message: Gitlab::Regex.project_name_regex_message }
   validates :path,
     presence: true,
     project_path: true,
-    length: { within: 0..255 },
+    length: { maximum: 255 },
     format: { with: Gitlab::Regex.project_path_regex,
               message: Gitlab::Regex.project_path_regex_message }
   validates :namespace, presence: true

app/models/snippet.rb

@@ -27,9 +27,9 @@ class Snippet < ActiveRecord::Base
   delegate :name, :email, to: :author, prefix: true, allow_nil: true
 
   validates :author, presence: true
-  validates :title, presence: true, length: { within: 0..255 }
+  validates :title, presence: true, length: { maximum: 255 }
   validates :file_name,
-    length: { within: 0..255 },
+    length: { maximum: 255 },
     format: { with: Gitlab::Regex.file_name_regex,
               message: Gitlab::Regex.file_name_regex_message }
 
@@ -94,6 +94,10 @@ class Snippet < ActiveRecord::Base
     0
   end
 
+  def file_name
+    super.to_s
+  end
+
   # alias for compatibility with blobs and highlighting
   def path
     file_name

spec/lib/gitlab/github_import/importer_spec.rb

@@ -155,7 +155,7 @@ describe Gitlab::GithubImport::Importer, lib: true do
           message: 'The remote data could not be fully imported.',
           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: :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: :release, url: 'https://api.github.com/repos/octocat/Hello-World/releases/2', errors: "Validation failed: Description can't be blank" }
           ]

spec/models/ci/variable_spec.rb

@@ -5,6 +5,13 @@ describe Ci::Variable, models: true do
 
   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
     subject.value = secret_value
   end

spec/models/concerns/issuable_spec.rb

@@ -35,7 +35,7 @@ describe Issue, "Issuable" do
     it { is_expected.to validate_presence_of(:iid) }
     it { is_expected.to validate_presence_of(:author) }
     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
 
   describe "Scope" do

spec/models/environment_spec.rb

@@ -13,9 +13,9 @@ describe Environment, models: true do
 
   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_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:
   # https://github.com/thoughtbot/shoulda-matchers/issues/336

spec/models/key_spec.rb

@@ -7,9 +7,13 @@ describe Key, models: true do
 
   describe "Validation" do
     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_length_of(:title).is_within(0..255) }
-    it { is_expected.to validate_length_of(:key).is_within(0..5000) }
+    it { is_expected.to validate_length_of(:key).is_at_most(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
 
   describe "Methods" do

spec/models/namespace_spec.rb

@@ -4,11 +4,18 @@ describe Namespace, models: true do
   let!(:namespace) { create(:namespace) }
 
   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_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_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
   end

spec/models/project_spec.rb

@@ -131,14 +131,18 @@ describe Project, models: true do
 
     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_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_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(:description).is_within(0..2000) }
+    it { is_expected.to validate_length_of(:path).is_at_most(255) }
+
+    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(:namespace) }
+
     it { is_expected.to validate_presence_of(:repository_storage) }
 
     it 'does not allow new projects beyond user limits' do

spec/models/snippet_spec.rb

@@ -23,9 +23,9 @@ describe Snippet, models: true do
     it { is_expected.to validate_presence_of(:author) }
 
     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) }
 
@@ -61,6 +61,26 @@ describe Snippet, models: true do
     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
     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

spec/models/user_spec.rb

@@ -79,7 +79,7 @@ describe User, models: true do
     it { is_expected.to allow_value(0).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
       subject { build(:user) }

spec/requests/api/deploy_keys_spec.rb

@@ -75,7 +75,6 @@ describe API::DeployKeys, api: true  do
       expect(response).to have_http_status(400)
       expect(json_response['message']['key']).to eq([
         'can\'t be blank',
-        'is too short (minimum is 0 characters)',
         'is invalid'
       ])
     end
@@ -85,8 +84,7 @@ describe API::DeployKeys, api: true  do
 
       expect(response).to have_http_status(400)
       expect(json_response['message']['title']).to eq([
-        'can\'t be blank',
-        'is too short (minimum is 0 characters)'
+        'can\'t be blank'
       ])
     end
 

spec/support/matchers/is_within.rb

-# 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