[#1719] Fix missing headers before error! call

[anaumov]

Fix issue with empty headers after error! method call #1719

It happened when headers were set in before block or endpoint body.

[dblock]

I do see what you're trying to do, but this may possibly not be the right fix because error! is now merging headers systematically. I realize no other specs break, but someone is bound to get burnt by it, no?

Please fix the CHANGELOG either way to get a green build.

[dblock]

There's a lot of header vs. headers going on here. I would rename headers = nil into additional_headers and use headers everywhere else in the body of this method as header and headers are aliases for semantic reasons: use header when accessing one and use headers when accessing all.

[anaumov]

@dblock thanks for a quick review! I've updated naming (headers -> additional_headers). There is something about header and headers. They are not aliases in the spec.

If I set puts "headers: #{headers}, header: #{header}" here https://github.com/anaumov/grape/blob/fix/headers_in_error_call/lib/grape/dsl/inside_route.rb#L139 and run bundle exec rspec spec/grape/endpoint_spec.rb:1100, output will be:

headers: {"Host"=>"example.org", "Cookie"=>""}, header: {"X-Test"=>"sample"}

I suspect it comes from the request.rb https://github.com/anaumov/grape/blob/fix/headers_in_error_call/lib/grape/request.rb#L30, but I'm not sure how to handle this.

I realize no other specs break, but someone is bound to get burnt by it, no?

It's possible if user supposes that headers should be blanked after error! method call. But such behavior is not documented.

[dblock]

I know it wasn't there, but lets document additional_headers above?

[dblock]

I realize no other specs break, but someone is bound to get burnt by it, no?

It's possible if user supposes that headers should be blanked after error! method call. But such behavior is not documented.

Could you please document the new behavior in README, add a note to UPGRADING and write a spec that ensures the headers are actually merged as part of this PR? Thank you.

[anaumov]

@dblock I've added docs and specs. Let me know if any corrections needed. Should I squash and rebase?

[dblock]

I squashed and merged. Thank you. Shoot an email to the grape mailing list describing this change and see what people say about it? Ask whether we need to support the previous behavior.

[anaumov]

@dblock cool! I sent an email.