This commit fixes generated SQL for `UNION` and `UNION ALL` involving `LIMIT`
or `ORDER BY` by wrapping `SELECT` statements appearing in an `UNION` or
`UNION ALL` in parentheses.
Similar to 50515fb45f36dfad067adbdda9fee41fcb326ca9, make sure we
require `ostruct` where we use `OpenStruct`, to get the build back to
green, while we work to remove its usage on tests. (see #51510.)
Sample error:
```
Error:
ActiveRecord::Encryption::ConfigurableTest#test_installing_autofiltered_parameters_will_add_the_encrypted_attribute_as_a_filter_parameter_using_the_dot_notation:
NameError: uninitialized constant ActiveRecord::Encryption::ConfigurableTest::OpenStruct
test/cases/encryption/configurable_test.rb:45:in `block in <class:ConfigurableTest>'
```
Also change another anonymous block to a named block as requested by
skipkayhil, although it does not trigger the same error as the method
does not contain any keyword arguments.
Fix: https://github.com/rails/rails/issues/51386
This significantly reduce the depth of the tree for large `OR`
conditions. I was initially a bit on the fence about that fix,
but given that `And` is already implemented this way, I see no
reasons not to do the same.
Amusingly, the reported repro script now makes SQLite fail:
```ruby
SQLite3::SQLException: Expression tree is too large (maximum depth 1000)
```
Ref: https://github.com/rails/rails/pull/26103
Ref: https://github.com/rails/rails/pull/51426
A fairly common mistake with Rails is to enqueue a job from inside a
transaction, and a record as argumemnt, which then lead to a RecordNotFound
error when picked up by the queue.
This is even one of the arguments advanced for job runners backed by the
database such as `solid_queue`, `delayed_job` or `good_job`. But relying
on this is undesirable iin my opinion as it makes the Active Job abstraction
leaky, and if in the future you need to migrate to another backend or even
just move the queue to a separate database, you may experience a lot of race
conditions of the sort.
But more generally, being able to defer work to after the current transaction
has been a missing feature of Active Record. Right now the only way to do it
is from a model callback, and this forces moving things in Active Record
models that sometimes are better done elsewhere. Even as a self-proclaimed
"service object skeptic", I often wanted this capability over the last decade,
and I'm sure it got asked or desired by many more people.
Also there's some 3rd party gems adding this capability using monkey patches.
It's not a reason to upstream the capability, but it's a proof that there is
demand for it.
Implementation wise, this proof of concept shows that it's not really hard to
implement, even with nested multi-db transactions support.
Co-Authored-By: Cristian Bica <cristian.bica@gmail.com>
That is, if the reflection found is not valid, we don't want to emit the warning
because it's misleading. It says the inverse association could have been automatically
inferred but wasn't because `automatically_invert_plural_associations` is disabled.
However, this is not true, because later on, when we check whether the reflection
is valid, we see it's not, and end up returning `nil`.
We had to revert rails/rails@6dd1929 due to some regressions it caused. Here are some tests that would prevent those regressions in the future. See previous commits for more detail.
This reverts commit 6dd1929b04de42cdff1264bb5618c7f6abe77a9c.
This introduced a change in behaviour since type_for_attribute is aware of custom types but lookup_cast_type does not.
Additionally, since we no longer to use the table and column info to get attributes, this introduced an issue where attribute types were not be correctly found for some queries, where we were joining a table that has a different name than the name of the reflection for that association.
This commit makes two types of queries retry-able by opting into our `allow_retry` flag:
1) SELECT queries we construct by walking the Arel tree via `#to_sql_and_binds`. We use a
new `retryable` attribute on collector classes, which defaults to true for most node types,
but will be set to false for non-idempotent node types (functions, SQL literals, etc). The
`retryable` value is returned from `#to_sql_and_binds` and used by `#select_all` and
passed down the call stack, eventually reaching the adapter's `#internal_exec_query` method.
Internally-generated SQL literals are marked as retryable via a new `retryable` attribute on
`Arel::Nodes::SqlLiteral`.
2) `#find` and `#find_by` queries with known attributes. We set `allow_retry: true` in `#cached_find_by`,
and pass this down to `#find_by_sql` and `#_query_by_sql`.
These changes ensure that queries we know are safe to retry can be retried automatically.
Some association options, e.g. `through`, were only added to
`.valid_options` list only if they were provided. It means that
sometimes ArgumentError's message would not mention all the
possible options.
Ref: https://github.com/rails/rails/pull/50284
While having the inverse association configured it generally positive
as it avoid some extra queries etc, infering it may break legecy code,
as evidenced by how it broke `ActiveStorage::Blob` in https://github.com/rails/rails/pull/50800
As such we can't just enable this behavior immediately, we need to provide
and upgrade path for users.
When `.connection` is called inside a `.with_connection` block
it cause the lease to become permanent (or sticky). This is to
ensure that legacy code that may hold onto this connection
won't cause thread safety issues.
However if you autidted the code that calls `.connection` and
know for sure it won't hold into the returned connection,
you can wrap it with `with_connection(prevent_permanent_checkout: true)`
to prevent the lease from becoming permanent and ensure the connection
is released at the end of the block.
Controls whether `ActiveRecord::Base.connection` raises an error, emits a deprecation warning, or neither.
`ActiveRecord::Base.connection` checkouts a database connection from the pool and keep it leased until the end of
the request or job. This behavior can be undesirable in environments that use many more threads or fibers than there
is available connections.
This configuration can be used to track down and eliminate code that calls `ActiveRecord::Base.connection` and
migrate it to use `ActiveRecord::Base.with_connection` instead.
The default behavior remains unchanged, and there is currently no plans to change the default.
The change to upserts (https://github.com/rails/rails/pull/51274) is causing
an issue with Vitess, since it doesn't support the `row_alias` syntax added
in MySQL 8.0.19.
There is an ongoing work (https://github.com/vitessio/vitess/pull/15510)
to add that support and it is likely to be included into Vitess v20,
but in the meantime it would be nice to have an ability to control that
behavior in the existing apps.
Association's `primary_key` can be composite when derived from associated class
`primary_key` or `query_constraints`. But we don't allow setting it explicitly
even though Rails is already capable of supporting it.
This commit allows `primary_key` association option to be an array.
Trilogy closes its connection when it encounters a timeout. Active
Record doesn't always account for this, and can query the adapter in
ensure blocks of certain adapter methods. Instead, we need to preface
these queries with active checks to make sure we don't swallow the
timeout and raise an adapter error instead.
Co-authored-by: Adrianna Chang <adrianna.chang@shopify.com>
Currently the only way to setup a composite foreign key is to use `query_constraints`.
With our intention to decouple `query_constraints` from `foreign_key` we need to allow
`foreign_key` to be an Array to preserve the current behavior achieved with `query_constraints`.
The memoization of Scheme#key_provider was removed completely in
https://github.com/rails/rails/pull/51019 because it prevented
overriding the `key_provider` via `with_encryption_context`. However,
this also made our tests for the HEY app about 5 times slower. We traced
this down to all attributes where we either provide a key or declare them
as deterministic and have to derive a key to instantiate the provider
every time we load them. This might happen hundreds of times per test,
and ultimately, we call
```
ActiveSupport::KeyGenerator#generate_key
```
and
```
OpenSSL::KDF.pbkdf2_hmac
```
hundreds of times. This adds significant overhead per test.
In reality, what's overridden bv `with_encryption_context` is the value
used as default provider, that is, `ActiveRecord::Encryption.key_provider`.
This is only used in `Scheme#key_provider` if the scheme doesn't already have
either a `key_provider` passed directly, or a `key`, or is `deterministic`.
The `key_provider` passed directly is already memoized simply by having it
stored as is. Let's memoize the other two, in order, so we save all those
extra calls to derive the same keys again and again.
This commit addresses the following errors against MySQL 8.0.18 or lower version of MySQL 8.0.
- Steps to reproduce
```ruby
git clone https://github.com/rails/rails
cd rails
git clone https://github.com/rails/buildkite-config .buildkite/
RUBY_IMAGE=ruby:3.3 docker-compose -f .buildkite/docker-compose.yml build base &&
CI=1 MYSQL_IMAGE=mysql:8.0.18 docker-compose -f .buildkite/docker-compose.yml run mysqldb runner activerecord 'rake db:mysql:rebuild test:mysql2'
```
- Actual behavior
```ruby
... snip ...
Error:
InsertAllTest#test_upsert_all_implicitly_sets_timestamps_on_create_when_model_record_timestamps_is_false_but_overridden:
ActiveRecord::StatementInvalid: Mysql2::Error: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'AS `ships_values` ON DUPLICATE KEY UPDATE updated_at=(CASE WHEN (`ships`.`name`<' at line 1
/usr/local/bundle/gems/mysql2-0.5.6/lib/mysql2/client.rb:151:in `_query'
/usr/local/bundle/gems/mysql2-0.5.6/lib/mysql2/client.rb:151:in `block in query'
/usr/local/bundle/gems/mysql2-0.5.6/lib/mysql2/client.rb:150:in `handle_interrupt'
/usr/local/bundle/gems/mysql2-0.5.6/lib/mysql2/client.rb:150:in `query'
lib/active_record/connection_adapters/mysql2/database_statements.rb:104:in `block (2 levels) in raw_execute'
lib/active_record/connection_adapters/abstract_adapter.rb:997:in `block in with_raw_connection'
/rails/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:23:in `handle_interrupt'
/rails/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:23:in `block in synchronize'
/rails/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:19:in `handle_interrupt'
/rails/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:19:in `synchronize'
lib/active_record/connection_adapters/abstract_adapter.rb:969:in `with_raw_connection'
lib/active_record/connection_adapters/mysql2/database_statements.rb:102:in `block in raw_execute'
/rails/activesupport/lib/active_support/notifications/instrumenter.rb:58:in `instrument'
lib/active_record/connection_adapters/abstract_adapter.rb:1112:in `log'
lib/active_record/connection_adapters/mysql2/database_statements.rb:101:in `raw_execute'
lib/active_record/connection_adapters/abstract_mysql_adapter.rb:237:in `execute_and_free'
lib/active_record/connection_adapters/mysql2/database_statements.rb:23:in `internal_exec_query'
lib/active_record/connection_adapters/abstract/database_statements.rb:171:in `exec_insert_all'
lib/active_record/connection_adapters/abstract/query_cache.rb:26:in `exec_insert_all'
lib/active_record/insert_all.rb:55:in `execute'
lib/active_record/insert_all.rb:13:in `block in execute'
lib/active_record/connection_adapters/abstract/connection_pool.rb:384:in `with_connection'
lib/active_record/connection_handling.rb:270:in `with_connection'
lib/active_record/insert_all.rb:12:in `execute'
lib/active_record/persistence.rb:363:in `upsert_all'
test/cases/insert_all_test.rb:561:in `block in test_upsert_all_implicitly_sets_timestamps_on_create_when_model_record_timestamps_is_false_but_overridden'
test/cases/insert_all_test.rb:809:in `with_record_timestamps'
test/cases/insert_all_test.rb:560:in `test_upsert_all_implicitly_sets_timestamps_on_create_when_model_record_timestamps_is_false_but_overridden'
bin/rails test /rails/activerecord/test/cases/insert_all_test.rb:557
E
... snip ...
8856 runs, 25842 assertions, 1 failures, 52 errors, 41 skips
```
Follow up #51274
Refer to these release notes, WL and commits for MySQL 8.0.19 and 8.0.20.
- MySQL 8.0.19 supports aliases in the VALUES and SET clauses of INSERT INTO ... ON DUPLICATE KEY UPDATE statement
https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-19.html
> MySQL now supports aliases in the VALUES and SET clauses of INSERT INTO ... ON DUPLICATE KEY UPDATE statement
> for the row to be inserted and its columns. Consider a statement such as this one:
https://dev.mysql.com/worklog/task/?id=6312c39355e9e6
- MySQL 8.0.20 deprecates the old `VALUES()` syntax in INSERT ... ON DUPLICATE KEY UPDATE statements
https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-20.html
> The use of VALUES() to access new row values in INSERT ... ON DUPLICATE KEY UPDATE statements
> is now deprecated, and is subject to removal in a future MySQL release.
> Instead, you should use aliases for the new row and its columns as implemented in MySQL 8.0.19 and later.
https://dev.mysql.com/worklog/task/?id=133256f3b9df50b
The API documentation for `ActiveRecord::Migration` mentions controlling
the level of log output through a `.verbose` class attribute. The line
isn't wrapped in `<tt>`, `+`, or backticks, so isn't emphasized as if it
were code.
This commit visually emphasizes that line so that it's more obviously
code.
This adds a `dirties` option to `ActiveRecord::Base.uncached` and
`ActiveRecord::ConnectionAdapters::ConnectionPool#uncached`.
Setting `dirties` to `false`, means database writes to the connection
pool will not mark any query caches as dirty.
The option defaults to `true` which retains the existing behaviour and
clears query caches on all connection pools used by the current thread.
Co-authored-by: Jeremy Daer <jeremy@rubyonrails.org>
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
Ref: https://github.com/rails/rails/pull/51083
The introduction of `ActiveRecord::Base.with_connection` somewhat broke
some expectations, namely that calling `.connection` would cause the
connection to be permenently leased, hence that future calls to it
would return the same connection, with all it's possible environmental
changes.
So any call to `.connection`, even inside `.with_connection` should
cause the lease to be sticky, and persist beyond the `with_connection`
block.
Also rename `.connection` into `.lease_connection`, as to make it
more explicit.
```ruby
assert_equal "Ruby on Rails", web_sites(:rubyonrails).name
assert_equal "Ruby on Rails", fixture(:web_sites, :rubyonrails).name
```
This was brought to me by someone with a `Metadata` model. The fixtures
accessor being `metadata` which conflicts with the `metadata` method
recently added in `Minitest`.
Extracted from: https://github.com/rails/rails/pull/50793
Right now quoting table or column names requires a leased Adapter
instance, even though none of the implementations actually requires
an active connection.
The idea here is to move these methods to the class so that the quoting
can be done without leasing a connection or even needing the connection
to ever have been established.
I also checked `activerecord-sqlserver-adapter` and `oracle-enhanced`
gems, and neither need an active connection.
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.
* relation#order supports hash like relation#where
This allows for an ActiveRecord::Relation to take a hash such as
`Topic.includes(:posts).order(posts: { created_at: :desc })`
* use is_a? to support subclasses of each
Co-authored-by: Rafael Mendonça França <rafael@rubyonrails.org>
Ref: https://github.com/rails/rails/pull/50793
To make not caching connection checkout viable, we need to reduced
the amount of places where we need a connection.
Once big source of this is query/relation building, where in many
cases it eagerly quote and interpolation bound values in SQL fragments.
Doing this requires an active connection because both MySQL and Postgres
may quote values differently based on the connection settings.
Instead of eagerly doing all this, we can instead just insert these
as bound values in the Arel AST. For adapters with prepared statements
this is better anyway as it will avoid leaking statements, and for those
that don't support it, it will simply delay the quoting to just
before the query is executed.
However, the `%` API (`where("title = %s", something)`) can't realistically
be fixed this way, but I don't see much value in it and it probably should
be deprecated and removed.
Followup: https://github.com/rails/rails/pull/50938
The behavior changed to always clear the query cache as soon
as it's disabled, on the assumption that once queries have been
performed without it, all bets are off.
However that isn't quite true, we can apply the same clearing
logic than when it's enabled. If you perform read only queries
without the cache, there is no reason to clear it.
* Prevent extra newlines after table generation block in shema.rb file when there are no foreign keys or indexes. This will produce valid code without linting issues (issue: Trailing whitespace detected)
* fix linting issues
* fix linting
Co-authored-by: Rafael Mendonça França <rafael@rubyonrails.org>
* Support encrypting binary columns
ActiveRecord Encryption doesn't prevent you from encrypting binary
columns but it doesn't have proper support for it either.
When the data is fed through encrypt/decrypt it is converted to a
String. This means that the the encryption layer is not transparent
to binary data - which should be passed as Type::Binary::Data.
As a result the data is not properly escaped in the SQL queries or
deserialized correctly after decryption.
However it just happens to work fine for MySQL and SQLite because the
MessageSerializer doesn't use any characters that need to be encoded.
However if you try to use a custom serializer that does then it breaks.
PostgreSQL on the other hand does not work - because the Bytea type
is passed a String rather than a Type::Binary::Data to deserialize, it
attempts to unescape the data and either mangles it or raises an error
if it contains null bytes.
The commit fixes the issue, by reserializing the data after
encryption and decryption. For text data that's a no-op, but for binary
data we'll convert it back to a Type::Binary::Data.
* Extract decrypt_as_text/encrypt_as_text
* Handle serialized binary data in encrypted columns
Calling `serialize` is not always possible, because the column type
might not expect to be serializing a String, for example when declared
as serialzed or store attribute.
With binary data the encryptor was passed an
`ActiveModel::Type::Binary::Data`` and returned a `String``. In order to
remain transparent we need to turn the data back into a
`ActiveModel::Type::Binary::Data` before passing it on.
We'll also rename `serialize`` to `text_to_database_type` to be a bit
more descriptive.
Another refactoring in relation to https://github.com/rails/rails/pull/50793
But it makes sense even without it.
Rather than each connection to have its own `BoundSchemaReflection`,
we can instead have `BoundSchemaReflection` hold a `ConnectionPool`,
from which it can checkout a connection to perform queries when needed.
If the current thread already leased a connection, it will be used.
This simplifies the interface quite a bit.
Some tests do leak connections, which cause some other tests to stall
for a very long time when they call `clear_all_connections`.
Each leaked connection takes 5 seconds to be cleared because of the
checkout timeout.
So on teardown to inspect all pools to make sure nothing was leaked
and if some connections were leaked we cleanup the state.
This will be eager loaded by the define_attribute_methods initializer
now that the schema cache can be automatically loaded for all
connection if the file is present on disk after #48716.
Extracted from https://github.com/rails/rails/pull/50793
The leased connection is yielded, and for the duration of the block,
any call to `ActiveRecord::Base.connection` will yield that same connection.
This is useful to perform a few database operations without causing a
connection to be leased for the entire duration of the request or job.
Mostly extracted from: https://github.com/rails/rails/pull/50793
- Use `#adapter_name` instead of `#class::ADAPTER_NAME`
- Avoid monkey patching some connection instances
- Ignore SCHEMA queries in some asssertions
Otherwise they could linger around and leak memory if a user
checkout connections from short lived fibers or threads.
The undocumented `connection_cache_key` hook point is eliminated
because it was essentially add to allow the connection pool to
be fiber based rather than thread based, which is now supported
out of the box.
Ref: https://github.com/rails/rails/pull/50793
If we want to stop caching the checked out connections,
then we must persist the cache in the pool, and assign it
to the connection when it's checked out.
The pool become responsible for managing the cache lifecycle.
This also open the door to sharing the cache between multiple
connections, which is valuable for read replicas, etc.
This change only really make sense if we go through with no
longer caching checked out connections. Otherwise it's just
extra complexity.
* Deprecate config.active_record.warn_on_records_fetched_greater_than
* Review changes
Co-authored-by: Rafael Mendonça França <rafael@franca.dev>
---------
Co-authored-by: Jason Nochlin <hundredwatt@users.noreply.github.com>
Co-authored-by: Rafael Mendonça França <rafael@franca.dev>
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.
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.
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
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.
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.
On my machine, running the whole Active Record test suite takes
`88` seconds, and `40` of these are spent in encryption tests.
Some of them also happen to flake because of random blips.
I appreciate the care that has been put into ensuring the overhead
of encrption was reasonable, but I don't think these tests justify
their cost.
Most of the time was spent waiting on the default 5 seconds
checkout timeout.
Reducing in for just these tests saves about 1 minute of run time
but more importantly saves me from trying to figure out if my
refactoring introduced a deadlock of some sort.
Before: `real 1m4.448s`
After: `real 0m6.395s`
The context of this example suggests what it really wants to
point to is `query_constraints` instead of `query_by`.
Also apply fixed-width font to the reference to the methods correctly.
* Switch ActiveSupport::TestCase teardown and setup callbacks to run in setup and teardown minitest lifecycle hooks.
Minitest provides `setup` and `teardown` lifecycle hooks to run code in. In general it is best practice to when defining your own test case class, to use `Minitest::TestCase.setup` and `Minitest::TestCase.teardown` instead of `before_setup` and `after_teardown`.
Per Minitest's Documentation on Lifecycle Hooks: https://docs.ruby-lang.org/en/2.1.0/MiniTest/Unit/LifecycleHooks.html
> before_setup()
> Runs before every test, before setup. This hook is meant for libraries to extend minitest. It is not meant to be used by test developers.
> after_teardown()
> Runs after every test, after teardown. This hook is meant for libraries to extend minitest. It is not meant to be used by test developers.
Since the `setup` and `teardown` ActiveSupport::TestCase callbacks are in essence user code, it makes sense to run during their corresponding Minitest Lifecycle hooks.
* Ensure test fixutres are torndown on errors in superclass after_teardown code.
By not adding wrapping the `teardown_fixtures` code, its possible that super raises an error and prevents the existing database transaction from rolling back.
`super` in general should only be calling `Minitest::Testcase.after_teardown` however, if another library were to override `Minitest::Testcase.after_teardown`, like the popular gem [rspec-mocks](https://github.com/rspec/rspec-mocks/blob/main/lib/rspec/mocks/minitest_integration.rb#L23) does, it causes all subsequent tests to retain any changes that were made to the database in the original test that errors.
* Remove unnecessary setup and teardown methods in tests
* update activesupport Changelog
* Fix linter issues in CHANGELOG
* fix tests with improper setup and teardown method definitions
* Fix final CHANGELOG lint
* Revert "Fix final CHANGELOG lint"
This reverts commit f30682eb629780862ccc63e1d3210dfe035e997e.
* Revert "fix tests with improper setup and teardown method definitions"
This reverts commit 1d5b88c8739695a4eed5c46924c9ffc6010353f5.
* Revert "Fix linter issues in CHANGELOG"
This reverts commit 60e89bd189cbcdf50d7e923a90ec5ebe1578a6e9.
* Revert "update activesupport Changelog"
This reverts commit 0f19bc324fec7a793cc34dcfede27017b5a24e46.
* Revert "Remove unnecessary setup and teardown methods in tests"
This reverts commit e5673f179ac01c814ab44017b97e7638aad6e775.
* Revert "Switch ActiveSupport::TestCase teardown and setup callbacks to run in setup and teardown minitest lifecycle hooks."
This reverts commit d08d92d86131d8643a275397d9b0c15995730a14.
* Rescue Minitest::Assertion errors in ActiveSupport::TestCase.teardown callback code to ensure all other after_teardown methods are called.
* Fix name of test class
* remove unused MyError class
* Fix module to not be in global namespace
Co-authored-by: Rafael Mendonça França <rafael@rubyonrails.org>
Not that `inspect` is anywhere close to be a hostspot, but just
to not allow a less defficient pattern.
Instead of allocating an UnboundMethod and bind_call it, we can
directly replace the method.
Add a `compress` option to ActiveRecord::Encryption::Encryptor, which
defaults to `true`. When set to `false`, the encryptor will never
compress the data.
This is useful for cases where the data is already compressed.
This can be used with the `encryptor` option in the model:
```ruby
class Record < ApplicationRecord
encrypts :field, encryptor: ActiveRecord::Encryption::Encryptor.new(compress: false)
end
```
When the application has more than one database configuration, only
the primary was being loaded by default on boot time. All the other
connection pools were loading the schema cache lazily.
This logic can be found in:
351a8d9bc9/activerecord/lib/active_record/connection_adapters/pool_config.rb (L13)351a8d9bc9/activerecord/lib/active_record/railtie.rb (L149-L178)
The lazy code path wasn't using the same logic to determine the name
of the default schema cache file that the Railties uses, so it
was loading the schema cache of the primary config to all the other
configs. If no table name coincided nothing bad would happen, but if
table names coincided, the schema would be completly wrong in all
non-primary connections.
This field returns the amount of rows returned by the query that emitted the notification.
This metric is useful in cases where one wants to detect queries with big result sets.
This commit makes a few changes to our trilogy error translation:
* https://github.com/trilogy-libraries/trilogy/pull/118 introduced
`Trilogy::EOFError` which we can use instead of matching on
`TRILOGY_CLOSED_CONNECTION`.
* https://github.com/trilogy-libraries/trilogy/pull/15 introduced
`Trilogy::ConnectionClosed`, which inherits from `IOError` for
backwards compatibility. As far as I can tell that's the only
`IOError` trilogy can raise, so this commit rescues the
trilogy-specific error instead.
* As far as I can tell Trilogy does not raise `SocketError`, so don't
bother translating that
* Don't treat TRILOGY_UNEXPECTED_PACKET as a connection error. If we get
this, it's probably a bug in trilogy that we should fix. I'd like to
eventually get rid of TRILOGY_INVALID_SEQUENCE_ID too, but we're
currently relying on it in a few tests (related to trilogy missing
caching_sha2_password auth support, if I recall correctly)
I'm kinda hoping we'll eventually be able to simplify this to something
like:
```rb
if exception.is_a?(Trilogy::ConnectionError)
ConnectionFailed.new(message, connection_pool: @pool)
else
super
end
```
but we'd need more changes to trilogy before that is possible.
1053 is the server shutdown error code. We see these fairly often at
GitHub when, for example, shutting down Vitess vtage processes for
maintenance. Translating the error to `ConnectionFailed` allows us to
treat these as retryable connection error, so we can reconnect and hit a
new vtage process.
This is all true regardless of whether the adapter is trilogy or mysql2
or whatever, so this commit moves the translation out into the abstract
mysql adapter.
In SQLite3-Ruby version 2.0, I would like to make row arrays frozen. I
tested the change, and it only breaks this test, so I'm changing the
test. I don't think we should be mutating the objects that the database
adapter returns
At GitHub we get a fair number of Trilogy `ETIMEDOUT` errors (for known
reasons that we might be able to improve somewhat, but I doubt we'll
make them go away entirely). These are very much retryable network
errors, so it'd be handy if these `ETIMEDOUT` errors were translated to
`ConnectionFailed` instead of `StatementInvalid`, making them
`retryable_connection_error`s.
ed2bc92b82/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb (L1077)
We're already translating `ECONNRESET` (via matching on the error
message) and `EPIPE` to `ConnectionFailed`. Rather than adding another
case, this commit treats all of the Trilogy `SystemCallError` subclasses
as `ConnectionFailed`.
This requires bumping trilogy 2.7 so we can get
https://github.com/trilogy-libraries/trilogy/pull/143
Closes [#49574][]
Issue #49574 outlines how an Active Record join model accessed through a
`has_many ..., through:` association is unable to infer an appropriate
`:inverse_of` association by pluralizing a predictably pluralizable
class name.
This commit resolves that issue by also checking a model's reflections
for a pluralized inverse name in addition to whatever's provided through
the `:as` option or inferred in the singular.
The issue shares a code sample:
```ruby
ActiveRecord::Schema.define do
create_table "listings", force: :cascade do |t|
t.bigint "list_id", null: false
t.bigint "pin_id", null: false
end
create_table "lists", force: :cascade do |t|
end
create_table "pins", force: :cascade do |t|
end
end
class Pin < ActiveRecord::Base
has_many :listings
end
class List < ActiveRecord::Base
has_many :listings
has_many :pins, through: :listings
end
class Listing < ActiveRecord::Base
belongs_to :list
belongs_to :pin
end
class BugTest < Minitest::Test
def test_association_stuff
list = List.create!
pin = list.pins.new
pin.save!
assert_equal [pin], list.reload.pins
assert_equal 1, list.reload.pins.count
end
end
```
Unfortunately, there isn't a one-to-one mapping in the test suite's
`test/model` directory for this type of structure. The most similar
associations were between the `Book`, `Subscription`, and `Subscriber`.
For the sake of ease, this commit wraps the test block in a new
`skipping_validations_for` helper method to ignore validations declared
on the `Subscription` join table model.
[#49574]: https://github.com/rails/rails/issues/49574
Adds more examples to `#in_order_of`:
- what happens when dealing with `enum` +columns+
- what happens when passing `nil` as a +value+ for nullable columns
Avoid corrupting the cache by mutating the return value, and also
sligthly reduce memory usage when the quoting format often return
an unmodified string.
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>
Prior 7.1 Rails always included `primary_key` in `RETURNING` clause on
record creation. This was changed in 7.1 to include more auto-populated
columns if such columns exist. This change lead to situations where
no columns were requested in `RETURNING` clause, even the `primary_key`.
This change brings back the old behavior of always requesting the
`primary_key` in `RETURNING` clause if no other columns are requested.
MySQL 5.7.5+ supports generated columns, which can be used to create a column that is computed from an expression. This commit fixes the escaping of the default value for such expressions if a single quote is included.
See the following for more: https://dev.mysql.com/blog-archive/generated-columns-in-mysql-5-7-5/
Option validation was [added][1] for 7.1+ Migration classes, and a
compatibility layer was added to ensure that previous Migration versions
do not have their options validated. However, the `t.references` method
was missing in the compatibility layer which results in pre 7.1
Migrations validating options passed to `t.references`.
This commit fixes the issue by adding t.references to the compatibility
layer.
See also a [similar fix][2] for `add_reference`
[1]: e6da3ebd6c65af23d134a9e01145f26600912008
[2]: 71b4e223018d180b7c96915c0df1c28afbf7cc53
Option validation was [added][1] for 7.1+ Migration classes, and
a compatibility layer was added to ensure that previous Migration
versions do not have their options validated. However, the add_reference
method was missing in the compatibility layer which results in pre 7.1
Migrations validating options passed to add_reference.
This commit fixes the issue by adding add_reference to the compatibility
layer. In addition to adding add_reference to the "no validation"
compatibility test, the test is refactored to run against each previous
migration version to ensure that they all behave consistently.
[1]: e6da3ebd6c65af23d134a9e01145f26600912008
Fix: https://github.com/rails/rails/pull/50745
I went a bit farther and handled all the boolean configs, not just `schema_cache`.
Co-Authored-By: Mike Coutermarsh <coutermarsh.mike@gmail.com>
