Merge pull request #26 from akxcv/master

Default version timestamp to "updated_at"

.rubocop.yml

@@ -23,7 +23,7 @@ Style/Documentation:
   Exclude:
     - 'spec/**/*.rb'
 
-Style/StringLiterals: 
+Style/StringLiterals:
   Enabled: false
 
 Style/SpaceInsideStringInterpolation:
@@ -49,4 +49,15 @@ Rails/Date:
   Enabled: false
 
 Rails/TimeZone:
-  Enabled: false
+  Enabled: false
+
+Style/NumericLiteralPrefix:
+  Enabled: false
+
+Lint/HandleExceptions:
+  Enabled: true
+  Exclude:
+    - 'spec/**/*.rb'
+
+Style/DotPosition:
+  EnforcedStyle: leading

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Change log
 
+## master
+
+- Add `--timestamp_column` option to model migration generator. ([@akxcv][])
+
+- Default version timestamp to timestamp column. ([@akxcv][])
+
 ## 0.4.1 (2017-02-06)
 
 - Add `--path` option to model migration generator. ([@palkan][])

README.md

@@ -75,6 +75,17 @@ By default, Logidze tries to infer the path to the model file from the model nam
 rails generate logidze:model Post --path "app/models/custom/post.rb"
 ```
 
+By default, Logidze tries to get a timestamp for a version from record's `updated_at` field whenever appropriate. If
+your model does not have that column, Logidze will gracefully fall back to `statement_timestamp()`.
+To change the column name or disable this feature completely, you can use the `timestamp_column` option:
+
+```ruby
+# will try to get the timestamp value from `time` column
+rails generate logidze:model Post --timestamp_column time
+# will always set version timestamp to `statement_timestamp()`
+rails generate logidze:model Post --timestamp_column nil # "null" and "false" will also work
+```
+
 ## Troubleshooting
 
 The most common problem is `"permission denied to set parameter "logidze.xxx"` caused by `ALTER DATABASE ...` query.

lib/generators/logidze/install/templates/migration.rb.erb

@@ -19,17 +19,19 @@ class <%= @migration_class_name %> < ActiveRecord::Migration
     execute <<-SQL
       DROP FUNCTION IF EXISTS logidze_version(bigint, jsonb);
       DROP FUNCTION IF EXISTS logidze_snapshot(jsonb);
+      DROP FUNCTION IF EXISTS logidze_version(bigint, jsonb, text[]);
+      DROP FUNCTION IF EXISTS logidze_snapshot(jsonb, text[]);
     SQL
     <% end %>
 
     execute <<-SQL
-      CREATE OR REPLACE FUNCTION logidze_version(v bigint, data jsonb, blacklist text[] DEFAULT '{}') RETURNS jsonb AS $body$
+      CREATE OR REPLACE FUNCTION logidze_version(v bigint, data jsonb, ts timestamp with time zone, blacklist text[] DEFAULT '{}') RETURNS jsonb AS $body$
         DECLARE
           buf jsonb;
         BEGIN
           buf := jsonb_build_object(
                    'ts',
-                   (extract(epoch from now()) * 1000)::bigint,
+                   (extract(epoch from ts) * 1000)::bigint,
                    'v',
                     v,
                     'c',
@@ -43,12 +45,19 @@ class <%= @migration_class_name %> < ActiveRecord::Migration
       $body$
       LANGUAGE plpgsql;
 
-      CREATE OR REPLACE FUNCTION logidze_snapshot(item jsonb, blacklist text[] DEFAULT '{}') RETURNS jsonb AS $body$
+      CREATE OR REPLACE FUNCTION logidze_snapshot(item jsonb, ts_column text, blacklist text[] DEFAULT '{}') RETURNS jsonb AS $body$
+        DECLARE
+          ts timestamp with time zone;
         BEGIN
+          IF ts_column IS NULL THEN
+            ts := statement_timestamp();
+          ELSE
+            ts := coalesce((item->>ts_column)::timestamp with time zone, statement_timestamp());
+          END IF;
           return json_build_object(
             'v', 1,
             'h', jsonb_build_array(
-                   logidze_version(1, item, blacklist)
+                   logidze_version(1, item, ts, blacklist)
                  )
             );
         END;
@@ -62,7 +71,7 @@ class <%= @migration_class_name %> < ActiveRecord::Migration
         BEGIN
           res := obj;
           FOREACH key IN ARRAY keys
-          LOOP 
+          LOOP
             res := res - key;
           END LOOP;
           RETURN res;
@@ -93,7 +102,7 @@ class <%= @migration_class_name %> < ActiveRecord::Migration
             jsonb_set(
               log_data->'h',
               '{1}',
-              merged 
+              merged
             ) - 0
           );
         END;
@@ -111,23 +120,35 @@ class <%= @migration_class_name %> < ActiveRecord::Migration
           iterator integer;
           item record;
           columns_blacklist text[];
+          ts timestamp with time zone;
+          ts_column text;
         BEGIN
-          columns_blacklist := TG_ARGV[1];
+          ts_column := NULLIF(TG_ARGV[1], 'null');
+          columns_blacklist := TG_ARGV[2];
 
           IF TG_OP = 'INSERT' THEN
 
-            NEW.log_data := logidze_snapshot(to_jsonb(NEW.*), columns_blacklist);
-          
+            NEW.log_data := logidze_snapshot(to_jsonb(NEW.*), ts_column, columns_blacklist);
+
           ELSIF TG_OP = 'UPDATE' THEN
-          
+
             IF OLD.log_data is NULL OR OLD.log_data = '{}'::jsonb THEN
-              NEW.log_data := logidze_snapshot(to_jsonb(NEW.*), columns_blacklist);
+              NEW.log_data := logidze_snapshot(to_jsonb(NEW.*), ts_column, columns_blacklist);
               RETURN NEW;
             END IF;
 
             history_limit := NULLIF(TG_ARGV[0], 'null');
             current_version := (NEW.log_data->>'v')::int;
 
+            IF ts_column IS NULL THEN
+              ts := statement_timestamp();
+            ELSE
+              ts := (to_jsonb(NEW.*)->>ts_column)::timestamp with time zone;
+              IF ts IS NULL OR ts = (to_jsonb(OLD.*)->>ts_column)::timestamp with time zone THEN
+                ts := statement_timestamp();
+              END IF;
+            END IF;
+
             IF NEW = OLD THEN
               RETURN NEW;
             END IF;
@@ -158,7 +179,7 @@ class <%= @migration_class_name %> < ActiveRecord::Migration
             NEW.log_data := jsonb_set(
               NEW.log_data,
               ARRAY['h', size::text],
-              logidze_version(new_v, changes, columns_blacklist),
+              logidze_version(new_v, changes, ts, columns_blacklist),
               true
             );
 
@@ -183,9 +204,9 @@ class <%= @migration_class_name %> < ActiveRecord::Migration
   def down
     <% unless update? %>
     execute <<-SQL
-      DROP FUNCTION logidze_version(bigint, jsonb, text[]) CASCADE;
+      DROP FUNCTION logidze_version(bigint, jsonb, timestamp with time zone, text[]) CASCADE;
       DROP FUNCTION logidze_compact_history(jsonb) CASCADE;
-      DROP FUNCTION logidze_snapshot(jsonb, text[]) CASCADE;
+      DROP FUNCTION logidze_snapshot(jsonb, text, text[]) CASCADE;
       DROP FUNCTION logidze_logger() CASCADE;
     SQL
     <% end %>

lib/generators/logidze/model/model_generator.rb

@@ -1,3 +1,4 @@
+# rubocop:disable Metrics/BlockLength
 # frozen_string_literal: true
 require "rails/generators"
 require "rails/generators/active_record/migration/migration_generator"
@@ -20,6 +21,9 @@ class ModelGenerator < ::ActiveRecord::Generators::Base # :nodoc:
       class_option :blacklist, type: :array, optional: true
       class_option :whitelist, type: :array, optional: true
 
+      class_option :timestamp_column, type: :string, optional: true,
+                                      desc: "Specify timestamp column"
+
       def generate_migration
         if options[:blacklist] && options[:whitelist]
           $stderr.puts "Use only one: --whitelist or --blacklist"
@@ -62,30 +66,46 @@ def columns_blacklist
                     class_name.constantize.column_names - options[:whitelist]
                   end
 
-          array || []
+          format_pgsql_array(array)
+        end
+
+        def timestamp_column
+          value = options.fetch(:timestamp_column, 'updated_at')
+          return if %w(nil null false).include?(value)
+          escape_pgsql_string(value)
         end
 
         def logidze_logger_parameters
-          if limit.nil? && columns_blacklist.empty?
-            ''
-          elsif !limit.nil? && columns_blacklist.empty?
-            limit
-          elsif !limit.nil? && !columns_blacklist.empty?
-            "#{limit}, #{format_pgsql_array(columns_blacklist)}"
-          elsif limit.nil? && !columns_blacklist.empty?
-            "null, #{format_pgsql_array(columns_blacklist)}"
-          end
+          format_pgsql_args(limit, timestamp_column, columns_blacklist)
         end
 
         def logidze_snapshot_parameters
-          return 'to_jsonb(t)' if columns_blacklist.empty?
-
-          "to_jsonb(t), #{format_pgsql_array(columns_blacklist)}"
+          format_pgsql_args('to_jsonb(t)', timestamp_column, columns_blacklist)
         end
 
         def format_pgsql_array(ruby_array)
+          return if ruby_array.blank?
           "'{" + ruby_array.join(', ') + "}'"
         end
+
+        def escape_pgsql_string(string)
+          return if string.blank?
+          "'#{string}'"
+        end
+
+        # Convenience method for formatting pg arguments.
+        # Some examples:
+        # format_pgsql_args('a', 'b', nil) #=> "a, b"
+        # format_pgsql_args(nil, '', 'c')  #=> "null, null, c"
+        # format_pgsql_args('a', '', [])   #=> "a"
+        def format_pgsql_args(*values)
+          args = []
+          values.reverse_each do |value|
+            formatted_value = value.presence || (args.any? && 'null')
+            args << formatted_value if formatted_value
+          end
+          args.compact.reverse.join(', ')
+        end
       end
 
       private

lib/generators/logidze/model/templates/migration.rb.erb

@@ -1,7 +1,7 @@
 class <%= @migration_class_name %> < ActiveRecord::Migration
   require 'logidze/migration'
   include Logidze::Migration
- 
+
   def up
     <% unless only_trigger? %>
     add_column :<%= table_name %>, :log_data, :jsonb

logidze.gemspec

@@ -27,4 +27,5 @@ Gem::Specification.new do |spec|
   spec.add_development_dependency "simplecov", ">= 0.3.8"
   spec.add_development_dependency "ammeter", "~> 1.1.3"
   spec.add_development_dependency "pry-byebug"
+  spec.add_development_dependency "timecop", "~> 0.8"
 end

spec/dummy/db/migrate/20160415094739_create_users.rb

@@ -5,7 +5,7 @@ def change
       t.integer :age
       t.boolean :active
       t.jsonb :log_data
-      t.timestamps null: false
+      t.timestamp :time
     end
   end
 end

spec/dummy/db/schema.rb

@@ -10,11 +10,10 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20170110144045) do
+ActiveRecord::Schema.define(version: 20160415194001) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
-  enable_extension "hstore"
 
   create_table "posts", force: :cascade do |t|
     t.string   "title"
@@ -29,8 +28,7 @@
     t.integer  "age"
     t.boolean  "active"
     t.jsonb    "log_data"
-    t.datetime "created_at", null: false
-    t.datetime "updated_at", null: false
+    t.datetime "time"
   end
 
 end

spec/generators/model_generator_spec.rb

@@ -35,7 +35,7 @@ class User < ActiveRecord::Base
         is_expected.to contain "add_column :users, :log_data, :jsonb"
         is_expected.to contain /create trigger logidze_on_users/i
         is_expected.to contain /before update or insert on users for each row/i
-        is_expected.to contain /execute procedure logidze_logger\(\);/i
+        is_expected.to contain /execute procedure logidze_logger\(null, 'updated_at'\);/i
         is_expected.to contain /drop trigger if exists logidze_on_users on users/i
         is_expected.to contain "remove_column :users, :log_data"
         is_expected.not_to contain(/update users/i)
@@ -48,7 +48,7 @@ class User < ActiveRecord::Base
 
         it "creates trigger with limit" do
           is_expected.to exist
-          is_expected.to contain(/execute procedure logidze_logger\(5\);/i)
+          is_expected.to contain(/execute procedure logidze_logger\(5, 'updated_at'\);/i)
         end
       end
 
@@ -58,7 +58,7 @@ class User < ActiveRecord::Base
         it "creates trigger with columns blacklist" do
           is_expected.to exist
           is_expected.to contain(
-            /execute procedure logidze_logger\(null, '\{age, active\}'\);/i
+            /execute procedure logidze_logger\(null, 'updated_at', '\{age, active\}'\);/i
           )
         end
       end
@@ -69,7 +69,7 @@ class User < ActiveRecord::Base
         it "creates backfill query" do
           is_expected.to exist
           is_expected.to contain(/update users as t/i)
-          is_expected.to contain(/set log_data = logidze_snapshot\(to_jsonb\(t\)\);/i)
+          is_expected.to contain(/set log_data = logidze_snapshot\(to_jsonb\(t\), 'updated_at'\);/i)
         end
       end
 
@@ -81,11 +81,35 @@ class User < ActiveRecord::Base
           is_expected.not_to contain "add_column :users, :log_data, :jsonb"
           is_expected.to contain /create trigger logidze_on_users/i
           is_expected.to contain /before update or insert on users for each row/i
-          is_expected.to contain /execute procedure logidze_logger\(\);/i
+          is_expected.to contain /execute procedure logidze_logger\(null, 'updated_at'\);/i
           is_expected.to contain /drop trigger if exists logidze_on_users on users/i
           is_expected.not_to contain "remove_column :users, :log_data"
         end
       end
+
+      context "with timestamp_column" do
+        context "custom column name" do
+          let(:args) { ["user", "--timestamp_column", "time"] }
+
+          it "creates trigger with 'time' timestamp column" do
+            is_expected.to exist
+            is_expected.to contain(
+              /execute procedure logidze_logger\(null, 'time'\);/i
+            )
+          end
+        end
+
+        context "nil" do
+          let(:args) { ["user", "--timestamp_column", "nil"] }
+
+          it "creates trigger without timestamp column" do
+            is_expected.to exist
+            is_expected.to contain(
+              /execute procedure logidze_logger\(\);/i
+            )
+          end
+        end
+      end
     end
 
     context "with namespace" do