Merge pull request #34468 from gmcgibbon/redact_sql_in_errors

Redact SQL in errors

activerecord/CHANGELOG.md

@@ -1,3 +1,23 @@
+*   Move `ActiveRecord::StatementInvalid` SQL to error property and include binds as separate error property.
+
+    `ActiveRecord::ConnectionAdapters::AbstractAdapter#translate_exception_class` now requires `binds` to be passed as the last argument.
+
+    `ActiveRecord::ConnectionAdapters::AbstractAdapter#translate_exception` now requires `message`, `sql`, and `binds` to be passed as keyword arguments.
+
+    Subclasses of `ActiveRecord::StatementInvalid` must now provide `sql:` and `binds:` arguments to `super`.
+
+    Example:
+
+    ```
+    class MySubclassedError < ActiveRecord::StatementInvalid
+      def initialize(message, sql:, binds:)
+        super(message, sql: sql, binds: binds)
+      end
+    end
+    ```
+
+    *Gannon McGibbon*
+
 *   Add an `:if_not_exists` option to `create_table`.
 
     Example:

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

@@ -580,14 +580,12 @@ module ActiveRecord
           $1.to_i if sql_type =~ /\((.*)\)/
         end
 
-        def translate_exception_class(e, sql)
+        def translate_exception_class(e, sql, binds)
-          begin
+          message = "#{e.class.name}: #{e.message}"
-            message = "#{e.class.name}: #{e.message}: #{sql}"
-          rescue Encoding::CompatibilityError
-            message = "#{e.class.name}: #{e.message.force_encoding sql.encoding}: #{sql}"
-          end
 
-          exception = translate_exception(e, message)
+          exception = translate_exception(
+            e, message: message, sql: sql, binds: binds
+          )
           exception.set_backtrace e.backtrace
           exception
         end
@@ -606,18 +604,18 @@ module ActiveRecord
                 yield
               end
             rescue => e
-              raise translate_exception_class(e, sql)
+              raise translate_exception_class(e, sql, binds)
             end
           end
         end
 
-        def translate_exception(exception, message)
+        def translate_exception(exception, message:, sql:, binds:)
           # override in derived class
           case exception
           when RuntimeError
             exception
           else
-            ActiveRecord::StatementInvalid.new(message)
+            ActiveRecord::StatementInvalid.new(message, sql: sql, binds: binds)
           end
         end
 

activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb

@@ -642,34 +642,34 @@ module ActiveRecord
         ER_QUERY_INTERRUPTED    = 1317
         ER_QUERY_TIMEOUT        = 3024
 
-        def translate_exception(exception, message)
+        def translate_exception(exception, message:, sql:, binds:)
           case error_number(exception)
           when ER_DUP_ENTRY
-            RecordNotUnique.new(message)
+            RecordNotUnique.new(message, sql: sql, binds: binds)
           when ER_NO_REFERENCED_ROW, ER_ROW_IS_REFERENCED, ER_ROW_IS_REFERENCED_2, ER_NO_REFERENCED_ROW_2
-            InvalidForeignKey.new(message)
+            InvalidForeignKey.new(message, sql: sql, binds: binds)
           when ER_CANNOT_ADD_FOREIGN
-            mismatched_foreign_key(message)
+            mismatched_foreign_key(message, sql: sql, binds: binds)
           when ER_CANNOT_CREATE_TABLE
             if message.include?("errno: 150")
-              mismatched_foreign_key(message)
+              mismatched_foreign_key(message, sql: sql, binds: binds)
             else
               super
             end
           when ER_DATA_TOO_LONG
-            ValueTooLong.new(message)
+            ValueTooLong.new(message, sql: sql, binds: binds)
           when ER_OUT_OF_RANGE
-            RangeError.new(message)
+            RangeError.new(message, sql: sql, binds: binds)
           when ER_NOT_NULL_VIOLATION, ER_DO_NOT_HAVE_DEFAULT
-            NotNullViolation.new(message)
+            NotNullViolation.new(message, sql: sql, binds: binds)
           when ER_LOCK_DEADLOCK
-            Deadlocked.new(message)
+            Deadlocked.new(message, sql: sql, binds: binds)
           when ER_LOCK_WAIT_TIMEOUT
-            LockWaitTimeout.new(message)
+            LockWaitTimeout.new(message, sql: sql, binds: binds)
           when ER_QUERY_TIMEOUT
-            StatementTimeout.new(message)
+            StatementTimeout.new(message, sql: sql, binds: binds)
           when ER_QUERY_INTERRUPTED
-            QueryCanceled.new(message)
+            QueryCanceled.new(message, sql: sql, binds: binds)
           else
             super
           end
@@ -800,11 +800,13 @@ module ActiveRecord
           Arel::Visitors::MySQL.new(self)
         end
 
-        def mismatched_foreign_key(message)
+        def mismatched_foreign_key(message, sql:, binds:)
-          parts = message.scan(/`(\w+)`[ $)]/).flatten
+          parts = sql.scan(/`(\w+)`[ $)]/).flatten
           MismatchedForeignKey.new(
             self,
             message: message,
+            sql: sql,
+            binds: binds,
             table: parts[0],
             foreign_key: parts[1],
             target_table: parts[2],

activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb

@@ -441,28 +441,28 @@ module ActiveRecord
         LOCK_NOT_AVAILABLE    = "55P03"
         QUERY_CANCELED        = "57014"
 
-        def translate_exception(exception, message)
+        def translate_exception(exception, message:, sql:, binds:)
           return exception unless exception.respond_to?(:result)
 
           case exception.result.try(:error_field, PG::PG_DIAG_SQLSTATE)
           when UNIQUE_VIOLATION
-            RecordNotUnique.new(message)
+            RecordNotUnique.new(message, sql: sql, binds: binds)
           when FOREIGN_KEY_VIOLATION
-            InvalidForeignKey.new(message)
+            InvalidForeignKey.new(message, sql: sql, binds: binds)
           when VALUE_LIMIT_VIOLATION
-            ValueTooLong.new(message)
+            ValueTooLong.new(message, sql: sql, binds: binds)
           when NUMERIC_VALUE_OUT_OF_RANGE
-            RangeError.new(message)
+            RangeError.new(message, sql: sql, binds: binds)
           when NOT_NULL_VIOLATION
-            NotNullViolation.new(message)
+            NotNullViolation.new(message, sql: sql, binds: binds)
           when SERIALIZATION_FAILURE
-            SerializationFailure.new(message)
+            SerializationFailure.new(message, sql: sql, binds: binds)
           when DEADLOCK_DETECTED
-            Deadlocked.new(message)
+            Deadlocked.new(message, sql: sql, binds: binds)
           when LOCK_NOT_AVAILABLE
-            LockWaitTimeout.new(message)
+            LockWaitTimeout.new(message, sql: sql, binds: binds)
           when QUERY_CANCELED
-            QueryCanceled.new(message)
+            QueryCanceled.new(message, sql: sql, binds: binds)
           else
             super
           end
@@ -642,7 +642,7 @@ module ActiveRecord
         def exec_cache(sql, name, binds)
           materialize_transactions
 
-          stmt_key = prepare_statement(sql)
+          stmt_key = prepare_statement(sql, binds)
           type_casted_binds = type_casted_binds(binds)
 
           log(sql, name, binds, type_casted_binds, stmt_key) do
@@ -696,7 +696,7 @@ module ActiveRecord
 
         # Prepare the statement if it hasn't been prepared, return
         # the statement key.
-        def prepare_statement(sql)
+        def prepare_statement(sql, binds)
           @lock.synchronize do
             sql_key = sql_key(sql)
             unless @statements.key? sql_key
@@ -704,7 +704,7 @@ module ActiveRecord
               begin
                 @connection.prepare nextkey, sql
               rescue => e
-                raise translate_exception_class(e, sql)
+                raise translate_exception_class(e, sql, binds)
               end
               # Clear the queue
               @connection.get_last_result

activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb

@@ -529,18 +529,18 @@ module ActiveRecord
           @sqlite_version ||= SQLite3Adapter::Version.new(query_value("SELECT sqlite_version(*)"))
         end
 
-        def translate_exception(exception, message)
+        def translate_exception(exception, message:, sql:, binds:)
           case exception.message
           # SQLite 3.8.2 returns a newly formatted error message:
           #   UNIQUE constraint failed: *table_name*.*column_name*
           # Older versions of SQLite return:
           #   column *column_name* is not unique
           when /column(s)? .* (is|are) not unique/, /UNIQUE constraint failed: .*/
-            RecordNotUnique.new(message)
+            RecordNotUnique.new(message, sql: sql, binds: binds)
           when /.* may not be NULL/, /NOT NULL constraint failed: .*/
-            NotNullViolation.new(message)
+            NotNullViolation.new(message, sql: sql, binds: binds)
           when /FOREIGN KEY constraint failed/i
-            InvalidForeignKey.new(message)
+            InvalidForeignKey.new(message, sql: sql, binds: binds)
           else
             super
           end

activerecord/lib/active_record/errors.rb

@@ -97,9 +97,13 @@ module ActiveRecord
   #
   # Wraps the underlying database error as +cause+.
   class StatementInvalid < ActiveRecordError
-    def initialize(message = nil)
+    def initialize(message = nil, sql: nil, binds: nil)
       super(message || $!.try(:message))
+      @sql = sql
+      @binds = binds
     end
+
+    attr_reader :sql, :binds
   end
 
   # Defunct wrapper class kept for compatibility.
@@ -118,7 +122,7 @@ module ActiveRecord
 
   # Raised when a foreign key constraint cannot be added because the column type does not match the referenced column type.
   class MismatchedForeignKey < StatementInvalid
-    def initialize(adapter = nil, message: nil, table: nil, foreign_key: nil, target_table: nil, primary_key: nil)
+    def initialize(adapter = nil, message: nil, sql: nil, binds: nil, table: nil, foreign_key: nil, target_table: nil, primary_key: nil)
       @adapter = adapter
       if table
         msg = +<<~EOM
@@ -135,7 +139,7 @@ module ActiveRecord
       if message
         msg << "\nOriginal message: #{message}"
       end
-      super(msg)
+      super(msg, sql: sql, binds: binds)
     end
 
     private

activerecord/test/cases/adapter_test.rb

@@ -286,18 +286,6 @@ module ActiveRecord
       assert_equal "special_db_type", @connection.type_to_sql(:special_db_type)
     end
 
-    unless current_adapter?(:PostgreSQLAdapter)
-      def test_log_invalid_encoding
-        error = assert_raises RuntimeError do
-          @connection.send :log, "SELECT 'ы' FROM DUAL" do
-            raise (+"ы").force_encoding(Encoding::ASCII_8BIT)
-          end
-        end
-
-        assert_equal "ы", error.message
-      end
-    end
-
     def test_supports_multi_insert_is_deprecated
       assert_deprecated { @connection.supports_multi_insert? }
     end

activerecord/test/cases/calculations_test.rb

@@ -218,8 +218,8 @@ class CalculationsTest < ActiveRecord::TestCase
       Account.select("credit_limit, firm_name").count
     }
 
-    assert_match %r{accounts}i, e.message
+    assert_match %r{accounts}i, e.sql
-    assert_match "credit_limit, firm_name", e.message
+    assert_match "credit_limit, firm_name", e.sql
   end
 
   def test_apply_distinct_in_count

activerecord/test/cases/statement_invalid_test.rb

@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+
+require "cases/helper"
+require "models/book"
+
+module ActiveRecord
+  class StatementInvalidTest < ActiveRecord::TestCase
+    fixtures :books
+
+    class MockDatabaseError < StandardError
+      def result
+        0
+      end
+
+      def error_number
+        0
+      end
+    end
+
+    test "message contains no sql" do
+      sql = Book.where(author_id: 96, cover: "hard").to_sql
+      error = assert_raises(ActiveRecord::StatementInvalid) do
+        Book.connection.send(:log, sql, Book.name) do
+          raise MockDatabaseError
+        end
+      end
+      assert_not error.message.include?("SELECT")
+    end
+
+    test "statement and binds are set on select" do
+      sql = Book.where(author_id: 96, cover: "hard").to_sql
+      binds = [Minitest::Mock.new, Minitest::Mock.new]
+      error = assert_raises(ActiveRecord::StatementInvalid) do
+        Book.connection.send(:log, sql, Book.name, binds) do
+          raise MockDatabaseError
+        end
+      end
+      assert_equal error.sql, sql
+      assert_equal error.binds, binds
+    end
+  end
+end