Revert "Merge pull request rsim#1669 from yahonda/drop_trigger_based_primary_key_support"

This reverts commit 7fc1f99, reversing
changes made to 4630a39.

Author: yahonda

lib/active_record/connection_adapters/oracle_enhanced/schema_dumper.rb

@@ -21,6 +21,8 @@ def tables(stream)
               # add table prefix or suffix for schema_migrations
               next if ignored? tbl
               table(tbl, stream)
+              # add primary key trigger if table has it
+              primary_key_trigger(tbl, stream)
             end
             # following table definitions
             # add foreign keys if table has them
@@ -33,6 +35,15 @@ def tables(stream)
             synonyms(stream)
           end
 
+          def primary_key_trigger(table_name, stream)
+            if @connection.has_primary_key_trigger?(table_name)
+              pk, _pk_seq = @connection.pk_and_sequence_for(table_name)
+              stream.print "  add_primary_key_trigger #{table_name.inspect}"
+              stream.print ", primary_key: \"#{pk}\"" if pk != "id"
+              stream.print "\n\n"
+            end
+          end
+
           def synonyms(stream)
             syns = @connection.synonyms
             syns.each do |syn|

lib/active_record/connection_adapters/oracle_enhanced/schema_statements.rb

@@ -700,11 +700,33 @@ def column_for(table_name, column_name)
           end
 
           def create_sequence_and_trigger(table_name, options)
-            # TODO: Needs rename since no triggers created
-            # This method will be removed since sequence will not be created separately
             seq_name = options[:sequence_name] || default_sequence_name(table_name)
             seq_start_value = options[:sequence_start_value] || default_sequence_start_value
             execute "CREATE SEQUENCE #{quote_table_name(seq_name)} START WITH #{seq_start_value}"
+
+            create_primary_key_trigger(table_name, options) if options[:primary_key_trigger]
+          end
+
+          def create_primary_key_trigger(table_name, options)
+            seq_name = options[:sequence_name] || default_sequence_name(table_name)
+            trigger_name = options[:trigger_name] || default_trigger_name(table_name)
+            primary_key = options[:primary_key] || Base.get_primary_key(table_name.to_s.singularize)
+            execute <<-SQL
+            CREATE OR REPLACE TRIGGER #{quote_table_name(trigger_name)}
+            BEFORE INSERT ON #{quote_table_name(table_name)} FOR EACH ROW
+            BEGIN
+              IF inserting THEN
+                IF :new.#{quote_column_name(primary_key)} IS NULL THEN
+                  SELECT #{quote_table_name(seq_name)}.NEXTVAL INTO :new.#{quote_column_name(primary_key)} FROM dual;
+                END IF;
+              END IF;
+            END;
+          SQL
+          end
+
+          def default_trigger_name(table_name)
+            # truncate table name if necessary to fit in max length of identifier
+            "#{table_name.to_s[0, table_name_length - 4]}_pkt"
           end
 
           def rebuild_primary_key_index_to_default_tablespace(table_name, options)

lib/active_record/connection_adapters/oracle_enhanced/schema_statements_ext.rb

@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+module ActiveRecord
+  module ConnectionAdapters
+    module OracleEnhanced
+      module SchemaStatementsExt
+        # Create primary key trigger (so that you can skip primary key value in INSERT statement).
+        # By default trigger name will be "table_name_pkt", you can override the name with
+        # :trigger_name option (but it is not recommended to override it as then this trigger will
+        # not be detected by ActiveRecord model and it will still do prefetching of sequence value).
+        #
+        #   add_primary_key_trigger :users
+        #
+        # You can also create primary key trigger using +create_table+ with :primary_key_trigger
+        # option:
+        #
+        #   create_table :users, :primary_key_trigger => true do |t|
+        #     # ...
+        #   end
+        #
+        def add_primary_key_trigger(table_name, options = {})
+          # call the same private method that is used for create_table :primary_key_trigger => true
+          create_primary_key_trigger(table_name, options)
+        end
+      end
+    end
+  end
+end

lib/active_record/connection_adapters/oracle_enhanced_adapter.rb

@@ -41,6 +41,7 @@
 require "active_record/connection_adapters/oracle_enhanced/schema_definitions"
 require "active_record/connection_adapters/oracle_enhanced/schema_dumper"
 require "active_record/connection_adapters/oracle_enhanced/schema_statements"
+require "active_record/connection_adapters/oracle_enhanced/schema_statements_ext"
 require "active_record/connection_adapters/oracle_enhanced/context_index"
 require "active_record/connection_adapters/oracle_enhanced/column"
 require "active_record/connection_adapters/oracle_enhanced/quoting"
@@ -149,6 +150,7 @@ module ConnectionAdapters #:nodoc:
     class OracleEnhancedAdapter < AbstractAdapter
       include OracleEnhanced::DatabaseStatements
       include OracleEnhanced::SchemaStatements
+      include OracleEnhanced::SchemaStatementsExt
       include OracleEnhanced::ContextIndex
       include OracleEnhanced::Quoting
       include OracleEnhanced::DatabaseLimits
@@ -477,7 +479,7 @@ def discard!
       # when inserting a new database record (see #prefetch_primary_key?).
       def next_sequence_value(sequence_name)
         # if sequence_name is set to :autogenerated then it means that primary key will be populated by trigger
-        raise ArgumentError.new "Trigger based primary key is not supported" if sequence_name == AUTOGENERATED_SEQUENCE_NAME
+        return nil if sequence_name == AUTOGENERATED_SEQUENCE_NAME
         # call directly connection method to avoid prepared statement which causes fetching of next sequence value twice
         select_value(<<~SQL.squish, "SCHEMA")
           SELECT #{quote_table_name(sequence_name)}.NEXTVAL FROM dual
@@ -489,11 +491,8 @@ def next_sequence_value(sequence_name)
       def prefetch_primary_key?(table_name = nil)
         return true if table_name.nil?
         table_name = table_name.to_s
-        do_not_prefetch = @do_not_prefetch_primary_key[table_name]
-        if do_not_prefetch.nil?
-          owner, desc_table_name = @connection.describe(table_name)
-          @do_not_prefetch_primary_key[table_name] = do_not_prefetch = !has_primary_key?(table_name, owner, desc_table_name)
-        end
+        owner, desc_table_name = @connection.describe(table_name)
+        do_not_prefetch = !has_primary_key?(table_name, owner, desc_table_name) || has_primary_key_trigger?(table_name, owner, desc_table_name)
         !do_not_prefetch
       end
 
@@ -559,6 +558,23 @@ def default_tablespace
         SQL
       end
 
+      # check if table has primary key trigger with _pkt suffix
+      def has_primary_key_trigger?(table_name, owner = nil, desc_table_name = nil)
+        (owner, desc_table_name) = @connection.describe(table_name) unless owner
+
+        trigger_name = default_trigger_name(table_name).upcase
+
+        !!select_value(<<-SQL.strip.gsub(/\s+/, " "), "Primary Key Trigger", [bind_string("owner", owner), bind_string("trigger_name", trigger_name), bind_string("owner", owner), bind_string("table_name", desc_table_name)])
+          SELECT trigger_name
+          FROM all_triggers
+          WHERE owner = :owner
+            AND trigger_name = :trigger_name
+            AND table_owner = :owner
+            AND table_name = :table_name
+            AND status = 'ENABLED'
+        SQL
+      end
+
       def column_definitions(table_name)
         (owner, desc_table_name) = @connection.describe(table_name)
 

spec/active_record/connection_adapters/oracle_enhanced/schema_dumper_spec.rb

@@ -108,6 +108,24 @@ def drop_test_posts_table
     end
   end
 
+  describe "table with primary key trigger" do
+
+    after(:each) do
+      drop_test_posts_table
+    end
+
+    it "should include primary key trigger in schema dump" do
+      create_test_posts_table(primary_key_trigger: true)
+      expect(standard_dump).to match(/create_table "test_posts".*add_primary_key_trigger "test_posts"/m)
+    end
+
+    it "should include primary key trigger with non-default primary key in schema dump" do
+      create_test_posts_table(primary_key_trigger: true, primary_key: "post_id")
+      expect(standard_dump).to match(/create_table "test_posts", primary_key: "post_id".*add_primary_key_trigger "test_posts", primary_key: "post_id"/m)
+    end
+
+  end
+
   describe "foreign key constraints" do
     before(:all) do
       schema_define do

spec/active_record/connection_adapters/oracle_enhanced/schema_statements_spec.rb

@@ -152,6 +152,172 @@ class ::TestEmployee < ActiveRecord::Base; end
     end
   end
 
+  describe "create table with primary key trigger" do
+    def create_table_with_trigger(options = {})
+      options.merge! primary_key_trigger: true, force: true
+      schema_define do
+        create_table :test_employees, options do |t|
+          t.string      :first_name
+          t.string      :last_name
+        end
+      end
+    end
+
+    def create_table_and_separately_trigger(options = {})
+      options.merge! force: true
+      schema_define do
+        create_table :test_employees, options do |t|
+          t.string      :first_name
+          t.string      :last_name
+        end
+        add_primary_key_trigger :test_employees, options
+      end
+    end
+
+    def drop_table_with_trigger(options = {})
+      seq_name = options[:sequence_name]
+      schema_define do
+        drop_table :test_employees, (seq_name ? { sequence_name: seq_name } : {})
+      end
+      Object.send(:remove_const, "TestEmployee")
+      ActiveRecord::Base.clear_cache!
+    end
+
+    describe "with default primary key" do
+      before(:all) do
+        @conn = ActiveRecord::Base.connection
+        create_table_with_trigger
+        class ::TestEmployee < ActiveRecord::Base
+        end
+      end
+
+      after(:all) do
+        drop_table_with_trigger
+      end
+
+      it "should populate primary key using trigger" do
+        expect do
+          @conn.execute "INSERT INTO test_employees (first_name) VALUES ('Raimonds')"
+        end.not_to raise_error
+      end
+
+      it "should return new key value using connection insert method" do
+        insert_id = @conn.insert("INSERT INTO test_employees (first_name) VALUES ('Raimonds')", nil, "id")
+        expect(@conn.select_value("SELECT test_employees_seq.currval FROM dual")).to eq(insert_id)
+      end
+
+      it "should create new record for model" do
+        e = TestEmployee.create!(first_name: "Raimonds")
+        expect(@conn.select_value("SELECT test_employees_seq.currval FROM dual")).to eq(e.id)
+      end
+
+      it "should not generate NoMethodError for :returning_id:Symbol" do
+        set_logger
+        @conn.reconnect! unless @conn.active?
+        @conn.insert("INSERT INTO test_employees (first_name) VALUES ('Yasuo')", nil, "id")
+        expect(@logger.output(:error)).not_to match(/^Could not log "sql.active_record" event. NoMethodError: undefined method `name' for :returning_id:Symbol/)
+        clear_logger
+      end
+
+    end
+
+    describe "with separate creation of primary key trigger" do
+      before(:all) do
+        ActiveRecord::Base.establish_connection(CONNECTION_PARAMS)
+        @conn = ActiveRecord::Base.connection
+        create_table_and_separately_trigger
+        class ::TestEmployee < ActiveRecord::Base
+        end
+      end
+
+      after(:all) do
+        drop_table_with_trigger
+      end
+
+      it "should populate primary key using trigger" do
+        expect do
+          @conn.execute "INSERT INTO test_employees (first_name) VALUES ('Raimonds')"
+        end.not_to raise_error
+      end
+
+      it "should return new key value using connection insert method" do
+        insert_id = @conn.insert("INSERT INTO test_employees (first_name) VALUES ('Raimonds')", nil, "id")
+        expect(@conn.select_value("SELECT test_employees_seq.currval FROM dual")).to eq(insert_id)
+      end
+
+      it "should create new record for model" do
+        e = TestEmployee.create!(first_name: "Raimonds")
+        expect(@conn.select_value("SELECT test_employees_seq.currval FROM dual")).to eq(e.id)
+      end
+    end
+
+    describe "with non-default primary key and non-default sequence name" do
+      before(:all) do
+        ActiveRecord::Base.establish_connection(CONNECTION_PARAMS)
+        @conn = ActiveRecord::Base.connection
+        @primary_key = "employee_id"
+        @sequence_name = "test_employees_s"
+        create_table_with_trigger(primary_key: @primary_key, sequence_name: @sequence_name)
+        class ::TestEmployee < ActiveRecord::Base
+          self.primary_key = "employee_id"
+        end
+      end
+
+      after(:all) do
+        drop_table_with_trigger(sequence_name: @sequence_name)
+      end
+
+      it "should populate primary key using trigger" do
+        expect do
+          @conn.execute "INSERT INTO test_employees (first_name) VALUES ('Raimonds')"
+        end.not_to raise_error
+      end
+
+      it "should return new key value using connection insert method" do
+        insert_id = @conn.insert("INSERT INTO test_employees (first_name) VALUES ('Raimonds')", nil, @primary_key)
+        expect(@conn.select_value("SELECT #{@sequence_name}.currval FROM dual")).to eq(insert_id)
+      end
+
+      it "should create new record for model with autogenerated sequence option" do
+        e = TestEmployee.create!(first_name: "Raimonds")
+        expect(@conn.select_value("SELECT #{@sequence_name}.currval FROM dual")).to eq(e.id)
+      end
+    end
+
+    describe "with non-default sequence name and non-default trigger name" do
+      before(:all) do
+        ActiveRecord::Base.establish_connection(CONNECTION_PARAMS)
+        @conn = ActiveRecord::Base.connection
+        @sequence_name = "test_employees_s"
+        create_table_with_trigger(sequence_name: @sequence_name, trigger_name: "test_employees_t1")
+        class ::TestEmployee < ActiveRecord::Base
+          self.sequence_name = :autogenerated
+        end
+      end
+
+      after(:all) do
+        drop_table_with_trigger(sequence_name: @sequence_name)
+      end
+
+      it "should populate primary key using trigger" do
+        expect do
+          @conn.execute "INSERT INTO test_employees (first_name) VALUES ('Raimonds')"
+        end.not_to raise_error
+      end
+
+      it "should return new key value using connection insert method" do
+        insert_id = @conn.insert("INSERT INTO test_employees (first_name) VALUES ('Raimonds')", nil, "id")
+        expect(@conn.select_value("SELECT #{@sequence_name}.currval FROM dual")).to eq(insert_id)
+      end
+
+      it "should create new record for model with autogenerated sequence option" do
+        e = TestEmployee.create!(first_name: "Raimonds")
+        expect(@conn.select_value("SELECT #{@sequence_name}.currval FROM dual")).to eq(e.id)
+      end
+    end
+
+  end
+
   describe "table and column comments" do
     def create_test_employees_table(table_comment = nil, column_comments = {})
       schema_define do
@@ -302,6 +468,44 @@ class ::TestEmployee < ActiveRecord::Base; end
     end
   end
 
+  describe "create triggers" do
+
+    before(:all) do
+      @conn = ActiveRecord::Base.connection
+      schema_define do
+        create_table  :test_employees do |t|
+          t.string    :first_name
+          t.string    :last_name
+        end
+      end
+      class ::TestEmployee < ActiveRecord::Base; end
+    end
+
+    after(:all) do
+      schema_define do
+        drop_table :test_employees
+      end
+      Object.send(:remove_const, "TestEmployee")
+      ActiveRecord::Base.clear_cache!
+    end
+
+    it "should create table trigger with :new reference" do
+      expect do
+        @conn.execute <<-SQL
+        CREATE OR REPLACE TRIGGER test_employees_pkt
+        BEFORE INSERT ON test_employees FOR EACH ROW
+        BEGIN
+          IF inserting THEN
+            IF :new.id IS NULL THEN
+              SELECT test_employees_seq.NEXTVAL INTO :new.id FROM dual;
+            END IF;
+          END IF;
+        END;
+        SQL
+      end.not_to raise_error
+    end
+  end
+
   describe "add index" do
     before(:all) do
       @conn = ActiveRecord::Base.connection