[Dbix-class] find_or_create and unique constraints

[Peter Rabbitson]

Bill Moseley wrote:
> 
> 
> On Sat, Oct 16, 2010 at 4:28 PM, Peter Rabbitson <rabbit+dbic at rabbit.us 
> <mailto:rabbit%2Bdbic at rabbit.us>> wrote:
> 
> 
>     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).
> 
> 
> Hi Peter,
> 
> I only had a few minutes to review the diffs but I think it looks good. 
>  Thanks for spending so much time on this.  A few comments:
> 
> Maybe move this up after the paragraph describing the behavior if no key 
> is found because it's related:
> 
>      447 If the resulting query produces more than one row, a warning to
>     the effect is
>      448 issued, though only the first row is constructed and returned
>     as C<$row_object>
> 
> 
> as the only reason to get multiple rows is if you don't specify a unique 
> key (or if your unique constraint on the Result class doesn't match the 
> database -- one of my cases).

Noted, will apply.

> I wonder if it would be useful to add a note about making sure correct 
> unique constraints are defined in the result classes.  Another one of 
> our mistakes was not having a unique constraint defined so find() fell 
> back to doing the general search() which returned no rows, but then on 
> create we hit duplicate key errors.

I don't think this will make it any clearer at this point (among the forest
of links to the unique constraint methods)

> 
> Finally, did you intend to remove the example of the list of key values?
> 
>   my $cd = $schema->resultset('CD')->find('Massive Attack', 'Mezzanine', {
>     key => 'cd_artist_title'
>   });

Yes, because the find(@values...) form works *only* if a PK is available,
which in turn makes the 'primary' constraint satisfiable, which in turn
makes 'key' redundant (it will work with or without).

> As for overriding find(), I'm not sure I can just croak if no "key" is 
> provided quite yet.  It would take quite a bit of work to find all 
> places find is called directly or indirectly and add a "key".  I wonder 
> how often people specify a "key".  My guess is not very often -- 
> especially since the normal case is to only have one unique constraint.

There's a subtle difference here - if 'key' is specified the constraint
*must* be enforced, no fallback takes place. This was the intention of
the author all along, but it was never documented properly, and the code
had a silly assumption in it as well.

> Maybe I can override find() and if "key" is not provided attempt to 
> determine a unique constraint and set it -- and warn or croak if a 
> unique constraint cannot be determined.
> 
> That was a bit easier to do previously as I wrapped (the private) 
> _unique_queries. And since that was only called when no "key" was 
> provided I could just check if _unique_queries returned any columns (at 
> least when additional search criteria was provided to find() ) and 
> warn/croak if no unique queries could be found.  Not sure how to do that 
> now.

I see... I will factor out the heuristics code again (though under a
saner name), but it will still stay private until a clearer way to move
forward emerges.
> 
>     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.
> 
> 
> A switch might be a good way to offer more strict-ness.  For  now, what 
> about just abstracting out two methods? Make a public method that 
> handles the no-"key" else block in find() like _unique_queries did 
> before, and another method to generate the warning about multiple rows. 

As I said, for now all I can do is make these private, as I don't want
to add another temporary kludge that we'll have to maintain forever
since it has been made public.

>  Then those can be overridden to enforce more strict behavior.  The 
> difficulty there is the unique query might just be on the result set 
> "cond" so it's not as that simple to determine if a unique constraint 
> was found in the combination of the passed in criteria and resultset 
> condition just by looking at the output from a _unique_queries type of 
> method.  Maybe you have a better idea?

Yes, you missed a part of the code:

sub _build_unique_cond {
   my ($self, $constraint_name, $extra_cond) = @_;

   my @c_cols = $self->result_source->unique_constraint_columns($constraint_name);

   # combination may fail if $self->{cond} is non-trivial
   my ($final_cond) = try {
     $self->_merge_with_rscond ($extra_cond)
   } catch {
     +{ %$extra_cond }
   };

^^^ This try block is what pulls in the resultset cond (if it can, thus
the try) Then the result of this best-effort combination is compared
against the columns of the requested constraint. There are also tests
that ensure this works (and will keep working).

So with that said I will make some changes today to provide you with the
necessary hooks, with the caveat that the hooks are still private and
*will* change down the road. I'd advise to add a ::ResultSet->can(foo)
to your test suite, so you can be notified in no uncertain terms when
these methods go away.

Cheers