17830: Customizes ActionDispatch::RequestId middleware to our needs.

Moves all the request id code to the proper middleware, and updates tests.
As functional tests are "unit tests for controllers", things that happen inside
middlewares need to be tested from integration tests.

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima@curii.com>

diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index fc33dde4477b45d059db2cbd7a63f919eb67e167..c39bdde4b878a26446404979b08e8c3bd08e2b75 100644 (file)
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -196,7 +196,7 @@ class ApplicationController < ActionController::Base
     end
     err[:errors] ||= args
     err[:errors].map! do |err|
-      err += " (" + Thread.current[:request_id] + ")"
+      err += " (#{request.request_id})"
     end
     err[:error_token] = [Time.now.utc.to_i, "%08x" % rand(16 ** 8)].join("+")
     status = err.delete(:status) || 422
@@ -419,17 +419,9 @@ class ApplicationController < ActionController::Base
   end
 
   def set_current_request_id
-    req_id = request.headers['X-Request-Id']
-    if !req_id || req_id.length < 1 || req_id.length > 1024
-      # Client-supplied ID is either missing or too long to be
-      # considered friendly.
-      req_id = "req-" + Random::DEFAULT.rand(2**128).to_s(36)[0..19]
-    end
-    response.headers['X-Request-Id'] = Thread.current[:request_id] = req_id
-    Rails.logger.tagged(req_id) do
+    Rails.logger.tagged(request.request_id) do
       yield
     end
-    Thread.current[:request_id] = nil
   end
 
   def append_info_to_payload(payload)

diff --git a/services/api/config/initializers/request_id_middleware.rb b/services/api/config/initializers/request_id_middleware.rb
new file mode 100644 (file)
index 0000000..e215880
--- /dev/null
+++ b/services/api/config/initializers/request_id_middleware.rb
@@ -0,0 +1,25 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+module CustomRequestId
+  def make_request_id(req_id)
+    if !req_id || req_id.length < 1 || req_id.length > 1024
+      # Client-supplied ID is either missing or too long to be
+      # considered friendly.
+      internal_request_id
+    else
+      req_id
+    end
+  end
+
+  def internal_request_id
+    "req-" + Random::DEFAULT.rand(2**128).to_s(36)[0..19]
+  end
+end
+
+class ActionDispatch::RequestId
+  # Instead of using the default UUID-like format for X-Request-Id headers,
+  # use our own.
+  prepend CustomRequestId
+end
\ No newline at end of file

diff --git a/services/api/test/functional/application_controller_test.rb b/services/api/test/functional/application_controller_test.rb
index 2cfa054448c29fcbbe3beb0b80edc37af514eb2e..af7882141e31973c7e28d0f42da16abb088ed88c 100644 (file)
--- a/services/api/test/functional/application_controller_test.rb
+++ b/services/api/test/functional/application_controller_test.rb
@@ -24,9 +24,6 @@ class ApplicationControllerTest < ActionController::TestCase
     token_time = token.split('+', 2).first.to_i
     assert_operator(token_time, :>=, @start_stamp, "error token too old")
     assert_operator(token_time, :<=, now_timestamp, "error token too new")
-    json_response['errors'].each do |err|
-      assert_match(/req-[a-z0-9]{20}/, err, "X-Request-Id value missing on error message")
-    end
   end
 
   def check_404(errmsg="Path not found")
@@ -56,28 +53,6 @@ class ApplicationControllerTest < ActionController::TestCase
     check_error_token
   end
 
-  test "X-Request-Id header" do
-    authorize_with :spectator
-    get(:index)
-    assert_match /^req-[0-9a-zA-Z]{20}$/, response.headers['X-Request-Id']
-  end
-
-  # The response header is the one that gets logged, so this test also
-  # ensures we log the ID supplied in the request, if any.
-  test "X-Request-Id given by client" do
-    authorize_with :spectator
-    @request.headers['X-Request-Id'] = 'abcdefG'
-    get(:index)
-    assert_equal 'abcdefG', response.headers['X-Request-Id']
-  end
-
-  test "X-Request-Id given by client is ignored if too long" do
-    authorize_with :spectator
-    @request.headers['X-Request-Id'] = 'abcdefG' * 1000
-    get(:index)
-    assert_match /^req-[0-9a-zA-Z]{20}$/, response.headers['X-Request-Id']
-  end
-
   ['foo', '', 'FALSE', 'TRUE', nil, [true], {a:true}, '"true"'].each do |bogus|
     test "bogus boolean parameter #{bogus.inspect} returns error" do
       @controller = Arvados::V1::GroupsController.new

diff --git a/services/api/test/integration/errors_test.rb b/services/api/test/integration/errors_test.rb
index 04843adcf7dc06649dbcd6274060b568b2da84d8..e3224f49127e83bf9b76f8887b83b65bf1733bc0 100644 (file)
--- a/services/api/test/integration/errors_test.rb
+++ b/services/api/test/integration/errors_test.rb
@@ -30,10 +30,29 @@ class ErrorsTest < ActionDispatch::IntegrationTest
     end
   end
 
-  test "X-Request-Id header format on non-existant object URL" do
+  test "X-Request-Id header" do
+    get "/", headers: auth(:spectator)
+    assert_match /^req-[0-9a-zA-Z]{20}$/, response.headers['X-Request-Id']
+  end
+
+  test "X-Request-Id header on non-existant object URL" do
     get "/arvados/v1/container_requests/invalid",
       params: {:format => :json}, headers: auth(:active)
     assert_response 404
     assert_match /^req-[0-9a-zA-Z]{20}$/, response.headers['X-Request-Id']
   end
+
+  # The response header is the one that gets logged, so this test also
+  # ensures we log the ID supplied in the request, if any.
+  test "X-Request-Id given by client" do
+    get "/", headers: auth(:spectator).merge({'X-Request-Id': 'abcdefG'})
+    assert_equal 'abcdefG', response.headers['X-Request-Id']
+  end
+
+  test "X-Request-Id given by client is ignored if too long" do
+    authorize_with :spectator
+    long_reqId = 'abcdefG' * 1000
+    get "/", headers: auth(:spectator).merge({'X-Request-Id': long_reqId})
+    assert_match /^req-[0-9a-zA-Z]{20}$/, response.headers['X-Request-Id']
+  end
 end