Add ActiveStorage/R2 backend for PublicImage alongside CarrierWave - #3969
Draft
sethherr wants to merge 14 commits into
Draft
Add ActiveStorage/R2 backend for PublicImage alongside CarrierWave#3969sethherr wants to merge 14 commits into
sethherr wants to merge 14 commits into
Conversation
PublicImage gains a has_one_attached :file with small/medium/large variants, resized for the current layouts rather than 2013's. image_url dispatches on which backend a record uses, so call sites are unchanged. track_variants is off: untracked variant keys are a digest of the blob key plus the transformation, so BlobUrl builds them without a query. Images::ProcessPublicImageJob strips EXIF from the original in place - direct uploads never reach Rails, so GPS would otherwise be published with the photo. Co-Authored-By: Claude <noreply@anthropic.com>
open_file short-circuits to attached_tempfile before consulting it, and activestorage downloads identically on Disk and S3 - the local-vs-remote question is carrierwave's alone. Co-Authored-By: Claude <noreply@anthropic.com>
Blob#delete already delete_prefixed's "variants/#{key}/", so purging cleans up
untracked variants - the comment claimed they'd need sweeping. What's actually
lost is enumerating them from the database.
image_url tested size.to_sym for membership but forwarded the raw value, so a
string passed the guard and then missed the symbol-keyed variant lookup.
Co-Authored-By: Claude <noreply@anthropic.com>
Attaching enqueues ActiveStorage's AnalyzeJob against the same blob the strip job rewrites, and Blob#analyze merges into metadata from whatever it read when it started - so whichever commits second drops the other's keys, losing "stripped" (forcing a pointless re-strip) or the dimensions. Marking the blob analyzed in memory is enough to stop the attachment's after_commit from enqueueing it; the job then analyzes off the stripped bytes. Nothing is persisted claiming analysis happened until the job does it. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Records an upload of a GPS-tagged iPhone photo against the bikeindex-dev R2 bucket, then asserts the stored original has no EXIF at all and that no identifying tag survives into any variant. vips copies EXIF through a transform - a variant built from an un-stripped original carries all 15 GPS fields, jpeg and webp alike - so the variants are only clean because the original is rewritten before any of them exist. It does re-synthesize orientation/resolution/colorspace on save, hence matching on identifying tags rather than expecting no metadata. Two fixes to make the cassette safe to commit: the filter list named R2_DEV_ACCESS_KEY twice and never the secret, and aws-sdk addresses R2 virtual-host style, so filtering R2_DEV_ENDPOINT matched nothing and left the account id in the URLs. Filtering the host instead also lets the cassette replay against the .env placeholder, which CI needs - the S3 client raises before WebMock can intercept when no credentials are set. Co-Authored-By: Claude <noreply@anthropic.com>
"stripped" was doing two jobs: guarding the re-encode and standing in for "this image is ready". It's written halfway through, so anything reading it as a completion signal sees a blob whose variants don't exist yet. Split it. "stripped" still lands right after the rewrite, because that's what it has to guard - preparing twice would re-encode the original - and a new "processed" is written last, once every variant is generated. file_needs_processing? reads that one, so a job that died partway through gets picked up again: the retry skips the strip, fills in the missing variants and only then marks it ready. prepare_image (was strip_metadata) now also rewrites HEIC as webp in the same decode/encode pass. Safari is the only browser that renders HEIC and the original is served directly, so an iPhone upload was otherwise undisplayable. The blob key is untouched, so variant keys and CDN urls still don't move; the filename is set before the upload, which re-identifies content_type using it. Co-Authored-By: Claude <noreply@anthropic.com>
TIFF is in the uploader's allowlist and no browser renders it, so it hit the same problem as HEIC: the original is served directly, undisplayable. Every format the allowlist accepts is now web-servable once the job has run. Co-Authored-By: Claude <noreply@anthropic.com>
The trait declared image/jpeg for every image_path, which read as wrong next to a .tif or .heic fixture. It was harmless - ActiveStorage re-identifies from the bytes on upload, which is why the tiff spec saw image/tiff regardless - but it would mislead the next person to add a fixture. Co-Authored-By: Claude <noreply@anthropic.com>
Running the spec/services suite re-recorded them under record: :new_episodes, changing only timestamps and Heroku NEL tokens, and `git add -A` picked them up. Reverted to their prior content - no behaviour change either way. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Groundwork for moving public images off CarrierWave onto ActiveStorage + R2.
PublicImagegains ahas_one_attached :filethat coexists with the legacyimagecolumn —image_urldispatches on whichever backend a record uses, so the ~40 existing call sites are untouched and there's no cutover to coordinate. Nothing writes to the new backend yet; that arrives with the direct-upload endpoint.large2000x1600 covers the show-page hero at 2x (it renders ~885 CSS px),medium1000x750,smallstays 300x300.smallis the one that stays jpeg — it feedsthumb_path, and Outlook desktop is still on the Word engine and won't render webp.track_variantsis off, againstload_defaults. Untracked variant keys are a digest of the blob key plus the transformation, soBlobUrl.for_variantbuilds them with no query and noprocessedexistence check — tracking would putactive_storage_variant_recordsplus a nested attachment and blob behind every image URL. The tradeoff is that purging a blob orphans its variants, so they need sweeping alongside unattached blobs.Images::ProcessPublicImageJobstrips EXIF from the original in place, then analyzes it and generates the variants. Direct uploads go browser to R2 without passing through Rails, so the original would otherwise keep the GPS coordinates of wherever the bike was photographed — and we serve originals publicly. Rewriting under the same blob key keeps variant keys and CDN URLs stable.PublicImagemarks the blob analyzed before enqueueing, which stops ActiveStorage from queueing its ownAnalyzeJob. Both writeblob.metadataby merging into whatever they read at start, so left concurrent, whichever commits second drops the other's keys — losingstrippedand forcing a re-strip, or losing the dimensions.open_filehandles both backends, so stolen-alert generation works either way.update_alert_imagesnow closes the tempfile it was leaking until GC.