Add version_error_behavior config option

CHANGELOG.md

@@ -11,7 +11,8 @@ recommendations of [keepachangelog.com](http://keepachangelog.com/).
 
 ### Added
 
-- None
+- [#1422](https://github.com/paper-trail-gem/paper_trail/pull/1450) - Add `version_error_behavior` config
+  config option to control error handling when creating/updating/deleting `Version` records.
 
 ### Fixed
 

README.md

@@ -289,6 +289,7 @@ Global configuration options affect all threads.
 - object_changes_adapter
 - serializer
 - version_limit
+- version_error_behavior
 
 Syntax example: (options described in detail later)
 

lib/paper_trail/config.rb

@@ -14,7 +14,8 @@ class Config
       :object_changes_adapter,
       :serializer,
       :version_limit,
-      :has_paper_trail_defaults
+      :has_paper_trail_defaults,
+      :version_error_behavior
     )
 
     def initialize
@@ -25,6 +26,7 @@ def initialize
       # Variables which affect all threads, whose access is *not* synchronized.
       @serializer = PaperTrail::Serializers::YAML
       @has_paper_trail_defaults = {}
+      @version_error_behavior = :legacy
     end
 
     # Indicates whether PaperTrail is on or off. Default: true.

lib/paper_trail/record_trail.rb

@@ -64,6 +64,8 @@ def record_create
         # of versions_assoc.build?, the association cache is unaware. So, we
         # invalidate the `versions` association cache with `reset`.
         versions.reset
+      rescue StandardError => e
+        handle_version_errors e, version, :create
       end
     end
 
@@ -81,12 +83,13 @@ def record_destroy(recording_order)
       # `data_for_destroy` but PT-AT still does.
       data = event.data.merge(data_for_destroy)
 
-      version = @record.class.paper_trail.version_class.create(data)
-      if version.errors.any?
-        log_version_errors(version, :destroy)
-      else
+      version = @record.class.paper_trail.version_class.new(data)
+      begin
+        version.save!
         assign_and_reset_version_association(version)
         version
+      rescue StandardError => e
+        handle_version_errors e, version, :destroy
       end
     end
 
@@ -108,14 +111,15 @@ def record_update(force:, in_after_callback:, is_touch:)
       )
       return unless version
 
-      if version.save
+      begin
+        version.save!
         # Because the version object was created using version_class.new instead
         # of versions_assoc.build?, the association cache is unaware. So, we
         # invalidate the `versions` association cache with `reset`.
         versions.reset
         version
-      else
-        log_version_errors(version, :update)
+      rescue StandardError => e
+        handle_version_errors e, version, :update
       end
     end
 
@@ -286,6 +290,26 @@ def log_version_errors(version, action)
       )
     end
 
+    # Centralized handler for version errors
+    # @api private
+    def handle_version_errors(e, version, action)
+      case PaperTrail.config.version_error_behavior
+      when :legacy
+        # legacy behavior was to raise on create and log on update/delete
+        if action == :create
+          raise e
+        else
+          log_version_errors(version, action)
+        end
+      when :log
+        log_version_errors(version, action)
+      when :exception
+        raise e
+      when :silent
+        # noop
+      end
+    end
+
     # @api private
     # @return - The created version object, so that plugins can use it, e.g.
     # paper_trail-association_tracking
@@ -298,11 +322,12 @@ def record_update_columns(changes)
       data.merge!(data_for_update_columns)
 
       versions_assoc = @record.send(@record.class.versions_association_name)
-      version = versions_assoc.create(data)
-      if version.errors.any?
-        log_version_errors(version, :update)
-      else
+      version = versions_assoc.new(data)
+      begin
+        version.save!
         version
+      rescue StandardError => e
+        handle_version_errors e, version, :update
       end
     end
 

spec/dummy_app/app/models/comment.rb

@@ -0,0 +1,5 @@
+# frozen_string_literal: true
+
+class Comment < ApplicationRecord
+  has_paper_trail versions: { class_name: "CommentVersion" }
+end

spec/dummy_app/app/versions/comment_version.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class CommentVersion < PaperTrail::Version
+  self.table_name = "comment_versions"
+  # add rails validation to require whodunnit
+  validates :whodunnit, presence: true
+end

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

@@ -114,6 +114,17 @@ def up
     end
     add_index :post_versions, %i[item_type item_id]
 
+    # Requires whodunnit column.
+    create_table :comment_versions, force: true do |t|
+      t.string   :item_type, null: false
+      t.integer  :item_id,   null: false
+      t.string   :event,     null: false
+      t.string   :whodunnit, null: false
+      t.text     :object
+      t.datetime :created_at, limit: 6
+    end
+    add_index :comment_versions, %i[item_type item_id]
+
     # Uses custom versions table `no_object_versions`.
     create_table :no_objects, force: true do |t|
       t.string :letter, null: false, limit: 1
@@ -224,6 +235,10 @@ def up
       t.string :content
     end
 
+    create_table :comments, force: true do |t|
+      t.string :content
+    end
+
     create_table :post_with_statuses, force: true do |t|
       t.integer :status
       t.timestamps null: false, limit: 6

spec/paper_trail/config_spec.rb

@@ -17,6 +17,227 @@ module PaperTrail
       end
     end
 
+    describe ".version_error_behavior", versioning: true do
+      let(:logger) { instance_double(Logger) }
+
+      before do
+        allow(logger).to receive(:warn)
+        allow(logger).to receive(:debug?).and_return(false)
+
+        ActiveRecord::Base.logger = logger
+      end
+
+      after do
+        ActiveRecord::Base.logger = nil
+      end
+
+      context "when :legacy" do
+        context "when version cannot be created" do
+          before { PaperTrail.config.version_error_behavior = :legacy }
+
+          it "raises an error but does not log on create" do
+            expect {
+              Comment.create!(content: "Henry")
+            }.to raise_error(ActiveRecord::RecordInvalid)
+
+            expect(logger).not_to have_received(:warn)
+          end
+
+          it "logs but does not raise error on update" do
+            comment = PaperTrail.request(whodunnit: "Foo") do
+              Comment.create!(content: "Henry")
+            end
+
+            expect {
+              comment.update!(content: "Brad")
+            }.not_to raise_error
+
+            expect(logger).to have_received(:warn)
+          end
+
+          it "does not raise error or log on update_columns" do
+            comment = PaperTrail.request(whodunnit: "Foo") do
+              Comment.create!(content: "Henry")
+            end
+
+            expect {
+              comment.update_columns(content: "Brad")
+            }.not_to raise_error
+
+            expect(logger).not_to have_received(:warn)
+          end
+
+          it "logs but does not raise error on destroy" do
+            comment = PaperTrail.request(whodunnit: "Foo") do
+              Comment.create!(content: "Henry")
+            end
+
+            expect {
+              comment.destroy!
+            }.not_to raise_error
+
+            expect(logger).to have_received(:warn)
+          end
+        end
+      end
+
+      context "when exception" do
+        context "when version cannot be created" do
+          before { PaperTrail.config.version_error_behavior = :exception }
+
+          after { PaperTrail.config.version_error_behavior = :legacy }
+
+          it "raises an error but does not log on create" do
+            expect {
+              Comment.create!(content: "Henry")
+            }.to raise_error(ActiveRecord::RecordInvalid)
+
+            expect(logger).not_to have_received(:warn)
+          end
+
+          it "raises an error but does not on update" do
+            comment = PaperTrail.request(whodunnit: "Foo") do
+              Comment.create!(content: "Henry")
+            end
+
+            expect {
+              comment.update!(content: "Brad")
+            }.to raise_error(ActiveRecord::RecordInvalid)
+
+            expect(logger).not_to have_received(:warn)
+          end
+
+          it "does not raise an error or log on update_columns" do
+            comment = PaperTrail.request(whodunnit: "Foo") do
+              Comment.create!(content: "Henry")
+            end
+
+            expect {
+              comment.update_columns(content: "Brad")
+            }.not_to raise_error
+
+            expect(logger).not_to have_received(:warn)
+          end
+
+          it "raises an error but does not log on destroy" do
+            comment = PaperTrail.request(whodunnit: "Foo") do
+              Comment.create!(content: "Henry")
+            end
+
+            expect {
+              comment.destroy!
+            }.to raise_error(ActiveRecord::RecordInvalid)
+
+            expect(logger).not_to have_received(:warn)
+          end
+        end
+      end
+
+      context "when log" do
+        context "when version cannot be created" do
+          before { PaperTrail.config.version_error_behavior = :log }
+
+          after { PaperTrail.config.version_error_behavior = :legacy }
+
+          it "logs and does not raise an error on create" do
+            expect {
+              Comment.create!(content: "Henry")
+            }.not_to raise_error
+
+            expect(logger).to have_received(:warn)
+          end
+
+          it "logs and does not raise an error on update" do
+            comment = PaperTrail.request(whodunnit: "Foo") do
+              Comment.create!(content: "Henry")
+            end
+
+            expect {
+              comment.update!(content: "Brad")
+            }.not_to raise_error
+
+            expect(logger).to have_received(:warn)
+          end
+
+          it "does not raise an error or log on update_columns" do
+            comment = PaperTrail.request(whodunnit: "Foo") do
+              Comment.create!(content: "Henry")
+            end
+
+            expect {
+              comment.update_columns(content: "Brad")
+            }.not_to raise_error
+
+            expect(logger).not_to have_received(:warn)
+          end
+
+          it "logs and does not raise an error on destroy" do
+            comment = PaperTrail.request(whodunnit: "Foo") do
+              Comment.create!(content: "Henry")
+            end
+
+            expect {
+              comment.destroy!
+            }.not_to raise_error
+
+            expect(logger).to have_received(:warn)
+          end
+        end
+      end
+
+      context "when silent" do
+        context "when version cannot be created" do
+          before { PaperTrail.config.version_error_behavior = :silent }
+
+          after { PaperTrail.config.version_error_behavior = :legacy }
+
+          it "does not log or raise an error on create" do
+            expect {
+              Comment.create!(content: "Henry")
+            }.not_to raise_error
+
+            expect(logger).not_to have_received(:warn)
+          end
+
+          it "does not log or raise an error on update" do
+            comment = PaperTrail.request(whodunnit: "Foo") do
+              Comment.create!(content: "Henry")
+            end
+
+            expect {
+              comment.update!(content: "Brad")
+            }.not_to raise_error
+
+            expect(logger).not_to have_received(:warn)
+          end
+
+          it "does not log or raise an error on update_columns" do
+            comment = PaperTrail.request(whodunnit: "Foo") do
+              Comment.create!(content: "Henry")
+            end
+
+            expect {
+              comment.update_columns(content: "Brad")
+            }.not_to raise_error
+
+            expect(logger).not_to have_received(:warn)
+          end
+
+          it "does not log or raise an error on destroy" do
+            comment = PaperTrail.request(whodunnit: "Foo") do
+              Comment.create!(content: "Henry")
+            end
+
+            expect {
+              comment.destroy!
+            }.not_to raise_error
+
+            expect(logger).not_to have_received(:warn)
+          end
+        end
+      end
+    end
+
     describe ".version_limit", versioning: true do
       after { PaperTrail.config.version_limit = nil }