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

headers set before error! is missing on response #1719

Closed
gencer opened this issue Dec 17, 2017 · 11 comments
Closed

headers set before error! is missing on response #1719

gencer opened this issue Dec 17, 2017 · 11 comments
Labels

Comments

@gencer
Copy link
Contributor

gencer commented Dec 17, 2017

Whenever I execute error!(), response does not contains my headers. In fact none of the headers like X-Request-Id. I presume this is due to calling a new Request instance and return that instead.

How do I make sure that my important headers are in every request that grape gave? Even in Rails::API I am able to hold headers on any error.

Of course, I could add my own Middleware. Is there any better or easy solution before middleware option?

@dblock
Copy link
Member

dblock commented Dec 17, 2017

What are "your" headers? I think anything you set with header ... should be there, but it looks like we don't document this or another behavior - would appreciate a PR with specs and docs.

You can set additional headers on error with

error! 'Unauthorized', 401, 'X-Error-Detail' => 'Invalid token.'

Lets clarify this for everyone.

@dblock dblock added the bug? label Dec 17, 2017
@gencer
Copy link
Contributor Author

gencer commented Dec 18, 2017

I have few headers that sent to user on every request in every action. I do not want to add them manually on each and every error! call. I already defined them on:

before do
  header ...
  header ...
end

based on request type, user and others. These headers can be different on each request by user or type. So, I would expect grape to respect my before.

Problem is, before gets called but all headers set in before are gone. If this is an expected behavior then I will stick with middlewares.

@dblock
Copy link
Member

dblock commented Dec 19, 2017

Does adding a header inside the API work?

get do
   header ...
   errror!
end

If it does, I think before and after should work the same way and it would be a bug.

Care to write some specs?

@gencer
Copy link
Contributor Author

gencer commented Dec 20, 2017

Note: If you want I can create an another branch with test in spec/ folder.

Expected: Both requests should pass.
Actual: Second test will fail.

error_header_spec.rb:

require 'spec_helper'

describe Grape::API do 
 let(:error_header) do
    Class.new(Grape::API) do
      before do
            header 'X-Grape-Before-Header', 'yeah'
      end
      after do
            header 'X-Grape-After-Header', 'yeah'
      end
      get '/success' do
            header 'X-Grape-Returns-Error', 'yeah'
      end
      get '/error' do
            header 'X-Grape-Returns-Error', 'yeah'
            error!({ success: false })
      end
    end
  end

  subject do
    ErrorHeader = error_header
    Class.new(Grape::API) do
      format :json
      mount ErrorHeader => '/'
    end
  end

  def app
    subject
  end

  it 'should returns all headers on success' do
  get '/success'
    expect(last_response.headers['X-Grape-Returns-Error']).to eq('yeah')
    expect(last_response.headers['X-Grape-Before-Header']).to eq('yeah')
    expect(last_response.headers['X-Grape-After-Header']).to eq('yeah')
  end
  
  it 'should returns all headers on error' do
  get '/error'
    expect(last_response.headers['X-Grape-Before-Header']).to eq('yeah')
    expect(last_response.headers['X-Grape-After-Header']).to eq('yeah')
    expect(last_response.headers['X-Grape-Returns-Error']).to eq('yeah')
  end
end

@dblock
Copy link
Member

dblock commented Dec 20, 2017

Just pull request the failing spec please. Reduce the code a bit, there's a bunch of unnecessary stuff like versioning. Extend the spec to all the scenarios for before, after and during the API so we can cover it all and make sure we fix it properly.

@gencer
Copy link
Contributor Author

gencer commented Dec 20, 2017

@dblock, How is it look like now? (As I am new to this rspec world :)) I would like to get confirm before proper pull request

@gencer gencer changed the title Why my headers are missing on error!()? headers set before error! is missing on response Dec 21, 2017
@fr33z3
Copy link

fr33z3 commented Jan 11, 2018

I think i found a reason for that case.
That's all because of the Grape::Middleware::Error. The problem is here

          error_response(catch(:error) do
            return @app.call(@env)
          end)

on this stage there are no custom headers set. So the headers set in before callbacks are not reachable.
I found a quick work around:

module Middlewares
  class SomeMiddleware < Grape::Middleware::Base
    def before
      header('A', 1.to_s)
    end
  end
end

then
insert_before Grape::Middleware::Error, Middlewares::SomeMiddleware
But the whole concept of such error catching is wrong IMHO and breaks the concept of before callbacks.

@meismann
Copy link

The above PR does not totally address the issue, IMHO. Errors cannot only be raised by calling error! explicitly. Errors may occur in the block given to the HTTP-verb method like

get do
  Model.find(param[:id]) # may raise an ActiveRecord::NotFoundError
end

or by Grape itsself during a POST request (i.e. before entering my code as defined in the endpoint's post-block) when the entity restricts certain attributes to have a state different from the one in which the attribute was posted.
I expect headers in the before block to be set on the response just as well as if I had called error! myself. This should be addressed in another PR. WDYT?

@dblock
Copy link
Member

dblock commented Apr 26, 2019

Can you write and PR a spec that describes this @meismann, please?

@dblock
Copy link
Member

dblock commented Apr 26, 2019

This particular issue as described though, ie. headers set before error! is fixed, so I am going to close it.

@meismann Open a new issue with a spec for the scenario you're describing.

@dblock dblock closed this as completed Apr 26, 2019
@dblock
Copy link
Member

dblock commented Apr 26, 2019

Closed via #1869

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

No branches or pull requests

4 participants