Skip to content

Add rubocop for AR exists? and present?/blank?

Qingyu Zhao requested to merge add-rubocop-ar-exists-any-none-present-empty into master

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 in SELECT "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
  • 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

/cc @gitlab-org/maintainers/rails-backend

Edited by 🤖 GitLab Bot 🤖

Merge request reports

Loading