From 4ef3e3491e2ecc34e7f4de1221d5ad7b8b4a1e24 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Mon, 9 Apr 2018 12:19:18 +0100
Subject: Add cop for has_many :through without disabled autoloading

Goldiloader is great, but has several issues with has_many :through relations:

* https://github.com/salsify/goldiloader/issues/12
* https://github.com/salsify/goldiloader/issues/14
* https://github.com/salsify/goldiloader/issues/18

Rather than try to figure out which applies in each case, we should just do the
drudge work of manually disabling autoloading for all relations of this type. We
can always use regular preloading for specific cases, but this way we avoid
generating invalid queries through Goldiloader's magic.
---
 rubocop/cop/gitlab/has_many_through_scope.rb | 45 ++++++++++++++++++++++++++++
 rubocop/rubocop.rb                           |  3 +-
 2 files changed, 47 insertions(+), 1 deletion(-)
 create mode 100644 rubocop/cop/gitlab/has_many_through_scope.rb

(limited to 'rubocop')

diff --git a/rubocop/cop/gitlab/has_many_through_scope.rb b/rubocop/cop/gitlab/has_many_through_scope.rb
new file mode 100644
index 00000000000..770a2a0529f
--- /dev/null
+++ b/rubocop/cop/gitlab/has_many_through_scope.rb
@@ -0,0 +1,45 @@
+require 'gitlab/styles/rubocop/model_helpers'
+
+module RuboCop
+  module Cop
+    module Gitlab
+      class HasManyThroughScope < RuboCop::Cop::Cop
+        include ::Gitlab::Styles::Rubocop::ModelHelpers
+
+        MSG = 'Always provide an explicit scope calling auto_include(false) when using has_many :through'.freeze
+
+        def_node_search :through?, <<~PATTERN
+          (pair (sym :through) _)
+        PATTERN
+
+        def_node_matcher :has_many_through?, <<~PATTERN
+          (send nil? :has_many ... #through?)
+        PATTERN
+
+        def_node_search :disables_auto_include?, <<~PATTERN
+          (send _ :auto_include false)
+        PATTERN
+
+        def_node_matcher :scope_disables_auto_include?, <<~PATTERN
+          (block (send nil? :lambda) _ #disables_auto_include?)
+        PATTERN
+
+        def on_send(node)
+          return unless in_model?(node)
+          return unless has_many_through?(node)
+
+          target = node
+          scope_argument = node.children[3]
+
+          if scope_argument.children[0].children.last == :lambda
+            return if scope_disables_auto_include?(scope_argument)
+
+            target = scope_argument
+          end
+
+          add_offense(target, location: :expression)
+        end
+      end
+    end
+  end
+end
diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb
index 406ec95ffc9..c2254332e7d 100644
--- a/rubocop/rubocop.rb
+++ b/rubocop/rubocop.rb
@@ -1,7 +1,8 @@
 # rubocop:disable Naming/FileName
+require_relative 'cop/gitlab/has_many_through_scope'
+require_relative 'cop/gitlab/httparty'
 require_relative 'cop/gitlab/module_with_instance_variables'
 require_relative 'cop/gitlab/predicate_memoization'
-require_relative 'cop/gitlab/httparty'
 require_relative 'cop/include_sidekiq_worker'
 require_relative 'cop/line_break_around_conditional_block'
 require_relative 'cop/migration/add_column'
-- 
cgit v1.2.3