lostisland / faraday

Simple, but flexible HTTP client library, with support for multiple backends.
https://lostisland.github.io/faraday
MIT License
5.71k stars 972 forks source link

Header matching inconsistency with strict_mode and verify_stubbed_calls #1488

Open grjones opened 1 year ago

grjones commented 1 year ago

Basic Info

Issue description

When adding rspec tests using Faraday's test facility, an error is falsely raised when the following are true:

  1. request(:json) is set
  2. 'Content-Type': 'application/json' is set in a :test adapter stub
  3. use_strict = true is set in the :test adapter
  4. verify_stubbed_calls is called

The resulting error is:

Faraday::Adapter::Test::Stubs::NotFound: no stubbed request for ...

This appears to be due to inconsistent header case normalization and case sensitive header matching in strict stubs.

Explicitly setting the Content-type header (with lowercase -type) is a workaround like so:

Faraday.new(url:) do |f|
  f.request(:json)

  # work-a-round
  f.headers = { 'Content-type' => 'application/json' }
end

Steps to reproduce

Create a faraday object like so:

client = Faraday.new(url: <your_base_url>) do |f|
  f.request(:json)
end

Set the expected stubs:

headers = { 'Content-Type': 'application/json' }

client.adapter(:test) do |stub|
  stub.post('/api/push', {}, headers) { [200, {}, {}] }
  stub.strict_mode = true
end

Use the client:

client.post('/api/push') do |req|
  req.body = {}
end

Run verify_stubbed_calls and observe the unexpected failure:

client.adapter(:test).build.stubs.verify_stubbed_calls
yykamei commented 1 year ago

Hi, thank you for reporting the issue.

There might be problems on Faraday::Utils::Headers; it can't handle Symbol header including - here.

https://github.com/lostisland/faraday/blob/f6c38689c6b8d46b83bd31393cb2474cf68d9012/lib/faraday/utils/headers.rb#L57-L63

Also, testing adapter does not type-cast non-String body as String, so stub.post('/api/push', {}, headers) should be stub.post('/api/push', '{}', headers) (I think testing adapter can cast the value 😅 )

I think you don't have to explicitly set Content-Type in the block of Faraday.new. Instead, the workaround is like this:

headers = { 'Content-Type' => 'application/json' }

client.adapter(:test) do |stub|
  stub.post('/api/push', '{}', headers) { [200, {}, {}] }
  stub.strict_mode = true
end

Here is the change:

diff --git a/a.rb b/a.rb
index a0a1cba..ef6599a 100644
--- a/a.rb
+++ b/a.rb
@@ -1,6 +1,6 @@
-headers = { 'Content-Type': 'application/json' }
+headers = { 'Content-Type' => 'application/json' }

 client.adapter(:test) do |stub|
-  stub.post('/api/push', {}, headers) { [200, {}, {}] }
+  stub.post('/api/push', '{}', headers) { [200, {}, {}] }
   stub.strict_mode = true
 end
grjones commented 1 year ago

Hi @yykamei. Yes, that also works. The bug though is that you'd want request(:json) to be able to work, which should be taking care of encoding the request body as json and setting the correct content-type. So, yours is also a workaround but without the request(:json), you'll need to encode as json your self. Eg:

stub.post('/api/push', payload.to_json, headers) { [200, {}, {}] }
grjones commented 1 year ago

Actually, we're both partly right. It is indeed that symbols aren't supported in the testing framework. So your headers change works. But, f.request(:json) also works. So, my working version is:

client = Faraday.new(url: <your_base_url>) do |f|
  f.request(:json)
end

headers = { 'Content-Type' => 'application/json' } # no longer a symbol

client.adapter(:test) do |stub|
  stub.post('/api/push', {}, headers) { [200, {}, {}] }
  stub.strict_mode = true
end

Thanks for your help. I don't know if this should just be closed? Maybe others would get tripped up by the symbol comparison.

yykamei commented 1 year ago

Maybe, I understood your point; the testing adapter should consider the JSON middleware when comparing the actually used request header with expected one. If developers use request(:json), testing adapter should also encode the passed stub body as JSON. Is it correct?