Bill Moseley wrote:
On Sat, Oct 16, 2010 at 4:28 PM, Peter Rabbitson <[email protected]
<mailto:rabbit%[email protected]>> 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
_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive: http://www.grokbase.com/group/[email protected]
