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

jsonapi-rails breaks the shortcut form of ActionController#render #24

Closed
smaximov opened this issue May 23, 2017 · 1 comment · Fixed by #25
Closed

jsonapi-rails breaks the shortcut form of ActionController#render #24

smaximov opened this issue May 23, 2017 · 1 comment · Fixed by #25

Comments

@smaximov
Copy link
Contributor

smaximov commented May 23, 2017

jsonapi-rails breaks the shortcut form of render which dispatches on the single argument's value (render :action_name, render 'template/name', render '/file/name').

I have stumbled upon this issue when tried out the 'ActiveRecord Errors' branch.

This can be reproduced on the current HEADs of both master (dc373eb) and ar-validation-errors (498c2b5).

Steps to reproduce

  1. (Optional) Create new gemset to ensure clean environment:
rvm use 2.3.3@jsonapi --create
  1. Create and configure new Rails 4.2 application:
gem install rails:4.2 bundler
rails new jsonapi-test --skip-bundle --skip-sprockets --skip-spring --skip-javascript --skip-turbolinks --skip-test-unit
cd jsonapi-test
cat <<RUBY > Gemfile
source 'https://rubygems.org'
gem 'rails', '4.2'
gem 'sqlite3'
gem 'jsonapi-rails', github: 'jsonapi-rb/jsonapi-rails', ref: 'dc373eb'
RUBY
bundle install
  1. Setup sample resource
bin/rails generate model Post title:text
echo '2.times { |i| Post.create!(title: "Post ##{i + 1}") }' > db/seeds.rb
bin/rake db:create db:migrate db:seed
bin/rails generate jsonapi:serializable Post
  1. Setup sample actions
cat <<RUBY > config/routes.rb
Rails.application.routes.draw do
  get '/api', to: 'application#api'
  get '/html', to: 'application#html'
end
RUBY
cat <<RUBY > app/controllers/application_controller.rb
class ApplicationController < ActionController::Base
  # sample action to make sure jsonapi-rails works
  def api
    render jsonapi: Post.all
  end

  # this action fails
  def html
    render :welcome
  end
end
RUBY
  1. Start Rails server and hit '/api' to make sure jsonapi-rails works.

  2. Hit '/html'

Expected behavior

Rails fails with an ActionView::MissingTemplate error.

Actual behavior

Rails fails with the following error:

NoMethodError (undefined method `merge' for :welcome:Symbol)
  app/controllers/application_controller.rb:9:in `html'

[... backtrace ...]

Workarounds

Some debugging reveals that the error comes from this line (introduced in 20a6756) where #merge is being called on the :welcome Symbol.

I'm not very familiar with the Rails internals, but it seems like the signature of JSONAPI::Rails::ActionController#render (which takes a single hash argument) is not compatible with the ActionPack's render (both AbstractController::Rendering and ActionController::Renderer can take variable number of arguments).

I see several workarounds:

  1. Change the signature of JSONAPI::Rails::ActionController#render and have it normalize its arguments somehow. But this will likely duplicate the work ActionPack does under the hood to process the shortcut syntax of render.

  2. Drop JSONAPI::Rails::ActionController#render and override AbstractController::Rendering#_normalize_options instead. I don't like it because _normalize_options is kinda undocumented and I have not seen a project that actually uses it.

  3. Move providing :_reverse_mapping inside the renderer proc, since the proc is evaluated in the controller context and has access to request. Another benefit is that it will modify options passed to the JSONAPI renderers only.

I have implemented the second and the third options and they both work on the sample application.

@beauby
Copy link
Member

beauby commented May 24, 2017

Thanks a lot for the thorough investigation and explanation! Got my laptop stolen tonight so I'll try to review and merge tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants