Unexpected controller prefix path de-duplication in view partial lookup paths

[sfnelson]

Steps to reproduce

Create nested controller and nested object views where there is overlap between the controller and object namespaces. For example:

• Courses::Quiz::Question and Courses::Quiz::QuestionsController
• Courses::Quiz::Question and Learning::Quiz::Extra::QuestionsController
• Courses::Quiz::Question and Learning::Courses::Quiz::QuestionsController

# frozen_string_literal: true

require "bundler/inline"

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

  gem "rails"
end

require "action_controller/railtie"
require "active_model/railtie"

class TestApp < Rails::Application
  config.root = __dir__
  config.hosts << "example.org"
  config.secret_key_base = "secret_key_base"

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

  routes.draw do
    get "/unrelated" => "learning/questions#new"
    get "/duplication" => "courses/quiz/questions#new"
    get "/collision" => "learning/quiz/extra/questions#new"
    get "/repetition" => "learning/courses/quiz/questions#new"
  end
end

module Courses
  module Quiz
    class Question
      include ActiveModel::Model

      attr_accessor :name
    end

    class QuestionsController < ActionController::Base
      def new
        render partial: ::Courses::Quiz::Question.new(name: "partial")
      end
    end
  end
end

module Learning
  class QuestionsController < ActionController::Base
    def new
      render partial: ::Courses::Quiz::Question.new(name: "partial")
    end
  end

  module Quiz
    module Extra
      class QuestionsController < ActionController::Base
        def new
          render partial: ::Courses::Quiz::Question.new(name: "partial")
        end
      end
    end
  end

  module Courses
    module Quiz
      class QuestionsController < ActionController::Base
        def new
          render partial: ::Courses::Quiz::Question.new(name: "partial")
        end
      end
    end
  end
end

require "minitest/autorun"
require "rack/test"

class BugTest < Minitest::Test
  include Rack::Test::Methods

  def setup
    super
    @views = Dir.mktmpdir
    ActionController::Base.prepend_view_path @views
  end

  # ✅ PASSES
  def test_unrelated_partial
    view_file "learning/courses/quiz/questions/_question.html.erb", "Nested <%= question.name %>"

    get "/unrelated"
    assert_equal "Nested partial", last_response.body
  end

  # ✅ PASSES
  def test_duplication
    view_file "courses/quiz/questions/_question.html.erb", "De-duplicated <%= question.name %>"

    get "/duplication"
    assert_equal "De-duplicated partial", last_response.body
  end

  # ❌ FAILS
  # AbstractRenderer#merge_prefix_into_object_path breaks after the first match, skips 'extra'
  def test_collision
    view_file "learning/courses/quiz/questions/_question.html.erb", "Harmful de-duplication <%= question.name %>"
    view_file "learning/courses/extra/courses/quiz/questions/_question.html.erb", "Nested <%= question.name %>"

    get "/collision"
    assert_equal "Collision partial", last_response.body
  end

  # ❌ FAILS
  # AbstractRenderer#merge_prefix_into_object_path fails to find overlap because offsets do not align
  def test_repetition
    view_file "learning/courses/quiz/questions/_question.html.erb", "De-duplicated <%= question.name %>"
    view_file "learning/courses/quiz/courses/quiz/questions/_question.html.erb", "Nested <%= question.name %>"

    get "/repetition"
    assert_equal "De-duplicated partial", last_response.body
  end

  private

  def app
    Rails.application
  end

  def view_file(name, content)
    path = Pathname.new(@views).join(name)
    path.dirname.tap(&:mkpath)
    path.write(content)
  end
end

Expected behavior

• Courses::Quiz::Question and Courses::Quiz::QuestionsController
  module duplication is detected and removed
• Courses::Quiz::Question and Learning::Quiz::Extra::QuestionsController
  quiz/extra suffix on the controller does not match the quiz prefix on the model so no de-duplication is performed. Partial should be learning/courses/extra/courses/quiz/questions/_question.html.erb
• Courses::Quiz::Question and Learning::Courses::Quiz::QuestionsController
  courses/quiz suffix on controller matches courses/quiz prefix on model so repetition is removed. Partial should be learning/courses/quiz/questions/_question.html.erb

Actual behavior

• ✅ Courses::Quiz::Question and Courses::Quiz::QuestionsController
  module duplication is detected and removed
• ❌ Courses::Quiz::Question and Learning::Quiz::Extra::QuestionsController
  Quiz is detected as overlap and Extra is dropped from the partial path, this is confusing and inconsistent
• ❌ Courses::Quiz::Question and Learning::Courses::Quiz::QuestionsController
  no module duplication is detected, so full module path to controller is prefixed on the object path – deep repetition

System configuration

Rails version: 7.1.0

Ruby version: 3.2.3

[sfnelson]

I've reviewed the history of this code and it's not clear what the intended behaviour should be.

The original implementation of ActionView::PartialRenderer#merge_path_into_partial introduced in dc1b0fd would remove any tokens that were repeated at the same position, so example 2 (Courses::Quiz::Question and Learning::Quiz::Extra::QuestionsController) would work as described.

The logic changed in 2995134 to add a break as soon as the first overlapping module was detected, but this ignores any subsequent modules, and only requires that the module depth is the same. This is the behaviour that still exists today.

There is very little test coverage to explain what behaviour the authors intended to support, and the behaviour as currently implemented is arguably wrong and at best confusing for users.

The case we could like to see addressed is where a controller has a module prefix that is not present on the object path, example above is a bit contrived, but I suspect many developers will have controllers in Admin modules. The additional prefix means that controller/object de-duplication does not work because the relative offsets are different.

merge_prefix_into_object_path("admin/quiz/questions_controller", "quiz/questions/_question.html.erb")
# => `admin/quiz/questions/_question.html.erb`

An alternative implementation would look for overlap at the end of the controller path and the beginning of the object path, and remove any duplication it detects. Here's an example implementation:

prefixes = []
prefix_array = File.dirname(prefix).split("/")

prefix_array.each_with_index do |dir, index|
  break if object_path.start_with?(prefix_array[index..-1].join("/"))
  prefixes << dir
end

(prefixes << object_path).join("/")

I'd be happy to provide a PR if this behaviour is acceptable.

[sfnelson]

Previous discussion from 2011: #1951

[rails-bot]

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 7-1-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

[sfnelson]

I can still reproduce this on 7-1 and main. Could I get someone's eyes over it? It's a simple fix, so if there's appetite I'd be happy to provide a PR for consideration.