Refactor maven packages code and add more specs

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

ee/app/finders/packages/package_file_finder.rb

+# frozen_string_literal: true
+class Packages::PackageFileFinder
+  attr_reader :package, :file_name
+
+  def initialize(package, file_name)
+    @package = package
+    @file_name = file_name
+  end
+
+  def execute
+    package_files.last
+  end
+
+  def execute!
+    package_files.last!
+  end
+
+  private
+
+  def package_files
+    package.package_files.where(file_name: file_name)
+  end
+end

ee/app/models/packages/package_file.rb

@@ -3,8 +3,8 @@ class Packages::PackageFile < ActiveRecord::Base
   belongs_to :package
 
   validates :package, presence: true
+  validates :file, presence: true
+  validates :file_name, presence: true
 
   mount_uploader :file, Packages::PackageFileUploader
-
-  scope :recent, -> { reorder(id: :desc) }
 end

ee/app/services/packages/create_package_file_service.rb

 # frozen_string_literal: true
-class Packages::CreatePackageFileService
-  attr_reader :package, :params
+module Packages
+  class CreatePackageFileService
+    attr_reader :package, :params
 
-  def initialize(package, params)
-    @package = package
-    @params = params
-  end
+    def initialize(package, params)
+      @package = package
+      @params = params
+    end
 
-  def execute
-    package.package_files.create!(
-      file:      params[:file],
-      size:      params[:size],
-      file_name: params[:file_name],
-      file_type: params[:file_type],
-      file_sha1: params[:file_sha1],
-      file_md5:  params[:file_md5]
-    )
+    def execute
+      package.package_files.create!(
+        file:      params[:file],
+        size:      params[:size],
+        file_name: params[:file_name],
+        file_type: params[:file_type],
+        file_sha1: params[:file_sha1],
+        file_md5:  params[:file_md5]
+      )
+    end
   end
 end

ee/lib/api/maven_packages.rb

@@ -43,6 +43,17 @@ module API
           [file_name, nil]
         end
       end
+
+      def verify_package_file(package_file, uploaded_file)
+        stored_sha1 = Digest::SHA256.hexdigest(package_file.file_sha1)
+        expected_sha1 = uploaded_file.sha256
+
+        if stored_sha1 == expected_sha1
+          no_content!
+        else
+          conflict!
+        end
+      end
     end
 
     params do
@@ -64,7 +75,8 @@ module API
         package = ::Packages::MavenPackageFinder
           .new(user_project, params[:path]).execute!
 
-        package_file = package.package_files.recent.find_by!(file_name: file_name)
+        package_file = ::Packages::PackageFileFinder
+          .new(package, file_name).execute!
 
         case format
         when 'md5'
@@ -142,23 +154,20 @@ module API
             version: version
           }
 
-          package = ::Packages::CreateMavenPackageService.new(user_project, current_user, package_params).execute
+          package = ::Packages::CreateMavenPackageService
+            .new(user_project, current_user, package_params).execute
         end
 
-        if format
-          # TODO: Extract in separate method
-          if format == 'sha1'
-            package_file = package.package_files.recent.find_by!(file_name: file_name)
-            stored_sha1 = Digest::SHA256.hexdigest(package_file.file_sha1)
-            expected_sha1 = uploaded_file.sha256
-
-            if stored_sha1 == expected_sha1
-              no_content!
-            else
-              conflict!
-            end
-          end
-        else
+        case format
+        when 'sha1'
+          # After uploading a file, Maven tries to upload a sha1 and md5 version of it.
+          # Since we store md5/sha1 in database we simply need to validate our hash
+          # against one uploaded by Maven. We do this for `sha1` format.
+          package_file = ::Packages::PackageFileFinder
+            .new(package, file_name).execute!
+
+          verify_package_file(package_file, uploaded_file)
+        when nil
           file_params = {
             file:      uploaded_file,
             size:      params['file.size'],

ee/spec/finders/packages/maven_package_finder_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe Packages::MavenPackageFinder do
+  let(:project) { create(:project) }
+  let(:package) { create(:maven_package, project: project) }
+
+  describe '#execute!' do
+    it 'returns a package' do
+      finder = described_class.new(project, package.maven_metadatum.path)
+
+      expect(finder.execute!).to eq(package)
+    end
+
+    it 'raises an error' do
+      finder = described_class.new(project, 'com/example/my-app/1.0-SNAPSHOT')
+
+      expect { finder.execute! }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+  end
+end

ee/spec/finders/packages/package_file_finder_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe Packages::PackageFileFinder do
+  let(:package) { create(:maven_package) }
+  let(:package_file) { package.package_files.first }
+
+  describe '#execute!' do
+    it 'returns a package file' do
+      finder = described_class.new(package, package_file.file_name)
+
+      expect(finder.execute!).to eq(package_file)
+    end
+
+    it 'raises an error' do
+      finder = described_class.new(package, 'unknown.jpg')
+
+      expect { finder.execute! }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+  end
+end

ee/spec/services/packages/create_maven_package_service_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe Packages::CreateMavenPackageService do
+  let(:project) { create(:project) }
+  let(:user) { create(:user) }
+  let(:app_name) { 'my-app'.freeze }
+  let(:version) { '1.0-SNAPSHOT'.freeze }
+  let(:path) { "my/company/app/#{app_name}" }
+  let(:path_with_version) { "#{path}/#{version}" }
+
+  describe '#execute' do
+    context 'with version' do
+      let(:params) do
+        {
+          path: path_with_version,
+          name: path,
+          version: version
+        }
+      end
+
+      it 'creates a new package with metadatum' do
+        package = described_class.new(project, user, params).execute
+
+        expect(package).to be_valid
+        expect(package.name).to eq(path)
+        expect(package.version).to eq(version)
+        expect(package.maven_metadatum).to be_valid
+        expect(package.maven_metadatum.path).to eq(path_with_version)
+        expect(package.maven_metadatum.app_group).to eq('my.company.app')
+        expect(package.maven_metadatum.app_name).to eq(app_name)
+        expect(package.maven_metadatum.app_version).to eq(version)
+      end
+    end
+
+    context 'without version' do
+      let(:params) do
+        {
+          path: path,
+          name: path,
+          version: nil
+        }
+      end
+
+      it 'creates a new package with metadatum' do
+        package = described_class.new(project, user, params).execute
+
+        expect(package).to be_valid
+        expect(package.name).to eq(path)
+        expect(package.version).to be nil
+        expect(package.maven_metadatum).to be_valid
+        expect(package.maven_metadatum.path).to eq(path)
+        expect(package.maven_metadatum.app_group).to eq('my.company.app')
+        expect(package.maven_metadatum.app_name).to eq(app_name)
+        expect(package.maven_metadatum.app_version).to be nil
+      end
+    end
+
+    context 'path is missing' do
+      let(:params) do
+        {
+          name: path,
+          version: version
+        }
+      end
+
+      it 'raises an error' do
+        service = described_class.new(project, user, params)
+
+        expect { service.execute }.to raise_error(ActiveRecord::RecordInvalid)
+      end
+    end
+  end
+end

ee/spec/services/packages/create_package_file_service_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe Packages::CreatePackageFileService do
+  let(:package) { create(:maven_package) }
+
+  describe '#execute' do
+    context 'with valid params' do
+      let(:params) do
+        {
+          file: Tempfile.new,
+          file_name: 'foo.jar'
+        }
+      end
+
+      it 'creates a new package file' do
+        package_file = described_class.new(package, params).execute
+
+        expect(package_file).to be_valid
+        expect(package_file.file_name).to eq('foo.jar')
+      end
+    end
+
+    context 'file is missing' do
+      let(:params) do
+        {
+          file_name: 'foo.jar'
+        }
+      end
+
+      it 'raises an error' do
+        service = described_class.new(package, params)
+
+        expect { service.execute }.to raise_error(ActiveRecord::RecordInvalid)
+      end
+    end
+  end
+end