Set OutputStream.write_buffer's default buffer to binmode #439

henkesn · 2020-02-19T13:23:28Z

Fixes #438

coveralls · 2020-02-19T13:30:29Z

Coverage increased (+0.002%) to 95.769% when pulling 66324a7 on henkeinfo:binary-outstream-buffer into 8d91d00 on rubyzip:master.

hainesr

Thanks for this. Just one comment/suggestion from me.

hainesr · 2020-02-19T17:22:55Z

lib/zip/output_stream.rb

@@ -57,7 +57,7 @@ def open(file_name, encrypter = nil)
      end

      # Same as #open but writes to a filestream instead
-      def write_buffer(io = ::StringIO.new(''), encrypter = nil)
+      def write_buffer(io = ::StringIO.new('').binmode, encrypter = nil)


What if we pass an IO in here that hasn't been set to binmode? Do we still see the behaviour you're trying to avoid? This change only sets a newly created StringIO to binmode. Is this a problem?

How about:

Suggested change

def write_buffer(io = ::StringIO.new('').binmode, encrypter = nil)

def write_buffer(io = ::StringIO.new(''), encrypter = nil)

io.binmode if io.respond_to?(:binmode)

Which would match behaviour in ::Zip::File.open_buffer?

I found another location where the binmode would be set if supported. I think this is a cleaner solution:
https://github.com/henkeinfo/rubyzip/blob/5ce4e13ddd33443f01fa33c8208423b76d99fce3/lib/zip/file.rb#L151
Yes, you're right with your comment. I rewrote the write_buffer to behave like the source above and also fixed the same thing in yet another method.

Yes I spotted that when I was looking through File and wondered why it was inconsistent. Thanks for the catch.

This looks good to me.

hainesr

This looks like a good patch to me.

@jdleesmiller I can't merge this myself, obvs, so does it look OK to you too?

jdleesmiller

Thanks for the patch! A few comments inline.

In my case this is UTF-8, which produces encoding errors in encoding sensitive use cases.

I'd be interested to see what the actual use case was that caused the error --- if we can get a test that failed (pre-fix) in the way that motivated you to open the issue, I think that will be more likely to catch future regressions.

There have been binmode problems in the past, notably #119 / #201, which resulted in this line:

rubyzip/lib/zip/file.rb

Lines 150 to 151 in 8d91d00

    
           # https://github.com/rubyzip/rubyzip/issues/119 
        
           io.binmode if io.respond_to?(:binmode)

However, all the tests still pass even if that line is deleted, so I feel like we have already have some gaps in this area.

lib/zip/file.rb

test/output_stream_test.rb

Co-Authored-By: John Lees-Miller <jdleesmiller@gmail.com>

henkesn · 2020-03-02T11:17:15Z

Thank you for your feedback @jdleesmiller. My use case was an extension in my code where i filtered empty strings before storing to a database (String#presence). I already faced some issues with ActiveRecord in the past, when trying to save invalid UTF8 strings even for BLOB columns. But that's already some time ago and I neither know how to reproduce nor if that issues persist with newer versions.
Regarding the next line

rubyzip/lib/zip/file.rb

Lines 150 to 151 in 8d91d00

    
           # https://github.com/rubyzip/rubyzip/issues/119 
        
           io.binmode if io.respond_to?(:binmode)

perhaps this line now is also superfluous because it internally calls OutputStream#write_buffer with my fix. But the yield call is done after this line and before the OutputStream#write_buffer, so removing this line potentially could break things.

jdleesmiller

OK, thanks for the updates. Looks good.

I think you're right that removing the existing binmode line would change what the user might get in the yield, so it's probably best to keep it.

jdleesmiller · 2020-03-14T10:30:09Z

test/output_stream_test.rb

@@ -6,6 +6,8 @@ class ZipOutputStreamTest < MiniTest::Test
  TEST_ZIP = TestZipFile::TEST_ZIP2.clone
  TEST_ZIP.zip_name = 'test/data/generated/output.zip'

+  ASCII8BIT = 'ASCII-8BIT'


This constant can now be removed, but I'll tidy it up post-merge.

kitsunde · 2020-07-20T12:58:25Z

This appears to have affected an upstream gem because of the encoding changing. weshatheleopard/rubyXL#377

* rubyzip 2.3.0からエンコーディング結果がASCII-8BITに変更された * rubyzip/rubyzip#439

hainesr requested changes Feb 19, 2020

View reviewed changes

henkesn force-pushed the binary-outstream-buffer branch from f0526d7 to b0ee268 Compare February 19, 2020 17:48

Set buffers to binmode by default

b0ee268

hainesr approved these changes Feb 19, 2020

View reviewed changes

jdleesmiller reviewed Feb 29, 2020

View reviewed changes

lib/zip/file.rb Outdated Show resolved Hide resolved

test/output_stream_test.rb Outdated Show resolved Hide resolved

henkesn and others added 2 commits March 2, 2020 11:00

Use existing constant for ASCII_8BIT

e33c07a

Co-Authored-By: John Lees-Miller <jdleesmiller@gmail.com>

Remove duplicate binmode call

66324a7

jdleesmiller approved these changes Mar 14, 2020

View reviewed changes

jdleesmiller merged commit b231b28 into rubyzip:master Mar 14, 2020

jdleesmiller added a commit that referenced this pull request Mar 14, 2020

Remove unused constant from #439

4c789c2

jdleesmiller added a commit that referenced this pull request Mar 14, 2020

Update changelog for #439 and #440

516941b

henkesn deleted the binary-outstream-buffer branch March 16, 2020 10:46

yn-misaki added a commit to medley-inc/ruby-xlsx-templater that referenced this pull request Apr 4, 2024

Update file open mode to binary

b46b98e

* rubyzip 2.3.0からエンコーディング結果がASCII-8BITに変更された * rubyzip/rubyzip#439

yn-misaki added a commit to medley-inc/ruby-docx-templater that referenced this pull request Apr 4, 2024

Update file open mode to binary

3483de3

* rubyzip 2.3.0からエンコーディング結果がASCII-8BITに変更された * rubyzip/rubyzip#439

yn-misaki mentioned this pull request Apr 4, 2024

File.open モードをバイナリに変更する medley-inc/ruby-xlsx-templater#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set OutputStream.write_buffer's default buffer to binmode #439

Set OutputStream.write_buffer's default buffer to binmode #439

henkesn commented Feb 19, 2020

coveralls commented Feb 19, 2020 •

edited

Loading

hainesr left a comment

hainesr Feb 19, 2020

henkesn Feb 19, 2020

hainesr Feb 19, 2020

hainesr left a comment •

edited

Loading

jdleesmiller left a comment

henkesn commented Mar 2, 2020

jdleesmiller left a comment

jdleesmiller Mar 14, 2020

henkesn Mar 16, 2020

kitsunde commented Jul 20, 2020

	def write_buffer(io = ::StringIO.new('').binmode, encrypter = nil)
	def write_buffer(io = ::StringIO.new(''), encrypter = nil)
	io.binmode if io.respond_to?(:binmode)

	# https://github.com/rubyzip/rubyzip/issues/119
	io.binmode if io.respond_to?(:binmode)

Set OutputStream.write_buffer's default buffer to binmode #439

Set OutputStream.write_buffer's default buffer to binmode #439

Conversation

henkesn commented Feb 19, 2020

coveralls commented Feb 19, 2020 • edited Loading

hainesr left a comment

Choose a reason for hiding this comment

hainesr Feb 19, 2020

Choose a reason for hiding this comment

henkesn Feb 19, 2020

Choose a reason for hiding this comment

hainesr Feb 19, 2020

Choose a reason for hiding this comment

hainesr left a comment • edited Loading

Choose a reason for hiding this comment

jdleesmiller left a comment

Choose a reason for hiding this comment

henkesn commented Mar 2, 2020

jdleesmiller left a comment

Choose a reason for hiding this comment

jdleesmiller Mar 14, 2020

Choose a reason for hiding this comment

henkesn Mar 16, 2020

Choose a reason for hiding this comment

kitsunde commented Jul 20, 2020

coveralls commented Feb 19, 2020 •

edited

Loading

hainesr left a comment •

edited

Loading