Merge pull request #13414 from britto/jb-fix-dependency-matching

Improve ERB dependency detection

3 files changed, 184 insertions, 29 deletions

diff --git a/actionview/CHANGELOG.md b/actionview/CHANGELOG.md
index a57da72d8c..19877ca8cb 100644
--- a/actionview/CHANGELOG.md
+++ b/actionview/CHANGELOG.md
@@ -1,3 +1,10 @@
+*   Improved ERB dependency detection. New argument types and formattings for the `render`
+    calls can be matched.
+
+    Fixes #13074 and #13116
+
+    *João Britto*
+
 *   Use `display:none` instead of `display:inline` for hidden fields
 
     Fixes #6403
diff --git a/actionview/lib/action_view/dependency_tracker.rb b/actionview/lib/action_view/dependency_tracker.rb
index b2e8334077..0ccf2515c5 100644
--- a/actionview/lib/action_view/dependency_tracker.rb
+++ b/actionview/lib/action_view/dependency_tracker.rb
@@ -1,7 +1,7 @@
 require 'thread_safe'
 
 module ActionView
-  class DependencyTracker
+  class DependencyTracker # :nodoc:
     @trackers = ThreadSafe::Cache.new
 
     def self.find_dependencies(name, template)
@@ -23,24 +23,52 @@ module ActionView
       @trackers.delete(handler)
     end
 
-    class ERBTracker
+    class ERBTracker # :nodoc:
       EXPLICIT_DEPENDENCY = /# Template Dependency: (\S+)/
 
+      # A valid ruby identifier - suitable for class, method and specially variable names
+      IDENTIFIER = /
+        [[:alpha:]_] # at least one uppercase letter, lowercase letter or underscore
+        [[:word:]]*  # followed by optional letters, numbers or underscores
+      /x
+
+      # Any kind of variable name. e.g. @instance, @@class, $global or local.
+      # Possibly following a method call chain
+      VARIABLE_OR_METHOD_CHAIN = /
+        (?:\$|@{1,2})?            # optional global, instance or class variable indicator
+        (?:#{IDENTIFIER}\.)*      # followed by an optional chain of zero-argument method calls
+        (?<dynamic>#{IDENTIFIER}) # and a final valid identifier, captured as DYNAMIC
+      /x
+
+      # A simple string literal. e.g. "School's out!"
+      STRING = /
+        (?<quote>['"]) # an opening quote
+        (?<static>.*?) # with anything inside, captured as STATIC
+        \k<quote>      # and a matching closing quote
+      /x
+
+      # Part of any hash containing the :partial key
+      PARTIAL_HASH_KEY = /
+        (?:\bpartial:|:partial\s*=>) # partial key in either old or new style hash syntax
+        \s*                          # followed by optional spaces
+      /x
+
       # Matches:
-      #   render partial: "comments/comment", collection: commentable.comments
-      #   render "comments/comments"
-      #   render 'comments/comments'
-      #   render('comments/comments')
+      #   partial: "comments/comment", collection: @all_comments => "comments/comment"
+      #   (object: @single_comment, partial: "comments/comment") => "comments/comment"
       #
-      #   render(@topic)         => render("topics/topic")
-      #   render(topics)         => render("topics/topic")
-      #   render(message.topics) => render("topics/topic")
-      RENDER_DEPENDENCY = /
-        render\s*                     # render, followed by optional whitespace
-        \(?                           # start an optional parenthesis for the render call
-        (partial:|:partial\s+=>)?\s*  # naming the partial, used with collection -- 1st capture
-        ([@a-z"'][@\w\/\."']+)        # the template name itself -- 2nd capture
-      /x
+      #   "comments/comments"
+      #   'comments/comments'
+      #   ('comments/comments')
+      #
+      #   (@topic)         => "topics/topic"
+      #    topics          => "topics/topic"
+      #   (message.topics) => "topics/topic"
+      RENDER_ARGUMENTS = /\A
+        (?:\s*\(?\s*)                             # optional opening paren surrounded by spaces
+        (?:.*?#{PARTIAL_HASH_KEY})?               # optional hash, up to the partial key declaration
+        (?:#{STRING}|#{VARIABLE_OR_METHOD_CHAIN}) # finally, the dependency name of interest
+      /xm
 
       def self.call(name, template)
         new(name, template).dependencies
@@ -68,19 +96,33 @@ module ActionView
         end
 
         def render_dependencies
-          source.scan(RENDER_DEPENDENCY).
-            collect(&:second).uniq.
+          render_dependencies = []
+          render_calls = source.split(/\brender\b/).drop(1)
 
-            # render(@topic)         => render("topics/topic")
-            # render(topics)         => render("topics/topic")
-            # render(message.topics) => render("topics/topic")
-            collect { |name| name.sub(/\A@?([a-z_]+\.)*([a-z_]+)\z/) { "#{$2.pluralize}/#{$2.singularize}" } }.
+          render_calls.each do |arguments|
+            arguments.scan(RENDER_ARGUMENTS) do
+              add_dynamic_dependency(render_dependencies, Regexp.last_match[:dynamic])
+              add_static_dependency(render_dependencies, Regexp.last_match[:static])
+            end
+          end
 
-            # render("headline") => render("message/headline")
-            collect { |name| name.include?("/") ? name : "#{directory}/#{name}" }.
+          render_dependencies.uniq
+        end
+
+        def add_dynamic_dependency(dependencies, dependency)
+          if dependency
+            dependencies << "#{dependency.pluralize}/#{dependency.singularize}"
+          end
+        end
 
-            # replace quotes from string renders
-            collect { |name| name.gsub(/["']/, "") }
+        def add_static_dependency(dependencies, dependency)
+          if dependency
+            if dependency.include?('/')
+              dependencies << dependency
+            else
+              dependencies << "#{directory}/#{dependency}"
+            end
+          end
         end
 
         def explicit_dependencies
diff --git a/actionview/test/template/dependency_tracker_test.rb b/actionview/test/template/dependency_tracker_test.rb
index 7a9b4b26ac..df3a0602d1 100644
--- a/actionview/test/template/dependency_tracker_test.rb
+++ b/actionview/test/template/dependency_tracker_test.rb
@@ -1,3 +1,5 @@
+# encoding: utf-8
+
 require 'abstract_unit'
 require 'action_view/dependency_tracker'
 
@@ -52,23 +54,127 @@ class ERBTrackerTest < Minitest::Test
 
   def test_dependency_of_erb_template_with_number_in_filename
     template = FakeTemplate.new("<%# render 'messages/message123' %>", :erb)
-    tracker = make_tracker('messages/_message123', template)
+    tracker = make_tracker("messages/_message123", template)
 
     assert_equal ["messages/message123"], tracker.dependencies
   end
 
   def test_finds_dependency_in_correct_directory
     template = FakeTemplate.new("<%# render(message.topic) %>", :erb)
-    tracker = make_tracker('messages/_message', template)
+    tracker = make_tracker("messages/_message", template)
 
     assert_equal ["topics/topic"], tracker.dependencies
   end
 
   def test_finds_dependency_in_correct_directory_with_underscore
     template = FakeTemplate.new("<%# render(message_type.messages) %>", :erb)
-    tracker = make_tracker('message_types/_message_type', template)
+    tracker = make_tracker("message_types/_message_type", template)
 
     assert_equal ["messages/message"], tracker.dependencies
   end
-end
 
+  def test_dependency_of_erb_template_with_no_spaces_after_render
+    template = FakeTemplate.new("<%# render'messages/message' %>", :erb)
+    tracker = make_tracker("messages/_message", template)
+
+    assert_equal ["messages/message"], tracker.dependencies
+  end
+
+  def test_finds_no_dependency_when_render_begins_the_name_of_an_identifier
+    template = FakeTemplate.new("<%# rendering 'it useless' %>", :erb)
+    tracker = make_tracker("resources/_resource", template)
+
+    assert_equal [], tracker.dependencies
+  end
+
+  def test_finds_no_dependency_when_render_ends_the_name_of_another_method
+    template = FakeTemplate.new("<%# surrender 'to reason' %>", :erb)
+    tracker = make_tracker("resources/_resource", template)
+
+    assert_equal [], tracker.dependencies
+  end
+
+  def test_finds_dependency_on_multiline_render_calls
+    template = FakeTemplate.new("<%#
+      render :object => @all_posts,
+             :partial => 'posts' %>", :erb)
+
+    tracker = make_tracker("some/_little_posts", template)
+
+    assert_equal ["some/posts"], tracker.dependencies
+  end
+
+  def test_finds_multiple_unrelated_odd_dependencies
+    template = FakeTemplate.new("
+      <%# render('shared/header', title: 'Title') %>
+      <h2>Section title</h2>
+      <%# render@section %>
+    ", :erb)
+
+    tracker = make_tracker("multiple/_dependencies", template)
+
+    assert_equal ["shared/header", "sections/section"], tracker.dependencies
+  end
+
+  def test_finds_dependencies_for_all_kinds_of_identifiers
+    template = FakeTemplate.new("
+      <%# render $globals %>
+      <%# render @instance_variables %>
+      <%# render @@class_variables %>
+    ", :erb)
+
+    tracker = make_tracker("identifiers/_all", template)
+
+    assert_equal [
+      "globals/global",
+      "instance_variables/instance_variable",
+      "class_variables/class_variable"
+    ], tracker.dependencies
+  end
+
+  def test_finds_dependencies_on_method_chains
+    template = FakeTemplate.new("<%# render @parent.child.grandchildren %>", :erb)
+    tracker = make_tracker("method/_chains", template)
+
+    assert_equal ["grandchildren/grandchild"], tracker.dependencies
+  end
+
+  def test_finds_dependencies_with_special_characters
+    template = FakeTemplate.new("<%# render @pokémon, partial: 'ピカチュウ' %>", :erb)
+    tracker = make_tracker("special/_characters", template)
+
+    assert_equal ["special/ピカチュウ"], tracker.dependencies
+  end
+
+  def test_finds_dependencies_with_quotes_within
+    template = FakeTemplate.new(%{
+      <%# render "single/quote's" %>
+      <%# render 'double/quote"s' %>
+    }, :erb)
+
+    tracker = make_tracker("quotes/_single_and_double", template)
+
+    assert_equal ["single/quote's", 'double/quote"s'], tracker.dependencies
+  end
+
+  def test_finds_dependencies_with_extra_spaces
+    template = FakeTemplate.new(%{
+      <%= render              "header" %>
+      <%= render    partial:  "form" %>
+      <%= render              @message %>
+      <%= render ( @message.events ) %>
+      <%= render    :collection => @message.comments,
+                    :partial =>    "comments/comment" %>
+    }, :erb)
+
+    tracker = make_tracker("spaces/_extra", template)
+
+    assert_equal [
+      "spaces/header",
+      "spaces/form",
+      "messages/message",
+      "events/event",
+      "comments/comment"
+    ], tracker.dependencies
+  end
+end