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.

Sign up for GitHub

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

Change FormatParser.parse_http to follow a redirect #190

Merged
merged 5 commits into from
Feb 17, 2021
Merged

Change FormatParser.parse_http to follow a redirect #190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
db19c62
Change parse_http to follow redirects
fabioperrella Feb 12, 2021
021f38f
Add a comment about the default adapter
fabioperrella Feb 15, 2021
d87a0d2
Update lib/remote_io.rb
fabioperrella Feb 16, 2021
2cb7dfb
Update spec/remote_fetching_spec.rb
fabioperrella Feb 16, 2021
43a9fe4
Replace deprecated expect_any_instance_of by instance_double
fabioperrella Feb 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions format_parser.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ Gem::Specification.new do |spec|
spec.add_dependency 'exifr', '~> 1', '>= 1.3.8'
spec.add_dependency 'id3tag', '~> 0.14'
spec.add_dependency 'faraday', '~> 0.13'
spec.add_dependency 'faraday_middleware', '~> 0.14'
spec.add_dependency 'measurometer', '~> 1'

spec.add_development_dependency 'rspec', '~> 3.0'
Expand Down
8 changes: 7 additions & 1 deletion lib/remote_io.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ class InvalidRequest < UpstreamError
# @param uri[URI, String] the remote URL to obtain
def initialize(uri)
require 'faraday'
require 'faraday_middleware/response/follow_redirects'
@uri = uri
@pos = 0
@remote_size = false
Expand Down Expand Up @@ -78,7 +79,12 @@ def request_range(range)
# We use a GET and not a HEAD request followed by a GET because
# S3 does not allow HEAD requests if you only presigned your URL for GETs, so we
# combine the first GET of a segment and retrieving the size of the resource
response = Faraday.get(@uri, nil, range: 'bytes=%d-%d' % [range.begin, range.end])
conn = Faraday.new do |faraday|
faraday.use FaradayMiddleware::FollowRedirects
# we still need the default adapter, more details: https://blog.thecodewhisperer.com/permalink/losing-time-to-faraday
faraday.adapter Faraday.default_adapter
end
response = conn.get(@uri, nil, range: 'bytes=%d-%d' % [range.begin, range.end])

case response.status
when 200, 206
Expand Down
11 changes: 11 additions & 0 deletions spec/remote_fetching_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
}
@server = WEBrick::HTTPServer.new(options)
@server.mount '/', WEBrick::HTTPServlet::FileHandler, fixtures_dir
@server.mount_proc '/redirect' do |req, res|
res.status = 302
res.header['Location'] = req.path.sub('/redirect', '')
end
trap('INT') { @server.stop }
@server_thread = Thread.new { @server.start }
end
Expand Down Expand Up @@ -91,6 +95,13 @@
end
end

context 'when the server responds with a redirect' do
it 'follows the redirect' do
file_information = FormatParser.parse_http('http://localhost:9399/redirect/TIFF/test.tif')
expect(file_information.format).to eq(:tif)
end
end

after(:all) do
@server.stop
@server_thread.join(0.5)
Expand Down
51 changes: 38 additions & 13 deletions spec/remote_io_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
rio = described_class.new('https://images.invalid/img.jpg')

fake_resp = double(headers: {'Content-Range' => '10-109/2577'}, status: 206, body: 'This is the response')
expect(Faraday).to receive(:get).with('https://images.invalid/img.jpg', nil, range: 'bytes=10-109').and_return(fake_resp)
faraday_conn = instance_double(Faraday::Connection, get: fake_resp)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could extract the double in a let and the allow in a before, to DRY things up a bit 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried but I think I can't do it.

Faraday is loaded only after calling FormatParser::RemoteIO.new (here):

def initialize(uri)
  require 'faraday'

I think it was done this way to avoid loading the gem when it's not necessary, for example, if you parse only local files.

So, when I tried to reference the Faraday class in the before block, I got the error uninitialized constant Faraday.

I think that it's worth to require the gem before just to change the test.

Is it ok for you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right...

Looking at it better, the subject could also be DRYed out, and that would also require the gem. It is more work than this PR covers, so I'm also fine with moving on with how it is at the moment.

allow(Faraday).to receive(:new).and_return(faraday_conn)
expect(faraday_conn).to receive(:get).with('https://images.invalid/img.jpg', nil, range: 'bytes=10-109')

rio.seek(10)
read_result = rio.read(100)
Expand All @@ -18,7 +20,9 @@
rio = described_class.new('https://images.invalid/img.jpg')

fake_resp = double(headers: {'Content-Range' => '10-109/2577'}, status: 200, body: 'This is the response')
expect(Faraday).to receive(:get).with('https://images.invalid/img.jpg', nil, range: 'bytes=10-109').and_return(fake_resp)
faraday_conn = instance_double(Faraday::Connection, get: fake_resp)
allow(Faraday).to receive(:new).and_return(faraday_conn)
expect(faraday_conn).to receive(:get).with('https://images.invalid/img.jpg', nil, range: 'bytes=10-109')

rio.seek(10)
read_result = rio.read(100)
Expand All @@ -29,7 +33,9 @@
rio = described_class.new('https://images.invalid/img.jpg')

fake_resp = double(headers: {}, status: 403, body: 'Please log in')
expect(Faraday).to receive(:get).with('https://images.invalid/img.jpg', nil, range: 'bytes=100-199').and_return(fake_resp)
faraday_conn = instance_double(Faraday::Connection, get: fake_resp)
allow(Faraday).to receive(:new).and_return(faraday_conn)
expect(faraday_conn).to receive(:get).with('https://images.invalid/img.jpg', nil, range: 'bytes=100-199')

rio.seek(100)
expect { rio.read(100) }.to raise_error(/replied with a 403 and refused/)
Expand All @@ -39,7 +45,9 @@
rio = described_class.new('https://images.invalid/img.jpg')

fake_resp = double(headers: {}, status: 416, body: 'You stepped off the ledge of the range')
expect(Faraday).to receive(:get).with('https://images.invalid/img.jpg', nil, range: 'bytes=100-199').and_return(fake_resp)
faraday_conn = instance_double(Faraday::Connection, get: fake_resp)
allow(Faraday).to receive(:new).and_return(faraday_conn)
expect(faraday_conn).to receive(:get).with('https://images.invalid/img.jpg', nil, range: 'bytes=100-199')

rio.seek(100)
expect(rio.read(100)).to be_nil
Expand All @@ -49,7 +57,9 @@
rio = described_class.new('https://images.invalid/img.jpg')

fake_resp = double(headers: {}, status: 403, body: 'Please log in')
expect(Faraday).to receive(:get).with('https://images.invalid/img.jpg', nil, range: 'bytes=100-199').and_return(fake_resp)
faraday_conn = instance_double(Faraday::Connection, get: fake_resp)
allow(Faraday).to receive(:new).and_return(faraday_conn)
expect(faraday_conn).to receive(:get).with('https://images.invalid/img.jpg', nil, range: 'bytes=100-199')

rio.seek(100)
# rubocop: disable Lint/AmbiguousBlockAssociation
Expand All @@ -60,7 +70,9 @@
rio = described_class.new('https://images.invalid/img.jpg')

fake_resp = double(headers: {}, status: 416, body: 'You jumped off the end of the file maam')
expect(Faraday).to receive(:get).with('https://images.invalid/img.jpg', nil, range: 'bytes=100-199').and_return(fake_resp)
faraday_conn = instance_double(Faraday::Connection, get: fake_resp)
allow(Faraday).to receive(:new).and_return(faraday_conn)
expect(faraday_conn).to receive(:get).with('https://images.invalid/img.jpg', nil, range: 'bytes=100-199')

rio.seek(100)
expect(rio.read(100)).to be_nil
Expand All @@ -69,15 +81,24 @@
it 'does not overwrite size when the range cannot be satisfied and the response is 416' do
rio = described_class.new('https://images.invalid/img.jpg')

fake_resp = double(headers: {'Content-Range' => 'bytes 0-0/13'}, status: 206, body: 'a')
expect(Faraday).to receive(:get).with('https://images.invalid/img.jpg', nil, range: 'bytes=0-0').and_return(fake_resp)
fake_resp1 = double(headers: {'Content-Range' => 'bytes 0-0/13'}, status: 206, body: 'a')
fake_resp2 = double(headers: {}, status: 416, body: 'You jumped off the end of the file maam')

faraday_conn = instance_double(Faraday::Connection)
allow(Faraday).to receive(:new).and_return(faraday_conn)
expect(faraday_conn).to receive(:get)
.with('https://images.invalid/img.jpg', nil, range: 'bytes=0-0')
.ordered
.and_return(fake_resp1)
expect(faraday_conn).to receive(:get)
.with('https://images.invalid/img.jpg', nil, range: 'bytes=100-199')
.ordered
.and_return(fake_resp2)

rio.read(1)

expect(rio.size).to eq(13)

fake_resp = double(headers: {}, status: 416, body: 'You jumped off the end of the file maam')
expect(Faraday).to receive(:get).with('https://images.invalid/img.jpg', nil, range: 'bytes=100-199').and_return(fake_resp)

rio.seek(100)
expect(rio.read(100)).to be_nil

Expand All @@ -88,7 +109,9 @@
rio = described_class.new('https://images.invalid/img.jpg')

fake_resp = double(headers: {}, status: 502, body: 'Guru meditation')
expect(Faraday).to receive(:get).with('https://images.invalid/img.jpg', nil, range: 'bytes=100-199').and_return(fake_resp)
faraday_conn = instance_double(Faraday::Connection, get: fake_resp)
allow(Faraday).to receive(:new).and_return(faraday_conn)
expect(faraday_conn).to receive(:get).with('https://images.invalid/img.jpg', nil, range: 'bytes=100-199')

rio.seek(100)
expect { rio.read(100) }.to raise_error(/replied with a 502 and we might want to retry/)
Expand All @@ -100,7 +123,9 @@
expect(rio.pos).to eq(0)

fake_resp = double(headers: {'Content-Range' => 'bytes 0-0/13'}, status: 206, body: 'a')
expect(Faraday).to receive(:get).with('https://images.invalid/img.jpg', nil, range: 'bytes=0-0').and_return(fake_resp)
faraday_conn = instance_double(Faraday::Connection, get: fake_resp)
allow(Faraday).to receive(:new).and_return(faraday_conn)
expect(faraday_conn).to receive(:get).with('https://images.invalid/img.jpg', nil, range: 'bytes=0-0')
rio.read(1)

expect(rio.pos).to eq(1)
Expand Down