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

Avoid modifying frozen strings #649

Merged
merged 1 commit into from
Mar 20, 2019

Conversation

Morriar
Copy link
Contributor

@Morriar Morriar commented Feb 19, 2019

Calling the force_encoding method on frozen strings can be a problem.
Especially for people using the # frozen_string_literal: true annotation.

To protect it, I added a call to dup before all calls to force_encoding.
If we decide the bump the ruby version to something newer we could use the +.

Signed-off-by: Alexandre Terrasa [email protected]

Signed-off-by: Alexandre Terrasa <[email protected]>
@jnunemaker
Copy link
Owner

I thought # frozen_string_literal: true only applied to the contents of that file, not all other libraries used in that file. What was the error you hit that led you down this path?

@TheSmartnik
Copy link
Collaborator

Well, if someone to run ruby with RUBYOPT="--enable-frozen-string-literal" it may be a problem. Unfortunately, httparty depends on a library that mutates strings anyway, so it won't be of much help 😞
sferik/multi_xml#64

@rafaelfranca
Copy link

I don't think those issues are related. # frozen_string_literal: true only applies to the content of that file, but let's say one of those contents are the body that is passed to httparty. In that case body will be frozen and httparty will try to mutate that string. This is what is happening here. We are not using RUBYOPT="--enable-frozen-string-literal".

You can reproduce the problem with HTTParty.post(url, "foo".freeze).

@TheSmartnik
Copy link
Collaborator

TheSmartnik commented Mar 20, 2019

@rafaelfranca thanks for clarifying, you're right

@Morriar sorry it look so long to review and thatnks for the pr

@TheSmartnik TheSmartnik merged commit 76e101d into jnunemaker:master Mar 20, 2019
@TheSmartnik
Copy link
Collaborator

@rafaelfranca @Morriar actually, I think I merged this too soon. The thing is HTTParty::TextEncoder is used only for handling response from server, which can't really be frozen with frozen_string_literal: true

You can reproduce the problem with HTTParty.post(url, "foo".freeze)

Second parameter accepts hash, not a string. So I can't really reproduce it 😕
Could you please provide a better reproduction example?

@rafaelfranca
Copy link

This is a combination between stubs using webmock and using httparty as client.

In a test like:

stub_request(:post, url, status: 200, body: '{"data": 1}'.freeze) # or with this stub being defined in a file with frozen_string_literal comment

HTTParty.post(url, some_hash)

this would fail.

@TheSmartnik
Copy link
Collaborator

Ah, I see. Didn't think about this use-case. Thanks for explaining @rafaelfranca

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 this pull request may close these issues.

4 participants