logidze icon indicating copy to clipboard operation
logidze copied to clipboard

Add ability to set responsible for the whole lifetime of the connection

Open Yegorov opened this issue 3 years ago • 2 comments

What is the purpose of this pull request?

Implement functionality described in this issues (202)

What changes did you make? (overview)

I added 2 methods in Logidze::Meta:

  • with_responsible! - for set responsible for the whole lifetime of the connection
  • clear_responsible! - clear responsible for the whole lifetime of the connection

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

Yes, maybe inheritance from MetaWithTransaction class, is wrong?

If there are any comments and suggestions, then I am ready to fix them.

Checklist

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

Yegorov avatar Sep 03 '21 07:09 Yegorov

maybe inheritance from MetaWithTransaction class, is wrong?

Yeah, that doesn't seem clear; maybe, we can instead enhance #perform method and a new method, say, set_toplevel_meta when block.nil?. And we can call pg_set_meta_param there.

Also, the current approach doesn't take into account a situation, when we call with_responsible! within a with_responsible(&block) (in this case, we do not clear the meta but revert to the previous value).

I suggest not allowing to call with_responsible! within another meta block, i.e.:

Logidze.with_meta(x) do
  Logidze.with_meta!(y) #=> raises error
end

On the other hand, setting multiple values should be possible:

Logidze.with_meta!(x) #=> current_meta == x
Logidze.with_meta!(y) #=> current_meta == x.merge(y)

palkan avatar Sep 03 '21 16:09 palkan

Forgot to say: thanks for the PR! Really appreciate the help 🙂

palkan avatar Sep 03 '21 16:09 palkan

Hey folks, a quick question/suggestion why don't leverage ActiveSupport::CurrentAttributes instead?

It works in a console and in a regular HTTP request to the app (I have a use case when I want to set metadata for the whole request after the user is authenticated).

toydestroyer avatar Oct 01 '22 05:10 toydestroyer

why don't leverage ActiveSupport::CurrentAttributes instead?

I think, mostly to avoid depending on Rails where possible. Although it's not the case right now, but we still want to support other ORM (Sequel, Rom-rb) some day, and thus, trying to not use Rails-specific concepts would help us to do that.

And we need to synchronize meta values between DB and Ruby, so just using Current is not enough anyway.

palkan avatar Oct 18 '22 01:10 palkan

Closed as stale

palkan avatar Jan 04 '23 00:01 palkan