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

Rack status code name changes breaks HttpStatusMatcher #2763

Closed
darrenboyd opened this issue Jun 12, 2024 · 6 comments · Fixed by #2765
Closed

Rack status code name changes breaks HttpStatusMatcher #2763

darrenboyd opened this issue Jun 12, 2024 · 6 comments · Fixed by #2765

Comments

@darrenboyd
Copy link
Contributor

What Ruby, Rails and RSpec versions are you using?

Ruby version: 3.3.3
Rails version: 7.1.3.4
RSpec version: 3.13.0
Rack version: 3.1.3

Observed behaviour

Rack has recently changed the name of the 422 status code from unprocessable_entity to unprocessable_content. However, both names will still resolve if Rack's public API is being used. However, the HttpStatusMatcher is looking at the Rack Constant directly, instead of using the provided API.

My suggestion is for the code in have_http_status.rb that looks like (around line 218)...

def set_expected_code!
  @expected ||=
    Rack::Utils::SYMBOL_TO_STATUS_CODE.fetch(expected_status) do
      raise ArgumentError,
            "Invalid HTTP status: #{expected_status.inspect}"
    end
end

be changed to....

def set_expected_code!
  @expected ||= Rack::Utils.status_code(expected_status)
end

Here's the behavior of the Rack::Utils.status_code method...

irb> Rack::Utils.status_code(:unprocessable_content)
  => 422
irb> Rack::Utils.status_code(:unprocessable_entity)
  => 422
irb> Rack::Utils.status_code(:invalidcode)
(irb):4:in `<main>': Unrecognized status code :invalidcode (ArgumentError)
@pirj
Copy link
Member

pirj commented Jun 13, 2024

Nice catch. Would you like to submit a PR?

@sarvesh-sankaranarayanan

@pirj @darrenboyd Shouldn't we actually ask developers to use the actual changed status code name as per rack-utils, instead of supporting the old name?

@pirj
Copy link
Member

pirj commented Jun 17, 2024

This is how the public rack api behaves.

Who knows, might be they’ll decide to change the status back?

How would you interpret the definition of the “symbol to status code” as referenced from the assert_response mapping description in Rails guides?

@ioquatix
Copy link

For what it's worth, Rack just follows the specifications as defined by various RFCs.

Relevant RFCs

Both of the following RFCs define 422 but with a one-word difference:

  • RFC 4918, "Proposed Standard", June 2007, "Unprocessable Entity"
  • RFC 9110, "Internet Standard", June 2022, "Unprocessable Content"

Potential Confusion: Even though RFC 9110 it is newer, it does not obsolete RFC 4918 (directly nor transitively).

Arguments in Favor of RFC 9110

  1. RFC 9110 is titled "HTTP Semantics" which is a direct fit for this library, whereas RFC 4918 is scoped to WebDAV.
  2. RFC 9110 is an Internet Standard while RFC 4918 is only a Proposed Standard.
  3. MDN uses RFC 9110 for the 422 status code
  4. RFC 9110 is newer (2022 vs. 2007)

RFC 9110: 422: Unprocessable Content

15.5.21. 422 Unprocessable Content
The 422 (Unprocessable Content) status code indicates that the server understands the content type of the request content (hence a 415 (Unsupported Media Type) status code is inappropriate), and the syntax of the request content is correct, but it was unable to process the contained instructions. For example, this status code can be sent if an XML request content contains well-formed (i.e., syntactically correct), but semantically erroneous XML instructions.

References

From hyperium/http#664.

@darrenboyd
Copy link
Contributor Author

Nice catch. Would you like to submit a PR?

The simplest possible PR is here:

#2765

@sarvesh-sankaranarayanan, yeah, I tend to prefer encouraging people to move forward with changes like this. However, this change is more about using Rack's API in a better way. It looks like the Rack developers are going to add a deprecation warning for this change, and then force people to move over a future version.

darrenboyd added a commit to darrenboyd/rspec-rails that referenced this issue Jun 18, 2024
@ioquatix
Copy link

Nice work team!

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 a pull request may close this issue.

4 participants