Issue 015/serializing postgres arrays #1018

hubertpompecki · 2017-12-04T16:44:37Z

This PR addresses #1015

Starting from Rails version 5.0.2 the default serializer of PostgreSQL columns returns an ActiveRecord::ConnectionAdapters::PostgreSQL::OID::Array object instead of a string. This new object is not suitable for JSON encoding and breaks versioning of any array fields backed by Postgres.

Whenever a PostgreSQL array is used, instead of asking Active Record for a serializer we introduce our own, which simply returns the underlying array without any modifications.

jaredbeck · 2017-12-04T16:59:58Z

lib/paper_trail/attribute_serializers/array_serializer.rb

+module PaperTrail
+  module AttributeSerializers
+    # :nodoc:
+    class ArraySerializer


I really like the OO design you've gone with. It gives us some parts that we can understand independently.

Regarding the name, the intention of the AttributeSerializers module is to "serialize" two attributes:

object

object_changes

There is no attribute named array, so I think this is either:

not the correct module to put this class in, or

the module should be given a more generic name

My first instinct is the former, that this is not the correct module to put this class in. What do you think?

Good point and I'd also lean towards no. 1.

Given that the original implementation obtains an AR serializer by calling #type_for_attribute(attr) then perhaps PaperTrail::TypeSerializers::PostgresArraySerializer would be a better name?

jaredbeck · 2017-12-04T17:00:18Z

lib/paper_trail/attribute_serializers/array_serializer.rb

@@ -0,0 +1,28 @@
+module PaperTrail
+  module AttributeSerializers
+    # :nodoc:


Please provide documentation for all new classes.

jaredbeck · 2017-12-04T17:04:09Z

spec/dummy_app/db/migrate/20110208155312_set_up_test_tables.rb

+      create_table :postgres_users, force: true do |t|
+        t.string     :name
+        t.integer    :post_ids,    array: true
+        t.datetime   :login_times, array: true


I like how you are testing two different types of arrays here. Also, it looks like this is the first test table to have arrays at all! Kind of surprising.

jaredbeck · 2017-12-04T17:05:09Z

spec/paper_trail/attribute_serializers/object_attribute_spec.rb

@@ -0,0 +1,37 @@
+require "spec_helper"
+
+RSpec.describe PaperTrail::AttributeSerializers::ObjectAttribute do


Please use the nested-module style.

module PaperTrail module AttributeSerializers RSpec.describe ObjectAttribute

jaredbeck · 2017-12-04T17:07:11Z

spec/spec_helper.rb

@@ -2,6 +2,7 @@
 ENV["DB"] ||= "sqlite"

 require "byebug"
+require "support/database_specific_specs"


Please create a separate PR for this. It's a good idea, but I'd like to handle it separately, and more comprehensively. For this PR, please use plain ruby conditionals the way other tests currently do.

jaredbeck · 2017-12-04T17:09:49Z

lib/paper_trail/attribute_serializers/attribute_serializer.rb

+    # with custom PaperTrail ones.
+    module AttributeSerializer
+      def self.for(klass, attr)
+        case active_record_serializer = klass.type_for_attribute(attr)


Please do not use the assignment operator in a case.

jaredbeck · 2017-12-04T17:11:01Z

lib/paper_trail/attribute_serializers/attribute_serializer.rb

+            active_record_serializer.subtype,
+            active_record_serializer.delimiter
+          )
+        else active_record_serializer


Please move this onto its own line:

else active_record_serializer end

jaredbeck · 2017-12-04T17:13:38Z

lib/paper_trail/attribute_serializers/attribute_serializer.rb

+    # not suited for JSON encoding. This factory
+    # replaces certain default Active Record serializers
+    # with custom PaperTrail ones.
+    module AttributeSerializer


I see what you were going for with this name. AttributeSerializer.for does read nicely, but I'd think I'd prefer AttributeSerializerFactory.for. It doesn't read as well, but you can tell the purpose of the module from its name alone.

jaredbeck · 2017-12-04T17:15:17Z

lib/paper_trail/attribute_serializers/attribute_serializer.rb

+    # Values returned by some Active Record serializers are
+    # not suited for JSON encoding. This factory
+    # replaces certain default Active Record serializers
+    # with custom PaperTrail ones.


A good, helpful comment. I'd change "JSON encoding" to "writing JSON to a text column.".

This phrasing disabiguates JSON-in-text-column from native postgres json(b).

jaredbeck · 2017-12-04T17:18:40Z

lib/paper_trail/attribute_serializers/cast_attribute_serializer.rb

@@ -32,15 +34,15 @@ def defined_enums
      # This implementation uses AR 5's `serialize` and `deserialize`.
      class CastAttributeSerializer
        def serialize(attr, val)
-          @klass.type_for_attribute(attr).serialize(val)
+          PaperTrail::AttributeSerializers::AttributeSerializer.for(@klass, attr).serialize(val)


It's not necessary to fully-qualify the constant here. AttributeSerializer alone is sufficient.

hubertpompecki

Thanks for the comprehensive code review @jaredbeck! A lot of good points there, I have made changes as requested.

jaredbeck · 2017-12-08T16:50:35Z

Thanks for your work on this Hubert!

This PR addresses paper-trail-gem#1015 Starting from Rails version 5.0.2 the default serializer of PostgreSQL columns returns an ActiveRecord::ConnectionAdapters::PostgreSQL::OID::Array object instead of a string. This new object is not suitable for JSON encoding and breaks versioning of any array fields backed by Postgres. Whenever a PostgreSQL array is used, instead of asking Active Record for a serializer we introduce our own, which simply returns the underlying array without any modifications.

hubertpompecki and others added 10 commits December 4, 2017 10:45

Serialize a postgres into a plain array

aae7d93

Add specs for deserializing arrays

fdb7f29

Rubocop

c784f20

Add comments

39bda99

Clean up

8bec622

Add byebug log to gitignore

764eae7

Add support for nested postgres subtypes

bf8e7d8

Merge branch 'master' into issue_015/serializing_postgres_arrays

e9b25c6

Merge branch 'master' into issue_015/serializing_postgres_arrays

99dc9af

Fix specs for postgres and ar < 5.0.2

dd7f239

jaredbeck requested changes Dec 4, 2017

View reviewed changes

Review comments, update specs

eb368b4

hubertpompecki commented Dec 5, 2017

View reviewed changes

jaredbeck approved these changes Dec 8, 2017

View reviewed changes

jaredbeck merged commit 4cce9b0 into paper-trail-gem:master Dec 8, 2017

jaredbeck mentioned this pull request Dec 8, 2017

PaperTrail::Serializers::JSON not compatible with Postgres arrays in Rails >= 5.0.2 #1015

Closed

jaredbeck mentioned this pull request Dec 10, 2017

Fix NameError when pg is not loaded #1023

Merged

hubertpompecki deleted the issue_015/serializing_postgres_arrays branch December 14, 2017 15:08

dependencies bot mentioned this pull request Dec 25, 2017

Update paper_trail in / from 8.0.1 to 8.1.2 podemos-info/census#116

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 015/serializing postgres arrays #1018

Issue 015/serializing postgres arrays #1018

hubertpompecki commented Dec 4, 2017 •

edited

Loading

jaredbeck Dec 4, 2017

hubertpompecki Dec 5, 2017

jaredbeck Dec 4, 2017

jaredbeck Dec 4, 2017

jaredbeck Dec 4, 2017

jaredbeck Dec 4, 2017

jaredbeck Dec 4, 2017

jaredbeck Dec 4, 2017

jaredbeck Dec 4, 2017

jaredbeck Dec 4, 2017

jaredbeck Dec 4, 2017

hubertpompecki left a comment •

edited

Loading

jaredbeck commented Dec 8, 2017

		@@ -0,0 +1,37 @@
		require "spec_helper"

		RSpec.describe PaperTrail::AttributeSerializers::ObjectAttribute do

Issue 015/serializing postgres arrays #1018

Issue 015/serializing postgres arrays #1018

Conversation

hubertpompecki commented Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hubertpompecki left a comment • edited Loading

Choose a reason for hiding this comment

jaredbeck commented Dec 8, 2017

hubertpompecki commented Dec 4, 2017 •

edited

Loading

hubertpompecki left a comment •

edited

Loading