From 2849c758ed7764c44b6ab035beaba52f162d4a34 Mon Sep 17 00:00:00 2001 From: Michael Raimondi Date: Mon, 6 Apr 2015 13:06:14 -0400 Subject: [PATCH 1/9] Use Postgres containment operator where available --- lib/paper_trail/version_concern.rb | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/lib/paper_trail/version_concern.rb b/lib/paper_trail/version_concern.rb index 32a1292cd..528e879af 100644 --- a/lib/paper_trail/version_concern.rb +++ b/lib/paper_trail/version_concern.rb @@ -86,12 +86,17 @@ def timestamp_sort_order(direction = 'asc') # identically-named method in the serializer being used. def where_object(args = {}) raise ArgumentError, 'expected to receive a Hash' unless args.is_a?(Hash) - arel_field = arel_table[:object] - where_conditions = args.map do |field, value| - PaperTrail.serializer.where_object_condition(arel_field, field, value) - end.reduce do |condition1, condition2| - condition1.and(condition2) + if object_col_is_json? + where_conditions = "object @> '#{args.to_json}'::#{columns_hash['object'].type}" + else + arel_field = arel_table[:object] + + where_conditions = args.map do |field, value| + PaperTrail.serializer.where_object_condition(arel_field, field, value) + end.reduce do |condition1, condition2| + condition1.and(condition2) + end end where(where_conditions) @@ -99,12 +104,17 @@ def where_object(args = {}) def where_object_changes(args = {}) raise ArgumentError, 'expected to receive a Hash' unless args.is_a?(Hash) - arel_field = arel_table[:object_changes] - where_conditions = args.map do |field, value| - PaperTrail.serializer.where_object_changes_condition(arel_field, field, value) - end.reduce do |condition1, condition2| - condition1.and(condition2) + if object_changes_col_is_json? + where_conditions = "object_changes @> '#{args.to_json}'::#{columns_hash['object_changes'].try(:type)}" + else + arel_field = arel_table[:object_changes] + + where_conditions = args.map do |field, value| + PaperTrail.serializer.where_object_changes_condition(arel_field, field, value) + end.reduce do |condition1, condition2| + condition1.and(condition2) + end end where(where_conditions) From dffce4dd3e394114c6eea44e6fbf5b5f0296a9cf Mon Sep 17 00:00:00 2001 From: Michael Raimondi Date: Thu, 9 Apr 2015 12:32:30 -0400 Subject: [PATCH 2/9] Update YAML engine check for new Ruby versions On Ruby 2.2.1, '::YAML' returns the class Psych, which has no ::ENGINE constant defined. --- lib/paper_trail/serializers/yaml.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/paper_trail/serializers/yaml.rb b/lib/paper_trail/serializers/yaml.rb index d4a02d42e..fe59edf8d 100644 --- a/lib/paper_trail/serializers/yaml.rb +++ b/lib/paper_trail/serializers/yaml.rb @@ -23,7 +23,8 @@ def where_object_condition(arel_field, field, value) # in the serialized object_changes def where_object_changes_condition(arel_field, field, value) # Need to check first (before) and secondary (after) fields - if defined?(::YAML::ENGINE) && ::YAML::ENGINE.yamler == 'psych' + if (defined?(::YAML::ENGINE) && ::YAML::ENGINE.yamler == 'psych') || + (defined?(::YAML) && defined?(Psych) && ::YAML == Psych) arel_field.matches("%\n#{field}:\n- #{value}\n%"). or(arel_field.matches("%\n#{field}:\n-%\n- #{value}\n%")) else # Syck adds extra spaces into array dumps From 28183d26c71161a08169ce96b2f3422bba3b6dc1 Mon Sep 17 00:00:00 2001 From: Michael Raimondi Date: Thu, 9 Apr 2015 13:26:24 -0400 Subject: [PATCH 3/9] Do not memoize column type checks Column types can (rarely) change dynamically as an application runs. We should check the type of a column on every query. --- lib/paper_trail/version_concern.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/paper_trail/version_concern.rb b/lib/paper_trail/version_concern.rb index 528e879af..53abaaff2 100644 --- a/lib/paper_trail/version_concern.rb +++ b/lib/paper_trail/version_concern.rb @@ -128,12 +128,12 @@ def primary_key_is_int? # Returns whether the `object` column is using the `json` type supported by PostgreSQL def object_col_is_json? - @object_col_is_json ||= [:json, :jsonb].include?(columns_hash['object'].type) + [:json, :jsonb].include?(columns_hash['object'].type) end # Returns whether the `object_changes` column is using the `json` type supported by PostgreSQL def object_changes_col_is_json? - @object_changes_col_is_json ||= [:json, :jsonb].include?(columns_hash['object_changes'].try(:type)) + [:json, :jsonb].include?(columns_hash['object_changes'].try(:type)) end end From ab4129adcc0e0c292d16dc0b4e236699046c4e5d Mon Sep 17 00:00:00 2001 From: Michael Raimondi Date: Thu, 9 Apr 2015 13:48:35 -0400 Subject: [PATCH 4/9] Vary column type for tests if DB is postgres --- spec/models/version_spec.rb | 84 +++++++++++++++++++++++-------------- 1 file changed, 53 insertions(+), 31 deletions(-) diff --git a/spec/models/version_spec.rb b/spec/models/version_spec.rb index 3881e0975..199a602fa 100644 --- a/spec/models/version_spec.rb +++ b/spec/models/version_spec.rb @@ -65,46 +65,68 @@ end describe "Class" do - describe '#where_object' do - it { expect(PaperTrail::Version).to respond_to(:where_object) } - - context "invalid arguments" do - it "should raise an error" do - expect { PaperTrail::Version.where_object(:foo) }.to raise_error(ArgumentError) - expect { PaperTrail::Version.where_object([]) }.to raise_error(ArgumentError) - end - end - - context "valid arguments", :versioning => true do - let(:widget) { Widget.new } - let(:name) { Faker::Name.first_name } - let(:int) { rand(10) + 1 } + column_overrides = [false] + column_overrides.concat(['jsonb']) if ENV['DB'] == 'postgres' + column_overrides.each do |override| + context "with a #{override || 'text'} column" do before do - widget.update_attributes!(:name => name, :an_integer => int) - widget.update_attributes!(:name => 'foobar', :an_integer => 100) - widget.update_attributes!(:name => Faker::Name.last_name, :an_integer => 15) + if override + ActiveRecord::Base.connection.execute("SAVEPOINT pgtest;") + ActiveRecord::Base.connection.execute("ALTER TABLE versions DROP COLUMN object;") + ActiveRecord::Base.connection.execute("ALTER TABLE versions ADD COLUMN object #{override};") + PaperTrail::Version.reset_column_information + end end - - context "`serializer == YAML`" do - specify { expect(PaperTrail.serializer).to be PaperTrail::Serializers::YAML } - - it "should be able to locate versions according to their `object` contents" do - expect(PaperTrail::Version.where_object(:name => name)).to eq([widget.versions[1]]) - expect(PaperTrail::Version.where_object(:an_integer => 100)).to eq([widget.versions[2]]) + after do + if override + ActiveRecord::Base.connection.execute("ROLLBACK TO SAVEPOINT pgtest;") + PaperTrail::Version.reset_column_information end end - context "`serializer == JSON`" do - before(:all) { PaperTrail.serializer = PaperTrail::Serializers::JSON } - specify { expect(PaperTrail.serializer).to be PaperTrail::Serializers::JSON } + describe '#where_object' do + it { expect(PaperTrail::Version).to respond_to(:where_object) } - it "should be able to locate versions according to their `object` contents" do - expect(PaperTrail::Version.where_object(:name => name)).to eq([widget.versions[1]]) - expect(PaperTrail::Version.where_object(:an_integer => 100)).to eq([widget.versions[2]]) + context "invalid arguments" do + it "should raise an error" do + expect { PaperTrail::Version.where_object(:foo) }.to raise_error(ArgumentError) + expect { PaperTrail::Version.where_object([]) }.to raise_error(ArgumentError) + end end - after(:all) { PaperTrail.serializer = PaperTrail::Serializers::YAML } + context "valid arguments", :versioning => true do + let(:widget) { Widget.new } + let(:name) { Faker::Name.first_name } + let(:int) { rand(10) + 1 } + + before do + widget.update_attributes!(:name => name, :an_integer => int) + widget.update_attributes!(:name => 'foobar', :an_integer => 100) + widget.update_attributes!(:name => Faker::Name.last_name, :an_integer => 15) + end + + context "`serializer == YAML`" do + specify { expect(PaperTrail.serializer).to be PaperTrail::Serializers::YAML } + + it "should be able to locate versions according to their `object` contents" do + expect(PaperTrail::Version.where_object(:name => name)).to eq([widget.versions[1]]) + expect(PaperTrail::Version.where_object(:an_integer => 100)).to eq([widget.versions[2]]) + end + end + + context "`serializer == JSON`" do + before(:all) { PaperTrail.serializer = PaperTrail::Serializers::JSON } + specify { expect(PaperTrail.serializer).to be PaperTrail::Serializers::JSON } + + it "should be able to locate versions according to their `object` contents" do + expect(PaperTrail::Version.where_object(:name => name)).to eq([widget.versions[1]]) + expect(PaperTrail::Version.where_object(:an_integer => 100)).to eq([widget.versions[2]]) + end + + after(:all) { PaperTrail.serializer = PaperTrail::Serializers::YAML } + end + end end end end From 62d1af0f4ccd543c53be95a2116f23f8eedc5602 Mon Sep 17 00:00:00 2001 From: Michael Raimondi Date: Thu, 9 Apr 2015 13:49:12 -0400 Subject: [PATCH 5/9] Containment operator is only available for JSONB columns --- lib/paper_trail/version_concern.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/paper_trail/version_concern.rb b/lib/paper_trail/version_concern.rb index 53abaaff2..9f74b15e8 100644 --- a/lib/paper_trail/version_concern.rb +++ b/lib/paper_trail/version_concern.rb @@ -87,8 +87,8 @@ def timestamp_sort_order(direction = 'asc') def where_object(args = {}) raise ArgumentError, 'expected to receive a Hash' unless args.is_a?(Hash) - if object_col_is_json? - where_conditions = "object @> '#{args.to_json}'::#{columns_hash['object'].type}" + if columns_hash['object'].type == :jsonb + where_conditions = "object @> '#{args.to_json}'::jsonb" else arel_field = arel_table[:object] @@ -105,8 +105,8 @@ def where_object(args = {}) def where_object_changes(args = {}) raise ArgumentError, 'expected to receive a Hash' unless args.is_a?(Hash) - if object_changes_col_is_json? - where_conditions = "object_changes @> '#{args.to_json}'::#{columns_hash['object_changes'].try(:type)}" + if columns_hash['object'].type == :jsonb + where_conditions = "object_changes @> '#{args.to_json}'::jsonb" else arel_field = arel_table[:object_changes] From 20c7dd73fe5c9fab219459351272bb6401248a78 Mon Sep 17 00:00:00 2001 From: Michael Raimondi Date: Thu, 9 Apr 2015 14:24:09 -0400 Subject: [PATCH 6/9] Build query for JSON column type --- lib/paper_trail/version_concern.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/paper_trail/version_concern.rb b/lib/paper_trail/version_concern.rb index 9f74b15e8..b2483db79 100644 --- a/lib/paper_trail/version_concern.rb +++ b/lib/paper_trail/version_concern.rb @@ -89,6 +89,11 @@ def where_object(args = {}) if columns_hash['object'].type == :jsonb where_conditions = "object @> '#{args.to_json}'::jsonb" + elsif columns_hash['object'].type == :json + where_conditions = args.map do |field, value| + "object->>'#{field}' = '#{value}'" + end + where_conditions = where_conditions.join(" AND ") else arel_field = arel_table[:object] From 16d35086587a35447a0c4549a270e445229a7bee Mon Sep 17 00:00:00 2001 From: Michael Raimondi Date: Thu, 9 Apr 2015 14:56:26 -0400 Subject: [PATCH 7/9] Add tests and refactor where_object_changes with JSONB --- lib/paper_trail/version_concern.rb | 3 +- spec/models/version_spec.rb | 92 +++++++++++++++--------------- 2 files changed, 49 insertions(+), 46 deletions(-) diff --git a/lib/paper_trail/version_concern.rb b/lib/paper_trail/version_concern.rb index b2483db79..087e07604 100644 --- a/lib/paper_trail/version_concern.rb +++ b/lib/paper_trail/version_concern.rb @@ -110,7 +110,8 @@ def where_object(args = {}) def where_object_changes(args = {}) raise ArgumentError, 'expected to receive a Hash' unless args.is_a?(Hash) - if columns_hash['object'].type == :jsonb + if columns_hash['object_changes'].type == :jsonb + args.each { |field, value| args[field] = [value] } where_conditions = "object_changes @> '#{args.to_json}'::jsonb" else arel_field = arel_table[:object_changes] diff --git a/spec/models/version_spec.rb b/spec/models/version_spec.rb index 199a602fa..4acca5f4d 100644 --- a/spec/models/version_spec.rb +++ b/spec/models/version_spec.rb @@ -66,15 +66,17 @@ describe "Class" do column_overrides = [false] - column_overrides.concat(['jsonb']) if ENV['DB'] == 'postgres' + column_overrides.concat(%w[jsonb]) if ENV['DB'] == 'postgres' column_overrides.each do |override| context "with a #{override || 'text'} column" do before do if override ActiveRecord::Base.connection.execute("SAVEPOINT pgtest;") - ActiveRecord::Base.connection.execute("ALTER TABLE versions DROP COLUMN object;") - ActiveRecord::Base.connection.execute("ALTER TABLE versions ADD COLUMN object #{override};") + %w[object object_changes].each do |column| + ActiveRecord::Base.connection.execute("ALTER TABLE versions DROP COLUMN #{column};") + ActiveRecord::Base.connection.execute("ALTER TABLE versions ADD COLUMN #{column} #{override};") + end PaperTrail::Version.reset_column_information end end @@ -128,59 +130,59 @@ end end end - end - end - describe '#where_object_changes' do - it { expect(PaperTrail::Version).to respond_to(:where_object_changes) } + describe '#where_object_changes' do + it { expect(PaperTrail::Version).to respond_to(:where_object_changes) } - context "invalid arguments" do - it "should raise an error" do - expect { PaperTrail::Version.where_object_changes(:foo) }.to raise_error(ArgumentError) - expect { PaperTrail::Version.where_object_changes([]) }.to raise_error(ArgumentError) - end - end + context "invalid arguments" do + it "should raise an error" do + expect { PaperTrail::Version.where_object_changes(:foo) }.to raise_error(ArgumentError) + expect { PaperTrail::Version.where_object_changes([]) }.to raise_error(ArgumentError) + end + end - context "valid arguments", :versioning => true do - let(:widget) { Widget.new } - let(:name) { Faker::Name.first_name } - let(:int) { rand(5) + 2 } + context "valid arguments", :versioning => true do + let(:widget) { Widget.new } + let(:name) { Faker::Name.first_name } + let(:int) { rand(5) + 2 } - before do - widget.update_attributes!(:name => name, :an_integer => 0) - widget.update_attributes!(:name => 'foobar', :an_integer => 77) - widget.update_attributes!(:name => Faker::Name.last_name, :an_integer => int) - end + before do + widget.update_attributes!(:name => name, :an_integer => 0) + widget.update_attributes!(:name => 'foobar', :an_integer => 77) + widget.update_attributes!(:name => Faker::Name.last_name, :an_integer => int) + end + + context "`serializer == YAML`" do + specify { expect(PaperTrail.serializer).to be PaperTrail::Serializers::YAML } - context "`serializer == YAML`" do - specify { expect(PaperTrail.serializer).to be PaperTrail::Serializers::YAML } + it "should be able to locate versions according to their `object_changes` contents" do + expect(widget.versions.where_object_changes(:name => name)).to eq(widget.versions[0..1]) + expect(widget.versions.where_object_changes(:an_integer => 77)).to eq(widget.versions[1..2]) + expect(widget.versions.where_object_changes(:an_integer => int)).to eq([widget.versions.last]) + end - it "should be able to locate versions according to their `object_changes` contents" do - expect(widget.versions.where_object_changes(:name => name)).to eq(widget.versions[0..1]) - expect(widget.versions.where_object_changes(:an_integer => 77)).to eq(widget.versions[1..2]) - expect(widget.versions.where_object_changes(:an_integer => int)).to eq([widget.versions.last]) - end + it "should be able to handle queries for multiple attributes" do + expect(widget.versions.where_object_changes(:an_integer => 77, :name => 'foobar')).to eq(widget.versions[1..2]) + end + end - it "should be able to handle queries for multiple attributes" do - expect(widget.versions.where_object_changes(:an_integer => 77, :name => 'foobar')).to eq(widget.versions[1..2]) - end - end + context "`serializer == JSON`" do + before(:all) { PaperTrail.serializer = PaperTrail::Serializers::JSON } + specify { expect(PaperTrail.serializer).to be PaperTrail::Serializers::JSON } - context "`serializer == JSON`" do - before(:all) { PaperTrail.serializer = PaperTrail::Serializers::JSON } - specify { expect(PaperTrail.serializer).to be PaperTrail::Serializers::JSON } + it "should be able to locate versions according to their `object_changes` contents" do + expect(widget.versions.where_object_changes(:name => name)).to eq(widget.versions[0..1]) + expect(widget.versions.where_object_changes(:an_integer => 77)).to eq(widget.versions[1..2]) + expect(widget.versions.where_object_changes(:an_integer => int)).to eq([widget.versions.last]) + end - it "should be able to locate versions according to their `object_changes` contents" do - expect(widget.versions.where_object_changes(:name => name)).to eq(widget.versions[0..1]) - expect(widget.versions.where_object_changes(:an_integer => 77)).to eq(widget.versions[1..2]) - expect(widget.versions.where_object_changes(:an_integer => int)).to eq([widget.versions.last]) - end + it "should be able to handle queries for multiple attributes" do + expect(widget.versions.where_object_changes(:an_integer => 77, :name => 'foobar')).to eq(widget.versions[1..2]) + end - it "should be able to handle queries for multiple attributes" do - expect(widget.versions.where_object_changes(:an_integer => 77, :name => 'foobar')).to eq(widget.versions[1..2]) + after(:all) { PaperTrail.serializer = PaperTrail::Serializers::YAML } + end end - - after(:all) { PaperTrail.serializer = PaperTrail::Serializers::YAML } end end end From 2abfdc901d5e9a82703ed6dd85549e0c312f14da Mon Sep 17 00:00:00 2001 From: Michael Raimondi Date: Thu, 9 Apr 2015 15:27:15 -0400 Subject: [PATCH 8/9] Implement where_object_changes query for JSON columns --- lib/paper_trail/version_concern.rb | 5 +++++ spec/models/version_spec.rb | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/paper_trail/version_concern.rb b/lib/paper_trail/version_concern.rb index 087e07604..04f673d02 100644 --- a/lib/paper_trail/version_concern.rb +++ b/lib/paper_trail/version_concern.rb @@ -113,6 +113,11 @@ def where_object_changes(args = {}) if columns_hash['object_changes'].type == :jsonb args.each { |field, value| args[field] = [value] } where_conditions = "object_changes @> '#{args.to_json}'::jsonb" + elsif columns_hash['object'].type == :json + where_conditions = args.map do |field, value| + "((object_changes->>'#{field}' ILIKE '[#{value.to_json},%') OR (object_changes->>'#{field}' ILIKE '[%,#{value.to_json}]%'))" + end + where_conditions = where_conditions.join(" AND ") else arel_field = arel_table[:object_changes] diff --git a/spec/models/version_spec.rb b/spec/models/version_spec.rb index 4acca5f4d..5cfb4b180 100644 --- a/spec/models/version_spec.rb +++ b/spec/models/version_spec.rb @@ -66,9 +66,9 @@ describe "Class" do column_overrides = [false] - column_overrides.concat(%w[jsonb]) if ENV['DB'] == 'postgres' + column_overrides.concat(%w[json jsonb]) if ENV['DB'] == 'postgres' - column_overrides.each do |override| + column_overrides.shuffle.each do |override| context "with a #{override || 'text'} column" do before do if override From 3460c9c8f958813b40c54c089569eab51e9c1bcc Mon Sep 17 00:00:00 2001 From: Michael Raimondi Date: Thu, 9 Apr 2015 15:34:58 -0400 Subject: [PATCH 9/9] Bump Postgres version in CI --- .travis.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 963b15888..f8e58dfa1 100644 --- a/.travis.yml +++ b/.travis.yml @@ -37,4 +37,6 @@ matrix: gemfile: Gemfile - rvm: 1.8.7 gemfile: Gemfile - \ No newline at end of file + +addons: + postgresql: "9.4"