Fix #280 - open_buffer mangles the content of the buffer it is given.
#360
Conversation
This now actually extracts the path from the IO if one is passed in.
It's already required in zip.rb.
Things are now more carefully set up, and if a buffer is passed in which represents a file that already exists then this is taken into account. All initialization is now done in File.new, rather than being split between there and File.open_buffer. This has also needed a bit of a re-write of Zip::File.initialize. I've tried to bring some logic to it as a result, and have added comments to explain what is now happening.
StringIO objects created within File.open_buffer were not being switched into binmode, but those passed in were. Fix this inconsistency and add a test.
There was a problem hiding this comment.
Thanks for putting this together. It looks good, and there are several open tickets about this method, so it's probably worth fixing, but I have a few questions.
Overall, this open_buffer method seems a bit confusing. It seems to have started its life as a way of handling in-memory zip files #49 but has grown in scope to accept arbitrary IOs. This makes the name a bit confusing IMHO (is it still opening a buffer if you pass a file IO?). It looks like InputStream handles IOs through open/new:
rubyzip/lib/zip/input_stream.rb
Lines 50 to 52 in 2ffbc3b
rubyzip/lib/zip/input_stream.rb
Lines 95 to 96 in 2ffbc3b
and does the file / IO detection in new. This interface seems a bit inconsistent (which happened before this PR). Whether there is a way of doing the same thing here without breaking compatibility with what's already there, I am not yet sure.
I'll leave this open for comment for another week or so (or whenever I get some time to come back to it). If I don't hear back, I'll probably merge it and then do another PR to see if I can tweak it.
| @comment = '' | ||
| @create = create ? true : false # allow any truthy value to mean true | ||
| if !buffer && ::File.size?(file_name) | ||
|
|
||
| if ::File.size?(@name.to_s) |
There was a problem hiding this comment.
If @name is a StringIO, @name.to_s will return something like "#<StringIO:0x00007f98b09766c0>". It's unlikely that such a file exists, of course, but hitting the file system to find this out seems a bit odd. Could it be reorganised to not check the file system at all when buffer is true?
| @@ -107,13 +114,14 @@ def test_open_buffer_with_stringio | |||
| def test_close_buffer_with_stringio | |||
| string_io = StringIO.new File.read('test/data/rubycode.zip') | |||
| zf = ::Zip::File.open_buffer string_io | |||
| assert(zf.close || true) # Poor man's refute_raises | |||
| assert_nil zf.close | |||
| end | |||
|
|
|||
| def test_close_buffer_with_io | |||
| f = File.open('test/data/rubycode.zip') | |||
| super() | ||
| @name = file_name | ||
| @name = path_or_io.respond_to?(:path) ? path_or_io.path : path_or_io |
There was a problem hiding this comment.
I think this is good... but it seems a bit optional. Was it aimed at solving a particular problem?
|
Thanks for picking this up! It's been a while since I looked at this, but I'll try and find time to get it all back in my head and respond properly soon. As I remember it though, it turned into one of those problems that uncovered all sorts of other problems, and was originally a piece of code that was rather unclear as to what was going on 😬 |
|
I reviewed this again today, and I think it is good to merge. Thanks again for the fix. |
|
Sorry for not getting round to looking at it again myself; a change of role at work has left little time for extra-curricular coding recently, but I hope to get back into it again soon. Thanks for merging, and thanks for picking up maintaining this gem @jdleesmiller. |
|
Definitely get that :) I was able to progress this mainly because we had a hackathon day at my company, Overleaf. I will work it up into a new release. |
|
Oh cool. I'm a big fan of Overleaf! |
Things are now more carefully set up, and if a buffer is passed in which represents a file that already exists then this is taken into account. All initialization is now done in File.new, rather than being split between there and File.open_buffer.
This has also needed a bit of a re-write of Zip::File.initialize. I've tried to bring some logic to it as a result, and have added comments to explain what is now happening.
This PR includes some other related fixes too. All tests pass.