More correct headers.

According to HTTP RFC 2616, GET and DELETE request
bodies should be ignored, so we shouldn't be using
them.

Sending a Content-Type header with a bodyless
request makes no sense and causes some servers to
fail as they try to JSON parse an empty string
(which is invalid).

This change removes the Content-Type header from
GET and DELETE requests and deprecates sending
a body with a DELETE request (currently only used
by the need API where a POST would be more appropriate).

lib/gds_api/base.rb

@@ -22,7 +22,7 @@ def create_client
                           :post_json, :post_json!,
                           :put_json, :put_json!,
                           :patch_json, :patch_json!,
-                          :delete_json, :delete_json!,
+                          :delete_json, :delete_json!, :delete_json_with_params!,
                           :get_raw, :get_raw!,
                           :put_multipart,
                           :post_multipart

lib/gds_api/json_client.rb

@@ -54,12 +54,17 @@ def initialize(options = {})
     def self.default_request_headers
       {
         'Accept' => 'application/json',
-        'Content-Type' => 'application/json',
         # GOVUK_APP_NAME is set for all apps by Puppet
         'User-Agent' => "gds-api-adapters/#{GdsApi::VERSION} (#{ENV["GOVUK_APP_NAME"]})"
       }
     end
 
+    def self.default_request_with_json_body_headers
+      self.default_request_headers.merge(
+        'Content-Type' => 'application/json',
+      )
+    end
+
     DEFAULT_TIMEOUT_IN_SECONDS = 4
     DEFAULT_CACHE_SIZE = 100
     DEFAULT_CACHE_TTL = 15 * 60 # 15 minutes
@@ -132,7 +137,19 @@ def patch_json!(url, params, additional_headers = {})
       do_json_request(:patch, url, params, additional_headers)
     end
 
-    def delete_json!(url, params = nil, additional_headers = {})
+    def delete_json!(url, additional_headers = {})
+      do_json_request(:delete, url, nil, additional_headers)
+    end
+
+    def delete_json_with_params!(url, params, additional_headers = {})
+      warn <<-doc
+        DEPRECATION NOTICE: Delete requests should not include parameters.
+
+        Do not use this method as the ability to do this will be removed.
+
+        Called from: #{caller[2]}
+      doc
+
       do_json_request(:delete, url, params, additional_headers)
     end
 
@@ -209,10 +226,12 @@ def with_timeout(method_params)
       )
     end
 
-    def with_headers(method_params, headers)
-      headers = headers.merge(GdsApi::GovukHeaders.headers)
+    def with_headers(method_params, default_headers, additional_headers)
       method_params.merge(
-        headers: method_params[:headers].merge(headers)
+        headers: default_headers
+          .merge(method_params[:headers] || {})
+          .merge(GdsApi::GovukHeaders.headers)
+          .merge(additional_headers)
       )
     end
 
@@ -270,12 +289,19 @@ def do_request(method, url, params = nil, additional_headers = {})
       method_params = {
         method: method,
         url: url,
-        headers: self.class.default_request_headers
       }
+
+      case method
+      when :get, :delete
+        default_headers = self.class.default_request_headers
+      else
+        default_headers = self.class.default_request_with_json_body_headers
+      end
+
       method_params[:payload] = params
-      method_params = with_auth_options(method_params)
       method_params = with_timeout(method_params)
-      method_params = with_headers(method_params, additional_headers)
+      method_params = with_headers(method_params, default_headers, additional_headers)
+      method_params = with_auth_options(method_params)
       if URI.parse(url).is_a? URI::HTTPS
         method_params = with_ssl_options(method_params)
       end

lib/gds_api/need_api.rb

@@ -42,7 +42,8 @@ def close(need_id, duplicate_of)
 
   def reopen(need_id, author)
     # author params: { "author" => { ... } }"
-    delete_json!("#{endpoint}/needs/#{CGI.escape(need_id.to_s)}/closed", author)
+    # NB: This should really be a POST
+    delete_json_with_params!("#{endpoint}/needs/#{CGI.escape(need_id.to_s)}/closed", author)
   end
 
   def create_note(note)

test/json_client_test.rb

@@ -15,6 +15,8 @@ def setup
     GdsApi::JsonClient.cache = nil
 
     @client = GdsApi::JsonClient.new
+
+    WebMock.disable_net_connect!
   end
 
   def teardown
@@ -770,7 +772,7 @@ def test_client_can_use_basic_auth
 
   def test_client_can_use_bearer_token
     client = GdsApi::JsonClient.new(bearer_token: 'SOME_BEARER_TOKEN')
-    expected_headers = GdsApi::JsonClient.default_request_headers.
+    expected_headers = GdsApi::JsonClient.default_request_with_json_body_headers.
       merge('Authorization' => 'Bearer SOME_BEARER_TOKEN')
 
     stub_request(:put, "http://some.other.endpoint/some.json").
@@ -820,7 +822,7 @@ def test_client_can_set_custom_headers_on_puts
   def test_client_can_set_custom_headers_on_deletes
     stub_request(:delete, "http://some.other.endpoint/some.json").to_return(:status => 200)
 
-    response = GdsApi::JsonClient.new.delete_json("http://some.other.endpoint/some.json", {},
+    response = GdsApi::JsonClient.new.delete_json("http://some.other.endpoint/some.json",
                                                   { "HEADER-A" => "B", "HEADER-C" => "D" })
 
     assert_requested(:delete, %r{/some.json}) do |request|
@@ -970,4 +972,21 @@ def test_should_use_custom_logger_specified_in_options
     client = GdsApi::JsonClient.new(logger: custom_logger)
     assert_same client.logger, custom_logger
   end
+
+  def test_should_avoid_content_type_header_on_get_without_body
+    url = "http://some.endpoint/some.json"
+    stub_request(:any, url)
+
+    @client.get_json!(url)
+    assert_requested(:get, url, headers: GdsApi::JsonClient.default_request_headers)
+
+    @client.delete_json!(url)
+    assert_requested(:delete, url, headers: GdsApi::JsonClient.default_request_headers)
+
+    @client.post_json!(url, test: "123")
+    assert_requested(:post, url, headers: GdsApi::JsonClient.default_request_with_json_body_headers)
+
+    @client.put_json!(url, test: "123")
+    assert_requested(:put, url, headers: GdsApi::JsonClient.default_request_with_json_body_headers)
+  end
 end

test/publishing_api_v2_test.rb

@@ -1121,9 +1121,7 @@ def content_item_for_content_id(content_id, attrs = {})
           method: :get,
           path: "/v2/linkables",
           query: "document_type=topic",
-          headers: {
-            "Content-Type" => "application/json",
-          },
+          headers: {},
         )
         .will_respond_with(
           status: 200,
@@ -1150,9 +1148,7 @@ def content_item_for_content_id(content_id, attrs = {})
           method: :get,
           path: "/v2/content",
           query: "content_format=topic&fields%5B%5D=title&fields%5B%5D=base_path",
-          headers: {
-            "Content-Type" => "application/json",
-          },
+          headers: {},
         )
         .will_respond_with(
           status: 200,
@@ -1196,9 +1192,7 @@ def content_item_for_content_id(content_id, attrs = {})
           method: :get,
           path: "/v2/content",
           query: "content_format=topic&fields%5B%5D=content_id&fields%5B%5D=locale",
-          headers: {
-            "Content-Type" => "application/json",
-          },
+          headers: {},
         )
         .will_respond_with(
           status: 200,
@@ -1240,9 +1234,7 @@ def content_item_for_content_id(content_id, attrs = {})
           method: :get,
           path: "/v2/content",
           query: "content_format=topic&fields%5B%5D=content_id&fields%5B%5D=locale&locale=fr",
-          headers: {
-            "Content-Type" => "application/json",
-          },
+          headers: {},
         )
         .will_respond_with(
           status: 200,
@@ -1284,9 +1276,7 @@ def content_item_for_content_id(content_id, attrs = {})
           method: :get,
           path: "/v2/content",
           query: "content_format=topic&fields%5B%5D=content_id&fields%5B%5D=locale&locale=all",
-          headers: {
-            "Content-Type" => "application/json",
-          },
+          headers: {},
         )
         .will_respond_with(
           status: 200,
@@ -1333,9 +1323,7 @@ def content_item_for_content_id(content_id, attrs = {})
           method: :get,
           path: "/v2/content",
           query: "content_format=topic&fields%5B%5D=content_id&fields%5B%5D=details",
-          headers: {
-            "Content-Type" => "application/json",
-          },
+          headers: {},
         )
         .will_respond_with(
           status: 200,
@@ -1509,9 +1497,7 @@ def content_item_for_content_id(content_id, attrs = {})
             method: :get,
             path: "/v2/linked/" + @linked_content_item['content_id'],
             query: "fields%5B%5D=content_id&fields%5B%5D=base_path&link_type=topic",
-            headers: {
-              "Content-Type" => "application/json",
-            },
+            headers: {},
           )
           .will_respond_with(
             status: 200,