Use 400 error instead of 500 on bad composer.json

When publishing a composer package via the API, if
the composer.json file is missing or malformed, the
backend returns a 500 error. This commit return a
more appropriate 400 Bad Request instead.

app/services/packages/composer/composer_json_service.rb

@@ -3,6 +3,8 @@
 module Packages
   module Composer
     class ComposerJsonService
+      InvalidJson = Class.new(StandardError)
+
       def initialize(project, target)
         @project, @target = project, target
       end
@@ -20,11 +22,11 @@ module Packages
 
         Gitlab::Json.parse(composer_file.data)
       rescue JSON::ParserError
-        raise 'Could not parse composer.json file. Invalid JSON.'
+        raise InvalidJson, 'Could not parse composer.json file. Invalid JSON.'
       end
 
       def composer_file_not_found!
-        raise 'The file composer.json was not found.'
+        raise InvalidJson, 'The file composer.json was not found.'
       end
     end
   end

changelogs/unreleased/260323-handle-malformed-composer-json.yml

+---
+title: Return 422 error rather than 500 when composer.json is missing or malformed
+merge_request: 44587
+author: David Barr @davebarr
+type: fixed

lib/api/composer_packages.rb

@@ -26,6 +26,10 @@ module API
       render_api_error!(e.message, 400)
     end
 
+    rescue_from Packages::Composer::ComposerJsonService::InvalidJson do |e|
+      render_api_error!(e.message, 422)
+    end
+
     helpers do
       def packages
         strong_memoize(:packages) do

spec/requests/api/composer_packages_spec.rb

@@ -289,6 +289,34 @@ RSpec.describe API::ComposerPackages do
         it_behaves_like 'process Composer api request', :developer, :not_found
       end
     end
+
+    context 'with invalid composer.json' do
+      let(:headers) { basic_auth_header(user.username, personal_access_token.token) }
+      let(:params) { { tag: 'v1.2.99' } }
+      let(:project) { create(:project, :custom_repo, files: files, group: group) }
+
+      before do
+        project.repository.add_tag(user, 'v1.2.99', 'master')
+      end
+
+      context 'with a missing composer.json file' do
+        let(:files) { { 'some_other_file' => '' } }
+
+        it_behaves_like 'process Composer api request', :developer, :unprocessable_entity
+      end
+
+      context 'with an empty composer.json file' do
+        let(:files) { { 'composer.json' => '' } }
+
+        it_behaves_like 'process Composer api request', :developer, :unprocessable_entity
+      end
+
+      context 'with a malformed composer.json file' do
+        let(:files) { { 'composer.json' => 'not_valid_JSON' } }
+
+        it_behaves_like 'process Composer api request', :developer, :unprocessable_entity
+      end
+    end
   end
 
   describe 'GET /api/v4/projects/:id/packages/composer/archives/*package_name?sha=:sha' do

spec/services/packages/composer/composer_json_service_spec.rb

@@ -23,7 +23,7 @@ RSpec.describe Packages::Composer::ComposerJsonService do
         let(:json) { '{ name": "package-name"}' }
 
         it 'raises an error' do
-          expect { subject }.to raise_error(/Invalid/)
+          expect { subject }.to raise_error(described_class::InvalidJson, /Invalid/)
         end
       end
     end
@@ -32,7 +32,7 @@ RSpec.describe Packages::Composer::ComposerJsonService do
       let(:project) { create(:project, :repository) }
 
       it 'raises an error' do
-        expect { subject }.to raise_error(/not found/)
+        expect { subject }.to raise_error(described_class::InvalidJson, /not found/)
       end
     end
   end