feat: monkey patch Webmachine render_error method to support problem+…

…json (#584)

lib/pact_broker/api.rb

@@ -1,5 +1,6 @@
 require "webmachine/adapters/rack_mapped"
 require "webmachine/application_monkey_patch"
+require "webmachine/render_error_monkey_patch"
 require "pact_broker/db/models"
 require "pact_broker/api/resources"
 require "pact_broker/api/decorators"

lib/pact_broker/app.rb

@@ -20,7 +20,6 @@
 require "rack/pact_broker/ui_authentication"
 require "rack/pact_broker/configurable_make_it_later"
 require "rack/pact_broker/no_auth"
-require "rack/pact_broker/convert_404_to_hal"
 require "rack/pact_broker/reset_thread_data"
 require "rack/pact_broker/add_vary_header"
 require "rack/pact_broker/use_when"
@@ -256,7 +255,6 @@ def build_api
       api_apps.unshift(@custom_api) if @custom_api
       builder = ::Rack::Builder.new
       builder.use @make_it_later_api_auth
-      builder.use Rack::PactBroker::Convert404ToHal
       builder.use Rack::PactBroker::DatabaseTransaction, configuration.database_connection
       builder.run Rack::Cascade.new(api_apps, [404])
       builder

lib/pact_broker/string_refinements.rb

@@ -59,6 +59,10 @@ def snakecase
         .downcase
       end
 
+      def dasherize
+        snakecase.tr("_", "-")
+      end
+
       # ripped from rubyworks/facets, thank you
       def camelcase(*separators)
         case separators.first

lib/webmachine/render_error_monkey_patch.rb

@@ -0,0 +1,52 @@
+require "webmachine/errors"
+require "pact_broker/string_refinements"
+
+# Monkey patches the render_error method so that it returns hal+json or problem+json instead of text/html
+
+module Webmachine
+  using PactBroker::StringRefinements
+
+  # Renders a standard error message body for the response. The
+  # standard messages are defined in localization files.
+  # @param [Integer] code the response status code
+  # @param [Request] req the request object
+  # @param [Response] req the response object
+  # @param [Hash] options keys to override the defaults when rendering
+  #     the response body
+  def self.render_error(code, req, res, options={})
+    res.code = code
+    unless res.body
+      title, message = t(["errors.#{code}.title", "errors.#{code}.message"],
+                         { :method => req.method,
+                           :error => res.error}.merge(options))
+
+      title = options[:title] if options[:title]
+      message = options[:message] if options[:message]
+
+      res.body = error_response_body(message, title, title.dasherize.gsub(/^\d+\-/, ""), code, req)
+      res.headers[CONTENT_TYPE] = error_response_content_type(req)
+    end
+    ensure_content_length(res)
+    ensure_date_header(res)
+  end
+
+  def self.problem_json_error_content_type?(request)
+    request.headers["Accept"]&.include?("application/problem+json")
+  end
+
+  def self.error_response_content_type(request)
+    if problem_json_error_content_type?(request)
+      "application/problem+json;charset=utf-8"
+    else
+      "application/json;charset=utf-8"
+    end
+  end
+
+  def self.error_response_body(detail, title, type, status, request)
+    if problem_json_error_content_type?(request)
+      PactBroker::Api::Decorators::CustomErrorProblemJSONDecorator.new(detail: detail, title: title, type: type, status: status).to_json
+    else
+      { error: detail }.to_json
+    end
+  end
+end

spec/integration/app_spec.rb

@@ -92,7 +92,7 @@ module PactBroker
 
         it "returns a 404" do
           expect(subject.status).to eq 404
-          expect(subject.headers["Content-Type"]).to include "text/html"
+          expect(subject.headers["Content-Type"]).to include "application/json"
         end
       end
 
@@ -128,7 +128,6 @@ module PactBroker
           expect(subject.headers["Content-Type"]).to eq "text/csv;charset=utf-8"
         end
       end
-
     end
 
     context "when the Accept header is */* (default curl)" do
@@ -149,7 +148,7 @@ module PactBroker
     context "when no Accept header is specified" do
       let(:env) { {} }
 
-      subject { get path, "", env; last_response }
+      subject { get path, "", env }
 
       describe "a request for root" do
         let(:path) { "/" }
@@ -198,7 +197,7 @@ module PactBroker
     describe "when a resource identifier contains a slash" do
       let(:path) { "/pacticipants/Foo/versions/1.2.3/tags/feat%2Fbar" }
 
-      subject { put path, nil, {"CONTENT_TYPE" => "application/json"}; last_response }
+      subject { put(path, nil, {"CONTENT_TYPE" => "application/json"}) }
 
       it "returns a success status" do
         expect(subject.status).to eq 201

spec/integration/endpoints/non_utf_8_spec.rb

@@ -35,7 +35,7 @@
   its(:status) { is_expected.to eq 400 }
 
   it "returns an error message" do
-    expect(JSON.parse(subject.body)).to eq("error" => "JSON::ParserError - 859: unexpected token at '{'")
+    expect(JSON.parse(subject.body)["error"]).to match(/JSON::ParserError.*unexpected token at '{'/)
   end
 end
 
@@ -49,6 +49,6 @@
   its(:status) { is_expected.to eq 400 }
 
   it "returns an error message" do
-    expect(JSON.parse(subject.body)).to eq("error" => "JSON::ParserError - 859: unexpected token at ''")
+    expect(JSON.parse(subject.body)["error"]).to match(/JSON::ParserError.*unexpected token at ''/)
   end
 end

spec/lib/pact_broker/app_spec.rb

@@ -333,11 +333,11 @@ class TestAuth2 < TestAuth1; end
       subject { get("/does/not/exist", nil, { "CONTENT_TYPE" => "application/hal+json" })  }
 
       it "returns a Content-Type of application/hal+json" do
-        expect(subject.headers["Content-Type"]).to eq "application/hal+json;charset=utf-8"
+        expect(subject.headers["Content-Type"]).to eq "application/json;charset=utf-8"
       end
 
       it "returns a JSON body" do
-        expect(subject.body).to eq ""
+        expect(subject.body).to eq "{\"error\":\"The requested document was not found on this server.\"}"
       end
 
       it "returns a 404" do

spec/lib/webmachine/render_error_monkey_patch_spec.rb

@@ -0,0 +1,54 @@
+require "webmachine/render_error_monkey_patch"
+
+module Webmachine
+  describe ".render_error" do
+    let(:request) { Webmachine::Request.new("GET", "http://example.org/foo", request_headers, "", {}) }
+    let(:request_headers) { Webmachine::Headers.new }
+    let(:response) { Webmachine::Response.new }
+    let(:options) { {} }
+
+    subject { Webmachine.render_error(404, request, response, options); response }
+
+    it "returns a JSON body" do
+      expect(JSON.parse(subject.body)).to eq "error" => "The requested document was not found on this server."
+    end
+
+    it "returns a json content-type" do
+      expect(subject.headers["Content-Type"]).to eq "application/json;charset=utf-8"
+    end
+
+    context "when the Accept header contains application/problem+json" do
+      let(:request_headers) { Webmachine::Headers.from_cgi("HTTP_ACCEPT" => "application/problem+json") }
+
+      let(:expected_body) do
+        {
+          "detail" => "The requested document was not found on this server.",
+          "status" => 404,
+          "title" => "404 Not Found",
+          "type" => "/problem/not-found"
+        }
+      end
+
+      it "returns a JSON body in problem json format" do
+        expect(JSON.parse(subject.body)).to eq expected_body
+      end
+
+      context "with custom options" do
+        let(:options) { { message: "hello world", title: "Title" } }
+
+        let(:expected_body) do
+          {
+            "detail" => "hello world",
+            "status" => 404,
+            "title" => "Title",
+            "type" => "/problem/title"
+          }
+        end
+
+        it "uses the custom message" do
+          expect(JSON.parse(subject.body)).to eq expected_body
+        end
+      end
+    end
+  end
+end