From 880c32ae61dc363c997fb04e1e20896e19d32e58 Mon Sep 17 00:00:00 2001 From: Oleg Kiviljov <oleg.kiviljov@gmail.com> Date: Tue, 26 Nov 2019 13:55:03 +0200 Subject: [PATCH 01/10] Add support for setting meta without transaction block --- lib/logidze/meta.rb | 75 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 60 insertions(+), 15 deletions(-) diff --git a/lib/logidze/meta.rb b/lib/logidze/meta.rb index ecea8d56..016d855d 100644 --- a/lib/logidze/meta.rb +++ b/lib/logidze/meta.rb @@ -3,18 +3,20 @@ module Logidze # :nodoc: # Provide methods to attach meta information module Meta - def with_meta(meta, &block) - MetaTransaction.wrap_with(meta, &block) + def with_meta(meta, transactional: true, &block) + return MetaWithTransaction.wrap_with(meta, &block) if transactional + + MetaWithoutTransaction.wrap_with(meta, &block) end - def with_responsible(responsible_id, &block) + def with_responsible(responsible_id, transactional: true, &block) return yield if responsible_id.nil? meta = {Logidze::History::Version::META_RESPONSIBLE => responsible_id} - with_meta(meta, &block) + with_meta(meta, transactional: transactional, &block) end - class MetaTransaction # :nodoc: + class MetaWrapper # :nodoc: def self.wrap_with(meta, &block) new(meta, &block).perform end @@ -28,6 +30,21 @@ def initialize(meta, &block) @block = block end + def current_meta + meta_stack.reduce(:merge) || {} + end + + def meta_stack + Thread.current[:meta] ||= [] + Thread.current[:meta] + end + + def encode_meta(value) + connection.quote(ActiveSupport::JSON.encode(value)) + end + end + + class MetaWithTransaction < MetaWrapper # :nodoc: def perform return if block.nil? return block.call if meta.nil? @@ -37,6 +54,18 @@ def perform private + def pg_set_meta_param(value) + connection.execute("SET LOCAL logidze.meta = #{encode_meta(value)};") + end + + def pg_reset_meta_param(prev_meta) + if prev_meta.empty? + connection.execute("SET LOCAL logidze.meta TO DEFAULT;") + else + pg_set_meta_param(prev_meta) + end + end + def call_block_in_meta_context prev_meta = current_meta @@ -50,30 +79,46 @@ def call_block_in_meta_context ensure meta_stack.pop end + end - def current_meta - meta_stack.reduce(:merge) || {} - end + class MetaWithoutTransaction < MetaWrapper # :nodoc: + def perform + raise ArgumentError, "Block must be given" unless block - def meta_stack - Thread.current[:meta] ||= [] - Thread.current[:meta] + call_block_in_meta_context end + private + def pg_set_meta_param(value) - encoded_meta = connection.quote(ActiveSupport::JSON.encode(value)) - connection.execute("SET LOCAL logidze.meta = #{encoded_meta};") + connection.execute("SET logidze.meta = #{encode_meta(value)};") end def pg_reset_meta_param(prev_meta) if prev_meta.empty? - connection.execute("SET LOCAL logidze.meta TO DEFAULT;") + connection.execute("SET logidze.meta TO DEFAULT;") else pg_set_meta_param(prev_meta) end end + + def call_block_in_meta_context + prev_meta = current_meta + + meta_stack.push(meta) + + pg_set_meta_param(current_meta) + result = block.call + + result + ensure + pg_reset_meta_param(prev_meta) + meta_stack.pop + end end - private_constant :MetaTransaction + private_constant :MetaWrapper + private_constant :MetaWithTransaction + private_constant :MetaWithoutTransaction end end From e3fb87054b0a2e998f84e10cf25872d5273658eb Mon Sep 17 00:00:00 2001 From: Oleg Kiviljov <oleg.kiviljov@gmail.com> Date: Tue, 26 Nov 2019 15:33:41 +0200 Subject: [PATCH 02/10] Extract duplicated methods into MetaWrapper class --- lib/logidze/meta.rb | 80 ++++++++++++++++++++++----------------------- 1 file changed, 39 insertions(+), 41 deletions(-) diff --git a/lib/logidze/meta.rb b/lib/logidze/meta.rb index 016d855d..4ed340d5 100644 --- a/lib/logidze/meta.rb +++ b/lib/logidze/meta.rb @@ -30,6 +30,13 @@ def initialize(meta, &block) @block = block end + def perform + return if block.nil? + return block.call if meta.nil? + + call_block_in_meta_context + end + def current_meta meta_stack.reduce(:merge) || {} end @@ -42,65 +49,48 @@ def meta_stack def encode_meta(value) connection.quote(ActiveSupport::JSON.encode(value)) end - end - - class MetaWithTransaction < MetaWrapper # :nodoc: - def perform - return if block.nil? - return block.call if meta.nil? - - ActiveRecord::Base.transaction { call_block_in_meta_context } - end - - private - - def pg_set_meta_param(value) - connection.execute("SET LOCAL logidze.meta = #{encode_meta(value)};") - end def pg_reset_meta_param(prev_meta) if prev_meta.empty? - connection.execute("SET LOCAL logidze.meta TO DEFAULT;") + pg_clear_meta_param else pg_set_meta_param(prev_meta) end end + end - def call_block_in_meta_context - prev_meta = current_meta - - meta_stack.push(meta) + class MetaWithTransaction < MetaWrapper # :nodoc: + private - pg_set_meta_param(current_meta) - result = block.call - pg_reset_meta_param(prev_meta) + def call_block_in_meta_context + ActiveRecord::Base.transaction do + begin + prev_meta = current_meta - result - ensure - meta_stack.pop - end - end + meta_stack.push(meta) - class MetaWithoutTransaction < MetaWrapper # :nodoc: - def perform - raise ArgumentError, "Block must be given" unless block + pg_set_meta_param(current_meta) + result = block.call + pg_reset_meta_param(prev_meta) - call_block_in_meta_context + result + ensure + meta_stack.pop + end + end end - private - def pg_set_meta_param(value) - connection.execute("SET logidze.meta = #{encode_meta(value)};") + connection.execute("SET LOCAL logidze.meta = #{encode_meta(value)};") end - def pg_reset_meta_param(prev_meta) - if prev_meta.empty? - connection.execute("SET logidze.meta TO DEFAULT;") - else - pg_set_meta_param(prev_meta) - end + def pg_clear_meta_param + connection.execute("SET LOCAL logidze.meta TO DEFAULT;") end + end + + class MetaWithoutTransaction < MetaWrapper # :nodoc: + private def call_block_in_meta_context prev_meta = current_meta @@ -115,6 +105,14 @@ def call_block_in_meta_context pg_reset_meta_param(prev_meta) meta_stack.pop end + + def pg_set_meta_param(value) + connection.execute("SET logidze.meta = #{encode_meta(value)};") + end + + def pg_clear_meta_param + connection.execute("SET logidze.meta TO DEFAULT;") + end end private_constant :MetaWrapper From b9c78b3c542d86a7ac02480e62ea80c2a22313c7 Mon Sep 17 00:00:00 2001 From: Oleg Kiviljov <oleg.kiviljov@gmail.com> Date: Tue, 26 Nov 2019 15:49:39 +0200 Subject: [PATCH 03/10] Add tests for transactional:false setting --- spec/integrations/meta_spec.rb | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/spec/integrations/meta_spec.rb b/spec/integrations/meta_spec.rb index 08741e77..8d334fb3 100644 --- a/spec/integrations/meta_spec.rb +++ b/spec/integrations/meta_spec.rb @@ -213,6 +213,28 @@ expect(subject.at_version(3).meta).to eq meta2 end end + + context "when transactional:false" do + it "resets meta setting after block finishes" do + # subject is a newly created user + Logidze.with_meta(meta, transactional: false) do + expect(subject.reload.meta).to eq meta + end + + # create another one and check that meta is nil here + expect(User.create!(name: "test", age: 10, active: false).reload.meta).to be_nil + end + + it "recovers after exception" do + ignore_exceptions do + Logidze.with_meta(meta, transactional: false) do + CustomUser.create! + end + end + + expect(subject.reload.meta).to be_nil + end + end end describe ".with_responsible" do From 37492c761d6142d4065212e7d1b4025cf0359c57 Mon Sep 17 00:00:00 2001 From: Oleg Kiviljov <oleg.kiviljov@gmail.com> Date: Wed, 27 Nov 2019 10:52:13 +0200 Subject: [PATCH 04/10] Refactor Meta.wrap_with method --- lib/logidze/meta.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/logidze/meta.rb b/lib/logidze/meta.rb index 4ed340d5..64daa851 100644 --- a/lib/logidze/meta.rb +++ b/lib/logidze/meta.rb @@ -4,9 +4,8 @@ module Logidze # :nodoc: # Provide methods to attach meta information module Meta def with_meta(meta, transactional: true, &block) - return MetaWithTransaction.wrap_with(meta, &block) if transactional - - MetaWithoutTransaction.wrap_with(meta, &block) + wrapper = transactional ? MetaWithTransaction : MetaWithoutTransaction + wrapper.wrap_with(meta, &block) end def with_responsible(responsible_id, transactional: true, &block) From 123f334c473502a52a7610ef04a6858f3f7b9bfd Mon Sep 17 00:00:00 2001 From: Oleg Kiviljov <oleg.kiviljov@gmail.com> Date: Wed, 27 Nov 2019 10:53:37 +0200 Subject: [PATCH 05/10] Raise ArgumentError if with_meta was called without block --- lib/logidze/meta.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/logidze/meta.rb b/lib/logidze/meta.rb index 64daa851..71ca2cf2 100644 --- a/lib/logidze/meta.rb +++ b/lib/logidze/meta.rb @@ -30,7 +30,7 @@ def initialize(meta, &block) end def perform - return if block.nil? + raise ArgumentError, 'Block must be given' unless block return block.call if meta.nil? call_block_in_meta_context From 3dc9ec9dc844cebc1f4be99b862cbdfa68607c9d Mon Sep 17 00:00:00 2001 From: Oleg Kiviljov <oleg.kiviljov@gmail.com> Date: Wed, 27 Nov 2019 10:54:25 +0200 Subject: [PATCH 06/10] Fix rubocop offense --- lib/logidze/meta.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/logidze/meta.rb b/lib/logidze/meta.rb index 71ca2cf2..558f1d77 100644 --- a/lib/logidze/meta.rb +++ b/lib/logidze/meta.rb @@ -30,7 +30,7 @@ def initialize(meta, &block) end def perform - raise ArgumentError, 'Block must be given' unless block + raise ArgumentError, "Block must be given" unless block return block.call if meta.nil? call_block_in_meta_context From 7bbc0117ec756dc40dfdc4b561dbd0ab34ed60f1 Mon Sep 17 00:00:00 2001 From: Oleg Kiviljov <oleg.kiviljov@gmail.com> Date: Fri, 29 Nov 2019 16:01:41 +0200 Subject: [PATCH 07/10] Extract call_block_in_meta_context method into MetaWrapper class --- lib/logidze/meta.rb | 43 ++++++++++++++----------------------------- 1 file changed, 14 insertions(+), 29 deletions(-) diff --git a/lib/logidze/meta.rb b/lib/logidze/meta.rb index 558f1d77..20772dc4 100644 --- a/lib/logidze/meta.rb +++ b/lib/logidze/meta.rb @@ -36,6 +36,19 @@ def perform call_block_in_meta_context end + def call_block_in_meta_context + prev_meta = current_meta + + meta_stack.push(meta) + + pg_set_meta_param(current_meta) + result = block.call + result + ensure + pg_reset_meta_param(prev_meta) + meta_stack.pop + end + def current_meta meta_stack.reduce(:merge) || {} end @@ -62,21 +75,7 @@ class MetaWithTransaction < MetaWrapper # :nodoc: private def call_block_in_meta_context - ActiveRecord::Base.transaction do - begin - prev_meta = current_meta - - meta_stack.push(meta) - - pg_set_meta_param(current_meta) - result = block.call - pg_reset_meta_param(prev_meta) - - result - ensure - meta_stack.pop - end - end + connection.transaction { super } end def pg_set_meta_param(value) @@ -91,20 +90,6 @@ def pg_clear_meta_param class MetaWithoutTransaction < MetaWrapper # :nodoc: private - def call_block_in_meta_context - prev_meta = current_meta - - meta_stack.push(meta) - - pg_set_meta_param(current_meta) - result = block.call - - result - ensure - pg_reset_meta_param(prev_meta) - meta_stack.pop - end - def pg_set_meta_param(value) connection.execute("SET logidze.meta = #{encode_meta(value)};") end From 3716831b1b3509cde45c268f318b9c2bc14a7626 Mon Sep 17 00:00:00 2001 From: Oleg Kiviljov <oleg.kiviljov@gmail.com> Date: Fri, 29 Nov 2019 17:31:27 +0200 Subject: [PATCH 08/10] Add changelog entry --- CHANGELOG.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6923d242..109a712d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,24 @@ + # Change log ## master (unreleased) +- PR [#143](https://github.com/palkan/logidze/pull/143) Add `:transactional` option to `#with_meta` and `#with_responsible` ([@oleg-kiviljov][]) + +Now it's possible to set meta and responsible without wrapping the block into a DB transaction. For backward compatibility `:transactional` option by default is set to `true`. + +Usage: + +```ruby +Logidze.with_meta({ip: request.ip}, transactional: false) do + post.save! +end +``` +or +```ruby +Logidze.with_responsible(user.id, transactional: false) do + post.save! +end +``` ## 0.11.0 (2019-08-15) From 4ea037563b51098fb5d3162489d034f88f64ac83 Mon Sep 17 00:00:00 2001 From: Oleg Kiviljov <oleg.kiviljov@gmail.com> Date: Fri, 29 Nov 2019 17:57:54 +0200 Subject: [PATCH 09/10] Add documentation about transactional option to README --- README.md | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index c1d3a434..3e94ed33 100644 --- a/README.md +++ b/README.md @@ -261,7 +261,13 @@ end Meta expects a hash to be passed so you won't need to encode and decode JSON manually. -**NOTE**: `.with_meta` wraps the block into a DB transaction (see https://github.com/palkan/logidze/issues/136). +By default `.with_meta` wraps the block into a DB transaction. That could lead to an unexpected behavior, especially, when using `.with_meta` within an around_action. To avoid wrapping the block into a DB transaction use `transactional: false` option. + +```ruby +Logidze.with_meta({ip: request.ip}, transactional: false) do + post.save! +end +``` ## Track responsibility (aka _whodunnit_) @@ -306,8 +312,13 @@ class ApplicationController < ActionController::Base end ``` -**NOTE**: `.with_responsible` wraps the block into a DB transaction (see https://github.com/palkan/logidze/issues/136). +By default `.with_responsible` wraps the block into a DB transaction. That could lead to an unexpected behavior, especially, when using `.with_responsible` within an around_action. To avoid wrapping the block into a DB transaction use `transactional: false` option. +```ruby +Logidze.with_responsible(user.id, transactional: false) do + post.save! +end +``` ## Disable logging temporary If you want to make update without logging (e.g., mass update), you can turn it off the following way: From cd37faba3044f3f2b3d27ad2674228ab1109264f Mon Sep 17 00:00:00 2001 From: Oleg Kiviljov <oleg.kiviljov@gmail.com> Date: Fri, 29 Nov 2019 18:40:37 +0200 Subject: [PATCH 10/10] Add nickname shortcut to changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 109a712d..27171282 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -302,3 +302,4 @@ This is a quick fix for a more general problem (see [#59](https://github.com/pal [@dmitrytsepelev]: https://github.com/DmitryTsepelev [@zocoi]: https://github.com/zocoi [@duderman]: https://github.com/duderman +[@oleg-kiviljov]: https://github.com/oleg-kiviljov