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 collectiveidea#572

Author: jdufresne

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

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'

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

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"

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