Fix another ::FilterColumn bug sigh...

( cherry-pick of b482a0958 )

This one is technically a regression introduced by dc6dadae, which aimed to
solve multiple runs on already-dirty columns. Unfortunately this very same
change broke update(). Overload get_dirty_columns to fix that, and add a
bunch of tests validating nothing crazy is going on.

I should have seen all of these problems when FC was initially considered,
but alas I was too damn inexperienced :/

diff --git a/Changes b/Changes
index 85d3051..b6b1a1b 100644 (file)
--- a/Changes
+++ b/Changes
@@ -1,6 +1,8 @@
 Revision history for DBIx::Class
 
     * Fixes
+        - Another relatively invasive set of ::FilterColumn changes, covering
+          potential data loss (RT#111567). Please run your regression tests!
         - Fix use of ::Schema::Versioned combined with a user-supplied
           $dbh->{HandleError} (GH#101)
         - Fix parsing of DSNs containing driver arguments (GH#99)

diff --git a/lib/DBIx/Class/FilterColumn.pm b/lib/DBIx/Class/FilterColumn.pm
index fedbf79..b7860c9 100644 (file)
--- a/lib/DBIx/Class/FilterColumn.pm
+++ b/lib/DBIx/Class/FilterColumn.pm
@@ -78,11 +78,13 @@ sub get_filtered_column {
 sub get_column {
   my ($self, $col) = @_;
 
-  if (exists $self->{_filtered_column}{$col}) {
-    return $self->{_column_data}{$col} ||= $self->_column_to_storage (
-      $col, $self->{_filtered_column}{$col}
-    );
-  }
+  ! exists $self->{_column_data}{$col}
+    and
+  exists $self->{_filtered_column}{$col}
+    and
+  $self->{_column_data}{$col} = $self->_column_to_storage (
+    $col, $self->{_filtered_column}{$col}
+  );
 
   return $self->next::method ($col);
 }
@@ -101,6 +103,22 @@ sub get_columns {
   $self->next::method (@_);
 }
 
+# and *another* separate codepath, argh!
+sub get_dirty_columns {
+  my $self = shift;
+
+  $self->{_dirty_columns}{$_}
+    and
+  ! exists $self->{_column_data}{$_}
+    and
+  $self->{_column_data}{$_} = $self->_column_to_storage (
+    $_, $self->{_filtered_column}{$_}
+  )
+    for keys %{$self->{_filtered_column}||{}};
+
+  $self->next::method(@_);
+}
+
 sub store_column {
   my ($self, $col) = (shift, @_);
 

diff --git a/t/row/filter_column.t b/t/row/filter_column.t
index 7823fa5..ecbb262 100644 (file)
--- a/t/row/filter_column.t
+++ b/t/row/filter_column.t
@@ -130,6 +130,72 @@ CACHE_TEST: {
   is $from_storage_ran, ++$expected_from, 'from did run';
   is $to_storage_ran, $expected_to,  'to did not run';
 
+  ok ! $artist->is_changed, 'object clean';
+  is_deeply
+    { $artist->get_dirty_columns },
+    {},
+    'dirty columns as expected',
+  ;
+
+  is $from_storage_ran, $expected_from, 'from did not run';
+  is $to_storage_ran, $expected_to,  'to did not run';
+
+  $artist->charfield(42);
+
+  is $from_storage_ran, $expected_from, 'from did not run';
+  is $to_storage_ran, ++$expected_to,  'to ran once, determining dirtyness';
+
+  is $artist->charfield, 42, 'setting once works';
+  ok $artist->is_column_changed('charfield'), 'column changed';
+  ok $artist->is_changed, 'object changed';
+  is_deeply
+    { $artist->get_dirty_columns },
+    { charfield => 21 },
+    'dirty columns as expected',
+  ;
+
+  is $from_storage_ran, $expected_from, 'from did not run';
+  is $to_storage_ran, $expected_to,  'to did not run';
+
+  $artist->charfield(66);
+  is $artist->charfield, 66, 'setting twice works';
+  ok $artist->is_column_changed('charfield'), 'column changed';
+  ok $artist->is_changed, 'object changed';
+
+  is $from_storage_ran, $expected_from, 'from did not run';
+  is $to_storage_ran, $expected_to,  'to did not run a second time on dirty column';
+
+  is_deeply
+    { $artist->get_dirty_columns },
+    { charfield => 33 },
+    'dirty columns as expected',
+  ;
+  is $from_storage_ran, $expected_from, 'from did not run';
+  is $to_storage_ran, ++$expected_to,  'to did run producing a new dirty_columns set';
+
+  is_deeply
+    { $artist->get_dirty_columns },
+    { charfield => 33 },
+    'dirty columns still as expected',
+  ;
+  is $from_storage_ran, $expected_from, 'from did not run';
+  is $to_storage_ran, $expected_to,  'to did not run on re-invoked get_dirty_columns';
+
+  $artist->update;
+  is $artist->charfield, 66, 'value still there';
+
+  is $from_storage_ran, $expected_from, 'from did not run';
+  is $to_storage_ran, $expected_to, 'to did not run ';
+
+  $artist->discard_changes;
+
+  is $from_storage_ran, $expected_from, 'from did not run after discard_changes';
+  is $to_storage_ran, $expected_to, 'to did not run after discard_changes';
+
+  is $artist->charfield, 66, 'value still there post reload';
+
+  is $from_storage_ran, ++$expected_from, 'from did run';
+  is $to_storage_ran, $expected_to,  'to did not run';
 }
 
 # test in-memory operations
@@ -137,6 +203,7 @@ for my $artist_maker (
   sub { $schema->resultset('Artist')->new({ charfield => 42 }) },
   sub { my $art = $schema->resultset('Artist')->new({}); $art->charfield(42); $art },
 ) {
+  $schema->resultset('Artist')->delete;
 
   my $expected_from = $from_storage_ran;
   my $expected_to   = $to_storage_ran;
@@ -150,6 +217,14 @@ for my $artist_maker (
   ok( $artist->has_column_loaded('charfield'), 'Filtered column marked as loaded under new' );
   is( $artist->charfield, 42, 'Proper unfiltered value' );
   is( $artist->get_column('charfield'), 21, 'Proper filtered value' );
+
+  $artist->insert;
+  ($raw_db_charfield) = $schema->resultset('Artist')
+                                ->search ($artist->ident_condition)
+                                 ->get_column('charfield')
+                                  ->next;
+
+  is $raw_db_charfield, 21, 'Proper value in database';
 }
 
 # test literals

diff --git a/xt/podcoverage.t b/xt/podcoverage.t
index a16a365..3ef6f56 100644 (file)
--- a/xt/podcoverage.t
+++ b/xt/podcoverage.t
@@ -57,6 +57,7 @@ my $exceptions = {
             store_column
             get_column
             get_columns
+            get_dirty_columns
             has_column_loaded
         /],
     },