Refactor code related to maven packages

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

db/schema.rb

@@ -1923,9 +1923,10 @@ ActiveRecord::Schema.define(version: 20180807153545) do
 
   create_table "packages_maven_metadata", force: :cascade do |t|
     t.integer "package_id", null: false
+    t.string "path", null: false
     t.string "app_group", null: false
     t.string "app_name", null: false
-    t.string "app_version", null: false
+    t.string "app_version"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
   end
@@ -1949,7 +1950,7 @@ ActiveRecord::Schema.define(version: 20180807153545) do
 
   create_table "packages_packages", force: :cascade do |t|
     t.integer "project_id", null: false
-    t.string "name"
+    t.string "name", null: false
     t.string "version"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false

ee/app/finders/packages/maven_package_finder.rb

+class Packages::MavenPackageFinder
+  attr_reader :project, :path
+
+  def initialize(project, path)
+    @project = project
+    @path = path
+  end
+
+  def execute
+    packages.last
+  end
+
+  def execute!
+    packages.last!
+  end
+
+  private
+
+  def packages
+    project.packages.joins(:maven_metadatum)
+      .where(packages_maven_metadata: { path: path })
+  end
+end

ee/app/services/packages/create_maven_package_service.rb

@@ -2,24 +2,22 @@
 module Packages
   class CreateMavenPackageService < BaseService
     def execute
-      package = Packages::Package.create!(
-        project: project,
-        name: full_app_name,
-        version: params[:app_version]
+      package = project.packages.create!(
+        name: params[:name],
+        version: params[:version]
       )
 
-      Packages::MavenMetadatum.create!(
-        package: package,
-        app_group: params[:app_group],
-        app_name: params[:app_name],
-        app_version: params[:app_version]
-      )
-    end
+      app_group, _, app_name = params[:name].rpartition('/')
+      app_group.tr!('/', '.')
 
-    private
+      package.create_maven_metadatum!(
+        path: params[:path],
+        app_group: app_group,
+        app_name: app_name,
+        app_version: params[:version]
+      )
 
-    def full_app_name
-      params[:app_group] + '/' + params[:app_name]
+      package
     end
   end
 end

ee/app/services/packages/create_package_file_service.rb

+class Packages::CreatePackageFileService
+  attr_reader :package, :params
+
+  def initialize(package, params)
+    @package = package
+    @params = params
+  end
+
+  def execute
+    package.package_files.create!(
+      file:      parmas[: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

ee/db/migrate/20180720120716_create_packages_packages.rb

@@ -5,7 +5,7 @@ class CreatePackagesPackages < ActiveRecord::Migration
   def change
     create_table :packages_packages do |t|
       t.references :project, index: true, foreign_key: { on_delete: :cascade }, null: false
-      t.string :name
+      t.string :name, null: false
       t.string :version
 
       t.timestamps_with_timezone null: false

ee/db/migrate/20180720121404_create_packages_maven_metadata.rb

@@ -9,9 +9,10 @@ class CreatePackagesMavenMetadata < ActiveRecord::Migration
   def up
     create_table :packages_maven_metadata do |t|
       t.references :package, index: true, null: false
+      t.string :path, null: false
       t.string :app_group, null: false
       t.string :app_name, null: false
-      t.string :app_version, null: false
+      t.string :app_version
 
       t.timestamps_with_timezone null: false
     end

ee/lib/api/maven_packages.rb

@@ -2,8 +2,6 @@
 module API
   class MavenPackages < Grape::API
     MAVEN_ENDPOINT_REQUIREMENTS = {
-      app_name: API::NO_SLASH_URL_PART_REGEX,
-      app_version: API::NO_SLASH_URL_PART_REGEX,
       file_name: API::NO_SLASH_URL_PART_REGEX
     }.freeze
 
@@ -25,14 +23,6 @@ module API
           [file_name, nil]
         end
       end
-
-      def valid_metadata_xml?(xml)
-        version = Nokogiri::XML(xml).css('metadata:root > version').text
-
-        # Skip handling top level maven-metadata.xml (one without the version) for now.
-        # Also make sure version in the metadata file is equal to one in the URL
-        version.present? && version == params[:app_version]
-      end
     end
 
     params do
@@ -43,21 +33,18 @@ module API
         detail 'This feature was introduced in GitLab 11.3'
       end
       params do
-        requires :app_group, type: String, desc: 'Package group id'
-        requires :app_name, type: String, desc: 'Package artifact id'
-        requires :app_version, type: String, desc: 'Package version'
+        requires :path, type: String, desc: 'Package path'
         requires :file_name, type: String, desc: 'Package file name'
       end
-      get ':id/packages/maven/*app_group/:app_name/:app_version/:file_name', requirements: MAVEN_ENDPOINT_REQUIREMENTS do
+      get ':id/packages/maven/*path/:file_name', requirements: MAVEN_ENDPOINT_REQUIREMENTS do
         unauthorized! unless can?(current_user, :read_package, user_project)
 
         file_name, format = extract_format(params[:file_name])
 
-        metadata = ::Packages::MavenMetadatum.find_by!(app_group: params[:app_group],
-                                                       app_name: params[:app_name],
-                                                       app_version: params[:app_version])
+        package = ::Packages::MavenPackageFinder
+          .new(user_project, params[:path]).execute!
 
-        package_file = metadata.package.package_files.recent.find_by!(file_name: file_name)
+        package_file = package.package_files.recent.find_by!(file_name: file_name)
 
         case format
         when 'md5'
@@ -73,12 +60,10 @@ module API
         detail 'This feature was introduced in GitLab 11.3'
       end
       params do
-        requires :app_group, type: String, desc: 'Package group id'
-        requires :app_name, type: String, desc: 'Package artifact id'
-        requires :app_version, type: String, desc: 'Package version'
+        requires :path, type: String, desc: 'Package path'
         requires :file_name, type: String, desc: 'Package file name'
       end
-      put ':id/packages/maven/*app_group/:app_name/:app_version/:file_name/authorize', requirements: MAVEN_ENDPOINT_REQUIREMENTS do
+      put ':id/packages/maven/*path/:file_name/authorize', requirements: MAVEN_ENDPOINT_REQUIREMENTS do
         not_allowed! unless Gitlab.config.packages.enabled
         unauthorized! unless can?(current_user, :admin_package, user_project)
 
@@ -94,17 +79,17 @@ module API
         detail 'This feature was introduced in GitLab 11.3'
       end
       params do
-        requires :app_group, type: String, desc: 'Package group id'
-        requires :app_name, type: String, desc: 'Package artifact id'
-        requires :app_version, type: String, desc: 'Package version'
+        requires :path, type: String, desc: 'Package path'
         requires :file_name, type: String, desc: 'Package file name'
         optional 'file.path', type: String, desc: %q(path to locally stored body (generated by Workhorse))
         optional 'file.name', type: String, desc: %q(real filename as send in Content-Disposition (generated by Workhorse))
         optional 'file.type', type: String, desc: %q(real content type as send in Content-Type (generated by Workhorse))
         optional 'file.size', type: Integer, desc: %q(real size of file (generated by Workhorse))
+        optional 'file.md5', type: String, desc: %q(md5 checksum of the file (generated by Workhorse))
+        optional 'file.sha1', type: String, desc: %q(sha1 checksum of the file (generated by Workhorse))
         optional 'file.sha256', type: String, desc: %q(sha256 checksum of the file (generated by Workhorse))
       end
-      put ':id/packages/maven/*app_group/:app_name/:app_version/:file_name', requirements: MAVEN_ENDPOINT_REQUIREMENTS do
+      put ':id/packages/maven/*path/:file_name', requirements: MAVEN_ENDPOINT_REQUIREMENTS do
         not_allowed! unless Gitlab.config.packages.enabled
         unauthorized! unless can?(current_user, :admin_package, user_project)
 
@@ -115,46 +100,48 @@ module API
         uploaded_file = UploadedFile.from_params(params, :file, ::Packages::PackageFileUploader.workhorse_local_upload_path)
         bad_request!('Missing package file!') unless uploaded_file
 
-        metadata = ::Packages::MavenMetadatum.find_by(app_group: params[:app_group],
-                                                      app_name: params[:app_name],
-                                                      app_version: params[:app_version])
-
-        # TODO: Refactor. We don't need to read file for every request, only metadata and md5/sha1
-        string_file = File.read(uploaded_file)
+        package = ::Packages::MavenPackageFinder
+          .new(user_project, params[:path]).execute
 
-        unless metadata
+        unless package
           if file_name == MAVEN_METADATA_FILE
-            return unless valid_metadata_xml?(string_file)
+            # Maven uploads several files during `mvn deploy` in next order:
+            #   - my-company/my-app/1.0-SNAPSHOT/my-app.jar
+            #   - my-company/my-app/1.0-SNAPSHOT/my-app.pom
+            #   - my-company/my-app/1.0-SNAPSHOT/maven-metadata.xml
+            #   - my-company/my-app/maven-metadata.xml
+            #
+            # The last xml file does not have VERSION in URL because it contains
+            # information about all versions.
+            package_name, version = params[:path], nil
+          else
+            package_name, _, version = params[:path].rpartition('/')
           end
 
-          # There is no metadata for this upload. We need to create a package
-          # record and corresponding maven metadata record
-          metadata = Packages::CreateMavenPackageService.new(user_project, current_user, params).execute
+          package_params = {
+            name: package_name,
+            path: params[:path],
+            version: version
+          }
+
+          package = ::Packages::CreateMavenPackageService.new(user_project, current_user, package_params).execute
         end
 
         if format
-          # Maven tries to create a md5 and sha1 files for each package file.
-          # Instead, we update existing package file record with such data.
-          package_file = metadata.package.package_files.recent.find_by!(file_name: file_name)
-
-          case format
-          when 'md5'
-            package_file.file_md5 = string_file
-          when 'sha1'
-            package_file.file_sha1 = string_file
-          end
+          # TODO: Validate our checksum with Maven.
+          no_content!
         else
-          # TODO: If maven-metadata.xml file is present for this package, we
-          # need to update it instead of creating a new one
-          package_file = metadata.package.package_files.new
-          package_file.file_name = file_name
-          package_file.file_type = file_name.rpartition('.').last
-
-          # Convert string into CarrierWave compatible StringIO object
-          package_file.file = uploaded_file
+          file_params = {
+            file:      uploaded_file,
+            size:      params['file.size'],
+            file_name: file_name,
+            file_type: params['file.type'],
+            file_sha1: params['file.sha1'],
+            file_md5:  params['file.md5']
+          }
+
+          ::Packages::CreatePackageFileService.new(package, file_params).execute
         end
-
-        package_file.save!
       end
     end
   end