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)

History Show all quoted text
  Mon Dec 13 08:24:49 2010 TONVOON [...] cpan.org - Ticket created
Subject: Reducing $SIG{__DIE__} errors in find()
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');
  Wed Dec 15 06:47:52 2010 ribasushi [...] leporine.io - Correspondence added
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.
  Wed Dec 15 06:47:53 2010 The RT System itself - Status changed from 'new' to 'open'
  Wed Dec 15 09:47:42 2010 TONVOON [...] cpan.org - Correspondence added
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
  Wed Dec 15 11:35:21 2010 rt-cpan [...] trout.me.uk - Correspondence added
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.
  Wed Dec 15 11:35:22 2010 rt-cpan [...] trout.me.uk - Status changed from 'open' to 'rejected'
  Wed Dec 15 12:05:28 2010 TONVOON [...] cpan.org - Correspondence added
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
  Wed Dec 15 12:05:29 2010 The RT System itself - Status changed from 'rejected' to 'open'
  Wed Dec 15 12:28:17 2010 rt-cpan [...] trout.me.uk - Correspondence added
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 :)
  Wed Dec 15 12:28:18 2010 rt-cpan [...] trout.me.uk - Status changed from 'open' to 'rejected'
  Wed Mar 23 09:39:42 2016 ribasushi [...] leporine.io - Correspondence added
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.
  Wed Mar 23 09:39:48 2016 ribasushi [...] leporine.io - Status changed from 'rejected' to 'patched'
  Sun Jan 28 12:31:27 2018 ribasushi [...] leporine.io - Dependency by ticket #124247 added