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: Ability to return error responses in json format also in development #20831

Merged
merged 23 commits into from Dec 9, 2015

Conversation

Projects
None yet
7 participants
@jmbejar
Contributor

jmbejar commented Jul 10, 2015

This PR aims to have a better experience when developing Rails app with the --api option.

Specifically, we found that error pages are always being delivered in html pages in development mode, without the chance to have errors responses rendered in json (or even xml) format. With these changes, developers would have the chance to better integrate error responses with client code in development mode.

The approach in this PR is based on how custom Rails apps work in development mode: responses to regular http requests render html page errors and responses to xhr requests render plane text pages.
We added similar code to have error responses to Rails API requests being rendered in the requested format (json by default).

Although the primary goal was to improve the development experience, we needed to fix another problem in the way.
If you request a page with a format extension in the URL (e.g. posts/1.json) and some error happens in the middleware stack before reaching application code, you get a html response (also in production mode). However, if you include an Accept header with the application/json mime type, you get an json response including the error code and message.

In the case where format is deduced from the URL extension, the format parameter value is parsed by the router but exceptions can happen before reaching that point. For example, in Rails API applications could happen that incoming requests have a bad formatted body, causing a json parse error. This exception is raised before format is parsed by routing. This test case shows the issue: https://github.com/rails/rails/compare/rails:master...jmbejar:rails-api-json-error-response?expand=1#diff-c3f1ef70a218168e1473ce5e5245ce14R116

The solution to this secondary problem is to compute the format value from the URL using a very simple approach if it is not set yet. The format parameter value is set again once routing code executes, so it does not implies any behavior change for the application code executed after routing.

@edwardloveall

This comment has been minimized.

Show comment
Hide comment
@edwardloveall

edwardloveall Jul 10, 2015

I really like the idea of errors coming back as the format you're requesting in.

I really like the idea of errors coming back as the format you're requesting in.

Show outdated Hide outdated actionpack/lib/action_dispatch/middleware/debug_exceptions.rb
else
raise exception
end
end
def render_for_non_api_application(request, wrapper)

This comment has been minimized.

@spastorino

spastorino Jul 10, 2015

Member

I'd call this render_for_default_application

@spastorino

spastorino Jul 10, 2015

Member

I'd call this render_for_default_application

This comment has been minimized.

@jmbejar

jmbejar Jul 11, 2015

Contributor

👍

@jmbejar

jmbejar Jul 11, 2015

Contributor

👍

@spastorino

This comment has been minimized.

Show comment
Hide comment
@spastorino

spastorino Jul 10, 2015

Member

Other than that seems good. Would be great if others can review ...

Member

spastorino commented Jul 10, 2015

Other than that seems good. Would be great if others can review ...

Show outdated Hide outdated railties/lib/rails/application/default_middleware_stack.rb
@@ -34,7 +34,7 @@ def build_stack
# Must come after Rack::MethodOverride to properly log overridden methods
middleware.use ::Rails::Rack::Logger, config.log_tags
middleware.use ::ActionDispatch::ShowExceptions, show_exceptions_app
middleware.use ::ActionDispatch::DebugExceptions, app
middleware.use ::ActionDispatch::DebugExceptions, app, config.api_only

This comment has been minimized.

@matthewd

matthewd Jul 10, 2015

Member

👎 on using api_only here.

I'm in two minds as to whether we should just always give a JSON error for a request that wants JSON... but if we want to keep the current HTML-for-JSON behaviour for "regular" applications, this should be based on a new config setting (which would get set to true by the API generator).

If "make Rails better for APIs" means "introduce some new configurable features, and some new defaults", we'll all win. But if it means "introduce an expanding number of places that are directly checking the value of config.api_only", I for one will be very sad.

@matthewd

matthewd Jul 10, 2015

Member

👎 on using api_only here.

I'm in two minds as to whether we should just always give a JSON error for a request that wants JSON... but if we want to keep the current HTML-for-JSON behaviour for "regular" applications, this should be based on a new config setting (which would get set to true by the API generator).

If "make Rails better for APIs" means "introduce some new configurable features, and some new defaults", we'll all win. But if it means "introduce an expanding number of places that are directly checking the value of config.api_only", I for one will be very sad.

This comment has been minimized.

@spastorino

spastorino Jul 10, 2015

Member

@matthewd agreed about the new config. That could also be done for the session, etc.

@spastorino

spastorino Jul 10, 2015

Member

@matthewd agreed about the new config. That could also be done for the session, etc.

This comment has been minimized.

@jmbejar

jmbejar Jul 11, 2015

Contributor

@matthewd @spastorino I like the idea of having a separate config. Will do.

@jmbejar

jmbejar Jul 11, 2015

Contributor

@matthewd @spastorino I like the idea of having a separate config. Will do.

This comment has been minimized.

@jmbejar

jmbejar Jul 21, 2015

Contributor

@matthewd @spastorino I've refactored this to have a separate config option. You can see the related changes here: ffa2a5d...jmbejar:rails-api-json-error-response

I initially tried having a boolean flag but it seems to fit better if we have a config setting with symbol values that describe the behavior of DebugExceptions. For now, we have :default and :api modes.

One drawback of this approach where we have separate config settings: if some app uses api_only = false and debug_exception_response_format = :api for some reason, it will end up having JSON responses for error requests while the app behaves as a default application.

This is a point of concern for me, but I understand that it would be caused by developer decisions since they will have more flexibility to define the behavior. In any case, I'm still pushing these commits to the PR to have more feedback and discussion.

@jmbejar

jmbejar Jul 21, 2015

Contributor

@matthewd @spastorino I've refactored this to have a separate config option. You can see the related changes here: ffa2a5d...jmbejar:rails-api-json-error-response

I initially tried having a boolean flag but it seems to fit better if we have a config setting with symbol values that describe the behavior of DebugExceptions. For now, we have :default and :api modes.

One drawback of this approach where we have separate config settings: if some app uses api_only = false and debug_exception_response_format = :api for some reason, it will end up having JSON responses for error requests while the app behaves as a default application.

This is a point of concern for me, but I understand that it would be caused by developer decisions since they will have more flexibility to define the behavior. In any case, I'm still pushing these commits to the PR to have more feedback and discussion.

@jmbejar

This comment has been minimized.

Show comment
Hide comment
@jmbejar

jmbejar Aug 11, 2015

Contributor

Just to make sure we're all in the same page, we need to wait for rails/web-console#152 (almost ready!) before going forward with this PR.

Contributor

jmbejar commented Aug 11, 2015

Just to make sure we're all in the same page, we need to wait for rails/web-console#152 (almost ready!) before going forward with this PR.

Show outdated Hide outdated railties/lib/rails/generators/rails/app/templates/Gemfile
<%- if options.api? -%>
# Access an IRB console on exception pages
<%- if options.dev? || options.edge? -%>
# gem 'web-console', github: 'rails/web-console'

This comment has been minimized.

@rafaelfranca

rafaelfranca Aug 11, 2015

Member

Why we are even adding web-console commented? If the app is API only we will not care about web-console

@rafaelfranca

rafaelfranca Aug 11, 2015

Member

Why we are even adding web-console commented? If the app is API only we will not care about web-console

This comment has been minimized.

@jmbejar

jmbejar Aug 11, 2015

Contributor

Since we decided to have a separate config value (debug_exception_response_format) to setup the debug response format, it's still valid to use web-console in an API only app. You can configure debug_exception_response_format = :default and api_only = true, so you should get HTML error pages on development where the console can be used.

In the other hand, JSON responses are the default configuration (debug_exception_response_format = :api), and for this reason it makes sense to exclude web-console by default. However, we're leaving it commented because it's still supported.

In a related note, we were waiting for changes in web-console to fully support this case (rails/web-console#152), but that's already merged :)

@jmbejar

jmbejar Aug 11, 2015

Contributor

Since we decided to have a separate config value (debug_exception_response_format) to setup the debug response format, it's still valid to use web-console in an API only app. You can configure debug_exception_response_format = :default and api_only = true, so you should get HTML error pages on development where the console can be used.

In the other hand, JSON responses are the default configuration (debug_exception_response_format = :api), and for this reason it makes sense to exclude web-console by default. However, we're leaving it commented because it's still supported.

In a related note, we were waiting for changes in web-console to fully support this case (rails/web-console#152), but that's already merged :)

Show outdated Hide outdated ...ils/generators/rails/app/templates/config/environments/development.rb.tt
# Return error responses in the format requested by the client
# or default to JSON format.
# This option is useful for Rails API only applications developers

This comment has been minimized.

@rafaelfranca

rafaelfranca Aug 11, 2015

Member

We can remove this second paragraph

@rafaelfranca

rafaelfranca Aug 11, 2015

Member

We can remove this second paragraph

This comment has been minimized.

@jmbejar

jmbejar Aug 11, 2015

Contributor

Agreed 👍

@jmbejar

jmbejar Aug 11, 2015

Contributor

Agreed 👍

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Aug 11, 2015

Member

You need to rebase this branch too

Member

rafaelfranca commented Aug 11, 2015

You need to rebase this branch too

Show outdated Hide outdated ...ils/generators/rails/app/templates/config/environments/development.rb.tt
# Return error responses in the format requested by the client
# or default to JSON format.
config.debug_exception_response_format = :api
<%- end -%>

This comment has been minimized.

@spastorino

spastorino Dec 9, 2015

Member

You can remove this now :)

@spastorino

spastorino Dec 9, 2015

Member

You can remove this now :)

Show outdated Hide outdated railties/test/generators/api_app_generator_test.rb
@@ -40,12 +40,18 @@ def test_api_modified_files
assert_no_match(/gem 'jbuilder'/, content)
assert_no_match(/gem 'web-console'/, content)
assert_match(/gem 'active_model_serializers'/, content)
assert_no_match(/gem 'web-console'/, content)

This comment has been minimized.

@spastorino

spastorino Dec 9, 2015

Member

This is duplicating line 41

@spastorino

spastorino Dec 9, 2015

Member

This is duplicating line 41

Show outdated Hide outdated railties/test/generators/api_app_generator_test.rb
end
assert_file "config/application.rb" do |content|
assert_match(/config.api_only = true/, content)
end
assert_file "config/environments/development.rb" do |content|
assert_match(/config.debug_exception_response_format = :api/, content)
end

This comment has been minimized.

@spastorino

spastorino Dec 9, 2015

Member

Also remove this

@spastorino

spastorino Dec 9, 2015

Member

Also remove this

Show outdated Hide outdated actionpack/lib/action_dispatch/middleware/debug_exceptions.rb
@@ -17,6 +17,7 @@ def debug_params(params)
clean_params = params.clone
clean_params.delete("action")
clean_params.delete("controller")
clean_params.delete("format")

This comment has been minimized.

@spastorino

spastorino Dec 9, 2015

Member

Remove this

@spastorino

spastorino Dec 9, 2015

Member

Remove this

jmbejar added some commits Jul 7, 2015

Do not add format key to request_params
I did this change but it is affecting how the request params end up
after being processed by the router.

To be in the safe side, I just take the format from the extension in the
URL when is not present in those params and it's being used only for the
`Request#formats` method
We don't need to set config.debug_exception_response_format given tha…
…t :api is the default value for only API apps
Show outdated Hide outdated actionpack/lib/action_dispatch/http/mime_negotiation.rb
@@ -67,6 +67,8 @@ def formats
v = if params_readable
Array(Mime[parameters[:format]])
elsif format_from_path_extension
[Mime[format_from_path_extension]]

This comment has been minimized.

@spastorino

spastorino Dec 9, 2015

Member

You're calling this method twice without memoizing the result. I'd store the result in a local var or try to memoize in some way.

@spastorino

spastorino Dec 9, 2015

Member

You're calling this method twice without memoizing the result. I'd store the result in a local var or try to memoize in some way.

@spastorino spastorino assigned spastorino and unassigned rafaelfranca Dec 9, 2015

spastorino added a commit that referenced this pull request Dec 9, 2015

Merge pull request #20831 from jmbejar/rails-api-json-error-response
Rails API: Ability to return error responses in json format also in development

@spastorino spastorino merged commit b11bca9 into rails:master Dec 9, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@prathamesh-sonpatki

This comment has been minimized.

Show comment
Hide comment
@prathamesh-sonpatki

prathamesh-sonpatki Apr 23, 2016

Member

@jmbejar Is the intent here to always return JSON response? Whenever we switch to :api mode for debug_exception_response_format, even HTML requests return JSON response.

@jmbejar Is the intent here to always return JSON response? Whenever we switch to :api mode for debug_exception_response_format, even HTML requests return JSON response.

This comment has been minimized.

Show comment
Hide comment
@jmbejar

jmbejar Apr 25, 2016

Contributor

The original intention was that error responses in :api mode always returns using the request's format. If the format can not be determined, JSON is the fallback. However, we focused mostly on API formats: JSON and XML.

Looking at the code now, I think we are not properly handling the case of HTML requests using the :api mode for debug_exception_response_format. I feel like we should render the normal HTML error page for HTML requests, whether is :api or :default mode.

Thoughs? /cc @spastorino

Contributor

jmbejar replied Apr 25, 2016

The original intention was that error responses in :api mode always returns using the request's format. If the format can not be determined, JSON is the fallback. However, we focused mostly on API formats: JSON and XML.

Looking at the code now, I think we are not properly handling the case of HTML requests using the :api mode for debug_exception_response_format. I feel like we should render the normal HTML error page for HTML requests, whether is :api or :default mode.

Thoughs? /cc @spastorino

This comment has been minimized.

Show comment
Hide comment
@prathamesh-sonpatki

prathamesh-sonpatki Apr 25, 2016

Member

Thanks. I was thinking on the same lines.

I feel like we should render the normal HTML error page for HTML requests, whether is :api or :default mode.

Do you mean we should handle this inside render_for_api_application method right?

Thanks. I was thinking on the same lines.

I feel like we should render the normal HTML error page for HTML requests, whether is :api or :default mode.

Do you mean we should handle this inside render_for_api_application method right?

This comment has been minimized.

Show comment
Hide comment
@jmbejar

jmbejar Apr 25, 2016

Contributor

We should probably use render_for_non_api_application if the format is HTML.

We're currently doing something like

if @api_only
  render_for_api_application
else
  render_for_non_api_application
end

I guess we could check the HTML format beforehand:

if @api_only && format != HTML
  render_for_api_application
else
  render_for_non_api_application
end
Contributor

jmbejar replied Apr 25, 2016

We should probably use render_for_non_api_application if the format is HTML.

We're currently doing something like

if @api_only
  render_for_api_application
else
  render_for_non_api_application
end

I guess we could check the HTML format beforehand:

if @api_only && format != HTML
  render_for_api_application
else
  render_for_non_api_application
end

This comment has been minimized.

Show comment
Hide comment
@jmbejar

jmbejar Apr 25, 2016

Contributor

If we follow this approach, render_for_api_application and render_for_non_api_application could have better names. Something like render_error_with_html_format and render_error_with_api_format or similar names.

Contributor

jmbejar replied Apr 25, 2016

If we follow this approach, render_for_api_application and render_for_non_api_application could have better names. Something like render_error_with_html_format and render_error_with_api_format or similar names.

This comment has been minimized.

Show comment
Hide comment

👍

This comment has been minimized.

Show comment
Hide comment
@jmbejar

jmbejar Apr 25, 2016

Contributor

@prathamesh-sonpatki Is it OK if I follow up this issue in a separate PR? Or do you plan to work on it instead?

Contributor

jmbejar replied Apr 25, 2016

@prathamesh-sonpatki Is it OK if I follow up this issue in a separate PR? Or do you plan to work on it instead?

This comment has been minimized.

Show comment
Hide comment
@vipulnsward

vipulnsward Apr 25, 2016

Member

👍 for PR. This is another thing @spastorino can look into at Railsconf 😄

Member

vipulnsward replied Apr 25, 2016

👍 for PR. This is another thing @spastorino can look into at Railsconf 😄

This comment has been minimized.

Show comment
Hide comment
@prathamesh-sonpatki

prathamesh-sonpatki Apr 25, 2016

Member

@jmbejar Either way. I haven't started working on it :)

@jmbejar Either way. I haven't started working on it :)

This comment has been minimized.

Show comment
Hide comment
@spastorino

spastorino Apr 25, 2016

Member

as @vipulnsward stated, we will look at this issue during RailsConf 😄

Member

spastorino replied Apr 25, 2016

as @vipulnsward stated, we will look at this issue during RailsConf 😄

@vipulnsward

This comment has been minimized.

Show comment
Hide comment
@vipulnsward

vipulnsward Apr 25, 2016

Member

Even here, we have the same behaviour.
Has this changed with API work, or has been so before?
If it has been so before, then what we have currently makes sense.

Even here, we have the same behaviour.
Has this changed with API work, or has been so before?
If it has been so before, then what we have currently makes sense.

This comment has been minimized.

Show comment
Hide comment
@jmbejar

jmbejar Apr 25, 2016

Contributor

The render_for_non_api_application method includes the classic behavior, it's how Rails error pages are being rendered without the :api option (and it behaves the same as it has been before the rails-api merge into Rails).

In this case, the if statement differentiates regular requests from ajax requests (where an HTML response does not make sense), it's not a problem with the formats as it's in the :api case that is being under discussion.

Contributor

jmbejar replied Apr 25, 2016

The render_for_non_api_application method includes the classic behavior, it's how Rails error pages are being rendered without the :api option (and it behaves the same as it has been before the rails-api merge into Rails).

In this case, the if statement differentiates regular requests from ajax requests (where an HTML response does not make sense), it's not a problem with the formats as it's in the :api case that is being under discussion.

This comment has been minimized.

Show comment
Hide comment
@vipulnsward

vipulnsward Apr 25, 2016

Member

So what I understand is:

  • If its non-api App, we render plain text even for json/html formats for xhr requests.
  • If its Api App we always render in respective format except html format, where its json for xhr requests.

I think we need to map out all these behaviours and confirm the expected ones. If what we are doing right now in render_for_non_api_application is to be considered, this is already not adhering to formats.

P.S: I would say we should fall back to json, just like we do right now.

Member

vipulnsward replied Apr 25, 2016

So what I understand is:

  • If its non-api App, we render plain text even for json/html formats for xhr requests.
  • If its Api App we always render in respective format except html format, where its json for xhr requests.

I think we need to map out all these behaviours and confirm the expected ones. If what we are doing right now in render_for_non_api_application is to be considered, this is already not adhering to formats.

P.S: I would say we should fall back to json, just like we do right now.

This comment has been minimized.

Show comment
Hide comment
@prathamesh-sonpatki

prathamesh-sonpatki Apr 25, 2016

Member

P.S: I would say we should fall back to json, just like we do right now.

Falling back to JSON in case of API app is fine. The only surprising thing was getting JSON response in an API app for HTML requests.

P.S: I would say we should fall back to json, just like we do right now.

Falling back to JSON in case of API app is fine. The only surprising thing was getting JSON response in an API app for HTML requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment