roo-rb / roo

Roo provides an interface to spreadsheets of several sorts.
MIT License
2.79k stars 501 forks source link

can't modify frozen String (FrozenError) in open_office.rb #509

Closed rknmag closed 1 year ago

rknmag commented 5 years ago

Steps to reproduce

require 'roo' require 'rspreadsheet'

fname = 'test_apos.ods' wb = Rspreadsheet.new sheet = wb.create_worksheet('Sheet1') sheet[1,1] = '' ### Note : The cell contain empty string wb.save(fname)

wb = Roo::Spreadsheet.open(fname) pp wb.last_row wb.close()

Issue

I write ods file using rspreadsheet gem. When I read back the file using roo, If any cell value contain empty string , then I get following error ..gems/roo-2.8.2/lib/roo/open_office.rb|520| in `gsub!': can't modify frozen String (FrozenError)

System configuration

Roo version: 2.8.2

Ruby version:2.6.1

rubycoder commented 3 years ago

I'm not using the rspreadsheet gem, but I'm seeing exactly the same error with Ruby 2.7.2 and roo 2.8.3, 2.8.2, 2.8.1, and 2.8.0 when reading a .csv file. However, it works with roo 2.7.1. I'm not using any of the roo 2.8.x features, so 2.7.1 is fine for me, at least for now.

vanboom commented 2 years ago

This is caused by the "frozen_string_literal: true" directive at the top of lib/roo/open_office.rb.

The logic in this file is not compatible with "frozen_string_literal: true" directive so it should be removed.

e.g.

str_v = ''   # <--- line 480
str_v.gsub!(/&apos;/, "'") # special case not supported by unescapeHTML, <--- line 518

This is not possible when treating all string literals as frozen.

vanboom commented 2 years ago

Is this gem still maintained? Please consider my PR to resolve this issue.

patrickkulling commented 1 year ago

@rknmag I know this issue is old, but can you provide a .ods file in order to reproduce the issue? I can't use the rspreadsheet gem since it's not working properly on mac anymore.

patrickkulling commented 1 year ago

@vanboom alternatively, your PR is doing two things. Can you create a separate PR including a test to reproduce the issue and/or validate the fix?

I 100% agree it's the frozen string literal and the fix is trivial, but it would be good to get the test coverage up so that this problem doesn't happen again.

rknmag commented 1 year ago

test_apos.ods I have re-run the program and attached the file. Hope it helps

patrickkulling commented 1 year ago

Thanks for the file @rknmag Can you still reproduce this with roo 2.9.0?

I was trying with ruby 2.7.0 & 3.1.2 and did no longer run into that issue

rknmag commented 1 year ago

Recent past I have not updated my Ruby, but I have Ruby 3.0 with roo 2.9.0. I ran the same program again and got the following trace back. I am not sure whether I have to update any other gem(s)

.rvm/gems/ruby-3.0.0@sprsheet/gems/roo-2.9.0/lib/roo/open_office.rb:520:in gsub!': can't modify frozen String: "" (FrozenError) from .rvm/gems/ruby-3.0.0@sprsheet/gems/roo-2.9.0/lib/roo/open_office.rb:520:inblock (4 levels) in read_cells' from .rvm/gems/ruby-3.0.0@sprsheet/gems/nokogiri-1.13.3-x86_64-linux/lib/nokogiri/xml/node_set.rb:233:in block in each' from .rvm/gems/ruby-3.0.0@sprsheet/gems/nokogiri-1.13.3-x86_64-linux/lib/nokogiri/xml/node_set.rb:232:inupto' from .rvm/gems/ruby-3.0.0@sprsheet/gems/nokogiri-1.13.3-x86_64-linux/lib/nokogiri/xml/node_set.rb:232:in each' from .rvm/gems/ruby-3.0.0@sprsheet/gems/roo-2.9.0/lib/roo/open_office.rb:485:inblock (3 levels) in read_cells' from .rvm/gems/ruby-3.0.0@sprsheet/gems/nokogiri-1.13.3-x86_64-linux/lib/nokogiri/xml/node_set.rb:233:in block in each' from .rvm/gems/ruby-3.0.0@sprsheet/gems/nokogiri-1.13.3-x86_64-linux/lib/nokogiri/xml/node_set.rb:232:inupto' from .rvm/gems/ruby-3.0.0@sprsheet/gems/nokogiri-1.13.3-x86_64-linux/lib/nokogiri/xml/node_set.rb:232:in each' from .rvm/gems/ruby-3.0.0@sprsheet/gems/roo-2.9.0/lib/roo/open_office.rb:474:inblock (2 levels) in read_cells' from .rvm/gems/ruby-3.0.0@sprsheet/gems/nokogiri-1.13.3-x86_64-linux/lib/nokogiri/xml/node_set.rb:233:in block in each' from .rvm/gems/ruby-3.0.0@sprsheet/gems/nokogiri-1.13.3-x86_64-linux/lib/nokogiri/xml/node_set.rb:232:inupto' from .rvm/gems/ruby-3.0.0@sprsheet/gems/nokogiri-1.13.3-x86_64-linux/lib/nokogiri/xml/node_set.rb:232:in each' from .rvm/gems/ruby-3.0.0@sprsheet/gems/roo-2.9.0/lib/roo/open_office.rb:465:inblock in read_cells' from .rvm/gems/ruby-3.0.0@sprsheet/gems/nokogiri-1.13.3-x86_64-linux/lib/nokogiri/xml/node_set.rb:233:in block in each' from .rvm/gems/ruby-3.0.0@sprsheet/gems/nokogiri-1.13.3-x86_64-linux/lib/nokogiri/xml/node_set.rb:232:inupto' from .rvm/gems/ruby-3.0.0@sprsheet/gems/nokogiri-1.13.3-x86_64-linux/lib/nokogiri/xml/node_set.rb:232:in each' from .rvm/gems/ruby-3.0.0@sprsheet/gems/roo-2.9.0/lib/roo/open_office.rb:459:inread_cells' from .rvm/gems/ruby-3.0.0@sprsheet/gems/roo-2.9.0/lib/roo/base.rb:119:in block (2 levels) in <class:Base>' from test_roo_apos.rb:11:in

'

patrickkulling commented 1 year ago

@rknmag Would you feel comfortable creating a unit test for it and open up a Pull Request with the fix (basically removing the frozen string literal comment from that file).

I would be happy to review it and merge it back

patrickkulling commented 1 year ago

@rknmag I was able to reproduce the issue and submitted a fix. I have decided to keep the frozen string literal due to it's performance optimisation potential but simply fixed the gsub! API