logidze
logidze copied to clipboard
Add ability to set responsible for the whole lifetime of the connection
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)
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)
Forgot to say: thanks for the PR! Really appreciate the help 🙂
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).
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.
Closed as stale