Placement of 'after_save' callback before 'after_*' callbacks causes false negatives and code duplication

[jerridan]

I have a rails model with many callbacks - including:

after_create :trigger_created_event, unless: :skip_subscriptions
after_update :trigger_updated_event, unless: :skip_subscriptions
after_destroy :trigger_deleted_event, unless: :skip_subscriptions

Rubocop returned no errors.

I added an after_save callback to the beginning of the group:

after_save :update_status
after_create :trigger_created_event, unless: :skip_subscriptions
after_update :trigger_updated_event, unless: :skip_subscriptions
after_destroy :trigger_deleted_event, unless: :skip_subscriptions

I then ran rubocop -A, which "corrected" many errors: 434 offenses detected, 434 offenses corrected

Looking at the file, I saw that some of the callbacks (outside of the above list) had been moved, with rubocop reporting that they were in the wrong order (they were not). Other callbacks were duplicated by rubocop up to ~50 times.

I undid those changes by rubocop, and changed the ordering of the after_* callbacks, like so:

after_create :trigger_created_event, unless: :skip_subscriptions
after_update :trigger_updated_event, unless: :skip_subscriptions
after_destroy :trigger_deleted_event, unless: :skip_subscriptions
after_save :update_status

Running rubocop -A again returned no errors.

Expected behavior

I would expect that rubocop either: a) Returned no errors, regardless of the ordering of the after_* callbacks b) Returned some error saying that after_save needs to be at the end of the list, if that is intended

Actual behavior

Rubocop returned >400 errors, moving some callbacks and duplicating others many times.

Steps to reproduce the problem

1. Take an existing rails model, with existing callbacks. The more the better.
2. Include a group of after_* callbacks, but not an after_save.
3. Ensure rubocop passes.
4. Add an after_save callback above the other after_* callbacks.
5. Run rubocop

RuboCop version

$ [bundle exec] rubocop -V
1.22.2 (using Parser 3.0.2.0, rubocop-ast 1.12.0, running on ruby 2.7.3 x86_64-darwin19)
  - rubocop-rails 2.12.4
  - rubocop-rspec 2.5.0

Note: Upgrading to 1.22.3 had no effect.

[pirj]

Was that only Rails/ActiveRecordCallbacksOrder? How do you feel about sending a PR with a failing spec to reproduce the issue? We can take it from there.

[jerridan]

Sure, might be a few days before I can get to it, but I'll try to send over a PR with a failing spec

[jerridan]

@pirj So I've learned more about this issue, and it's not what I thought it was, but I think there's still a bug.

By default, everything works as expected. It's because my team has custom rules in our rubocop.yml for the Layout/ClassStructure - ExpectedOrder rules. It's odd though, because the issue exists even though our custom ordering of the after_ callbacks seems to match rubocop's default.

Still working on a failing spec. One thing that would be helpful is if you could suggest a good way of setting custom rules within a spec? I'm having trouble finding an example.

[pirj]

@jerridan Interesting case!

way of setting custom rules within a spec

Do you mean something like that? https://github.com/rubocop/rubocop-rails/blob/3ecda946a111aa43db38f0478fc95606511d7301/spec/rubocop/cop/rails/uniq_before_pluck_spec.rb#L91

[jerridan]

Sorry, it's been a while since I looked at this. Unfortunately, I've been unable to write a test case that replicates what I see. I think it has to do with both Layout/ClassStructure - ExpectedOrder and Rails/ActiveRecordCallbacksOrder having the same error, and somehow causing a cascading effect. But I can't be sure, since I can't replicate it in a test.

In any case, we were able to fix things on our end relatively quickly, so we aren't blocked.