-
-
Notifications
You must be signed in to change notification settings - Fork 270
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
Add Rails/ResponseParsedBody
cop
#863
Conversation
I addressed the CI failure with the following Pull Request: |
Note: At first I created the following Pull Request in rubocop-rspec, but it seemed to be more suitable for rubocop-rails, so I re-created this Cop here. |
2229be4
to
5f7bacc
Compare
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.
Nice! We've been using our own version of this cop in our org for many years. We also correct Oj.load(response.body)
(another popular JSON parser) -- but that might be overkill.
# response.parsed_body | ||
class ResponseParsedBody < Base | ||
extend AutoCorrector | ||
|
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.
Add
minimum_target_rails_version 5.0
154caea
to
0867a8e
Compare
# | ||
# @safety | ||
# This cop is unsafe because Content-Type may not be | ||
# `application/json`. |
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.
Is Content-Type guaranteed for code before autocorrection? I'm wondering if autocorrection will change Content-Type. So, If Content-Type doesn't change, it looks like there is no change of safety.
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.
It's unsafe because the behavior changes a bit.
JSON.parse(body.response)
attempts to parse the body as JSON regardless of Content-Type
. e.g. if the controller spits out JSON but marks it as application/xml
, this will still work. Whereas parsed_body
will use the appropriate parser based on Content-Type
, and will raise an exception if the response can't be parsed.
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 get it. It seems to be an incompatibility behavior potential rather than a false positive potential. So it's unsafe for autocorrection.
d77fe72
to
0c5ccee
Compare
Fixes rubocop#940. This PR marks `Rails/ResponseParsedBody` as unsafe because I was misleading below: rubocop#863 (comment) It's actually unsafe.
Since Rails 5, there is an often overlooked feature:
response.parsed_body
.As a typical pattern, many Rails applications have
JSON.parse(response.body)
in their controlelr-specs and request-specs, but most of these could be replaced byresponse.parsed_body
. So it would be nice for rubocop-rails to have a Cop that recommendsresponse.parsed_body
.I think there are several advantages to using
response.parsed_body
.JSON
What do you think?
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}.md
if the new code introduces user-observable changes. See changelog entry format for details.