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

Check Encodings before calling force_encoding in Addressable::URI #341

Merged
merged 7 commits into from Aug 18, 2022

Conversation

ACBullen
Copy link
Contributor

@ACBullen ACBullen commented Apr 23, 2019

Reopening this pr with just the minimum changes to fix the issue blocking our use of the current version.

This is a quick fix to this issue, just adding in an extra check to avoid mutating strings when it isn't necessary

Close #254

lib/addressable/uri.rb Outdated Show resolved Hide resolved
@dentarg
Copy link
Collaborator

dentarg commented Apr 23, 2019

Reopening this pr with just the minimum changes to fix the issue blocking our use of the current version.

I'm curious, what are the issue you are facing with the current version?

This is a quick fix to this issue, just adding in an extra check to avoid mutating strings when it isn't necessary

#254 is about constants, but this PR only touches instance variables, so I don't think it is the same.

@ACBullen
Copy link
Contributor Author

The issue with constants is sort of an extension of the issue we have right now since we are normalizing the Addressable::URI object and then deep freezing. Even if the path or other part of the uri doesn't map to a constant, we are marking all of those strings as intended to be immutable. Since the force_encoding runs every time we try to reference parts of the parsed and normalized URI, this causes the same RuntimeError: can't modify frozen String to return.

Without this check, after the strings go through normalization, they are in their final state and do not mutate unless something tries to reassign one of the.

@dentarg
Copy link
Collaborator

dentarg commented Apr 23, 2019

Got it, thanks.

I think we need to add tests for this.

spec/addressable/uri_spec.rb Outdated Show resolved Hide resolved
spec/addressable/uri_spec.rb Outdated Show resolved Hide resolved
spec/addressable/uri_spec.rb Outdated Show resolved Hide resolved
spec/addressable/uri_spec.rb Outdated Show resolved Hide resolved
spec/addressable/uri_spec.rb Outdated Show resolved Hide resolved
spec/addressable/uri_spec.rb Outdated Show resolved Hide resolved
spec/addressable/uri_spec.rb Outdated Show resolved Hide resolved
spec/addressable/uri_spec.rb Outdated Show resolved Hide resolved
spec/addressable/uri_spec.rb Show resolved Hide resolved
Gemfile Outdated Show resolved Hide resolved
@ACBullen ACBullen force-pushed the alexb/check_before_force_encoding branch from 5b65b93 to 0acc1c2 Compare April 24, 2019 22:12
spec/addressable/uri_spec.rb Outdated Show resolved Hide resolved
spec/addressable/uri_spec.rb Outdated Show resolved Hide resolved
spec/addressable/uri_spec.rb Outdated Show resolved Hide resolved
spec/addressable/uri_spec.rb Outdated Show resolved Hide resolved
spec/addressable/uri_spec.rb Show resolved Hide resolved
@sporkmonger sporkmonger changed the base branch from master to main July 3, 2021 04:45
lib/addressable/uri.rb Outdated Show resolved Hide resolved
calling force_encoding on them

This prevents unnecessary mutation of these variables when already set
and appropriately encoded. Necessary for our use case as we freeze our
Addressable::URI objects to ensure they don't get changed when passed around
@baseballlover723 baseballlover723 force-pushed the alexb/check_before_force_encoding branch from 0acc1c2 to d00693a Compare August 31, 2021 01:52
@baseballlover723 baseballlover723 force-pushed the alexb/check_before_force_encoding branch from d00693a to 8194318 Compare August 31, 2021 01:56
@baseballlover723
Copy link
Contributor

@dentarg I have rebased and updated this PR. Let me know if theres anything else you want me to change.

end

it "should not allow destructive operations" do
expect { @uri.normalize! }.to raise_error(FrozenError)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FrozenError only exist in Ruby 2.5 and newer (I just learned this too). Sounds like it was RuntimeError before: jruby/jruby#5106

Hence the test failures. Do you mind addressing this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pushed some changes

spec/addressable/uri_spec.rb Outdated Show resolved Hide resolved
spec/addressable/uri_spec.rb Outdated Show resolved Hide resolved
spec/addressable/uri_spec.rb Show resolved Hide resolved
@baseballlover723 baseballlover723 force-pushed the alexb/check_before_force_encoding branch 2 times, most recently from f7a8274 to 7b0f707 Compare September 1, 2021 01:44
spec/addressable/uri_spec.rb Outdated Show resolved Hide resolved
spec/addressable/uri_spec.rb Show resolved Hide resolved
@baseballlover723
Copy link
Contributor

@dentarg This is ready for review again

sporkmonger
sporkmonger previously requested changes Jul 30, 2022

it "should not allow destructive operations" do
ruby_check = Gem::Version.new(RUBY_VERSION) < Gem::Version.new("2.5.0")
error = ruby_check ? RuntimeError : FrozenError
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to have this test not care at all which error type is raised.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I'm following... it says should not allow destructive operations and we expect @uri.normalize! to raise RuntimeError/FrozenError (can't modify frozen String). Is that because you shouldn't be able to use #normalize! on a frozen Addressable::URI? Maybe we can make that even more clear in the test? (The @uri.freeze happens way up in the before block so it is easy to miss)

Maybe Addressable::URI#normalize! should detect that the URI is frozen and raise an error defined by Addressable? (FrozenURIError perhaps?)


Ah, this particular spec doesn't test the changes made in this branch, I ran this against Addressable 2.8.0

# Ruby 2.4.3

irb(main):005:0* @uri = Addressable::URI.parse("http://user:password@example.com:8080/path?query=value#fragment")
=> #<Addressable::URI:0x3ff6e204dd78 URI:http://user:password@example.com:8080/path?query=value#fragment>
irb(main):006:0> @uri.freeze
=> #<Addressable::URI:0x3ff6e204dd78 URI:http://user:password@example.com:8080/path?query=value#fragment>
irb(main):007:0> @uri.normalize!
RuntimeError: can't modify frozen Addressable::URI
	from /Users/dentarg/.gem/ruby/2.4.3/gems/addressable-2.8.0/lib/addressable/uri.rb:2518:in `remove_instance_variable'
	from /Users/dentarg/.gem/ruby/2.4.3/gems/addressable-2.8.0/lib/addressable/uri.rb:2518:in `block in replace_self'
	from /Users/dentarg/.gem/ruby/2.4.3/gems/addressable-2.8.0/lib/addressable/uri.rb:2516:in `each'
	from /Users/dentarg/.gem/ruby/2.4.3/gems/addressable-2.8.0/lib/addressable/uri.rb:2516:in `replace_self'
	from /Users/dentarg/.gem/ruby/2.4.3/gems/addressable-2.8.0/lib/addressable/uri.rb:2211:in `normalize!'
	from (irb):7
	from /Users/dentarg/.rubies/2.4.3/bin/irb:11:in `<main>'
# Ruby 3.1.2

irb(main):005:0> @uri = Addressable::URI.parse("http://user:password@example.com:8080/path?query=value#fragment")
=> #<Addressable::URI:0xb43c URI:http://user:password@example.com:8080/path?query=value#fragment>
irb(main):006:0>
irb(main):007:0> @uri.freeze
=> #<Addressable::URI:0xb43c URI:http://user:password@example.com:8080/path?query=value#fragment>
irb(main):008:0> @uri.normalize!
/Users/dentarg/.gem/ruby/3.1.2/gems/addressable-2.8.0/lib/addressable/uri.rb:2518:in `remove_instance_variable': can't modify frozen Addressable::URI: #<Addressable::URI:0xb43c URI:http://user:password@example.com:8080/path?query=value#fragment> (FrozenError)
	from /Users/dentarg/.gem/ruby/3.1.2/gems/addressable-2.8.0/lib/addressable/uri.rb:2518:in `block in replace_self'
	from /Users/dentarg/.gem/ruby/3.1.2/gems/addressable-2.8.0/lib/addressable/uri.rb:2516:in `each'
	from /Users/dentarg/.gem/ruby/3.1.2/gems/addressable-2.8.0/lib/addressable/uri.rb:2516:in `replace_self'
	from /Users/dentarg/.gem/ruby/3.1.2/gems/addressable-2.8.0/lib/addressable/uri.rb:2211:in `normalize!'
	from (irb):8:in `<main>'
	from /Users/dentarg/.rubies/3.1.2/lib/ruby/gems/3.1.0/gems/irb-1.4.1/exe/irb:11:in `<top (required)>'
	from /Users/dentarg/.rubies/3.1.2/bin/irb:25:in `load'
	from /Users/dentarg/.rubies/3.1.2/bin/irb:25:in `<main>'

Perhaps we shouldn't change this behaviour as I suggested above


@sporkmonger should we just check error message? Expect can't modify frozen Addressable::URI. Those can change from Ruby to Ruby version too, but probably unlikely in this case IMHO.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly I'd prefer to have the test make the same checks regardless of Ruby version. If the error message is consistent between versions but the type is not, it seems fine to check the error message, but if that's inconsistent too, I think it's fine to verify that an error, any error, has been thrown.

I think I'd rather not do a custom error type because I think if we did a custom type it should probably inherit from FrozenError on versions of Ruby that have it, and that just shifts the problem out of the tests and into the library itself. I'm not sure that's an improvement.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can have the test make the same checks regardless of Ruby version. If we look at the hierarchy of exceptions at https://ruby-doc.org/core-2.6.5/Exception.html we can see that FrozenError is a subclass of RuntimeError. That should never change really. We can use #is_a? and always check if it is a RuntimeError:

irb(main):007:1* ex = begin
irb(main):008:1*   raise FrozenError, "can't modify frozen String"
irb(main):009:1* rescue => e
irb(main):010:1*   e
irb(main):011:0> end
=> #<FrozenError: can't modify frozen String>
irb(main):012:0> ex.inspect
=> "#<FrozenError: can't modify frozen String>"
irb(main):013:0> ex.is_a?(RuntimeError)
=> true

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(We don't need to check the message)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any other changes required? I've implemented the above changes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now, thanks!

spec/addressable/uri_spec.rb Outdated Show resolved Hide resolved
spec/addressable/uri_spec.rb Outdated Show resolved Hide resolved
@baseballlover723 baseballlover723 force-pushed the alexb/check_before_force_encoding branch from 85aa396 to b5e929b Compare August 1, 2022 06:40
@dentarg dentarg dismissed sporkmonger’s stale review August 18, 2022 18:28

The requested changed has been made

@dentarg dentarg merged commit 860fede into sporkmonger:main Aug 18, 2022
32 checks passed
@baseballlover723 baseballlover723 deleted the alexb/check_before_force_encoding branch August 18, 2022 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Constants in Addressable::URI sometimes get mutated
5 participants