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

Faraday #998

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Faraday #998

wants to merge 24 commits into from

Conversation

LukeIGS
Copy link

@LukeIGS LukeIGS commented Jan 4, 2024

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

Did you add tests for your changes?

Yes

Summary of changes

#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: savonrb/wasabi#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
Copy link
Member

pcai commented Jan 5, 2024

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

@pcai
Copy link
Member

pcai commented Jan 8, 2024

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

  • get Update to be compatible with Faraday wasabi#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
Copy link
Author

LukeIGS commented Jan 8, 2024

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
Copy link
Author

LukeIGS commented Jan 9, 2024

looks like code climate is happy now.

@LukeIGS
Copy link
Author

LukeIGS commented Jan 9, 2024

@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 pcai mentioned this pull request Feb 11, 2024
@pcai
Copy link
Member

pcai commented Feb 13, 2024

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
Copy link
Author

LukeIGS commented Feb 14, 2024

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
Copy link
Member

pcai commented Mar 18, 2024

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
Copy link
Author

LukeIGS commented Mar 18, 2024

I suppose we could just assign to nil

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

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.

None yet

2 participants