Add ActiveStorage/R2 backend for PublicImage alongside CarrierWave

.env

@@ -41,8 +41,10 @@ TWILIO_NUMBER=+15005550006
 ALLOWED_WRITE_ORGANIZATIONS=foo,bar
 TRANSLATION_IO_API_KEY=test
 CLOUDFLARE_TOKEN=test
-ACTIVE_STORAGE_HOST=https://uploads.bikeindex.org
-ACTIVE_STORAGE_HOST_DEV=https://dev-uploads.bikeindex.org
+# Placeholders so the S3 client can sign while replaying cassettes; real values in .env.test
+R2_TEST_ENDPOINT=https://r2-test.example.com
+R2_TEST_ACCESS_KEY=test
+R2_TEST_ACCESS_KEY_SECRET=test
 LIVE_EXTERNAL_API_SPECS=false
 STOP_HELING_BASE_URL=test
 VERLOREN_OF_GEVONDEN_BASE_URL=test

app/jobs/image_jobs/process_public_image_job.rb

@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+
+require "image_processing/vips"
+
+module ImageJobs
+  class ProcessPublicImageJob < ApplicationJob
+    sidekiq_options queue: "med_priority"
+
+    def perform(public_image_id)
+      public_image = PublicImage.unscoped.find_by(id: public_image_id)
+      return unless public_image&.file_needs_processing?
+
+      blob = public_image.file.blob
+      prepare_image(blob) unless blob.binx_data.to_h["stripped"] # A second pass would re-encode
+      # Not `preprocessed` - those generate off the un-stripped original, racing the strip
+      PublicImage::VARIANTS.each_key { |size| public_image.file.variant(size).processed }
+
+      stamp!(blob, "processed" => true) # Last, so it means every variant exists
+    end
+
+    private
+
+    # Uploads reach R2 without passing through Rails, so the original still carries the GPS
+    # coordinates of wherever the bike was photographed. Reuses blob.key to keep URLs stable;
+    # vips autorotates on load, so orientation survives losing the tag that encoded it.
+    def prepare_image(blob)
+      to_webp = PublicImage::WEBP_SOURCE_TYPES.include?(blob.content_type)
+      prepared = blob.open do |file|
+        source = ImageProcessing::Vips.source(file).saver(strip: true)
+        # n: -1 keeps every gif frame; vips reads page one otherwise. Not when converting -
+        # pages there are a tiff's scans, which shouldn't become an animation
+        to_webp ? source.convert("webp").call : source.loader(n: -1).call
+      end
+      # Before the upload, which re-identifies content_type using the filename
+      blob.filename = "#{blob.filename.base}.webp" if to_webp
+      blob.upload(prepared) # Resets checksum/byte_size, which still describe the pre-strip bytes
+      stamp!(blob, "stripped" => true)
+    ensure
+      prepared&.close!
+    end
+
+    # Not metadata - a direct upload posts that, so a client could claim to be processed
+    def stamp!(blob, values)
+      blob.update!(binx_data: blob.binx_data.to_h.merge(values))
+    end
+  end
+end

app/models/concerns/bike_attributable.rb

@@ -204,7 +204,11 @@ def image_url(size = nil)
       return stock_photo_url.present? ? stock_photo_url : nil
     end
 
-    image_col = public_images.limit(1).first&.image
+    public_image = public_images.limit(1).first
+    # PublicImage owns which backend a row is on
+    return public_image.image_url(size) if public_image&.activestorage?
+
+    image_col = public_image&.image
     # NOTE: avoid image_col.blank? — on Fog storage it issues an S3 HEAD per call (timed out the API search).
     return nil if image_col&.path.blank? && !REMOTE_IMAGE_FALLBACK_URLS
 

app/models/public_image.rb

@@ -28,13 +28,36 @@ class PublicImage < ApplicationRecord
     photo_of_receipt: 6
   }.freeze
 
-  mount_uploader :image, PublicImageUploader
+  # Sized for display rather than for the source: `large` covers the show page hero at 2x, `small`
+  # the search result cards. `small` stays jpeg because `thumb_path` feeds emails, and Outlook
+  # desktop (still the Word rendering engine) won't render webp.
+  VARIANTS = {
+    small: {resize_to_fill: [300, 300], format: :jpeg},
+    medium: {resize_to_fit: [1000, 750], format: :webp},
+    large: {resize_to_fit: [2000, 1600], format: :webp}
+  }.freeze
+
+  # No browser renders TIFF and only Safari HEIC, so the job rewrites these as webp
+  WEBP_SOURCE_TYPES = %w[image/heic image/heif image/tiff].freeze
+
+  # Direct uploads bypass the uploader, so nothing else holds them to a format we can serve.
+  # Wider than carrierwave's whitelist by HEIC - anything we convert has to be permitted
+  FILE_CONTENT_TYPES = (ApplicationUploader.extensions.map { Marcel::MimeType.for(extension: it) } +
+    WEBP_SOURCE_TYPES).uniq.freeze
+
+  mount_uploader :image, PublicImageUploader # Legacy, migrating to :file
   process_in_background :image, CarrierWaveProcessJob # Defer version generation so large uploads don't hit the 30s Rack::Timeout
 
+  has_one_attached :file do |attachable|
+    VARIANTS.each { |name, transformations| attachable.variant(name, **transformations) }
+  end
+
   enum :kind, KIND_ENUM
 
   belongs_to :imageable, polymorphic: true
 
+  # Only when a file is assigned, so a legacy carrierwave save pays no attachment query
+  validate :file_permitted, if: -> { attachment_changes["file"].present? }
   attr_writer :image_cache
   attr_accessor :skip_update
 
@@ -43,6 +66,15 @@ class PublicImage < ApplicationRecord
 
   default_scope { where(is_private: false).order(:listing_order) }
   scope :bike, -> { where(imageable_type: "Bike") }
+  # Complements, so the counts add up to the migration's progress
+  scope :activestorage, -> { where.associated(:file_attachment) }
+  scope :carrierwave, -> { where.missing(:file_attachment) }
+
+  # Checked before the blob exists on direct upload, and again by the validation after
+  def self.file_permitted?(content_type:, byte_size:)
+    FILE_CONTENT_TYPES.include?(content_type) &&
+      byte_size.to_i.between?(1, PublicImageUploader::MAX_FILE_SIZE)
+  end
 
   def default_name
     if bike?
@@ -64,6 +96,26 @@ def bike?
     imageable_type == "Bike"
   end
 
+  # A row holding both is activestorage: the attachment supersedes the carrierwave version
+  def activestorage?
+    file.attached?
+  end
+
+  def carrierwave? = !activestorage?
+
+  # Both backends name their sizes the same, so callers pass one either way - the
+  # activestorage dimensions are just larger
+  def image_url(size = nil)
+    return image.url(*size) unless activestorage?
+
+    BlobUrl.for_variant(file, size&.to_sym&.presence_in(VARIANTS.keys))
+  end
+
+  # "processed" lands only once the variants exist, so a job that died partway is picked up again
+  def file_needs_processing?
+    activestorage? && !file.blob.binx_data.to_h["processed"]
+  end
+
   # Method to make create_revised.js easier to handle
   def bike_type
     return false unless %w[Bike BikeVersion].include?(imageable_type)
@@ -78,6 +130,8 @@ def enqueue_after_commit_jobs
       return ImageJobs::ExternalUrlStoreJob.perform_async(id)
     end
 
+    ImageJobs::ProcessPublicImageJob.perform_async(id) if file_needs_processing?
+
     imageable&.update(updated_at: Time.current)
     return true unless bike?
 
@@ -95,15 +149,18 @@ def process_image_upload
   end
 
   # Because the way we load the file is different if it's remote or local
-  # This is hacky, but whatever
+  # This is hacky, but whatever. Only asked of carrierwave images - activestorage downloads
+  # the same way whichever service it's on.
   def local_file?
     image&._storage&.to_s == "CarrierWave::Storage::File"
   end
 
   # Always a file on disk - URI.open returns a StringIO for remote files under 10kb,
   # which image processors can't read.
-  # Returns nil when a local file is missing on disk (e.g. sandbox without synced uploads)
+  # Returns nil when the file is missing (e.g. sandbox without synced uploads)
   def open_file
+    return attached_tempfile if activestorage?
+
     if local_file?
       File.open(image.path, "r") if File.exist?(image.path)
     else
@@ -113,6 +170,26 @@ def open_file
 
   private
 
+  # A direct upload declares both, but attaching re-identifies content_type from the stored bytes
+  # and byte_size is signed into the presigned PUT - so S3 rejects a body of any other length
+  def file_permitted
+    blob = attachment_changes["file"].blob
+    return if self.class.file_permitted?(content_type: blob.content_type, byte_size: blob.byte_size)
+
+    errors.add(:file, :invalid)
+  end
+
+  # Not blob.open: it unlinks on block exit, and the caller needs the file to outlive this.
+  # Plain download is one GET; the chunked form adds two HEADs on S3
+  def attached_tempfile
+    tempfile = Tempfile.new(["public_image", File.extname(file.filename.to_s)], binmode: true)
+    tempfile.write(file.blob.download)
+    tempfile.tap(&:rewind)
+  rescue ActiveStorage::FileNotFoundError
+    tempfile&.close!
+    nil
+  end
+
   def remote_storage?
     PublicImageUploader.storage == CarrierWave::Storage::Fog
   end

app/services/blob_url.rb

@@ -7,7 +7,11 @@ module BlobUrl
   SERVICE = Bikeindex::Application.config.active_storage.service
   LOCAL_STORAGE = %i[local test].include?(SERVICE)
   STORAGE_HOST = ENV.fetch("ACTIVE_STORAGE_HOST", "https://uploads.bikeindex.org")
-  STORAGE_HOST_DEV = ENV.fetch("ACTIVE_STORAGE_HOST_DEV", nil)
+  # Each non-production bucket has its own domain; an unlisted service serves from production's
+  STORAGE_HOSTS = {
+    cloudflare_dev: ENV.fetch("ACTIVE_STORAGE_HOST_DEV", "https://dev-uploads.bikeindex.org"),
+    cloudflare_test: ENV.fetch("ACTIVE_STORAGE_HOST_TEST", "https://test-uploads.bikeindex.org")
+  }.freeze
 
   def for(blob = nil)
     return if blob.blank?
@@ -19,6 +23,20 @@ def for(blob = nil)
     end
   end
 
+  # `size` is a named variant. Never calls `processed` - that would be a storage existence
+  # check per image per render, and the post-attach job guarantees they exist
+  def for_variant(attached = nil, size = nil)
+    return if attached.blank?
+    return self.for(attached.blob) if size.blank?
+
+    variant = attached.variant(size)
+    if local_storage?(attached.blob)
+      Rails.application.routes.url_helpers.rails_representation_url(variant)
+    else
+      File.join(storage_host_for(attached.blob), variant.key) # Deterministic, no query
+    end
+  end
+
   #
   # private below here
   #
@@ -28,9 +46,7 @@ def local_storage?(blob)
   end
 
   def storage_host_for(blob)
-    return STORAGE_HOST if STORAGE_HOST_DEV.blank? || blob.service&.name != :cloudflare_dev
-
-    STORAGE_HOST_DEV
+    STORAGE_HOSTS[blob.service&.name] || STORAGE_HOST
   end
 
   conceal :local_storage?, :storage_host_for

app/services/image_services/stolen_processor.rb

@@ -48,6 +48,10 @@ def update_alert_images(stolen_record, force_regenerate: false, public_image_id:
       end
       stolen_record.bike&.update(updated_at: Time.current)
       stolen_record
+    ensure
+      # Tempfiles need close! or they sit in /tmp until GC; a local carrierwave File is the
+      # stored image itself, so only close it
+      image.respond_to?(:close!) ? image.close! : image&.close
     end
 
     #

app/views/bikes/_main_show_block.html.haml

@@ -14,7 +14,7 @@
             - @bike.public_images.select(&:image_url).each_with_index do |public_image, index|
               - thumb_class = index == 0 ? 'current-thumb' : '' # make the first image current
               %li
-                %a.clickable-image{ class: thumb_class, data: { id: "image#{public_image.id}", img: public_image.image_url(:large), link: public_image.image.url } }
+                %a.clickable-image{ class: thumb_class, data: { id: "image#{public_image.id}", img: public_image.image_url(:large), link: public_image.image_url } }
                   = image_tag public_image.image_url(:small), alt: "#{public_image.name}", id: "i|#{public_image.listing_order}", data: { controller: "image-fallback", action: "error->image-fallback#useOriginal", "image-fallback-url-value": public_image.image_url }
         :plain
           <script id="current-photo-template" type="x-tmpl-mustache">

config/application.rb

@@ -38,6 +38,10 @@ class Application < Rails::Application
     config.active_job.queue_adapter = :sidekiq
     config.active_job.default_queue_name = :low_priority
 
+    # Overrides load_defaults. Untracked variant keys are a digest of the blob key, so BlobUrl
+    # builds them without a query - at the cost of only being able to count them from the bucket.
+    config.active_storage.track_variants = false
+
     # Use our custom error pages
     config.exceptions_app = routes
 

config/initializers/content_security_policy.rb

@@ -8,8 +8,12 @@
   config.content_security_policy do |policy|
     policy.default_src :self
     policy.font_src :self, "https://fonts.gstatic.com", "http://fonts.gstatic.com", "https://themes.googleusercontent.com", :data
+    # Blobs serve from the bucket's own domain, one per environment, and an unlisted host renders
+    # nothing. Duplicates BlobUrl because this runs before autoloading; blob_url_spec catches drift
     policy.img_src :self, "https://files.bikeindex.org",
-      "https://uploads.bikeindex.org",
+      ENV.fetch("ACTIVE_STORAGE_HOST", "https://uploads.bikeindex.org"),
+      ENV.fetch("ACTIVE_STORAGE_HOST_DEV", "https://dev-uploads.bikeindex.org"),
+      ENV.fetch("ACTIVE_STORAGE_HOST_TEST", "https://test-uploads.bikeindex.org"),
       "https://maps.bikeindex.org",
       "https://bikebook.s3.amazonaws.com",
       "https://www.googletagmanager.com",

config/storage.yml

@@ -8,6 +8,17 @@ local:
   root: <%= Rails.root.join("storage") %>
   public: true
 
+# Only for the specs that upload for real (image_jobs/process_public_image_job_spec); the rest of the
+# suite stays on :test. Its own bucket so CI churn doesn't land where review apps serve from.
+cloudflare_test:
+  service: S3
+  endpoint: <%= ENV["R2_TEST_ENDPOINT"] %>
+  access_key_id: <%= ENV["R2_TEST_ACCESS_KEY"] %>
+  secret_access_key: <%= ENV["R2_TEST_ACCESS_KEY_SECRET"] %>
+  bucket: bikeindex-test
+  region: auto
+  public: true
+
 cloudflare_dev:
   service: S3
   endpoint: <%= ENV["R2_DEV_ENDPOINT"] %>

spec/factories/public_images.rb

@@ -4,10 +4,11 @@
 
     transient do
       filename { nil }
+      image_path { "spec/fixtures/bike_photo-landscape.jpeg" }
     end
 
     after(:build) do |public_image, evaluator|
-      next if public_image.image.present?
+      next if public_image.image.present? || public_image.file.attached?
 
       model_type = public_image.imageable_type.underscore
       model_id = public_image.imageable.id
@@ -21,10 +22,12 @@
     end
 
     trait :with_image_file do
-      transient do
-        image_path { "spec/fixtures/bike_photo-landscape.jpeg" }
-      end
       image { File.open(Rails.root.join(image_path)) }
     end
+
+    # ActiveStorage rather than CarrierWave
+    trait :with_attached_file do
+      file { {io: File.open(Rails.root.join(image_path)), filename: File.basename(image_path)} }
+    end
   end
 end