palkan / logidze

Database changes log for Rails
MIT License
1.59k stars 74 forks source link

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

Closed Yegorov closed 1 year ago

Yegorov commented 3 years ago

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:

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

palkan commented 3 years ago

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 commented 3 years ago

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

toydestroyer commented 1 year ago

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).

palkan commented 1 year ago

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 commented 1 year ago

Closed as stale