port ModelBehavior cop over from bridge, fixes SD-1119
test plan: * see test commit Change-Id: Ibf3a663c0e04cc463595a4f0bd6dae81e572eff3 Reviewed-on: https://gerrit.instructure.com/79564 Reviewed-by: Jon Jensen <jon@instructure.com> Tested-by: Jenkins Product-Review: Landon Wilkins <lwilkins@instructure.com> QA-Review: Landon Wilkins <lwilkins@instructure.com>
This commit is contained in:
parent
bb06990fe8
commit
6cb3339a72
|
@ -18,6 +18,27 @@ Migration/ConcurrentIndex: &Migration
|
|||
Include:
|
||||
- "**/db/migrate/*.rb"
|
||||
|
||||
Migration/ModelBehavior:
|
||||
Enabled: true
|
||||
Include:
|
||||
- "**/db/migrate/*.rb"
|
||||
- "lib/data_fixup/*.rb"
|
||||
Whitelist: [
|
||||
Canvas
|
||||
DataFixup
|
||||
Switchman
|
||||
Shard
|
||||
Account.default
|
||||
Account.site_admin
|
||||
*.quoted_table_name
|
||||
*.connection
|
||||
*.transaction
|
||||
*.find_ids_in_batches
|
||||
*.find_ids_in_ranges
|
||||
*.update_all
|
||||
*.delete_all
|
||||
]
|
||||
|
||||
Migration/PrimaryKey:
|
||||
<<: *Migration
|
||||
|
||||
|
|
|
@ -21,6 +21,7 @@ require 'rubocop_canvas/cops/lint/no_file_utils_rm_rf'
|
|||
require 'rubocop_canvas/cops/lint/no_sleep'
|
||||
## migration
|
||||
require 'rubocop_canvas/cops/migration/concurrent_index'
|
||||
require 'rubocop_canvas/cops/migration/model_behavior'
|
||||
require 'rubocop_canvas/cops/migration/primary_key'
|
||||
require 'rubocop_canvas/cops/migration/remove_column'
|
||||
require 'rubocop_canvas/cops/migration/send_later'
|
||||
|
|
|
@ -0,0 +1,98 @@
|
|||
require "active_support/core_ext/string/inflections"
|
||||
|
||||
module RuboCop
|
||||
module Cop
|
||||
module Migration
|
||||
class ModelBehavior < Cop
|
||||
def on_class(node)
|
||||
klass, _ = *node.to_a
|
||||
whitelist << resolve_const(klass)
|
||||
end
|
||||
|
||||
# e.g.
|
||||
# User = Class.new(ActiveRecord::Base)
|
||||
# SOME_ARRAY = [...]
|
||||
def on_casgn(node)
|
||||
parent, const = *node.to_a
|
||||
full_const = resolve_const(parent) << const
|
||||
whitelist << full_const
|
||||
end
|
||||
|
||||
def on_send(node)
|
||||
return if ignored_node?(node)
|
||||
|
||||
receiver, methods = resolve_receiver(node)
|
||||
receiver = resolve_const(receiver)
|
||||
return if receiver.empty?
|
||||
|
||||
# so that we don't get redundant errors for a big long chain of
|
||||
# method calls
|
||||
ignore_sends(node.to_a[0])
|
||||
|
||||
parts = receiver + methods
|
||||
return true if whitelist.any? do |whitelist_parts|
|
||||
if whitelist_parts[0] == :*
|
||||
methods.last == whitelist_parts.last
|
||||
else
|
||||
parts[0, whitelist_parts.length] == whitelist_parts
|
||||
end
|
||||
end
|
||||
|
||||
receiver = receiver.join("::")
|
||||
if model?(receiver)
|
||||
message = "If possible, avoid auto-loaded models in a migration; define them here so the behavior doesn't change (e.g. `#{receiver} = Class.new(ActiveRecord::Base)`)."
|
||||
else
|
||||
message = "If possible, avoid auto-loaded classes/modules in a migration; define any behavior you need here. If you really can't though, add it to the whitelist."
|
||||
end
|
||||
add_offense node, :expression, message, :convention
|
||||
end
|
||||
|
||||
def model?(const_name)
|
||||
File.exist?("app/models/#{const_name.underscore}.rb")
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def ignore_sends(node)
|
||||
while node && node.type == :send
|
||||
ignore_node node
|
||||
node = node.to_a[0]
|
||||
end
|
||||
end
|
||||
|
||||
# just ruby built-ins (e.g. Class, String, etc) ... this happens
|
||||
# to also get rubocop and its dependencies, but ¯\_(ツ)_/¯
|
||||
BASE_WHITELIST = Module.constants.map(&:to_s)
|
||||
|
||||
def whitelist
|
||||
@whitelist ||= (BASE_WHITELIST + cop_config["Whitelist"]).map do |item|
|
||||
item.split(/\.|::/).map(&:to_sym)
|
||||
end
|
||||
end
|
||||
|
||||
# follow chained methods calls back to receiver, e.g.
|
||||
# User.where("foo").order("lol").update_all =>
|
||||
# [receiver_node, [:where, :order, :update_all]]
|
||||
def resolve_receiver(node)
|
||||
receiver, method_name, _args = *node
|
||||
methods = []
|
||||
if receiver && receiver.type == :send
|
||||
receiver, methods = resolve_receiver(receiver)
|
||||
end
|
||||
methods << method_name
|
||||
[receiver, methods]
|
||||
end
|
||||
|
||||
def resolve_const(node)
|
||||
result = []
|
||||
while node
|
||||
return [] unless node.type == :const || node.type == :cbase
|
||||
node, value = node.to_a
|
||||
result.unshift value if value
|
||||
end
|
||||
result
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
|
@ -17,6 +17,8 @@ Gem::Specification.new do |spec|
|
|||
spec.add_runtime_dependency "rubocop", "0.35.1"
|
||||
spec.add_development_dependency "bundler", "~> 1.7"
|
||||
spec.add_development_dependency "pry", "~> 0.10.1"
|
||||
spec.add_development_dependency "pry-nav", "~> 0.2.4"
|
||||
spec.add_development_dependency "rake", "~> 10.0"
|
||||
spec.add_development_dependency "rspec", "~> 3.2.0"
|
||||
spec.add_development_dependency "activesupport", "~> 4.2"
|
||||
end
|
||||
|
|
|
@ -0,0 +1,80 @@
|
|||
describe RuboCop::Cop::Migration::ModelBehavior do
|
||||
let(:config) {
|
||||
RuboCop::Config.new(
|
||||
"Migration/ModelBehavior" => {
|
||||
"Enabled" => true,
|
||||
"Included" => ["- db/migrate/*"],
|
||||
"Whitelist" => [
|
||||
"Migrations::FooFix",
|
||||
"*.update_all",
|
||||
"*.delete_all"
|
||||
]
|
||||
}
|
||||
)
|
||||
}
|
||||
|
||||
subject { described_class.new(config) }
|
||||
|
||||
it "should find no offenses when calling whitelisted classes/methods" do
|
||||
inspect_source(subject, %{
|
||||
class Foo < ActiveRecord::Migration
|
||||
def up
|
||||
Migrations::FooFix.run
|
||||
end
|
||||
end
|
||||
})
|
||||
expect(subject.offenses.size).to eq(0)
|
||||
end
|
||||
|
||||
it "should find no offenses when calling whitelisted methods" do
|
||||
inspect_source(subject, %{
|
||||
class Foo < ActiveRecord::Migration
|
||||
def up
|
||||
User.foo.bar.baz.update_all(name: "sally")
|
||||
Course.where(id: [1,2,3]).delete_all
|
||||
end
|
||||
end
|
||||
})
|
||||
expect(subject.offenses.size).to eq(0)
|
||||
end
|
||||
|
||||
it "should find no offenses when calling receiver-less methods" do
|
||||
inspect_source(subject, %{
|
||||
class Foo < ActiveRecord::Migration
|
||||
module Lol
|
||||
end
|
||||
|
||||
include Lol
|
||||
end
|
||||
})
|
||||
expect(subject.offenses.size).to eq(0)
|
||||
end
|
||||
|
||||
it "should find no offenses when referencing classes defined in the file itself" do
|
||||
inspect_source(subject, %{
|
||||
class User < ActiveRecord::Base; end
|
||||
Course = Class.new(ActiveRecord::Base)
|
||||
|
||||
class Foo < ActiveRecord::Migration
|
||||
def up
|
||||
User.find_each { |u| u.save! }
|
||||
Course.find_each { |c| c.save! }
|
||||
end
|
||||
end
|
||||
})
|
||||
expect(subject.offenses.size).to eq(0)
|
||||
end
|
||||
|
||||
it "should error if referencing unknown/auto-loaded classes" do
|
||||
inspect_source(subject, %{
|
||||
class Foo < ActiveRecord::Migration
|
||||
def up
|
||||
User.find_each { |u| u.save! }
|
||||
Course.where(id: [1,2,3]).find_each { |c| c.save! }
|
||||
end
|
||||
end
|
||||
})
|
||||
expect(subject.offenses.size).to eq(2)
|
||||
expect(subject.offenses.all? { |off| off.severity.name == :convention })
|
||||
end
|
||||
end
|
Loading…
Reference in New Issue