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

Allow to pass the adapter option in globals, pass it to HTTPI when performing SOAP calls. #566

Closed
wants to merge 2 commits into from

Conversation

Envek
Copy link
Contributor

@Envek Envek commented Apr 27, 2014

It will be useful for those people who need to use different HTTPI adapters for different services, see #564

I don't know, how to test this, please tell me any ideas and suggestions, I'll write tests and append it to this request.

Closes #564
Requires savonrb/wasabi#44 to be merged in and published with version bump (included).

…performing SOAP calls.

There is no tests yet.
@tjarratt
Copy link
Contributor

Hey @Envek I'm not entirely sure how you might test this either. I'll think about it for a bit and get back to you.

I'm not opposed to this pull request -- in fact I'm a little surprised it wasn't possible to do this! However, I did notice when I read the documentation for HTTPI that you can globally set the adapter, as you identified in #544

e.g.:

HTTPI.adapter = :curb

If you wanted to work around this issue, you'd have to stash off a reference to the default adapter and then set it again, like so:

original_adapter = HTTPI::Adapter.use # probably :httpclient or :net_http, whichever can be loaded first
HTTPI.adapter = :my_tricky_adapter
@tricky_service.call(:baz, message: {param: '1'})
HTTPI.adapter = original_adapter
@usual_service.call(:huh, message: {param: '2'})

However, I have to wonder if this is a good idea. It's kind of hard for me to personally imagine a case where one HTTP adapter wouldn't work for multiple services, but perhaps I'm underestimating how hard it can be to work with SOAP services. Tentatively 👍 in favor of merging this while I try to be clever and think about how to test these changes.

@tjarratt
Copy link
Contributor

Added some discussion to savonrb/wasabi#44 re: testing. Unsure how well that pattern applies here, because that also tests the same behavior in Wasabi, but I think it helps drive out the use case that you care about here.

@Envek
Copy link
Contributor Author

Envek commented Apr 29, 2014

Let me explain little further.

There is one SOAP service we need to integrate with. In staging environment we're integrating with staging version of this service, in production — with production version. Staging version works via simple HTTP with basic auth, but production one works via HTTPS with GOST encryption and client certificate authentication.

And the GOST encryption is a issue — Ruby OpenSSL module just doesn't understand GOST private key, but system OpenSSL does, so I've found a workaround with openssl s_client command, and wrote (and published) HTTPI adapter around it. This command line tool is pretty slow and unreliable, but for now it's the only solution that works and we're going to production tomorrow 😄 Moreover, this tool just can't connect to usual HTTP servers (without SSL).

We need to integrate with a lot of SOAP services, so for other services it's better to use default adapters.

I'm personally dislike usage of global calls to HTTPI.adapter, it's just unsightly. Also, I just don't understand Ruby internals enough, would such calls impact other instances of Savon in other threads in case of thread-based servers like Puma or not? We're using Unicorn and everything should be OK, but I'm still little afraid anyway.

@tjarratt
Copy link
Contributor

Wow. I can understand better now the perspective that you're coming from. It really does seem that an individual instance of a Savon::Client must have a reference to the adapter it should use, for precisely the threading concerns you've raised.

I'll do my best to get these PRs merged in as soon as we get some tests in. Could be that I find time tomorrow to backfill the tests for Wasabi and release a new version of that, but it really depends on work and whether anything crazy comes up.

Hope your push to go to production today went well!

@Envek
Copy link
Contributor Author

Envek commented Apr 30, 2014

We're using code directly from my fork's branches, so there is no need for such a haste. I'll write tests a bit later (probably on the weekend). Thanks for the examples and help!

@Envek
Copy link
Contributor Author

Envek commented May 3, 2014

I've added two tests for this. What do you think?

To run these tests you need either an updated wasabi (with savonrb/wasabi#44 merged in) or you can temporarily specify in gemspec gem wasabi_with_adapter (temporary gem I've published due to the dependency in our internal gem, as we can't tell gemspec to use code from VCS).

Also, some time ago I've pushed some tests for wasabi too.

@tjarratt
Copy link
Contributor

tjarratt commented May 4, 2014

Merged this from the command line because of some strange git history issues. This should be in Savon 2.5.0 on rubygems.org now, and on master as of 6bd65bb. Thanks so much for the pull request, @Envek!

@tjarratt tjarratt closed this May 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants