From d6f61d6e3d0393610afbc3664d58c6d4308e8125 Mon Sep 17 00:00:00 2001
From: Jon Dufresne <jon.dufresne@gmail.com>
Date: Sun, 3 Apr 2022 16:43:56 -0700
Subject: [PATCH] Make several columns in the audits table NOT NULL

Improve data integrity by adding NOT NULL constraints to columns that
should _always_ have a value. This helps catch application and library
bugs earlier by catching mishandling of data.

Fixes #572
---
 .../templates/change_audits_columns_not_null.rb | 17 +++++++++++++++++
 lib/generators/audited/templates/install.rb     |  8 ++++----
 lib/generators/audited/upgrade_generator.rb     | 12 +++++++++++-
 spec/support/active_record/schema.rb            |  8 ++++----
 test/upgrade_generator_test.rb                  | 13 +++++++++++++
 5 files changed, 49 insertions(+), 9 deletions(-)
 create mode 100644 lib/generators/audited/templates/change_audits_columns_not_null.rb

diff --git a/lib/generators/audited/templates/change_audits_columns_not_null.rb b/lib/generators/audited/templates/change_audits_columns_not_null.rb
new file mode 100644
index 00000000..d47a986d
--- /dev/null
+++ b/lib/generators/audited/templates/change_audits_columns_not_null.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+class <%= migration_class_name %> < <%= migration_parent %>
+  def self.up
+    change_column_null :audits, :action, false
+    change_column_null :audits, :audited_changes, false
+    change_column_null :audits, :version, false
+    change_column_null :audits, :created_at, false
+  end
+
+  def self.down
+    change_column_null :audits, :action, true
+    change_column_null :audits, :audited_changes, true
+    change_column_null :audits, :version, true
+    change_column_null :audits, :created_at, true
+  end
+end
diff --git a/lib/generators/audited/templates/install.rb b/lib/generators/audited/templates/install.rb
index 5c6807f9..8c405ae9 100644
--- a/lib/generators/audited/templates/install.rb
+++ b/lib/generators/audited/templates/install.rb
@@ -10,13 +10,13 @@ def self.up
       t.column :user_id, :<%= options[:audited_user_id_column_type] %>
       t.column :user_type, :string
       t.column :username, :string
-      t.column :action, :string
-      t.column :audited_changes, :<%= options[:audited_changes_column_type] %>
-      t.column :version, :integer, :default => 0
+      t.column :action, :string, null: false
+      t.column :audited_changes, :<%= options[:audited_changes_column_type] %>, null: false
+      t.column :version, :integer, :default => 0, null: false
       t.column :comment, :string
       t.column :remote_address, :string
       t.column :request_uuid, :string
-      t.column :created_at, :datetime
+      t.column :created_at, :datetime, null: false
     end
 
     add_index :audits, [:auditable_type, :auditable_id, :version], :name => 'auditable_index'
diff --git a/lib/generators/audited/upgrade_generator.rb b/lib/generators/audited/upgrade_generator.rb
index b66d082d..8c6d5244 100644
--- a/lib/generators/audited/upgrade_generator.rb
+++ b/lib/generators/audited/upgrade_generator.rb
@@ -26,7 +26,7 @@ def copy_templates
 
       def migrations_to_be_applied
         Audited::Audit.reset_column_information
-        columns = Audited::Audit.columns.map(&:name)
+        columns = Audited::Audit.columns.map { |column| [column.name, column] }.to_h
         indexes = Audited::Audit.connection.indexes(Audited::Audit.table_name)
 
         yield :add_comment_to_audits unless columns.include?("comment")
@@ -64,6 +64,16 @@ def migrations_to_be_applied
         if indexes.any? { |i| i.columns == %w[auditable_type auditable_id] }
           yield :add_version_to_auditable_index
         end
+
+        columns_not_null = [
+          "action",
+          "audited_changes",
+          "version",
+          "created_at",
+        ]
+        if columns_not_null.any? { |column| columns[column].null }
+          yield :change_audits_columns_not_null
+        end
       end
     end
   end
diff --git a/spec/support/active_record/schema.rb b/spec/support/active_record/schema.rb
index 7145bc0c..bef0a0fa 100644
--- a/spec/support/active_record/schema.rb
+++ b/spec/support/active_record/schema.rb
@@ -73,13 +73,13 @@
     t.column :user_id, :integer
     t.column :user_type, :string
     t.column :username, :string
-    t.column :action, :string
-    t.column :audited_changes, :text
-    t.column :version, :integer, default: 0
+    t.column :action, :string, null: false
+    t.column :audited_changes, :text, null: false
+    t.column :version, :integer, default: 0, null: false
     t.column :comment, :string
     t.column :remote_address, :string
     t.column :request_uuid, :string
-    t.column :created_at, :datetime
+    t.column :created_at, :datetime, null: false
   end
 
   add_index :audits, [:auditable_id, :auditable_type], name: "auditable_index"
diff --git a/test/upgrade_generator_test.rb b/test/upgrade_generator_test.rb
index 3ec3a6b8..59446352 100644
--- a/test/upgrade_generator_test.rb
+++ b/test/upgrade_generator_test.rb
@@ -94,4 +94,17 @@ class UpgradeGeneratorTest < Rails::Generators::TestCase
       assert_includes(content, "class AddCommentToAudits < ActiveRecord::Migration[#{ActiveRecord::Migration.current_version}]\n")
     end
   end
+
+  test "should set several audits columns NOT NULL" do
+    load_schema 1
+
+    run_generator %w[upgrade]
+
+    assert_migration "db/migrate/change_audits_columns_not_null.rb" do |content|
+      assert_includes(content, 'change_column_null :audits, :action, false')
+      assert_includes(content, 'change_column_null :audits, :audited_changes, false')
+      assert_includes(content, 'change_column_null :audits, :version, false')
+      assert_includes(content, 'change_column_null :audits, :created_at, false')
+    end
+  end
 end