Use :maximum instead of :within for length validators with a 0..N range25209-improve-length-validators

Signed-off-by: Rémy Coutable <remy@rymai.me>

11 files changed, 58 insertions, 27 deletions

diff --git a/spec/lib/gitlab/github_import/importer_spec.rb b/spec/lib/gitlab/github_import/importer_spec.rb
index 000b9aa6f83..9e027839f59 100644
--- a/spec/lib/gitlab/github_import/importer_spec.rb
+++ b/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" }
           ]
diff --git a/spec/models/ci/variable_spec.rb b/spec/models/ci/variable_spec.rb
index 4e7833c3162..bee9f714849 100644
--- a/spec/models/ci/variable_spec.rb
+++ b/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
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index 6f84bffe046..4fa06a8c60a 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/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
diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb
index d06665197db..c8170164898 100644
--- a/spec/models/environment_spec.rb
+++ b/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
diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb
index 90731f55470..2a33d819138 100644
--- a/spec/models/key_spec.rb
+++ b/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
diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb
index 431b3e4435f..ba0ed4a3603 100644
--- a/spec/models/namespace_spec.rb
+++ b/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
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 8abcce42ce0..f7c8c97fdee 100644
--- a/spec/models/project_spec.rb
+++ b/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
diff --git a/spec/models/snippet_spec.rb b/spec/models/snippet_spec.rb
index f62f6bacbaa..279dc30c357 100644
--- a/spec/models/snippet_spec.rb
+++ b/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) }
 
@@ -46,6 +46,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
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 14c891994d0..95fe2dc65d9 100644
--- a/spec/models/user_spec.rb
+++ b/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) }
diff --git a/spec/requests/api/deploy_keys_spec.rb b/spec/requests/api/deploy_keys_spec.rb
index 5fa7299044e..aabab8e6ae6 100644
--- a/spec/requests/api/deploy_keys_spec.rb
+++ b/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
 
diff --git a/spec/support/matchers/is_within.rb b/spec/support/matchers/is_within.rb
deleted file mode 100644
index 0c35fc7e899..00000000000
--- a/spec/support/matchers/is_within.rb
+++ /dev/null
@@ -1,9 +0,0 @@
-# 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