[Dbix-class] find_or_create and unique constraints

[Peter Rabbitson]

Bill Moseley wrote:
> 
> 
> On Sat, Oct 16, 2010 at 1:57 AM, Peter Rabbitson <rabbit+dbic at rabbit.us 
> <mailto:rabbit%2Bdbic at rabbit.us>> wrote:
> 
>     I should have worded it differently, I meant "architectural
>     correctness".
>     But I respectfully disagree with the part of your post about "violating
>     principle of least surprise". All mature APIs have warts, and DBIC is by
>     far no exception. While breaking compat to correct glaring deficiencies
>     in operation is crucial, breaking compat *just* for the sake of removing
>     such "the api is ugly" warts is unacceptable.
>     What started this thread wasn't a codepath failure, it was a blatant
>     documentation failure, and this is being rectified asap.
> 
> 
> Hi Peter,
> 
> We have perhaps a dozen people that may touch the app code, and the sad 
> truth is not all, no few, read the docs well enough.  That's in no way 
> DBIC's fault but it's what often happens in a busy office.  Clearly, we 
> don't want to break the existing API in anyway -- surprises due to an 
> upgrade is never good.  But, providing a way for people (managers) to 
> tighten the API can be useful.

Indeed, read on.

> (quoting from previous message)
> 
>     Do you think that a stronger warning about multiple rows and non
>     unique constraints used in find() with, say, an option "strict_find
>     => 1" that could be added to enforce it with the goal to make it the
>     default in later versions would still break DBIC for many existing
>     users?
> 
>     I went to look at the docs and realized that they do not actually
>     talk about this at all, so we started with different initial info.
>     This option had existed since the very beginning and is called
>     'key', which basically means "you either satisfy this particular
>     unique constraint or you don't find anything". Without a 'key'
>     argument, find() becomes a best-effort wrapper around search() with
>     some extra juice.
> 
> 
> I'm aware of the "key" option but have relied on the "automatic" 
> approach find uses if not provided a key to determine a unique 
> constraint.  My concern is I doubt that it would be used consistently 
> w/o some type of enforcement.  I wonder how common its usage is.
> 
> Also, I IIRC, it's possible to specify a key but not provide any or all 
> of the key's columns which results in a non-unique query.  (Of course 
> there's nothing to prevent calling $rs->find w/o any keys either.)

This has now been fixed, thank you for making me look at the code close
enough to realize how bad it really was :)
http://git.shadowcat.co.uk/gitweb/gitweb.cgi?p=dbsrgits/DBIx-Class.git;a=commit;h=95d028fecb626f3a90438c51dfdac98bdaf9eab3

> Again, it's pretty easy for me to add code to report on non-unique 
> constraints and when duplicate rows are returned (as we had because a 
> column was added as a unique constraint in the db but not updated in 
> DBIC).  I've now disabled it as we fix code and clean up the database 
> (as it overwhelmed the logs).  But, it would have been great to have it 
> from the start of this project to catch early on.  I suspect others 
> could easily fall into the same situation.

So all you have to do now is override find() in your base resultset class
to croak if no key attribute has been supplied and there you have it -
fully tightened unique find for the entire app (which is also pretty hard
to circumvent, at least by accident).

That said I am starting to ponder the idea of an app-wide strict switch,
however this is not something that is likely to happen this month.

Also please offer your comments on the new documentation:

diff: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?p=dbsrgits/DBIx-Class.git;a=commitdiff;h=8e0a1115f5fbe517102444fd52d51d7d2a610795
result: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?p=dbsrgits/DBIx-Class.git;a=blob;f=lib/DBIx/Class/ResultSet.pm;hb=8e0a1115f5fbe517102444fd52d51d7d2a610795

Thanks for persisting :)

Cheers!