Skip to content

Conversation

@bmarini
Copy link
Contributor

@bmarini bmarini commented Jan 26, 2017

This is related to the 204 no content changes made #1532 and #1549 #1550

Seems like the behavior should be as follows:

1. If status was not set: use that. 2. If status was not set: a) If body is formatted to an empty string, use 204 No Content b) If body is formatted to a non empty string, use a default status for method, (GET: 200, POST: 201, PATCH: 200, DELETE: 200). 
@grape-bot
Copy link

1 Warning
⚠️ Unless you’re refactoring existing code, please update CHANGELOG.md.
1 Message
📖 We really appreciate pull requests that demonstrate issues, even without a fix. That said, the next step is to try and fix the failing tests!

Here's an example of a CHANGELOG.md entry:

* [#1565](https://github.com/ruby-grape/grape/pull/1565): Expose bug with response codes - [@bmarini](https://github.com/bmarini).

Generated by 🚫 danger

@bmarini
Copy link
Contributor Author

bmarini commented Jan 26, 2017

I ran into some issues trying to fix at the status method in inside_route.rb:

 def status(status = nil) case status when Symbol raise ArgumentError, "Status code :#{status} is invalid." unless Rack::Utils::SYMBOL_TO_STATUS_CODE.keys.include?(status) @status = Rack::Utils.status_code(status) when Integer @status = status when nil return @status if @status case request.request_method.to_s.upcase when Grape::Http::Headers::POST 201 when Grape::Http::Headers::DELETE if @body.present? 200 else 204 end else 200 end else raise ArgumentError, 'Status code must be Integer or Symbol.' end
  1. @Body isn't set as the return value of the endpoint block.
  2. If @Body is {} or [] it'll still evaluate as not present, even though it'll serialize to json as a non empty string.

So I think the logic has to be moved til after the formatters run to know if the status should be set to 204 or not.

@coveralls
Copy link

coveralls commented Jan 26, 2017

Coverage Status

Coverage decreased (-0.008%) to 99.063% when pulling ede533e on remind101:expose-response-code-bug into e1a14bf on ruby-grape:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants