A shortcut to setup controller environment #18546

brainopia · 2015-01-15T20:35:30Z

Related to #18409

Some reiteration first.
What we looking for is an easy way to render templates outside of controllers.

View templates are rendered as part of request-response cycle. There are easy cases when templates would not depend on anything except ivars from controller, but a lot of templates use helpers that depend on request environment in one form or another.

Sometimes helpers depend on stuff that's easy to setup, like a scheme, host or script name. But request environment is also used by gems to hold transient data associated with current request (e.g., helpers from devise would raise an exception if request environment hasn't been prepared by its middleware).

Therefore two use-cases arise.

Ability to render templates in custom request environment.
Ability to render templates in an environment that was passed through middleware stack.

This PR deals with the first part.

# this would setup following environment keys: REQUEST_METHOD, SERVER_NAME, SERVER_PORT, QUERY_STRING, PATH_INFO, rack.url_scheme, HTTPS, rack.input
FooController.setup_for(_uri_).render_to_string _render_options_  

# or more detailed setup
controller = FooController.setup_for _uri_, script_name: 'bar', method: 'post', 'env_key' => 'baz'
... # configure controller instance a bit
controller.render_to_string _render_options_

Where _render_options_ could contain assigns: { a: 'b' } to set ivars.

/cc @dhh

brainopia · 2015-01-15T20:51:18Z

The API is supported for arbitrary controller since rendering depends on lookup_context and view_context (both can differ between controllers (e.g., different helpers because of helper_method or access to different views because of custom view_paths)).

brainopia · 2015-01-18T00:11:54Z

I've added ActionController::Renderer, example usage can be seen in tests https://github.com/brainopia/rails/blob/action_view_render/actionpack/test/controller/renderer_test.rb

dhh · 2015-01-18T18:19:38Z

This is great, Ravil! I’m wondering if we shouldn’t actually also pipe ApplicationController.render => ApplicationController.renderer.render. I think that’ll be the default case.

On Jan 17, 2015, at 4:12 PM, Ravil Bayramgalin notifications@github.com wrote:

I've added ActionController::Renderer, API examples can be seen in tests https://github.com/brainopia/rails/blob/action_view_render/actionpack/test/controller/renderer_test.rb https://github.com/brainopia/rails/blob/action_view_render/actionpack/test/controller/renderer_test.rb
—
Reply to this email directly or view it on GitHub #18546 (comment).

brainopia · 2015-01-19T02:11:40Z

Thanks for guiding, David!

ApplicationController.render looks good, I've added this shortcut.

I have a few doubts over ApplicationController.renderer.new(...).render instead of ApplicationController.renderer(...).render. Maybe the latter is a bit better? Although the former has an advantage with ability to inherit class from ApplicationController.renderer (a rare use-case). What would you prefer?

dhh · 2015-01-19T21:41:25Z

Sweet. I like the idea that ApplicationController.renderer refers to a class, not a method. Then that class has a class-method #render for the common case, but if you need specialization, you can instantiate that class and do that. So I think that's good. Especially since we also have the #defaults approach.

dhh · 2015-01-19T21:42:51Z

actionpack/lib/action_controller/renderer.rb

+      def for(controller)
+        Class.new self do
+          self.controller = controller
+          self.defaults = {


Let's add config/initializers/application_controller_renderer.rb that shows how to use this on setup. Specifically the way to toggle https depending on the env and setting the http_host.

dhh · 2015-01-19T21:45:55Z

Once the few coding notes have been taken care of, it looks like we're ready to merge? Lovely work, @brainopia.

brainopia · 2015-01-20T11:06:59Z

David, thank you for code style checking! I've updated code and I'll make sure to reflect it in future pull-request. If there are more tidbits to follow I'd be happy to.

I'd like your advice on what would you prefer to see in the initializer.

# option 1
class ApplicationRenderer < ApplicationController.renderer
  # customize env statically
  defaults[:https] = false
  defaults[:host] = 'example.org'

  def initialize(env = {})
    # customize env dynamically
    super
  end

  def render(*args)
    # customize render args
    super
  end
end

# option 2
ApplicationController.renderer.class_eval do
  self.defaults = Rack::MockRequest.env_for('https://my.tld')
end

Should we create a new renderer class or redefine an existing. It makes sense to support ApplicationController.render shortcut, therefore redefining ApplicationController.renderer wins in that aspect compared to inheriting from it. But may be reopening class with class_eval looks a bit inappropriate to show off in the initializer?

And should we show all possible ways to customize renderer behavior (as in option 1) or defaults is enough (as in option 2).

Should we use simple defaults update (as in option 1) or show off Rack::MockRequest.env_for?

As an alternative to initializer we can add a line (commented out by default) directly to application_controller.rb to show an example:

class ApplicationController < ActionController::Base
  # renderer.defaults.merge! https: true, host: 'my.tld' <====

  # Prevent CSRF attacks by raising an exception.
  # For APIs, you may want to use :null_session instead.
  protect_from_forgery with: :exception
end

And of course thorough examples in documentation would alleviate this problem. I'm going to push docs soon (although english is not my native language, so it will look a bit (or a lot, I can't judge) crusty).

rafaelfranca · 2015-01-20T17:13:04Z

actionpack/lib/action_controller/metal.rb

+      to_a
+    end
+
+    def set_request!(request)


Is set_request! supposed to be used by users?

Generally, no. Should I mark it as #:nodoc:, would it be enough?

Should I mark it as #:nodoc:, would it be enough?

Yes. 👍

brainopia · 2015-01-21T00:44:59Z

@rafaelfranca I've updated code to reflect your notes, thank you!
The only thing after latest change that makes me a bit uneasy - https://github.com/rails/rails/pull/18546/files#diff-568a4685d0dc681235508ba36884cbf9R32, it's a bit harder to redefine view_context.

@dhh The question above regarding initializers (#18546 (comment)) is still actual.

rafaelfranca · 2015-01-21T01:06:16Z

💣 I forgot that view_context could be overridden and it is even documented. I quick search in GitHub show that people is using it https://github.com/search?p=1&q=%22def+view_context%22&ref=searchresults&type=Code&utf8=%E2%9C%93. So I guess we can't change it 😢.

brainopia · 2015-01-21T11:49:04Z

@rafaelfranca yeah, I've reverted to previous :assigns implementation 🍰

brainopia · 2015-01-21T20:03:22Z

@dhh here you go 😄

rafaelfranca · 2015-01-21T20:34:12Z

actionpack/lib/action_controller/renderer.rb

+    class << self
+      delegate :render, to: :new
+
+      def for(controller)


Is this method public API?

rafaelfranca · 2015-01-21T20:35:53Z

Awesome work on this @brainopia

Add `ActionController::Metal#set_request!` to set a request on controller instance without calling dispatch.

To have an easier way to setup a controller instance with custom environment

Render arbitrary templates outside of controller actions

brainopia · 2015-01-21T22:06:13Z

@rafaelfranca Thank you for the help! I think now I've covered all bases 😄

rafaelfranca · 2015-01-21T22:09:54Z

Cool! Now it is good to merge. @dhh want to do the last review?

dhh · 2015-01-22T01:04:03Z

Looks good to me! Great work, guys.

A shortcut to setup controller environment

rafaelfranca · 2015-01-22T14:09:33Z

❤️ 💚 💙 💛 💜

brainopia · 2015-01-22T14:10:59Z

🙇 🙇 🙇

egilburg · 2015-01-22T23:26:06Z

❤️ this could make it easier for our use case to support poll-based async renders for really big api queries

egilburg · 2015-01-22T23:29:12Z

actionpack/lib/action_controller/metal/rendering.rb

+      # Returns a renderer class (inherited from ActionController::Renderer)
+      # for the controller.
+      def renderer
+        @renderer ||= Renderer.for(self)


is/should this be thread safe?

Yes, this will return a class to create a new instances of renderer from (so it doesn't matter if it's shared). And even if you use one instance of renderer across threads it's still be ok, since actual rendering happens always on a new controller instance.

This is not thread safe. Two unrelated instances can easily be created here. It should almost certainly be wrapped in a mutex.

There is no point.

Renderer.for - returns a factory for current controller. It won't matter if it's the same factory object or different one for the same controller.

@brainopia what if two different controllers use this code, but share the same thread and thus the ivar set on the class? When the second controller calls the renderer method, the @renderer ivar would already exist, except it'd be instantiated with the wrong controller instance and thus be a factory for the wrong controller.

@egilburg each controller represents a different class, ivars won't be shared between them. @renderer will be set and accessible only inside context of the current controller class.

Right, which is why there's a thread safety issue. Renderer.for(ApplicationController) should always return the same instance, which it doesn't in this case. If we have any mutable state in the renderer which we'd like to rely on, this will break.

renderer class has mutable defaults, but it does not matter. The reason is the same why access to the one renderer class across threads is safe even though access to underlying defaults is not protected by mutex.

defaults stores a static (predefined) value. That's enough to be safe in our case.
Dynamic environment should be passed as renderer.new(env) (if a developer would try to use defaults to dynamically change the environment he would need to use a mutex himself anyway to wrap a change to defaults and consequent render call).

I can explain it further if you want.

The ||= is a read-check-write race condition, and since this is essentially a global (since it is stored on the class) we have to add a lock. Here's a test to demo:

require 'action_pack' require 'action_controller' require 'active_support' require 'concurrent/atomic/count_down_latch' wait_latch = Concurrent::CountDownLatch.new 100 go_latch = Concurrent::CountDownLatch.new t = 100.times.map { |i| Thread.new { wait_latch.count_down go_latch.wait renderer = ActionController::Base.renderer renderer.defaults[:foo] = i renderer } } wait_latch.wait go_latch.count_down renderers = t.map(&:value) p renderers.map(&:object_id).uniq.length p renderers.uniq.map { |r| r.defaults[:foo] } p ActionController::Base.renderer.defaults[:foo]

On my machine using MRI (ruby 2.3.0dev (2015-09-12 trunk 51832) [x86_64-darwin14]) I'm seeing between 24 and 34 unique instances of the renderer set on the controller class. Also you will not be able to predict the value of ActionController::Base.renderer.defaults[:foo].

I'm adding a mutex for now, but I'll see if I can change this to instantiate the renderer when the inherited hook fires so that we can have lock-free reads.

nafaabout · 2015-11-12T07:30:01Z

Oh, thank you guys, you're doing great work. I am glad I belong to this community and this is because of you. Big Thanks.

We sometimes say "✂️ newline after `private`" in a code review (e.g. rails#18546 (comment), rails#34832 (comment)). Now `Layout/EmptyLinesAroundAccessModifier` cop have new enforced style `EnforcedStyle: only_before` (rubocop/rubocop#7059). That cop and enforced style will reduce the our code review cost.

We sometimes say "✂️ newline after `private`" in a code review (e.g. rails/rails#18546 (comment), rails/rails#34832 (comment)). Now `Layout/EmptyLinesAroundAccessModifier` cop have new enforced style `EnforcedStyle: only_before` (rubocop/rubocop#7059). That cop and enforced style will reduce the our code review cost.

brainopia force-pushed the action_view_render branch from 13083fa to 6e5bab4 Compare January 15, 2015 20:40

dhh reviewed Jan 19, 2015
View reviewed changes

brainopia force-pushed the action_view_render branch from 55abfd6 to bf486b5 Compare January 20, 2015 09:19

rafaelfranca reviewed Jan 20, 2015
View reviewed changes

rafaelfranca added actionpack actionview labels Jan 20, 2015

rafaelfranca added this to the 5.0.0 milestone Jan 20, 2015

brainopia force-pushed the action_view_render branch from bf486b5 to 316c898 Compare January 21, 2015 00:37

brainopia force-pushed the action_view_render branch from 316c898 to 14da053 Compare January 21, 2015 11:45

brainopia force-pushed the action_view_render branch from ebc4797 to 98c3296 Compare January 21, 2015 20:02

rafaelfranca reviewed Jan 21, 2015
View reviewed changes

brainopia added 6 commits January 21, 2015 23:53

Add ActionController::Metal#set_request!

ee6e13f

Add `ActionController::Metal#set_request!` to set a request on controller instance without calling dispatch.

Support :assigns option when rendering with controllers/mailers.

685142e

Add ActionController#build_with_env

47b6fe0

To have an easier way to setup a controller instance with custom environment

Add ActionController::Renderer

801e399

Render arbitrary templates outside of controller actions

Add ActionController::Base.render

6566289

Add ApplicationController.renderer initializer [ci skip]

3011b64

brainopia force-pushed the action_view_render branch from 98c3296 to 3011b64 Compare January 21, 2015 22:03

rafaelfranca added a commit that referenced this pull request Jan 22, 2015

Merge pull request #18546 from brainopia/action_view_render

e36ecf4

A shortcut to setup controller environment

rafaelfranca merged commit e36ecf4 into rails:master Jan 22, 2015

rafaelfranca mentioned this pull request Jan 22, 2015

Make it easy to render templates outside of controllers #18409

Closed

egilburg reviewed Jan 22, 2015
View reviewed changes

rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015

timdiggins mentioned this pull request Sep 28, 2016

Readme: Update user profile page instructions thredded/thredded#419

Merged

kamipo mentioned this pull request Jun 13, 2019

Enable Layout/EmptyLinesAroundAccessModifier cop #36472

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A shortcut to setup controller environment #18546

A shortcut to setup controller environment #18546

brainopia commented Jan 15, 2015

brainopia commented Jan 15, 2015

brainopia commented Jan 18, 2015

dhh commented Jan 18, 2015

brainopia commented Jan 19, 2015

dhh commented Jan 19, 2015

dhh Jan 19, 2015

dhh commented Jan 19, 2015

brainopia commented Jan 20, 2015

rafaelfranca Jan 20, 2015

brainopia Jan 20, 2015

rafaelfranca Jan 20, 2015

brainopia commented Jan 21, 2015

rafaelfranca commented Jan 21, 2015

brainopia commented Jan 21, 2015

brainopia commented Jan 21, 2015

rafaelfranca Jan 21, 2015

rafaelfranca commented Jan 21, 2015

brainopia commented Jan 21, 2015

rafaelfranca commented Jan 21, 2015

dhh commented Jan 22, 2015

rafaelfranca commented Jan 22, 2015

brainopia commented Jan 22, 2015

egilburg commented Jan 22, 2015

egilburg Jan 22, 2015

brainopia Jan 22, 2015

sgrif Jan 24, 2015

brainopia Jan 24, 2015

egilburg Jan 24, 2015

brainopia Jan 24, 2015

sgrif Jan 25, 2015

brainopia Jan 25, 2015

tenderlove Sep 14, 2015

nafaabout commented Nov 12, 2015

A shortcut to setup controller environment #18546

A shortcut to setup controller environment #18546

Conversation

brainopia commented Jan 15, 2015

brainopia commented Jan 15, 2015

brainopia commented Jan 18, 2015

dhh commented Jan 18, 2015

brainopia commented Jan 19, 2015

dhh commented Jan 19, 2015

Choose a reason for hiding this comment

dhh commented Jan 19, 2015

brainopia commented Jan 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brainopia commented Jan 21, 2015

rafaelfranca commented Jan 21, 2015

brainopia commented Jan 21, 2015

brainopia commented Jan 21, 2015

Choose a reason for hiding this comment

rafaelfranca commented Jan 21, 2015

brainopia commented Jan 21, 2015

rafaelfranca commented Jan 21, 2015

dhh commented Jan 22, 2015

rafaelfranca commented Jan 22, 2015

brainopia commented Jan 22, 2015

egilburg commented Jan 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nafaabout commented Nov 12, 2015