From 192b7bcfacd550312b37e74d858b63b77d2469d5 Mon Sep 17 00:00:00 2001 From: Gannon McGibbon Date: Thu, 15 Nov 2018 19:55:47 -0500 Subject: [PATCH] Redact SQL in errors Move `ActiveRecord::StatementInvalid` SQL to error property. Also add bindings as an error property. --- activerecord/CHANGELOG.md | 20 +++++++++ .../connection_adapters/abstract_adapter.rb | 18 ++++---- .../abstract_mysql_adapter.rb | 30 ++++++------- .../connection_adapters/postgresql_adapter.rb | 26 ++++++------ .../connection_adapters/sqlite3_adapter.rb | 8 ++-- activerecord/lib/active_record/errors.rb | 10 +++-- activerecord/test/cases/adapter_test.rb | 12 ------ activerecord/test/cases/calculations_test.rb | 4 +- .../test/cases/statement_invalid_test.rb | 42 +++++++++++++++++++ 9 files changed, 112 insertions(+), 58 deletions(-) create mode 100644 activerecord/test/cases/statement_invalid_test.rb diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 2a237f86cf..24a64e9deb 100644 --- a/activerecord/CHANGELOG.md +++ b/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: diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index 0fe868478c..bd095fbc73 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/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) - begin - message = "#{e.class.name}: #{e.message}: #{sql}" - rescue Encoding::CompatibilityError - message = "#{e.class.name}: #{e.message.force_encoding sql.encoding}: #{sql}" - end + def translate_exception_class(e, sql, binds) + message = "#{e.class.name}: #{e.message}" - 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 diff --git a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb index 13c799b64a..2d7f34ef24 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb +++ b/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) - parts = message.scan(/`(\w+)`[ $)]/).flatten + def mismatched_foreign_key(message, sql:, binds:) + 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], diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb index d2ed699ee2..62ef5f72d9 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/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 diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb index 3312c3de01..b2daae9ace 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb +++ b/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 diff --git a/activerecord/lib/active_record/errors.rb b/activerecord/lib/active_record/errors.rb index f61bc7b9e8..7e6d4c1d46 100644 --- a/activerecord/lib/active_record/errors.rb +++ b/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 diff --git a/activerecord/test/cases/adapter_test.rb b/activerecord/test/cases/adapter_test.rb index 64c2b51f83..d3ed59bccd 100644 --- a/activerecord/test/cases/adapter_test.rb +++ b/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 diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index 5c9ed42173..97bce90c8b 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/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 "credit_limit, firm_name", e.message + assert_match %r{accounts}i, e.sql + assert_match "credit_limit, firm_name", e.sql end def test_apply_distinct_in_count diff --git a/activerecord/test/cases/statement_invalid_test.rb b/activerecord/test/cases/statement_invalid_test.rb new file mode 100644 index 0000000000..16ea69c1bd --- /dev/null +++ b/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