From 7a36eb25e1981c7b0b62c98ae43a415bc79a7968 Mon Sep 17 00:00:00 2001 From: Jon Dufresne 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 --- .../change_audits_columns_not_null.rb | 21 +++++++++++++++++++ lib/generators/audited/templates/install.rb | 12 +++++------ lib/generators/audited/upgrade_generator.rb | 14 ++++++++++++- spec/audited/auditor_spec.rb | 4 ++-- spec/support/active_record/schema.rb | 12 +++++------ 5 files changed, 48 insertions(+), 15 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..39030a27 --- /dev/null +++ b/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 diff --git a/lib/generators/audited/templates/install.rb b/lib/generators/audited/templates/install.rb index 5c6807f9..69e29977 100644 --- a/lib/generators/audited/templates/install.rb +++ b/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' diff --git a/lib/generators/audited/upgrade_generator.rb b/lib/generators/audited/upgrade_generator.rb index b66d082d..7f1eca86 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,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 diff --git a/spec/audited/auditor_spec.rb b/spec/audited/auditor_spec.rb index da04972b..1c8aba0e 100644 --- a/spec/audited/auditor_spec.rb +++ b/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 diff --git a/spec/support/active_record/schema.rb b/spec/support/active_record/schema.rb index 7145bc0c..1a28d84c 100644 --- a/spec/support/active_record/schema.rb +++ b/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"