Add new Rails/LexicallyScopedActionFilter cop

[wata727]

This cop checks that methods specified in the filter's only or except options are explicitly defined in the controller.

You can specify methods of superclass or methods added by mixins on the filter, but these confuse developers. If you specify methods where are defined on another controller, you should define the filter in that controller.

Also, as another advantage, you can notice the method name typo 😄

wdyt?

---

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).

[pocke]

You can use node.str_content.to_sym for readability, and it needs .to_sym.

[pocke]

BTW, probably we should add StrNode#value.

[wata727]

I think so. This should be done with another pull request.

[pocke]

Could you add a test case to check array of strings? For example:

before_action :something, only: %w[foo bar]

Maybe the cop does not pass the test case now.

[pocke]

I think it should be like the following.

node.values.map do |v|
  case v.type
  when :sym
    v.value
  when :str
    v.str_content.to_sym
  end
end.compact

I think the cop should ignore not-literal value.

[pocke]

And add test cases to check false positive.

expect_no_offenses <<-RUBY
before_action :require_login, except: 'health_check'

def health_check
end
RUBY

[pocke]

Maybe you can use message: keyword argument to avoid using the instance variable. Like add_offense(node, message: message(inexplicit_method_names))

[pocke]

Consider default resource(s) actions, that are index, show, create, ....
They are defined always. So we can use these actions without method definition.
For example, the following works.

# controller
class FooController < ApplicationController
  before_action :require_login, only: :show

  # empty action definition
end

<!-- app/views/foo/show.html.erb -->
Hello, <%= current_user.name >!

So I think the cop should not add any offenses for these resource(s) actions.

[bbatsov]

The name of the cop is pretty confusing, btw. It should be something like Inherited(or Included)ActionFilter or conversely LexicallyScopeActionFilter.

[wata727]

Thanks for feedback :)

And add test cases to check false positive.

Umm... I think that this example is not false positive :(

Consider default resource(s) actions, that are index, show, create, ....
They are defined always. So we can use these actions without method definition.
For example, the following works.

There are examples other than default resources that work without having to define methods on the same controller. The following example works correctly for /foo/bar.

# config/routes.rb
Rails.application.routes.draw do
  get 'foo/bar'
end

# controller
class FooController < ApplicationController
  before_action :require_login, only: :bar

  # empty action definition
end

<!-- app/views/foo/bar.html.erb -->
Hello, <%= current_user.name >!

I think that it is undesirable to define filter dependent on such as routes or views.
Is it because the default resource action should be avoided because it is often used?

The name of the cop is pretty confusing, btw. It should be something like Inherited(or Included)ActionFilter or conversely LexicallyScopeActionFilter.

The name is a very difficult problem...
It needs to be concerned that this cop does not point to the method executed by the filter.
I think LexicallyScopeActionFilter is good, but there may be better names.

[pocke]

And add test cases to check false positive.

Umm... I think that this example is not false positive :(

In the first version( efc1a5a ), I guess the cop added offense for the code, because the cop compared a string (in before_action) and symbol (in method definition). So I think we should have a test case for the case.

There are examples other than default resources that work without having to define methods on the same controller. The following example works correctly for /foo/bar.

Oh, I see. I think it's confusing behaviour...
So, I think the cop should not ignore the actions, and we should define actions explicitly.

[wata727]

I renamed the cop name to Rails/LexicallyScopeActionFilter.

[bbatsov]

I made a typo - I meant to suggest LexicallyScopedActionFilter. Sorry about that!

[Drenmi]

Initializing an empty array and then pushing into it inside an #each can be replaced with #map. 🙂

[wata727]

Umm, I wanted to use the type filter with #each_child_node.
Perhaps #map_child_node may be necessary.

[wata727]

Oops, I noticed that #each_child_node will returnEnumerator when not passing block. I will use this.

[koic]

Using reject instead of map will make compact unnecessary.

methods = array_values(methods_node).reject do |method|
  defined_methods.include?(method)
end

[bbatsov]

@wata727 Maybe you should also add this as a suggestion to the Rails style guide.

[bquorning]

Thank you for this very useful cop @wata727 ❤️