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

ActiveSupport::CurrentAttributes provides a thread-isolated attributes singleton #29180

Merged
merged 18 commits into from May 26, 2017

Conversation

Projects
None yet
@dhh
Member

dhh commented May 22, 2017

Abstract super class that provides a thread-isolated attributes singleton.
Primary use case is keeping all the per-request attributes easily available to the whole system.

The following full app-like example demonstrates how to use a Current class to
facilitate easy access to the global, per-request attributes without passing them deeply
around everywhere:

# app/models/current.rb
class Current < ActiveSupport::CurrentAttributes
  attribute :account, :user
  attribute :request_id, :user_agent, :ip_address 
  
  resets { Time.zone = nil }
  
  def user=(user)
    super
    self.account = user.account
    Time.zone = user.time_zone
  end
end

# app/controllers/concerns/authentication.rb
module Authentication
  extend ActiveSupport::Concern

  included do
    before_action :set_current_authenticated_user
  end

  private
    def set_current_authenticated_user
      Current.user = User.find(cookies.signed[:user_id])
    end
end

# app/controllers/concerns/set_current_request_details.rb
module SetCurrentRequestDetails
  extend ActiveSupport::Concern

  included do
    before_action do
      Current.request_id = request.uuid
      Current.user_agent = request.user_agent
      Current.ip_address = request.ip
    end
  end
end  

class ApplicationController < ActionController::Base
  include Authentication
  include SetCurrentRequestDetails
end

class MessagesController < ApplicationController
  def create
    Current.account.messages.create(message_params)
  end
end

class Message < ApplicationRecord
  belongs_to :creator, default: -> { Current.user }
  after_create { |message| Event.create(record: message) }
end

class Event < ApplicationRecord
  before_create do
    self.request_id = Current.request_id
    self.user_agent = Current.user_agent
    self.ip_address = Current.ip_address
  end
end

A word of caution: It's easy to overdo a global singleton like Current and tangle your model as a result.
Current should only be used for a few, top-level globals, like account, user, and request details.
The attributes stuck in Current should be used by more or less all actions on all requests. If you start
sticking controller-specific attributes in there, you're going to create a mess.

@dhh dhh added the activesupport label May 22, 2017

@dhh dhh added this to the 5.2.0 milestone May 22, 2017

dhh added some commits May 22, 2017

@kaspth

kaspth approved these changes May 22, 2017

Some minor details, but 👍 on the concept.

end
def person=(person)
attributes[:person] = person

This comment has been minimized.

@kaspth

kaspth May 22, 2017

Member

Reading this I'd think super would just work here. Should we make that work or too much of a hassle?

This comment has been minimized.

@dhh

dhh May 22, 2017

Member

Do take a swing and see what contortions are needed, if you'd like 👍

This comment has been minimized.

@dhh

dhh May 22, 2017

Member

Actually, not sure about that. We declare the attributes in the same class. They didn't come from super. I think super is too clever.

This comment has been minimized.

@matthewd

matthewd May 22, 2017

Member

We make it work in AR/AMo, where they similarly belong to the current class.

It just means that instead of defining the built-in methods in the class directly, we put them into an anonymous module included for that purpose.

It's cleverer than we need to be, but I think it's simple enough that it's worth the gain in "do what I mean" expressiveness. It'll help people who do expect it to work, and I can't think of any way it'd manage to unpleasantly surprise someone who doesn't.

This comment has been minimized.

@kaspth

kaspth May 22, 2017

Member

Pushed a commit for this. Opted to not define the singleton methods within the module, since those should just be left be by users.

Person = Struct.new(:name, :time_zone)
class Current < ActiveSupport::CurrentAttributes
attribute :world, :account, :person

This comment has been minimized.

@kaspth

kaspth May 22, 2017

Member

Could this be conflated with attribute from Active Model and Active Record?

This comment has been minimized.

@dhh

dhh May 22, 2017

Member

It's the same concept, so I think it's good that it's using the same name.

This comment has been minimized.

@kaspth

kaspth May 22, 2017

Member

I still think we could confuse people into thinking that the Active Model attributes type casting stuff is available here.

So since these are mostly attributes in name only — there's no dirty tracking, aliasing or other goodies from Active Model — perhaps we should just think of them as accessors?

class Current < ActiveSupport::CurrentAccessors
  accesses :world, :account, :person # Or maybe even `attr_accessor`?
end
Person = Struct.new(:name, :time_zone)
class Current < ActiveSupport::CurrentAttributes

This comment has been minimized.

@kaspth

kaspth May 22, 2017

Member

Should we namespace this and Person so it won't clash with other tests?

This comment has been minimized.

@kaspth

kaspth May 22, 2017

Member

Done.

# end
# end
# end
def expose(exposed_attributes)

This comment has been minimized.

@kaspth

kaspth May 22, 2017

Member

Perhaps make this **exposed_attributes so we get the type features for free (and the method won't NoMethodError on keys.

This comment has been minimized.

@kaspth

kaspth May 22, 2017

Member

On the other hand why not just do this:

def expose(exposed_attributes)
  old_attributes, @attributes = @attributes, exposed_attributes
  yield
ensure
  @attributes = old_attributes
end

This comment has been minimized.

@dhh

dhh May 22, 2017

Member

If we don't call the writers, we don't trigger stuff like Time.zone =.

# around everywhere:
#
# # app/services/current.rb
# require 'active_support/current_attributes'

This comment has been minimized.

@matthewd

matthewd May 22, 2017

Member

active_support/all should include this, so the require won't be necessary.

This comment has been minimized.

@kaspth

kaspth May 22, 2017

Member

I just set CurrentAttributes up as an autoload in the top level active_support.rb mirroring ActiveSupport::Concern.

@cristianbica

This comment has been minimized.

Member

cristianbica commented May 22, 2017

Since thread_mattr_accessor appeared I'm using something like:

module Current
  thread_mattr_accessor :user

  module_function
    def reset
      Current.user = nil
    end
end

and in controllers I'm doing the hygiene:

class ApplicationController < ActionController::Base
  around_action :reset_current

  def reset_current
    Current.reset
    yield
  ensure
    Current.reset
  end
end

What would be nice is if we provide a fire-and-forget functionality which will do the resetting for each request. I'm thinking either a class flag (resets_on_action_controller_request ... or similar) or a subclass in actionpack (ActionController::CurrentRequestAttributes). Implementation-wise I would subscribe to start_processing.action_controller and process_action.action_controller.

kaspth added some commits May 22, 2017

Move stubs into test namespace.
Thus they won't conflict with other Current and Person stubs.
@kaspth

This comment has been minimized.

Member

kaspth commented May 22, 2017

@dhh was toying with the idea of generating app/services/current.rb by default. But we'd most likely just generate an app/controllers/concerns/current/reset.rb auto included in ApplicationController and a setup { Current.reset } in test_helper.rb, rather than hooking into log subscribing.

@matthewd

This comment has been minimized.

Member

matthewd commented May 22, 2017

I believe we'll be able to reset fully automatically through the executor.

@kaspth

This comment has been minimized.

Member

kaspth commented May 22, 2017

@matthewd ahh, right, of course!

@kaspth

This comment has been minimized.

Member

kaspth commented May 22, 2017

Right, we could keep track of the subclasses:

class ActiveSupport::CurrentAttributes
  mattr_accessor(:subclasses) { [] }

  class << self
    def inherited(subclass)
      subclasses << subclass
    end

    def reset_all
      subclasses.each(&:reset)
    end
  end
end

Then schedule an ActiveSupport::CurrentAttributes.reset_all executor hook in Active Support's railtie.

kaspth and others added some commits May 22, 2017

Support super in attribute methods.
Define instance level accessors in an included module such that
`super` in an overriden accessor works, akin to Active Model.
Spare users the manual require.
Follow the example of concerns, autoload in the top level Active Support file.
@maclover7

This comment has been minimized.

Member

maclover7 commented May 24, 2017

Two quick things:

  1. This seems like something that is fairly app specific, and something that could live in a gem/outside of core elsewhere. As a sidenote to this, I'm more in favor of current_user and similar controller methods, rather than this globals approach.

  2. Is the idea to provide an API similar to that of the Active Record attributes API? I think using the attribute macro might trip people up.

@guilleiguaran

This comment has been minimized.

Member

guilleiguaran commented May 25, 2017

I'm more in favor of current_user and similar controller methods, rather than this globals approach

I think both approach aren't incompatible at all, in the app I'm working currently we have current_user and similar controller methods but we have a couple of globals depending on the location (e.g Region.current) of the user making the request and many business rules depending on this. We reference those globals in many models, views, controllers, helpers, mailers, concerns and using the global definitely makes the code cleaner than passing the current_region around all the classes of the stack.

As @dhh said this should be used carefully and in a few cases.

@guilleiguaran

guilleiguaran approved these changes May 25, 2017 edited

👍 but would like to see the recommendation by @matthewd / @kaspth in #29180 (comment) done before of merge

@dhh

This comment has been minimized.

Member

dhh commented May 25, 2017

When does that executor run? Want to make sure we're clearing both before and after a request starts, just to ensure nothing is leaking anywhere.

Also, I think we can make it explicit that a CurrentAttributes class participates in this auto-resetting scheme. Maybe just have resets_on_request or something like that that adds it to the auto reset pool.

@dhh

This comment has been minimized.

Member

dhh commented May 25, 2017

Jon,

  1. I don't see anything app specific about tracking current user/account/region/request_id. Every app I've worked on has had some homegrown way of doing this.
  2. Attribute is a generic name, like Object. I don't think there's any expectation of that having a global definition based on an implementation in AR.
@cristianbica

This comment has been minimized.

Member

cristianbica commented May 25, 2017

@dhh executor runs before and after the request in a middleware

@cristianbica

This comment has been minimized.

Member

cristianbica commented May 25, 2017

@dhh IMO this is great addition to the framework. I'm using everywhere class Current and thread_mattr_accessor but I need to do the cleanup in controllers.
As probably @maclover7 is trying to say there are some features that I believe are too much. The parts that I don't think I'm going to use are:

  • bidelegate ... I don't any real usage for it; I wouldn't instantiate the Current class; I would even add a delegate_missing_to :instance and create the attributes methods only on the instance
  • set ... I don't have a use case for it; Plus I would remove your example with using Current in jobs :)
  • resets callbacks; seems overkill for the object. A reset method that people can override in subclasses and call super should be more than enough. Callbacks are useful if people want to have the opportunity to act on reset from outside the object.
Automatically reset every instance.
Skips the need for users to actively embed something that resets
their CurrentAttributes instances.
test "current customer is assigned and cleared" do
get "/customers/set_current_customer"
assert_equal 200, last_response.status
assert_match(/david, Copenhagen/, last_response.body)

This comment has been minimized.

@kaspth

kaspth May 25, 2017

Member

It would be nice if I could figure out how to test that the customer and Time.zone is reset after the request as well. Any bright ideas? 😄

# Reset all attributes. Should be called before and after actions, when used as a per-request singleton.
def reset
run_callbacks :reset do
self.attributes = {}

This comment has been minimized.

@kaspth

kaspth May 25, 2017

Member

@dhh the assignment here means that reset's return value is that empty hash. I found it a little odd when running it from the console. If that's okay to you, I won't touch it.

This comment has been minimized.

@dhh

dhh May 26, 2017

Member

Doesn't bother me. We make no promises about the return value. Although, I suppose we could return self so you can chain it. But don't have a use case for that.

matthewd and others added some commits May 25, 2017

Delegate all missing methods to the instance
This allows regular `delegate` to serve, so we don't need bidelegate.
Properly test resetting after execution cycle.
Also remove the stale puts debugging.
@kaspth

This comment has been minimized.

Member

kaspth commented May 26, 2017

Got the auto-reset setup!

One thing that bothers me about CurrentAttributes is that they aren't attributes in the Active Model sense, there's no dirty tracking, aliasing etc. It's true they're roughly the same concept, so some overlap is warranted.

But these attributes are more purely about broadening the places where they're accessible within an app. They're accessors.

How about:

class Current < ActiveSupport::CurrentAccessors
  accesses :world, :account, :person # Or maybe even `attr_accessor`?
end
@dhh

This comment has been minimized.

Member

dhh commented May 26, 2017

Awesome, @kaspth! The attributes thing doesn't bother me the slightest. Accessors is just beating around the bush, and imo less clear. Ruby itself uses attr_accessor, which is short for attribute_accessor. Here, we're actually declaring the attributes.

Anyway, this is all looking good to ship. I'll update the docs and merge. Much appreciate the API debate and the implementation upgrades 🙏👏🎉

setup do
build_app
app_file "app/services/current.rb", <<-RUBY

This comment has been minimized.

@bogdanvlviv

bogdanvlviv May 26, 2017

Contributor

For consistency

-app_file "app/services/current.rb", <<-RUBY
+app_file "app/models/current.rb", <<-RUBY

https://github.com/rails/rails/pull/29180/files#diff-3c3c0f647bc4702f9453c173a707aa06R10

end
def compute_attributes(keys)
keys.collect { |key| [ key, public_send(key) ] }.to_h

This comment has been minimized.

@bogdanvlviv

bogdanvlviv May 26, 2017

Contributor
-keys.collect { |key| [ key, public_send(key) ] }.to_h
+keys.collect { |key, _| [ key, public_send(key) ] }.to_h

This comment has been minimized.

@dhh

dhh May 26, 2017

Member

Why? We're iterating over just an array of the keys, not a full hash.

This comment has been minimized.

@dhh dhh merged commit 24a8644 into master May 26, 2017

2 of 3 checks passed

continuous-integration/travis-ci/push The Travis CI build failed
Details
codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dhh dhh deleted the current-attributes branch May 26, 2017

@kaspth

This comment has been minimized.

Member

kaspth commented May 27, 2017

Ruby itself uses attr_accessor, which is short for attribute_accessor

Ah yeah, good point. 👍 on sticking with attribute.

I noticed that storing instances in Thread.current means CurrentAttributes can't support dev reloading. I'm looking into clearing those on a reload to support that.

@clairity

This comment has been minimized.

clairity commented Jun 17, 2017

@kaspth do you have any idea on timeline for this feature to be released? i'm currently using a hacky homegrown implementation that i'd love to replace right away with this implementation (especially since i'm working on some related changes in my codebase right now).

@kaspth

This comment has been minimized.

Member

kaspth commented Jun 19, 2017

Rails 5.2 sometime this year.

@radar

This comment has been minimized.

Contributor

radar commented Jun 22, 2017

I am very opposed to this feature but after what happened last time I opposed a change on Rails I am not willing to fight this fight. It is a massive footgun and you're doing a massive disservice to the framework to introduce a global state.

Instead, I have stated my case in a blog post: http://ryanbigg.com/2017/06/current-considered-harmful. Afterall, this is what DHH encouraged last time.

If you really want to add oomph to the outrage engine, you could also post a medium article about how this is making you consider not using Rails (or was the final straw that made you leave). Both are popular choices for discharging bile, and I encourage them over reopening old tickets.

Choose to interpret it as bile if you wish. I meant it in a well-meaning fashion. Everyone's entitled to their own opinions, right?

@dhh

This comment has been minimized.

Member

dhh commented Jun 22, 2017

@zernie

This comment has been minimized.

zernie commented Jun 24, 2017

@dhh I agree that we can enjoy the rest of the menu together. But it's the overall direction of the framework that a lot of people are worried about; meanwhile the rest of the programming community shies away from bad design patterns, Rails happily embraces them.

@dhh

This comment has been minimized.

Member

dhh commented Jun 24, 2017

Rails has been embracing patterns that some people consider to be "bad" since inception, yet we are somehow still here and in better shape than ever. I'm going to go with yesterday's weather and predict that the inclusion of Current will not change that conclusion one iota either.

Anyway, enjoy the rest of the menu!

@ahazem

This comment has been minimized.

ahazem commented Jul 7, 2017

@clairity We have extracted the feature into a gem: https://github.com/coup-mobility/activesupport-current_attributes. Can be used with Rails >= 4.2.

@dhh

This comment has been minimized.

Member

dhh commented Jul 7, 2017

@clairity

This comment has been minimized.

clairity commented Jul 7, 2017

yes thanks @ahazem! i dropped it into my app and it's looking good so far!

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jul 7, 2017

I opened an issue to track the required license change in that gem coup-mobility/activesupport-current_attributes#2

@pavel-jurasek-hypofriend-de

This comment has been minimized.

pavel-jurasek-hypofriend-de commented Jul 7, 2017

@rafaelfranca @dhh please check coup-mobility/activesupport-current_attributes@1cd6ffc this commit should fix copyright issue please just confirm!

ruby_rails_is_love

@ahazem

This comment has been minimized.

ahazem commented Jul 7, 2017

@dhh Thanks for the pointer! The license file was auto-generated by bundler gem, which picked up my name from git config. We have updated the file.

@clairity Glad you find it helpful. :)

@kaspth

This comment has been minimized.

Member

kaspth commented Jul 9, 2017

@ahazem you might want to pull in its latest incarnation from the master branch. I've made some extra commits since merge. https://github.com/rails/rails/commits/master/activesupport/lib/active_support/current_attributes.rb :)

@ahazem

This comment has been minimized.

ahazem commented Jul 10, 2017

@kaspth Nice, will pull them in! Thanks for pointing that out.

@deivid-rodriguez deivid-rodriguez referenced this pull request Jul 5, 2018

Merged

AJAX authorization modals #3753

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment