Fix digesting non-HTML templates with non-unique logical names #25411

Merged
merged 5 commits into from Jun 16, 2016

Projects

None yet

3 participants

@javan
Member
javan commented Jun 15, 2016 edited

This fixes a pathological-sounding-but-it-happened-to-us scenario that caused incorrect template digests for */* requests that render non-HTML (JSON, in our case) templates. The scene looks something like this:

  1. An appended view path: append_view_path(Rails.root.join("app/views/api"))
  2. Partials in this path with the same logical path as partials in app/views, but for different formats: app/views/api/todos/_todo.json.jbuilder and app/views/todos/_todo.html.erb, both "logically" known as todos/todo. Both of these partials have cache fragments.
  3. A controller action (schedules#show) with a corresponding JSON template (show.json.jbuilder) that in turn renders a collection of partials: json.todos @todos, partial: 'todos/todo', as: :todo.
  4. A client-side fetch requests to this action that doesn't set an Accept header so it defaults to */*.

The JSON response is correctly returned, but the template digest it uses for caching is computed using the HTML templates. Altering _todo.json.jbuilder does not invalidate its cache, but altering _todo.html.erb does.

We worked around the issue by adding an Accept: application/json header. Adding a .json format to the URL would have worked around it too.

Here are the new tests failing without the changes to ActionView::Digestor:

$ rake test TEST=test/template/digestor_test.rb
Run options: --seed 2948

# Running:

........F.............F...............

Finished in 5.274653s, 7.2043 runs/s, 10.4272 assertions/s.

  1) Failure:
TemplateDigestorTest#test_template_formats_of_nested_deps_with_non_default_rendered_format [/Users/javan/Projects/rails/actionview/test/template/digestor_test.rb:165]:
Expected: [:json]
  Actual: [:json, :html]


  2) Failure:
TemplateDigestorTest#test_different_formats_with_same_logical_template_names_results_in_different_digests [/Users/javan/Projects/rails/actionview/test/template/digestor_test.rb:287]:
Expected "18824a8cf013bb976bc39b5809a262c4" to not be equal to "18824a8cf013bb976bc39b5809a262c4".

38 runs, 55 assertions, 2 failures, 0 errors, 0 skips

All green with the changes.

/cc @jeremy @matthewd

@matthewd matthewd merged commit 2ff5a17 into rails:master Jun 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@matthewd
Member

5-0-stable: 8c9204d

@javan
Member
javan commented Jun 16, 2016

Thank you! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment