Ensure that Carrierwave uploader versions are tracked in uploads table and uploaded to object storage
The custom favicon feature introduced in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/14497 does not currently work when upload object storage is configured (like on GitLab.com), because the existing logic responsible for recording uploads in the DB and uploading them to object storage is not prepared to handle uploaders with multiple versions like the new FaviconUploader
with its resized and converted favicon_main
version.
After uploading a header logo, and a favicon, Upload.where(model: Appearance.last)
looks like this:
[
#<Upload id: 1416267, size: 15753, path: "appearance/header_logo/1/1mq7Ekw8_400x400.jpg", checksum: nil, model_id: 1, model_type: "Appearance", uploader: "AttachmentUploader", created_at: "2018-06-11 08:35:16", store: 2, mount_point: "header_logo", secret: nil>,
#<Upload id: 1416265, size: 92135, path: "appearance/favicon/1/1mq7Ekw8_400x400.png", checksum: nil, model_id: 1, model_type: "Appearance", uploader: "FaviconUploader", created_at: "2018-06-11 08:33:32", store: 2, mount_point: "favicon", secret: nil>,
#<Upload id: 1416264, size: 2624, path: "uploads/-/system/appearance/favicon/1/1mq7Ekw8_400...", checksum: "e487d6b69c736aa5ba5394243dddf6d63acc5050ebed9fdfdb...", model_id: 1, model_type: "Appearance", uploader: "FaviconUploader::Uploader70107565775020", created_at: "2018-06-11 08:33:32", store: 1, mount_point: "favicon", secret: nil>,
#<Upload id: 1416262, size: 2624, path: "uploads/-/system/appearance/favicon/1/1mq7Ekw8_400...", checksum: "e487d6b69c736aa5ba5394243dddf6d63acc5050ebed9fdfdb...", model_id: 1, model_type: "Appearance", uploader: "FaviconUploader::Uploader70109789441760", created_at: "2018-06-11 08:33:29", store: 1, mount_point: "favicon", secret: nil>
]
You can see that the main header logo and favicon uploads both have a proper uploader
, have store
set to 2
(ObjectStorage::Store::REMOTE
), and an path
relative to the object storage root.
The last two uploads have an uploader
value that cannot actually be constantized, because it refers to the dynamic version-specific uploader for appearance.favicon.favicon_main
. They have store
set to 1
(ObjectStorage::Store::LOCAL
), and a path
relative to app/public/
. We have two, with different values for uploader
, presumably because RecordsUploads::Concern#record_upload
was called twice, and the second run couldn't find the earlier upload
because of the mismatching uploader
. The uploads were never uploaded to file storage, but appearance.favicon.favicon_main.file_storage?
still returns false
, presumably because the main favicon uploader was uploaded.
For %11.0, I think the quickest workaround is to remove the favicon_main
version and make it very clear to the user that they are expected to upload an appropriately sized favicon, because we will not do any automatic resizing. In a subsequent release, we should teach RecordsUploads
and ObjectStorage
about different uploader versions, and restore the favicon_main
option.
@koffeinfrei What do you think? I'm happy to remove the favicon_main
version from %11.0 myself; would you be able look into the work required to restore it in %11.1?
cc @marin