WIP: Refactor cop with def_node_search

Makes it a simpler cop but maybe a bit more brittle?

2 files changed, 90 insertions, 148 deletions

diff --git a/rubocop/cop/security/to_json.rb b/rubocop/cop/security/to_json.rb
index 80aa67a277a..b15aff7aa66 100644
--- a/rubocop/cop/security/to_json.rb
+++ b/rubocop/cop/security/to_json.rb
@@ -6,21 +6,6 @@ module RuboCop
           (send !{nil hash} :to_json $...)
         PATTERN
 
-        def_node_matcher :with_include?, <<~PATTERN
-          (pair (sym :include) (hash $...))
-        PATTERN
-
-        def_node_matcher :with_only?, <<~PATTERN
-          (pair
-            (sym _)
-            (hash
-              (pair
-                (sym :only) (array ...)
-              )
-            )
-          )
-        PATTERN
-
         MSG = "Don't use `to_json` without specifying `only`".freeze
 
         def on_send(node)
@@ -40,12 +25,21 @@ module RuboCop
 
         private
 
+        def_node_matcher :with_include?, <<~PATTERN
+          (pair (sym :include) (hash $...))
+        PATTERN
+
+        def_node_search :with_only?, <<~PATTERN
+          (pair (sym :only) (array ...))
+        PATTERN
+
         def check_pairs(pairs)
-          # Find `include: {...}` pairs
-          with_include = pairs.collect { |pair| with_include?(pair) }.flatten.compact
-          return if with_include.empty?
+          nodes = pairs.collect { |pair| with_include?(pair) }.flatten.compact
+
+          # Check the pairs themselves if we didn't find any `include:` above
+          nodes = pairs if nodes.empty?
 
-          with_include.each do |(child_node)|
+          nodes.each do |child_node|
             next if with_only?(child_node)
 
             add_offense(child_node, :expression)
diff --git a/spec/rubocop/cop/security/to_json_spec.rb b/spec/rubocop/cop/security/to_json_spec.rb
index 97ab3c60f65..bb9e4b57c95 100644
--- a/spec/rubocop/cop/security/to_json_spec.rb
+++ b/spec/rubocop/cop/security/to_json_spec.rb
@@ -11,64 +11,111 @@ describe RuboCop::Cop::Security::ToJson do
   subject(:cop) { described_class.new }
 
   it 'ignores calls except `to_json`' do
-    inspect_source(cop, "render text: 'Hello'")
+    inspect_source(cop, 'render json: foo')
 
     expect(cop.offenses).to be_empty
   end
 
-  context 'to_json with `include`' do
-    it 'adds an offense' do
+  context 'to_json with options' do
+    it 'does nothing when provided `only`' do
+      # s(:hash,
+      #   s(:pair,
+      #     s(:sym, :only),
+      #     s(:array,
+      #       s(:sym, :name),
+      #       s(:sym, :username))))
       inspect_source(cop, <<~EOS)
-        render json: issue.to_json(
+        render json: @issue.to_json(only: [:name, :username])
+      EOS
+
+      expect(cop.offenses).to be_empty
+    end
+
+    it 'does nothing when provided `only` and `methods`' do
+      # s(:hash,
+      #   s(:pair,
+      #     s(:sym, :only),
+      #     s(:array,
+      #       s(:sym, :name),
+      #       s(:sym, :username))),
+      #   s(:pair,
+      #     s(:sym, :methods),
+      #     s(:array,
+      #       s(:sym, :avatar_url))))
+      inspect_source(cop, <<~EOS)
+        render json: @issue.to_json(only: [:name, :username], methods: [:avatar_url])
+      EOS
+
+      expect(cop.offenses).to be_empty
+    end
+
+    it 'adds an offense to `include`d attributes without `only` option' do
+      # s(:hash,
+      #   s(:pair,
+      #     s(:sym, :include),
+      #     s(:hash,
+      #       s(:pair,
+      #         s(:sym, :milestone),
+      #         s(:hash)),
+      #       s(:pair,
+      #         s(:sym, :assignee),
+      #         s(:hash,
+      #           s(:pair,
+      #             s(:sym, :methods),
+      #             s(:sym, :avatar_url)))),
+      #       s(:pair,
+      #         s(:sym, :author),
+      #         s(:hash,
+      #           s(:pair,
+      #             s(:sym, :only),
+      #             s(:array,
+      #               s(:str, "foo"),
+      #               s(:str, "bar"))))))))
+      inspect_source(cop, <<~EOS)
+        render json: @issue.to_json(
           include: {
             milestone: {},
             assignee: { methods: :avatar_url },
-            labels: { methods: :text_color }
-          },
-          methods: [:task_status, :task_status_short]
+            author: { only: %w[foo bar] },
+          }
         )
       EOS
 
       aggregate_failures do
-        expect(cop.offenses.size).to eq(3)
+        expect(cop.offenses.size).to eq(2)
         expect(cop.highlights).to contain_exactly(
           'milestone: {}',
-          'assignee: { methods: :avatar_url }',
-          'labels: { methods: :text_color }'
+          'assignee: { methods: :avatar_url }'
         )
       end
     end
 
-  end
-
-  context 'to_json without `include`' do
-    it 'does nothing when `only` is specified' do
-      source = %q(current_user.created_projects.where(import_type: "gitlab").to_json(only: [:id, :import_status]))
-      inspect_source(cop, source)
+    it 'adds an offense for `except`' do
+      inspect_source(cop, <<~EOS)
+        render json: @issue.to_json(except: [:private_token])
+      EOS
 
-      expect(cop.offenses).to be_empty
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(1)
+        expect(cop.highlights).to contain_exactly('except: [:private_token]')
+      end
     end
-
-    # it 'adds an offense without `only`' do
-    #   source = %q(current_user.created_projects.where(import_type: "gitlab").to_json(except: [:id, :import_status]))
-    #   inspect_source(cop, source)
-    #
-    #   aggregate_failures do
-    #     expect(cop.offenses.size).to eq(1)
-    #     expect(cop.highlights).to contain_exactly("except: [:id, :import_status]")
-    #   end
-    # end
   end
 
   context 'to_json without options' do
+    it 'does nothing when called with nil receiver' do
+      inspect_source(cop, 'to_json')
+
+      expect(cop.offenses).to be_empty
+    end
     it 'does nothing when called directly on a Hash' do
-      inspect_source(cop, "{}.to_json")
+      inspect_source(cop, '{}.to_json')
 
       expect(cop.offenses).to be_empty
     end
 
-    it 'adds an offense when called on object' do
-      inspect_source(cop, "foo.to_json")
+    it 'adds an offense when called on variable' do
+      inspect_source(cop, 'foo.to_json')
 
       aggregate_failures do
         expect(cop.offenses.size).to eq(1)
@@ -76,103 +123,4 @@ describe RuboCop::Cop::Security::ToJson do
       end
     end
   end
-
-  # context 'when a class has a body' do
-  #   it 'does nothing' do
-  #     inspect_source(cop, 'class CustomError < StandardError; def foo; end; end')
-  #
-  #     expect(cop.offenses).to be_empty
-  #   end
-  # end
-  #
-  # context 'when a class has no explicit superclass' do
-  #   it 'does nothing' do
-  #     inspect_source(cop, 'class CustomError; end')
-  #
-  #     expect(cop.offenses).to be_empty
-  #   end
-  # end
-  #
-  # context 'when a class has a superclass that does not end in Error' do
-  #   it 'does nothing' do
-  #     inspect_source(cop, 'class CustomError < BasicObject; end')
-  #
-  #     expect(cop.offenses).to be_empty
-  #   end
-  # end
-  #
-  # context 'when a class is empty and inherits from a class ending in Error' do
-  #   context 'when the class is on a single line' do
-  #     let(:source) do
-  #       <<-SOURCE
-  #         module Foo
-  #           class CustomError < Bar::Baz::BaseError; end
-  #         end
-  #       SOURCE
-  #     end
-  #
-  #     let(:expected) do
-  #       <<-EXPECTED
-  #         module Foo
-  #           CustomError = Class.new(Bar::Baz::BaseError)
-  #         end
-  #       EXPECTED
-  #     end
-  #
-  #     it 'registers an offense' do
-  #       expected_highlights = source.split("\n")[1].strip
-  #
-  #       inspect_source(cop, source)
-  #
-  #       aggregate_failures do
-  #         expect(cop.offenses.size).to eq(1)
-  #         expect(cop.offenses.map(&:line)).to eq([2])
-  #         expect(cop.highlights).to contain_exactly(expected_highlights)
-  #       end
-  #     end
-  #
-  #     it 'autocorrects to the right version' do
-  #       autocorrected = autocorrect_source(cop, source, 'foo/custom_error.rb')
-  #
-  #       expect(autocorrected).to eq(expected)
-  #     end
-  #   end
-  #
-  #   context 'when the class is on multiple lines' do
-  #     let(:source) do
-  #       <<-SOURCE
-  #         module Foo
-  #           class CustomError < Bar::Baz::BaseError
-  #           end
-  #         end
-  #       SOURCE
-  #     end
-  #
-  #     let(:expected) do
-  #       <<-EXPECTED
-  #         module Foo
-  #           CustomError = Class.new(Bar::Baz::BaseError)
-  #         end
-  #       EXPECTED
-  #     end
-  #
-  #     it 'registers an offense' do
-  #       expected_highlights = source.split("\n")[1..2].join("\n").strip
-  #
-  #       inspect_source(cop, source)
-  #
-  #       aggregate_failures do
-  #         expect(cop.offenses.size).to eq(1)
-  #         expect(cop.offenses.map(&:line)).to eq([2])
-  #         expect(cop.highlights).to contain_exactly(expected_highlights)
-  #       end
-  #     end
-  #
-  #     it 'autocorrects to the right version' do
-  #       autocorrected = autocorrect_source(cop, source, 'foo/custom_error.rb')
-  #
-  #       expect(autocorrected).to eq(expected)
-  #     end
-  #   end
-  # end
 end