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,21 @@
+# frozen_string_literal: true
+
+class <%= migration_class_name %> < <%= migration_parent %>
+  def self.up
+    change_column_null :audits, :auditable_id, false
+    change_column_null :audits, :auditable_type, false
+    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, :auditable_id, true
+    change_column_null :audits, :auditable_type, true
+    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

@@ -3,20 +3,20 @@
 class <%= migration_class_name %> < <%= migration_parent %>
   def self.up
     create_table :audits, :force => true do |t|
-      t.column :auditable_id, :integer
-      t.column :auditable_type, :string
+      t.column :auditable_id, :integer, null: false
+      t.column :auditable_type, :string, null: false
       t.column :associated_id, :integer
       t.column :associated_type, :string
       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,18 @@ 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 = [
+          "auditable_id",
+          "auditable_type",
+          "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/audited/auditor_spec.rb

@@ -447,7 +447,7 @@ def non_column_attr=(val)
     end
 
     it "should be able to reconstruct a destroyed record without history" do
-      @user.audits.delete_all
+      Audited::Audit.where(auditable: @user).delete_all
       @user.destroy
 
       revision = @user.audits.first.revision
@@ -634,7 +634,7 @@ def stub_global_max_audits(max_audits)
     end
 
     it "should be empty if no audits exist" do
-      user.audits.delete_all
+      Audited::Audit.where(auditable: user).delete_all
       expect(user.revisions).to be_empty
     end
 

spec/support/active_record/schema.rb

@@ -66,20 +66,20 @@
   end
 
   create_table :audits do |t|
-    t.column :auditable_id, :integer
-    t.column :auditable_type, :string
+    t.column :auditable_id, :integer, null: false
+    t.column :auditable_type, :string, null: false
     t.column :associated_id, :integer
     t.column :associated_type, :string
     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"