Merge branch 'ac-releases-api-with-assets' into 'master'

Support asset links creation in `POST /projects/:id/release`

See merge request gitlab-org/gitlab-ce!24056

app/models/release.rb

@@ -10,6 +10,10 @@ class Release < ActiveRecord::Base
   # releases prior to 11.7 have no author
   belongs_to :author, class_name: 'User'
 
+  has_many :links, class_name: 'Releases::Link'
+
+  accepts_nested_attributes_for :links, allow_destroy: true
+
   validates :description, :project, :tag, presence: true
 
   scope :sorted, -> { order(created_at: :desc) }
@@ -26,6 +30,16 @@ class Release < ActiveRecord::Base
     actual_tag.nil?
   end
 
+  def assets_count
+    links.count + sources.count
+  end
+
+  def sources
+    strong_memoize(:sources) do
+      Releases::Source.all(project, tag)
+    end
+  end
+
   private
 
   def actual_sha

app/models/releases/link.rb

@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+module Releases
+  class Link < ActiveRecord::Base
+    self.table_name = 'release_links'
+
+    belongs_to :release
+
+    validates :url, presence: true, url: true
+    validates :name, presence: true, uniqueness: { scope: :release }
+
+    scope :sorted, -> { order(created_at: :desc) }
+
+    def internal?
+      url.start_with?(release.project.web_url)
+    end
+
+    def external?
+      !internal?
+    end
+  end
+end

app/models/releases/source.rb

@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+module Releases
+  class Source
+    include ActiveModel::Model
+
+    attr_accessor :project, :tag_name, :format
+
+    FORMATS = %w(zip tar.gz tar.bz2 tar).freeze
+
+    class << self
+      def all(project, tag_name)
+        Releases::Source::FORMATS.map do |format|
+          Releases::Source.new(project: project,
+                               tag_name: tag_name,
+                               format: format)
+        end
+      end
+    end
+
+    def url
+      Gitlab::Routing
+        .url_helpers
+        .project_archive_url(project,
+                             id: File.join(tag_name, archive_prefix),
+                             format: format)
+    end
+
+    private
+
+    def archive_prefix
+      "#{project.path}-#{tag_name.tr('/', '-')}"
+    end
+  end
+end

app/services/releases/create_service.rb

@@ -43,11 +43,12 @@ module Releases
         description: description,
         author: current_user,
         tag: tag.name,
-        sha: tag.dereferenced_target.sha
+        sha: tag.dereferenced_target.sha,
+        links_attributes: params.dig(:assets, 'links') || []
       )
 
       success(tag: tag, release: release)
-    rescue ActiveRecord::RecordInvalid => e
+    rescue => e
       error(e.message, 400)
     end
   end

changelogs/unreleased/ac-releases-api-with-assets.yml

@@ -0,0 +1,5 @@
+---
+title: Support CURD operation for Links as one of the Release assets
+merge_request: 24056
+author:
+type: changed

db/migrate/20181228175414_create_releases_link_table.rb

@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+class CreateReleasesLinkTable < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    create_table :release_links, id: :bigserial do |t|
+      t.references :release, null: false, index: false, foreign_key: { on_delete: :cascade }
+      t.string :url, null: false
+      t.string :name, null: false
+      t.timestamps_with_timezone null: false
+
+      t.index [:release_id, :url], unique: true
+      t.index [:release_id, :name], unique: true
+    end
+  end
+end

db/schema.rb

@@ -1798,6 +1798,16 @@ ActiveRecord::Schema.define(version: 20190103140724) do
     t.index ["source_type", "source_id"], name: "index_redirect_routes_on_source_type_and_source_id", using: :btree
   end
 
+  create_table "release_links", id: :bigserial, force: :cascade do |t|
+    t.integer "release_id", null: false
+    t.string "url", null: false
+    t.string "name", null: false
+    t.datetime_with_timezone "created_at", null: false
+    t.datetime_with_timezone "updated_at", null: false
+    t.index ["release_id", "name"], name: "index_release_links_on_release_id_and_name", unique: true, using: :btree
+    t.index ["release_id", "url"], name: "index_release_links_on_release_id_and_url", unique: true, using: :btree
+  end
+
   create_table "releases", force: :cascade do |t|
     t.string "tag"
     t.text "description"
@@ -2439,6 +2449,7 @@ ActiveRecord::Schema.define(version: 20190103140724) do
   add_foreign_key "protected_tag_create_access_levels", "users"
   add_foreign_key "protected_tags", "projects", name: "fk_8e4af87648", on_delete: :cascade
   add_foreign_key "push_event_payloads", "events", name: "fk_36c74129da", on_delete: :cascade
+  add_foreign_key "release_links", "releases", on_delete: :cascade
   add_foreign_key "releases", "projects", name: "fk_47fe2a0596", on_delete: :cascade
   add_foreign_key "releases", "users", column: "author_id", name: "fk_8e4456f90f", on_delete: :nullify
   add_foreign_key "remote_mirrors", "projects", on_delete: :cascade

lib/api/entities.rb

@@ -1093,12 +1093,34 @@ module API
       expose :description
     end
 
+    module Releases
+      class Link < Grape::Entity
+        expose :id
+        expose :name
+        expose :url
+        expose :external?, as: :external
+      end
+
+      class Source < Grape::Entity
+        expose :format
+        expose :url
+      end
+    end
+
     class Release < TagRelease
       expose :name
       expose :description_html
       expose :created_at
       expose :author, using: Entities::UserBasic, if: -> (release, _) { release.author.present? }
       expose :commit, using: Entities::Commit
+
+      expose :assets do
+        expose :assets_count, as: :count
+        expose :sources, using: Entities::Releases::Source
+        expose :links, using: Entities::Releases::Link do |release, options|
+          release.links.sorted
+        end
+      end
     end
 
     class Tag < Grape::Entity

lib/api/releases.rb

@@ -49,6 +49,12 @@ module API
         requires :name,        type: String, desc: 'The name of the release'
         requires :description, type: String, desc: 'The release notes'
         optional :ref,         type: String, desc: 'The commit sha or branch name'
+        optional :assets, type: Hash do
+          optional :links, type: Array do
+            requires :name, type: String
+            requires :url, type: String
+          end
+        end
       end
       post ':id/releases' do
         authorize_create_release!

lib/gitlab/import_export/import_export.yml

@@ -28,7 +28,8 @@ project_tree:
     - notes:
         :author
   - releases:
-      :author
+    - :author
+    - :links
   - project_members:
     - :user
   - merge_requests:

lib/gitlab/import_export/relation_factory.rb

@@ -23,7 +23,8 @@ module Gitlab
                     custom_attributes: 'ProjectCustomAttribute',
                     project_badges: 'Badge',
                     metrics: 'MergeRequest::Metrics',
-                    ci_cd_settings: 'ProjectCiCdSetting' }.freeze
+                    ci_cd_settings: 'ProjectCiCdSetting',
+                    links: 'Releases::Link' }.freeze
 
       USER_REFERENCES = %w[author_id assignee_id updated_by_id merged_by_id latest_closed_by_id user_id created_by_id last_edited_by_id merge_user_id resolved_by_id closed_by_id].freeze
 

spec/factories/releases/link.rb

@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :release_link, class: ::Releases::Link do
+    release
+    sequence(:name) { |n| "release-18.#{n}.dmg" }
+    sequence(:url) { |n| "https://example.com/scrambled-url/app-#{n}.zip" }
+  end
+end

spec/fixtures/api/schemas/release.json

@@ -12,6 +12,25 @@
     },
     "author": {
       "oneOf": [{ "type": "null" }, { "$ref": "public_api/v4/user/basic.json" }]
+    },
+    "assets": {
+      "count": { "type": "integer" },
+      "links": {
+        "type": "array",
+        "items": {
+          "id": "integer",
+          "name": "string",
+          "url": "string",
+          "external": "boolean"
+        }
+      },
+      "sources": {
+        "type": "array",
+        "items": {
+          "format": "zip",
+          "url": "string"
+        }
+      }
     }
   },
   "additionalProperties": false

spec/lib/gitlab/import_export/all_models.yml

@@ -66,6 +66,9 @@ snippets:
 releases:
 - author
 - project
+- links
+links:
+- release
 project_members:
 - created_by
 - user

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -120,6 +120,13 @@ Release:
 - project_id
 - created_at
 - updated_at
+Releases::Link:
+- id
+- release_id
+- url
+- name
+- created_at
+- updated_at
 ProjectMember:
 - id
 - access_level

spec/models/release_spec.rb

@@ -10,10 +10,35 @@ RSpec.describe Release do
   describe 'associations' do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to belong_to(:author).class_name('User') }
+    it { is_expected.to have_many(:links).class_name('Releases::Link') }
   end
 
   describe 'validation' do
     it { is_expected.to validate_presence_of(:project) }
     it { is_expected.to validate_presence_of(:description) }
   end
+
+  describe '#assets_count' do
+    subject { release.assets_count }
+
+    it 'returns the number of sources' do
+      is_expected.to eq(Releases::Source::FORMATS.count)
+    end
+
+    context 'when a links exists' do
+      let!(:link) { create(:release_link, release: release) }
+
+      it 'counts the link as an asset' do
+        is_expected.to eq(1 + Releases::Source::FORMATS.count)
+      end
+    end
+  end
+
+  describe '#sources' do
+    subject { release.sources }
+
+    it 'returns sources' do
+      is_expected.to all(be_a(Releases::Source))
+    end
+  end
 end

spec/models/releases/link_spec.rb

@@ -0,0 +1,70 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Releases::Link do
+  let(:release) { create(:release, project: project) }
+  let(:project) { create(:project) }
+
+  describe 'associations' do
+    it { is_expected.to belong_to(:release) }
+  end
+
+  describe 'validation' do
+    it { is_expected.to validate_presence_of(:url) }
+    it { is_expected.to validate_presence_of(:name) }
+
+    context 'when url is invalid' do
+      let(:link) { build(:release_link, url: 'hoge') }
+
+      it 'will be invalid' do
+        expect(link).to be_invalid
+      end
+    end
+
+    context 'when duplicate name is added to a release' do
+      let!(:link) { create(:release_link, name: 'alpha', release: release) }
+
+      it 'raises an error' do
+        expect do
+          create(:release_link, name: 'alpha', release: release)
+        end.to raise_error(ActiveRecord::RecordInvalid)
+      end
+    end
+  end
+
+  describe '.sorted' do
+    subject { described_class.sorted }
+
+    let!(:link_1) { create(:release_link, name: 'alpha', release: release, created_at: 1.day.ago) }
+    let!(:link_2) { create(:release_link, name: 'beta', release: release, created_at: 2.days.ago) }
+
+    it 'returns a list of links by created_at order' do
+      is_expected.to eq([link_1, link_2])
+    end
+  end
+
+  describe '#internal?' do
+    subject { link.internal? }
+
+    let(:link) { build(:release_link, release: release, url: url) }
+    let(:url) { "#{project.web_url}/-/jobs/140463678/artifacts/download" }
+
+    it { is_expected.to be_truthy }
+
+    context 'when link does not include project web url' do
+      let(:url) { 'https://google.com/-/jobs/140463678/artifacts/download' }
+
+      it { is_expected.to be_falsy }
+    end
+  end
+
+  describe '#external?' do
+    subject { link.external? }
+
+    let(:link) { build(:release_link, release: release, url: url) }
+    let(:url) { 'https://google.com/-/jobs/140463678/artifacts/download' }
+
+    it { is_expected.to be_truthy }
+  end
+end

spec/models/releases/source_spec.rb

@@ -0,0 +1,41 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Releases::Source do
+  set(:project) { create(:project, :repository, name: 'finance-cal') }
+  let(:tag_name) { 'v1.0' }
+
+  describe '.all' do
+    subject { described_class.all(project, tag_name) }
+
+    it 'returns all formats of sources' do
+      expect(subject.map(&:format))
+        .to match_array(described_class::FORMATS)
+    end
+  end
+
+  describe '#url' do
+    subject { source.url }
+
+    let(:source) do
+      described_class.new(project: project, tag_name: tag_name, format: format)
+    end
+
+    let(:format) { 'zip' }
+
+    it 'returns zip archived source url' do
+      is_expected
+        .to eq("#{project.web_url}/-/archive/v1.0/finance-cal-v1.0.zip")
+    end
+
+    context 'when ref is directory structure' do
+      let(:tag_name) { 'beta/v1.0' }
+
+      it 'converts slash to dash' do
+        is_expected
+          .to eq("#{project.web_url}/-/archive/beta/v1.0/finance-cal-beta-v1.0.zip")
+      end
+    end
+  end
+end

spec/requests/api/releases_spec.rb

@@ -121,6 +121,7 @@ describe API::Releases do
         expect(json_response['description']).to eq('This is v0.1')
         expect(json_response['author']['name']).to eq(maintainer.name)
         expect(json_response['commit']['id']).to eq(commit.id)
+        expect(json_response['assets']['count']).to eq(4)
       end
 
       it 'matches response schema' do
@@ -128,6 +129,53 @@ describe API::Releases do
 
         expect(response).to match_response_schema('release')
       end
+
+      it 'contains source information as assets' do
+        get api("/projects/#{project.id}/releases/v0.1", maintainer)
+
+        expect(json_response['assets']['sources'].map { |h| h['format'] })
+          .to match_array(release.sources.map(&:format))
+        expect(json_response['assets']['sources'].map { |h| h['url'] })
+          .to match_array(release.sources.map(&:url))
+      end
+
+      context 'when release has link asset' do
+        let!(:link) do
+          create(:release_link,
+                 release: release,
+                 name: 'release-18.04.dmg',
+                 url: url)
+        end
+
+        let(:url) { 'https://my-external-hosting.example.com/scrambled-url/app.zip' }
+
+        it 'contains link information as assets' do
+          get api("/projects/#{project.id}/releases/v0.1", maintainer)
+
+          expect(json_response['assets']['links'].count).to eq(1)
+          expect(json_response['assets']['links'].first['id']).to eq(link.id)
+          expect(json_response['assets']['links'].first['name'])
+            .to eq('release-18.04.dmg')
+          expect(json_response['assets']['links'].first['url'])
+            .to eq('https://my-external-hosting.example.com/scrambled-url/app.zip')
+          expect(json_response['assets']['links'].first['external'])
+            .to be_truthy
+        end
+
+        context 'when link is internal' do
+          let(:url) do
+            "#{project.web_url}/-/jobs/artifacts/v11.6.0-rc4/download?" \
+            "job=rspec-mysql+41%2F50"
+          end
+
+          it 'has external false' do
+            get api("/projects/#{project.id}/releases/v0.1", maintainer)
+
+            expect(json_response['assets']['links'].first['external'])
+              .to be_falsy
+          end
+        end
+      end
     end
 
     context 'when specified tag is not found in the project' do
@@ -254,6 +302,90 @@ describe API::Releases do
           expect(response).to have_gitlab_http_status(:forbidden)
         end
       end
+
+      context 'when create assets altogether' do
+        let(:base_params) do
+          {
+            name: 'New release',
+            tag_name: 'v0.1',
+            description: 'Super nice release'
+          }
+        end
+
+        context 'when create one asset' do
+          let(:params) do
+            base_params.merge({
+              assets: {
+                links: [{ name: 'beta', url: 'https://dosuken.example.com/inspection.exe' }]
+              }
+            })
+          end
+
+          it 'accepts the request' do
+            post api("/projects/#{project.id}/releases", maintainer), params: params
+
+            expect(response).to have_gitlab_http_status(:created)
+          end
+
+          it 'creates an asset with specified parameters' do
+            post api("/projects/#{project.id}/releases", maintainer), params: params
+
+            expect(json_response['assets']['links'].count).to eq(1)
+            expect(json_response['assets']['links'].first['name']).to eq('beta')
+            expect(json_response['assets']['links'].first['url'])
+              .to eq('https://dosuken.example.com/inspection.exe')
+          end
+
+          it 'matches response schema' do
+            post api("/projects/#{project.id}/releases", maintainer), params: params
+
+            expect(response).to match_response_schema('release')
+          end
+        end
+
+        context 'when create two assets' do
+          let(:params) do
+            base_params.merge({
+              assets: {
+                links: [
+                  { name: 'alpha', url: 'https://dosuken.example.com/alpha.exe' },
+                  { name: 'beta', url: 'https://dosuken.example.com/beta.exe' }
+                ]
+              }
+            })
+          end
+
+          it 'creates two assets with specified parameters' do
+            post api("/projects/#{project.id}/releases", maintainer), params: params
+
+            expect(json_response['assets']['links'].count).to eq(2)
+            expect(json_response['assets']['links'].map { |h| h['name'] })
+              .to match_array(%w[alpha beta])
+            expect(json_response['assets']['links'].map { |h| h['url'] })
+              .to match_array(%w[https://dosuken.example.com/alpha.exe
+                                 https://dosuken.example.com/beta.exe])
+          end
+
+          context 'when link names are duplicates' do
+            let(:params) do
+              base_params.merge({
+                assets: {
+                  links: [
+                    { name: 'alpha', url: 'https://dosuken.example.com/alpha.exe' },
+                    { name: 'alpha', url: 'https://dosuken.example.com/beta.exe' }
+                  ]
+                }
+              })
+            end
+
+            it 'recognizes as a bad request' do
+              post api("/projects/#{project.id}/releases", maintainer), params: params
+
+              expect(response).to have_gitlab_http_status(:bad_request)
+            end
+          end
+        end
+      end
     end
 
     context 'when tag does not exist in git repository' do