Skip Menu |
 

This queue is for tickets about the DBIx-Class CPAN distribution.

Report information
The Basics
Id: 63874
Status: patched
Priority: 0/
Queue: DBIx-Class

People
Owner: Nobody in particular
Requestors: TONVOON [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: Normal
Broken in: (no value)
Fixed in: (no value)

Attachments


Subject: Reducing $SIG{__DIE__} errors in find()
Download (untitled) / with headers
text/plain 962b
We updated Opsview to the latest DBIx::Class 0.08124 and got loads of unexpected errors. In one script, we set $SIG{__DIE__} to catch any unexpected conditions and store those. With the update, we were getting lots of errors with: DBIx::Class::InflateColumn::get_inflated_column(): Unable to satisfy requested constraint 'name', no values for column(s): 'name' at (eval 1396) line 2 This is due to _build_unique_cond issuing a throw_exception when conditions are passed in that do not match with the unique_conditions it expects. However, a call like $rs->find(1) will try all the different unique constraints to see if it can find something, thus causing the above message to get thrown in non-exceptional circumstances. I've made a patch against 0.08124 (attached) which tells _build_unique_cond to avoid throwing this error if a flag is set. There is also an update to t/60core.t to catch this situation. Can this go in a future DBIx-Class release?
Subject: dbix_class_with_less_dies.patch
diff -ur DBIx-Class-0.08124/lib/DBIx/Class/ResultSet.pm DBIx-Class-0.08124.with_less_dies/lib/DBIx/Class/ResultSet.pm --- DBIx-Class-0.08124/lib/DBIx/Class/ResultSet.pm 2010-10-27 23:15:17.000000000 +0000 +++ DBIx-Class-0.08124.with_less_dies/lib/DBIx/Class/ResultSet.pm 2010-12-09 11:17:39.000000000 +0000 @@ -552,9 +552,7 @@ join "\x00", sort $rsrc->unique_constraint_columns($c_name) }++; - push @unique_queries, try { - $self->_build_unique_cond ($c_name, $call_cond) - } || (); + push @unique_queries, ( $self->_build_unique_cond ($c_name, $call_cond, 1) || () ); } $final_cond = @unique_queries @@ -612,7 +610,7 @@ } sub _build_unique_cond { - my ($self, $constraint_name, $extra_cond) = @_; + my ($self, $constraint_name, $extra_cond, $hide_throw) = @_; my @c_cols = $self->result_source->unique_constraint_columns($constraint_name); @@ -627,6 +625,7 @@ $final_cond = { map { $_ => $final_cond->{$_} } @c_cols }; if (my @missing = grep { ! defined $final_cond->{$_} } (@c_cols) ) { + return undef if $hide_throw; $self->throw_exception( sprintf ( "Unable to satisfy requested constraint '%s', no values for column(s): %s", $constraint_name, join (', ', map { "'$_'" } @missing), diff -ur DBIx-Class-0.08124/t/60core.t DBIx-Class-0.08124.with_less_dies/t/60core.t --- DBIx-Class-0.08124/t/60core.t 2010-12-09 11:23:55.000000000 +0000 +++ DBIx-Class-0.08124.with_less_dies/t/60core.t 2010-12-09 11:26:14.000000000 +0000 @@ -8,6 +8,12 @@ use DBICTest; use DBIC::SqlMakerTest; +my $die_calls = 0; +$SIG{__DIE__} = sub { + #warn("Died: @_"); + $die_calls++; +}; + my $schema = DBICTest->init_schema(); eval { require DateTime::Format::SQLite }; @@ -147,7 +153,9 @@ is($new_obj->in_storage, 0, 'new artist is not in storage'); } +$die_calls = 0; my $cd = $schema->resultset("CD")->find(1); +is( $die_calls, 0, "No exceptions passed to SIG{__DIE__} due to looking for unique keys" ); my %cols = $cd->get_columns; is(keys %cols, 6, 'get_columns number of columns ok');
Download (untitled) / with headers
text/plain 1.1k
On Mon Dec 13 08:24:49 2010, TONVOON wrote: Show quoted text
> We updated Opsview to the latest DBIx::Class 0.08124 and got loads of > unexpected errors. > > In one script, we set $SIG{__DIE__} to catch any unexpected conditions > and store those. With > the update, we were getting lots of errors with: > > DBIx::Class::InflateColumn::get_inflated_column(): Unable to satisfy > requested constraint > 'name', no values for column(s): 'name' at (eval 1396) line 2 > > This is due to _build_unique_cond issuing a throw_exception when > conditions are passed in > that do not match with the unique_conditions it expects. However, a > call like $rs->find(1) > will try all the different unique constraints to see if it can find > something, thus causing the > above message to get thrown in non-exceptional circumstances. > > I've made a patch against 0.08124 (attached) which tells > _build_unique_cond to avoid > throwing this error if a flag is set. There is also an update to > t/60core.t to catch this situation. > > Can this go in a future DBIx-Class release?
Not until you explain in detail why you believe this error is not warranted. Please elaborate why this is something you consider in need of fixing.
Download (untitled) / with headers
text/plain 1.6k
On Wed Dec 15 06:47:52 2010, RIBASUSHI wrote: Show quoted text
> Not until you explain in detail why you believe this error is not > warranted. Please elaborate why this is something you consider in need > of fixing.
If $SIG{__DIE__} is set, then, because the Try::Tiny block does not block __DIE__ signals, the __DIE__ block will get called for "non-important" errors. In the patch to t/60core.t, uncomment the warn() in the $SIG{__DIE__} block and run it. You will get errors such as: Died: DBIx::Class::Row::make_column_dirty(): No such column 'name2' at t/60core.t line 46 Died: DBIx::Class::Relationship::CascadeActions::delete(): Not in database at t/60core.t line 80 Died: DBIx::Class::ResultSet::find(): find() expects either a column/value hashref, or a list of values corresponding to the columns of the specified unique constraint 'primary' at t/60core.t line 133 These are fine to capture because they represent programming problems, or errors when making calls, or errors due to invalid parameters. However, without the patch, you will get messages like: Died: DBIx::Class::Row::discard_changes(): Unable to satisfy requested constraint 'artist_name', no values for column(s): 'name' at t/60core.t line 104 Died: DBIx::Class::Row::discard_changes(): Unable to satisfy requested constraint 'u_nullable', no values for column(s): 'charfield', 'rank' at t/60core.t line 104 These are occurring due to find(5) trying to see if an item can be found for each unique constraint. These are not "important" errors, so I think shouldn't get propagated to the $SIG{__DIE__} routine. If this explanation is not sufficient, do you have an alternative solution? Ton
Download (untitled) / with headers
text/plain 960b
On Wed Dec 15 09:47:42 2010, TONVOON wrote: Show quoted text
> If this explanation is not sufficient, do you have an alternative > solution?
Your usage of SIGDIE is foolish, misguided, and was always wrong. I presume that your aim is to gain additional information about exceptions? If so, store that information somewhere and then use it in your eval/catch/etc. once the exception propagates back to your code (Catalyst::Plugin::StackTrace does this). If not, I fail to see what a warn statement in your eval/catch/etc. handler would not achieve, or a SIGDIE checking $^S to see if the exception is about to kill the script. Either way, exception based flow control is a perfectly sensible pattern and one in common usage, and we just happen to be the first thing that your SIGDIE abuse broke against. Ergo, marking this ticket as rejected in the absence of a PEBKAC status in this RT install. You know where to find me on IRC if you can't figure out how to fix your hack.
Download (untitled) / with headers
text/plain 846b
On Wed Dec 15 11:35:21 2010, MSTROUT wrote: Show quoted text
> On Wed Dec 15 09:47:42 2010, TONVOON wrote:
> > If this explanation is not sufficient, do you have an alternative > > solution?
> > Your usage of SIGDIE is foolish, misguided, and was always wrong. > > I presume that your aim is to gain additional information about > exceptions? If so, store that information somewhere and then use it in > your eval/catch/etc. once the exception propagates back to your code > (Catalyst::Plugin::StackTrace does this). If not, I fail to see what a > warn statement in your eval/catch/etc. handler would not achieve, or a > SIGDIE checking $^S to see if the exception is about to kill the script.
It seems $^S, which I was unaware of, was the key piece of information I was missing. Using that is sufficient to fix my SIGDIE abuse. Apologies for the noise. Ton
Download (untitled) / with headers
text/plain 287b
On Wed Dec 15 12:05:28 2010, TONVOON wrote: Show quoted text
> It seems $^S, which I was unaware of, was the key piece of information > I was missing. Using > that is sufficient to fix my SIGDIE abuse. > > Apologies for the noise.
Top stuff. Setting back to rejected (since your reply re-opened it :)
Download (untitled) / with headers
text/plain 229b
A number of recent failure modes prompted a fundamental rework of the internals, which happens to address this issue as well, particularly in https://github.com/dbsrgits/dbix-class/commit/d0289ee13. Updating ticket for posterity.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.