Add cop to check for ActiveModel errors hash direct manipulation #491

lulalala · 2021-05-20T12:51:29Z

This cop checks direct manipulation of ActiveModel#errors as hash.
These operations are deprecated in Rails 6.1 and will not work in Rails 7.

This was recently added to GitLab codebase and doesn't seem to create too much noise, but any advice is welcomed.

It's my first time using the parser so if there is a more elegant way to write the checker please let me know ^^.

# bad
user.errors[:name] << 'msg'
user.errors.messages[:name] << 'msg'

# good
user.errors.add(:name, 'msg')

# bad
user.errors[:name].clear
user.errors.messages[:name].clear

# good
user.errors.delete(:name)

Before submitting the PR make sure the following are checked:

Wrote good commit messages.
Commit message starts with [Fix #issue-number] (if the related issue exists).
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
The PR relates to only one subject with a clear title
and description in grammatically correct, complete sentences.
Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.
If this is a new cop, consider making a corresponding update to the Rails Style Guide.

koic · 2021-05-21T09:58:27Z

config/default.yml

@@ -49,6 +49,11 @@ Rails/ActionFilter:
  Include:
    - app/controllers/**/*.rb

+Rails/ActiveModelErrorsDirectManipulation:
+  Description: 'Avoid manipulating ActiveModel errors hash directly.'
+  Enabled: false


Why is it disabled by default?

I was not sure if this will be helpful, or creating too much noise. But if you are okay then I do want to try making it enabled.

koic · 2021-05-21T10:02:23Z

lib/rubocop/cop/rails/active_model_errors_direct_manipulation.rb

+module RuboCop
+  module Cop
+    module Rails
+      # This cop checks direct manipulation of ActiveModel#errors as hash.


Is it ActiveModel::Errors or ActiveModel::Validators#errors?

Suggested change

# This cop checks direct manipulation of ActiveModel#errors as hash.

# This cop checks direct manipulation of `ActiveModel::Errors` as hash.

The method is defined in ActiveModel::Validations#errors, but people mostly know it as model.errors. It is also a class of ActiveModel::Errors. So both are correct.

koic · 2021-05-21T10:02:29Z

lib/rubocop/cop/rails/active_model_errors_direct_manipulation.rb

+      #   # good
+      #   user.errors.delete(:name)
+      #
+      class ActiveModelErrorsDirectManipulation < Base


How about the name DeprecatedActiveModelErrorsMethods?

If the cop is targeting Rails 6.1 or higher only, can you specify extend TargetRailsVersion and minimum_target_rails_version 6.1 (and add tests)?

https://github.com/rubocop/rubocop-rails/blob/v2.10.1/lib/rubocop/cop/rails/application_record.rb#L21-L23

https://github.com/rubocop/rubocop-rails/blob/v2.10.1/spec/rubocop/cop/rails/application_record_spec.rb#L4

https://github.com/rubocop/rubocop-rails/blob/v2.10.1/spec/rubocop/cop/rails/application_record_spec.rb#L52

There are many other methods which are deprecated. This is only the dynamic manipulation part. What do you think of DeprecatedActiveModelErrorsDynamicManipulation?

If the cop is targeting Rails 6.1 or higher only

I recommend running this before upgrading to Rails 6.1, so developers can upgrade the usage, and be ready for Rails 7 in the future. In theory it should be runnable since Rails 3.0.

How about the name DeprecatedActiveModelErrorsMethods?

I originally aligned this with other cops, e.g.: ActiveRecordCallbacksOrder and ActiveSupportAliases, so I feel maybe we should keep this for consistency?

lib/rubocop/cop/rails/active_model_errors_direct_manipulation.rb

spec/rubocop/cop/rails/active_model_errors_direct_manipulation_spec.rb

koic · 2021-05-21T10:04:44Z

spec/rubocop/cop/rails/active_model_errors_direct_manipulation_spec.rb

+        RUBY
+      end
+    end
+  end


It may be acceptable if receiver is not specified:
errors[:name] << 'msg'

Can you add the test case?

Now I think about it, I feel it is important if the receiver is not specified. However it could leave to many false positives. I want to find a way so this receiver-less check can be done only to files under the models directory. I wonder if that is possible.

koic · 2021-05-21T10:06:57Z

lib/rubocop/cop/rails/active_model_errors_direct_manipulation.rb

+      #   user.errors.delete(:name)
+      #
+      class ActiveModelErrorsDirectManipulation < Base
+        MSG = 'Avoid manipulating ActiveModel errors as hash directly.'


Suggested change

MSG = 'Avoid manipulating ActiveModel errors as hash directly.'

MSG = 'Avoid manipulating `ActiveModel::Errors` as hash directly.'

swanson · 2021-05-24T15:24:00Z

@lulalala Is there any guidance on how to migrate away from direct Hash access when testing? Not sure if it fits better as an rubocop-rspec check, but I can imagine there are lots of codebases using these patterns:

expect(model.errors).to have_key(:name)
expect(model.errors[:name]).to include("cannot be blank")

lulalala · 2021-05-24T15:54:28Z

@lulalala Is there any guidance on how to migrate away from direct Hash access when testing? Not sure if it fits better as an rubocop-rspec check, but I can imagine there are lots of codebases using these patterns:

@swanson This cop will only dealt with manipulation, not reading(accessing), but to answer your questions:

expect(model.errors).to have_key(:name)

For this, we will be moving to:

expect(model.errors.attribute_names).to include(:name)

expect(model.errors[:name]).to include("cannot be blank")

My preferred way would be:

expect(model.errors.added?(:name, :blank)

But if you have suggestions or other thoughts, let me know :).

lulalala · 2021-05-24T15:59:06Z

Hi @koic I've made some changes and replied to your questions. Could you check please? Thanks! 🏓

lulalala · 2021-06-09T12:46:49Z

Hi @koic could you recheck please? Thanks!

lulalala · 2021-07-18T08:49:50Z

Hi @koic could you recheck please? I've rebased and everything is passing now. Thanks!

lulalala · 2021-09-02T06:10:23Z

Hi @koic could you recheck please? I've rebased and everything is passing now. Thanks!

pirj

Useful cop, indeed. Thanks for the contribution.
A few mostly cosmetic notes below.

Before it's merged, would you run the cop against some large codebases to check how well it performs?

CHANGELOG.md

docs/modules/ROOT/pages/cops.adoc

pirj · 2021-12-25T08:11:29Z

lib/rubocop/cop/rails/active_model_errors_direct_manipulation.rb

+        private
+
+        def file_type
+          filename = File.expand_path(processed_source.buffer.name)


Why is expand_path needed? What is usually in processed_source.buffer.name, just user.rb?

After some checking processed_source.buffer.name just returns full path name. Sorry it was long ago so I can't remember why I added this 😛. I've remove it.

pirj · 2021-12-25T08:25:08Z

lib/rubocop/cop/rails/active_model_errors_direct_manipulation.rb

+          model_file: '{nil? send ivar lvar}'
+        }.freeze
+
+        PATTERN = {


Would you agree to separate those patterns and define them statically? It's possible to combine them with { }.
Like:

MANIPULATIVE_METHODS = Set[:<<, :append, ...].freeze def_node_pattern :root_manipulation?, <<~PATTERN (send (send (send %1 :errors) :[] ...) MANIPULATIVE_METHODS ... ) ) PATTERN def_node_pattern :any_manipulation?, <<~PATTERN { root_manipulation?(%1) root_assignment?(%1) } PATTERN def_node_pattern :general?, <<~PATTERN {send ivar lvar} PATTERN def on_send(node) send_pattern = '{nil? send ivar lvar}' # I might be mistaken, check docs https://docs.rubocop.org/rubocop-ast/node_pattern.html#composing-complex-expressions-with-multiple-matchers else :general? # or like this? https://github.com/rubocop/rubocop-rspec/blob/573f22bbbd54e5332231b9823b87843550ae21a4/lib/rubocop/cop/rspec/multiple_expectations.rb#L91 end any_manipulation?(node, send_pattern) do |node| add_offence(...) end end

Sure if we can avoid all these string interpolation. However I actually know very little about how the rubocop-ast works. I tried to apply what you provided, but it would say

undefined method `def_node_pattern' for RuboCop::Cop::Rails::ActiveModelErrorsDirectManipulation:Class
Did you mean? def_node_matcher

When I tried using def_node_matcher it would say:

RuboCop::AST::NodePattern::Invalid:
parse error on value :")" (")")

If you could pull some AST expertise on this it would be greatly appreciated.

Let's try it step by step.
Constant simplification:

I'll just send the whole thing as I see a canonical implementation.
Leaving it up to you if you find it readable - it's up to you to support the cop (hopefully :) anyway.

class ActiveModelErrorsDirectManipulation < Base MSG = 'Avoid manipulating ActiveModel errors as hash directly.' MANIPULATIVE_METHODS = Set[%i[<< append clear collect! compact! concat delete delete_at delete_if drop drop_while fill filter! keep_if flatten! insert map! pop prepend push reject! replace reverse! rotate! select! shift shuffle! slice! sort! sort_by! uniq! unshift]].freeze def_node_matcher :general?, '{send ivar lvar}' def_node_matcher :model?, '{nil? send ivar lvar}' def_node_matcher :any_manipulation?, <<~PATTERN { #root_manipulation? #root_assignment? #messages_details_manipulation? #messages_details_assignment? } PATTERN def_node_matcher :root_manipulation?, <<~PATTERN (send (send (send #file_type_match? :errors) :[] ...) #MANIPULATIVE_METHODS ... ) PATTERN def_node_matcher :root_assignment?, <<~PATTERN (send (send #file_type_match? :errors) :[]= ...) PATTERN def_node_matcher :messages_details_manipulation?, <<~PATTERN (send (send (send (send #file_type_match? :errors) {:messages :details}) :[] ...) #MANIPULATIVE_METHODS ...) PATTERN def_node_matcher :messages_details_assignment?, <<~PATTERN (send (send (send #file_type_match? :errors) {:messages :details}) :[]= ...) PATTERN def on_send(node) any_manipulation?(node) do add_offense(node) end end private def file_type_match?(node) model_file? ? model?(node) : general?(node) end def model_file? processed_source.buffer.name.include?('/models/') end end

Thanks so much! I made some renamings and it works. I've placed this as a separate commit with you being the author. Could you review? 🏓

lulalala · 2021-12-28T16:27:03Z

Thanks @pirj I've pushed some changes up (except the last one because it's not working).

This is extracted from gitlab, which is quite large already, but I still went ahead and tried "real-world-rails". Would you happen to know how to rubocop into git submodules' content?

pirj · 2021-12-28T18:23:17Z

I guess the failure is due to

expected "add_active_model_errors_direct_manipulation.md" to match /\A(new|fix|change)_.+/

it should start with "new_".

config/default.yml

pirj

Perfect, thank you for the contribution, your patience and persistence! 👏

Merry Christmas and a Happy New year, @lulalala !

pirj · 2021-12-29T07:15:27Z

config/default.yml

+Rails/ActiveModelErrorsDirectManipulation:
+  Description: 'Avoid manipulating ActiveModel errors hash directly.'
+  Enabled: pending
+  Safe: false


Just out of curiosity – is there a good reason to make this cop unsafe? There is a possibility that it detects non-AR statements, but I haven't seen such code in practice.

Going back to the question I forgot to answer:

Would you happen to know how to rubocop into git submodules' content?

You git checkout --include-submodules, and then fd --hidden --no-ignore .rubocop | xargs rm. I don't know a better way to skip submodule's RuboCop configuration.

But usually GitLab's source is a good enough sandbox.

@pirj Yes, non-AR classes with errors method is what I fear, would that qualify as "unsafe"? We don't have those in GitLab codebase, but I feel errors is generic enough that it can happen in utility classes.

Thanks. I found that there are some false positives on non-manipulative methods:

redmine/app/controllers/wiki_controller.rb:74:10: C: Rails/ActiveModelErrorsDirectManipulation: Avoid manipulating ActiveModel errors as hash directly. if @page.errors[:title].blank? ^^^^^^^^^^^^^^^^^^^^^^^^^^^

This happens after the refactoring. I might be able to take a look tomorrow (and adding more tests). So please hold off from merging for now. Thanks!

@pirj I've added specs and fixed the issue:

The MANIPULATIVE_METHODS was a single element Set (the element being an array). I splat it now.

The constant seems to be references without # so I removed it.

Spec seems to pass so I guess all is good?

pirj · 2022-01-03T16:23:14Z

Looks good to me!

lulalala · 2022-01-04T06:42:27Z

@pirj great! Should I ask you or @koic to merge?

koic · 2022-01-05T06:44:59Z

I'm wondering about false positives for many of the methods defined in MANIPULATIVE_METHODS.

Maybe it seems better useful to add auto-correction with a cop called Rails/DeprecatedActiveModelErrorsMethods that is limited to the following three deprecated methods.

% cd path/to/github.com/rails/rails
% git checkout v6.1.4
% git grep 'ActiveModel::Errors message'
activemodel/lib/active_model/errors.rb:617:      ActiveSupport::Deprecation.warn("Calling `delete` to an ActiveModel::Errors messages hash is deprecated. Please call `ActiveModel::Errors#delete` instead.")
activemodel/lib/active_model/errors.rb:646:      ActiveSupport::Deprecation.warn("Calling `<<` to an ActiveModel::Errors message array in order to add an error is deprecated. Please call `ActiveModel::Errors#add` instead.")
activemodel/lib/active_model/errors.rb:654:      ActiveSupport::Deprecation.warn("Calling `clear` to an ActiveModel::Errors message array in order to delete all errors is deprecated. Please call `ActiveModel::Errors#delete` instead.")

lulalala · 2022-01-05T16:17:50Z

@koic Due to the false positives I don't think adding auto correct is a good idea. It's better for them to inspect each case and manually adjust. If you are afraid of false positives, how about make this off by default?

The example you grepped in Rails was written by me (and there are a few others like slice!). I think I should have marked other Enumerable methods as deprecated. I totally forgot, but that's exactly why this cop is valuable in finding all of these manipulative methods, and not just the 3 you mentioned. WDYT?

koic · 2022-01-06T04:18:49Z

Well, I will merge this cop after the next bug fix release.

Before that, please add @safety directive and that description to the doc and squash your commits into one.
e.g. https://github.com/rubocop/rubocop-rails/pull/575/files#diff-55131ec8d7f289891ec6c20ab3d87bdf66c4c9da7bb29bc9a5482abf6e1ba2ed

Apart from that, I may have a look at what to do with (unsafe) auto-correction later. Anyway, it will not covered in this PR :-)

lulalala · 2022-01-06T07:39:23Z

@koic I've added @safety now. Thanks.

You already mentioned that you will review this after the next bug release though (which was 2.12.3). I was hoping we can speed up the process if there is nothing blocking the way, please?

koic · 2022-01-06T07:49:55Z

lib/rubocop/cop/rails/active_model_errors_direct_manipulation.rb

+      # @safety
+      #   This cop is unsafe because it can report `errors` manipulation on non-ActiveModel,
+      #   which is obviously valid.
+      #   The cop has no way of knowing whether a variable is an ActiveModel or not,


Is the comma at the end of the sentence a typo?

Yes, my bad. I've corrected this.

koic

I think this cop useful. One change I would like you to make is to rename to Rails/DeprecatedActiveModelErrorsMethods cop. I was stuck at the name Rails/ActiveModelErrorsDirectManipulation cop, so I will merge this PR after renaming.

lulalala · 2022-03-06T07:08:18Z

@koic I've renamed the cop. Thanks.

koic · 2022-03-06T09:07:55Z

This looks good to me. Can you squash your commits into one?

These are deprecated in Rails 6.1 and will be removed in Rails 7. See rails/rails#32313 for details. The cop acts in two modes: For files under `/models` directory, any `errors` call, whether with receiver or not, will be checked. For general files, only `errors` calls with receivers will be checked. E.g. `errors[:bar] = []` is without receiver. It will record an offense if it is a model file. It will not record an offense if it is other general fie. This is to reduce false-positives, since other classes may also have a `errors` method.

lulalala · 2022-03-06T14:21:38Z

@koic OK I've squashed them. Thanks.

koic · 2022-03-06T14:43:13Z

@lulalala Thank you!

lulalala · 2022-11-25T16:12:38Z

`[]` by itself is not deprecated, but `[]<<` is. I think in rails 7 we still want to prevent these kind of direct manipulation since they will not have a lasting effect. Bohdan Schepansky ***@***.***> 於 2022年11月25日週五 20:48 寫道：

…

Looks like this cop has been made obsolete by this commit <rails/rails@ef40a92>, and [] is no longer deprecated. What's interesting, though, is that this has actually happened *before* this PR was even merged... — Reply to this email directly, view it on GitHub <#491 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACZSPQKFOO5ICG4AY2I67LWKCYTRANCNFSM45G6GWOQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

alfuken · 2022-11-26T08:10:41Z

Indeed it is so! I realised my mistake just as I posted that comment, so deleted it right after. :) Sorry for the inconvenience caused.

lulalala force-pushed the model-errors-direct-manipulation branch 2 times, most recently from 138cd0c to 1399e20 Compare May 20, 2021 13:05

lulalala changed the title ~~Add rule to check for ActiveModel errors hash direct manipulation~~ Add cop to check for ActiveModel errors hash direct manipulation May 20, 2021

koic reviewed May 21, 2021

View reviewed changes

lulalala force-pushed the model-errors-direct-manipulation branch from 9b91e7e to 1fcef9d Compare May 24, 2021 14:34

lulalala force-pushed the model-errors-direct-manipulation branch from 1fcef9d to 9bd4fd2 Compare May 24, 2021 15:58

lulalala force-pushed the model-errors-direct-manipulation branch from 9bd4fd2 to ed3c3eb Compare May 28, 2021 10:28

lulalala force-pushed the model-errors-direct-manipulation branch 3 times, most recently from 34dc95b to a019d12 Compare June 9, 2021 12:44

lulalala force-pushed the model-errors-direct-manipulation branch 3 times, most recently from 7900677 to 50c27fe Compare July 18, 2021 08:44

pirj approved these changes Dec 25, 2021

View reviewed changes

lulalala force-pushed the model-errors-direct-manipulation branch from 50c27fe to 7939fbb Compare December 28, 2021 15:27

pirj reviewed Dec 28, 2021

View reviewed changes

config/default.yml Outdated Show resolved Hide resolved

pirj reviewed Dec 28, 2021

View reviewed changes

config/default.yml Outdated Show resolved Hide resolved

lulalala force-pushed the model-errors-direct-manipulation branch 3 times, most recently from d63797d to 62a13f8 Compare December 29, 2021 06:25

pirj approved these changes Dec 29, 2021

View reviewed changes

lulalala force-pushed the model-errors-direct-manipulation branch from 62a13f8 to b82aad2 Compare January 3, 2022 12:07

lulalala force-pushed the model-errors-direct-manipulation branch from b82aad2 to 857df6f Compare January 6, 2022 07:29

koic reviewed Jan 6, 2022

View reviewed changes

koic force-pushed the master branch from 45247fb to 6e31622 Compare January 9, 2022 20:57

lulalala force-pushed the model-errors-direct-manipulation branch from 857df6f to e7ae6bb Compare January 10, 2022 00:52

koic requested changes Mar 4, 2022

View reviewed changes

lulalala force-pushed the model-errors-direct-manipulation branch 2 times, most recently from 70a3473 to be384aa Compare March 6, 2022 07:03

lulalala force-pushed the model-errors-direct-manipulation branch from be384aa to c145336 Compare March 6, 2022 14:20

koic approved these changes Mar 6, 2022

View reviewed changes

koic merged commit 97faf22 into rubocop:master Mar 6, 2022

	# This cop checks direct manipulation of ActiveModel#errors as hash.
	# This cop checks direct manipulation of `ActiveModel::Errors` as hash.

	MSG = 'Avoid manipulating ActiveModel errors as hash directly.'
	MSG = 'Avoid manipulating `ActiveModel::Errors` as hash directly.'

Add cop to check for ActiveModel errors hash direct manipulation #491

Add cop to check for ActiveModel errors hash direct manipulation #491

Conversation

lulalala commented May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swanson commented May 24, 2021

lulalala commented May 24, 2021 • edited Loading

lulalala commented May 24, 2021

lulalala commented Jun 9, 2021

lulalala commented Jul 18, 2021

lulalala commented Sep 2, 2021

pirj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lulalala Dec 29, 2021 • edited Loading

Choose a reason for hiding this comment

lulalala commented Dec 28, 2021

pirj commented Dec 28, 2021

pirj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pirj commented Jan 3, 2022

lulalala commented Jan 4, 2022

koic commented Jan 5, 2022 • edited Loading

lulalala commented Jan 5, 2022

koic commented Jan 6, 2022

lulalala commented Jan 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koic left a comment

Choose a reason for hiding this comment

lulalala commented Mar 6, 2022

koic commented Mar 6, 2022

lulalala commented Mar 6, 2022

koic commented Mar 6, 2022

lulalala commented Nov 25, 2022 via email

alfuken commented Nov 26, 2022

lulalala commented May 20, 2021 •

edited

Loading

lulalala commented May 24, 2021 •

edited

Loading

lulalala Dec 29, 2021 •

edited

Loading

koic commented Jan 5, 2022 •

edited

Loading