From f5363f43df7b4da00ac96824586446845bb2c4e5 Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Sun, 12 Jul 2020 08:38:34 +1000 Subject: [PATCH] chore: improve handling of JSON body parsing --- .../api/resources/default_base_resource.rb | 21 +++++--- .../api/resources/error_handler.rb | 2 +- lib/pact_broker/api/resources/pacticipant.rb | 2 +- .../provider_pacts_for_verification.rb | 2 +- .../api/resources/verifications.rb | 2 +- lib/pact_broker/api/resources/webhook.rb | 2 +- .../resources/default_base_resource_spec.rb | 50 ++++++++++++++++++- 7 files changed, 69 insertions(+), 12 deletions(-) diff --git a/lib/pact_broker/api/resources/default_base_resource.rb b/lib/pact_broker/api/resources/default_base_resource.rb index d8daa138d..6fd6b9c81 100644 --- a/lib/pact_broker/api/resources/default_base_resource.rb +++ b/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 diff --git a/lib/pact_broker/api/resources/error_handler.rb b/lib/pact_broker/api/resources/error_handler.rb index 966749874..9a83721d8 100644 --- a/lib/pact_broker/api/resources/error_handler.rb +++ b/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) diff --git a/lib/pact_broker/api/resources/pacticipant.rb b/lib/pact_broker/api/resources/pacticipant.rb index 27fd681c0..af016d1fe 100644 --- a/lib/pact_broker/api/resources/pacticipant.rb +++ b/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) diff --git a/lib/pact_broker/api/resources/provider_pacts_for_verification.rb b/lib/pact_broker/api/resources/provider_pacts_for_verification.rb index e50692aa4..e7a605899 100644 --- a/lib/pact_broker/api/resources/provider_pacts_for_verification.rb +++ b/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 diff --git a/lib/pact_broker/api/resources/verifications.rb b/lib/pact_broker/api/resources/verifications.rb index b1f35bcff..c6b2bdb13 100644 --- a/lib/pact_broker/api/resources/verifications.rb +++ b/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 diff --git a/lib/pact_broker/api/resources/webhook.rb b/lib/pact_broker/api/resources/webhook.rb index 1a88bc1b1..f56c391a3 100644 --- a/lib/pact_broker/api/resources/webhook.rb +++ b/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) diff --git a/spec/lib/pact_broker/api/resources/default_base_resource_spec.rb b/spec/lib/pact_broker/api/resources/default_base_resource_spec.rb index 1200b01a4..f855468aa 100644 --- a/spec/lib/pact_broker/api/resources/default_base_resource_spec.rb +++ b/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 }