Fix another ::FilterColumn bug sigh...

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 8ce62d6..2fc18e9 100644 (file)
--- a/Changes
+++ b/Changes
@@ -11,6 +11,8 @@ Revision history for DBIx::Class
           an underlying search_rs(), as by design these arguments would be
           used only on the first call to ->related_resultset(), and ignored
           afterwards. Instead an exception (detailing the fix) is thrown.
+        - Another relatively invasive set of ::FilterColumn changes, covering
+          potential data loss (RT#111567). Please run your regression tests!
         - Increased checking for the correctness of the is_nullable attribute
           within the prefetch result parser may highlight previously unknown
           mismatches between your codebase and data source

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 af7a951..cf7e245 100644 (file)
--- a/t/row/filter_column.t
+++ b/t/row/filter_column.t
@@ -132,6 +132,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
@@ -139,6 +205,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;
@@ -152,6 +219,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/dist/pod_coverage.t b/xt/dist/pod_coverage.t
index a3acbe4..97d4975 100644 (file)
--- a/xt/dist/pod_coverage.t
+++ b/xt/dist/pod_coverage.t
@@ -58,6 +58,7 @@ my $exceptions = {
             store_column
             get_column
             get_columns
+            get_dirty_columns
             has_column_loaded
         /],
     },