diff options
author | Sean McGivern <sean@gitlab.com> | 2018-04-19 19:22:23 +0300 |
---|---|---|
committer | Sean McGivern <sean@gitlab.com> | 2018-06-05 16:57:19 +0300 |
commit | 6ecf819f73ae28547e1b816780de0d85c3a653cf (patch) | |
tree | f81c50bb4e8a1984530e130e61871173fc73d4b9 /app/models/concerns/avatarable.rb | |
parent | e11a1001dcdc1ea5c65845fb0897b861b5c0b92d (diff) |
Fix an N+1 in avatar URLs
This is tricky: the query was being run in
`ObjectStorage::Extension::RecordsUploads#retrieve_from_store!`, but we can't
just add batch loading there, because the `#upload=` method there would use the
result immediately, making the batch only have one item.
Instead, we can pre-emptively add an item to the batch whenever an avatarable
object is initialized, and then reuse that batch item in
`#retrieve_from_store!`. However, this also has problems:
1. There is a lot of logic in `Avatarable#retrieve_upload_from_batch`.
2. Some of that logic constructs a 'fake' model for the batch key. This should
be fine, because of ActiveRecord's override of `#==`, but it relies on that
staying the same.
Diffstat (limited to 'app/models/concerns/avatarable.rb')
-rw-r--r-- | app/models/concerns/avatarable.rb | 47 |
1 files changed, 47 insertions, 0 deletions
diff --git a/app/models/concerns/avatarable.rb b/app/models/concerns/avatarable.rb index 13246a774e3..095897b08e3 100644 --- a/app/models/concerns/avatarable.rb +++ b/app/models/concerns/avatarable.rb @@ -4,11 +4,14 @@ module Avatarable included do prepend ShadowMethods include ObjectStorage::BackgroundMove + include Gitlab::Utils::StrongMemoize validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? } validates :avatar, file_size: { maximum: 200.kilobytes.to_i } mount_uploader :avatar, AvatarUploader + + after_initialize :add_avatar_to_batch end module ShadowMethods @@ -18,6 +21,17 @@ module Avatarable avatar_path(only_path: args.fetch(:only_path, true)) || super end + + def retrieve_upload(identifier, paths) + upload = retrieve_upload_from_batch(identifier) + + # This fallback is needed when deleting an upload, because we may have + # already been removed from the DB. We have to check an explicit `#nil?` + # because it's a BatchLoader instance. + upload = super if upload.nil? + + upload + end end def avatar_type @@ -52,4 +66,37 @@ module Avatarable url_base + avatar.local_url end + + # Path that is persisted in the tracking Upload model. Used to fetch the + # upload from the model. + def upload_paths(identifier) + avatar_mounter.blank_uploader.store_dirs.map { |store, path| File.join(path, identifier) } + end + + private + + def retrieve_upload_from_batch(identifier) + BatchLoader.for(identifier: identifier, model: self).batch(key: self.class) do |upload_params, loader, args| + model_class = args[:key] + paths = upload_params.flat_map do |params| + params[:model].upload_paths(params[:identifier]) + end + + Upload.where(uploader: AvatarUploader, path: paths).find_each do |upload| + model = model_class.instantiate('id' => upload.model_id) + + loader.call({ model: model, identifier: File.basename(upload.path) }, upload) + end + end + end + + def add_avatar_to_batch + return unless avatar_mounter + + avatar_mounter.read_identifiers.each { |identifier| retrieve_upload_from_batch(identifier) } + end + + def avatar_mounter + strong_memoize(:avatar_mounter) { _mounter(:avatar) } + end end |