"Redundant all" is too naive

[zverok]

I am not sure whether it should be reported as style guide problem, or rubocop-rails problem, but it is a problem :)

Basically, the styleguide claims and the cop supports this claim, that those statements are equivalent:

User.all.<anymethod>
# and
User.<anymethod>

user.articles.all.<anymethod>
# and
user.articles.<anymethod>

The latter is incorrect at least in a case of delete_all (which I shoot myself in a foot with just recently, after update to the latest rubocop-rails):

• user.articles.all.delete_all is ActiveRecord::Relation#delete_all which will just do SQL DELETE, done and forgotten;
• but user.articles.delete_all is ActiveRecord::Associations::CollectionProxy#delete_all which behaves "according to a deletion strategy", which, say, for has_many is by default :nullify.

E.g.:

• user.articles.all.delete_all = DELETE FROM articles WHERE user_id=1 (delete articles in this scope)
• user.articles.delete_all = UPDATE articles WHERE user_id=1 SET user_id=NULL (remove articles of this scope from the collection "articles of this user")

Which is... Quite a difference. I love Rails and its conventions 😍

So... Probably both the style guide and the cop should be much more cautious here.

[koic]

cc @masato-bkn

[pirj]

Hey Victor!

Interesting case with delete_all indeed. But how is the styleguide/cop responsible for such an inconsistent and surprising behavior in Rails?
Without any cop or a guideline, wouldn’t you anyway remove the ‘.all’ part from that statement?

I wouldn’t be surprised if it was a many to many association, like ‘peter.friends.delete_all’. But it would remove records from the join table, not nullify, wouldn’t it?

I’d stick to “no .all unless it’s directly called on the model” and let Rails reconsider its default that goes against the principle of least surprise.

Wondering what actions would you guys take to make everyone’s lifes easier?

[pirj]

The example ‘person.pets.delete(Pet.find(1))’ looks quite weird. I would probably write it differently, but this notation deserves to exist.

In contrast to the surprising nullify for delete_all on has_many, this is a real dilemma. Pets can become orphans, and another person can adopt it. But an article, a doctor appointment, a diary note?
How would Rails know the semantic? They had to pick a default, and they chose a safer one that doesn’t remove records. And that would fail on the DB level with proper FK constraints if the AR mapping is broken, or someone accidentally removes the ‘.all’.

To this moment I was trying to avoid ‘dependent:’ as much as possible, and to rely on DB consteaints instead (‘add_foreign_key … on_delete: :cascade’.
One reason was that one of my past projects had such a threaded record structure that deleting a user could take longer than an hour.

However, your example shows that onitting ‘dependent:’ isn’t always a good idea.

Do you think it makes sense to require to specify ‘dependent:’ on every has_many/has_many through?

[zverok]

My only point was that suggesting (in the style guide and cop rule) that .all can safely be removed from every statement is very dangerous. In my case, the change was caught by non-null violation (and it still took me quite some time to understand what happened), but I might imagine somebody's code's behavior changing without them noticing.

So, at the very least:

• style guide should make a clear statement that behavior with and without .all most probably is different for associations;
• I honestly don't know what's the sane behavior for a cop. I don't know how many codebases are actually relying on model.association.delete_all (or, as ours did, on model.association.all.delete_all). If it were up to me I'd say the rule that doesn't work in 50% of its possible usages is not a rule at all 🤷

PS: On a more philosophical note, there could be a lot of discussion around CollectionProxy design (which sometimes has the same behavior as a naked scope, and sometimes has completely different with same method names, and it all "makes sense in context" until you work in a big production codebase or follow the wrong assumption), but that's just the reality we have 🤷

[pirj]

can safely be removed from every statement is very dangerous

It’s a very good point. I doubt that the guide is providing guarantees that switching from a “bad” style to a “good” style is always safe and has no side effects. Take, for example, the load Rails config defaults rule. Blindly turning the loading on can have disastrous effects, not all of which can be caught by specs or on a low-load few-data staging server.

Speaking if the cop, it should be marked as unsafe for autocorrection.

[pirj]

A practical clarification. Why wasn’t the change from ‘user.articles.all.delete_all’ to ‘user.articles.delete_all’ detected? I recon it should have been caught by a not null database constraint on ‘articles.user_id’ column?

[zverok]

I doubt that the guide is providing guarantees that switching from a “bad” style to a “good” style is always safe and has no side effects.

That's true. But I believe that at least mentioning the fact about possible change of the semantics should be considered 🤷
Though, thinking a bit further of it, it seems that in the wild only delete_all and similar methods will be affected. So that might be the point to mention in the guide/fix in cop (checking for those specific methods).

Why wasn’t the change from user.articles.all.delete_all to user.articles.delete_all detected?

It was caught by the test immediately, but I spent some time trying to understand why delete_all behavior changed 😁 (And in some codebases there might be no DB-level constraints to caught that.)

[masato-bkn]

Thank you for pointing that out.

Considering that the behavior of some methods changes when the receiver is a associations, it might be better to limit this style guide and cop to situations where the receiver is a Model🤔

When the receiver is a Model, at least for methods in ActiveRecord::Querying::QUERYING_METHODS, they are delegated to all. So, I think omitting all does not change the behavior.

[pirj]

The guideline can make a note regarding the ‘delete’/‘delete_all’ and the default ‘dependent’.
It would still tell to not use the ‘.all’, in all cases.
But it, ot another guideline/cop would could tell to use ‘destroy’/‘destroy_all’ instead.

Do you think this would promote the style that has the least surprise?

[masato-bkn]

Do you think this would promote the style that has the least surprise?

OK, I generally agree with you!

The guideline can make a note regarding the ‘delete’/‘delete_all’ and the default ‘dependent’.

At present, this cop checks for ActiveRecord::Querying::QUERYING_METHODS.
Regarding delete, it's not targeted by this cop, so I'm uncertain whether we need to mention it in the note for this style guide.

Also, for destroy_all, since its behavior changes depending on whether all is present or not, I think a note is necessary.