Discuss adopting Rails 7.1+ config.application_controller.raise_on_missing_callback_actions

[eoinkelly]

Rails 7.1 enables a feature where it will raise exception if your controller callback references an action which doesn't exist. Unfortunately this check seems to be performed before the if:/unless: option of the callback is evaluated. More details on the feature:

• Blog post: https://www.shakacode.com/blog/rails-adds-ability-to-raise-error-on-missing-callback-actions/
• PR: https://github.com/rails/rails/pull/43487

Our generated ApplicationController includes the following:

# this example is from the devise variant but all variants have this (minus the
# `unless ...` stuff)
after_action :verify_authorized, except: :index, unless: :devise_controller?
after_action :verify_policy_scoped, only: :index, unless: :devise_controller?

These after_action raise exceptions if they are run in a controller which does not have an index action. For example, the devise SessionsController and RegistionsController e.g.

Unknown action
The index action could not be found for the :verify_policy_scoped
callback on Users::SessionsController, but it is listed in the controller's
:only option.
Raising for missing callback actions is a new default in Rails 7.1, if you'd
like to turn this off you can delete the option from the environment configurations
or set `config.action_controller.raise_on_missing_callback_actions` to `false`.

These after_actions will also fail on any controller we define in future that doesn't have an index.

What are our options for handling this?

Option 1: Just disable config.action_controller.raise_on_missing_callback_actions

Pros/Cons (++/--)

• ++ Smallest change possible to get 7.1 support
• -- We miss out on the new protection against typos that the new check provides

Option 2: Move the controller action name check into the if:/unless: options

I tried this locally and it does work.

Pros/Cons (++/--)

• ++ We get to keep our pundit checks and adopt the new Rails safety check

• -- The code is less obvious and more clever now. We can mitigate this a bit by trying to choose a good name for the if:/unless: method but it's still more surprising/clever than what we currently have.

  # Notice the absence of `except: ...` option. 
  # Definitely awkward naming. Maybe too awkward?
  after_action :verify_authorized, unless: :devise_controller_or_index_action?
  after_action :verify_policy_scoped, unless: :devise_controller_or_non_index_action?
  
  private
  
  def devise_controller_or_index_action?
    return true if action_name == "index"
    devise_controller?
  end
  
  def devise_controller_or_non_index_action?
    return true if action_name != "index"
    devise_controller?
  end

Option 3: Remove the pundit policy checks

I'm rejecting this option immediately because losing policy checks is a much worse outcome than missing out on a new safety check from Rails.

[joshmcarthur]

I'd prefer option one, at least for now while we see what approaches others take. Filters, particularly at the ApplicationController level, ideally shouldn't be making such assumptions about the shape a particular controller takes. I can see the value, but it feels like coding around it is worse.

[lukeify]

We have decided on option one.