Clean up checks to see if DidYouMean is defined

As of Ruby 2.7 DidYouMean is included as a default gem, so there is no
need to check if DidYouMean is defined in the test suite. We still need
to check if the DidYouMean modules are defined in the actual code, as
someone might run Rails with DidYouMean disabled by using the
`--disable-did_you_mean` flag. This is ussually done for performance
reasons.

This commit also includes some of the changes made by Yuki in:
rails#39555
These changes include replacing Jaro with the more accurate
SpellChecker, and using DidYouMean::Correctable for simplere
corrections.

The DidYouMean::SpellChecker does have a treshold for corrections.
If there is not enough similarity it might not return a suggestion.
To stop the tests from failing some test data had to be changed.

For example, `non_existent` does not meet the treshold for `hello`, but
`ello` does:

DidYouMean::SpellChecker.new(dictionary: %w[hello]).correct('non_existent')
=> []
DidYouMean::SpellChecker.new(dictionary: %w[hello]).correct('ello')
=> ["hello"]

The treshold makes sense for spelling errors. But maybe we should add a
different SpellChecker that helps to get a suggestion even if there is
little overlap. For example for when a model only has 2 attributes
(title and body), it's helpful to get a suggestion for `name`

Co-Authored-By: Yuki Nishijima <[email protected]>

Author: p8

actionpack/lib/abstract_controller/base.rb

@@ -16,28 +16,13 @@ def initialize(message = nil, controller = nil, action = nil)
       super(message)
     end
 
-    class Correction
-      def initialize(error)
-        @error = error
-      end
+    if defined?(DidYouMean::Correctable) && defined?(DidYouMean::SpellChecker)
+      include DidYouMean::Correctable
 
       def corrections
-        if @error.action
-          maybe_these = @error.controller.class.action_methods
-
-          maybe_these.sort_by { |n|
-            DidYouMean::Jaro.distance(@error.action.to_s, n)
-          }.reverse.first(4)
-        else
-          []
-        end
+        @corrections ||= DidYouMean::SpellChecker.new(dictionary: controller.class.action_methods).correct(action)
       end
     end
-
-    # We may not have DYM, and DYM might not let us register error handlers
-    if defined?(DidYouMean) && DidYouMean.respond_to?(:correct_error)
-      DidYouMean.correct_error(self, Correction)
-    end
   end
 
   # AbstractController::Base is a low-level API. Nobody should be

actionpack/lib/action_controller/metal/exceptions.rb

@@ -33,29 +33,18 @@ def initialize(message, routes = nil, route_name = nil, method_name = nil)
       super(message)
     end
 
-    class Correction
-      def initialize(error)
-        @error = error
-      end
+    if defined?(DidYouMean::Correctable) && defined?(DidYouMean::SpellChecker)
+      include DidYouMean::Correctable
 
       def corrections
-        if @error.method_name
-          maybe_these = @error.routes.named_routes.helper_names.grep(/#{@error.route_name}/)
-          maybe_these -= [@error.method_name.to_s] # remove exact match
-
-          maybe_these.sort_by { |n|
-            DidYouMean::Jaro.distance(@error.route_name, n)
-          }.reverse.first(4)
-        else
-          []
+        @corrections ||= begin
+          maybe_these = routes&.named_routes&.helper_names&.grep(/#{route_name}/) || []
+          maybe_these -= [method_name.to_s] # remove exact match
+
+          DidYouMean::SpellChecker.new(dictionary: maybe_these).correct(route_name)
         end
       end
     end
-
-    # We may not have DYM, and DYM might not let us register error handlers
-    if defined?(DidYouMean) && DidYouMean.respond_to?(:correct_error)
-      DidYouMean.correct_error(self, Correction)
-    end
   end
 
   class MethodNotAllowed < ActionControllerError #:nodoc:

actionpack/lib/action_controller/metal/strong_parameters.rb

@@ -27,28 +27,13 @@ def initialize(param, keys = nil) # :nodoc:
       super("param is missing or the value is empty: #{param}")
     end
 
-    class Correction
-      def initialize(error)
-        @error = error
-      end
+    if defined?(DidYouMean::Correctable) && defined?(DidYouMean::SpellChecker)
+      include DidYouMean::Correctable
 
       def corrections
-        if @error.param && @error.keys
-          maybe_these = @error.keys
-
-          maybe_these.sort_by { |n|
-            DidYouMean::Jaro.distance(@error.param.to_s, n)
-          }.reverse.first(4)
-        else
-          []
-        end
+        @corrections ||= DidYouMean::SpellChecker.new(dictionary: keys).correct(param.to_s)
       end
     end
-
-    # We may not have DYM, and DYM might not let us register error handlers
-    if defined?(DidYouMean) && DidYouMean.respond_to?(:correct_error)
-      DidYouMean.correct_error(self, Correction)
-    end
   end
 
   # Raised when a supplied parameter is not expected and

actionpack/test/controller/base_test.rb

@@ -172,14 +172,12 @@ def test_process_should_be_precise
     assert_equal "The action 'non_existent' could not be found for EmptyController", exception.message
   end
 
-  if defined?(DidYouMean) && DidYouMean.respond_to?(:correct_error)
-    def test_exceptions_have_suggestions_for_fix
-      use_controller SimpleController
-      exception = assert_raise AbstractController::ActionNotFound do
-        get :non_existent
-      end
-      assert_match "Did you mean?", exception.message
+  def test_exceptions_have_suggestions_for_fix
+    use_controller SimpleController
+    exception = assert_raise AbstractController::ActionNotFound do
+      get :ello
     end
+    assert_match "Did you mean?", exception.message
   end
 
   def test_action_missing_should_work

actionpack/test/controller/required_params_test.rb

@@ -22,18 +22,16 @@ class ActionControllerRequiredParamsTest < ActionController::TestCase
     end
   end
 
-  if defined?(DidYouMean) && DidYouMean.respond_to?(:correct_error)
-    test "exceptions have suggestions for fix" do
-      error = assert_raise ActionController::ParameterMissing do
-        post :create, params: { magazine: { name: "Mjallo!" } }
-      end
-      assert_match "Did you mean?", error.message
-
-      error = assert_raise ActionController::ParameterMissing do
-        post :create, params: { book: { title: "Mjallo!" } }
-      end
-      assert_match "Did you mean?", error.message
+  test "exceptions have suggestions for fix" do
+    error = assert_raise ActionController::ParameterMissing do
+      post :create, params: { boko: { name: "Mjallo!" } }
     end
+    assert_match "Did you mean?", error.message
+
+    error = assert_raise ActionController::ParameterMissing do
+      post :create, params: { book: { naem: "Mjallo!" } }
+    end
+    assert_match "Did you mean?", error.message
   end
 
   test "required parameters that are present will not raise" do

actionpack/test/dispatch/debug_exceptions_test.rb

@@ -64,7 +64,7 @@ def call(env)
         [404, { "X-Cascade" => "pass" }, self]
       when "/not_found"
         controller = SimpleController.new
-        raise AbstractController::ActionNotFound.new(nil, controller, :not_found)
+        raise AbstractController::ActionNotFound.new(nil, controller, :ello)
       when "/runtime_error"
         raise RuntimeError
       when "/method_not_allowed"
@@ -98,7 +98,7 @@ def call(env)
       when "/missing_keys"
         raise ActionController::UrlGenerationError, "No route matches"
       when "/parameter_missing"
-        raise ActionController::ParameterMissing.new(:missing_param_key, %w(valid_param_key))
+        raise ActionController::ParameterMissing.new(:invalid_param_key, %w(valid_param_key))
       when "/original_syntax_error"
         eval "broke_syntax =" # `eval` need for raise native SyntaxError at runtime
       when "/syntax_error_into_view"
@@ -318,20 +318,18 @@ def call(env)
     assert_match(/ActionDispatch::Http::MimeNegotiation::InvalidType/, body)
   end
 
-  if defined?(DidYouMean) && DidYouMean.respond_to?(:correct_error)
-    test "rescue with suggestions" do
-      @app = DevelopmentApp
+  test "rescue with suggestions" do
+    @app = DevelopmentApp
 
-      get "/not_found", headers: { "action_dispatch.show_exceptions" => true }
-      assert_response 404
-      assert_select("b", /Did you mean\?/)
-      assert_select("li", "hello")
+    get "/not_found", headers: { "action_dispatch.show_exceptions" => true }
+    assert_response 404
+    assert_select("b", /Did you mean\?/)
+    assert_select("li", "hello")
 
-      get "/parameter_missing", headers: { "action_dispatch.show_exceptions" => true }
-      assert_response 400
-      assert_select("b", /Did you mean\?/)
-      assert_select("li", "valid_param_key")
-    end
+    get "/parameter_missing", headers: { "action_dispatch.show_exceptions" => true }
+    assert_response 400
+    assert_select("b", /Did you mean\?/)
+    assert_select("li", "valid_param_key")
   end
 
   test "rescue with HTML format for HTML API request" do

actionpack/test/dispatch/routing_test.rb

@@ -4913,11 +4913,9 @@ def app; APP end
     assert_match message, error.message
   end
 
-  if defined?(DidYouMean) && DidYouMean.respond_to?(:correct_error)
-    test "exceptions have suggestions for fix" do
-      error = assert_raises(ActionController::UrlGenerationError) { product_path(nil, "id" => "url-tested") }
-      assert_match "Did you mean?", error.message
-    end
+  test "exceptions have suggestions for fix" do
+    error = assert_raises(ActionController::UrlGenerationError) { product_path(nil, "id" => "url-tested") }
+    assert_match "Did you mean?", error.message
   end
 
   # FIXME: we should fix all locations that raise this exception to provide
@@ -4926,8 +4924,8 @@ def app; APP end
   # we don't want to break other code.
   test "correct for empty UrlGenerationError" do
     err = ActionController::UrlGenerationError.new("oh no!")
-    correction = ActionController::UrlGenerationError::Correction.new(err)
-    assert_equal [], correction.corrections
+
+    assert_equal [], err.corrections
   end
 end
 

actionview/test/template/render_test.rb

@@ -627,31 +627,29 @@ def test_render_partial_with_layout_raises_descriptive_error
     assert_match "Missing partial /_true with", e.message
   end
 
-  if defined?(DidYouMean) && DidYouMean.respond_to?(:correct_error)
-    def test_render_partial_provides_spellcheck
-      e = assert_raises(ActionView::MissingTemplate) { @view.render(partial: "test/partail") }
-      assert_match %r{Did you mean\?  test/partial\n *test/partialhtml}, e.message
-    end
+  def test_render_partial_provides_spellcheck
+    e = assert_raises(ActionView::MissingTemplate) { @view.render(partial: "test/partail") }
+    assert_match %r{Did you mean\?  test/partial\n *test/partialhtml}, e.message
+  end
 
-    def test_spellcheck_doesnt_list_directories
-      e = assert_raises(ActionView::MissingTemplate) { @view.render(partial: "test/directory") }
-      assert_match %r{Did you mean\?}, e.message
-      assert_no_match %r{Did you mean\?  test/directory\n}, e.message # test/hello is a directory
-    end
+  def test_spellcheck_doesnt_list_directories
+    e = assert_raises(ActionView::MissingTemplate) { @view.render(partial: "test/directory") }
+    assert_match %r{Did you mean\?}, e.message
+    assert_no_match %r{Did you mean\?  test/directory\n}, e.message # test/hello is a directory
+  end
 
-    def test_spellcheck_only_lists_templates
-      e = assert_raises(ActionView::MissingTemplate) { @view.render(template: "test/partial") }
+  def test_spellcheck_only_lists_templates
+    e = assert_raises(ActionView::MissingTemplate) { @view.render(template: "test/partial") }
 
-      assert_match %r{Did you mean\?}, e.message
-      assert_no_match %r{Did you mean\?  test/partial\n}, e.message
-    end
+    assert_match %r{Did you mean\?}, e.message
+    assert_no_match %r{Did you mean\?  test/partial\n}, e.message
+  end
 
-    def test_spellcheck_only_lists_partials
-      e = assert_raises(ActionView::MissingTemplate) { @view.render(partial: "test/template") }
+  def test_spellcheck_only_lists_partials
+    e = assert_raises(ActionView::MissingTemplate) { @view.render(partial: "test/template") }
 
-      assert_match %r{Did you mean\?}, e.message
-      assert_no_match %r{Did you mean\?  test/template\n}, e.message
-    end
+    assert_match %r{Did you mean\?}, e.message
+    assert_no_match %r{Did you mean\?  test/template\n}, e.message
   end
 
   def test_render_partial_wrong_details_no_spellcheck

activerecord/lib/active_record/associations.rb

@@ -13,28 +13,16 @@ def initialize(record = nil, association_name = nil)
       end
     end
 
-    class Correction
-      def initialize(error)
-        @error = error
-      end
+    if defined?(DidYouMean::Correctable) && defined?(DidYouMean::SpellChecker)
+      include DidYouMean::Correctable
 
       def corrections
-        if @error.association_name
-          maybe_these = @error.record.class.reflections.keys
-
-          maybe_these.sort_by { |n|
-            DidYouMean::Jaro.distance(@error.association_name.to_s, n)
-          }.reverse.first(4)
-        else
-          []
+        @corrections ||= begin
+          maybe_these = record&.class&.reflections&.keys
+          DidYouMean::SpellChecker.new(dictionary: maybe_these).correct(association_name.to_s)
         end
       end
     end
-
-    # We may not have DYM, and DYM might not let us register error handlers
-    if defined?(DidYouMean) && DidYouMean.respond_to?(:correct_error)
-      DidYouMean.correct_error(self, Correction)
-    end
   end
 
   class InverseOfAssociationNotFoundError < ActiveRecordError #:nodoc:
@@ -49,28 +37,20 @@ def initialize(reflection = nil, associated_class = nil)
       end
     end
 
-    class Correction
-      def initialize(error)
-        @error = error
-      end
+    if defined?(DidYouMean::Correctable) && defined?(DidYouMean::SpellChecker)
+      include DidYouMean::Correctable
 
       def corrections
-        if @error.reflection && @error.associated_class
-          maybe_these = @error.associated_class.reflections.keys
-
-          maybe_these.sort_by { |n|
-            DidYouMean::Jaro.distance(@error.reflection.options[:inverse_of].to_s, n)
-          }.reverse.first(4)
+        if reflection && associated_class
+          @corrections ||= begin
+            maybe_these = associated_class.reflections.keys
+            DidYouMean::SpellChecker.new(dictionary: maybe_these).correct(reflection.options[:inverse_of].to_s)
+          end
         else
           []
         end
       end
     end
-
-    # We may not have DYM, and DYM might not let us register error handlers
-    if defined?(DidYouMean) && DidYouMean.respond_to?(:correct_error)
-      DidYouMean.correct_error(self, Correction)
-    end
   end
 
   class HasManyThroughAssociationNotFoundError < ActiveRecordError #:nodoc:
@@ -86,29 +66,21 @@ def initialize(owner_class = nil, reflection = nil)
       end
     end
 
-    class Correction
-      def initialize(error)
-        @error = error
-      end
+    if defined?(DidYouMean::Correctable) && defined?(DidYouMean::SpellChecker)
+      include DidYouMean::Correctable
 
       def corrections
-        if @error.reflection && @error.owner_class
-          maybe_these = @error.owner_class.reflections.keys
-          maybe_these -= [@error.reflection.name.to_s] # remove failing reflection
-
-          maybe_these.sort_by { |n|
-            DidYouMean::Jaro.distance(@error.reflection.options[:through].to_s, n)
-          }.reverse.first(4)
+        if owner_class && reflection
+          @corrections ||= begin
+            maybe_these = owner_class.reflections.keys
+            maybe_these -= [reflection.name.to_s] # remove failing reflection
+            DidYouMean::SpellChecker.new(dictionary: maybe_these).correct(reflection.options[:through].to_s)
+          end
         else
           []
         end
       end
     end
-
-    # We may not have DYM, and DYM might not let us register error handlers
-    if defined?(DidYouMean) && DidYouMean.respond_to?(:correct_error)
-      DidYouMean.correct_error(self, Correction)
-    end
   end
 
   class HasManyThroughAssociationPolymorphicSourceError < ActiveRecordError #:nodoc: