mirrors
/
rails
mirror of https://github.com/rails/rails
8
0
Fork 0
Code Issues Projects Releases Wiki Activity

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 <yk.nishijima@gmail.com>
This commit is contained in:
Petrik 2021-05-31 18:27:47 +02:00
parent 7179dcb269
commit 0409ed57ac
16 changed files with 108 additions and 195 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
actionpack/lib/abstract_controller/base.rb
View File

@ -16,30 +16,15 @@ 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
[]
@corrections ||= DidYouMean::SpellChecker.new(dictionary: controller.class.action_methods).correct(action)
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
# AbstractController::Base is a low-level API. Nobody should be
# using it directly, and subclasses (like ActionController::Base) are
# expected to provide their own +render+ method, since rendering means

23
actionpack/lib/action_controller/metal/exceptions.rb
View File

@ -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:

21
actionpack/lib/action_controller/metal/strong_parameters.rb
View File

@ -27,30 +27,15 @@ 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
[]
@corrections ||= DidYouMean::SpellChecker.new(dictionary: keys).correct(param.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
# Raised when a supplied parameter is not expected and
# ActionController::Parameters.action_on_unpermitted_parameters
# is set to <tt>:raise</tt>.

4
actionpack/test/controller/base_test.rb
View File

@ -172,15 +172,13 @@ 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
get :ello
end
assert_match "Did you mean?", exception.message
end
end
def test_action_missing_should_work
use_controller ActionMissingController

6
actionpack/test/controller/required_params_test.rb
View File

@ -22,19 +22,17 @@ 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!" } }
post :create, params: { boko: { name: "Mjallo!" } }
end
assert_match "Did you mean?", error.message
error = assert_raise ActionController::ParameterMissing do
post :create, params: { book: { title: "Mjallo!" } }
post :create, params: { book: { naem: "Mjallo!" } }
end
assert_match "Did you mean?", error.message
end
end
test "required parameters that are present will not raise" do
post :create, params: { book: { name: "Mjallo!" } }

6
actionpack/test/dispatch/debug_exceptions_test.rb
View File

@ -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,7 +318,6 @@ 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
@ -332,7 +331,6 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest
assert_select("b", /Did you mean\?/)
assert_select("li", "valid_param_key")
end
end
test "rescue with HTML format for HTML API request" do
@app = ApiApp

6
actionpack/test/dispatch/routing_test.rb
View File

@ -4913,12 +4913,10 @@ 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
end
# FIXME: we should fix all locations that raise this exception to provide
# the info DidYouMean needs and then delete this test. Just adding the
@ -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

2
actionview/test/template/render_test.rb
View File

@ -627,7 +627,6 @@ 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
@ -652,7 +651,6 @@ module RenderTestCases
assert_match %r{Did you mean\?}, e.message
assert_no_match %r{Did you mean\? test/template\n}, e.message
end
end
def test_render_partial_wrong_details_no_spellcheck
e = assert_raises(ActionView::MissingTemplate) { @view.render(partial: "test/partial_with_only_html_version", formats: [:xml]) }

68
activerecord/lib/active_record/associations.rb
View File

@ -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:

6
activerecord/test/cases/associations/eager_test.rb
View File

@ -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)
Post.all.merge!(includes: :taggingz).find(6)
}
assert_match "Did you mean?", error.message
end
assert_match "Did you mean? tagging\n", error.message
end
def test_eager_has_many_through_with_order

10
activerecord/test/cases/associations/inverse_associations_test.rb
View File

@ -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
}
assert_match "Did you mean?", error.message
assert_equal "super_human", error.corrections.first
end
assert_equal "confused_human", error.corrections.first
end
end
@ -751,19 +749,17 @@ 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
Face.first.confused_human
}
assert_match "Did you mean?", error.message
assert_equal "confused_face", error.corrections.first
end
end
def test_building_has_many_parent_association_inverses_one_record
with_has_many_inversing(Interest) do

2
activerecord/test/cases/associations/join_model_test.rb
View File

@ -341,14 +341,12 @@ 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
end
def test_has_many_through_join_model_with_conditions
assert_equal [], posts(:welcome).invalid_taggings

2
activerecord/test/models/author.rb
View File

@ -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

2
activerecord/test/models/face.rb
View File

@ -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

2
activerecord/test/models/human.rb
View File

@ -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

2
railties/lib/rails/command/base.rb
View File

@ -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