chore: improve handling of JSON body parsing

Author: bethesque

lib/pact_broker/api/resources/default_base_resource.rb

@@ -12,7 +12,7 @@
 module PactBroker
   module Api
     module Resources
-      class InvalidJsonError < StandardError ; end
+      class InvalidJsonError < PactBroker::Error ; end
 
       class DefaultBaseResource < Webmachine::Resource
         include PactBroker::Services
@@ -85,12 +85,21 @@ def handle_exception e
           PactBroker::Api::Resources::ErrorHandler.call(e, request, response)
         end
 
-        def params
-          @params ||= JSON.parse(request.body.to_s, { symbolize_names: true }.merge(PACT_PARSING_OPTIONS)) #Not load! Otherwise it will try to load Ruby classes.
+        def params(options = {})
+          return options[:default] if options.key?(:default) && request_body.empty?
+
+          symbolize_names = !options.key?(:symbolize_names) || options[:symbolize_names]
+          if symbolize_names
+            @params_with_symbol_keys ||= JSON.parse(request_body, { symbolize_names: true }.merge(PACT_PARSING_OPTIONS)) #Not load! Otherwise it will try to load Ruby classes.
+          else
+            @params_with_string_keys ||= JSON.parse(request_body, { symbolize_names: false }.merge(PACT_PARSING_OPTIONS)) #Not load! Otherwise it will try to load Ruby classes.
+          end
+        rescue JSON::JSONError => e
+          raise InvalidJsonError.new("Error parsing JSON - #{e.message}")
         end
 
         def params_with_string_keys
-          @params_with_string_keys ||= JSON.parse(request.body.to_s, { symbolize_names: false }.merge(PACT_PARSING_OPTIONS))
+          params(symbolize_names: false)
         end
 
         def pact_params
@@ -99,12 +108,12 @@ def pact_params
 
         def set_json_error_message message
           response.headers['Content-Type'] = 'application/hal+json;charset=utf-8'
-          response.body = {error: message}.to_json
+          response.body = { error: message }.to_json
         end
 
         def set_json_validation_error_messages errors
           response.headers['Content-Type'] = 'application/hal+json;charset=utf-8'
-          response.body = {errors: errors}.to_json
+          response.body = { errors: errors }.to_json
         end
 
         def request_body

lib/pact_broker/api/resources/error_handler.rb

@@ -27,7 +27,7 @@ def self.generate_error_reference
         end
 
         def self.reportable?(e)
-          !e.is_a?(PactBroker::Error) && !e.is_a?(JSON::GeneratorError)
+          !e.is_a?(PactBroker::Error) && !e.is_a?(JSON::JSONError)
         end
 
         def self.log_as_warning?(e)

lib/pact_broker/api/resources/pacticipant.rb

@@ -31,7 +31,7 @@ def known_methods
 
         def from_json
           if pacticipant
-            @pacticipant = pacticipant_service.update params_with_string_keys.merge('name' => pacticipant_name)
+            @pacticipant = pacticipant_service.update params(symbolize_names: false).merge('name' => pacticipant_name)
           else
             @pacticipant = pacticipant_service.create params.merge(:name => pacticipant_name)
             response.headers["Location"] = pacticipant_url(base_url, pacticipant)

lib/pact_broker/api/resources/provider_pacts_for_verification.rb

@@ -72,7 +72,7 @@ def query
             if request.get?
               Rack::Utils.parse_nested_query(request.uri.query)
             elsif request.post?
-              params_with_string_keys
+              params(symbolize_names: false, default: {})
             end
           end
         end

lib/pact_broker/api/resources/verifications.rb

@@ -52,7 +52,7 @@ def create_path
         end
 
         def from_json
-          verification = verification_service.create(next_verification_number, params_with_string_keys, pact, webhook_options)
+          verification = verification_service.create(next_verification_number, params(symbolize_names: false), pact, webhook_options)
           response.body = decorator_for(verification).to_json(user_options: { base_url: base_url })
           true
         end

lib/pact_broker/api/resources/webhook.rb

@@ -39,7 +39,7 @@ def malformed_request?
 
         def from_json
           if webhook
-            @webhook = webhook_service.update_by_uuid uuid, params_with_string_keys
+            @webhook = webhook_service.update_by_uuid(uuid, params(symbolize_names: false))
             response.body = to_json
           else
             @webhook = webhook_service.create(uuid, parsed_webhook, consumer, provider)

spec/lib/pact_broker/api/resources/default_base_resource_spec.rb

@@ -4,14 +4,62 @@ module PactBroker
   module Api
     module Resources
       describe DefaultBaseResource do
-        let(:request) { double('request', uri: uri, base_uri: URI("http://example.org/")).as_null_object }
+        let(:request) { double('request', body: body, uri: uri, base_uri: URI("http://example.org/")).as_null_object }
         let(:response) { double('response') }
         let(:uri) { URI('http://example.org/path?query') }
+        let(:body) { double('body', to_s: body_string) }
+        let(:body_string) { '' }
 
         subject { BaseResource.new(request, response) }
 
         its(:resource_url) { is_expected.to eq 'http://example.org/path' }
 
+        describe "params" do
+          let(:body_string) { { foo: 'bar' }.to_json }
+
+          context "when the body is invalid JSON" do
+            let(:body_string) { '{' }
+
+            it "raises an error" do
+              expect { subject.params }.to raise_error InvalidJsonError
+            end
+          end
+
+          context "when the body is empty and a default is provided" do
+            let(:body_string) { '' }
+
+            it "returns the default" do
+              expect(subject.params(default: 'foo')).to eq 'foo'
+            end
+          end
+
+          context "when the body is empty and no default is provided" do
+            let(:body_string) { '' }
+
+            it "raises an error" do
+              expect { subject.params }.to raise_error InvalidJsonError
+            end
+          end
+
+          context "when symbolize_names is not set" do
+            it "symbolizes the names" do
+              expect(subject.params.keys).to eq [:foo]
+            end
+          end
+
+          context "when symbolize_names is true" do
+            it "symbolizes the names" do
+              expect(subject.params(symbolize_names: true).keys).to eq [:foo]
+            end
+          end
+
+          context "when symbolize_names is false" do
+            it "does not symbolize the names" do
+              expect(subject.params(symbolize_names: false).keys).to eq ['foo']
+            end
+          end
+        end
+
         describe "options" do
           subject { options "/"; last_response }