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

Use "insert 0" instead of "use" in README #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

abicky
Copy link

@abicky abicky commented May 1, 2019

The status code won't be correct when it is changed in rescue_from block, so use "insert 0" instead of "use" in README.

I describe more details below.

Before

class MyApi < Grape::API
  use GrapeLogging::Middleware::RequestLogger

  rescue_from :all do |e|
    error!('bad request', 400)
  end

  get do
    raise 'error'
  end
end
% curl -i localhost:3000
HTTP/1.1 400 Bad Request
Content-Type: text/plain
Cache-Control: no-cache
X-Request-Id: 8e5d48cf-c752-4e45-aaa0-ef9c2ac197ef
X-Runtime: 0.030238
Content-Length: 11

bad request

The server log is like below:

[2019-05-02 00:29:25 +0900] INFO -- 500 -- db=0 total=4.98 view=4.98 -- GET / host=localhost params={}

As you can see, the status code in the log is 500 in spite of the fact that the actual status code is 400.

After

class MyApi < Grape::API
  insert 0, GrapeLogging::Middleware::RequestLogger

  rescue_from :all do |e|
    error!('bad request', 400)
  end

  get do
    raise 'error'
  end
end
% curl -i localhost:3000
HTTP/1.1 400 Bad Request
Content-Type: text/plain
Cache-Control: no-cache
X-Request-Id: ba717065-72f3-4de1-8b6d-21b6258714bc
X-Runtime: 0.016160
Content-Length: 11

bad request

The server log is like below:

[2019-05-02 00:30:33 +0900] INFO -- 400 -- db=0 total=1.46 view=1.46 -- GET / host=localhost params={}

Why is the status code incorrect if we use "use"?

The block of rescue_from is called by a subclass of Grape::Middleware::Error but the middleware stack is like below if we use "use":

  1. Rack::Head
  2. A subclass of Grape::Middleware::Error
  3. GrapeLogging::Middleware::RequestLogger

cf. https://github.com/ruby-grape/grape/blob/v1.2.3/lib/grape/endpoint.rb#L282-L294

We can change the order by using "insert 0" and we can see the correct status code in our log.

  1. GrapeLogging::Middleware::RequestLogger
  2. Rack::Head
  3. A subclass of Grape::Middleware::Error

The status code won't be correct when it is changed
in `rescue_from` block, so use "insert 0" instead of
"use" in README.
@thedarkside
Copy link

Thank you for the explanation. Ive already reported it in #45, partially changed with #74 and i have finalized it in the same way by opening the pr #75

Haven't seen this pr up until now.

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 this pull request may close these issues.

2 participants