Bug #91375 for DBIx-Class-ResultSet-RecursiveUpdate: possible bug with including constraints in a relationship definition?

Thu Dec 12 17:02:59 2013 ether [...] cpan.org - Ticket created

Subject:

possible bug with including constraints in a relationship definition?

13:45 < ether> I just discovered that when specifying an extra condition in a relationship (the third argument to has_many or whatever), if one wants to add a condition on a field being null, one cannot say "$args->{foreign_alias}.fieldname" => undef -- you have to say "$args->{foreign_alias}.fieldname" => { '=' => undef } 13:46 < ether> with the former, the query isn't even done -- you simply get undef back when you invoke the relationship. 13:46 < ether> I spent a little while trying to figure out why it looked like I had no associated rows :/ 14:08 < jnap> ether: sounds like you found a bug to me, anyone disagree? 14:09 < jnap> I would mostly expect those relationship args to be mostly like search args This seems like too basic an issue to be a real bug, but if so, I'm not sure what I'm doing wrong.

Wed Dec 18 18:53:26 2013 ether [...] cpan.org - Correspondence added

There is now a demonstration test case in topic/rt91375_relationship_null_constraint

Thu Dec 19 03:45:13 2013 ribasushi [...] leporine.io - Correspondence added

On Wed Dec 18 18:53:26 2013, ETHER wrote: Show quoted text

> There is now a demonstration test case in > topic/rt91375_relationship_null_constraint

<ribasushi> ether: I played with the test case a bit - I can't seem to get anywhere <ribasushi> ether: your original test in e90ec01b goes 'title' => undef, which never matches because title is non-nullable (so dbic dtrt and does not return you anything) <ribasushi> ether: I expanded and complicated the test in 7c5fc2d, and it still all works as expected <ribasushi> ether: something else is at play, you need to go back to the original situation where you saw the behavior and dig much deeper (that is - I *do* believe you saw something odd, just the current test is *not* it :) Also something I did not mention on IRC - the DBIC_TRACE=1 output looks correct and expected as well.

Thu Dec 19 03:45:15 2013 The RT System itself - Status changed from 'new' to 'open'

Thu Dec 19 13:00:39 2013 ether [...] cpan.org - Correspondence added

Show quoted text

> <ribasushi> ether: I played with the test case a bit - I can't seem to > get anywhere

Agreed, after fixing up my test to do what I intended originally, it works. I'm including the code below so as to not clutter the git branches further. (For those of the CE inclination who are interested in the source of this issue, I encountered this while writing CE::Schema::SearchMajorGroup->belongs_to(major => ...); with the constraint "$args->{foreign_alias}.delete_time" => { '=', undef }. # in DBICTest::Schema::Artist: __PACKAGE__->has_many( cds_without_genre => 'DBICTest::Schema::CD', sub { my $args = shift; return +{ "$args->{foreign_alias}.artist" => { -ident => "$args->{self_alias}.artistid" }, # this doesn't seem to work... "$args->{foreign_alias}.genreid" => undef, # instead we need to say this: # "$args->{foreign_alias}.genreid" => { '=' => undef }, } }, ); my $rel_rs = $artist_rs->search_related(cds_without_genre => { artist => 1 }, { order_by => ['cdid'] }); # $rel_rs->all_hri should return two rows

Fri Jan 03 06:04:06 2014 ribasushi [...] leporine.io - Correspondence added

On Thu Dec 19 13:00:39 2013, ETHER wrote: Show quoted text

> > <ribasushi> ether: I played with the test case a bit - I can't seem > > to > > get anywhere

> > > Agreed, after fixing up my test to do what I intended originally, it > works.

I added your test (with squashing) to master at https://github.com/dbsrgits/dbix-class/commit/18c294f430. Since it is not clear what was causing the issue, and circumstances point to something taking place in your darkpan code, I will close this ticket for the time being. Feel free to reopen when/if more info becomes available. Cheers

Fri Jan 03 06:04:09 2014 ribasushi [...] leporine.io - Status changed from 'open' to 'resolved'

Fri Jan 03 13:30:27 2014 ether [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #91375] possible bug with including constraints in a relationship definition?
Date:	Fri, 3 Jan 2014 10:30:08 -0800
To:	Peter Rabbitson via RT <bug-DBIx-Class [...] rt.cpan.org>
From:	Karen Etheridge <ether [...] cpan.org>

On Fri, Jan 03, 2014 at 06:04:08AM -0500, Peter Rabbitson via RT wrote: Show quoted text

> I added your test (with squashing) to master at https://github.com/dbsrgits/dbix-class/commit/18c294f430. Since it is not clear what was causing the issue, and circumstances point to something taking place in your darkpan code, I will close this ticket for the time being. Feel free to reopen when/if more info becomes available.

It might be related to the column in question (that I was constraining to "must be null") having an inflation to DateTime defined in the schema... will investigate more shortly.

Thu Sep 04 14:57:19 2014 ribasushi [...] leporine.io - Correspondence added

On Fri Jan 03 19:30:27 2014, ETHER wrote: Show quoted text

> > It might be related to the column in question (that I was constraining > to > "must be null") having an inflation to DateTime defined in the > schema... will > investigate more shortly.

It is and it isn't. While DBIC itself handles the entire situation correctly (and in fact never invokes the inflation/deflation codepath) this is not the case for the DBIx::Class::RecursiveUpdate component (where I am moving this ticket). The problem stems from this line: https://metacpan.org/source/GSHANK/DBIx-Class-ResultSet-RecursiveUpdate-0.34/lib/DBIx/Class/ResultSet/RecursiveUpdate.pm#L202 where DBIC::RU simply takes everything returned from the inocation of the (internal, undocumented, and soon deprecated) _resolve_condition method and tries to assign it via an accessor which of course blows up, given that IC::DT accessors correctly expect a bare DateTime object. There however is a fix, which comes from the massive rewrite of the condition resolver in the upcoming DBIC 0.082800. The new method in question is *still* private, but this is just a precaution in case of a glaring omission being detected in its implementation. Currently the got-to-be-final API looks like this: https://github.com/dbsrgits/dbix-class/blob/8d54fafa1/lib/DBIx/Class/ResultSource.pm#L1835-L1853 What DBIC::RU should do is use the inferred_values hashref in order to do proper assignment. In places where the information *must* be properly resolved - supplying the infer_values_based_on argument will force an all-or-nothing approach. Let me know if this solves your problem and/or if you have any further questions. Cheers

Thu Sep 04 14:57:54 2014 ribasushi [...] leporine.io - Status changed from 'resolved' to 'open'

Thu Sep 04 14:57:54 2014 ribasushi [...] leporine.io - Queue changed from DBIx-Class to DBIx-Class-ResultSet-RecursiveUpdate

Fri Oct 02 15:11:02 2015 ether [...] cpan.org - Correspondence added

On 2014-09-04 11:57:19, RIBASUSHI wrote: Show quoted text

> The problem stems from this line: > https://metacpan.org/source/GSHANK/DBIx-Class-ResultSet- > RecursiveUpdate-0.34/lib/DBIx/Class/ResultSet/RecursiveUpdate.pm#L202 > where DBIC::RU simply takes everything returned from the inocation of > the (internal, undocumented, and soon deprecated) _resolve_condition > method and tries to assign it via an accessor which of course blows > up, given that IC::DT accessors correctly expect a bare DateTime > object. > > There however is a fix, which comes from the massive rewrite of the > condition resolver in the upcoming DBIC 0.082800. The new method in > question is *still* private, but this is just a precaution in case of > a glaring omission being detected in its implementation. Currently the > got-to-be-final API looks like this: https://github.com/dbsrgits/dbix- > class/blob/8d54fafa1/lib/DBIx/Class/ResultSource.pm#L1835-L1853 > > What DBIC::RU should do is use the inferred_values hashref in order to > do proper assignment. In places where the information *must* be > properly resolved - supplying the infer_values_based_on argument will > force an all-or-nothing approach. Let me know if this solves your > problem and/or if you have any further questions.

What can we do about this ticket? I have code (and features) that are blocked on not being able to construct proper join conditions in relationships that are used by some forms.

Fri Oct 02 19:03:02 2015 gshank [...] cpan.org - Correspondence added

Show quoted text

> What can we do about this ticket? I have code (and features) that are > blocked on not being able to construct proper join conditions in > relationships that are used by some forms.

Do you have a testcase?