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

No method for OPTIONS HTTP verb #305

Closed
gjtorikian opened this issue Sep 23, 2013 · 18 comments
Closed

No method for OPTIONS HTTP verb #305

gjtorikian opened this issue Sep 23, 2013 · 18 comments
Labels
Projects
Milestone

Comments

@gjtorikian
Copy link

Shouldn't it be included in this array?

https://github.com/lostisland/faraday/blob/master/lib/faraday/connection.rb#L137

@mislav
Copy link
Contributor

mislav commented Sep 25, 2013

Unfortunately, Connection already has the options accessor so we can't repurpose it to make an OPTIONS request. You'll have to use run_request(:options)

@mislav mislav closed this as completed Sep 25, 2013
@gjtorikian
Copy link
Author

Perhaps it is a naive assumption but wouldn't it be easier to remap :options to, I dunno, :configuration, rather than have a different usage pattern?

@mislav
Copy link
Contributor

mislav commented Sep 25, 2013

I don't want to change an established API to support a rarely used HTTP
method that's easily accessible through run_request.

@sferik
Copy link
Member

sferik commented Sep 26, 2013

IMHO, this decision should be reconsidered. OPTIONS is a valid HTTP method name and run_request is not always a suitable alternative. If we're going to break the public API, it's better to do it now, before we release 1.0, than regret the decision later.

@mislav
Copy link
Contributor

mislav commented Sep 26, 2013

Why is run_request not always a suitable alternative?

For lower-level library authors that build on top of Faraday, I would
actually recommend using run_request over get, post methods.

@sferik
Copy link
Member

sferik commented Sep 26, 2013

@mislav #run_request doesn't take a block, like #get, #post, etc. Take a look at how I’m using Faraday in Twitter::REST::Client#request: https://github.com/sferik/twitter/blob/master/lib/twitter/rest/client.rb#L130-L144

@mislav
Copy link
Contributor

mislav commented Sep 26, 2013

#run_request doesn't take a block, like #get, #post, etc.

What makes you think so?

Your use case is a perfect example when to use run_request, i.e. to avoid send.

@sferik
Copy link
Member

sferik commented Sep 26, 2013

Now I remember why I don’t like #run_request. I was actually using it at one point in the Twitter::Client#request code but removed it as part of a refactoring: sferik/twitter-ruby@2d70b64. I think you must concede, the code is much cleaner with #send (I wish this was not the case).

With #run_request, I had to worry about whether the request was a PUT or POST and set the request body vs. query params for GET, DELETE, etc. With the verb methods, I can simply pass them a hash and they know how to handle it correctly. IMHO, this is a much friendlier interface than with #run_request.

I’d challenge you to submit a pull request to the twitter gem that replaces #send with #run_request that results in less complex code (according to Code Climate or some other static analysis).


In defense of the OPTIONS method, it's very hard to guess the future popularity of HTTP verbs. In HTTP 1.0, there was only GET and POST. In 1999, more verbs were added in HTTP 1.1, but they were mostly ignored until Rails 2 added support for PUT and DELETE in resource routes. Now, in Rails 4, PATCH is supported alongside PUT. A few years ago, you might have (correctly) claimed that “PATCH is not very popular” and therefore doesn’t require first-class support. Today, all new API servers written in Rails support PATCH, including the GitHub API, which supported PATCH before Rails 4 was released. The GitHub API also supports OPTIONS for Cross Origin Resource Sharing (CORS). This use of OPTIONS may not be popular yet but it will become popular almost overnight if CORS is added as a default in Rails 5. If this happens, I think you will regret brushing off this issue.

HTTP verbs are important reserved words for an HTTP library. There are only a few of them and in my opinion, we should support them all as first-class citizens in Faraday 1.0, even if that means breaking things along the way.

@mislav mislav reopened this Sep 26, 2013
@crazymykl
Copy link

CORS is a very important use case. I would be disappointed if 1.0 ships without OPTIONS as a first class verb.

@nhocki
Copy link

nhocki commented Feb 5, 2015

I am terribly sorry to bump this, but any agreement on supporting options as a method for OPTIONS requests? I'd be more than happy to submit a pull request to make this happen.

@mislav
Copy link
Contributor

mislav commented Feb 7, 2015

I'm still not sold on the idea. What would the current options method be called then?

Other methods that we support are verbs: get, post, put, delete. It makes it easy to understand that some action is happening when you call them. options wouldn't be a verb and as such I don't think it would make a nice shorthand method. If people are using OPTIONS a ton in day-to-day code and can't bear to use run_request for some reason (I'd like to hear that reason!) then I would suggest naming the new method http_options to indicate that this is a HTTP request method.

@nhocki
Copy link

nhocki commented Feb 7, 2015

The argument of "the other methods are verbs" should not matter. They are not methods in Faraday because they are verbs, those method exist because they are HTTP methods. The same should happen for options IMO.

If this is the reason for not having options, then head should not be defined, as "head" in HTTP is not about "going", the verb.

I totally get the concern about breaking the API with options, but it's a big surprise that it is not "supported". It's way more consistent to have all HTTP methods handled the same.

And about not using run_request, like it was pointed before, the code without it is much much cleaner.

It's still your call (obviously :-) ) to decide to support this or not, but I'd love to change your mind about it, and if it's not going to be supported, that it's for something else other than "options is not a verb". 

@bithavoc
Copy link

bithavoc commented Feb 8, 2015

+1 @nhocki

@dwo
Copy link

dwo commented Jun 8, 2015

This caught me by surprise today. All of the examples with conn.get, conn.post, conn.delete etc. lead me to believe the obvious way to do an OPTIONS request was conn.options.

Perhaps we could document this inconsistency and its run_request workaround in the Faraday::Connection rdocs or the README? I'm happy to compose a pull request.

@guyisra
Copy link

guyisra commented Feb 9, 2016

+1

Why isn't options the same as GET, POST, DELETE?

@iMacTia iMacTia added this to the v1.0 milestone Jul 21, 2017
@iMacTia
Copy link
Member

iMacTia commented Jul 21, 2017

Even though I personally agree with @mislav, I can't ignore @sferik opinion and the community feedback. I checked the code and noticed that options is simply an attr_reader, plus it's being used mainly by tests.
However, we're talking about a public API that could potentially break some implementations being changed, hence this will be addressed only in Faraday 1.0
Until then, happy to re-discuss this if anyone is against it

edtjones pushed a commit to errorstudio/her that referenced this issue Nov 3, 2017
…run_request() method instead of the dynamically-generated method associated with the verb name (Faraday.options is a different thing - see lostisland/faraday#305)
@escoberik
Copy link
Contributor

I think conn.options should totally be a thing. It's existence is heavily by the existence of conn.get, conn.post, etc. But since it would break backwards compatibility, v1.0 sounds like a nice target to put this into.

@iMacTia iMacTia added this to To Do in v1.0 Jan 31, 2018
@iMacTia iMacTia added this to To do in v2.0 via automation Feb 20, 2019
@iMacTia iMacTia removed this from To Do in v1.0 Feb 20, 2019
technoweenie added a commit that referenced this issue Feb 20, 2019
v2.0 automation moved this from To do to Done Feb 20, 2019
@iMacTia iMacTia added this to Backlog in v1.0 via automation Feb 20, 2019
@iMacTia iMacTia removed this from Done in v2.0 Feb 20, 2019
@technoweenie technoweenie moved this from Backlog to In progress in v1.0 Feb 21, 2019
@technoweenie
Copy link
Member

Good news, everyone! I implemented this by overloading #options to support the new behavior, while maintaining the existing behavior. #857

@technoweenie technoweenie moved this from In progress to Done in v1.0 Feb 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v1.0
Done
Development

No branches or pull requests