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

Rails api #19832

Merged
merged 60 commits into from Jun 11, 2015
Merged

Rails api #19832

merged 60 commits into from Jun 11, 2015

Conversation

@dhh
Copy link
Member

dhh commented Apr 20, 2015

[Description extracted from http://wyeworks.com/blog/2015/4/20/rails-api-is-going-to-be-included-in-rails-5/]

A decision was made to incorporate Rails API into Rails core πŸŽ‰ πŸŽ‰ πŸŽ‰.

What Is Rails API?

The original idea behind Rails API was to serve as a starting point for a version of Rails better suited for JS-heavy apps. As of today, Rails API provides: trimmed down controllers and middleware stack together with a matching set of generators, all specifically tailored for API type applications.

Next steps

We still need to discuss the β€œRails way” for API applications, how API apps should be built and, what features we’d like included from our original list of ideas. In particular:

  • Do we want to avoid asset generation in Rails by having a back-end and a front-end apps?
  • Do we prefer to have a single application and keep asset generation in Rails instead?
  • Do we like Active Model Serializers better than Jbuilder?
  • If not, can we make Rails API play nicely with Jbuilder?

def one
if stale?(:last_modified => Time.now.utc.beginning_of_day, :etag => [:foo, 123])
render :text => "Hi!"

This comment has been minimized.

@arthurnn

arthurnn Apr 20, 2015 Member

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

This comment has been minimized.

@dhh

dhh Apr 20, 2015 Author Member

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.

This comment has been minimized.

end

private

This comment has been minimized.

@dhh

dhh Apr 20, 2015 Author Member

βœ‚οΈ newline and intent below private.

This comment has been minimized.

@arthurnn

arthurnn Apr 20, 2015 Member

also, I think this needs to follow the private indentation. (sorry, dhh said that already)

This comment has been minimized.

@ZhangHanDong
Copy link

ZhangHanDong commented Apr 21, 2015

Good job, although I have been using Rails Metal for my Api.

@spastorino
Copy link
Member

spastorino commented Apr 21, 2015

Hey @dhh, thanks for opening the PR.
We need to discuss a lot of stuff about this, I've written a short blog post that also has links for a previous article I've written in the past http://wyeworks.com/blog/2015/4/20/rails-api-is-going-to-be-included-in-rails-5/

Some of the things that needs discussions are ...

  • Would we like to have a backend app and a frontend app and then avoid assets generation in Rails?
  • Would we prefer to have just one app and keep assets generation in Rails?
  • Would we like to include Active Model Serializers by default or jbuilder?
  • How this will play nicely when used with jbuilder?
# Prevent CSRF attacks by raising an exception.
# For APIs, you may want to use :null_session instead.
protect_from_forgery with: :exception
<%- end -%>

This comment has been minimized.

@guilleiguaran

guilleiguaran Apr 21, 2015 Member

What about that advice:

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

Don't we want to do that for APIs?

This comment has been minimized.

@hammerdr

hammerdr Apr 21, 2015 Contributor

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.

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Apr 21, 2015 Member

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.

This comment has been minimized.

@hammerdr

hammerdr Apr 21, 2015 Contributor

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.

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Apr 21, 2015 Member

Sorry, I misunderstood! Was thinking in the context of a public API. Thanks for correcting me. πŸ˜„

This comment has been minimized.

@jsanders

jsanders Apr 21, 2015 Contributor

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.

This comment has been minimized.

@spastorino

spastorino Apr 21, 2015 Member

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.

@rafaelfranca rafaelfranca added this to the 5.0.0 milestone Apr 21, 2015
# end
#
# class PostsController < ApplicationController
# respond_to :json, :xml

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 21, 2015 Member

This feature was removed from core.

This comment has been minimized.

@spastorino

spastorino Apr 21, 2015 Member

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

This comment has been minimized.

class API < Metal
abstract!

module Compatibility

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 21, 2015 Member

Do you remember why we need this module?

This comment has been minimized.

@spastorino

spastorino Apr 21, 2015 Member

To be honest I don't remember very well and was in my todo list to review it. What tries to do if I'm not wrong is to avoid crashing when plugins, etc assumes that your controller is a Base controller which won't be true. But maybe we should try to remove and see what happens without this.

@@ -0,0 +1,14 @@
module ActionController
module ApiRendering

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 21, 2015 Member

What is the difference between ApiRendering and the one included inside ActionController::Base?

This comment has been minimized.

@jmbejar

jmbejar Apr 21, 2015 Contributor

It seems this module was added to avoid the inclusion of ActionView::Rendering (this is currently being included on rails-api).

However, I tested this PR without this module and it seems to work properly.

I think this still works because the render method options are processed in the Renderers module here (and api controllers still use this module).

In brief, we should be fine just including Rendering, no need to have a specific ApiRendering.

This comment has been minimized.

@@ -0,0 +1,47 @@
module ActionDispatch
class ApiPublicExceptions

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 21, 2015 Member

The same question that ApiRendering about this one.

This comment has been minimized.

@jmbejar

jmbejar Apr 21, 2015 Contributor

In fact, ApiPublicExceptions is never used apart from tests (it's not included in the default middleware like rails-api gem does here).

After testing for a while, I'm pretty convinced that exceptions are rendered in the json response fine without the need of this module. The existent PublicException is doing exactly the same, I think.

@spastorino I think we can just remove this file and related tests, but maybe I'm missing something?

This comment has been minimized.

@spastorino

spastorino Apr 21, 2015 Member

@rafaelfranca great catches bro. Yeah this was needed some time ago but it is not anymore.
Removed spastorino@c23d78e

@chancancode
Copy link
Member

chancancode commented Apr 21, 2015

@rwz you might have opinions on some of the open questions?

@dhh
Copy link
Member Author

dhh commented Apr 21, 2015

Agree on keeping assets out of the app. Need to take another (long over
due) look at AMS. But regardless of whether we go with AMS or jbuilder as
default, both should be totally equal and trivial to swap between.

On Tue, Apr 21, 2015 at 5:08 PM, Santiago Pastorino <
notifications@github.com> wrote:

BTW, because I left some open questions without answers, my take on those
is ...

Would we like to have a backend app and a frontend app and then avoid
assets generation in Rails?
Yes. So Rails should only generate the API and completely skip assets
generation.

Would we prefer to have just one app and keep assets generation in
Rails?
No.

Would we like to include Active Model Serializers by default or
jbuilder?
AMS.

How this will play nicely when used with jbuilder?
It should work.

But given that the questions could be a bit controversial I left it there
to be able to discuss.

β€”
Reply to this email directly or view it on GitHub
#19832 (comment).

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

internal_controller || internal_asset

This comment has been minimized.

@omegahm

omegahm Apr 21, 2015 Contributor

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

This comment has been minimized.

@spastorino

spastorino Apr 22, 2015 Member

Agreed and fixed.

@@ -45,7 +45,11 @@ def action
end

def internal?
controller.to_s =~ %r{\Arails/(info|mailers|welcome)} || path =~ %r{\A#{Rails.application.config.assets.prefix}\z}
internal_controller = controller.to_s =~ %r{\arails/(info|mailers|welcome)}
internal_asset = Rails.application.config.respond_to?(:assets) &&

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 22, 2015 Member

Why config is not responding to assets anymore? I could not find where we remove it.

This comment has been minimized.

@lucasmazza

lucasmazza Apr 22, 2015 Member

it's defined by sprockets-rails now, since 5172d93

This comment has been minimized.

@spastorino

spastorino Apr 22, 2015 Member

Thing is sprockets is not included when using Rails API. Anyway maybe we can fix this in sprockets railtie but unsure what's better given the current way that sprockets is hooking into AV.

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 22, 2015 Member

Got it. I think it seems good this way


# Force sprockets to be skipped when generating http only app.
# Can't modify options hash as it's frozen by default.
self.options = options.merge(skip_sprockets: true, skip_javascript: true).freeze if options[:api]

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 22, 2015 Member

can't we just override/define skip_sprockets and skip_javascript methods and check if options[:api] is present inside both?

This comment has been minimized.

@jmbejar

jmbejar May 5, 2015 Contributor

This is implemented in this way taking advantage of existent conditional logic in templates.

Examples: https://github.com/rails/rails/blob/master/railties/lib/rails/generators/rails/app/templates/config/environments/development.rb.tt#L29
https://github.com/rails/rails/blob/master/railties/lib/rails/generators/rails/app/templates/config/environments/production.rb.tt#L21

I think it's better to have this done here, so we can avoid having more conditionals in the templates.

Thoughts?

This comment has been minimized.

@rafaelfranca

rafaelfranca May 11, 2015 Member

I'm suggesting to compose the conditionals in new methods. This way you would have to change the templates, but we don't need to change an implementation detail.

This comment has been minimized.

@spastorino

spastorino May 13, 2015 Member

@rafaelfranca to do what you're mentioning we would need to change things like ...

  1. https://github.com/rails/rails/blob/master/railties/lib/rails/generators/app_base.rb#L174
  2. https://github.com/rails/rails/blob/master/railties/lib/rails/generators/app_base.rb#L170

The most trivial change ends being like https://gist.github.com/spastorino/7989a586151e8da13351, not just adding one method.

I don't think the diff is much better than what we already have but we should reconsider following a better approach in the future. Something around the lines of having an options class instead of a hash and defining methods in it for each possibility.

This comment has been minimized.

@@ -244,11 +251,31 @@ def create_test_files
end

def create_tmp_files
build(:tmp)
build(:tmp) unless options[:api]

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 22, 2015 Member

why we would remove tmp in an API application? The application still need pids and temporary stuff

This comment has been minimized.

@ekampp

ekampp Apr 24, 2015

Tmp might also be useful for temporary file processing, if you're not using a file service.

This comment has been minimized.

@jmbejar

jmbejar May 5, 2015 Contributor

Just had a discussion with @spastorino. We agree that there is no reason to remove tmp or vendor folder.

We will revert this change.

This comment has been minimized.

end

def create_vendor_files
build(:vendor)
build(:vendor) unless options[:api]

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 22, 2015 Member

I think vendor is also needed in an API application. For example, I usually store internal gems engines inside vendor/gems to keep them in the same project.

This comment has been minimized.

@ekampp

ekampp Apr 24, 2015

Also, if you have a very closed-off system, using the bundle package --all or bundle --path vendor/bundle both uses the vendor dir.

This comment has been minimized.

module ActionController
# API Controller is a lightweight version of <tt>ActionController::Base</tt>,
# created for applications that don't require all functionality that a complete
# \Rails controller provides, allowing you to create faster controllers for

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 22, 2015 Member

"faster controllers" I don't believe it is true and even it is we know that it would matter in real world. I'd change it to allowing you to create controllers with just the features that you need for API only applications.

This comment has been minimized.

@ekampp

ekampp Apr 24, 2015

Loading less is inherently faster than loading more. The statement is correct. Weather or not it's relevant to the real world is debatable.

This comment has been minimized.

# 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. This makes the entire controller stack thinner and
# faster, suitable for API applications. It doesn't mean you won't have such

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 22, 2015 Member

same here about the "faster". I'd just omit it

This comment has been minimized.

This comment has been minimized.

#
# class MetalController
# ActionController::API.without_modules(:Redirecting, :DataStreaming).each do |left|
# include left

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 22, 2015 Member

Why leaving the include for the users since it is only what they can do? I think we should implement it for ourself and just require users to do:

ActionController::API.without_modules(:Redirecting, :DataStreaming)

This comment has been minimized.

@spastorino

spastorino Apr 22, 2015 Member

This feature already exists in Base so I wouldn't change the current API

This comment has been minimized.


class RedirectToApiController < ActionController::API
def one
redirect_to :action => "two"

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 22, 2015 Member

New hash syntax

class RenderersApiController < ActionController::API
class Model
def to_json(options = {})
{ :a => 'b' }.to_json(options)

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 22, 2015 Member

new hash syntax

end

def to_xml(options = {})
{ :a => 'b' }.to_xml(options)

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 22, 2015 Member

new hash syntax

end

def one
render :json => Model.new

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 22, 2015 Member

new hash syntax

end

def two
render :xml => Model.new

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 22, 2015 Member

new hash syntax

def test_render_json
get :one
assert_response :success
assert_equal({ :a => 'b' }.to_json, @response.body)

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 22, 2015 Member

new hash syntax

spastorino added a commit that referenced this pull request Jun 11, 2015
Rails api
@spastorino spastorino merged commit 21f7bcb into rails:master Jun 11, 2015
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@bf4
Copy link
Contributor

bf4 commented Jun 11, 2015

@spastorino Awesome! Would you be interested in me writing a PR along the lines of https://groups.google.com/d/topic/rubyonrails-core/K8t4-DZ_DkQ/discussion (as discussed above)?

@joaomdmoura
Copy link

joaomdmoura commented Jun 11, 2015

Amazing! Really happy you made it! Let's keep up the good work!

@claudiob
Copy link
Member

claudiob commented Jun 11, 2015

πŸ‘ πŸ‘ πŸ‘ πŸ‘ πŸ‘ πŸ‘

@ajoy39
Copy link

ajoy39 commented Jun 12, 2015

Great work!Β 

β€”
Sent from Mailbox

On Thu, Jun 11, 2015 at 4:04 PM, Claudio B. notifications@github.com
wrote:

πŸ‘ πŸ‘ πŸ‘ πŸ‘ πŸ‘ πŸ‘

Reply to this email directly or view it on GitHub:
#19832 (comment)

@dhh
Copy link
Member Author

dhh commented Jun 12, 2015

🀘

@gfvcastro
Copy link
Contributor

gfvcastro commented Jun 13, 2015

Excellent. πŸ‘ πŸŽ‰

@vestimir
Copy link

vestimir commented Jun 13, 2015

πŸ––

@robin850
Copy link
Member

robin850 commented Jun 23, 2015

(ping #20612)

@yosriady
Copy link

yosriady commented Jun 23, 2015

Brilliant! 🎊

@robertomiranda

This comment has been minimized.

Copy link
Contributor

robertomiranda commented on ebcc15c Jun 23, 2015

❀️

@Justin-lu
Copy link

Justin-lu commented Jun 28, 2015

🀘

@spuyet
Copy link
Contributor

spuyet commented Jun 29, 2015

πŸ‘

@aalizzwell58
Copy link

aalizzwell58 commented Jun 30, 2015

Good job πŸ‘

@Theminijohn
Copy link

Theminijohn commented Jun 30, 2015

πŸ‘

@hapiben
Copy link

hapiben commented Jun 30, 2015

Sweet!

@seemsindie
Copy link

seemsindie commented Jul 1, 2015

Ok, i have to ask, how i can use this now?

@bf4
Copy link
Contributor

bf4 commented Jul 1, 2015

@dhh @spastorino lock comments, please?

@robin850
Copy link
Member

robin850 commented Jul 1, 2015

@seemsindie : You have to use Rails' master ; this is not yet released.

@bf4 : Yes, you're right!

@rails rails locked and limited conversation to collaborators Jul 1, 2015
@rails rails unlocked this conversation Jul 2, 2015
@rails rails locked and limited conversation to collaborators Jul 2, 2015
require 'test_helper'

<% module_namespacing do -%>
class <%= controller_class_name %>ControllerTest < ActionController::TestCase

This comment has been minimized.

@dhh

dhh Dec 4, 2015 Author Member

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

This comment has been minimized.

@spastorino

spastorino Dec 7, 2015 Member

πŸ‘ will take a look at it

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.