Add Rails/UnprocessableContentStatus cop
#1520
Conversation
d881eba to
a4824e5
Compare
|
|
||
| private | ||
|
|
||
| def rack_3_1_or_newer? |
There was a problem hiding this comment.
I believe you can use requires_gem for this: https://docs.rubocop.org/rubocop/development.html#requiring-a-gem
You may have to also do let(:gem_versions) { { 'rack' => '3.1.0' } } in tests to have them actually run, not 100% sure about that though
There was a problem hiding this comment.
TIL I learned about requires_gem! Thanks for letting me know. It's much nicer.
| # render json: { error: "Invalid data" }, status: :unprocessable_content | ||
| # head :unprocessable_content | ||
| # | ||
| class UnprocessableContentStatus < Base |
There was a problem hiding this comment.
I think it would be better to name it Rails/DeprecatedHttpStatusNames instead of specializing it to a specific name. This way, it can be extended in the future when new deprecated status names appear.
Additionally, this cop could also check for payload_too_large.
https://github.com/rack/rack/blob/v3.2.0/lib/rack/utils.rb#L576-L579
There was a problem hiding this comment.
Good suggestion! I've updated the cop to be more generally useful. I noticed the HttpStatus cop too and took some inspiration for the updated matcher. I also considered whether it made sense to have that cop handle the deprecations, but thought the deprecation may be helpful to have separately, and to not change the isolated focus of that.
| (pair (sym :status) (sym :unprocessable_entity)) | ||
| PATTERN | ||
|
|
||
| def on_sym(node) |
There was a problem hiding this comment.
This is only for head :unprocessable_entity, right? redirect_to is already handled by on_pair, same with render.
In that case it seems better to use on_send with RESTRICT_ON_SEND and simply look at the first positional argument.
There was a problem hiding this comment.
That is what it was for. I looked at Rails/HttpStatus and used the same approach there, which I think probably makes the most sense.
There was a problem hiding this comment.
Rails actually considers both symbols to part of their public api, regardless of what rack does with them: rails/rails#53383. So the next rails releases will no longer show the warning from rack.
This could still be nice for consistency + the other status code koic mentioned.
ec4e9e4 to
cbd4725
Compare
|
@koic this would be valuable in rubygems.org codebase. Any plan to merge and release? |
| payload_too_large: :content_too_large | ||
| }.freeze | ||
|
|
||
| MSG = 'Use `:%<preferred>s` instead of `:%<deprecated>s`. The `:%<deprecated>s` status is deprecated.' |
There was a problem hiding this comment.
It can be made into a slightly simpler message.
| MSG = 'Use `:%<preferred>s` instead of `:%<deprecated>s`. The `:%<deprecated>s` status is deprecated.' | |
| MSG = 'Prefer `:%<preferred>s` over `:%<deprecated>s`.' |
| module RuboCop | ||
| module Cop | ||
| module Rails | ||
| # Enforces the use of the current HTTP status names instead of deprecated ones. |
There was a problem hiding this comment.
| # Enforces the use of the current HTTP status names instead of deprecated ones. | |
| # Enforces consistency by using the current HTTP status names. |
config/default.yml
Outdated
| VersionChanged: '2.18' | ||
|
|
||
| Rails/DeprecatedHttpStatusNames: | ||
| Description: 'Use the new HTTP status names instead of deprecated ones.' |
There was a problem hiding this comment.
| Description: 'Use the new HTTP status names instead of deprecated ones.' | |
| Description: 'Enforces consistency by using the current HTTP status names.' |
| # render json: { error: "Invalid data" }, status: :unprocessable_content | ||
| # head :content_too_large | ||
| # | ||
| class DeprecatedHttpStatusNames < Base |
There was a problem hiding this comment.
Since the warning has disappeared, how about naming it as follows?
| class DeprecatedHttpStatusNames < Base | |
| class HttpStatusNameConsistency < Base |
|
|
||
| RESTRICT_ON_SEND = %i[render redirect_to head assert_response assert_redirected_to].freeze | ||
|
|
||
| DEPRECATED_STATUSES = { |
There was a problem hiding this comment.
Could you also rename these constant and method names?
There was a problem hiding this comment.
@koic I updated the constant to be PREFERRED_STATUSES. Did you have another name in mind?
As for RESTRICT_ON_SEND, what's the issue with that name? I used the same naming as the one in RuboCop::Cop::Rails::HttpStatus.
There was a problem hiding this comment.
RESTRICT_ON_SEND is fine; find_deprecated_status_names should be renamed
There was a problem hiding this comment.
Ah, I see because of the deprecated mention. I renamed the method check_status_name_consistency. I removed the find part since the result of each call isn't used.
| add_offense(node, message: message) do |corrector| | ||
| corrector.replace(node, ":#{preferred_status}") | ||
| end | ||
| elsif node.respond_to?(:children) |
There was a problem hiding this comment.
Seems like this condition is not necessary? We do check for child.is_a?(Parser::AST::Node) later
| context 'with :unprocessable_entity' do | ||
| it 'registers an offense when using :unprocessable_entity in render' do | ||
| expect_offense(<<~RUBY) | ||
| render json: { error: 'Invalid data' }, status: :unprocessable_entity |
There was a problem hiding this comment.
This case works fine, but let's add an assertion that head or render with variable status (e.g. head some_variable) do not lead to offenses
|
LGTM, hope it will be merged and released soon 👍🏻 |
| deprecated_status = node.value | ||
| preferred_status = PREFERRED_STATUSES[deprecated_status] | ||
|
|
||
| message = format(MSG, deprecated: deprecated_status, preferred: preferred_status) |
There was a problem hiding this comment.
There seem to be some outdated deprecated terms left around this area. Can you update them to more appropriate terminology?
| end | ||
|
|
||
| context 'with mixed deprecated statuses' do | ||
| it 'handles multiple deprecated statuses in the same code' do |
|
@tuxagon Looks good to me. Can you squash your commits into one? |
About
In Rack
3.1.0, the status naming for HTTP status 422 was changed from:unprocessable_entityto:unprocessable_contentto reflect the status. Both are supported, but there is a warning message that will display if using:unprocessable_entity.This new cop provides a way to have Rubocop register the old status name usages as offenses and auto-correct them as well.
Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).bundle exec rake default. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.mdif the new code introduces user-observable changes. See changelog entry format for details.If this is a new cop, consider making a corresponding update to the Rails Style Guide.