Faraday

[LukeIGS]

What kind of change is this? Conversion to faraday full of breaking changes

Did you add tests for your changes?

Yes

Summary of changes

https://github.com/savonrb/savon/issues/992

So far the biggest changes of note are that faraday doesn't support setting cert files except the ca, it doesn't support using an alternative ssl cipher and since it doesn't open cert files, it doesn't worry about using a password to decrypt them. The end user is expected to open and decrypt the client key using openssl and provide the result to faraday instead. As such I went ahead and deprecated those globals for now. If we wanted to keep this functionality, savon would have to handle those logic flows.

Faraday also uses various middlewares to enhance its auth capabilities. Currently I'm solving for NTLM in savon proper, but it might make more sense to write a middleware to handle NTLM auth, along the lines of how digest is handled.

Since these middlewares are not included out of box with faraday, I've added them as development dependencies and added errors when trying to load them if they aren't present. I used the same approach for NTLM auth, though rubyntlm is such a light weight gem, adding it as a solid dep would probably be acceptable too.

Worth noting that CI will not be able to build this unless the associated wasabi version is built and available, since it also relies on some very minor changes to support faraday. see: https://github.com/savonrb/wasabi/pull/116

The API for adapters is also slightly different since savon's adapters are often built using parameters, so I just take an array of options to forward

[pcai]

Thanks for putting this together. planning to take a look at it all this weekend

[pcai]

Thanks again for this PR. Here's a quick plan:

• get https://github.com/savonrb/wasabi/pull/116 merged in and cut a release of wasabi
• rebase this branch and retest, get everything green and document an upgrade path
• cut a prerelease and try to get some feedback from the wild
• do a final release for savon 3.0 after a few weeks

thoughts? let me know if I can help with any part in particular

[LukeIGS]

The biggest concern i have is whether NTLM is going to function correctly, i have a local server i can use to integration test it though so pre-release sounds like the best course of action.

Upgrade path is going to be interesting, for the most part the biggest thing is going to be for anyone who actually uses the encrypted cert file functionality, they'll need to open the cert file using openssl and provide it that way. https://docs.ruby-lang.org/en/3.2/OpenSSL.html#module-OpenSSL-label-Loading+an+Encrypted+Key

We could provide a helper internally that does this in order to preserve that API though.

I guess also people using custom adapters would need to rebuild their adapter on top of faraday's API.

I tried to preserve as much public api as i could.

[LukeIGS]

looks like code climate is happy now.

[LukeIGS]

@pcai do have a quick question for you, regarding the implementation of cookie parsing. Currently I have it set up so you can pass array'd cookie values under _: in the cookie hash, though i feel that's a little unclear, ex: https://github.com/savonrb/savon/pull/998/files#diff-04f40daa7e1b300e4e12f372a2632611ea7434aa135c935917652720a1fc3e7eR231-R235

Implemented at https://github.com/savonrb/savon/pull/998/files#diff-92754a50069d8bad42f3a8abb8d0be2cf0606271ebf4077ab1430787abb98263L102-L104

Can you think of a better way to handle this?

[pcai]

I'll need some time to delve into your cookie question, and may have some clarifying questions for you. I'm not familiar with this feature and don't work in savon day-to-day. Thanks for your patience.

I recently released a number of updates, including to wasabi, which will hopefully get us closer to passing all tests in this repo.

[LukeIGS]

Looking like we need to explicitly require mutex_m in the manifest for 3.4 compat, since it's been removed from stdlib..

I only use a small subset of savon's features as well and it's been one of those write and forget things, the only thing that sparked this project for me was a combination of a hackathon and getting annoyed at those rack warnings, plus a fear of repeating getting stuck on an old ruby version due to a dependency like with tiny_tds. There's really no hurry for me as long as 3.3 is a maintained ruby version though.

I should have time to do a test build though and see if ntlm behaves itself in the near future.

[pcai]

RE: your array cookie value question, I understand now: you allow a new magic key of literal "underscore" in the cookies hash. It allows passing an array of cookie flags like "HttpOnly" or "Secure". I'll need to think on this a bit, I would prefer NOT introducing a special case and relying on a key-value like "HttpOnly" => "" to get the desired result, but I don't think this is backwards compatible because this appears to be the current way to pass an empty cookie value, and that's probably a valid use case? hmmmm

[LukeIGS]

I suppose we could just assign to nil

{
  'some-cookie': 'choc-chip'
  'HttpOnly': nil
}

[eclectic-coding]

Checking in on the status of this PR.

I maintain a Rails application that uses savon and with the latest Ruby (3.3.3), I am getting a lot of Rack::Utils::HeaderHash errors. Our application works fine with Ruby 3.3.1.

I think this PR might resolve our issues and allow us to upgrade.

Just curious.

[LukeIGS]

Honestly forgotten about, i need to fix the cookie approach, I'll look into getting that done asap, thanks for the reminder. (Timeframe is probably within the next 3 or so days.)

[LukeIGS]

@pcai Looks like the test application relies on Rack::Auth::Digest::MD5 which is gone, so i'll have to update that too.

Seems like rack's opinion on the subject is that the upgrade path for digest auth is that there is no upgrade path for digest auth, and any tooling using it should simply drop support. I'm going to go that route for now...

[LukeIGS]

looks like 3.4 removes httpclient from stdlib... guess i'll add that to our gemspec as a dev gem.

[LukeIGS]

@pcai Alright, at this point we're probably ready to cut a prerelease. The deprecation specs just fail on 3.4 because of ascii encoding differences, and fixing that spec on 3.4 would break it on every other version.

[pcai]

thx @LukeIGS i will take a look today

@eclectic-coding please make sure you force rack < 3.1 - the recent release of rack, if bundled, forces an old and incompatible version of httpi 3.x to be resolved, which is the cause of your errors. This was fixed in httpi 4.0.3 https://github.com/savonrb/httpi/releases/tag/v4.0.3

I plan on making a minor bugfix release this wk to mainline to force httpi 4.x+

[pcai]

Unless I'm reading the logs wrong, its more than just an encoding difference - ruby-head returns the full namespace in addition to the method name

[eclectic-coding]

@pcai Thanks. I will give this a try for now.

Thank you @LukeIGS and @pcai for your work and commitment