Action View: Fallback to existing partial when possible #50852

seanpdoyle · 2024-01-23T15:19:52Z

Motivation / Background

A controller declared in the top-level module can render a top-level Active Model instance whose partial is declared in the root view directory (like articles/_article.html.erb).

A controller scoped within a module can render an Active Model instance whose partial is similarly scoped within view directory (like scoped/articles/_article.html.erb).

A controller scoped within a module cannot render an Active Model instance whose partial is declared in the root view directory (like articles/_article.html.erb), despite the absence of a similarly scoped partial.

This is intended behavior that's powered by
config.action_view.prefix_partial_path_with_controller_namespace = true (true by default).

This change was introduced in March of 2012 as part of #5625.

Detail

As a consumer of Action View, my intuition is that the lookup would fallback, in the same way that a controller that inherits from ApplicationController could define its own view, then rely on fallback to render an app/views/application partial.

This commit modifies the behavior to gracefully fall back to the root-level view partial.

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
Tests are added or updated if you fix a bug or add a feature.
CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

actionview/lib/action_view/renderer/object_renderer.rb

rafaelfranca

I think the behavior makes sense but we should not be manipularing the partial name to get the right object. This should probably be implemented in the lookup object not in render

actionview/lib/action_view/lookup_context.rb

seanpdoyle · 2024-04-20T19:04:50Z

actionview/lib/action_view/path_set.rb

@@ -43,7 +43,7 @@ def find(path, prefixes, partial, details, details_key, locals)
    end

    def find_all(path, prefixes, partial, details, details_key, locals)
-      search_combinations(prefixes) do |resolver, prefix|
+      search_combinations(*prefixes, path.pluralize) do |resolver, prefix|


While this passes the suite, this change feels too far removed from the original problem. The resolved directory is coincidentally the path name pluralized.

I believe resolving the issue will require changes to how the ObjectRenderer constructs its set of prefixes to prepend to the ViewPath (depending on the config.action_view.prefix_partial_path_with_controller_namespace configuration value).

I like where you're going with this. 👍 You may want consider another case in your testing. Your current solution adds a search combination that strips all namespaces, which doesn't seem to cover when a controller-namespaced view would like to render a model from another namespace...

For example, if Scoped::Article had an ActiveStorage file attribute:

class Scoped::Article < ApplicationRecord has_one_attached :file end

The file attribute is is namespaced under ActiveStorage, with to_partial_path of "active_storage/attachments/attachment".

If we try to render with two partials:

<%# app/views/scoped/articles/_article.html.erb %> Rendered controller-namespaced article <%= render article.file %> <%# app/views/active_storage/attachments/_attachment.html.erb %> Rendered namespaced attachment

Your current idea removes both "scoped" and "active_storage" namespaces, so the attachment's partial wouldn't be found with:

- search_combinations(*["scoped/active_storage/attachments", "attachments"]) # => ActionView::MissingTemplate - Missing partial scoped/active_storage/attachments/_attachment

Would it make sense to add search paths as each namespace is removed?:

+ search_combinations(*["scoped/active_storage/attachments", "active_storage/attachments", "attachments"])

Then folks aren't forced to pick all-or-nothing namespacing, but can use any combination that suits their needs. Perhaps they'd have an "override" partial in one namespace, but use a general one elsewhere.

Thanks for your work on this!

seanpdoyle · 2024-10-04T13:25:11Z

actionview/test/actionpack/controller/render_test.rb

+  def test_rendering_with_different_namespaces
+    @controller = Fun::GamesController.new
+    def @controller.hello_world
+      file = ::ActiveStorage::Attachment.new("file.txt")
+      namespaced_article = ::Namespaced::Article.new(file)
+      render partial: namespaced_article
+    end
+
+    get :hello_world
+    assert_equal "Rendered attachment: file.txt", @response.body
+  end


@ansonhoyt thank you for the feedback!

I've tried to incorporate test coverage for the scenario you've outlined. Does the test coverage mimic what you had in mind?

Yes, these look good.

It's all fun and games until someone's model has a different namespace. 😆

seanpdoyle · 2024-10-04T13:26:16Z

actionview/lib/action_view/path_set.rb

@@ -56,7 +56,7 @@ def exists?(path, prefixes, partial, details, details_key, locals)

    private
      def search_combinations(prefixes)
-        prefixes = Array(prefixes)
+        prefixes = Array(prefixes).flat_map { |prefix| [prefix, prefix.split("/").tap(&:shift).join("/")] }.uniq


The goal of this block is to transform an Array like ["fun/active_storage/attachments/_attachment"] into an Array with duplicates removed consisting of

%w[ fun/active_storage/attachments/_attachment active_storage/attachments/_attachment attachments/_attachment ]

I know there is a more clear and concise way to achieve this outcome, so please share any suggestions you might have. Something like String#delete_prefix, but prefix.delete_until("/")

Seems reasonable. Sorry I'm not in my codebase atm, but is this just doing one shift on each prefix? If so, a prefix of fun/active_storage/attachments/ would now gain active_storage/attachments/ but not attachments/?

Hoping to circle back to my effected app in a couple weeks and play with your search_combinations change.

That's a good point, the current implementation only omits a single prefix at a time. I think it might need to recurse until it's out of directory names.

Closes [rails#50844][] Motivation / Background --- A controller declared in the top-level module can render a top-level Active Model instance whose partial is declared in the root view directory (like `articles/_article.html.erb`). A controller scoped within a module can render an Active Model instance whose partial is similarly scoped within view directory (like `scoped/articles/_article.html.erb`). A controller scoped within a module cannot render an Active Model instance whose partial is declared in the root view directory (like `articles/_article.html.erb`), despite the absence of a similarly scoped partial. This is intended behavior that's powered by [`config.action_view.prefix_partial_path_with_controller_namespace = true`][prefix_partial_path_with_controller_namespace] (`true` by default). This change was introduced in March of 2012 as part of [rails#5625][]. Detail --- As a consumer of Action View, my intuition is that the lookup would fallback, in the same way that a controller that inherits from `ApplicationController` could define its own view, then rely on fallback to render an `app/views/application` partial. This commit modifies the behavior to gracefully fall back to the root-level view partial. Checklist --- Before submitting the PR make sure the following are checked: * [x] This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs. * [x] Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: `[Fix #issue-number]` * [x] Tests are added or updated if you fix a bug or add a feature. * [x] CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included. [#59844]: rails#50844 [prefix_partial_path_with_controller_namespace]: https://guides.rubyonrails.org/configuring.html#config-action-view-prefix-partial-path-with-controller-namespace [rails#5625]: rails#5625

rails-bot bot added the actionview label Jan 23, 2024

seanpdoyle mentioned this pull request Jan 23, 2024

ActionView::Template::Error: Missing Active Model partial when rendered from Controller declared in module #50844

Open

seanpdoyle commented Jan 23, 2024

View reviewed changes

actionview/lib/action_view/renderer/object_renderer.rb Outdated Show resolved Hide resolved

seanpdoyle force-pushed the issue-50844 branch from 9f467f1 to 901bd68 Compare March 2, 2024 15:09

seanpdoyle force-pushed the issue-50844 branch 3 times, most recently from 63a2281 to 387e939 Compare April 19, 2024 17:42

rafaelfranca requested changes Apr 19, 2024

View reviewed changes

seanpdoyle force-pushed the issue-50844 branch from 387e939 to 56f574a Compare April 20, 2024 16:55

seanpdoyle requested a review from rafaelfranca April 20, 2024 16:55

seanpdoyle commented Apr 20, 2024

View reviewed changes

actionview/lib/action_view/lookup_context.rb Outdated Show resolved Hide resolved

seanpdoyle force-pushed the issue-50844 branch from 56f574a to 334de31 Compare April 20, 2024 18:46

seanpdoyle commented Apr 20, 2024

View reviewed changes

seanpdoyle force-pushed the issue-50844 branch 2 times, most recently from 6f1147a to 065ae94 Compare May 16, 2024 17:15

seanpdoyle force-pushed the issue-50844 branch 2 times, most recently from e6bb0a2 to a6e4d98 Compare October 4, 2024 13:24

seanpdoyle commented Oct 4, 2024

View reviewed changes

seanpdoyle requested a review from ansonhoyt October 4, 2024 13:25

seanpdoyle commented Oct 4, 2024

View reviewed changes

seanpdoyle force-pushed the issue-50844 branch from a6e4d98 to 78ea822 Compare October 4, 2024 15:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Action View: Fallback to existing partial when possible #50852

Action View: Fallback to existing partial when possible #50852

seanpdoyle commented Jan 23, 2024 •

edited

Loading

rafaelfranca left a comment

seanpdoyle Apr 20, 2024

ansonhoyt Jun 4, 2024

seanpdoyle Oct 4, 2024

ansonhoyt Oct 4, 2024

seanpdoyle Oct 4, 2024 •

edited

Loading

ansonhoyt Oct 4, 2024

seanpdoyle Oct 4, 2024

Action View: Fallback to existing partial when possible #50852

Are you sure you want to change the base?

Action View: Fallback to existing partial when possible #50852

Conversation

seanpdoyle commented Jan 23, 2024 • edited Loading

Motivation / Background

Detail

Checklist

rafaelfranca left a comment

Choose a reason for hiding this comment

seanpdoyle Apr 20, 2024

Choose a reason for hiding this comment

ansonhoyt Jun 4, 2024

Choose a reason for hiding this comment

seanpdoyle Oct 4, 2024

Choose a reason for hiding this comment

ansonhoyt Oct 4, 2024

Choose a reason for hiding this comment

seanpdoyle Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

ansonhoyt Oct 4, 2024

Choose a reason for hiding this comment

seanpdoyle Oct 4, 2024

Choose a reason for hiding this comment

seanpdoyle commented Jan 23, 2024 •

edited

Loading

seanpdoyle Oct 4, 2024 •

edited

Loading