Skip to content

Add new Lint/UriEscapeUnescape cop #4702

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

Merged
merged 2 commits into from
Sep 10, 2017

Conversation

koic
Copy link
Member

@koic koic commented Sep 5, 2017

Feature

This cop identifies places where URI.escape can be replaced by CGI.escape, URI.encode_www_form or URI.encode_www_form_component depending on your specific use case.
Also this cop identifies places where URI.unescape can be replaced by CGI.unescape, URI.decode_www_form or URI.decode_www_form_component depending on your specific use case.

% cat /tmp/example.rb
# frozen_string_literal: true

require 'uri'

URI.escape('http://example.com')
URI.encode('http://example.com')
URI.unescape('http://example.com')
URI.decode('http://example.com')
% bundle exec rubocop /tmp/example.rb
Inspecting 1 file
W

Offenses:

/tmp/example.rb:5:1: W: URI.escape 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.
URI.escape('http://example.com')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/tmp/example.rb:6:1: W: URI.encode 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.
URI.encode('http://example.com')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/tmp/example.rb:7:1: W: URI.unescape method is obsolete and should not be used. Instead, use CGI.unescape, URI.decode_www_form or URI.decode_www_form_component depending on your specific use case.
URI.unescape('http://example.com')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/tmp/example.rb:8:1: W: URI.decode method is obsolete and should not be used. Instead, use CGI.unescape, URI.decode_www_form or URI.decode_www_form_component depending on your specific use case.
URI.decode('http://example.com')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 4 offenses detected

Target Problem

Emulate the following Ruby's warnings.

% ruby -w -ruri -e "URI.escape('http://example.com')"
-e:1:in `<main>': warning: URI.escape is obsolete

% ruby -w -ruri -e "URI.encode('http://example.com')"
-e:1:in `<main>': warning: URI.escape is obsolete

% ruby -w -ruri -e "URI.unescape('http://example.com')"
-e:1:in `<main>': warning: URI.unescape is obsolete

% ruby -w -ruri -e "URI.decode('http://example.com')"
-e:1:in `<main>': warning: URI.unescape is obsolete

Other Information

This cop does not have autocorrect because these methods to replace depends on use case.

The following is an excerpt from the API Doc.

URI.escape

https://github.com/ruby/ruby/blob/cd6df5fb3c1e9b965071d6a92ed1c7d4a938560f/lib/uri/common.rb#L80-L87

    # == Description
    #
    # Escapes the string, replacing all unsafe characters with codes.
    #
    # 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
    #

URI.unescape

https://github.com/ruby/ruby/blob/cd6df5fb3c1e9b965071d6a92ed1c7d4a938560f/lib/uri/common.rb#L117-L122

    # == Description
    #
    # This method is obsolete and should not be used. Instead, use
    # CGI.unescape, URI.decode_www_form or URI.decode_www_form_component
    # depending on your specific use case.
    #

Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Used the same coding conventions as the rest of the project.
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • All tests(rake spec) are passing.
  • The new code doesn't generate RuboCop offenses that are checked by rake internal_investigation.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Updated cop documentation with rake generate_cops_documentation (required only when you've added a new cop or changed the configuration/documentation of an existing cop).

Sorry, something went wrong.

@koic koic force-pushed the add_new_lint_uri_escape_unescape branch from a95564f to 5bcc92b Compare September 5, 2017 15:32
Copy link
Collaborator

@pocke pocke left a comment

Choose a reason for hiding this comment

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

👍

@bbatsov bbatsov merged commit b50d0bb into rubocop:master Sep 10, 2017
@koic koic deleted the add_new_lint_uri_escape_unescape branch September 10, 2017 07:50
@jonworek
Copy link

jonworek commented Nov 6, 2017

I'm going through some of my code, but I'm hung up on the correct remediation for a violation of this rule.

depending on your specific use case

It's not obvious to me which use cases warrant which of the three options that were given. Are you able to elaborate?

Thanks.

@yuki24
Copy link

yuki24 commented Nov 14, 2017

I also don't see why CGI.unescape should take the precedence over URI.unescape, aside from so over-used "consistency". Blindly suggesting and replacing valid use cases without explaining why is extremely unhelpful and non educational.

@Drenmi
Copy link
Collaborator

Drenmi commented Nov 14, 2017

It's not obvious to me which use cases warrant which of the three options that were given. Are you able to elaborate?

CGI.escape URL encodes an arbitrary string. If that sounds like what you are doing, then that's the one you want. This is by far the more common use case. 🙂

URI.encode_www_form dumps an Enumerable to application/x-www-form-urlencoded data. (Basically a long query string.)

I also don't see why CGI.unescape should take the precedence over URI.unescape [...]

Because URI.unescape was marked as deprecated in Ruby trunk, and generates a deprecation warning in 2.4. I tried digging into the issue tracker, but there's no explanation why it was deprecated. You'd need to ask the right person on the Ruby core team.

Blindly suggesting and replacing valid use cases without explaining why is extremely unhelpful and non educational.

I agree, and the educational aspect is arguably the more important one. This message was ported directly from the commit in Ruby trunk. Perhaps "obsolete" should be reworded to "deprecated", but "obsolete" is the wording used in Ruby's own deprecation warning, and the suggested replacement was made by Ruby core team members themselves.

@yuki24
Copy link

yuki24 commented Nov 14, 2017

Thanks @Drenmi for your fantastic explanation! I'm now 100% a proponent of this cop.

@koic koic mentioned this pull request Jan 21, 2019
8 tasks
@matkoniecz
Copy link

I found http://blade.nagaokaut.ac.jp/cgi-bin/vframe.rb/ruby/ruby-core/29293?29179-31097 but I still have no idea why Ruby core team deprecated it.

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.

None yet

7 participants