From 0409ed57acf4f0ef93b3dcc665a16e3ac02dbee6 Mon Sep 17 00:00:00 2001 From: Petrik Date: Mon, 31 May 2021 18:27:47 +0200 Subject: [PATCH] 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: https://github.com/rails/rails/pull/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 --- actionpack/lib/abstract_controller/base.rb | 21 +----- .../lib/action_controller/metal/exceptions.rb | 23 ++----- .../metal/strong_parameters.rb | 21 +----- actionpack/test/controller/base_test.rb | 12 ++-- .../test/controller/required_params_test.rb | 20 +++--- .../test/dispatch/debug_exceptions_test.rb | 26 ++++--- actionpack/test/dispatch/routing_test.rb | 12 ++-- actionview/test/template/render_test.rb | 38 +++++------ .../lib/active_record/associations.rb | 68 ++++++------------- .../test/cases/associations/eager_test.rb | 12 ++-- .../associations/inverse_associations_test.rb | 30 ++++---- .../cases/associations/join_model_test.rb | 12 ++-- activerecord/test/models/author.rb | 2 +- activerecord/test/models/face.rb | 2 +- activerecord/test/models/human.rb | 2 +- railties/lib/rails/command/base.rb | 2 +- 16 files changed, 108 insertions(+), 195 deletions(-) diff --git a/actionpack/lib/abstract_controller/base.rb b/actionpack/lib/abstract_controller/base.rb index f2ff5738e3..c2ed3a5b56 100644 --- a/actionpack/lib/abstract_controller/base.rb +++ b/actionpack/lib/abstract_controller/base.rb @@ -16,28 +16,13 @@ module AbstractController 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 diff --git a/actionpack/lib/action_controller/metal/exceptions.rb b/actionpack/lib/action_controller/metal/exceptions.rb index 11fc816d08..22332a5ca9 100644 --- a/actionpack/lib/action_controller/metal/exceptions.rb +++ b/actionpack/lib/action_controller/metal/exceptions.rb @@ -33,29 +33,18 @@ module ActionController 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 + @corrections ||= begin + maybe_these = routes&.named_routes&.helper_names&.grep(/#{route_name}/) || [] + maybe_these -= [method_name.to_s] # remove exact match - maybe_these.sort_by { |n| - DidYouMean::Jaro.distance(@error.route_name, n) - }.reverse.first(4) - else - [] + 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: diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index c14220bf6e..167b9c725c 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -27,28 +27,13 @@ module ActionController 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 diff --git a/actionpack/test/controller/base_test.rb b/actionpack/test/controller/base_test.rb index d76d6854dd..c3d9f0287e 100644 --- a/actionpack/test/controller/base_test.rb +++ b/actionpack/test/controller/base_test.rb @@ -172,14 +172,12 @@ class PerformActionTest < ActionController::TestCase 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 diff --git a/actionpack/test/controller/required_params_test.rb b/actionpack/test/controller/required_params_test.rb index c4ec4fdf74..12cbccf671 100644 --- a/actionpack/test/controller/required_params_test.rb +++ b/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 diff --git a/actionpack/test/dispatch/debug_exceptions_test.rb b/actionpack/test/dispatch/debug_exceptions_test.rb index 2a45ce1e32..bb104fbd64 100644 --- a/actionpack/test/dispatch/debug_exceptions_test.rb +++ b/actionpack/test/dispatch/debug_exceptions_test.rb @@ -64,7 +64,7 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest [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 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest 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 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest 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 diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb index d3cffa86a3..ee3a1cb86b 100644 --- a/actionpack/test/dispatch/routing_test.rb +++ b/actionpack/test/dispatch/routing_test.rb @@ -4913,11 +4913,9 @@ class TestUrlGenerationErrors < ActionDispatch::IntegrationTest 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 @@ class TestUrlGenerationErrors < ActionDispatch::IntegrationTest # 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 diff --git a/actionview/test/template/render_test.rb b/actionview/test/template/render_test.rb index e050797a62..f109c7189a 100644 --- a/actionview/test/template/render_test.rb +++ b/actionview/test/template/render_test.rb @@ -627,31 +627,29 @@ module RenderTestCases 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 diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index f5ea027c67..778dc6dc78 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -13,28 +13,16 @@ module ActiveRecord 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 @@ module ActiveRecord 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 @@ module ActiveRecord 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: diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb index b38bc384db..96e187703c 100644 --- a/activerecord/test/cases/associations/eager_test.rb +++ b/activerecord/test/cases/associations/eager_test.rb @@ -848,13 +848,11 @@ class EagerAssociationTest < ActiveRecord::TestCase assert_match(/Association named 'monkeys' was not found on Post; perhaps you misspelled it\?/, e.message) end - if defined?(DidYouMean) && DidYouMean.respond_to?(:correct_error) - test "exceptions have suggestions for fix" do - error = assert_raise(ActiveRecord::AssociationNotFoundError) { - Post.all.merge!(includes: :monkeys).find(6) - } - assert_match "Did you mean?", error.message - end + test "exceptions have suggestions for fix" do + error = assert_raise(ActiveRecord::AssociationNotFoundError) { + Post.all.merge!(includes: :taggingz).find(6) + } + assert_match "Did you mean? tagging\n", error.message end def test_eager_has_many_through_with_order diff --git a/activerecord/test/cases/associations/inverse_associations_test.rb b/activerecord/test/cases/associations/inverse_associations_test.rb index eb991dc6aa..8c855f66ac 100644 --- a/activerecord/test/cases/associations/inverse_associations_test.rb +++ b/activerecord/test/cases/associations/inverse_associations_test.rb @@ -336,15 +336,13 @@ class InverseHasOneTests < ActiveRecord::TestCase assert_raise(ActiveRecord::InverseOfAssociationNotFoundError) { Human.first.confused_face } end - if defined?(DidYouMean) && DidYouMean.respond_to?(:correct_error) - def test_trying_to_use_inverses_that_dont_exist_should_have_suggestions_for_fix - error = assert_raise(ActiveRecord::InverseOfAssociationNotFoundError) { - Human.first.confused_face - } + def test_trying_to_use_inverses_that_dont_exist_should_have_suggestions_for_fix + error = assert_raise(ActiveRecord::InverseOfAssociationNotFoundError) { + Human.first.confused_face + } - assert_match "Did you mean?", error.message - assert_equal "super_human", error.corrections.first - end + assert_match "Did you mean?", error.message + assert_equal "confused_human", error.corrections.first end end @@ -751,18 +749,16 @@ class InverseBelongsToTests < ActiveRecord::TestCase end def test_trying_to_use_inverses_that_dont_exist_should_raise_an_error - assert_raise(ActiveRecord::InverseOfAssociationNotFoundError) { Face.first.puzzled_human } + assert_raise(ActiveRecord::InverseOfAssociationNotFoundError) { Face.first.confused_human } end - if defined?(DidYouMean) && DidYouMean.respond_to?(:correct_error) - def test_trying_to_use_inverses_that_dont_exist_should_have_suggestions_for_fix - error = assert_raise(ActiveRecord::InverseOfAssociationNotFoundError) { - Face.first.puzzled_human - } + def test_trying_to_use_inverses_that_dont_exist_should_have_suggestions_for_fix + error = assert_raise(ActiveRecord::InverseOfAssociationNotFoundError) { + Face.first.confused_human + } - assert_match "Did you mean?", error.message - assert_equal "confused_face", error.corrections.first - end + assert_match "Did you mean?", error.message + assert_equal "confused_face", error.corrections.first end def test_building_has_many_parent_association_inverses_one_record diff --git a/activerecord/test/cases/associations/join_model_test.rb b/activerecord/test/cases/associations/join_model_test.rb index a9f0b3358a..13a8f7def4 100644 --- a/activerecord/test/cases/associations/join_model_test.rb +++ b/activerecord/test/cases/associations/join_model_test.rb @@ -341,13 +341,11 @@ class AssociationsJoinModelTest < ActiveRecord::TestCase assert_raise(ActiveRecord::HasManyThroughAssociationNotFoundError) { authors(:david).nothings } end - if defined?(DidYouMean) && DidYouMean.respond_to?(:correct_error) - def test_exceptions_have_suggestions_for_fix - error = assert_raise(ActiveRecord::HasManyThroughAssociationNotFoundError) { - authors(:david).nothings - } - assert_match "Did you mean?", error.message - end + def test_exceptions_have_suggestions_for_fix + error = assert_raise(ActiveRecord::HasManyThroughAssociationNotFoundError) { + authors(:david).nothings + } + assert_match "Did you mean?", error.message end def test_has_many_through_join_model_with_conditions diff --git a/activerecord/test/models/author.rb b/activerecord/test/models/author.rb index 64eca27f74..0d97b10552 100644 --- a/activerecord/test/models/author.rb +++ b/activerecord/test/models/author.rb @@ -149,7 +149,7 @@ class Author < ActiveRecord::Base has_many :categorized_posts, through: :categorizations, source: :post has_many :unique_categorized_posts, -> { distinct }, through: :categorizations, source: :post - has_many :nothings, through: :kateggorisatons, class_name: "Category" + has_many :nothings, through: :kateggorizatons, class_name: "Category" has_many :author_favorites has_many :favorite_authors, -> { order("name") }, through: :author_favorites diff --git a/activerecord/test/models/face.rb b/activerecord/test/models/face.rb index 49de4b1f64..1811dc8494 100644 --- a/activerecord/test/models/face.rb +++ b/activerecord/test/models/face.rb @@ -8,7 +8,7 @@ class Face < ActiveRecord::Base # Oracle identifier length is limited to 30 bytes or less, `polymorphic` renamed `poly` belongs_to :poly_human_without_inverse, polymorphic: true # These are "broken" inverse_of associations for the purposes of testing - belongs_to :puzzled_human, class_name: "Human", inverse_of: :puzzled_face + belongs_to :confused_human, class_name: "Human", inverse_of: :cnffused_face belongs_to :puzzled_polymorphic_human, polymorphic: true, inverse_of: :puzzled_polymorphic_face validate do diff --git a/activerecord/test/models/human.rb b/activerecord/test/models/human.rb index 606a818640..e32a4902d1 100644 --- a/activerecord/test/models/human.rb +++ b/activerecord/test/models/human.rb @@ -24,7 +24,7 @@ class Human < ActiveRecord::Base after_add: :add_called, inverse_of: :polymorphic_human # These are "broken" inverse_of associations for the purposes of testing - has_one :confused_face, class_name: "Face", inverse_of: :confused_human + has_one :confused_face, class_name: "Face", inverse_of: :cnffused_human has_many :secret_interests, class_name: "Interest", inverse_of: :secret_human has_one :mixed_case_monkey diff --git a/railties/lib/rails/command/base.rb b/railties/lib/rails/command/base.rb index 2b37074466..f025b38616 100644 --- a/railties/lib/rails/command/base.rb +++ b/railties/lib/rails/command/base.rb @@ -26,7 +26,7 @@ module Rails include DidYouMean::Correctable def corrections - DidYouMean::SpellChecker.new(dictionary: options).correct(key) + @corrections ||= DidYouMean::SpellChecker.new(dictionary: options).correct(key) end end end