-
-
Notifications
You must be signed in to change notification settings - Fork 120
Gracefully handle the rename of Status 422 Unprocessable Content #490
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
Conversation
lib/hanami/http/status.rb
Outdated
| SYMBOLS = ::Rack::Utils::SYMBOL_TO_STATUS_CODE | ||
| SYMBOLS = ::Rack::Utils::SYMBOL_TO_STATUS_CODE.dup | ||
|
|
||
| if Gem.loaded_specs["rack"].version.version.start_with?("3") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplest way I could think to determine this with a minimum of allocations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking we could just use Rack::VERSION to check against rather than dropping this far, but rack 2's version const is... weird.
I think I might change that second version call for to_s as Gem::Version does respond with a string version
Gem.loaded_specs["rack"].version.to_s.start_with?("3")± be hanami console
forked[development]> Gem.loaded_specs["rack"].version.to_s.start_with?("3")
=> true
forked[development]> Gem.loaded_specs["rack"].version.to_s
=> "3.2.3"
and in rack 2
± be hanami console
forked[development]> Rack::VERSION
=> [1, 3]
forked[development]> Rack
=> Rack
forked[development]> Rack::VERSION.to_s
=> "[1, 3]"
forked[development]> Gem.loaded_specs["rack"].version.version
=> "2.2.20"
forked[development]> Gem.loaded_specs["rack"].version
=> Gem::Version.new("2.2.20")
forked[development]> Gem.loaded_specs["rack"].version.to_s
=> "2.2.20"
forked[development]> Gem.loaded_specs["rack"].version.to_s
=> "2.2.20"
forked[development]> Gem.loaded_specs["rack"].version.class
=> Gem::Version
forked[development]> Gem.loaded_specs["rack"].version.version
=> "2.2.20"
forked[development]> Gem.loaded_specs["rack"].version.to_s
=> "2.2.20"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gem::Version#to_s appears to be an alias to #version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a simple check for this that you can reuse: Hanami::Action.rack_3? See lib/hanami/action/rack_utils.rb and feel free to grep for it across the repo to see how we use it.
That said, based on my comment below, we may no longer need a Rack version check for this PR.
lib/hanami/http/status.rb
Outdated
| if Gem.loaded_specs["rack"].version.version.start_with?("3") | ||
| SYMBOLS[:unprocessable_entity] = 422 | ||
| else | ||
| SYMBOLS[:unprocessable_content] = 422 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we feel about keeping :unprocessable_entity in addition to the new symbol for backwards compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that because we've documented unprocessable_entity, it would be good for us to maintain it in the future.
I think for our 2.3 docs, we should show both unprocessable_content and unprocessable_content as working for 422, and should update this PR to work with both, for either version of Rack.
This feels important during this Rack v2/v3 transitional phase, which I expect we might keep for ~a year or so.
I think this might also simplify this PR? We'd just merge unprocessable_content and unprocessable_content keys into our SYMBOLS hash regardless of the version of rack we see being used?
timriley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for doing this, @alassek! This will take away one little hitch for our users and hopefully make their upgrade nicer :)
I've left a thought via a comment about a change in approach. Keen to hear what you think.
lib/hanami/http/status.rb
Outdated
| SYMBOLS = ::Rack::Utils::SYMBOL_TO_STATUS_CODE | ||
| SYMBOLS = ::Rack::Utils::SYMBOL_TO_STATUS_CODE.dup | ||
|
|
||
| if Gem.loaded_specs["rack"].version.version.start_with?("3") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a simple check for this that you can reuse: Hanami::Action.rack_3? See lib/hanami/action/rack_utils.rb and feel free to grep for it across the repo to see how we use it.
That said, based on my comment below, we may no longer need a Rack version check for this PR.
lib/hanami/http/status.rb
Outdated
| if Gem.loaded_specs["rack"].version.version.start_with?("3") | ||
| SYMBOLS[:unprocessable_entity] = 422 | ||
| else | ||
| SYMBOLS[:unprocessable_content] = 422 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that because we've documented unprocessable_entity, it would be good for us to maintain it in the future.
I think for our 2.3 docs, we should show both unprocessable_content and unprocessable_content as working for 422, and should update this PR to work with both, for either version of Rack.
This feels important during this Rack v2/v3 transitional phase, which I expect we might keep for ~a year or so.
I think this might also simplify this PR? We'd just merge unprocessable_content and unprocessable_content keys into our SYMBOLS hash regardless of the version of rack we see being used?
27f55a1 to
f8fdb6a
Compare
|
@timriley yep that's a cleaner approach, but good to know we have a helper for that. I've also opened a documentation PR at hanami/guides#308 |
timriley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @alassek, looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is clever! 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Written originally for activerecord-pg_enum, I needed very fine-grained version distinction for my test suite
Rack 3 changed the symbolic name of 422 from
:unprocessable_entityto:unprocessable_content.This follows from the rename in the HTTP spec. https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/422