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.

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

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

Closed
rknmag opened this issue Jul 26, 2019 · 10 comments · Fixed by #581
Closed

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

rknmag opened this issue Jul 26, 2019 · 10 comments · Fixed by #581

Comments

@rknmag
Copy link

rknmag commented Jul 26, 2019

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
Copy link

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
Copy link

vanboom commented Mar 22, 2022

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
Copy link

vanboom commented Mar 22, 2022

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

@patrickkulling
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Author

rknmag commented Jan 20, 2023

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

@patrickkulling
Copy link
Contributor

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
Copy link
Author

rknmag commented Jan 21, 2023

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:in block (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:in upto'
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:in block (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:in upto'
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:in block (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:in upto'
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:in block 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:in upto'
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:in read_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
Copy link
Contributor

@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
Copy link
Contributor

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants