Merge pull request #293 from github/richa-d/opaqueIDErrorOn5xx

Change opaque ID middleware to not throw OpaqueIDError on 5xx responses
github · Jan 24, 2024 · 652589b · 652589b
2 parents 9b22fcc + 6f73793
commit 652589b
Show file tree

Hide file tree

Showing 2 changed files with 44 additions and 8 deletions.
diff --git a/lib/elastomer_client/middleware/opaque_id.rb b/lib/elastomer_client/middleware/opaque_id.rb
@@ -35,7 +35,8 @@ def call(env)
 
         @app.call(env).on_complete do |renv|
           response_uuid = renv[:response_headers][X_OPAQUE_ID]
-          if uuid != response_uuid
+          # Don't raise OpaqueIdError if the response is a 5xx
+          if !response_uuid.nil? && uuid != response_uuid && renv.status < 500
             raise ::ElastomerClient::Client::OpaqueIdError,
                   "Conflicting 'X-Opaque-Id' headers: request #{uuid.inspect}, response #{response_uuid.inspect}"
           end

diff --git a/test/client_test.rb b/test/client_test.rb
@@ -416,15 +416,50 @@
     end
   end
 
-  it "does not throw OpaqueIdError for mocked response with empty opaque id" do
-    opts = $client_params.merge \
-      opaque_id: true
-    client = ElastomerClient::Client.new(**opts) do |connection|
-      connection.request(:mock_response) { |env| env.body = "{}" }
+  describe "OpaqueIDError conditionals" do
+    it "does not throw OpaqueIdError for mocked response with empty opaque id" do
+      opts = $client_params.merge \
+        opaque_id: true
+      client = ElastomerClient::Client.new(**opts) do |connection|
+        connection.request(:mock_response) { |env| env.body = "{}" }
+      end
+
+      response = client.get("/")
+
+      assert_equal "yes", response.headers["Fake"]
+    end
+
+    it "throws OpaqueIdError on mismatched ID" do
+      client_params = $client_params.merge \
+        opaque_id: true
+      client = ElastomerClient::Client.new(**client_params)
+
+      test_url = "#{client.url}/"
+      stub_request(:get, test_url).and_return(status: 200, headers: { "Content-Type" => "application/json", "X-Opaque-Id" => "foo" })
+
+      assert_raises(ElastomerClient::Client::OpaqueIdError) { client.request :get, test_url, {} }
     end
 
-    response = client.get("/")
+    it "throws OpaqueIdError on empty string ID" do
+      client_params = $client_params.merge \
+        opaque_id: true
+      client = ElastomerClient::Client.new(**client_params)
+
+      test_url = "#{client.url}/"
+      stub_request(:get, test_url).and_return(status: 200, headers: { "Content-Type" => "application/json", "X-Opaque-Id" => "" })
+
+      assert_raises(ElastomerClient::Client::OpaqueIdError) { client.request :get, test_url, {} }
+    end
 
-    assert_equal "yes", response.headers["Fake"]
+    it "throws ServerError and not OpaqueIdError on 5xx response and nil ID" do
+      client_params = $client_params.merge \
+        opaque_id: true
+      client = ElastomerClient::Client.new(**client_params)
+
+      test_url = "#{client.url}/"
+      stub_request(:get, test_url).and_return(status: 503, headers: { "Content-Type" => "application/json" })
+
+      assert_raises(ElastomerClient::Client::ServerError) { client.request :get, test_url, {} }
+    end
   end
 end