Keep orignal exception #5641

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

This change would enable better error handling of failures do to complex constraints and triggers.

There is a lot of useful information in the original error.

Owner
jeremy commented Mar 29, 2012

Retaining the original exception is great, but this breaks everything that does rescue ActiveRecord::StatementInvalid

ActiveRecord::WrappedDatabaseException inherits from ActiveRecord::StatementInvalid so your code would catch it.

Owner
jeremy commented Mar 29, 2012

Indeed! My bad.

@jeremy can we revisit this, to see if it can be merged? Perhaps we'd need some test?

Owner
jeremy commented May 1, 2012

👍 :)

So is it that decides if this gets merged ?

@bjornblomqvist yeah, it is, but first could you add a test for it please?

There are some failing tests when I apply this change, like this one:

test_add_column_not_null_with_default(ActiveRecord::Migration::ChangeSchemaTest) [./rails/activerecord/test/cases/migration/change_schema_test.rb:193]:
[ActiveRecord::StatementInvalid] exception expected, not
Class: <ActiveRecord::WrappedDatabaseException>
Message: <"SQLite3::ConstraintException: testings.bar may not be NULL: insert into testings (\"id\", \"foo\", \"bar\") values (2, 'hello', NULL)">

Apparently because minitest asserts using class==. This means I'd have to change from StatementInvalid to WrappedDatabaseException everywhere in the test suite, and if someone is relying on StatementInvalid in their tests using assert_raise, it'd also fail, making this change not backward compatible.

@jeremy can we go ahead with the change, or we stop here?

Apparently we stop here, let us know about any update and I'll reopen, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment