Skip to content

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

Edited by Douwe Maan