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

Connection methods #861

Merged
merged 13 commits into from Feb 25, 2019
Merged

Connection methods #861

merged 13 commits into from Feb 25, 2019

Conversation

technoweenie
Copy link
Member

@technoweenie technoweenie commented Feb 21, 2019

This adds generic tests for all of the supported request method convenience methods on Faraday::Connection, and adds support for TRACE and CONNECT. Both methods were put in the METHODS_WITH_QUERY const based on the linked MDN doc pages for each method. The MDN pages state that requests using either method will not have a request body. However, you can override this with Faraday's block syntax.

res = conn.trace("/foo") do |req|
  req.body = "t r a c e"
end

NOTE: the CONNECT method was removed in #1104, before it was shipped in a full Faraday release.

@technoweenie
Copy link
Member Author

These shared tests uncovered a slight difference in behavior between #options and the other convenience methods that don't typically have a body (#get, #head, etc).

 1) making requests with #options behaves like method with query makes request with path
     Failure/Error: expect(res.env.request_body).to be_nil
     
       expected: nil
            got: ""
     Shared Example Group: "method with query" called from ./spec/faraday/connection_methods_spec.rb:70
     # ./spec/faraday/connection_methods_spec.rb:15:in `block (3 levels) in <top (required)>'

This is because Faraday::Env::MethodsWithBodies includes :options as of 2343a4e. It turns out that line moved files and classes several times, before ending up on Faraday::Env. A new Faraday::METHODS_WITH_BODY const was added to define the convenience helpers without options.

MDN says that OPTIONS requests have no body, so I'm comfortable with removing that from Faraday::Env::MethodsWithBodies.

@technoweenie
Copy link
Member Author

I'm going to merge the added tests to the existing shared examples in spec/support/shared_examples/adapter.rb so everything is together.

@iMacTia
Copy link
Member

iMacTia commented Feb 21, 2019

The presence of multiple sources for methods with bodies might be due to the parallel work on 0.x and 1.0 branches. We should indeed consolidate them and have a single source of truth (my suggestion would be on the main Faraday module, as they will be available everywhere this way.

@technoweenie
Copy link
Member Author

@iMacTia I had to tweak my change because I discovered the constants are used for slightly different purposes.

  • Faraday::METHODS_WITH_QUERY and Faraday::METHODS_WITH_BODY determine which convenience helpers are created on Faraday::Connection.
  • Faraday::Env::MethodsWithBodies determines which HTTP verbs expect to have a request body.

They're different, but both Faraday::Env::MethodsWithBodies and Faraday::METHODS_WITH_BODY should be the same (except one is a String Array, and the other is a Set of Symbols). However, If there was a Faraday::Env::MethodsWithQueries constant, it would have to include :options. But, we specifically do not want "options" in Faraday::METHODS_WITH_QUERY, because its implementation is custom.

It's a difference worth noting, but for now I will link Faraday::Env::MethodsWithBodies to Faraday::METHODS_WITH_BODY.

Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

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

Overall approved, just a couple of quick comments.

spec/faraday/adapter/excon_spec.rb Outdated Show resolved Hide resolved
spec/support/shared_examples/request_method.rb Outdated Show resolved Hide resolved
@iMacTia
Copy link
Member

iMacTia commented Feb 25, 2019

I've changed the only outstanding comment so it's good to go now 👍

@iMacTia iMacTia merged commit e00e900 into master Feb 25, 2019
@iMacTia iMacTia deleted the connection-verbs branch February 25, 2019 15:40
technoweenie added a commit that referenced this pull request Feb 25, 2019
@technoweenie
Copy link
Member Author

So clearly I've never implemented a Proxy or Websocket server (or fully read the MDN links above), but now I think that the CONNECT method should be removed from Faraday. I added it because I initially thought it was some other obscure method like TRACE or OPTIONS. A Faraday user isn't going to be implementing a proxy (each adapter has its own proxy support) or websocket (out of scope) client.

@technoweenie technoweenie mentioned this pull request Jun 25, 2019
3 tasks
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.

None yet

2 participants