From: Peter Rabbitson Date: Sat, 2 Apr 2016 15:41:52 +0000 (+0200) Subject: Fix another ::FilterColumn bug sigh... X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=b482a0958732d987d8f742c7bdd61816a898f083;p=dbsrgits%2FDBIx-Class-Historic.git 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 --- 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 --- 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 --- 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 --- 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 /], },