mostly working with strong_migrations integration

README.md

@@ -31,8 +31,3 @@ git clone --depth 1 git://github.com/bbatsov/rubocop.git vendor/rubocop
 After checking out the repo, run `bin/setup` to install dependencies. Then, run `rake spec` to run the tests. You can also run `bin/console` for an interactive prompt that will allow you to experiment.
 
 To install this gem onto your local machine, run `bundle exec rake install`. To release a new version, update the version number in `version.rb`, and then run `bundle exec rake release`, which will create a git tag for the version, push git commits and tags, and push the `.gem` file to [rubygems.org](https://rubygems.org).
-
-## TODO
-
-https://www.braintreepayments.com/blog/safe-operations-for-high-volume-postgresql/
-http://leopard.in.ua/2016/09/20/safe-and-unsafe-operations-postgresql

lib/rubocop-migration.rb

@@ -3,10 +3,11 @@
 require "active_record"
 # Only load a portion of strong_migrations
 require "strong_migrations/migration"
+require "strong_migrations/unsafe_migration"
 require "rubocop"
-require "safe_ruby"
 
 require "rubocop/migration/version"
+require "rubocop/migration/strong_migrations_checker"
 require "rubocop/cop/migration/unsafe_operation"
 
 module RuboCop

lib/rubocop/cop/migration/unsafe_operation.rb

@@ -30,7 +30,7 @@ class UnsafeOperation < Cop
         PATTERN
 
         def_node_search :schema_statement_match, <<-PATTERN
-          (send nil ${#{SCHEMA_STATEMENTS_PATTERN}} $...)
+          $(send nil ${#{SCHEMA_STATEMENTS_PATTERN}} $...)
         PATTERN
 
         def investigate(processed_source)
@@ -45,11 +45,14 @@ def investigate(processed_source)
         private
 
         def investigate_migration_method(method_node)
-          schema_statement_match(method_node) do |method_name, args_nodes|
+          schema_statement_match(method_node) do |statement_node, method_name, args_nodes|
             # TODO: Better/safer way to do this?
             args_source = "[#{args_nodes.map(&:source).join(', ')}]"
-            args = SafeRuby.eval(args_source)
-            p method_name, args
+            args = eval(args_source)
+
+            checker = RuboCop::Migration::StrongMigrationsChecker.new
+            error = checker.check_operation(method_name, *args)
+            add_offense(statement_node, :expression, error) if error
           end
         end
       end

lib/rubocop/migration/strong_migrations_checker.rb

@@ -0,0 +1,30 @@
+module RuboCop
+  module Migration
+    class StrongMigrationsChecker
+      include StrongMigrations::Migration
+
+      def version_safe?
+        false
+      end
+
+      def postgresql?
+        true
+      end
+
+      def check_operation(method_name, *args)
+        send(method_name, *args)
+      rescue StrongMigrations::UnsafeMigration => e
+        strip_wait_message(e.message)
+      rescue NoMethodError
+        # Do nothing, this method is unrecognized by StrongMigrations unsafe
+        # operations and is likely safe.
+      end
+
+      private
+
+      def strip_wait_message(error_message)
+        error_message.split("\n\n", 2).last
+      end
+    end
+  end
+end

rubocop-migration.gemspec

@@ -27,7 +27,6 @@ Gem::Specification.new do |spec|
   spec.add_dependency "activerecord", ">= 4"
   spec.add_dependency "strong_migrations", "~> 0.1"
   spec.add_dependency "rubocop", ">= 0.48.0"
-  spec.add_dependency "safe_ruby", "~> 1.0"
 
   spec.add_development_dependency "bundler", "~> 1.14"
   spec.add_development_dependency "rake", "~> 10.0"

spec/rubocop/cop/migration/unsafe_operation_spec.rb

@@ -5,14 +5,6 @@
     inspect_source(cop, source)
   end
 
-  # shared_examples "a non-concurrent index" do
-  #   it "registers an offense" do
-  #     expect(cop.offenses.size).to eq(1)
-  #     expect(cop.offenses.first.message).to eq("Use `algorithm: :concurrently` to avoid locking table.")
-  #     expect(cop.highlights).to eq([source])
-  #   end
-  # end
-  #
   shared_examples "valid code" do
     it "doesn't register an offense" do
       expect(cop.offenses).to be_empty
@@ -30,7 +22,7 @@ class MyModel < ActiveRecord::Base
     it_behaves_like "valid code"
   end
 
-  context "a valid migration class", :focus do
+  context "a valid migration class" do
     let(:source) do
       <<-SOURCE
       class AddSomeColumnToUsers < ActiveRecord::Migration[5.0]
@@ -39,6 +31,7 @@ def change
           if data_source_exists?
             add_column :users, :some_id, :integer
             add_index :users, :some_id, algorithm: :concurrently
+            add_reference :users, :something, index: false
           end
         end
       end
@@ -48,7 +41,7 @@ def change
     it_behaves_like "valid code"
   end
 
-  context "non-concurrent" do
+  context "add_index non-concurrently" do
     let(:source) do
       <<-SOURCE
       class AddSomeColumnToUsers < ActiveRecord::Migration
@@ -61,7 +54,45 @@ def change
     end
 
     it "registers an offense" do
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.offenses.first.message).to match(/\AAdding a non-concurrent index/)
+      expect(cop.highlights).to eq(["add_index :users, :some_id"])
+    end
+  end
 
+  context "add_reference with an index non-concurrently" do
+    let(:source) do
+      <<-SOURCE
+      class AddSomeColumnToUsers < ActiveRecord::Migration
+        def change
+          add_reference :users, :something, index: true
+        end
+      end
+      SOURCE
+    end
+
+    it "registers an offense" do
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.offenses.first.message).to match(/\AAdding a non-concurrent index/)
+      expect(cop.highlights).to eq(["add_reference :users, :something, index: true"])
+    end
+  end
+
+  context "add_column with default value" do
+    let(:source) do
+      <<-SOURCE
+      class AddSomeColumnToUsers < ActiveRecord::Migration
+        def change
+          add_column :users, :schmuck, :boolean, default: false
+        end
+      end
+      SOURCE
+    end
+
+    it "registers an offense" do
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.offenses.first.message).to match(/\AAdding a column with a non-null default/)
+      expect(cop.highlights).to eq(["add_column :users, :schmuck, :boolean, default: false"])
     end
   end
 end