Child controller's before action is overridden by parent

[samzhao2008]

Steps to reproduce

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", branch: "main"
end

require "action_controller/railtie"
require 'rails/test_help'
class TestApp < Rails::Application
  config.root = __dir__
  config.hosts << "www.example.com"
  secrets.secret_key_base = "secret_key_base"

  config.logger = Logger.new($stdout)
  Rails.logger  = config.logger

  routes.draw do
    get "/child_index" => "child#index"
    get "/child_show" => "child#show"
    get "/base_index" => "base#index"
    get "/base_show" => "base#show"
  end
end

class BaseController < ActionController::Base
  def index
    render plain: "BaseController#index"
  end

  def show
    render plain: "BaseController#show"
  end
end

class ChildController < BaseController
  def index
    render plain: "ChildController#index"
  end

  def show
    render plain: "ChildController#show"
  end
end

module ChildControllerDecorator
  def self.prepended(base)
    base.before_action :set_instance_var, only: :index
  end

  def set_instance_var
    @instance_var = :index
  end
end
ChildController.prepend(ChildControllerDecorator)

module BaseControllerDecorator
  def self.prepended(base)
    base.before_action :set_instance_var, only: :show
  end

  def set_instance_var
    @instance_var = :show
  end
end
BaseController.prepend(BaseControllerDecorator)

require "minitest/autorun"

class ChildControllerTest < ActionDispatch::IntegrationTest
  
  test "instance var should equal to :index when child index action is invoked" do
    get "/child_index"
    assert_equal :index, @controller.instance_variable_get(:@instance_var)
  end

  test "instance var should equal to :nil when child show action is invoked" do
    get "/child_show"
    assert_equal nil, @controller.instance_variable_get(:@instance_var)
  end

  private
    def app
      Rails.application
    end
end

Expected behavior

ChildController's before action should not be overridden by parent. In the test case, @instance_var should be set to :index, and only be set when index action is executed.

Actual behavior

ChildController's before action is overridden by parent. wrong execution condition and value.

System configuration

Rails version: main branch

Ruby version: 2.7.2

In order to figure it out, I read the source code. It seems every time the before_action or prepend_before_action is executed, there is a operation called remove_duplicates which remove the filters with the same name from itself and its descendants. It should not remove filters with the same name from its descendants, right?

[RolandStuder]

I think this is regular behavior of Ruby. You used prepend which puts the modules before the actual classes into the ancestor chain. So as you prepend the BaseController, this will show up in the ancestor chain before the ChildController. With an include it should work fine

This article might be helpful https://medium.com/@leo_hetsch/ruby-modules-include-vs-prepend-vs-extend-f09837a5b073

[samzhao2008]

@RolandStuder
Thank you for your comment. But the issue here is not about the usage and normal behavior of ruby prepend or include. It's about before_action. BaseController's before action set_instance_var overrides ChildController's before actionset_instance_var.

[Austio]

To get this to pass

remove BaseController.prepend(BaseControllerDecorator)

or

in BaseControllerDecorator, set only: :show to only: :index

[samzhao2008]

@Austio Thanks. I'm reporting a potential bug, modifying the test doesn't make sense here. Actually the code I posted is a simplified version I met in real life. I found the wired behavior and dig into the source code a little bit, and it seems there are something not quite right.

[Austio]

@samzhao2008 reporting investigation so others can pick up and don't have to take those steps

This also works using Classes for callbacks instead of methods, i think i see where this bug could be. Hopefully adjusting to classes will unblock your more complex case.

module ChildControllerDecorator
  def self.prepended(base)
    base.before_action SetInstanceVariable, only: :index
  end

  class SetInstanceVariable
    def self.before(controller)
      unless controller.instance_variable_set(:@instance_var, :index)
      end
    end
  end
end
ChildController.prepend(ChildControllerDecorator)

module BaseControllerDecorator
  def self.prepended(base)
    base.before_action SetInstanceVariable, only: :show
  end

  class SetInstanceVariable
    def self.before(controller)
      unless controller.instance_variable_set(:@instance_var, :show)
      end
    end
  end
end

[Austio]

On using classes

I think your best bet here is to use Classes to define your callbacks, then the load order will not matter here at all.

The reason that this works with classes is due to the callbacks only deduping when we have a symbol

On load order

If you switch the order of prepending (do Base first, then Child) there is not an issue here.

I'm not sure if this is a bug in callbacks because the load order is different than what i would normally expect (normally meaning a plain rails project with no manual includes like the example) from what would be loaded in a rails/ruby application. There very well could be a "cache key" issue that i'd like to dig deeper into. I'd like to see if i can reproduce any other error scenarios here before writing this off.

This is happening because internally when we define callbacks on the Base after they are defined on the Child, we update the callbacks on the Child with the method.

# Current behavior, this fails
ChildController.prepend(ChildControllerDecorator)
BaseController.prepend(BaseControllerDecorator)

# "normal" loading order, this will work
BaseController.prepend(BaseControllerDecorator)
ChildController.prepend(ChildControllerDecorator)

[samzhao2008]

@Austio Thanks for introducing to use class for before_action. I never know that before. I think the method/symbol way is a more common use case. So, the behavior should be consistent in all cases. If it's not, it's a bug which should be fixed. Yes, switch the 2 lines works. But, it's just the way to make the test pass. In a real life project these lines of code may in many different files, we can not and should not care about the load order.

[Austio]

@samzhao2008 I see what you mean here. The logic for removing duplicates was added to prevent duplicates within the same controller. I'll see if i can patch this in a reasonable way.

In meantime, encourage you to define these callbacks with Classes to sidestep the load order issue.

[samzhao2008]

@Austio nice! I'm also trying to fix it.