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]: 0618d2d84a
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.
Ref: https://github.com/rails/rails/pull/50793
Transactional fixtures are currently tightly coupled with the pool
active connection. It assumes calling `pool.connection` will memoize
the checked out connection and leverage that to start a transaction
on it and ensure all subsequent accesses will get the same connection.
To allow to remove checkout caching (or make it optional), we first
must decouple transactional fixtures to not rely on it.
The idea is to behave similarly, but store the connection in
the pool as a special "pinned" connection, and not as the regular
active connection.
This allows to always return the same pinned connection,
but without necessarily assigning it as the active connection.
Additionally, this pinning impact all threads and fibers, so
that all threads have a consistent view of the database state.
Extracted from: https://github.com/rails/rails/pull/50999
- Make fixtures setup and teardown methods private.
- Don't run adapter thread safety tests with sqlite3_mem
- Make foreign_key_tests more resilient to leaked state
- Use `exit!` in fork to avoid `at_exit` side effects.
- Disable transactional fixtures in tests that do a lot of low level
assertions on connections or connection pools.
The `create` method is currently marked as an alias of `new`. However,
because `new` is later overridden, it's no longer an alias.
This requires wrapping the `alias_method` with stopdoc/startdoc, as the
method is still marked as an alias otherwise (adding `:nodoc:` doesn't
help).
The `initialize` method has to be wrapped with stopdoc/startdoc as well,
as the `new` method will still be documented for the initialized.
Adding a `:nodoc:` instead will remove documentation for all following
methods.
All headers in a guide get a unique `dom_id` to make anchor links work.
If a header is already present we would prefix it with the `dom_id` of
the parent node. This would not work for headers without parent nodes.
This commit simplifies the `dom_id` uniqueness by only prepending the
parent node if it exists. This can still result in duplicates at the
same level, but for these we already show a warning:
*** DUPLICATE ID: 'some_id', please make sure that there are no
headings with the same name at the same level.
Co-authored-by: Carlos Antonio da Silva <carlosantoniodasilva@gmail.com>
When set, an `ActiveRecord::InvalidMigrationTimestampError` will be raised if the timestamp
prefix for a migration is more than a day ahead of the timestamp associated with the current time.
This is done to prevent forward-dating of migration files, which can impact migration generation
and other migration commands.
It is turned off by default, but will be turned on for applications starting in Rails 7.2.
As well as `disconnect!` and `verify!`.
This generally isn't a big problem as connections must not be shared between
threads, but is required when running transactional tests or system tests
and could lead to a SEGV.
Extracted from: https://github.com/rails/rails/pull/50999
Some tests may use the connections in ways that cause the fixtures
transaction to be committed or rolled back. The typical case being
doing schema change query in MySQL, which automatically commits the
transaction. But ther eare more subtle cases.
The general idea here is to ensure our transaction is correctly
rolling back during teardown. If it fails, then we assume something
might have mutated some of the inserted fixtures, so we invalidate
the cache to ensure the next test will reset them.
This issue is particularly common in Active Record's own test suite
since transaction fixtures are enabled by default but we have
many tests create tables and such.
We could treat this case as an error, but since we can gracefully
recover from it, I don't think it's worth it.
`self.class` is a fairly narrow cache key, so it doesn't hit that much,
but more importantly, since nothing clears that cache, on large test
suites it keeps growing extremely large.
Using the list of fixtures as a cache key doesn't strictly solve
the growth issue, but most classes actually load all fixtures so
this should shrink the cache size considerably.
Rafael Mendonça França