Make sure raw RABX column is utf8-encoded.

Without doing this, a call to e.g. $contact->set_extra_fields(@meta)
in PopulateServiceList.pm, with an unchanged meta that contains some
Unicode values, can write to the database (and cause an unneeded row
in the history table), because the column from the database is UTF-8
decoded, whilst the new text is UTF-8 encoded.

It looks like an attempt was made in filter_from_storage to fix this
issue, but the column comparison for marking a column as dirty takes
place without this being called.

3 files changed, 61 insertions, 4 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 9b67c782f..595768272 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -28,6 +28,7 @@
         - Add space below "map page" contents on narrow screens.
         - Use relative report links where possible. #1995
         - Improve inline checkbox spacing. #2411
+        - Prevent duplicate contact history creation with Unicode data.
     - Development improvements:
         - Make front page cache time configurable.
         - Better working of /fakemapit/ under https.
diff --git a/perllib/FixMyStreet/DB/RABXColumn.pm b/perllib/FixMyStreet/DB/RABXColumn.pm
index 5f1583018..d14b48dc8 100644
--- a/perllib/FixMyStreet/DB/RABXColumn.pm
+++ b/perllib/FixMyStreet/DB/RABXColumn.pm
@@ -59,7 +59,6 @@ sub rabx_column {
                 my $self = shift;
                 my $ser  = shift;
                 return undef unless defined $ser;
-                utf8::encode($ser) if utf8::is_utf8($ser);
                 my $h = new IO::String($ser);
                 return RABX::wire_rd($h);
             },
@@ -78,18 +77,23 @@ sub rabx_column {
     $RABX_COLUMNS{ _get_class_identifier($class) }{$col} = 1;
 }
 
+# The underlying column should always be UTF-8 encoded bytes.
+sub get_column {
+    my ($self, $col) = @_;
+    my $res = $self->next::method ($col);
+    utf8::encode($res) if $RABX_COLUMNS{_get_class_identifier($self)}{$col} && utf8::is_utf8($res);
+    return $res;
+}
 
 sub set_filtered_column {
     my ($self, $col, $val) = @_;
 
-    my $class = ref $self;
-
     # because filtered objects may be expensive to marshall for storage there
     # is a cache that attempts to detect if they have changed or not. For us
     # this cache breaks things and our marshalling is cheap, so clear it when
     # trying set a column.
     delete $self->{_filtered_column}{$col}
-        if $RABX_COLUMNS{ _get_class_identifier($class) }{$col};
+        if $RABX_COLUMNS{ _get_class_identifier($self) }{$col};
 
     return $self->next::method($col, $val);
 }
diff --git a/t/open311/populate-service-list.t b/t/open311/populate-service-list.t
index 9747dfd2f..4d70dfebc 100644
--- a/t/open311/populate-service-list.t
+++ b/t/open311/populate-service-list.t
@@ -19,6 +19,7 @@ package main;
 
 use FixMyStreet::Test;
 use FixMyStreet::DB;
+use utf8;
 
 use_ok( 'Open311::PopulateServiceList' );
 use_ok( 'Open311' );
@@ -297,6 +298,47 @@ for my $test (
         ',
     },
     {
+        desc => 'check meta data unchanging',
+        has_meta => 1,
+        has_no_history => 1,
+        orig_meta => [ {
+            variable => 'true',
+            code => 'type',
+            datatype => 'string',
+            required => 'true',
+            datatype_description => 'Colour of bin',
+            order => 1,
+            description => 'Cólour of bin'
+
+        } ],
+        end_meta => [ {
+            variable => 'true',
+            code => 'type',
+            datatype => 'string',
+            required => 'true',
+            datatype_description => 'Colour of bin',
+            order => 1,
+            description => 'Cólour of bin'
+
+        } ],
+        meta_xml => '<?xml version="1.0" encoding="utf-8"?>
+    <service_definition>
+        <service_code>100</service_code>
+        <attributes>
+            <attribute>
+                <variable>true</variable>
+                <code>type</code>
+                <datatype>string</datatype>
+                <required>true</required>
+                <datatype_description>Colour of bin</datatype_description>
+                <order>1</order>
+                <description>Cólour of bin</description>
+            </attribute>
+        </attributes>
+    </service_definition>
+        ',
+    },
+    {
         desc => 'check meta data removed',
         has_meta => 0,
         end_meta => [],
@@ -389,11 +431,21 @@ for my $test (
         $processor->_current_open311( $o );
         $processor->_current_body( $body );
 
+        my $count = FixMyStreet::DB->resultset('ContactsHistory')->search({
+            contact_id => $contact->id,
+        })->count;
+
         $processor->process_services( $service_list );
 
         $contact->discard_changes;
 
         is_deeply $contact->get_extra_fields, $test->{end_meta}, 'meta data saved';
+
+        if ($test->{has_no_history}) {
+            is +FixMyStreet::DB->resultset('ContactsHistory')->search({
+                contact_id => $contact->id,
+            })->count, $count, 'No new history';
+        }
     };
 }