Usage of bind variables for volatile filter conditions (3) (#2125)

* Usage of bind variables for volatile filter conditions

* Fix rubocop issues

Author: rammpeter

lib/active_record/connection_adapters/oracle_enhanced/connection.rb

@@ -36,25 +36,25 @@ def describe(name)
             sql = <<~SQL.squish
               SELECT owner, table_name, 'TABLE' name_type
               FROM all_tables
-              WHERE owner = '#{table_owner}'
-                AND table_name = '#{table_name}'
+              WHERE owner = :table_owner
+                AND table_name = :table_name
               UNION ALL
               SELECT owner, view_name table_name, 'VIEW' name_type
               FROM all_views
-              WHERE owner = '#{table_owner}'
-                AND view_name = '#{table_name}'
+              WHERE owner = :table_owner
+                AND view_name = :table_name
               UNION ALL
               SELECT table_owner, table_name, 'SYNONYM' name_type
               FROM all_synonyms
-              WHERE owner = '#{table_owner}'
-                AND synonym_name = '#{table_name}'
+              WHERE owner = :table_owner
+                AND synonym_name = :table_name
               UNION ALL
               SELECT table_owner, table_name, 'SYNONYM' name_type
               FROM all_synonyms
               WHERE owner = 'PUBLIC'
-                AND synonym_name = '#{real_name}'
+                AND synonym_name = :real_name
             SQL
-            if result = _select_one(sql)
+            if result = _select_one(sql, "CONNECTION", [table_owner, table_name, table_owner, table_name, table_owner, table_name, real_name])
               case result["name_type"]
               when "SYNONYM"
                 describe("#{result['owner'] && "#{result['owner']}."}#{result['table_name']}")
@@ -92,14 +92,23 @@ def _oracle_downcase(column_name)
 
           # Returns a record hash with the column names as keys and column values
           # as values.
+          # binds is a array of native values in contrast to ActiveRecord::Relation::QueryAttribute
           def _select_one(arel, name = nil, binds = [])
-            result = select(arel)
-            result.first if result
+            cursor = prepare(arel)
+            cursor.bind_params(binds)
+            cursor.exec
+            columns = cursor.get_col_names.map do |col_name|
+              _oracle_downcase(col_name)
+            end
+            row = cursor.fetch
+            columns.each_with_index.map { |x, i| [x, row[i]] }.to_h if row
+          ensure
+            cursor.close
           end
 
           # Returns a single value from a record
           def _select_value(arel, name = nil, binds = [])
-            if result = _select_one(arel)
+            if result = _select_one(arel, name, binds)
               result.values.first
             end
           end

lib/active_record/connection_adapters/oracle_enhanced/schema_statements.rb

@@ -367,12 +367,12 @@ def index_name(table_name, options) #:nodoc:
         # Will always query database and not index cache.
         def index_name_exists?(table_name, index_name)
           (_owner, table_name) = @connection.describe(table_name)
-          result = select_value(<<~SQL.squish, "SCHEMA")
+          result = select_value(<<~SQL.squish, "SCHEMA", [bind_string("table_name", table_name), bind_string("index_name", index_name.to_s.upcase)])
             SELECT /*+ OPTIMIZER_FEATURES_ENABLE('11.2.0.2') */ 1 FROM all_indexes i
             WHERE i.owner = SYS_CONTEXT('userenv', 'current_schema')
                AND i.table_owner = SYS_CONTEXT('userenv', 'current_schema')
-               AND i.table_name = '#{table_name}'
-               AND i.index_name = '#{index_name.to_s.upcase}'
+               AND i.table_name = :table_name
+               AND i.index_name = :index_name
           SQL
           result == 1
         end

lib/active_record/connection_adapters/oracle_enhanced/structure_dump.rb

@@ -30,11 +30,11 @@ def structure_dump #:nodoc:
           tables.each do |table_name|
             virtual_columns = virtual_columns_for(table_name) if supports_virtual_columns?
             ddl = +"CREATE#{ ' GLOBAL TEMPORARY' if temporary_table?(table_name)} TABLE \"#{table_name}\" (\n"
-            columns = select_all(<<~SQL.squish, "SCHEMA")
+            columns = select_all(<<~SQL.squish, "SCHEMA", [bind_string("table_name", table_name)])
               SELECT /*+ OPTIMIZER_FEATURES_ENABLE('11.2.0.2') */ column_name, data_type, data_length, char_used, char_length,
               data_precision, data_scale, data_default, nullable
               FROM all_tab_columns
-              WHERE table_name = '#{table_name}'
+              WHERE table_name = :table_name
               AND owner = SYS_CONTEXT('userenv', 'current_schema')
               ORDER BY column_id
             SQL
@@ -174,10 +174,10 @@ def structure_dump_table_comments(table_name)
 
         def structure_dump_column_comments(table_name)
           comments = []
-          columns = select_values(<<~SQL.squish, "SCHEMA")
+          columns = select_values(<<~SQL.squish, "SCHEMA", [bind_string("table_name", table_name)])
             SELECT /*+ OPTIMIZER_FEATURES_ENABLE('11.2.0.2') */ column_name FROM all_tab_columns
             WHERE owner = SYS_CONTEXT('userenv', 'current_schema')
-            AND table_name = '#{table_name}' ORDER BY column_id
+            AND table_name = :table_name ORDER BY column_id
           SQL
 
           columns.each do |column|
@@ -218,11 +218,11 @@ def structure_dump_db_stored_code #:nodoc:
           SQL
           all_source.each do |source|
             ddl = +"CREATE OR REPLACE   \n"
-            texts = select_all(<<~SQL.squish, "all source at structure dump")
+            texts = select_all(<<~SQL.squish, "all source at structure dump", [bind_string("source_name", source["name"]), bind_string("source_type", source["type"])])
               SELECT /*+ OPTIMIZER_FEATURES_ENABLE('11.2.0.2') */ text
               FROM all_source
-              WHERE name = '#{source['name']}'
-              AND type = '#{source['type']}'
+              WHERE name = :source_name
+              AND type = :source_type
               AND owner = SYS_CONTEXT('userenv', 'current_schema')
               ORDER BY line
             SQL
@@ -323,12 +323,12 @@ def execute_structure_dump(string)
         # Called only if `supports_virtual_columns?` returns true
         # return [{'column_name' => 'FOOS', 'data_default' => '...'}, ...]
         def virtual_columns_for(table)
-          select_all(<<~SQL.squish, "SCHEMA")
+          select_all(<<~SQL.squish, "SCHEMA", [bind_string("table_name", table.upcase)])
             SELECT /*+ OPTIMIZER_FEATURES_ENABLE('11.2.0.2') */ column_name, data_default
             FROM all_tab_cols
             WHERE virtual_column = 'YES'
             AND owner = SYS_CONTEXT('userenv', 'current_schema')
-            AND table_name = '#{table.upcase}'
+            AND table_name = :table_name
           SQL
         end
 

lib/active_record/connection_adapters/oracle_enhanced_adapter.rb

@@ -562,7 +562,7 @@ def default_tablespace
       def column_definitions(table_name)
         (owner, desc_table_name) = @connection.describe(table_name)
 
-        select_all(<<~SQL.squish, "SCHEMA")
+        select_all(<<~SQL.squish, "SCHEMA", [bind_string("owner", owner), bind_string("table_name", desc_table_name)])
           SELECT /*+ OPTIMIZER_FEATURES_ENABLE('11.2.0.2') */ cols.column_name AS name, cols.data_type AS sql_type,
                  cols.data_default, cols.nullable, cols.virtual_column, cols.hidden_column,
                  cols.data_type_owner AS sql_type_owner,
@@ -575,8 +575,8 @@ def column_definitions(table_name)
                  DECODE(data_type, 'NUMBER', data_scale, NULL) AS scale,
                  comments.comments as column_comment
             FROM all_tab_cols cols, all_col_comments comments
-           WHERE cols.owner      = '#{owner}'
-             AND cols.table_name = #{quote(desc_table_name)}
+           WHERE cols.owner      = :owner
+             AND cols.table_name = :table_name
              AND cols.hidden_column = 'NO'
              AND cols.owner = comments.owner
              AND cols.table_name = comments.table_name
@@ -594,19 +594,19 @@ def clear_table_columns_cache(table_name)
       def pk_and_sequence_for(table_name, owner = nil, desc_table_name = nil) #:nodoc:
         (owner, desc_table_name) = @connection.describe(table_name)
 
-        seqs = select_values(<<~SQL.squish, "SCHEMA")
+        seqs = select_values_forcing_binds(<<~SQL.squish, "SCHEMA", [bind_string("owner", owner), bind_string("sequence_name", default_sequence_name(desc_table_name))])
           select /*+ OPTIMIZER_FEATURES_ENABLE('11.2.0.2') */ us.sequence_name
           from all_sequences us
-          where us.sequence_owner = '#{owner}'
-          and us.sequence_name = upper(#{quote(default_sequence_name(desc_table_name))})
+          where us.sequence_owner = :owner
+          and us.sequence_name = upper(:sequence_name)
         SQL
 
         # changed back from user_constraints to all_constraints for consistency
-        pks = select_values(<<~SQL.squish, "SCHEMA")
+        pks = select_values_forcing_binds(<<~SQL.squish, "SCHEMA", [bind_string("owner", owner), bind_string("table_name", desc_table_name)])
           SELECT /*+ OPTIMIZER_FEATURES_ENABLE('11.2.0.2') */ cc.column_name
             FROM all_constraints c, all_cons_columns cc
-           WHERE c.owner = '#{owner}'
-             AND c.table_name = #{quote(desc_table_name)}
+           WHERE c.owner = :owner
+             AND c.table_name = :table_name
              AND c.constraint_type = 'P'
              AND cc.owner = c.owner
              AND cc.constraint_name = c.constraint_name
@@ -636,7 +636,7 @@ def has_primary_key?(table_name, owner = nil, desc_table_name = nil) #:nodoc:
       def primary_keys(table_name) # :nodoc:
         (_owner, desc_table_name) = @connection.describe(table_name)
 
-        pks = select_values(<<~SQL.squish, "SCHEMA", [bind_string("table_name", desc_table_name)])
+        pks = select_values_forcing_binds(<<~SQL.squish, "SCHEMA", [bind_string("table_name", desc_table_name)])
           SELECT /*+ OPTIMIZER_FEATURES_ENABLE('11.2.0.2') */ cc.column_name
             FROM all_constraints c, all_cons_columns cc
            WHERE c.owner = SYS_CONTEXT('userenv', 'current_schema')
@@ -666,7 +666,7 @@ def columns_for_distinct(columns, orders) #:nodoc:
       end
 
       def temporary_table?(table_name) #:nodoc:
-        select_value(<<~SQL.squish, "SCHEMA", [bind_string("table_name", table_name.upcase)]) == "Y"
+        select_value_forcing_binds(<<~SQL.squish, "SCHEMA", [bind_string("table_name", table_name.upcase)]) == "Y"
           SELECT /*+ OPTIMIZER_FEATURES_ENABLE('11.2.0.2') */
           temporary FROM all_tables WHERE table_name = :table_name and owner = SYS_CONTEXT('userenv', 'current_schema')
         SQL
@@ -764,6 +764,22 @@ def bind_string(name, value)
           ActiveRecord::Relation::QueryAttribute.new(name, value, Type::OracleEnhanced::String.new)
         end
 
+        # call select_values using binds even if surrounding SQL preparation/execution is done + # with conn.unprepared_statement (like AR.to_sql)
+        def select_values_forcing_binds(arel, name, binds)
+          # remove possible force of unprepared SQL during dictionary access
+          unprepared_statement_forced = prepared_statements_disabled_cache.include?(object_id)
+          prepared_statements_disabled_cache.delete(object_id) if unprepared_statement_forced
+
+          select_values(arel, name, binds)
+        ensure
+          # Restore unprepared_statement setting for surrounding SQL
+          prepared_statements_disabled_cache.add(object_id) if unprepared_statement_forced
+        end
+
+        def select_value_forcing_binds(arel, name, binds)
+          single_value_from_rows(select_values_forcing_binds(arel, name, binds))
+        end
+
         ActiveRecord::Type.register(:boolean, Type::OracleEnhanced::Boolean, adapter: :oracleenhanced)
         ActiveRecord::Type.register(:json, Type::OracleEnhanced::Json, adapter: :oracleenhanced)
     end

spec/active_record/connection_adapters/oracle_enhanced_adapter_spec.rb

@@ -577,9 +577,29 @@ class TestLog < ActiveRecord::Base
       schema_define do
         drop_table :test_posts, if_exists: true
         create_table :test_posts
+
+        drop_table :users, if_exists: true
+        create_table :users, force: true do |t|
+          t.string :name
+          t.integer :group_id
+        end
+
+        drop_table :groups, if_exists: true
+        create_table :groups, force: true do |t|
+          t.string :name
+        end
       end
+
       class ::TestPost < ActiveRecord::Base
       end
+
+      class User < ActiveRecord::Base
+        belongs_to :group
+      end
+
+      class Group < ActiveRecord::Base
+        has_one :user
+      end
     end
 
     before(:each) do
@@ -594,6 +614,8 @@ class ::TestPost < ActiveRecord::Base
     after(:all) do
       schema_define do
         drop_table :test_posts
+        drop_table :users
+        drop_table :groups
       end
       Object.send(:remove_const, "TestPost")
       ActiveRecord::Base.clear_cache!
@@ -610,22 +632,27 @@ class ::TestPost < ActiveRecord::Base
        expect(@logger.logged(:debug).last).to match(/\["table_name", "TEST_POSTS"\]/)
      end
 
-    it "should return content from columns without bind usage" do
+    it "should return content from columns witt bind usage" do
       expect(@conn.columns("TEST_POSTS").length).to be > 0
-      expect(@logger.logged(:debug).last).not_to match(/:table_name/)
-      expect(@logger.logged(:debug).last).not_to match(/\["table_name", "TEST_POSTS"\]/)
+      expect(@logger.logged(:debug).last).to match(/:table_name/)
+      expect(@logger.logged(:debug).last).to match(/\["table_name", "TEST_POSTS"\]/)
     end
 
-    it "should return pk and sequence from pk_and_sequence_for without bind usage" do
+    it "should return pk and sequence from pk_and_sequence_for with bind usage" do
       expect(@conn.pk_and_sequence_for("TEST_POSTS").length).to eq 2
-      expect(@logger.logged(:debug).last).not_to match(/\["table_name", "TEST_POSTS"\]/)
+      expect(@logger.logged(:debug).last).to match(/\["table_name", "TEST_POSTS"\]/)
     end
 
     it "should return pk from primary_keys with bind usage" do
       expect(@conn.primary_keys("TEST_POSTS")).to eq ["id"]
       expect(@logger.logged(:debug).last).to match(/\["table_name", "TEST_POSTS"\]/)
     end
 
+    it "should not raise missing IN/OUT parameter like issue 1687 " do
+      # "to_sql" enforces unprepared_statement including dictionary access SQLs
+      expect { User.joins(:group).to_sql }.not_to raise_exception
+    end
+
     it "should return false from temporary_table? with bind usage" do
       expect(@conn.temporary_table?("TEST_POSTS")).to eq false
       expect(@logger.logged(:debug).last).to match(/:table_name/)