Rails Ruby on Rails → 3.0.2

Comments and changes to this ticket

• You flagged this item as spam.
  

  Michael Schuerig April 5th, 2009 @ 12:55 AM

  • Title changed from “Raise specific exceptions for violated database constraints” to “[Patch] Raise specific exceptions for violated database constraints”

• You flagged this item as spam.
  

  JasonKing April 5th, 2009 @ 01:53 AM

  Looks good.

  I had been working on something like this a little while back in some spare cycles - the one thing I really found useful was to pull out the column name that had caused the violation. That lets you use the exceptions in your validations (and makes a non-race vuo really trivial) flagging the field to the end user that caused the problem.

  I wonder if there's room in your patch to have a look at this? MySQL and SQLite both report the column(s) in the error message.

  With Postgres I think you just get the index name. So you'd need to infer it from the name of the index (fragile - relies on the index having been created using a migration helper) or query the DB for that index to find the column(s) (or something else magical :)

• 

  Michael Koziarski April 5th, 2009 @ 03:19 AM

  • Assigned user set to “Michael Koziarski”

  I think all those subclasses should have an accessor for the original exception. That way there's no dataloss and people wanting to do awesome things with the original exceptions can get access to it. e.g.

  
  rescue => e
    if e.original_exception.some_crazy_stuff
  

  I also like the idea of having that mapping stuff happen in a seperate class rather than just the adapter, but that's neither here nor there.

• You flagged this item as spam.
  

  Michael Schuerig April 5th, 2009 @ 08:56 AM

  I've added the original adapter exception to ActiveRecord::InvalidStatement (and descendents).

  Using a separate class for the mapping seems a bit like overkill to me at this stage where the code involved comprises just a few lines. Extracting it would mean an additional class for each adapter. In case the amount of code grows, however, I'm all for separation.

  • db-exceptions.patch 12.4 KB

• You flagged this item as spam.
  

  Michael Schuerig April 5th, 2009 @ 09:20 AM

  • Tag changed from “activerecord, constraint, exception, foreign-key, uniqueness” to “activerecord, constraint, exception, foreign-key, patch, uniqueness”

  Jason, I've considered your idea, and while I'm all for providing as much information as possible in an exception, I'm not sure whether it is really a good idea in this case.

  Apart from violated uniqueness constraints (and retryable concurrency-related errors), all other exception raised from the database indicate programming errors, as far as I can see. In that case, all information is available in the log, just not easily interpreted by the program. IOW, you should not try to recover from these exceptions, you should fix the bugs that caused them.

  As to implementing validates_uniqueness_of using DB exceptions, here's what I do in a mocdel class to handle the newly minted exception. The drawback is that #create_or_update is private.

  
    def create_or_update
      Awarding.transaction(:requires_new => true) do
        # The nested transaction is required 
        # at least for PostgreSQL as otherwise
        # the failing INSERT aborts the entire
        # (outer) transaction.
        super
      end
    rescue ActiveRecord::RecordNotUnique => e
      errors.add_to_base('Sorry, already taken.')
      false
    end
  

  Code like this could be pushed into ActiveRecord.

• 

  Michael Koziarski April 5th, 2009 @ 09:58 AM

  Last thoughts on this are, do sqlite and postgresql not have error codes? I'm not sure the current approach you're taking there will work with LANG != C

• You flagged this item as spam.
  

  Michael Schuerig April 5th, 2009 @ 11:51 AM

  I would have much preferred to dispatch on error codes for all databases, unfortunately this doesn't appear to be possible with the drivers as they are.

  The pg driver doesn't propagate any error codes. Any improvements would have to rely on a patched driver.

  The sqlite3 driver does support error codes, but all the interesting causes map to error code 1 and SQLite3::SQLException, as far as I can tell.

  For both PostgreSQL and SQLite3, the error message doesn't seem to depend on the locale of the client app. It might be affected by the locale of the server.

• You flagged this item as spam.
  

  JasonKing April 5th, 2009 @ 12:31 PM

  From earlier - I think you're right and that it's cleaner to leave my suggestion out of your patch. Propagating the original exception is the best solution. Let a plugin wrap it up in a custom v_u_o or for some other purpose.

• You flagged this item as spam.
  

  Max Lapshin April 20th, 2009 @ 05:51 PM

  It seems, that Postgresql libpq doesn't return error codes at all

• You flagged this item as spam.
  

  Nate Wiger April 22nd, 2009 @ 08:05 PM

  +1 for this please

  This would be a HUGE help for us. We use unique contraints and foreign keys all over the place, and explicitly DON'T use validates_uniqueness_of due to performance concerns.

  Thanks.

• You flagged this item as spam.
  

  blythe April 27th, 2009 @ 05:46 PM

  +1 for this Would love to use this to gracefully rescue unique and foreign keys. Would be cool to have the violated key name also like exception.key_name for tables with multiple fk indexes if supported by the DB but as is, is a huge help.

• You flagged this item as spam.
  

  Michael Schuerig April 27th, 2009 @ 08:07 PM

  I'm currently trying to figure out a way to get at any information from PostgreSQL after an error occurred. For a uniqueness violation, I can get the columns involved, but I need a usable connection. Unfortunately, PostgreSQL rejects any attempt to use the connection until a ROLLBACK is executed.

  Now, there are three cases that I don't know how to distinguish when I translate the exception:

  • There is no transaction in progress and the connection is usable as is.
  • I'm in a transaction and the connection is not usable until ROLLBACK.
  • I'm in a nested transaction and the connection is not usable until ROLLBACK TO SAVEPOINT or ROLLBACK.
• 

  Michael Koziarski April 28th, 2009 @ 05:34 AM

  Fundamentally I'm uncomfortable doing anything that relies on issuing subsequent queries. If the API doesnt let you extract the relevant information, then just make sure that the error message and the like are available (as they are with a chained exception).

  If it turns out people can figure out a reliable way to figure out which constraint / column caused the error, we can roll that in at a later date.

• You flagged this item as spam.
  

  Michael Schuerig April 28th, 2009 @ 09:23 AM

  Good point, then I'll leave the patch as it is. Is there anything still missing?

• 

  Michael Koziarski April 28th, 2009 @ 09:36 AM

  if we can get some +1s from people using older postgresql releases, and/or the postgres gem instead of the pg gem. Then I'm happy to apply it.

• You flagged this item as spam.
  

  Will Bryant May 11th, 2009 @ 02:59 AM

  I'm getting one sqlite test failure from this patch:

  1) Failure: test_uniqueness_violations_are_translated_to_specific_exception(AdapterTest)

  [./test/cases/adapter_test.rb:136:in `test_uniqueness_violations_are_translated_to_specific_exception'
   ./test/cases/../../../activesupport/lib/active_support/testing/setup_and_teardown.rb:62:in `__send__'
   ./test/cases/../../../activesupport/lib/active_support/testing/setup_and_teardown.rb:62:in `run']:
  
  

  <ActiveRecord::RecordNotUnique> exception expected but was Class: <ActiveRecord::StatementInvalid> Message: <"SQLite3::SQLException: SQL logic error or missing database: INSERT INTO subscribers(nick) VALUES('me')"> ---Backtrace--- ./test/cases/../../lib/active_record/connection_adapters/abstract_adapter.rb:212:in log' ./test/cases/../../lib/active_record/connection_adapters/sqlite_adapter.rb:157:inexecute_without_query_record' ./test/cases/../../lib/active_record/connection_adapters/sqlite_adapter.rb:402:in catch_schema_changes' ./test/cases/../../lib/active_record/connection_adapters/sqlite_adapter.rb:157:inexecute_without_query_record' ./test/cases/helper.rb:37:in execute' ./test/cases/adapter_test.rb:137:intest_uniqueness_violations_are_translated_to_specific_exception' ./test/cases/adapter_test.rb:136:in test_uniqueness_violations_are_translated_to_specific_exception' ./test/cases/../../../activesupport/lib/active_support/testing/setup_and_teardown.rb:62:insend'

  ./test/cases/../../../activesupport/lib/active_support/testing/setup_and_teardown.rb:62:in run'

  sqlite3-ruby 1.2.1, sqlite3 macport @3.6.11_0.

• You flagged this item as spam.
  

  Michael Schuerig May 11th, 2009 @ 10:24 AM

  Will, I can't reproduce this problem. I've tried it on the 2-3-stable branch of a fresh git clone. Applied the patches and ran the tests for mysql, postgresql, sqlite3, and sqlite.

  The drivers/gems I've been using are sqlite-ruby-2.2.3 and sqlite3-ruby-1.2.4.

• You flagged this item as spam.
  

  Sava Chankov May 12th, 2009 @ 11:16 PM

  1. Tested with postgres gem and PostgreSQL 8.3.

  This and many other ActiveRecord tests don't pass with PostgreSQL 8.1 because of test/schema/postgresql_specific_schema.rb relying on DROP TABLE ... IF EXISTS which was introduced in 8.2.

• 

  Michael Koziarski May 14th, 2009 @ 01:29 AM

  Can anyone reproduce the sqlite errors? I'm happy to apply this but am
  a little worried by them

• You flagged this item as spam.
  

  Michael Schuerig June 14th, 2009 @ 06:49 PM

  As Sava points out, ActiveRecord tests are not compatible with PostgreSQL versions older than 8.2. Am I right in taking this as evidence that these older versions are de facto unsupported as they are not covered by any quality assurance?

  The sqlite problems appear to be non-reproducible. So, what's holding back this patch? I'm happy to put in some more work if that's what is needed.

• 

  Michael Koziarski June 15th, 2009 @ 09:54 AM

  • Milestone cleared.

  The only thing holding this back now is me having a few free moments.

  I'll take a look tomorrow.

• 

  Repository June 26th, 2009 @ 06:03 AM

  • State changed from “new” to “committed”

  (from [b5dfdc714fab7d2836e0a979ca88b4a17db9ec06]) Make sure the wrapped exceptions also have the original exception available.

  [#2419 state:committed] http://github.com/rails/rails/commit/b5dfdc714fab7d2836e0a979ca88b4...

• You flagged this item as spam.
  

  Jordan Brough November 12th, 2009 @ 10:35 PM

  For anyone interested I've built on this to create a patch for Rails to handle db-level unique constraints gracefully, which has several advantages in many cases. Ticket is #3486

• You flagged this item as spam.
  

  Jordan Brough November 12th, 2009 @ 10:37 PM

  Also, is there a chance of applying the commits from ticket to to 2-3-stable? (talking about 4d614ec0429ce40c4784162c45ed06e125c0d7de, 605acee71391729ae82ba9012bc37f2f1716fb80, 9b39032925b68a724bd75174db642bc3d2f891fb) They seem to apply cleanly for me.

• You flagged this item as spam.
  

  arzvi November 19th, 2009 @ 07:14 AM

  I am rails newbie, I tried installing your patch using
  scripts/plugin install
  and used your code to test, but throws error. Please help

• You flagged this item as spam.
  

  JasonKing November 19th, 2009 @ 06:26 PM

  arzvi - "rails newbie"s really shouldn't be trying to apply patches to the Rails core. Just wait for the 3.0 release.

• You flagged this item as spam.
  

  tribalvibes March 28th, 2010 @ 03:23 AM

  @arzvi Here is a rollup of the patch based on Rails 2.3.5 release. Just drop this in your config/initializers. It omits the tests but "seems to work" for me with mysql.

  • add_record_not_unique_exception.rb 3 KB
• 

  Jeremy Kemper October 15th, 2010 @ 11:01 PM

  • Milestone set to “3.0.2”
  • Importance changed from “” to “”

  [bulk edit]