[jifty-devel] Jifty-DBI: patch proposal for a delete method for the Collection class

[Frederic BLANC]

 Hmm, sent the message before attaching the new patch version :-(
I'm trying again with the attachement that time.

 -F.

On Thu, Oct 19, 2006 at 02:53:43PM -0400, Jesse Vincent wrote:
> 
[...snip...]
> 
> What would you think about making the method name something a little
> more explicit? 
> 
> $collection->delete_all_records() ?
> 
> 		->delete_records()?

Yes, delete_records() is more accurate than just delete().


> For something adding new SQL (especially new SQL that alters the
> database), tests are essential.

 I am trying to write some tests, but never use the Test::More module,
so I have to learn how to use it before...


> This is also, I think, the first time, users can alter the database's
> content from the collection class.

 Everything has to start some day ;-)  Maybe the delete_records() name is
misnamed too.  Perhaps removed_records_from_db() would be a better name
for that method?


> Given the way we use JDBI in Jifty,
> this would have the side-effect of walking around our access control.

 I concede you that if this method is used as it in Jifty, the access
control would be bypassed, but not all applications need to check
access rights per record-contents basis, most of them (IMO) just need
to check if the user can delete at the table level.
 And I think that the access control may be reintroduced by limiting
the collection to the records the user has the right to delete in most
of the case where access control has to be done regarding the
record-contents.


> This also doesn't quite have the effect I'd expect. 
> 
> If I create a collection, check that it has the right things in it and
> then call delete:
> 	1) I'm not sure I get the right result with regard to paging
> 	2) If someone else has added a matching row in the meantime,
> 	   I'm going to be a VERY sad user ;)

 If you create a collection using various criteria to match the right
things, then do a while ($item = $collection->next) { $item->delete },
I see a 3rd case where someone will be unpleased too: let's say that a
first collection is set like this:
1> $uselessTeam = Society::PersonCollection->new();
1> $uselessTeam->limit( column => 'team', value => 'useless');
1> while($fired = $uselessTeam->next) {
but because it is quite a big team, the following collection was defined
like this:
2> $cheapTeam = Society::PersonCollection->new();
2> $cheapTeam->limit( column => 'cost', operator => '<', value => 1000);
and run:
2> while($item = $cheap->next) { $item->team('cheap') }
before the first call to delete happened:
1>    $fired->delete
1> }
Of course, most of the cheap people were suppose to come from the 'useless'
team; I let you guess who is not going to be happy in this story ;-)

 With my method[*], when you set up your collection and check it has the right
things in it by going trough a ->next() loop, the delete will only happen
on the collection's records that still match the collection's initial criteria.

 [*]: with the corrected patch in attachement. 

> Would you be happy with a solution that called the records' delete
> methods instead?  That has the added benefit of catching model-specific
> delete code too :)

 Unfortunately no :( because for the application I'm writting, I really need
to be able to delete groups of records in one delete.

 Best regards,
 -F.

PS: I did some change for that new patch version that I did not have time
    to test yet.
-------------- next part --------------
Index: Collection.pm
===================================================================
--- Collection.pm	(revision 2030)
+++ Collection.pm	(working copy)
@@ -1605,6 +1605,122 @@
     );
 }
 
+=head2 delete_records
+
+Delete this collection's records from the database. On failure return a
+Class::ReturnValue with the error. On success, return 1;
+
+This is an error to try to delete a joined Collection.
+
+This method has two hooks
+
+=over
+
+=item before_delete
+
+This method is called before the records deletion, if it exists. On
+failure it returns a L<Class::ReturnValue> with the error.  On success
+it returns 1.
+
+If this method returns an error, it causes the records deletion to abort and
+return the return value from this hook.
+
+=item after_delete
+
+This method is called after deletion, with a reference to the return
+value from the delete operation.
+
+=back
+
+=cut
+
+sub delete_records {
+    my $self = shift;
+    if ( $self->can('before_delete') ) {
+        my $before_ret = $self->before_delete();
+        return $before_ret unless ($before_ret);
+    }
+    my $ret = $self->__delete_records();
+    $self->after_delete( \$ret ) if $self->can('after_delete');
+    return ($ret);
+}
+
+sub __delete_records {
+    my $self = shift;
+
+    # Collection must not be a joined Collection:
+    if ($self->_is_joined) {
+        my $ret = Class::ReturnValue->new();
+        my $message = "Records from joined Collection cannot be deleted";
+        $ret->as_array( 0, $message );
+        $ret->as_error(
+                       errno        => 6,
+                       do_backtrace => 0,
+                       message      => $message
+        );
+        return $ret;
+    }
+
+    # Get the where clause.
+    my $where = $self->_bulk_where_clause();
+    unless (defined $where) {
+        # We have nothing to delete, but this is not an error:
+        return (1);
+    }
+
+    my $query_string = "DELETE FROM " . $self->table . $where;
+    my $return       = $self->_handle->simple_query( $query_string );
+
+    $self->clean_slate(); # This collection cannot be used anymore: clear all.
+
+    if ( UNIVERSAL::isa( 'Class::ReturnValue', $return ) ) {
+        return ($return);
+    } else {
+        return (1);
+    }
+}
+
+# Constructs a where clause suitable for bulk operation (update, delete)
+# on the Collection object.
+# It return either the where clause (an empty string if the collection
+# represent the whole table content, or a string starting with 'WHERE') or
+# undef if the collection is empty.
+sub _bulk_where_clause {
+    my $self = shift;
+
+    my $where = '';
+    if ($self->{'must_redo_search'}) {
+        # if the 'must_redo_search' flag is set, that means that no call
+        # to the add_record() method has occured yet, so we can use
+        # the restrictions to build the where clause if they exist:
+        $where = $self->_where_clause if $self->_is_limited;
+    } else {
+        # otherwise, we build the where clause using the items stored
+        # in the collection:
+        if (scalar @{$self->{items}} > 0) {
+            my $record_class = $self->record_class;
+            die "Jifty::DBI::Collection needs to be subclassed; " .
+                "override new_item\n"
+                                     unless $record_class;
+            my $pkey = $record_class->new()->primary_keys()->[0];
+ 
+            $where = $self->_where_clause if $self->_is_limited;
+            if ($where =~ m/^ WHERE/i) {
+                $where .= ' AND ';
+            } else {
+                $where = ' WHERE ';
+            }
+            $where .= $pkey . ' IN(';
+            my @list = map { $_->id; } @{$self->{items}};
+            $where .= CORE::join(', ', @list); 
+            $where .= ')';
+        } else { # The collection is empty
+            return undef;
+        }
+    }
+    return $where;
+}
+
 1;
 __END__