Add support for setting meta without transaction block

[oleg-kiviljov]

What is the purpose of this pull request?

Adds support for setting meta without the database transaction block.

What changes did you make? (overview)

I've added the possibility to specify if meta information should be set within the transaction block or not. For this I have added the optional argument to the .with_meta method and split the MetaTransaction class into 3 separate classes, MetaWrapper, MetaWithTransaction and MetaWithoutTransaction.

Is there anything you'd like reviewers to focus on?

...

Checklist

• I've added tests for this change
• I've added a Changelog entry
• I've updated a documentation (Readme)

[oleg-kiviljov]

Hi @palkan, can you please tell me which tests do I need to write for added functionality? I'm only making my first steps in the testing world but very eager to learn.

[palkan]

can you please tell me which tests do I need to write for added functionality?

The main thing we want to check here is that meta is not leaking outside the .with_meta block.

So, we can add the following scenario to meta_spec:

# pseudo-code
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
    # we already have a similar example
    ignore_exceptions do
      Logidze.with_meta(meta, transactional: false) do
        CustomUser.create!
      end
    end

    expect(subject.reload.meta).to be_nil
  end
end

[palkan]

I think, we can move this to the base class (MetaWrapper).

[palkan]

Let's extract this into a pg_clear_meta_param method and move pg_reset_meta_param to the base class.. Thus, we'll only have to override two methods here.

[palkan]

Let's move this check to the base class. Don't know why we don't have it in the current implementation. @DmitryTsepelev ?

And we can make this perform the base implementation and only override call_block_in_meta_context in the transactional subclass, not #perform.

[DmitryTsepelev]

Fair point, it does not make any sense to call .with_meta without a block

[oleg-kiviljov]

How does this look now?

[palkan]

Let's do the following:

• Implement call_block_in_meta_context in the base class:

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

• Here we can override this method by wrapping super with transaction:

def call_block_in_meta_context
  ActiveRecord::Base.transaction { super }
end

• Drop call_block_in_meta_context in MetaWithoutTransaction (the current implementation is not complete — we should take meta_stack into account; the base implementation above would work in non-transactional case without any change)

[palkan]

Looks good 👍

Let's add a change log entry and mention this feature in the Readme, and we're done!

[palkan]

One more thing: please, add the value for your nickname's shortcut to the end of this file.

[palkan]

Great! Thanks for your help!