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

A shortcut to setup controller environment #18546

Merged
merged 6 commits into from Jan 22, 2015

Conversation

@brainopia
Copy link
Contributor

@brainopia brainopia commented Jan 15, 2015

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 brainopia force-pushed the brainopia:action_view_render branch Jan 15, 2015
@brainopia
Copy link
Contributor Author

@brainopia brainopia commented Jan 15, 2015

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
Copy link
Contributor Author

@brainopia brainopia commented Jan 18, 2015

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

@dhh dhh commented Jan 18, 2015

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
Copy link
Contributor Author

@brainopia brainopia commented Jan 19, 2015

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

@dhh dhh commented Jan 19, 2015

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
dhh reviewed Jan 19, 2015
View changes
actionpack/lib/action_controller/renderer.rb Outdated
def for(controller)
Class.new self do
self.controller = controller
self.defaults = {

This comment has been minimized.

@dhh

dhh Jan 19, 2015
Member

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
dhh reviewed Jan 19, 2015
View changes
actionpack/lib/action_controller/renderer.rb Outdated
end
end

def initialize(env={})

This comment has been minimized.

@dhh

dhh Jan 19, 2015
Member

I like spacing such as env = {}

@dhh
dhh reviewed Jan 19, 2015
View changes
actionpack/lib/action_controller/renderer.rb Outdated
end

private

This comment has been minimized.

@dhh

dhh Jan 19, 2015
Member

✂️ the newline, indent all methods below the privacy protection.

@dhh
dhh reviewed Jan 19, 2015
View changes
actionpack/lib/action_controller/renderer.rb Outdated

private

def normalize_keys(env)

This comment has been minimized.

@dhh

dhh Jan 19, 2015
Member

Use Composed Method to split the three operations in this method into their own private methods.

@dhh
Copy link
Member

@dhh dhh commented Jan 19, 2015

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

@brainopia brainopia force-pushed the brainopia:action_view_render branch Jan 20, 2015
@brainopia
Copy link
Contributor Author

@brainopia brainopia commented Jan 20, 2015

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
rafaelfranca reviewed Jan 20, 2015
View changes
actionpack/lib/action_controller/metal.rb Outdated
to_a
end

def set_request!(request)

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2015
Member

Is set_request! supposed to be used by users?

This comment has been minimized.

@brainopia

brainopia Jan 20, 2015
Author Contributor

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

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2015
Member

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

Yes. 👍

@rafaelfranca
rafaelfranca reviewed Jan 20, 2015
View changes
actionpack/lib/action_controller/metal/rack_delegation.rb Outdated
def dispatch(action, request)
module ClassMethods
def setup_env(env={})
new.tap {|_| _.set_request! ActionDispatch::Request.new(env) }

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2015
Member

Name the _ variable and put space before the |

@rafaelfranca
rafaelfranca reviewed Jan 20, 2015
View changes
actionpack/lib/action_controller/metal/rack_delegation.rb Outdated
@@ -8,9 +8,15 @@ module RackDelegation
delegate :headers, :status=, :location=, :content_type=,
:status, :location, :content_type, :response_code, :to => "@_response"

def dispatch(action, request)
module ClassMethods
def setup_env(env={})

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2015
Member

Space here

@@ -92,12 +92,16 @@ def rendered_format
# Find and render a template based on the options given.
# :api: private
def _render_template(options) #:nodoc:
variant = options[:variant]
variant = options.delete(:variant)

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2015
Member

Do we need to delete? We usually avoid to mutate the arguments of a method.

This comment has been minimized.

@brainopia

brainopia Jan 20, 2015
Author Contributor

_render_template is not a user-accessible method and methods atop mutate options freely (assigning and deleting keys), e. g.

https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/metal/rendering.rb#L56
https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/metal/rendering.rb#L63
https://github.com/rails/rails/blob/master/actionpack/lib/abstract_controller/rendering.rb#L107

If we'd like to preserve original arguments unchanged then a duplicate of hash is needed at https://github.com/brainopia/rails/blob/action_view_render/actionview/lib/action_view/rendering.rb#L122 or all the methods mutating options should be fixed.

I'm fine with either, tell me what do you prefer and I'll do it.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2015
Member

There is any need to mutate now. If so let's keep it. If not we should not
mutate.

On Tue, Jan 20, 2015, 17:37 Ravil Bayramgalin notifications@github.com
wrote:

In actionview/lib/action_view/rendering.rb
#18546 (diff):

@@ -92,12 +92,16 @@ def rendered_format
# Find and render a template based on the options given.
# :api: private
def _render_template(options) #:nodoc:

  •    variant = options[:variant]
    
  •    variant = options.delete(:variant)
    

_render_template is not a user-accessible method and methods atop mutate
options freely (assigning and deleting keys), e. g.

https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/metal/rendering.rb#L56

https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/metal/rendering.rb#L63

https://github.com/rails/rails/blob/master/actionpack/lib/abstract_controller/rendering.rb#L107

If we'd like to preserve original arguments unchanged then a duplicate of
hash is needed at
https://github.com/brainopia/rails/blob/action_view_render/actionview/lib/action_view/rendering.rb#L122
or all the methods mutating options should be fixed.

I'm fine with either, tell me what do you prefer and I'll do it.


Reply to this email directly or view it on GitHub
https://github.com/rails/rails/pull/18546/files#r23248812.

This comment has been minimized.

@brainopia

brainopia Jan 20, 2015
Author Contributor

:variant is not used by view_renderer and is only needed by lookup_context. To make clear that it's not used further, it's removed from options.

The same with :assigns.

This comment has been minimized.

@rafaelfranca
rafaelfranca reviewed Jan 20, 2015
View changes
actionview/lib/action_view/rendering.rb Outdated
assigns = options.delete(:assigns)
context = view_context

context.assign assigns if assigns

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2015
Member

What if we change view_context to take assigns parameter with default to view_assigns and we could build the object with the correct assigns instead of changing them?

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2015
Member

Also I'd like to kill the view_assigns dependency on Action View entirely and always receive a assigns option, but not sure if it is possible. Worth to check

This comment has been minimized.

@brainopia

brainopia Jan 20, 2015
Author Contributor

Sure, I'll update view_context method to receive an optional value of :assigns and return a context with it.

I'd prefer to leave removal of view_assigns dependency from ActionView as a separate pull-request, if you don't mind.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2015
Member

I'd prefer to leave removal of view_assigns dependency from ActionView as a separate pull-request, if you don't mind.

👍 this is what I had in mind.

@@ -8,9 +8,15 @@ module RackDelegation
delegate :headers, :status=, :location=, :content_type=,
:status, :location, :content_type, :response_code, :to => "@_response"

def dispatch(action, request)
module ClassMethods

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2015
Member

Not sure if this is the proper place to put this method. Also the name is not good enough. It is returning a new instance of a controller right?

This comment has been minimized.

@brainopia

brainopia Jan 20, 2015
Author Contributor

Yeah, it's a method to create a new controller instance in a given rack environment.
Since the environment is a rack territory, this is the best file I could think of to put this method in.

I'll change name/file if you wish.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2015
Member

Right. The place now make sense but the name is still not ideal. Maybe controller_for_env?

This comment has been minimized.

@brainopia

brainopia Jan 20, 2015
Author Contributor

What do you think about new_for_env? It reflects a constructor/initialization purpose and describes arguments without a bit of redundancy of FooController.controller_for_env.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2015
Member

maybe build_with_env or buid_for_env?

This comment has been minimized.

@brainopia

brainopia Jan 20, 2015
Author Contributor

Yes, looks good! Especially, I like build_with_env 😄

@rafaelfranca
rafaelfranca reviewed Jan 20, 2015
View changes
actionpack/lib/action_controller/renderer.rb Outdated
end

def initialize(env = {})
@env = normalize_keys(defaults).merge normalize_keys(env)

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2015
Member

Why we need to do all this gymnastics with env?

This comment has been minimized.

@brainopia

brainopia Jan 20, 2015
Author Contributor

Why do we need to normalize keys of environment? To match expected format of rack env by the rest.

Or why do we need defaults? It's the minimal environment in which different helpers and request would work without exceptions each time.

@rafaelfranca rafaelfranca added this to the 5.0.0 milestone Jan 20, 2015
@brainopia brainopia force-pushed the brainopia:action_view_render branch Jan 21, 2015
@brainopia
Copy link
Contributor Author

@brainopia brainopia commented Jan 21, 2015

@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
Copy link
Member

@rafaelfranca rafaelfranca commented Jan 21, 2015

💣 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 brainopia force-pushed the brainopia:action_view_render branch Jan 21, 2015
@brainopia
Copy link
Contributor Author

@brainopia brainopia commented Jan 21, 2015

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

@brainopia brainopia force-pushed the brainopia:action_view_render branch Jan 21, 2015
@brainopia
Copy link
Contributor Author

@brainopia brainopia commented Jan 21, 2015

@dhh here you go 😄

@rafaelfranca
rafaelfranca reviewed Jan 21, 2015
View changes
actionpack/lib/action_controller/renderer.rb Outdated
end
end

def initialize(env = {})

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 21, 2015
Member

Is this method public API?

@rafaelfranca
rafaelfranca reviewed Jan 21, 2015
View changes
actionpack/lib/action_controller/renderer.rb Outdated
@env['action_dispatch.routes'] = controller._routes
end

def render(*args)

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 21, 2015
Member

Do we need to document this method?

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jan 21, 2015

Awesome work on this @brainopia

brainopia added 6 commits Jan 15, 2015
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 brainopia force-pushed the brainopia:action_view_render branch to 3011b64 Jan 21, 2015
@brainopia
Copy link
Contributor Author

@brainopia brainopia commented Jan 21, 2015

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

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jan 21, 2015

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

@dhh
Copy link
Member

@dhh dhh commented Jan 22, 2015

Looks good to me! Great work, guys.

rafaelfranca added a commit that referenced this pull request Jan 22, 2015
A shortcut to setup controller environment
@rafaelfranca rafaelfranca merged commit e36ecf4 into rails:master Jan 22, 2015
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jan 22, 2015

❤️ 💚 💙 💛 💜

@brainopia
Copy link
Contributor Author

@brainopia brainopia commented Jan 22, 2015

🙇 🙇 🙇

@egilburg
Copy link
Contributor

@egilburg egilburg commented Jan 22, 2015

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

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

This comment has been minimized.

@egilburg

egilburg Jan 22, 2015
Contributor

is/should this be thread safe?

This comment has been minimized.

@brainopia

brainopia Jan 22, 2015
Author Contributor

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 comment has been minimized.

@sgrif

sgrif Jan 24, 2015
Contributor

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

This comment has been minimized.

@brainopia

brainopia Jan 24, 2015
Author Contributor

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.

This comment has been minimized.

@egilburg

egilburg Jan 24, 2015
Contributor

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

This comment has been minimized.

@brainopia

brainopia Jan 24, 2015
Author Contributor

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

This comment has been minimized.

@sgrif

sgrif Jan 25, 2015
Contributor

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.

This comment has been minimized.

@brainopia

brainopia Jan 25, 2015
Author Contributor

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.

This comment has been minimized.

@tenderlove

tenderlove Sep 14, 2015
Member

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

@nafaabout nafaabout commented Nov 12, 2015

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.

@claudiob

This comment has been minimized.

@brainopia I just created a brand new app with Rails 5.0.0.beta1 and I saw this file in config/initializer.

It's not clear at first sight what it does.
Maybe you could add a comment in this file which, similarly to other initializers, explains what happens when you uncomment the code.

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015
kamipo added a commit to kamipo/rails that referenced this pull request Jun 13, 2019
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-hq/rubocop#7059).

That cop and enforced style will reduce the our code review cost.
koic added a commit to koic/oracle-enhanced that referenced this pull request Apr 15, 2020
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-hq/rubocop#7059).

That cop and enforced style will reduce the our code review cost.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.