-
Notifications
You must be signed in to change notification settings - Fork 898
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
Ruby 2.7: Replace URI.escape/encode and unescape/decode with URI::DEFAULT_PARSER escape/unescape #21036
Conversation
Gemfile
Outdated
@@ -21,6 +21,7 @@ end | |||
manageiq_plugin "manageiq-schema" | |||
|
|||
# Unmodified gems | |||
gem "addressable", "~>2.5", :require => false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already require addressable in core code:
% git grep Addressable
app/models/service_template_transformation_plan_task.rb: :path => "/vmfs/volumes/#{Addressable::URI.escape(storage.name)}/#{Addressable::URI.escape(source.location)}"
We already have dependencies that require addressable 2.5 at a minumum:
manageiq-providers-scvmm (0.1.0)
addressable (~> 2.4)
google-api-client (0.50.0)
addressable (~> 2.5, >= 2.5.1)
azure-armrest (0.10.0)
addressable (~> 2.5.0)
azure-signature (0.3.0)
addressable (~> 2)
hawkular-client (5.0.0)
addressable
http (4.4.1)
addressable (~> 2.3)
json-schema (2.8.1)
addressable (>= 2.4)
signet (0.14.1)
addressable (~> 2.3)
wasabi (3.6.1)
addressable
webmock (3.11.2)
addressable (>= 2.3.6)
app/models/vm_or_template.rb
Outdated
@@ -1,4 +1,5 @@ | |||
require 'ancestry' | |||
require 'addressable' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alphabetical ad < an
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should have been removed now that the caller was removed in #21021
My only concern is that they aren't exactly the same, and I don't know if that can cause problems or not [10] pry(main)> s = "abc 123 !@\#$%^&*()_+{}|:\"<>?-=[]\\;',./"
=> "abc 123 !@\#$%^&*()_+{}|:\"<>?-=[]\\;',./"
[11] pry(main)> URI.encode(s)
=> "abc%20123%20!@%23$%25%5E&*()_+%7B%7D%7C:%22%3C%3E?-=[]%5C;',./"
[12] pry(main)> Addressable::URI.escape(s)
=> "abc%20123%20!@\#$%25%5E&*()_+%7B%7D%7C:%22%3C%3E?-=%5B%5D%5C;',./"
[13] pry(main)> CGI.escape(s)
=> "abc+123+%21%40%23%24%25%5E%26%2A%28%29_%2B%7B%7D%7C%3A%22%3C%3E%3F-%3D%5B%5D%5C%3B%27%2C.%2F"
[14] pry(main)> ERB::Util.url_encode(s)
=> "abc%20123%20%21%40%23%24%25%5E%26%2A%28%29_%2B%7B%7D%7C%3A%22%3C%3E%3F-%3D%5B%5D%5C%3B%27%2C.%2F" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with this. I would rather go the CGI.escape
or something else built in, but as mentioned, Addressable
is already being included by those that prefer to use it, so no point in dying on this hill.
🚢 🇮🇹
|
I added examples for |
of note, when compared to Also note that |
app/models/file_depot_ftp.rb
Outdated
@@ -125,7 +126,7 @@ def destination_path | |||
|
|||
def base_path | |||
# uri: "ftp://ftp.example.com/incoming" => #<Pathname:incoming> | |||
path = URI(URI.encode(uri)).path | |||
path = URI(Addressable::URI.escape(uri)).path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why this was encoded in the first place...
[25] pry(main)> URI("ftp://ftp.example.com/incoming").path
=> "incoming"
[26] pry(main)> URI(URI.encode("ftp://ftp.example.com/incoming")).path
=> "incoming"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also note:
[28] pry(main)> URI(ERB::Util.url_encode("ftp://ftp.example.com/incoming")).path
=> "ftp%3A%2F%2Fftp.example.com%2Fincoming"
[29] pry(main)> URI(CGI.escape("ftp://ftp.example.com/incoming")).path
=> "ftp%3A%2F%2Fftp.example.com%2Fincoming"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that if this had had a fragment attached, it seems Addressable::URI is better than URI.encode:
[30] pry(main)> URI(URI.encode("ftp://ftp.example.com/incoming#fragment")).path
=> "incoming%23fragment"
[31] pry(main)> URI(Addressable::URI.encode("ftp://ftp.example.com/incoming#fragment")).path
=> "incoming"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I wrote that... so let me look into it...
Just kidding, no I didn't. It was all LJ 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that if this had had a fragment attached...
@Fryguy I don't think that a fragment would ever be valid for an FTP URL though. But perhaps I am wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just kidding, no I didn't. It was all LJ 😄
Haha, now I have to see why I did it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NickLaMuro found the original reason for the URI.encode:
commit 27492b325bb3892bb5b5bc1184d92c6e3c08f374
Author: Gregg Tanzillo <[email protected]>
Date: Fri Sep 4 18:52:16 2009 +0000
BugzID: 6494
Added URI encode/decode wherever we handle log depot URIs and paths to address errors when posting to a URI that had spaces in the path.
git-svn-id: http://miq-ubuntusub.manageiq.com/svn/svnrepos/Manageiq/trunk@16098 3c68ef56-dcc3-11dc-9475-a42b84ecc76f
Diff
diff --git a/vmdb/app/models/log_file.rb b/vmdb/app/models/log_file.rb
index a0daa40ca1..5398fc6b30 100644
--- a/vmdb/app/models/log_file.rb
+++ b/vmdb/app/models/log_file.rb
@@ -109,7 +109,7 @@ def self.empty_logfile(historical)
def self.get_post_method(settings)
return nil if settings[:uri].nil?
- scheme, userinfo, host, port, registry, path, opaque, query, fragment = URI.split(settings[:uri])
+ scheme, userinfo, host, port, registry, path, opaque, query, fragment = URI.split(URI.encode(settings[:uri]))
return scheme
end
@@ -130,7 +130,7 @@ def self.verify_log_depot_settings(settings)
def self.connect_ftp(settings)
log_header = "MIQ(#{self.class.name}-connect_ftp)"
begin
- scheme, userinfo, host, port, registry, path, opaque, query, fragment = URI.split(settings[:uri])
+ scheme, userinfo, host, port, registry, path, opaque, query, fragment = URI.split(URI.encode(settings[:uri]))
$log.info("#{log_header} Connecting to [#{settings[:uri]}]...")
ftp = Net::FTP.new(host)
ftp.login(settings[:username], settings[:password])
@@ -158,11 +158,11 @@ def add_log_file_ftp(fname)
$log.info("#{log_header} Adding log file [#{fname}] to ftp [#{settings[:uri]}...")
- scheme, userinfo, host, port, registry, path, opaque, query, fragment = URI.split(settings[:uri])
+ scheme, userinfo, host, port, registry, path, opaque, query, fragment = URI.split(URI.encode(settings[:uri]))
server = self.resource
zone = server.zone
- path = [path, "#{zone.name}_#{zone.id}", "#{server.name}_#{server.id}"].join("/")
+ path = [URI.decode(path), "#{zone.name}_#{zone.id}", "#{server.name}_#{server.id}"].join("/")
ftp = self.class.connect_ftp(settings)
@@ -202,7 +202,7 @@ def remove_log_file_ftp
$log.info("#{log_header} Removing log file [#{self.log_uri}]...")
- scheme, userinfo, host, port, registry, path, opaque, query, fragment = URI.split(self.log_uri)
+ scheme, userinfo, host, port, registry, path, opaque, query, fragment = URI.split(URI.encode(self.log_uri))
path = URI.decode(path)
ftp = self.class.connect_ftp(settings.merge(:uri => self.log_uri))
@@ -226,7 +226,7 @@ def file_exists_ftp?(existing_ftp = nil)
$log.info("#{log_header} Checking existance of log file [#{self.log_uri}]...")
- scheme, userinfo, host, port, registry, path, opaque, query, fragment = URI.split(self.log_uri)
+ scheme, userinfo, host, port, registry, path, opaque, query, fragment = URI.split(URI.encode(self.log_uri))
path = URI.decode(path)
ftp = existing_ftp || self.class.connect_ftp(settings.merge(:uri => self.log_uri))
Looking at the old FogBugz ticket I see these log lines:
[----] I, [2009-08-26T16:19:15.285493 #7703] INFO -- : MIQ(LogFile-file_exists_ftp?) Checking existance of log file [ftp://ftp.manageiq.com/thenn/from_xav/20090826/Chicago_2/EVM ChiMgmt_1/Current_Chicago_2_EVM ChiMgmt_1_20090826_090208_20090826_161910.zip]...
[----] E, [2009-08-26T16:19:15.285798 #7703] ERROR -- : MIQ(LogFile-add_log_file_ftp) 'bad URI(is not URI?): ftp://ftp.manageiq.com/thenn/from_xav/20090826/Chicago_2/EVM ChiMgmt_1/Current_Chicago_2_EVM ChiMgmt_1_20090826_090208_20090826_161910.zip', writing to FTP: [ftp://ftp.manageiq.com/thenn/from_xav/20090826], Username: [thennessy@manageiq]
[----] I, [2009-08-26T16:19:15.295576 #7703] INFO -- : MIQ(MiqServer-post_current_logs) Task: [1] Posting of current logs failed for EVM ChiMgmt Chicago MiqServer 1 due to error: [RuntimeError] ['bad URI(is not URI?): ftp://ftp.manageiq.com/thenn/from_xav/20090826/Chicago_2/EVM ChiMgmt_1/Current_Chicago_2_EVM ChiMgmt_1_20090826_090208_20090826_161910.zip', writing to FTP: [ftp://ftp.manageiq.com/thenn/from_xav/20090826], Username: [thennessy@manageiq]]
So it seems this was only added to handle spaces in incoming URLs. If we add (or have) specs for spaces, then we can choose any encoder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for digging into this. I was going to look at the various places we used it to find what specific characters were we concerned with. I'll look at the remaining ones to see if it's the same story.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I added tests demonstrating what worked previously with 2.6/2.7 with both URI.decode and Addressable::URI.decode.
After going through all of the commits, the only common thread was to deal with converting spaces to %20
as @Fryguy pointed out. I did the other escaped characters just for "fun".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted by @Fryguy it probably is safer to go with a route that gives us the same result back given a set of known escape characters. URI::Parser#escape
seems to do the same thing as the previous method:
irb> s = "abc 123 !@\#$%^&*()_+{}|:\"<>?-=[]\\;',./"
#=> "abc 123 !@\#$%^&*()_+{}|:\"<>?-=[]\\;',./"
irb> URI::Parser.new.escape(s)
#=> "abc%20123%20!@%23$%25%5E&*()_+%7B%7D%7C:%22%3C%3E?-=[]%5C;',./"
haha, this is why I did this as a separate PR... thanks for the comments... I'll review them |
This became unmergeable because #21021 got merged. |
5afa1a8
to
1171087
Compare
Do we want to go that route? I'm fine with it if it works. I can't tell if that's a backdoor around what they were trying to avoid by warning about |
Since the purpose of the original code was to handle spaces, then anything that handles spaces works for me (I'd even be fine with a gsub(" ", "%20")) |
yeah, the problem it's all over the place and there were commits in other repos that I'd like to do consistently for the same concerns. Since it looks like we want to be careful with at least space and From what I gather, the only commits that mention a reason, it's either space or for Logfile:
and also this one mentioned previously:
This one talks about
I think that code still exists in the vmware event parsing code. |
I'll just try this with my existing tests and be done with it. I don't want to change behavior, just get rid of warnings. EDIT... typo |
Fixes various warnings of this variety: app/models/file_depot_ftp.rb:128: warning: URI.escape is obsolete app/models/mixins/file_depot_mixin.rb:42: warning: URI.escape is obsolete Ruby 2.7 prints this warning and they suggest the following from https://ruby-doc.org/stdlib-2.7.2/libdoc/uri/rdoc/URI/Escape.html#method-i-escape-label-Description: ``` This method is obsolete and should not be used. Instead, use CGI.escape, URI.encode_www_form or URI.encode_www_form_component depending on your specific use case. ```
1171087
to
27c3b3a
Compare
Thanks @NickLaMuro, I went with |
Since rubocop complains about using See https://github.com/ruby/ruby/blob/ruby_2_6/lib/uri/common.rb#L22 |
Checked commits jrafanie/manageiq@1bf76ed~...fe279f2 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint |
Just for clarity, this approach is safe on ruby 2.5/2.6/2.7, it's an instance of the same RFC2396 Parser class:
|
I also tested on Ruby 3, and it works the same as in the 2.x versions as tested, just without the |
Fixes various warnings of this variety: app/models/file_depot_ftp.rb:128: warning: URI.escape is obsolete app/models/mixins/file_depot_mixin.rb:42: warning: URI.escape is obsolete From: ManageIQ/manageiq#21036 Related: ManageIQ/manageiq#19678
Fixes various warnings of this variety: app/models/file_depot_ftp.rb:128: warning: URI.escape is obsolete app/models/mixins/file_depot_mixin.rb:42: warning: URI.escape is obsolete From: ManageIQ/manageiq#21036 Related: ManageIQ/manageiq#19678
Fixes various warnings of this variety: app/models/file_depot_ftp.rb:128: warning: URI.escape is obsolete app/models/mixins/file_depot_mixin.rb:42: warning: URI.escape is obsolete From: ManageIQ/manageiq#21036 Related: ManageIQ/manageiq#19678 (transferred from ManageIQ/manageiq-gems-pending@3bddaa4)
Fixes various warnings of this variety: app/models/file_depot_ftp.rb:128: warning: URI.escape is obsolete app/models/mixins/file_depot_mixin.rb:42: warning: URI.escape is obsolete From: ManageIQ/manageiq#21036 Related: ManageIQ/manageiq#19678 (transferred from ManageIQ/manageiq-gems-pending@3bddaa4)
Fixes various warnings of this variety: app/models/file_depot_ftp.rb:128: warning: URI.escape is obsolete app/models/mixins/file_depot_mixin.rb:42: warning: URI.escape is obsolete From: ManageIQ/manageiq#21036 Related: ManageIQ/manageiq#19678 (transferred from ManageIQ/manageiq-gems-pending@3bddaa4)
Fixes various warnings of this variety: app/models/file_depot_ftp.rb:128: warning: URI.escape is obsolete app/models/mixins/file_depot_mixin.rb:42: warning: URI.escape is obsolete From: ManageIQ/manageiq#21036 Related: ManageIQ/manageiq#19678 (transferred from ManageIQ/manageiq-gems-pending@3bddaa4)
apipie-bindings 0.4.0 was bumped as it added ruby 3 support, which dropped some URI methods such as decode/encode/escape/unescape. See also ManageIQ/manageiq#21036
Note:
URI::DEFAULT_PARSER == URI::Parser.new
https://github.com/ruby/ruby/blob/ruby_2_6/lib/uri/common.rb#L22
Fixes various warnings of this variety:
app/models/file_depot_ftp.rb:128: warning: URI.escape is obsolete
app/models/mixins/file_depot_mixin.rb:42: warning: URI.escape is obsolete
Ruby 2.7 prints this warning and they suggest the following from https://ruby-doc.org/stdlib-2.7.2/libdoc/uri/rdoc/URI/Escape.html#method-i-escape-label-Description: