Open zverok opened 10 months ago
cc @masato-bkn
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?
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?
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:
.all
most probably is different for associations;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 :shrug: 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 :shrug:
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.
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?
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 :shrug:
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
touser.articles.delete_all
detected?
It was caught by the test immediately, but I spent some time trying to understand why delete_all
behavior changed :grin: (And in some codebases there might be no DB-level constraints to caught that.)
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.
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?
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.
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:
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;user.articles.delete_all
is ActiveRecord::Associations::CollectionProxy#delete_all which behaves "according to a deletion strategy", which, say, forhas_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 :heart_eyes:
So... Probably both the style guide and the cop should be much more cautious here.