should we use new hash syntax on code we are adding?

Yes please.

On Mon, Apr 20, 2015 at 10:58 PM, Arthur Nogueira Neves <
notifications@github.com> wrote:

In actionpack/test/controller/api/conditional_get_test.rb
#19832 (comment):

@@ -0,0 +1,57 @@
+require 'abstract_unit'
+require 'active_support/core_ext/integer/time'
+require 'active_support/core_ext/numeric/time'
+
+class ConditionalGetApiController < ActionController::API

• before_action :handle_last_modified_and_etags, :only => :two

• def one
• if stale?(:last_modified => Time.now.utc.beginning_of_day, :etag => [:foo, 123])

•  render :text => "Hi!"
  

should we use new hash syntax on code we are adding?

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

Done

What about that advice:

# For APIs, you may want to use :null_session instead.

Don't we want to do that for APIs?

null_session should be an explicit choice, I think. If you have a API endpoint that doesn't have any parameters to validate, the null_session will still execute the endpoint. Exception is safer.

Doesn't seem to make sense for a controller in an API-only app to have cross-site request forgery protection. Also, from the docs, API controllers do not have session handling:

An API Controller is different from a normal controller in the sense that by default it doesn't include a number of features that are usually required by browser access only: layouts and templates rendering, cookies, sessions, flash, assets, and so on.

Many APIs would want CSRF protection. This includes single page javascript applications.

null_session isn't the same thing as a browser session, but is instead more like an 'empty' request. It's more like passing a NullObject to a controller instead of raising an exception. The benefit of doing so is that you have more explicit control over the actions regarding that request on a per action basis. With exceptions, you're handling the exception in a more generic location (controller level instead of action level).

Not all of this is 100% verified, just off of my memory. Apologies if I got something wrong.

Sorry, I misunderstood! Was thinking in the context of a public API. Thanks for correcting me. 😄

I'm not sure if there's a consensus on this, or what that consensus is if it exists, but I think it still makes sense to use a cookie for authentication when serving an API to a client using AJAX through a browser, because you can use an HttpOnly cookie, which javascript can't access, rather than local storage, which it can. If you're doing that, then CSRF protection is still important.

Having said that, it doesn't look (based on your quote) like API controllers will be using cookie-based sessions by default, so maybe they don't need CSRF protection by default either.

Edit: Wrote this before seeing @hammerdr's comment – I'll leave it here anyway, but it might be irrelevant.

It doesn't need CSRF protection, we are not adding session stuff on this configuration.
The comment is still valid for regular Rails applications where you can build APIs, for those APIs maybe null_session is a good idea. For Rails API's apis you don't need this kind of stuff.

This feature was removed from core.

Good catch, I maybe I should just remove all that.

Fixed spastorino@a848e2e

The original is using the || short-circuit in a much nicer way then here.
You do all the work for internal_asset even though you don't need it.

A nicer solution might be to split it into two private methods and call them with the same short-circuiting logic:

def internal?
  internal_controller? || internal_asset?
end

...

private

def internal_controller?
  controller.to_s =~ %r{\arails/(info|mailers|welcome)}
end

def internal_asset?
  Rails.application.config.respond_to?(:assets) &&
    path =~ %r{\a#{Rails.application.config.assets.prefix}\z}
end

Agreed and fixed.

We should replace this with an ActionDispatch::IntegrationTest generator instead. ActionController::TestCase is legacy ware.

👍 will take a look at it