1. 8
  1.  

  2. 5

    I think DHH probably opened himself up to criticisms like this by choosing really terrible example use-cases. Just as with the default_scope considered harmful conventional wisdom, these points are true in the general case but there are situations where it is legitimately a good choice.

    Let’s ignore DHH’s sloppy use-case presentation and look deeper.

    One of the points against thread-local variables is:

    Not cleaning up thread-locals might be an issue if your framework reuses threads

    Let’s look at the very first sentence of the documentation:

    Abstract super class that provides a thread-isolated attributes singleton, which resets automatically before and after [each] request.

    Why bring up this criticism of the thread-local pattern when this class was specifically designed to solve it?

    This combined with the fact that his quotation includes the Law of Demeter, which he immediately reiterates as if it weren’t there, suggests to me that perhaps this post was put together in haste without very much thought.

    Now to the meat of his argument: that a per-request setting is best done within the controller.

    A better implementation would be a current_user method which evaluates its find when it is called. You’ll see this pattern in a lot of Rails applications.

    def current_user
      @current_user ||= User.find(cookies.signed[:user_id])
    end
    

    The entire point of doing this is for situations where a simple helper method wouldn’t work. Ryan got sidetracked by bad examples, but failed to consider situations where this would be useful.

    Here, DHH is implicitly linking the message’s creator to the Current.user by using the default option on belongs_to. I believe that this violates the MVC layer abstraction. The Current.user is just “magically” available in the model, with absolutely no context of how it got there in the first place.

    Yes, explicit assignment of a relationship is preferable – except when it isn’t. Consider:

    Depending on developers to remember to explicitly include this information defeats the purpose. This is where global, per-request state is necessary. And having a well-defined place to do it, that automatically takes care of clearing data after the request, is a welcome addition.

    One point that Ryan fails to address is: when developers run into a situation where some form of global state is necessary, they frequently implement it in a way that is not thread-safe. In fact thread-safety is kind of tricky in Ruby. Global state should be avoided if possible, but when you need it it should be done safely. That is why this exists. In fact, this is explicitly stated in the docs:

    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.

    1. 2
      • An auditing framework such as paper_trail that records who initiated changes
      • A multitenancy system that isolates data through scoping

      But this is neither :)

      This is where global, per-request state is necessary

      Doesn’t that sound kind of contradictory? Global vs Request.

      Even if you make it global, we’re talking about data that’s specific to the request being handled. Why not just add it into the request object instead? I’m not familiar with Rails, but don’t web frameworks generally have something like that?

      The paper trail and multi-tenancy stuff could be handled with something like Python’s WSGI middleware, or Java’s Servlet Filters, right? Something that can modify a request before it gets handled by the application code.

      There’s no need for global state in those cases.

      And having a well-defined place to do it, that automatically takes care of clearing data after the request, is a welcome addition.

      Automatic clearing of data is only a benefit if there’s data that needs to be cleared. In other words, it’s not a real benefit when you can just not use global state.

      1. 1

        But this is neither :)

        This is a building block to aid the in development of such features.

        Doesn’t that sound kind of contradictory? Global vs Request.

        A “global” object in Ruby doesn’t seem to mean the same thing as how you are using it. In a forking server, the global environment is created specifically to service a request, and then destroyed. In a threaded server the environment survives, but you would never want state to survive between requests. The only global state that is remotely safe to use is short-lived, and destroyed after the request is finished.

        Even if you make it global, we’re talking about data that’s specific to the request being handled. Why not just add it into the request object instead? I’m not familiar with Rails, but don’t web frameworks generally have something like that?

        The request object is only accessible to code executing in the controller. Code that can access the request object has no need of global state like this, and should be using a helper method like TFA suggests.

        The paper trail and multi-tenancy stuff could be handled with something like Python’s WSGI middleware, or Java’s Servlet Filters, right? Something that can modify a request before it gets handled by the application code.

        That’s precisely how it already does work, but you still have to store that value somewhere. This is simply providing a place to put it that is safely reset before/after each request, and stores its data in a thread-safe manner.

        There’s no need for global state in those cases.

        I think this essentially exists solely because of how ActiveRecord callbacks work. They execute outside the context of the code that invokes them, and have no knowledge of the request state.

        1. 1

          I think this essentially exists solely because of how ActiveRecord callbacks work. They execute outside the context of the code that invokes them, and have no knowledge of the request state.

          That sounds like an underlying problem. But it seems I don’t know enough about Rails to really comment further, so let’s leave it at that.

    2. 4

      From the article: Rails has enough magic in it and it certainly doesn’t need any more.

      That’s Rails’ entire niche! It more or less defined what a magical web framework could look like. The magic might make reasoning about a complex codebase difficult, but the project has been quite consistent in its approach to these sorts of tradeoffs. Omakase, and the chef is kind of bonkers but is clearly a genius.

      1. 1

        The “x is considered harmful” meme is considered harmful.

        1. 1

          belongs_to :creator, default: -> { Current.user } …I believe that this violates the MVC layer abstraction.

          I think so, too. In a web app, “current user” is a request-specific concept; it’s “the user for the current request.”

          Controllers are in charge of requests. If information about the current request is needed in the model, it should be explicitly handed from the controller to the model.

          A model that can only be used within the context of an HTTP request is a malformed model.