Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't register the request/response json middleware for Faraday 1.10+ #288

Merged
merged 4 commits into from
Sep 23, 2024

Conversation

iMacTia
Copy link
Member

@iMacTia iMacTia commented Sep 23, 2024

Summary

Faraday 1.10 ships with an out-of-the-box JSON middleware, so we want to to default to that one rather than the faraday_middleware one.
See this comment for more details.

Faraday 1.10 ships with an out-of-the-box JSON middleware, so we want to to default to that one rather than the faraday_middleware one.
@iMacTia iMacTia self-assigned this Sep 23, 2024
@olleolleolle
Copy link
Member

It looks like the change is sound, but the CI needs a little love.

ERROR:  Error installing bundler:
	The last version of bundler (>= 0) to support your Ruby & RubyGems was 2.4.22. Try installing it with `gem install bundler -v 2.4.22`
	bundler requires Ruby version >= 3.0.0. The current ruby version is 2.6.10.210.

@iMacTia
Copy link
Member Author

iMacTia commented Sep 23, 2024

I've updated the CI workflow, hopefully it will work now.
If not, I'll take a look later 🙂

@olleolleolle
Copy link
Member

olleolleolle commented Sep 23, 2024

@iMacTia I am making a separate PR which tries to lean on ruby-setup as much as possible. #290

@olleolleolle
Copy link
Member

@iMacTia I pushed changes to get to green, this branch can now be resolved.

@olleolleolle
Copy link
Member

I will resolve the conflict in this PR right now.

@olleolleolle
Copy link
Member

I got excited and extracted the #291 as well, to keep the edits in this change as focused as we can.

Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@olleolleolle olleolleolle merged commit 88ed602 into main Sep 23, 2024
9 checks passed
@olleolleolle olleolleolle deleted the mg/fix-json-compatibility branch September 23, 2024 10:31
@WvanLelyveld
Copy link

This PR seems to break things for us after all.
We have

> Gem::Version.new(Faraday::VERSION)
#<Gem::Version "1.10.4">

and upgrading faraday_middleware from 1.2.0 to 1.2.1 breaks all tests that do some kind of JSON importing.
Seems like the JSON middleware is not loaded correctly somehow.
Am I doing something wrong here?

@iMacTia
Copy link
Member Author

iMacTia commented Sep 24, 2024

@WvanLelyveld do you have an error or, even better, a stacktrace to share?

@WvanLelyveld
Copy link

WvanLelyveld commented Sep 24, 2024

Sure. The error message becomes: NoMethodError: undefined method 'each' for an instance of String
This is because my mocked endpoint now returns a string instead of a hash.
This is my test:

describe Connectors::Credit::Client do
  let(:supplier)    { create(:supplier) }
  let(:connector)   { supplier.connector }
  let(:endpoint)    { "#{supplier.connector_config[:url]}/balances" }
  let(:credit_list) { load_fixture('balances') }

  it 'gets the list of credits' do
    WebMock.stub_request(:get, endpoint).to_return(status: 200, body: credit_list)
    credit_list = connector.credit_list
    expect(credit_list.keys.count).to eq 13
  end

  def load_fixture(filename)
    Rails.root.join("spec/fixtures/api_clients/#{filename}.json")
  end
end

The connector.credit_list now fails with the NoMethodError.
When I put a debugger there, I can see the request to balances now returns a String in faraday_middleware 1.2.1, where it returned a Hash in 1.2.0.

The credit_list is defined as follows:

        def credit_list
          credit_list = {}
          api_client.balances.faraday_response.body['balances'].each do |i|
            credit_list[i['currency']] = i['amount']
          end
          credit_list
        end

This now errors because faraday_response.body is now mocked as a string instead of a hash.

@WvanLelyveld
Copy link

WvanLelyveld commented Sep 25, 2024

@iMacTia I've made a test repo with two branches.
You can see the only difference is the version of faraday_middleware, v1.2.0 and v.1.2.1
In the main branch, v1.2.1, rspec fails, where in the working-with-1.2.0 branch, the rspec test passes.

This should illustrate that this version breaks things in certain case.
I'm suspecting it has something to do with the absence of the response_headers in the mocked response.
The original FaradayMiddleware::ParseJson class used in faraday_middleware v1.2.0 parses this response, even in the absence of headers, where the Faraday::Response::Json class used in v1.2.1 does not parse when there are no headers present.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants