Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use the lookup_context to find the correct template path #42437

Merged
merged 1 commit into from Jun 23, 2021
Merged

Use the lookup_context to find the correct template path #42437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion actionpack/lib/action_controller/metal/etag_with_template_digest.rb
View file
Open in desktop
Expand Up @@ -44,7 +44,7 @@ def determine_template_etag(options)
# template digest from the ETag.
def pick_template_for_etag(options)
unless options[:template] == false
options[:template] || "#{controller_path}/#{action_name}"
options[:template] || lookup_context.find_all(action_name, _prefixes).first&.virtual_path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not lookup_context.find(action_name, _prefixes).virtual_path ? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

find will raise an error if it doesn't find the template. I don't think we want to raise here since we didn't raise before. It fails a lot of actionpack tests that don't have templates but ask about the etag.

Maybe it would be worth switching to find? should we raise if it can't find the template you want as part of your etag? That would be new behavior, but might help people find problems with their cache keys? I am unsure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HParker I would avoid trying to solve problems people aren't experiencing, this PR already adds enough value IMO ❤️

end
end

Expand Down
28 changes: 28 additions & 0 deletions actionpack/test/controller/render_test.rb
View file
Open in desktop
Expand Up @@ -65,6 +65,12 @@ def hello_world
end
end

class InheritedRenderTestController < ImplicitRenderTestController
def hello_world
fresh_when(etag: "abc")
end
end

class TestController < ActionController::Base
protect_from_forgery

Expand Down Expand Up @@ -724,6 +730,28 @@ def test_etag_reflects_template_digest
end
end

class InheritedEtagRenderTest < ActionController::TestCase
tests InheritedRenderTestController
include TemplateModificationHelper

def test_etag_reflects_template_digest
get :hello_world
assert_response :ok
assert_not_nil etag = @response.etag

request.if_none_match = etag
get :hello_world
assert_response :not_modified

modify_template("implicit_render_test/hello_world") do
request.if_none_match = etag
get :hello_world
assert_response :ok
assert_not_equal etag, @response.etag
end
end
end

class MetalRenderTest < ActionController::TestCase
tests MetalTestController

Expand Down