Allow Faraday 2.x #91
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…raday_follow-redirects gem The new gem is compatible with both faraday 1 and 2, while faraday-middleware (which included a redirect middleware) has a gemspec that does not allow faraday 2.
…encoding On upgrade to faraday 2, HTTP responses can come back in specific encodings per the content-type of response. This effected one of our tests, that is using a live query to google.com (probably not a great idea), that comes back ISO-8859-1. But it actually would have failed if UTF-8 too, because we're trying to gsub some intentionally illegal bytes. I am not sure if the approach to encoding is totally sound in this gem in general -- XML does have to be Unicode legally, but can be UTF-8 or -16, this strip routine may be assuming UTF8? But not actually properly making it with the ruby UTF8 encoding? But this is just an attempt to change as little as possible leaving as backward compat as possible while passing tests in faraday 2; and resolving the specific problem of the strip_invalid_utf_8_chars failing when response comes back with ruby encoding tag from faraday 2.
|
@mcritchlow and @orangewolf anyone interested in reviewing? If I don't hear from anyone, I may do a merge myself. |
orangewolf
reviewed
Apr 12, 2022
lib/oai/client.rb
Outdated
| @@ -330,14 +331,27 @@ def parse_date(value) | |||
| # Regex is from WebCollab: | |||
| # http://webcollab.sourceforge.net/unicode.html | |||
| def strip_invalid_utf_8_chars(xml) | |||
| xml && xml.gsub(/[\x00-\x08\x10\x0B\x0C\x0E-\x19\x7F] | |||
| return xml unless xml | |||
There was a problem hiding this comment.
return xml if xml ? this seems like it only ever returns nil
There was a problem hiding this comment.
correct, it exits the method early to return nil unless xml is present. Similar to what the xml && did in the one-liner version previously. Do you think it would read better return nil unless xml? Or other?
There was a problem hiding this comment.
that reads more intentional and direct to me, but its a small quibble as long as this is doing what is intended.
orangewolf
approved these changes
Apr 13, 2022
|
Thanks @orangewolf! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #89
Allow Faraday 2.x, by switching from faraday-middleware to the new faraday_follow-redirects gem
By switching from faraday-middleware to the new faraday_follow-redirects gem
The new gem is compatible with both faraday 1 and 2, while faraday-middleware (which included a redirect middleware) has a gemspec that does not allow faraday 2.
I manually tested on faraday 1 by editing Gemfile locally -- it passes. I'm not sure if we want to regularly/automated test on both faraday 1 and 2? Right now we have no automated CI at all, see #87
Strip invalid chars without raising encoding exception regardless of encoding
On upgrade to faraday 2, HTTP responses can come back in specific encodings per the content-type of response. This effected one of our tests, that is using a live query to google.com (probably not a great idea), that comes back ISO-8859-1. But it actually would have failed if UTF-8 too, because we're trying to gsub some intentionally illegal bytes. I am not sure if the approach to encoding is totally sound in this gem in general -- XML does have to be Unicode legally, but can be UTF-8 or -16, this strip routine may be assuming UTF8? But not actually properly making it with the ruby UTF8 encoding?
But this is just an attempt to change as little as possible leaving as backward compat as possible while passing tests in faraday 2; and resolving the specific problem of the strip_invalid_utf_8_chars failing when response comes back with ruby encoding tag from faraday 2.