[Dbix-class] RFC: What to do with $resultset->single returning multiple rows?

[John Napiorkowski]

Hi,

While trying to track down a bug in .08 trunk that was causing an error on mysql I discovered that the problem was due to a change in the way DBIx::Class::Storage::DBI->select_single handled results that returned more than a single row.  In 0.08010 and earlier, if ->select_single was run against SQL that returned more than a single row, only the first row was returned and the remaining ones silently discarded.  On DBIC trunk, a bit of code was added that would carp out if more rows existed:

sub select_single {
  my $self = shift;
  my ($rv, $sth, @bind) = $self->_select(@_);
  my @row = $sth->fetchrow_array;
  carp "Query returned more than one row" if $sth->fetchrow_array;
  # Need to call finish() to work round broken DBDs
  $sth->finish();
  return @row;
}


Now, from what I can see, the entire point of this method is to help optimise $resultset->find by just returning a single row without creating a whole cursor.  However, DBIC::Storage::DBI->select_single is also publicly exposed as $resultset->single and is documented as a optimised select for a single row.  Nothing in the docs or previous versions warned users that >single should only be run against SQL that returns a single row.  In fact, one would think that ->find is reserved for this purpose.  So by adding this additional constraint at that point I am very certain we are going to be breaking peoples code.  I know in fact $resultset->single is used in a few spots in our codebase in places that will explode if the above carp is kept.

I was lead to this because the mysql error was due to the fact the line:

carp "Query returned more than one row" if $sth->fetchrow_array;

is going to die when the $sth is exhausted (which was generating 'fetch without execute' errors).  I could fix this to work properly, but I really feel it's not such a good idea to change the API behavior like this without some sort of deprecation cycle.  In addition I am not certain why should change it.  I can see a need for a quick, "get me the first something you find" function.  However at the very least we should update the docs to reflect clarified semantices between  $resultset->first, ->single, and ->find.

So what do you all think?  Our options are:

-1- Revert ->select_single to the old behavior but deprecate $resultset->single so that we can eventually remove it from the API at some point, freeing us to constraint ->select_single when it's no longer part of the public API

-2- Revert ->select_single to the old behavior and deprecate $resultset->single for SQL that returns multiple rows.  Change the carp to some sort of warn and fix the 'fetch without execute' bug.

-3- Revert ->select_single to the old behavior and properly document the existing behavior, in particular to clarify the differences between ->first, ->single, and ->find.  This is basically reverting to the 0.08010 behavior, documenting it, adding a test, etc.

-4-> Push this change through.  Fix the 'fetch without excute bug' and tell everyone they need to grep/ack through their code and replace ->single with either ->first or ->find for the cases the underlying SQL could return more than one row.

To be honest I prefer option 3, since that's the easiest thing for me and my deployment.  I really don't see much of a use for $resultset->single that only is useful against SQL that is guaranteed to return a single row.  Seems to me that is what ->find is for.  However I do see value in a cursorless form of ->first, since sometimes you just want to grab the first thing as fast as possible.

Certainly at the minimum we should clarify the docs.  I think we should push people away from ->single if what they really want in ->first.

I'm ready to make these changes, fix up the docs, add a test.  Let's just decide so it's part of this next release.

--John


      ____________________________________________________________________________________
Be a better friend, newshound, and 
know-it-all with Yahoo! Mobile.  Try it now.  http://mobile.yahoo.com/;_ylt=Ahu06i62sR8HDtDypao8Wcj9tAcJ