Add rubocop for AR exists? and present?/blank?
Description of the proposal
We have updated this Cop, limit the scope to check the .exists
against .present?/.blank?
in the same view file:
- contains an active record relation @AR_relation.exists?
- it also contains @AR_relation.present? or @AR_relation.blank?
In this case, it will most likely result in redundant database query:
-
@AR_relation.exists?
will always trigger a database query - if
@AR_relation
has not loaded somewhere else, it will need another database query for @AR_relation.present?/blank?
Ideally, we just want to ban the usage of @AR_relation.present?
and @AR_relation.blank?
. But in Rails, Array/Hash has the method .present?
and .blank?
, it will be false positive for Array/Hash which is too noisy. So we check .exists?
first, if a variable calls .exists?
method, it is most likely an Active Record relation.
BTW, more background history:
The original proposal was to check the co-appearance of
.exists?
.any?/.none?/.present?/.empty?
With the discussion thread !31095 (comment 338793202), we realized:
-
.exists?/.any?/none?/empty?
will trigger the same DB query, thus they won't trigger DB query due to Rails SQL query cache. -
.present/.blank?
will trigger different query than.exists?
, thus Rails SQL query cache won't help them. Especially.present?/.blank?
will result inSELECT "users".* FROM "users"
query. This could be unnecessarily expensive !31095 (comment 338858755).
The benefit of allowing co-usage of .any?/.none?/.empty?/.exists?
in the same HAML file, AFAIU, is to increase the readability of the code.
Check-list
-
Make sure this MR enables a static analysis check rule for new usage but ignores current offenses (current offenses are fixed together in this MR) -
Mention this proposal in the relevant Slack channels (e.g. #development
,#backend
,#frontend
) - [-] If there is a choice to make between two potential styles, set up an emoji vote in the MR:
- CHOICE_A:
🅰 - CHOICE_B:
🅱 - Vote yourself for both choices so that people know these are the choices
- CHOICE_A:
-
The MR doesn't have significant objections, and is getting a majority of 👍 vs👎 (remember that we don't need to reach a consensus) -
(If applicable) One style is getting a majority of vote (compared to the other choice) -
(If applicable) Update the MR with the chosen style - [-] Create a follow-up issue to fix the current offenses as a separate iteration: ISSUE_LINK
-
Follow the review process as usual -
Once approved and merged by a maintainer, mention it again: -
In the relevant Slack channels (e.g. #development
,#backend
,#frontend
) -
(Optional depending on the impact of the change) In the Engineering Week in Review
-