Merge pull request #29 from Invoca/TECH-4995/restore-model-pk-detection

TECH-4995: fix bug detecting compound primary keys in Rails 4

Author: ColinDKelley

CHANGELOG.md

@@ -4,6 +4,10 @@ Inspired by [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 Note: this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [0.4.1] - Unreleased
+### Fixed
+- Fixed a bug detecting compound primary keys in Rails 4.
+
 ## [0.4.0] - 2020-11-20
 ### Added
 - Fields may be declared with `serialize: true` (any value with a valid `.to_yaml` stored as YAML),
@@ -61,6 +65,7 @@ using the appropriate Rails configuration attributes.
 ### Added
 - Initial version from https://github.com/Invoca/hobo_fields v4.1.0.
 
+[0.4.1]: https://github.com/Invoca/declare_schema/compare/v0.4.0...v0.4.1
 [0.4.0]: https://github.com/Invoca/declare_schema/compare/v0.3.1...v0.4.0
 [0.3.1]: https://github.com/Invoca/declare_schema/compare/v0.3.0...v0.3.1
 [0.3.0]: https://github.com/Invoca/declare_schema/compare/v0.2.0...v0.3.0

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    declare_schema (0.4.0)
+    declare_schema (0.4.1)
       rails (>= 4.2)
 
 GEM

lib/declare_schema/model/index_definition.rb

@@ -37,16 +37,9 @@ class << self
         def for_model(model, old_table_name = nil)
           t = old_table_name || model.table_name
 
-          primary_key_columns = Array(model.connection.primary_key(t)).presence || begin
-            cols = model.connection.columns(t)
-            Array(
-              if cols.any? { |col| col.name == 'id' }
-                'id'
-              else
-                cols.find { |col| col.type.to_s.include?('int') }&.name or raise "could not guess primary key for #{t} in #{cols.inspect}"
-              end
-            )
-          end
+          primary_key_columns = Array(model.connection.primary_key(t)).presence || sqlite_compound_primary_key(model, t) or
+            raise "could not find primary key for table #{t} in #{model.connection.columns(t).inspect}"
+
           primary_key_found = false
           index_definitions = model.connection.indexes(t).map do |i|
             model.ignore_indexes.include?(i.name) and next
@@ -55,7 +48,8 @@ def for_model(model, old_table_name = nil)
                 raise "primary key on #{t} was not unique on #{primary_key_columns} (was unique=#{i.unique} on #{i.columns})"
               primary_key_found = true
             elsif i.columns == primary_key_columns && i.unique
-              raise "found primary key index on #{t}.#{primary_key_columns} but it was called #{i.name}"
+              # skip this primary key index since we'll create it below, with PRIMARY_KEY_NAME
+              next
             end
             new(model, i.columns, name: i.name, unique: i.unique, where: i.where, table_name: old_table_name)
           end.compact
@@ -65,6 +59,30 @@ def for_model(model, old_table_name = nil)
           end
           index_definitions
         end
+
+        private
+
+        # This is the old approach which is still needed for SQLite
+        def sqlite_compound_primary_key(model, table)
+          ActiveRecord::Base.connection.class.name.match?(/SQLite3Adapter/) or return nil
+
+          connection = model.connection.dup
+
+          class << connection   # defeat Rails MySQL driver code that skips the primary key by changing its name to a symbol
+            def each_hash(result)
+              super do |hash|
+                if hash[:Key_name] == PRIMARY_KEY_NAME
+                  hash[:Key_name] = PRIMARY_KEY_NAME.to_sym
+                end
+                yield hash
+              end
+            end
+          end
+
+          pk_index = connection.indexes(table).find { |index| index.name.to_s == PRIMARY_KEY_NAME } or return nil
+
+          Array(pk_index.columns)
+        end
       end
 
       def primary_key?

lib/declare_schema/version.rb

@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 module DeclareSchema
-  VERSION = "0.4.0"
+  VERSION = "0.4.1"
 end

spec/lib/declare_schema/api_spec.rb

@@ -39,7 +39,7 @@ class Advert < #{active_record_base_class}
 
       load_models
 
-      unless Rails::VERSION::MAJOR >= 6
+      if Rails::VERSION::MAJOR == 5
         # TODO: get this to work on Travis for Rails 6
         generate_migrations '-n', '-m'
       end

spec/lib/declare_schema/migration_generator_spec.rb

@@ -43,6 +43,12 @@ class Advert < ActiveRecord::Base
     ActiveRecord::Migration.class_eval(up)
     expect(Advert.columns.map(&:name)).to eq(["id", "name"])
 
+    if Rails::VERSION::MAJOR < 5
+      # Rails 4 sqlite driver doesn't create PK properly. Fix that by dropping and recreating.
+      ActiveRecord::Base.connection.execute("drop table adverts")
+      ActiveRecord::Base.connection.execute('CREATE TABLE "adverts" ("id" integer PRIMARY KEY AUTOINCREMENT NOT NULL, "name" varchar(255))')
+    end
+
     class Advert < ActiveRecord::Base
       fields do
         name :string, limit: 255, null: true
@@ -610,9 +616,8 @@ class Advertisement < ActiveRecord::Base
     # Dropping tables is where the automatic down-migration really comes in handy:
 
     rails4_table_create = <<~EOS.strip
-      create_table "adverts", id: false, force: :cascade do |t|
-        t.integer "id",   limit: 8
-        t.string  "name", limit: 255
+      create_table "adverts", force: :cascade do |t|
+        t.string "name", limit: 255
       end
     EOS
 

spec/lib/declare_schema/model/index_definition_spec.rb

@@ -13,16 +13,25 @@
   before do
     load File.expand_path('../prepare_testapp.rb', __dir__)
 
-    class IndexSettingsTestModel < ActiveRecord::Base
+    class IndexDefinitionTestModel < ActiveRecord::Base
       fields do
         name :string, limit: 127, index: true
 
         timestamps
       end
     end
+
+    class IndexDefinitionCompoundIndexModel < ActiveRecord::Base
+      fields do
+        fk1_id :integer
+        fk2_id :integer
+
+        timestamps
+      end
+    end
   end
 
-  let(:model_class) { IndexSettingsTestModel }
+  let(:model_class) { IndexDefinitionTestModel }
 
   describe 'instance methods' do
     let(:model) { model_class.new }
@@ -53,28 +62,59 @@ class IndexSettingsTestModel < ActiveRecord::Base
     context 'with a migrated database' do
       before do
         ActiveRecord::Base.connection.execute <<~EOS
-            CREATE TABLE index_settings_test_models (
+            CREATE TABLE index_definition_test_models (
               id INTEGER NOT NULL PRIMARY KEY,
               name TEXT NOT NULL
             )
         EOS
         ActiveRecord::Base.connection.execute <<~EOS
-          CREATE UNIQUE INDEX index_settings_test_models_on_name ON index_settings_test_models(name)
+          CREATE UNIQUE INDEX index_definition_test_models_on_name ON index_definition_test_models(name)
+        EOS
+        ActiveRecord::Base.connection.execute <<~EOS
+            CREATE TABLE index_definition_compound_index_models (
+              fk1_id INTEGER NULL,
+              fk2_id INTEGER NULL,
+              PRIMARY KEY (fk1_id, fk2_id)
+            )
         EOS
         ActiveRecord::Base.connection.schema_cache.clear!
       end
 
       describe 'for_model' do
         subject { described_class.for_model(model_class) }
 
-        it 'returns the indexes for the model' do
-          expect(subject.size).to eq(2), subject.inspect
-          expect([:name, :columns, :unique].map { |attr| subject[0].send(attr) }).to eq(
-            ['index_settings_test_models_on_name', ['name'], true]
-          )
-          expect([:name, :columns, :unique].map { |attr| subject[1].send(attr) }).to eq(
-            ['PRIMARY', ['id'], true]
-          )
+        context 'with single-column PK' do
+          it 'returns the indexes for the model' do
+            expect(subject.size).to eq(2), subject.inspect
+            expect([:name, :columns, :unique].map { |attr| subject[0].send(attr) }).to eq(
+              ['index_definition_test_models_on_name', ['name'], true]
+            )
+            expect([:name, :columns, :unique].map { |attr| subject[1].send(attr) }).to eq(
+              ['PRIMARY', ['id'], true]
+            )
+          end
+        end
+
+        context 'with compound-column PK' do
+          let(:model_class) { IndexDefinitionCompoundIndexModel }
+
+          it 'returns the indexes for the model' do
+            # Simulate MySQL for Rails 4 work-around
+            if Rails::VERSION::MAJOR < 5
+              expect(model_class.connection).to receive(:primary_key).with('index_definition_compound_index_models').and_return(nil)
+              connection_stub = instance_double(ActiveRecord::ConnectionAdapters::SQLite3Adapter, "connection")
+              expect(connection_stub).to receive(:indexes).
+                with('index_definition_compound_index_models').
+                and_return([DeclareSchema::Model::IndexDefinition.new(model_class, ['fk1_id', 'fk2_id'], name: 'PRIMARY')])
+
+              expect(model_class.connection).to receive(:dup).and_return(connection_stub)
+            end
+
+            expect(subject.size).to eq(1), subject.inspect
+            expect([:name, :columns, :unique].map { |attr| subject[0].send(attr) }).to eq(
+              ['PRIMARY', ['fk1_id', 'fk2_id'], true]
+            )
+          end
         end
       end
     end