diff options
author | Alejandro RodrÃguez <alejorro70@gmail.com> | 2016-06-06 16:19:39 -0400 |
---|---|---|
committer | Alejandro RodrÃguez <alejorro70@gmail.com> | 2016-06-08 10:26:05 -0400 |
commit | fade1a4cdebb4124048e9764486812627333ff95 (patch) | |
tree | b8911fb45861a1019f3724b74fffeca765fcc2d0 /app/models/concerns | |
parent | ad83c3085513dd248b979d445e545e88a17c6ebc (diff) | |
download | gitlab-ce-fade1a4cdebb4124048e9764486812627333ff95.tar.gz |
Fix pseudo n+1 queries with Note and Note Authors in issuables APIs
This was not a clear cut n+1 query, given that if you're directly subscribed to all issues
that the API is returning you never really need to check for the notes. However, if you're
subscribed to _all_ of them, then for each issuable you need to go once to `notes`, and
once to `users` (for the authors). By preemtively loading notes and authors, at worst you have
1 extra query, and at best you saved 2n extra queries. We also took advantage of this preloading
of notes when counting user notes.
Diffstat (limited to 'app/models/concerns')
-rw-r--r-- | app/models/concerns/issuable.rb | 24 |
1 files changed, 21 insertions, 3 deletions
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 58e7557fdc6..0ccd3474b81 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -17,7 +17,12 @@ module Issuable belongs_to :assignee, class_name: "User" belongs_to :updated_by, class_name: "User" belongs_to :milestone - has_many :notes, as: :noteable, dependent: :destroy + has_many :notes, as: :noteable, dependent: :destroy do + def authors_loaded? + # We check first if we're loaded to not load unnecesarily. + loaded? && to_a.all? { |note| note.association(:author).loaded? } + end + end has_many :label_links, as: :target, dependent: :destroy has_many :labels, through: :label_links has_many :todos, as: :target, dependent: :destroy @@ -44,6 +49,7 @@ module Issuable scope :without_label, -> { joins("LEFT OUTER JOIN label_links ON label_links.target_type = '#{name}' AND label_links.target_id = #{table_name}.id").where(label_links: { id: nil }) } scope :join_project, -> { joins(:project) } + scope :inc_notes_with_associations, -> { includes(notes: :author) } scope :references_project, -> { references(:project) } scope :non_archived, -> { join_project.where(projects: { archived: false }) } @@ -179,7 +185,13 @@ module Issuable end def user_notes_count - notes.user.count + if notes.loaded? + # Use the in-memory association to select and count to avoid hitting the db + notes.to_a.count { |note| !note.system? } + else + # do the count query + notes.user.count + end end def subscribed_without_subscriptions?(user) @@ -239,7 +251,13 @@ module Issuable end def notes_with_associations - notes.includes(:author) + # If A has_many Bs, and B has_many Cs, and you do + # `A.includes(b: :c).each { |a| a.b.includes(:c) }`, sadly ActiveRecord + # will do the inclusion again. So, we check if all notes in the relation + # already have their authors loaded (possibly because the scope + # `inc_notes_with_associations` was used) and skip the inclusion if that's + # the case. + notes.authors_loaded? ? notes : notes.includes(:author) end def updated_tasks |