Since #50715, there are cases where `ActiveJob::Arguments` is used
but not required anymore in the test environment. This could be a
test for a custom argument serializer.
One solution could be to add a require "active_job/arguments" in
all the usages, but that isn't a convention we follow in Rails.
Instead, let's register the autoload for `ActiveJob::Arguments` in
`active_job.rb` so that it's always available when needed.
Trilogy doesn't currently support prepared statements. The error that
applications would see is a `StatementInvalid` error. This doesn't quite point
you to the fact this isn't supported. So raise a more appropriate error
pointing to what to change.
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
Configure ActiveRecord::Encryption (ARE) on ActiveRecord::Base (AR)
loading, so that ARE configs are ready before AR models start using
`encrypts` to declare encrypted attributes.
This means that you can add ARE configurations in initializers, as long
as you don't trigger the loading of ActiveRecord::Base or your AR models
in prior initializers.
Fix: https://github.com/rails/rails/issues/51002
In the default middleware stack, the `ShowExceptions` middleware is
lower than `ActionDispatch::Execturor` and will handle most exceptions
causing `Executor` not to witness any.
Instead we need to rely on `action_dispatch.exception` being added
into the request env.
Enums have historically been defined using keyword arguments:
```ruby
class Function > ApplicationRecord
enum color: [:red, :blue],
type: [:instance, :class],
_scopes: false
```
This has the advantage of being able to define multiple enums at once
with the same options. However, it also has a downside that enum options
must be prefixed with an underscore to separate them from the enum
definitions (to enable models to have enums with the same name as an
option).
In Rails 7, a new syntax was [introduced][1] to instead define enums with
positional arguments:
```ruby
class Function > ApplicationRecord
enum :color, [:red, :blue], scopes: false
enum :type, [:instance, :class], scopes: false
```
This new syntax eliminates the need to prefix options with an underscore,
and the docs were updated to recommend this new syntax.
However, both versions of the API have been supported since, and it has
started to cause some problems:
The first issue is that the available options have drifted. In Rails
7.1, an option was added to make assigning an invalid enum value use
validation errors instead of runtime errors. However, the equivalent
underscored prefix option was not added for the original enum syntax
Articles have been created that describe the new option in Rails 7.1,
but the examples in the articles use un-prefixed options with the old
syntax. This confusion has also lead to issues opened asking why that
incorrect syntax is not working.
Additionally, the presence of underscored options is just generally
confusing because it tends to imply an option is for internal use.
This commit aims to fix all of these issues by deprecating the old enum
syntax. With only one way to define enums, options cannot drift and
there will be less confusion around how enums should be defined.
[1]: 0618d2d84a501aea93c898aec504ff9a0e09d6f2
This paragraph tends to cause confustion with Rails developers who have just learned the correct control flow of `redirect_to` and `render`.
A phrase above it in the "Avoiding Double Render Errors" section clears up a common misunderstanding that `render` will stop execution within a controller action. That misunderstanding is further corrected with the following phrase.
> But this will _not_ stop the rest of the code in the `show` action from running
This section is often enough to get the point across for newer developers who might often be running into double render errors, but soon after the phrase which attempts the same illustration of `redirect_to`'s execution confuses the matter by using the same language as the earlier statement to mean the opposite.
> Sometimes inexperienced developers think of `redirect_to` as a sort of `goto` command, moving execution from one place to another in your Rails code. This is _not_ correct. Your code stops running and waits for a new request from the browser.
Here "Your code stops running" means that the action will run to completion and not jump to another site in controller code. This often re-confuses the concept they've just learned and undermines their understanding. By being more specific in the second phrase, we can reinforce the concept instead.
Prefer references to git's default configuration, rather than encouraging customized configurations.
`git config core.excludesFile` defaults to `${XDG_CONFIG_HOME:-~/.config}/git/ignore`. This updates the .gitignore template to suggest the default location for git's global gitignore rather than a customized location.
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>
This changes TestParser to parse with prism instead of ripper if it
is available for the current version of Ruby. It's within the margin
for the speed, and its significantly less code that is easier to
read and should be easier to maintain.
See: https://bugs.ruby-lang.org/issues/20250
The bug exist all the way since Ruby 2.7, if you `clone` a `Proc`
object on which you already accessed `object_id`, when its clone
is GCed Ruby will crash.
By accessing the clone's `object_id` right away, we prevent the
crash.
Fix: https://github.com/rails/rails/issues/50930
While Action View is predominantly meant to render text,
in sime case it's used to render more complex object.
So we shouldn't assume `#_run` returns a buffer.
