From 98a5976b787fad0797bc5e3231c48ab3f400bce6 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Fri, 29 Mar 2019 11:23:05 +0000
Subject: [PATCH] Document ApplicationRecord / pluck_primary_key

We also enable the rubocop that makes it mandatory
---
 .rubocop.yml                        | 14 ++++++++++++++
 doc/development/sql.md              | 15 +++++++++++++++
 lib/tasks/tokens.rake               |  2 +-
 scripts/insert-rspec-profiling-data |  2 +-
 4 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/.rubocop.yml b/.rubocop.yml
index 9143966b864..648d59e8062 100644
--- a/.rubocop.yml
+++ b/.rubocop.yml
@@ -146,6 +146,20 @@ Naming/FileName:
     - XSS
     - GRPC
 
+Rails/ApplicationRecord:
+  Enabled: true
+  Exclude:
+    # Models in database migrations should not subclass from ApplicationRecord
+    # as they need to be as decoupled from application code as possible
+    - db/**/*.rb
+    - lib/gitlab/background_migration/**/*.rb
+    - lib/gitlab/database/**/*.rb
+    - spec/**/*.rb
+    - ee/db/**/*.rb
+    - ee/lib/gitlab/background_migration/**/*.rb
+    - ee/lib/ee/gitlab/background_migration/**/*.rb
+    - ee/spec/**/*.rb
+
 # GitLab ###################################################################
 
 Gitlab/ModuleWithInstanceVariables:
diff --git a/doc/development/sql.md b/doc/development/sql.md
index 47519d39e74..edeca7fb298 100644
--- a/doc/development/sql.md
+++ b/doc/development/sql.md
@@ -155,6 +155,21 @@ The _only_ time you should use `pluck` is when you actually need to operate on
 the values in Ruby itself (e.g. write them to a file). In almost all other cases
 you should ask yourself "Can I not just use a sub-query?".
 
+In line with our `CodeReuse/ActiveRecord` cop, you should only use forms like
+`pluck(:id)` or `pluck(:user_id)` within model code. In the former case, you can
+use the `ApplicationRecord`-provided `.pluck_primary_key` helper method instead.
+In the latter, you should add a small helper method to the relevant model.
+
+## Inherit from ApplicationRecord
+
+Most models in the GitLab codebase should inherit from `ApplicationRecord`,
+rather than from `ActiveRecord::Base`. This allows helper methods to be easily
+added.
+
+An exception to this rule exists for models created in database migrations. As
+these should be isolated from application code, they should continue to subclass
+from `ActiveRecord::Base`.
+
 ## Use UNIONs
 
 UNIONs aren't very commonly used in most Rails applications but they're very
diff --git a/lib/tasks/tokens.rake b/lib/tasks/tokens.rake
index eec024f9bbb..46635cd7c8f 100644
--- a/lib/tasks/tokens.rake
+++ b/lib/tasks/tokens.rake
@@ -24,7 +24,7 @@ namespace :tokens do
   end
 end
 
-class TmpUser < ActiveRecord::Base
+class TmpUser < ActiveRecord::Base # rubocop:disable Rails/ApplicationRecord
   include TokenAuthenticatable
 
   self.table_name = 'users'
diff --git a/scripts/insert-rspec-profiling-data b/scripts/insert-rspec-profiling-data
index 10e337b9972..b34379764e0 100755
--- a/scripts/insert-rspec-profiling-data
+++ b/scripts/insert-rspec-profiling-data
@@ -22,7 +22,7 @@ module RspecProfiling
         ENV['RSPEC_PROFILING_POSTGRES_URL']
       end
 
-      class Result < ActiveRecord::Base
+      class Result < ActiveRecord::Base # rubocop:disable Rails/ApplicationRecord
         acts_as_copy_target
       end
     end