AutoRemoveColumns should not override an explicit columns attribute #27

Closed
derobert opened this Issue Apr 5, 2014 · 4 comments

Projects

None yet

2 participants

Contributor
derobert commented Apr 5, 2014

I tried this. Note that Pool::Passholder's holder_photo is a bytea, so is auto-removed:

my $row = $c->model('Pool::Passholder')->find($passholder_no, { key => 'primary', columns => [ 'holder_photo' ] })

And was rather surprised when it selected all the columns except holder_photo. It turns out it completely ignored the columns attribute.

This appears to be a bug in how RemoveColumns is implemented. A trivial fix is:

Index: libdbix-class-helpers-perl-2.019004/lib/DBIx/Class/Helper/ResultSet/RemoveColumns.pm
===================================================================
--- libdbix-class-helpers-perl-2.019004.orig/lib/DBIx/Class/Helper/ResultSet/RemoveColumns.pm   2014-02-14 08:53:27.0000
+++ libdbix-class-helpers-perl-2.019004/lib/DBIx/Class/Helper/ResultSet/RemoveColumns.pm        2014-04-05 13:36:38.2834
@@ -13,7 +13,7 @@

    my $attrs  = $self->{attrs}; # not copying on purpose...

-   if ( $attrs->{remove_columns} ) {
+   if ( !$attrs->{columns} && $attrs->{remove_columns} ) {
       my %rc = map { $_ => 1 } @{$attrs->{remove_columns}};
       $attrs->{columns} = [
          grep { !$rc{$_} } $self->result_source->columns
Owner
frioux commented Apr 5, 2014

Oh awesome! I'll see if I can get this merged tonight; thanks!

Contributor
derobert commented Apr 5, 2014

By the way, I got a test failure in t/schema/lint-contents.t, where it expected Bloaty to be undef. Those are counts, which shouldn't be undef... So possibly I've accidentally fixed Issue #24 as well.

@derobert derobert added a commit to derobert/Westerley-Pool that referenced this issue Apr 5, 2014
@derobert derobert Only pull the BLOB when needed. Better error.
This turned out to be fun. Google found me an old email from 2010
<https://www.mail-archive.com/dbix-class@lists.scsys.co.uk/msg04161.html>.
A message there indicated fREW Schmidt is planning on implementing.
Found out he did, at
<https://metacpan.org/pod/DBIx::Class::Helper::ResultSet::RemoveColumns>
and that even better, there is one to do it automatically
<https://metacpan.org/pod/DBIx::Class::Helper::ResultSet::AutoRemoveColumns>.

Implemented that, and had the JPEG controller explicitly ask for the
column... and it was ignored. After some fiddling (do I remember DBIC
syntax right? Yes.) turns out to be a bug in that RemoveColumns
module... Bug reported, along with a quick patch
<frioux/DBIx-Class-Helpers#27> and it now
works.

Finally.

Also, when an image isn't found, don't die(). Instead, send back a 404.
75bbd66
Owner
frioux commented Apr 5, 2014

yeah, that's what I was thinking and why I responded to this as quickly as I did, because I really did not want to look at that issue ☻

Owner
frioux commented Apr 6, 2014

Thanks again for the help! Fixed and released

@frioux frioux closed this Apr 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment