savonrb / httpi

Common interface for Ruby's HTTP clients
http://httpirb.com
MIT License
300 stars 150 forks source link

Installing Twitter Gem breaks Savon due to HTTPI LOAD_ORDER preferring http over net_http #156

Closed lustremedia closed 9 years ago

lustremedia commented 9 years ago

Using savon (2.11.1) Using httpi (2.4.1) Using twitter (5.14.0) Using http (0.6.4)

I just installed the twitter Gem (https://github.com/sferik/twitter) which has http Gem as dependency while using the savon Gem .

HTTPI then preferred http over the working net_http (this seems to be the default in Rails) in the LOAD_ORDER which then breaks Savon's requests with following error: NoMethodError (undefined methodheaders' for #):`

After installing httpclient everything worked fine again due to the LOAD_ORDER https://github.com/savonrb/httpi/blob/master/lib/httpi/adapter.rb#L16

This seems to be related to https://github.com/savonrb/savon/issues/488

The error messages are not very clear and the stacktrace for debugging was really non-existing but pointing to the Savon API calls. I think this ought to be remedied in some way.

lustremedia commented 9 years ago

Also most probably relates to #16 which would have been a time saver if the right error message would have been thrown!

mikecmpbll commented 9 years ago

relates more to #148. You can change the adapter that Savon uses though, as explained in comments on other issue:

client = Savon.client(
      ...
      :adapter => :net_http
    )

Seems as though this headers issue is an issue with the 'http' adapter; not sure what version of the 'http' gem @Connorhd developed the adapter against?

lustremedia commented 9 years ago

@mikecmpbll what is the recommended adapter? Couldn't the team of savon just select one as dependency and be done with it? It certainly is the abstraction that caused the error but I am sure there is a reason for that.

The time consuming part was making sense of the error message and the close to "non-existing stacktrace" about what caused the issue and from what gem it stemmed from.

and yeah #148 seems to be the issue, but not under the update condition. However savonrb/savon#488 showed me the way on how to resolve it and how to make sense of it all.

mikecmpbll commented 9 years ago

@lustremedia the LOAD_ORDER that you mentioned specifies HTTPI's adapter preference order, however you can use whichever adapter works best for you. HTTPI is a single interface to the myriad of http adapters available. The reason for your error appears to be an issue with the adapter for the http.rb gem, but I haven't had time yet to look in to it.

The error almost certainly stems from this line of the adapter, though: https://github.com/savonrb/httpi/blob/master/lib/httpi/adapter/http.rb#L72 so it should be fairly easy for someone to work out what's up.

The update itself isn't a condition in #148–the conditions are version 2.4.1 of httpi which has the http.rb support, and having the http gem installed.

FWIW, I understand that Savon 3 will be dropping the HTTPI dependency and leaving it up to Savon end-users to extend their favourite HTTP library.

lustremedia commented 9 years ago

@mikecmpbll As you could see from my incident description, I resolved the issue but wanted to let you guys know about. I searched the web and even this repo but #148 did not come up for me.

Documentation about :adapter switch in savon is also nowhere to be found. Thanks for taking your time however to point out more about the issue and dropping the HTTPI dependency would be less headaches in the future I think.

Cheers

rogerleite commented 9 years ago

Hi @lustremedia. Sorry for the trouble. I don't know what httpi can do to help on these cases. Maybe savon could log what adapter is using or something like this.

@mikeantonelli thanks for helping! :+1:

mikecmpbll commented 9 years ago

@lustremedia thanks for reporting it, definitely something here that needs to be fixed so I'll see if I can check it out this evening.

and yep, for some reason the adapter option is not documented for Savon..

lustremedia commented 9 years ago

Hi @rogerleite As I suggested: pick a single production ready adapter for savon and ship it with savon as dependency, but keep the option to change the adapter in the client if the developer wants something different.

In that case, a well tested adapter is always present as default. If the developer chooses to use a different adapter but the adapter is broken he/she can report a bug with the adapter and savon would not be affected, but could point to the default adapter.

@mikecmpbll :+1: Thanks for the help!

And thank you both for the great and fast responses! :+1:

lustremedia commented 9 years ago

@rogerleite also you could have the option to reject support for less active adapters and have less troubles.

rogerleite commented 9 years ago

@lustremedia i agree with you. Seeing https://github.com/savonrb/savon/issues/488, this suggestion can avoid a lot of problem, you can open an issue on savon.

Thanks for reporting! I'm copying @tjarratt to be aware of this.

lustremedia commented 9 years ago

@rogerleite see savonrb/savon#710 cheers

Connorhd commented 9 years ago

Sorry for causing pain here since its my adapter thats not working, the problem is you have an older version of the http gem. If you update to a more recent version it should work.

lustremedia commented 9 years ago

@Connorhd np! See, if I get the newest http gem it might break the twitter gem since the twitter gem has the http gem as dependency locked at a certain version which seems to work for them. Maybe they have already updated, but this triggered the savon LOAD_ORDER and broke Savon ...

Connorhd commented 9 years ago

It looks like the next version of the twitter gem uses newer http, obviously that doesn't help you now :(

I wonder if httpi should allow adapters to specify extra requirements to be enabled, so a minimum version could be given.

lustremedia commented 9 years ago

@Connorhd I think if part of your code plays such an important role as to be a showstopper, maybe it is time for savon to decide on one http client and be done with it, rather than supporting all http clients and trying to figure out a LOAD_ORDER of the least client that is going to break.

mikecmpbll commented 9 years ago

That's a discussion for over at savon. As far as HTTPI is concerned, I agree that adapters should be able to specify version dependency for their respective libraries.