Improve AR validation of Release modelac-releases-api-improve-ar-validation-of-release-model

add comment

3 files changed, 37 insertions, 20 deletions

diff --git a/app/models/release.rb b/app/models/release.rb
index 0dae5c90394..773ac2a4a94 100644
--- a/app/models/release.rb
+++ b/app/models/release.rb
@@ -16,6 +16,14 @@ class Release < ActiveRecord::Base
 
   validates :description, :project, :tag, presence: true
 
+  ##
+  # There are several projects which have been violating this rule on gitlab.com.
+  # We should not create such duplicate rows anymore.
+  validates :tag, uniqueness: { scope: :project }, on: :create
+
+  validates :sha, presence: true, on: :create
+  validates :name, presence: true
+
   scope :sorted, -> { order(created_at: :desc) }
 
   delegate :repository, to: :project
diff --git a/spec/factories/releases.rb b/spec/factories/releases.rb
index cab6b4a811f..ba644869fb9 100644
--- a/spec/factories/releases.rb
+++ b/spec/factories/releases.rb
@@ -8,8 +8,12 @@ FactoryBot.define do
     author
 
     trait :legacy do
-      sha nil
-      author nil
+      after(:create) do |release|
+        ##
+        # Legacy release records do not have sha and author
+        release.update_column(:sha, nil)
+        release.update_column(:author_id, nil)
+      end
     end
   end
 end
diff --git a/spec/requests/api/releases_spec.rb b/spec/requests/api/releases_spec.rb
index 978fa0142c2..4e272829b31 100644
--- a/spec/requests/api/releases_spec.rb
+++ b/spec/requests/api/releases_spec.rb
@@ -17,21 +17,8 @@ describe API::Releases do
 
   describe 'GET /projects/:id/releases' do
     context 'when there are two releases' do
-      let!(:release_1) do
-        create(:release,
-               project: project,
-               tag: 'v0.1',
-               author: maintainer,
-               created_at: 2.days.ago)
-      end
-
-      let!(:release_2) do
-        create(:release,
-               project: project,
-               tag: 'v0.2',
-               author: maintainer,
-               created_at: 1.day.ago)
-      end
+      let!(:release_1) { create(:release, project: project, tag: 'v0.1', author: maintainer) }
+      let!(:release_2) { create(:release, project: project, tag: 'v0.2', author: maintainer) }
 
       it 'returns 200 HTTP status' do
         get api("/projects/#{project.id}/releases", maintainer)
@@ -182,7 +169,7 @@ describe API::Releases do
       it 'cannot find the release entry' do
         get api("/projects/#{project.id}/releases/non_exist_tag", maintainer)
 
-        expect(response).to have_gitlab_http_status(:forbidden)
+        expect(response).to have_gitlab_http_status(:not_found)
       end
     end
 
@@ -244,6 +231,24 @@ describe API::Releases do
       expect(project.releases.last.description).to eq('Super nice release')
     end
 
+    context 'when name is empty' do
+      let(:params) do
+        {
+          name: '',
+          tag_name: 'v0.1',
+          description: 'Super nice release'
+        }
+      end
+
+      it 'returns an error as validation failure' do
+        post api("/projects/#{project.id}/releases", maintainer), params: params
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+        expect(json_response['message'])
+          .to eq("Validation failed: Name can't be blank")
+      end
+    end
+
     context 'when description is empty' do
       let(:params) do
         {
@@ -534,7 +539,7 @@ describe API::Releases do
       it 'forbids the request' do
         put api("/projects/#{project.id}/releases/v0.1", maintainer), params: params
 
-        expect(response).to have_gitlab_http_status(:forbidden)
+        expect(response).to have_gitlab_http_status(:not_found)
       end
     end
 
@@ -619,7 +624,7 @@ describe API::Releases do
       it 'forbids the request' do
         delete api("/projects/#{project.id}/releases/v0.1", maintainer)
 
-        expect(response).to have_gitlab_http_status(:forbidden)
+        expect(response).to have_gitlab_http_status(:not_found)
       end
     end