Fix#51284
Both Proxy controllers in Active Storage set the caching headers early
before streaming.
In some instances (see #51284), it is possible for the file
download (from the service to server) to fail before we send the first
byte to the client (response not yet committed).
In those instances, this change would invalidate the cache and return
a better response status before closing the stream.
It looks like `preview_image_needed_before_processing_variants?` was added recently,
but it's stated as being public API.
However, it looks like it's more of an implementation detail that's not meant for Active Storage users.
So this marks it as nodoc, so we're not on the hook for maintaining it.
This follows up on https://github.com/rails/rails/pull/51030, that introduces a
behaviour change for previewable attachments that don't specify any variants at all
or no variants that require pre-processing via `TransformJob`.
Before, when no variant required pre-processing, Attachment#transform_variants_later
didn't do anything. Now, a `ActiveStorage::PreviewImageJob` is enqueued with
the attachment's blob and a empty array.
This happened to work with Marcel 1.0.2 and earlier since magic byte
sniffing sees that Illustrator files are PDFs internally, causing these
files to be treated as `application/pdf` despite having a declared
content type of `application/illustrator` and an `.ai` file extension.
Marcel 1.0.3 corrected this to the more specific `application/illustrator`
subtype of `application/pdf`, but the MuPDF previewer only accepts the
parent `application/pdf` type.
Changing it to accept PDF and any child types allows the previewer to
explicitly work with Illustrator files again, which was only a happy
accident previously.
Update tests to clarify content type detection heuristic after exposing
a regression in Marcel 1.0.3 that wasn't caught by its test suite:
1. magic bytes
2. declared content type, unless it's binary
3. filename extension
4. binary: application/octet-stream
Replaced by `#lease_connection` to better reflect what it does.
`ActiveRecord::Base#connection` is deprecated in the same way
but without a removal timeline nor a deprecation warning.
Inside the Active Record test suite, we do remove `Base.connection`
to ensure it's not used internally.
Some callsites have been converted to use `with_connection`,
some other have been more simply migrated to `lease_connection`
and will serve as a list of callsites to convert for
https://github.com/rails/rails/pull/50793
Extracted from: https://github.com/rails/rails/pull/50793
Similar to the recent refactoring of schema caches, rather than to directly
hold a connection, they now hold a pool and checkout a connection when needed.
Managed to reproduce Rails Nightly CI failure
at https://buildkite.com/rails/rails-nightly/builds/149#018d9052-1b2d-48fa-9d74-a39df3f3f1d6/1251-1291
This commit allows both 33 and 34 as its height because this issue is isolated
that thedifference comes from libvips and/or ruby-vips behavior differences, not Active Storage.
* Steps to reprodude
Run this test on Ubuntu 22.04. It should not reproduce on Ubuntu 23.10.
```
git clone https://github.com/rails/rails
cd rails
rm Gemfile.lock
cd activestorage
bin/test test/models/variant_test.rb -n test_resized_variation_of_WEBP_blob
```
* Expected behavior
It should pass.
* Actual behavior
It fails because the height of the thumbnail is 34.
```
$ bin/test test/models/variant_test.rb -n test_resized_variation_of_WEBP_blob
F
Failure:
ActiveStorage::VariantTest#test_resized_variation_of_WEBP_blob [test/models/variant_test.rb:125]:
Expected: 33
Actual: 34
bin/test test/models/variant_test.rb:117
```
Refer to https://github.com/libvips/ruby-vips/issues/383
This fixes race condition in Active Storage when multiple preprocessed variants are defined for a `Previewable` file is attached.
When a variant is specified for a "previewable" file type (e.g. video or PDF) attachment, a `preview_image` attachment is first created and attached on the original blob and then any user-specified variants are derived from _that_ preview image. When those variants are named and have `preprocessed: true`, the jobs to create those variants are queued simultaneously.
Example from my case:
```ruby
has_one_attached :file, dependent: :purge_later do |attachable|
attachable.variant :preview, resize_to_fill: [400, 400], preprocessed: true
attachable.variant :still, format: "jpg", saver: {quality: 85}, preprocessed: true
end
```
When a `Previewable` attachment is created (a video, in my case), `TransformJob.perform_later` is called for each named variant with `preprocessed: true`. Unless your queue adapter is synchronous (e.g. :inline or :test), this results in a race condition in which every such variant's worker will check `processed?`, see that no `preview_image` attachment exists yet on the `ActiveStorage::Blob`, and:
1. Redundantly download the file from storage
2. Create duplicative ActiveStorage::Attachment and `ActiveStorage::Blob` records for the `preview_image` attachment (all but one of which will be orphaned from the original blob's `has_one_attached :preview_image`)
3. Create variant blobs (and associated `ActiveStorage::VariantRecord`) that are similarly orphaned (by virtue of being a variant of an orphaned `preview_image` blob)
As a result, if the video is ever purged, `PurgeJob` will only find the current `has_one_attached :preview_image` and whatever variant demanded it into existence, then leave the rest as orphaned records in the database and in storage.
Pretty simple: wrap the first step of the job in `blob.with_lock {}`. By pessimistically locking on the blob, we can prevent processing the preview image multiple times by multiple `TransformJob` jobs running concurrently.
Alternate approaches would all be more work:
* Queuing a `PreviewImage` job instead of N `TransformJob` and have it, only after `preview_image` is attached, enqueue those `TransformJob` jobs
* Batching up all the named variant transformations into a single meta-job
Writing a test for this inside Rails would be difficult because it would require running the resulting TransformJob jobs concurrently. I [started a test](https://github.com/searls/rails/blob/fix-video-duplicate-preview-variants/activestorage/test/models/variant_with_record_test.rb#L348-L367) but failed to reproduce, in part because the test queue adapter will perform enqueued jobs inline instead of concurrently. In order to write a test that replicated the issue appropriately, we might first need a new option for `perform_enqueued_jobs(async: true) { … }`
If you're interested, [this gist](https://gist.github.com/searls/5b8298abe88b3206f670ea3c6d574aab) includes a driver script and output before and after the patch showing it working.
I only found this because I'm a total cheapskate and was literally counting records in my S3 bucket to ensure `PurgeJob` worked. Then I wasted the next two days trying to figure out why before landing on this. I strongly suspect that ActiveStorage users who host video and take advantage of `preprocessed: true` named variants will have a lot of orphaned stuff floating around their buckets.
To see if you have any such "zombie" preview_images (and presumably, associated variants) floating around your application that would survive calls to `purge` on the owning attachment, you could write a query like this:
```
ActiveStorage::Attachment
.joins("INNER JOIN active_storage_attachments as other_attachments ON
active_storage_attachments.record_id = other_attachments.record_id AND
active_storage_attachments.id != other_attachments.id")
.where(
:name => "preview_image",
:record_type => "ActiveStorage::Blob",
"other_attachments.name" => "preview_image",
"other_attachments.record_type" => "ActiveStorage::Blob"
)
.distinct
```
Clearing out one's production database and backend storage to get this all right-sized should be a fun exercise for the reader.
Co-authored-by: Aaron Patterson <aaron.patterson@gmail.com>
ActiveStorage::Filename was missing quotes when encoded,
generating invalid json like this -
```
JSON.generate(foo: ActiveStorage::Filename.new("bar.pdf") # => '{"foo":bar.pdf}'
```
Delete to_json and rely on the implementation from ActiveSupport::ToJsonWithActiveSupportEncoder
This change is an up-port of #50787 which applied a similar fix to 7-1-stable.
Since these tests weren't running before, we didn't notice when the DirectUploads controller tests were broken. My theory is that it has something to do with changing `response.parsed_body` to return a HWIA (in #49003).
This change is different from the 7-1-stable PR in that it removes the need to stringify or symbolize any keys, since we are comparing the metadata with string keys. This is a follow up to #43705.
Co-authored-by: Sean Doyle <seanpdoyle@users.noreply.github.com>
Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com>
The VideoAnalyzer uses ffprobe (part of ffmpeg) to determine the
rotation of a video. The VideoAnalyzer#angle returns tags["rotate"] if
it is available, else it attempts to get "rotation" from the Display
Matrix side_data.
However, ffprobe exhibits different behavior with different versions:
ffprobe version 4.4.2-0ubuntu on Ubuntu 22.04 returns a "rotate" : "90"
tag.
In contrast, ffprobe version 6.0-6ubuntu1 on Ubuntu 23.10 does not
return the "rotate" tag; instead, it determines the angle from the
display matrix side_data, which returns "rotation": -90.
To account for this difference, we now check for both values in the test.
Fixes https://github.com/rails/rails/issues/50853
The video analyzer was relying on the positional reference of the Display
Matrix side_data to fetch the rotation value. However, the side_data is
not guaranteed to be in the same position. For instance, HDR videos shot
on iOS have "DOVI configuration record" side_data in the first position,
followed by the "Display Matrix" side data containing the rotation value.
This fix removes the positional reference and explicitely searches for
the "Display Matrix" side_data to retrieve the rotation angle.
- This is a fix needed to unblock
https://github.com/rails/rails/pull/50284,
because Active Storage relies on a Active Record bug.
The problem is best understood with a small snippet:
```
blob = ActiveStorage::Blob.new
ActiveStorage::Attachment.new(blob: blob)
ActiveStorage::Attachment.new(blob: blob)
# Currently:
p blob.attachments #=> #<ActiveRecord::Associations::CollectionProxy []>
# Once the Active Record bug is fixed:
p blob.attachments #=> #<ActiveRecord::Associations::CollectionProxy [#<ActiveStorage::Attachment id: nil, name: nil, record_type: nil, record_id: nil, blob_id: nil, created_at: nil>, #<ActiveStorage::Attachment id: nil, name: nil, record_type: nil, record_id: nil, blob_id: nil, created_at: nil>]>
# Trying to save the blob would result in trying to create 2 attachments which
# fails because of unique constrainsts.
```
### Code path
The real code path that does what the snippet above does is located here:
9c3ffab47c/activestorage/lib/active_storage/attached/many.rb (L52)
It's basically doing this:
```
user.images.attach "photo1.png"
# Initialize a Blob record and an Attachment
user.images.attach "photo2.png"
# Get the Blob from above, create another Attachment
# Initialize a new Blob record and an new Attachment
# rinse and repeat every time `attach` is called
```
Basically each time we call `attach`, we grab the previous blobs that were attached
(and that already have an Attachment record), and
### Solution
- Explicitly set the `inverse_of`, so that it behaves as if #50284 is shipped
- Don't build a new attachment for blob already having one.
### Tests
I didn't add tests, the test suite is already well covered, adding the `inverse_of`
without any changes breaks the test suite already. You can try by running
for instance the `activestorage/test/models/attached/many_test.rb`.
The Rails documentation uses the `:include:` directive to inline the
README of the framework into the main documentation page. As the
README's aren't in the root directory from where SDoc is run we need to
add the framework path to the include:
# :include: activesupport/README.md
This results in a warning when installing the gems as generating the rdoc for the gem is run from the gem/framework root:
Couldn't find file to include 'activesupport/README.rdoc' from lib/active_support.rb
The `:include:` RDoc directive supports includes relative to the current
file as well:
# :include: ../README.md
This makes sure it works for the Rails API docs and the separate gems.
Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>
For non-image attachments (like videos), generated representations are
created as a preview_image_attachment instead of as normal variants, and
as a result aren't included in the `with_attached_` scopes. This adds
those preview image attachments to the `includes` of these scopes to
avoid an N+1 when iterating over a collection of attachments and
fetching the key of their representation (variants).
Co-Authored-By: Justin Searls <searls@gmail.com>
* Remove pidfile in production
* Update changelog
* Update activestorage/test/dummy/config/puma.rb
Co-authored-by: Rafael Mendonça França <rafael@franca.dev>
* Update template and other dummy files
---------
Co-authored-by: Rafael Mendonça França <rafael@franca.dev>
RDoc treats consecutive indented lines as a single code block. For code
examples that span multiple files / languages, this confuses the syntax
highlighter and makes the examples harder to read. Unfortunately, RDoc
doesn't provide syntax to prevent this, and it ignores multiple
consecutive blank lines. However, by inserting an empty tag such as
`<code></code>`, we can force RDoc to recognize separate code blocks.
Now that we dropped support for Ruby 2.7, we no longer
need to check if variables are defined before accessing them
to avoid the undefined variable warning.
Adds space inside array literal brackets in ActiveStorage/ActionText migrations.
The new `rubocop-rails-omakase` enables `Layout/SpaceInsideArrayLiteralBrackets`,
which failed on the migrations created when installing ActiveStorage and ActionText.
Until now, Rails only droped compatibility with older
rubies on new majors, but I propose to change this policy
because it causes us to either keep compatibility with long
EOLed rubies or to bump the Rails major more often, and to
drop multiple Ruby versions at once when we bump the major.
In my opinion it's a bad alignments of incentives. And we'd
be much better to just drop support in new minors whenever they
go EOL (so 3 years).
Also Ruby being an upstream dependency, it's not even
a semver violation AFAICT.
Since Rails 7.2 isn't planned before a few months, we
can already drop Ruby 3.0 as it will be EOL in March.
