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

Expose bug with response codes #1565

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

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants