Commit 5819ca1a authored by Yorick Peterse's avatar Yorick Peterse

Added Cop to blacklist polymorphic associations

One should really use a separate table instead of using polymorphic
associations.

See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11168 for
more information.
parent 44d65c36
...@@ -5,7 +5,7 @@ class AwardEmoji < ActiveRecord::Base ...@@ -5,7 +5,7 @@ class AwardEmoji < ActiveRecord::Base
include Participable include Participable
include GhostUser include GhostUser
belongs_to :awardable, polymorphic: true belongs_to :awardable, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
belongs_to :user belongs_to :user
validates :awardable, :user, presence: true validates :awardable, :user, presence: true
......
...@@ -4,7 +4,7 @@ class Deployment < ActiveRecord::Base ...@@ -4,7 +4,7 @@ class Deployment < ActiveRecord::Base
belongs_to :project, required: true, validate: true belongs_to :project, required: true, validate: true
belongs_to :environment, required: true, validate: true belongs_to :environment, required: true, validate: true
belongs_to :user belongs_to :user
belongs_to :deployable, polymorphic: true belongs_to :deployable, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
validates :sha, presence: true validates :sha, presence: true
validates :ref, presence: true validates :ref, presence: true
......
...@@ -47,7 +47,7 @@ class Event < ActiveRecord::Base ...@@ -47,7 +47,7 @@ class Event < ActiveRecord::Base
belongs_to :author, class_name: "User" belongs_to :author, class_name: "User"
belongs_to :project belongs_to :project
belongs_to :target, polymorphic: true belongs_to :target, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
# For Hash only # For Hash only
serialize :data # rubocop:disable Cop/ActiverecordSerialize serialize :data # rubocop:disable Cop/ActiverecordSerialize
......
class LabelLink < ActiveRecord::Base class LabelLink < ActiveRecord::Base
include Importable include Importable
belongs_to :target, polymorphic: true belongs_to :target, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
belongs_to :label belongs_to :label
validates :target, presence: true, unless: :importing? validates :target, presence: true, unless: :importing?
......
...@@ -8,7 +8,7 @@ class Member < ActiveRecord::Base ...@@ -8,7 +8,7 @@ class Member < ActiveRecord::Base
belongs_to :created_by, class_name: "User" belongs_to :created_by, class_name: "User"
belongs_to :user belongs_to :user
belongs_to :source, polymorphic: true belongs_to :source, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
delegate :name, :username, :email, to: :user, prefix: true delegate :name, :username, :email, to: :user, prefix: true
......
...@@ -41,7 +41,7 @@ class Note < ActiveRecord::Base ...@@ -41,7 +41,7 @@ class Note < ActiveRecord::Base
participant :author participant :author
belongs_to :project belongs_to :project
belongs_to :noteable, polymorphic: true, touch: true belongs_to :noteable, polymorphic: true, touch: true # rubocop:disable Cop/PolymorphicAssociations
belongs_to :author, class_name: "User" belongs_to :author, class_name: "User"
belongs_to :updated_by, class_name: "User" belongs_to :updated_by, class_name: "User"
belongs_to :last_edited_by, class_name: 'User' belongs_to :last_edited_by, class_name: 'User'
......
...@@ -4,7 +4,7 @@ class NotificationSetting < ActiveRecord::Base ...@@ -4,7 +4,7 @@ class NotificationSetting < ActiveRecord::Base
default_value_for :level, NotificationSetting.levels[:global] default_value_for :level, NotificationSetting.levels[:global]
belongs_to :user belongs_to :user
belongs_to :source, polymorphic: true belongs_to :source, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
belongs_to :project, foreign_key: 'source_id' belongs_to :project, foreign_key: 'source_id'
validates :user, presence: true validates :user, presence: true
......
class RedirectRoute < ActiveRecord::Base class RedirectRoute < ActiveRecord::Base
belongs_to :source, polymorphic: true belongs_to :source, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
validates :source, presence: true validates :source, presence: true
......
class Route < ActiveRecord::Base class Route < ActiveRecord::Base
belongs_to :source, polymorphic: true belongs_to :source, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
validates :source, presence: true validates :source, presence: true
......
...@@ -2,7 +2,7 @@ class SentNotification < ActiveRecord::Base ...@@ -2,7 +2,7 @@ class SentNotification < ActiveRecord::Base
serialize :position, Gitlab::Diff::Position # rubocop:disable Cop/ActiverecordSerialize serialize :position, Gitlab::Diff::Position # rubocop:disable Cop/ActiverecordSerialize
belongs_to :project belongs_to :project
belongs_to :noteable, polymorphic: true belongs_to :noteable, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
belongs_to :recipient, class_name: "User" belongs_to :recipient, class_name: "User"
validates :project, :recipient, presence: true validates :project, :recipient, presence: true
......
class Subscription < ActiveRecord::Base class Subscription < ActiveRecord::Base
belongs_to :user belongs_to :user
belongs_to :project belongs_to :project
belongs_to :subscribable, polymorphic: true belongs_to :subscribable, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
validates :user, :subscribable, presence: true validates :user, :subscribable, presence: true
......
...@@ -22,7 +22,7 @@ class Todo < ActiveRecord::Base ...@@ -22,7 +22,7 @@ class Todo < ActiveRecord::Base
belongs_to :author, class_name: "User" belongs_to :author, class_name: "User"
belongs_to :note belongs_to :note
belongs_to :project belongs_to :project
belongs_to :target, polymorphic: true, touch: true belongs_to :target, polymorphic: true, touch: true # rubocop:disable Cop/PolymorphicAssociations
belongs_to :user belongs_to :user
delegate :name, :email, to: :author, prefix: true, allow_nil: true delegate :name, :email, to: :author, prefix: true, allow_nil: true
......
...@@ -2,7 +2,7 @@ class Upload < ActiveRecord::Base ...@@ -2,7 +2,7 @@ class Upload < ActiveRecord::Base
# Upper limit for foreground checksum processing # Upper limit for foreground checksum processing
CHECKSUM_THRESHOLD = 100.megabytes CHECKSUM_THRESHOLD = 100.megabytes
belongs_to :model, polymorphic: true belongs_to :model, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
validates :size, presence: true validates :size, presence: true
validates :path, presence: true validates :path, presence: true
......
class UserAgentDetail < ActiveRecord::Base class UserAgentDetail < ActiveRecord::Base
belongs_to :subject, polymorphic: true belongs_to :subject, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
validates :user_agent, :ip_address, :subject_id, :subject_type, presence: true validates :user_agent, :ip_address, :subject_id, :subject_type, presence: true
......
require_relative '../model_helpers'
module RuboCop module RuboCop
module Cop module Cop
# Cop that prevents the use of `serialize` in ActiveRecord models. # Cop that prevents the use of `serialize` in ActiveRecord models.
class ActiverecordSerialize < RuboCop::Cop::Cop class ActiverecordSerialize < RuboCop::Cop::Cop
include ModelHelpers
MSG = 'Do not store serialized data in the database, use separate columns and/or tables instead'.freeze MSG = 'Do not store serialized data in the database, use separate columns and/or tables instead'.freeze
def on_send(node) def on_send(node)
return unless in_models?(node) return unless in_model?(node)
add_offense(node, :selector) if node.children[1] == :serialize add_offense(node, :selector) if node.children[1] == :serialize
end end
def models_path
File.join(Dir.pwd, 'app', 'models')
end
def in_models?(node)
path = node.location.expression.source_buffer.name
path.start_with?(models_path)
end
end end
end end
end end
require_relative '../model_helpers'
module RuboCop
module Cop
# Cop that prevents the use of polymorphic associations
class PolymorphicAssociations < RuboCop::Cop::Cop
include ModelHelpers
MSG = 'Do not use polymorphic associations, use separate tables instead'.freeze
def on_send(node)
return unless in_model?(node)
return unless node.children[1] == :belongs_to
node.children.last.each_node(:pair) do |pair|
key_name = pair.children[0].children[0]
add_offense(pair, :expression) if key_name == :polymorphic
end
end
end
end
end
module RuboCop
module ModelHelpers
# Returns true if the given node originated from the models directory.
def in_model?(node)
path = node.location.expression.source_buffer.name
models_path = File.join(Dir.pwd, 'app', 'models')
path.start_with?(models_path)
end
end
end
...@@ -2,6 +2,7 @@ require_relative 'cop/custom_error_class' ...@@ -2,6 +2,7 @@ require_relative 'cop/custom_error_class'
require_relative 'cop/gem_fetcher' require_relative 'cop/gem_fetcher'
require_relative 'cop/activerecord_serialize' require_relative 'cop/activerecord_serialize'
require_relative 'cop/redirect_with_status' require_relative 'cop/redirect_with_status'
require_relative 'cop/polymorphic_associations'
require_relative 'cop/migration/add_column' require_relative 'cop/migration/add_column'
require_relative 'cop/migration/add_column_with_default_to_large_table' require_relative 'cop/migration/add_column_with_default_to_large_table'
require_relative 'cop/migration/add_concurrent_foreign_key' require_relative 'cop/migration/add_concurrent_foreign_key'
......
...@@ -10,7 +10,7 @@ describe RuboCop::Cop::ActiverecordSerialize do ...@@ -10,7 +10,7 @@ describe RuboCop::Cop::ActiverecordSerialize do
context 'inside the app/models directory' do context 'inside the app/models directory' do
it 'registers an offense when serialize is used' do it 'registers an offense when serialize is used' do
allow(cop).to receive(:in_models?).and_return(true) allow(cop).to receive(:in_model?).and_return(true)
inspect_source(cop, 'serialize :foo') inspect_source(cop, 'serialize :foo')
...@@ -23,7 +23,7 @@ describe RuboCop::Cop::ActiverecordSerialize do ...@@ -23,7 +23,7 @@ describe RuboCop::Cop::ActiverecordSerialize do
context 'outside the app/models directory' do context 'outside the app/models directory' do
it 'does nothing' do it 'does nothing' do
allow(cop).to receive(:in_models?).and_return(false) allow(cop).to receive(:in_model?).and_return(false)
inspect_source(cop, 'serialize :foo') inspect_source(cop, 'serialize :foo')
......
require 'spec_helper'
require 'rubocop'
require 'rubocop/rspec/support'
require_relative '../../../rubocop/cop/polymorphic_associations'
describe RuboCop::Cop::PolymorphicAssociations do
include CopHelper
subject(:cop) { described_class.new }
context 'inside the app/models directory' do
it 'registers an offense when polymorphic: true is used' do
allow(cop).to receive(:in_model?).and_return(true)
inspect_source(cop, 'belongs_to :foo, polymorphic: true')
aggregate_failures do
expect(cop.offenses.size).to eq(1)
expect(cop.offenses.map(&:line)).to eq([1])
end
end
end
context 'outside the app/models directory' do
it 'does nothing' do
allow(cop).to receive(:in_model?).and_return(false)
inspect_source(cop, 'belongs_to :foo, polymorphic: true')
expect(cop.offenses).to be_empty
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment