before_action, skip_before_action を強制する Custom Cop をつくる

特定の before_action / skip_before_action の記述を強制する Custom Cop を作りたくなったため、作成の過程で検討したことと、実施したことを記録する。

on_class を使う

着手当初、on_send で作成しようとしたが、before_action には only オプションがあるため、controller class に対して定義されているメソッドを知る必要があると考えた。

そのため、クラスで定義、利用されているメソッドの情報を Node から取得できる on_class を利用することとした。

尚、controller なのかどうかを確認するためのメソッドについては、rubocop-railsのRails/ActionControllerFlashBeforeRender にある action_controller? を参考にするとよい。

Controllerクラス上で定義されている public メソッドを抽出する

コントローラに定義されているメソッドは、Rails/ActionOrder の action_declarations が参考になる。

メソッド名は上記のリンクを参考に取得できるが、そのメソッドが public かどうかは RuboCop::Cop::DefNode の node_visibility や、RuboCop::Cop::DefNode の non_public? で抽出できる。

    def_node_search :action_declarations, '(def {%1} ...)'

    def public_method_names(node)
      action_declarations(node, actions).filter_map do |current|
        next if node_visibility(current) != :public || non_public?(current)

        current.method_name
      end
    end

    def actions
      %i[index show create update destroy]
      # .rubocop.yml でフィルタするメソッド名を管理してもよい
    end

ここで抽出した public メソッドが、before_action や skip_before_action の only オプション に渡される。

before_action/skip_before_action の only に指定されているメソッドを抽出する

ここでは対象のメソッドを filter_action! というメソッドを対象として記述する。

before_action は only つけたりつけなかったりができるため、この両方に対応する必要がある。only あり/なしを $ をつかった上で一気に書ける方法がよくわからなかったため、別々に定義した。

    def_node_search :filter_action, <<~PATTERN
      (send nil? :before_action (sym :filter_action!) (hash <(pair (sym :only) $_ ...)>))
    PATTERN

    def_node_search :filter_action_no_args?, <<~PATTERN
      (send nil? :before_action (sym :filter_action!))
    PATTERN

key である only に対応する value を $_ として受けて後で使えるようにしている。

また、super class や module で before_action を使っている場合に skip_before_action を使われる可能性があるため、これらの def_node_search を定義する。

    def_node_search :skip_filter_action, <<~PATTERN
      (send nil? :skip_before_action (sym :filter_action!) (hash <(pair (sym :only) $_ ...)>))
    PATTERN

    def_node_search :skip_filter_action_no_args?, <<~PATTERN
      (send nil? :skip_before_action (sym :filter_action!))
    PATTERN

$_ には only: value の value の部分が適用される。単体のシンボルになることも Array になることもあるので両方に対応するよう、以下のようにした。

    def filter_action_method_names(node)
      filter_action(node).flat_map do |arg|
        if arg.array_type?
          arg.values.map(&:value)
        elsif arg.sym_type?
          arg.value.to_sym
        end
      end
    end

    def skipped_filter_action_method_names(node)
      skip_filter_action(node).flat_map do |arg|
        if arg.array_type?
          arg.values.map(&:value)
        elsif arg.sym_type?
          arg.value.to_sym
        end
      end
    end

強制するロジックをつくる

警告のメッセージは簡易に書いているが、以下のようになった。

before_action と skip_before_action の only なしが共存しないこと、同一メソッドが両方の only の対象にならないこと、before_action 内 / skip_before_action 内で同一メソッドの指定が重複していないこと、only の対象となっていないメソッドがないこと、をチェックしている。

    def on_class(node)
      return unless action_controller?(node)

      public_method_names = public_method_names(node)
      filter_action_target = filter_action_method_names(node)
      skipped_target = skipped_filter_action_method_names(node)

      # public メソッドなければ終了する
      return if public_method_names.blank?

      # before_action :filter_action! と skip_before_action :filter_action! の引数なし両方の指定がある
      if filter_action_no_args?(node) && skip_filter_action_no_args?(node)
        add_offense(node, message: '重複指定エラー')
        return
      end

      # before_action :filter_action! 引数なしは全指定なのに skip_before_action :filter_action! にメソッド指定がある
      if filter_action_no_args?(node) && skipped_target.present?
        add_offense(node, message: '重複指定エラー')
        return
      end

      # skip_before_action :filter_action! 引数なしは全指定なのに before_action :filter_action! にメソッド指定がある
      if skip_filter_action_no_args?(node) && filter_action_target.present?
        add_offense(node, message: '重複指定エラー')
        return
      end

      # only にわたすメソッド に重複がある
      duplicated_methods = filter_action_target & skipped_target
      if duplicated_methods.present?
        add_offense(node, message: '重複指定エラー')
        return
      end

      # only にわたしたメソッドに重複がある
      dup_in_before_action = filter_action_target.select { |m| filter_action_target.count(m) > 1 }
      dup_in_skip_before_action = skipping_target.select { |m| skipping_target.count(m) > 1 }
      dup_methods = (dup_in_before_action + dup_in_skip_before_action).uniq
      if dup_methods.present?
        add_offense(node, message: '重複指定エラー')
        return
      end

      public_method_names.each do |method_name|
        next if filter_action_target.include?(method_name) || skipped_target.include?(method_name)

        add_offense(node, message: '指定漏れエラー')
      end
    end

before_action 内 / skip_before_action 内で同一メソッドの指定が重複していないこと、については、別の Custom Cop に切り出したほうが正しそうだが、ここでは含めるようにした。

まとめ

before_action / skip_before_action を強制する Custom Cop をつくる際に検討、記述したことをまとめた。

以下の点に注意して実装した。

• public メソッドを抽出して、それらに対し指定漏れがないようにする
• only 指定あり/指定なし で適用範囲が変わることを考慮にいれる
• メソッド指定の重複が before_action / skip_before_action で重複しないようにする
• only 内で対象メソッドの重複がないようにする
• 対象にするメソッドの指定漏れがないようにする

運用していく中で考慮漏れに露呈する可能性はあるが、その場合は追加で対処する。