Inconsistent "invalid header value" response codes

[monfresh]

Hi,

After searching through some older issues, it looks like it was agreed that an invalid header value would return a 406 error. Is that still the case? I'm getting mixed responses. Sometimes I get a 500, sometimes a 404.

404:

http -h http://localhost:8080/api/locations Accept:"application/vnd.ohanapi.v1+json"
=> 404 Not Found
=> No route matches [GET] "/api/locations"

500:

http -h http://localhost:8080/api/locations Accept:"application/vnd.ohanapi-v1+json; version=1"
=> 500 Internal Server Error
RuntimeError at /api/locations
==============================

> Invalid header value: "application/vnd.ohanapi-v1+json; version=1"

Here's the full backtrace:

Full backtrace
--------------

 - rack-accept (0.4.5) lib/rack/accept/header.rb:19:in `block in parse'
 - rack-accept (0.4.5) lib/rack/accept/header.rb:13:in `each'
 - rack-accept (0.4.5) lib/rack/accept/header.rb:13:in `parse'
 - rack-accept (0.4.5) lib/rack/accept/header.rb:70:in `initialize'
 - rack-accept (0.4.5) lib/rack/accept/media_type.rb:49:in `initialize'
 - grape (0.5.0) lib/grape/middleware/versioner/header.rb:27:in `new'
 - grape (0.5.0) lib/grape/middleware/versioner/header.rb:27:in `before'
 - grape (0.5.0) lib/grape/middleware/base.rb:23:in `call!'
 - grape (0.5.0) lib/grape/middleware/base.rb:18:in `call'
 - grape (0.5.0) lib/grape/middleware/error.rb:26:in `block in call!'
 - grape (0.5.0) lib/grape/middleware/error.rb:25:in `catch'
 - grape (0.5.0) lib/grape/middleware/error.rb:25:in `call!'
 - grape (0.5.0) lib/grape/middleware/base.rb:18:in `call'
 - rack (1.4.5) lib/rack/head.rb:9:in `call'
 - rack (1.4.5) lib/rack/builder.rb:134:in `call'
 - grape (0.5.0) lib/grape/endpoint.rb:165:in `call!'
 - grape (0.5.0) lib/grape/endpoint.rb:155:in `call'
 - rack-mount (0.8.3) lib/rack/mount/route_set.rb:152:in `block in call'
 - rack-mount (0.8.3) lib/rack/mount/code_generation.rb:96:in `block in recognize'
 - rack-mount (0.8.3) lib/rack/mount/code_generation.rb:75:in `optimized_each'
 - rack-mount (0.8.3) lib/rack/mount/code_generation.rb:95:in `recognize'
 - rack-mount (0.8.3) lib/rack/mount/route_set.rb:141:in `call'
 - grape (0.5.0) lib/grape/api.rb:480:in `call'
 - grape (0.5.0) lib/grape/api.rb:49:in `call!'
 - grape (0.5.0) lib/grape/api.rb:45:in `call'
 - journey (1.0.4) lib/journey/router.rb:68:in `block in call'
 - journey (1.0.4) lib/journey/router.rb:56:in `each'
 - journey (1.0.4) lib/journey/router.rb:56:in `call'
 - actionpack (3.2.13) lib/action_dispatch/routing/route_set.rb:612:in `call'
 - rack-pjax (0.7.0) lib/rack/pjax.rb:12:in `call'
 - newrelic_rpm (3.6.6.147) lib/new_relic/rack/error_collector.rb:43:in `call'
 - newrelic_rpm (3.6.6.147) lib/new_relic/rack/agent_hooks.rb:22:in `call'
 - newrelic_rpm (3.6.6.147) lib/new_relic/rack/browser_monitoring.rb:16:in `call'
 - newrelic_rpm (3.6.6.147) lib/new_relic/rack/developer_mode.rb:28:in `call'
 - bullet (4.6.0) lib/bullet/rack.rb:13:in `call'
 - mongoid (3.1.4) lib/rack/mongoid/middleware/identity_map.rb:34:in `block in call'
 - mongoid (3.1.4) lib/mongoid/unit_of_work.rb:39:in `unit_of_work'
 - mongoid (3.1.4) lib/rack/mongoid/middleware/identity_map.rb:34:in `call'
 - rack-cors (0.2.8) lib/rack/cors.rb:54:in `call'
 - warden (1.2.3) lib/warden/manager.rb:35:in `block in call'
 - warden (1.2.3) lib/warden/manager.rb:34:in `catch'
 - warden (1.2.3) lib/warden/manager.rb:34:in `call'
 - actionpack (3.2.13) lib/action_dispatch/middleware/best_standards_support.rb:17:in `call'
 - rack (1.4.5) lib/rack/etag.rb:23:in `call'
 - rack (1.4.5) lib/rack/conditionalget.rb:25:in `call'
 - actionpack (3.2.13) lib/action_dispatch/middleware/head.rb:14:in `call'
 - remotipart (1.2.1) lib/remotipart/middleware.rb:27:in `call'
 - actionpack (3.2.13) lib/action_dispatch/middleware/params_parser.rb:21:in `call'
 - actionpack (3.2.13) lib/action_dispatch/middleware/flash.rb:242:in `call'
 - rack (1.4.5) lib/rack/session/abstract/id.rb:210:in `context'
 - rack (1.4.5) lib/rack/session/abstract/id.rb:205:in `call'
 - actionpack (3.2.13) lib/action_dispatch/middleware/cookies.rb:341:in `call'
 - actionpack (3.2.13) lib/action_dispatch/middleware/callbacks.rb:28:in `block in call'
 - activesupport (3.2.13) lib/active_support/callbacks.rb:405:in `_run__544010001126416267__call__746849294108779694__callbacks'
 - activesupport (3.2.13) lib/active_support/callbacks.rb:405:in `__run_callback'
 - activesupport (3.2.13) lib/active_support/callbacks.rb:385:in `_run_call_callbacks'
 - activesupport (3.2.13) lib/active_support/callbacks.rb:81:in `run_callbacks'
 - actionpack (3.2.13) lib/action_dispatch/middleware/callbacks.rb:27:in `call'
 - actionpack (3.2.13) lib/action_dispatch/middleware/reloader.rb:65:in `call'
 - actionpack (3.2.13) lib/action_dispatch/middleware/remote_ip.rb:31:in `call'
 - better_errors (0.9.0) lib/better_errors/middleware.rb:84:in `protected_app_call'
 - better_errors (0.9.0) lib/better_errors/middleware.rb:79:in `better_errors_call'
 - better_errors (0.9.0) lib/better_errors/middleware.rb:56:in `call'
 - actionpack (3.2.13) lib/action_dispatch/middleware/debug_exceptions.rb:16:in `call'
 - actionpack (3.2.13) lib/action_dispatch/middleware/show_exceptions.rb:56:in `call'
 - railties (3.2.13) lib/rails/rack/logger.rb:32:in `call_app'
 - railties (3.2.13) lib/rails/rack/logger.rb:16:in `block in call'
 - activesupport (3.2.13) lib/active_support/tagged_logging.rb:22:in `tagged'
 - railties (3.2.13) lib/rails/rack/logger.rb:16:in `call'
 - quiet_assets (1.0.2) lib/quiet_assets.rb:18:in `call_with_quiet_assets'
 - actionpack (3.2.13) lib/action_dispatch/middleware/request_id.rb:22:in `call'
 - rack (1.4.5) lib/rack/methodoverride.rb:21:in `call'
 - rack (1.4.5) lib/rack/runtime.rb:17:in `call'
 - activesupport (3.2.13) lib/active_support/cache/strategy/local_cache.rb:72:in `call'
 - lib/api_defender.rb:47:in `call'
 - rack (1.4.5) lib/rack/lock.rb:15:in `call'
 - actionpack (3.2.13) lib/action_dispatch/middleware/static.rb:63:in `call'
 - rack-timeout (0.0.4) lib/rack/timeout.rb:16:in `block in call'
 - /Users/moncef/.rvm/rubies/ruby-2.0.0-p247/lib/ruby/2.0.0/timeout.rb:66:in `timeout'
 - rack-timeout (0.0.4) lib/rack/timeout.rb:16:in `call'
 - railties (3.2.13) lib/rails/engine.rb:479:in `call'
 - railties (3.2.13) lib/rails/application.rb:223:in `call'
 - railties (3.2.13) lib/rails/railtie/configurable.rb:30:in `method_missing'
 - rack (1.4.5) lib/rack/lint.rb:48:in `_call'
 - rack (1.4.5) lib/rack/lint.rb:36:in `call'
 - rack (1.4.5) lib/rack/showexceptions.rb:24:in `call'
 - rack (1.4.5) lib/rack/commonlogger.rb:33:in `call'
 - rack (1.4.5) lib/rack/chunked.rb:43:in `call'
 - rack (1.4.5) lib/rack/content_length.rb:14:in `call'
 - unicorn (4.6.3) lib/unicorn/http_server.rb:552:in `process_client'
 - unicorn (4.6.3) lib/unicorn/http_server.rb:632:in `worker_loop'
 - unicorn (4.6.3) lib/unicorn/http_server.rb:500:in `spawn_missing_workers'
 - unicorn (4.6.3) lib/unicorn/http_server.rb:142:in `start'
 - unicorn (4.6.3) bin/unicorn:126:in `<top (required)>'
 -  () Users/moncef/.rvm/gems/ruby-2.0.0-p247@ohana-api/bin/unicorn:23:in `load'
 -  () Users/moncef/.rvm/gems/ruby-2.0.0-p247@ohana-api/bin/unicorn:23:in `<main>'
 -  () Users/moncef/.rvm/gems/ruby-2.0.0-p247@ohana-api/bin/ruby_noexec_wrapper:14:in `eval'
 -  () Users/moncef/.rvm/gems/ruby-2.0.0-p247@ohana-api/bin/ruby_noexec_wrapper:14:in `<main>'

[dblock]

Do you think you could build an RSpec test that reproduces this? Should definitely not return a 500.

[bwalex]

This reproduces the problem. I'll look into whether I can fix it.

  context 'version headers' do
    before do
      subject.version 'v1', using: :header, vendor: 'ohanapi'
      subject.get '/test' do
        "Hello!"
      end
    end

    it 'should result in a 406 response if they are invalid' do
      get '/test', {}, 'HTTP_ACCEPT' => 'application/vnd.ohanapi.v1+json'
      last_response.status.should == 406
    end

    it 'should result in a 406 response if they are invalid (2)' do
      get '/test', {}, 'HTTP_ACCEPT' => 'application/vnd.ohanapi.v1+json; version=1'
      last_response.status.should == 406
    end
  end

Failures:

  1) Grape::Endpoint version headers should result in a 406 response if they are invalid
     Failure/Error: last_response.status.should == 406
       expected: 406
            got: 404 (using ==)
     # ./spec/grape/endpoint_spec.rb:747:in `block (3 levels) in <top (required)>'

  2) Grape::Endpoint version headers should result in a 406 response if they are invalid (2)
     Failure/Error: get '/test', {}, 'HTTP_ACCEPT' => 'application/vnd.ohanapi.v1+json; version=1'
     RuntimeError:
       Invalid header value: "application/vnd.ohanapi.v1+json; version=1"
     # ./lib/grape/middleware/versioner/header.rb:26:in `new'
     # ./lib/grape/middleware/versioner/header.rb:26:in `before'
     # ./lib/grape/middleware/base.rb:23:in `call!'
     # ./lib/grape/middleware/base.rb:18:in `call'
     # ./lib/grape/middleware/error.rb:26:in `block in call!'
     # ./lib/grape/middleware/error.rb:25:in `catch'
     # ./lib/grape/middleware/error.rb:25:in `call!'
     # ./lib/grape/middleware/base.rb:18:in `call'
     # ./lib/grape/endpoint.rb:157:in `call!'
     # ./lib/grape/endpoint.rb:145:in `call'
     # ./lib/grape/api.rb:524:in `call'
     # ./lib/grape/api.rb:42:in `call!'
     # ./lib/grape/api.rb:38:in `call'
     # ./spec/grape/endpoint_spec.rb:751:in `block (3 levels) in <top (required)>'

[bwalex]

Fixing the '500' problem is trivial enough, but also results in a 404. there's a comment in the relevant code mentioning X-Cascade so that Rack::Mount attempts other routes. Not sure if that's what's resulting in the 404 instead of the intended 406.

--- i/lib/grape/middleware/versioner/header.rb
+++ w/lib/grape/middleware/versioner/header.rb
@@ -23,7 +23,11 @@ module Grape
       # route.
       class Header < Base
         def before
-          header = Rack::Accept::MediaType.new env['HTTP_ACCEPT']
+          begin
+            header = Rack::Accept::MediaType.new env['HTTP_ACCEPT']
+          rescue Exception => e
+            throw :error, status: 406, headers: error_headers, message: e.to_s
+          end

           if strict?
             # If no Accept header:

[bwalex]

Indeed - setting the cascade: false option on the version seems to work fine and result in 406. I'll clean this up and submit a pull request later today.

[dblock]

Closing, fixed in 0.7.0.