[Dbix-class] '+select'/'+as' is broken when chaining resultsets

[BUCHMULLER Norbert]

On Tue, 28 Oct 2008 11:21:25 +0100 Peter Rabbitson
<rabbit+list at rabbit.us> wrote:

> > Regarding 1.: I think that should be OK.
> 
> These tests go into t/76retrieve.t (new file)

Done (see the new patch).

> > Regarding 2.: we should have such a test IMHO, otherwise we cannot
> > trust the other test cases (as we cannot be sure that a column really
> > exists - which the state is currently).
> 
> This belongs in t/76retrieve.t too

No, not really. Let me point out that the test in line 36 of
t/88_result_set_column.t can never fail. This line is the following:

ok(defined($psrs->get_column('count')), '+select/+as count');

Now, $psrs->get_column() _always_ returns a DBIx::Class::ResultSetColumn
object, independently of whether the named column exists in the resultset
or not. Please try it: alter the column name to something else ('foo'
comes to mind:-), and see that the test still passes. And it means that
those tests test nothing.. :-(

The same problem affects a lot of other tests in that file.

> Apparently you should be able to call get_column on a nonexistent
> resultset column,

Yes, I agree with that. (That was my reasoning why I did not try to 'fix'
DBIx::Class::ResultSetColumn or DBIx::Class::ResultSet but tried to fix
the tests.)

> with the tests in t/88_result_set_column.t safeguarding this.

This clause I don't really understand.

> All in all your new patch should make no modifications to
> t/88_result_set_column.t at all.

I still think that we should, but probably not the way I did. :-)

The other approach that came to my mind is to change

 defined($psrs->get_column('...'))

to

 defined($psrs->get_column('...')->first)

But the later expression does not fail with a false value but an
exception if the column does not exist. That means that we have to tweak
the tests a bit, and write Test::Exception check methods instead, eg.
change this:

 ok(defined($psrs->get_column('...')), '...');

to

 lives_ok($psrs->get_column('...')->first, '...');

What do you think of it?

norbi