Re: 0.97!

[prev] [thread] [next] [Date index for 2004/04/30]

--On Friday, April 30, 2004 7:35 AM +0100 Tony Bowden 
<tony-cdbitalk@xxxxx.xxx> wrote:

> On Fri, Apr 30, 2004 at 07:27:37AM +0100, Tony Bowden wrote:
>> 0.97 is out (at last)!
>> You can pick it up from CPAN, or directly from
>>   http://www.tmtm.com/CPAN/Class-DBI-0.97.tar.gz

Thanks for all the work that went into this!

As the owner of the one reported problem, let me start the patching process 
towards 0.97:

Problem:  Say object A maintains a has_a relationship to object B via A's 
attribute B_link.  When A is created or updated, B_link is deflated to a 
SQL-compatible value (usually a string).  If the SQL fails, or if B_link is 
part of A's key, then it's never reinflated to point to the real B, so 
future attempts to treat A->B_link as an an object will blow up.

Fix: Reinflate the links.

Attached is a patch which accomplishes this via an 'inflate' trigger point 
symmetric with the 'deflate_for_*' trigger points.  This allows the 
application to safely continue from a SQL error in an insert or update (in 
my case, I was checking for a duplicate key error and handling it).  As a 
byproduct, it's now OK PK columns to have has_a relationships; the only 
limitation is that the target object must deflate/stringify to the SQL 
value of the key.

The patch adds three tests to 10-mysql.t.  The first passes in 0.97.  The 
second fails, but would be corrected by using an 'after_create' trigger in 
HasA.pm; correcting this would allow has_as across PK columns but not fix 
continuation from errors.  The third passes with the use of the 'inflate' 
trigger only.

The patch also modifies the existing private method _insert_row() slightly, 
in that it may now throw an exception.  Since it isn't called from anywhere 
but _create(), it'd arguably be a bit more efficient to inline it, but 
others might be using it somewhere.

I hope this is a useful starting point for discussion; as Mr. Bowden has 
mentioned, the details of these relationships will no doubt change as the 
attributes of a CDBI object get smarter.

--
Regards,
Charles Bailey  < bailey _at_ newman _dot_ upenn _dot_ edu >
Newman Center at the University of Pennsylvania

diff -Bbur Class-DBI-0.96/lib/Class/DBI/Relationship/HasA.pm 
Class-DBI-0.96_patched/lib/Class/DBI/Relationship/HasA.pm
--- Class-DBI-0.96/lib/Class/DBI/Relationship/HasA.pm	Sun Apr 25 11:33:36 
2004
+++ Class-DBI-0.96_patched/lib/Class/DBI/Relationship/HasA.pm	Fri Apr 30 
12:09:16 2004
@@ -24,6 +24,7 @@
 	return (
 		select              => $self->_inflator,
 		"after_set_$column" => $self->_inflator,
+		inflate             => $self->_inflator,
 		deflate_for_create  => $self->_deflator(1),
 		deflate_for_update  => $self->_deflator,
 	);
diff -Bbur Class-DBI-0.96/lib/Class/DBI.pm 
Class-DBI-0.96_patched/lib/Class/DBI.pm
--- Class-DBI-0.96/lib/Class/DBI.pm	Fri Apr 30 03:22:12 2004
+++ Class-DBI-0.96_patched/lib/Class/DBI.pm	Fri Apr 30 12:08:19 2004
@@ -587,7 +587,18 @@
 		($class->has_real_column($col) ? $real : $temp)->{$col} =
 			$self->_attrs($col);
 	}
-	$self->_insert_row($real);
+	eval { $self->_insert_row($real); };
+	my $err = $@;
+	# Restore linked objects even if insert failed
+	$self->call_trigger('inflate');
+	if ($err) {
+		my $class = ref $self;
+		return $self->_croak(
+			"Can't insert new $class: $err",
+			err    => $err,
+			method => 'create'
+		);
+	}

 	my @primary_columns = $class->primary_columns;
 	$self->_attribute_store(
@@ -626,7 +637,6 @@
 sub _insert_row {
 	my $self = shift;
 	my $data = shift;
-	eval {
 		my @columns = keys %$data;
 		my $sth     = $self->sql_MakeNewObj(
 			join(', ', @columns),
@@ -638,15 +648,6 @@
 		$data->{ $primary_columns[0] } = $self->_auto_increment_value
 			if @primary_columns == 1
 			&& !defined $data->{ $primary_columns[0] };
-	};
-	if ($@) {
-		my $class = ref $self;
-		return $self->_croak(
-			"Can't insert new $class: $@",
-			err    => $@,
-			method => 'create'
-		);
-	}
 	return 1;
 }

@@ -776,11 +777,14 @@
 	$self->call_trigger('before_update');
 	return 1 unless my @changed_cols = $self->is_changed;
 	$self->call_trigger('deflate_for_update');
-	my @primary_columns = $self->primary_columns;
 	my $sth             = $self->sql_update($self->_update_line);
 	$class->_bind_param($sth, \@changed_cols);
 	my $rows = eval { $sth->execute($self->_update_vals, $self->id); };
-	return $self->_croak("Can't update $self: $@", err => $@) if $@;
+	my $err = $@;
+	# Restore linked objects even if SQL update fails
+	$self->call_trigger('inflate');
+	return $self->_croak("Can't update $self: $err",
+			     err => $err, method => 'update') if $err;

 	# enable this once new fixed DBD::SQLite is released:
 	if (0 and $rows != 1) {    # should always only update one row
diff -Bbur Class-DBI-0.96/t/10-mysql.t Class-DBI-0.96_patched/t/10-mysql.t
--- Class-DBI-0.96/t/10-mysql.t	Sun Apr 25 11:33:36 2004
+++ Class-DBI-0.96_patched/t/10-mysql.t	Fri Apr 30 11:03:33 2004
@@ -9,7 +9,7 @@
 eval { require 't/testlib/MyFoo.pm' };
 plan skip_all => "Need MySQL for this test" if $@;

-plan tests => 64;
+plan tests => 67;

 package main;

@@ -51,6 +51,7 @@
 ok(my $l2 = MyStarLink->create({ film => $f1, star => $s2 }), "Link 2");
 ok(my $l3 = MyStarLink->create({ film => $f2, star => $s1 }), "Link 3");
 ok(my $l4 = MyStarLink->create({ film => $f2, star => $s3 }), "Link 4");
+isa_ok($l4->film, 'MyFilm', 'has_a target is an object');

 ok(my $lm1 = MyStarLinkMCPK->create({ film => $f1, star => $s1 }),
 	"Link MCPK 1");
@@ -60,6 +61,7 @@
 	"Link MCPK 3");
 ok(my $lm4 = MyStarLinkMCPK->create({ film => $f2, star => $s3 }),
 	"Link MCPK 4");
+isa_ok($lm4->film, 'MyFilm', 'has_a target is an object');

 {    # Warnings for scalar context?
 	my $err = "";
@@ -76,6 +78,7 @@
 my $lm5 = eval { MyStarLinkMCPK->create({ film => $f2, star => $s3 }) };
 ok(!$lm5, "Can't create duplicate");
 ok($@ =~ /^Can't insert .* duplicate/i, "Duplicate create caused 
exception");
+isa_ok($lm4->film, 'MyFilm', 'has_a target is still an object');

 # create one to delete
 ok(my $lm6 = MyStarLinkMCPK->create({ film => $f2, star => $s2 }),