Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

Yegorov
Copy link

@Yegorov Yegorov commented Sep 3, 2021

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

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

@palkan
Copy link
Owner

palkan commented Sep 3, 2021

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
Copy link
Owner

palkan commented Sep 3, 2021

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

@toydestroyer
Copy link

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
Copy link
Owner

palkan commented Oct 18, 2022

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
Copy link
Owner

palkan commented Jan 4, 2023

Closed as stale

@palkan palkan closed this Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants