The specification describes values must be URL encoded first, then any remaining single quote characters must be escaped: https://google.github.io/sqlcommenter/spec/#value-serialization
Fortunately ERB::Util.url_encode already encodes this character, unlike for example Javascript's encodeURIComponent, so this step is unneccessary.
Fix Active Record Query Logs SQLCommenter sorting
Serialized key-value pairs must be sorted by lexicographic order: https://google.github.io/sqlcommenter/spec/#sorting
Forgot to update the changelog when we changed the default values for
`config.active_record.query_log_tags_format` from `:legacy` to
`:sqlcommenter`.
This change is a followup to: https://github.com/rails/rails/issues/46179
Prior to this commit, `ciphertext_for` returned the cleartext of values
that had not yet been encrypted, such as with an unpersisted record:
```ruby
Post.encrypts :body
post = Post.create!(body: "Hello")
post.ciphertext_for(:body)
# => "{\"p\":\"abc..."
post.body = "World"
post.ciphertext_for(:body)
# => "World"
```
This commit fixes `ciphertext_for` to always return the ciphertext of
encrypted attributes:
```ruby
Post.encrypts :body
post = Post.create!(body: "Hello")
post.ciphertext_for(:body)
# => "{\"p\":\"abc..."
post.body = "World"
post.ciphertext_for(:body)
# => "{\"p\":\"xyz..."
```
Fixed a bug that caused the alias name of "group by" to be too long and the first half of the name would be the same in both cases if it was cut by max identifier length.
Fix#46285
Co-authored-by: Yusaku ONO <yono@users.noreply.github.com>
The `BeforeTypeCast` module defines `read_attribute_before_type_cast`,
`attributes_before_type_cast`, and `*_before_type_cast` attribute
methods. It also defines `attributes_for_database` and `*_for_database`
attribute methods, but no corresponding `read_attribute_for_database`
method.
This commit adds the missing `read_attribute_for_database` method.
Prior to this commit, encrypted attributes that used column default
values appeared to be encrypted on create, but were not:
```ruby
Book.encrypts :name
book = Book.create!
book.name
# => "<untitled>"
book.name_before_type_cast
# => "{\"p\":\"abc..."
book.reload.name_before_type_cast
# => "<untitled>"
```
This commit ensures attributes with column default values are encrypted:
```ruby
Book.encrypts :name
book = Book.create!
book.name
# => "<untitled>"
book.name_before_type_cast
# => "{\"p\":\"abc..."
book.reload.name_before_type_cast
# => "{\"p\":\"abc..."
```
The existing "support encrypted attributes defined on columns with
default values" test in `encryptable_record_test.rb` shows the intended
behavior, but it was not failing without a `reload`.
In a multi-db world, delegating from `Base` to the handler doesn't make
much sense. Applications should know when they are dealing with a single
connection (Base.connection) or the handler which deals with multiple
connections. Delegating to the connection handler from Base breaks this
contract and makes behavior confusing. I think eventually a new object
will replace the handler altogether but for now I'd like to just
separate concerns to avoid confusion.
We don't need to pass the db configs to `check_pending_migrations`
because we always want to loop through all of them so the argument
doesn't gain us anything. It's also a private API so applications
shouldn't be consuming this method (and therefore makes it even less
necessary to allow configs to be passed in)
Follow-up to #44625.
In #44625, the `SerializeCastValue` module was added to allow types to
avoid a redundant call to `cast` when serializing a value for the
database. Because it introduced a new method (`serialize_cast_value`)
that was not part of the `ActiveModel::Type::Value` contract, it was
designed to be opt-in. Furthermore, to guard against incompatible
`serialize` and `serialize_cast_value` implementations in types that
override `serialize` but (unintentionally) inherit `serialize_cast_value`,
types were required to explicitly include the `SerializeCastValue`
module to activate the optimization. i.e. It was not sufficient just to
have `SerializeCastValue` in the ancestor chain.
The `SerializeCastValue` module is not part of the public API, and there
are no plans to change that, which meant user-created custom types could
not benefit from this optimization.
This commit changes the opt-in condition such that it is sufficient for
the owner of the `serialize_cast_value` method to be the same or below
the owner of the `serialize` method in the ancestor chain. This means
a user-created type that only overrides `cast`, **not** `serialize`,
will now benefit from the optimization. For example, a type like:
```ruby
class DowncasedString < ActiveModel::Type::String
def cast(value)
super&.downcase
end
end
```
As demonstrated in the benchmark below, this commit does not change the
current performance of the built-in Active Model types. However, for a
simple custom type like `DowncasedString`, the performance of
`value_for_database` is twice as fast. For types with more expensive
`cast` operations, the improvement may be greater.
**Benchmark**
```ruby
# frozen_string_literal: true
require "benchmark/ips"
require "active_model"
class DowncasedString < ActiveModel::Type::String
def cast(value)
super&.downcase
end
end
ActiveModel::Type.register(:downcased_string, DowncasedString)
VALUES = {
my_big_integer: "123456",
my_boolean: "true",
my_date: "1999-12-31",
my_datetime: "1999-12-31 12:34:56 UTC",
my_decimal: "123.456",
my_float: "123.456",
my_immutable_string: "abcdef",
my_integer: "123456",
my_string: "abcdef",
my_time: "1999-12-31T12:34:56.789-10:00",
my_downcased_string: "AbcDef",
}
TYPES = VALUES.to_h { |name, value| [name, name.to_s.delete_prefix("my_").to_sym] }
class MyModel
include ActiveModel::API
include ActiveModel::Attributes
TYPES.each do |name, type|
attribute name, type
end
end
attribute_set = MyModel.new(VALUES).instance_variable_get(:@attributes)
TYPES.each do |name, type|
attribute = attribute_set[name.to_s]
Benchmark.ips do |x|
x.report(type.to_s) { attribute.value_for_database }
end
end
```
**Before**
```
big_integer 2.986M (± 1.2%) i/s - 15.161M in 5.078972s
boolean 2.980M (± 1.1%) i/s - 15.074M in 5.059456s
date 2.960M (± 1.1%) i/s - 14.831M in 5.011355s
datetime 1.368M (± 0.9%) i/s - 6.964M in 5.092074s
decimal 2.930M (± 1.2%) i/s - 14.911M in 5.089048s
float 2.932M (± 1.3%) i/s - 14.713M in 5.018512s
immutable_string 3.013M (± 1.3%) i/s - 15.239M in 5.058085s
integer 1.603M (± 0.8%) i/s - 8.096M in 5.052046s
string 2.977M (± 1.1%) i/s - 15.168M in 5.094874s
time 1.338M (± 0.9%) i/s - 6.699M in 5.006046s
downcased_string 1.394M (± 0.9%) i/s - 7.034M in 5.046972s
```
**After**
```
big_integer 3.016M (± 1.0%) i/s - 15.238M in 5.053005s
boolean 2.965M (± 1.3%) i/s - 15.037M in 5.071921s
date 2.924M (± 1.0%) i/s - 14.754M in 5.046294s
datetime 1.435M (± 0.9%) i/s - 7.295M in 5.082498s
decimal 2.950M (± 0.9%) i/s - 14.800M in 5.017225s
float 2.964M (± 0.9%) i/s - 14.987M in 5.056405s
immutable_string 2.907M (± 1.4%) i/s - 14.677M in 5.049194s
integer 1.638M (± 0.9%) i/s - 8.227M in 5.022401s
string 2.971M (± 1.0%) i/s - 14.891M in 5.011709s
time 1.454M (± 0.9%) i/s - 7.384M in 5.079993s
downcased_string 2.939M (± 0.9%) i/s - 14.872M in 5.061100s
```
Previously, most version of the Migration class would call `super` in
compatible_table_definition to ensure that they append all of the later
Migration verion's TableDefinition modules. However, the latest
Migration class did not do this and prepended its own TableDefinition
because there was no super method for it to call.
This led to an issue where Action Text migrations started failing on
Rails main, after migration option validation was added in e6da3eb. The
new functionality was not supposed to affect V6_0 Migrations, but it was
because both V6_1 and V7_0 were not calling super.
This commit fixes the issue by correctly calling super in both classes.
It also adds a compatible_table_definition method to Current that
returns the value passed in so that all of the versioned Migrations can
always return super.
In #38235 I moved advisory locks to their own named connections, then
in #39758 the advisory lock was left on Base.connection but then moved
it it's own connection handler. I believe with #45450 that this change
was made obsolete and can be returned to the prior behavior without
having to open an additional connection. The tests added pass and I also
tested this in my local demo to ensure that this is working correctly.
When I originally changed the behavior here Matthew noted that this
could be surprising for some setups that expect only one connection for
a running migration. I thought there was an issue related to this but I
can't find it.
This commit adds a step to validate the options that are used when managing columns and tables in migrations.
The intention is to only validate options for new migrations that are added. Invalid options used in old migrations are silently ignored like they always have been.
Fixes#33284Fixes#39230
Co-authored-by: George Wambold <georgewambold@gmail.com>
We recently let a few very easy to avoid warnings get merged.
The root cause is that locally the test suite doesn't run in
verbose mode unless you explictly pass `-w`.
On CI warnings are enabled, but there is no reason to look at the
build output unless something is failing. And even if one wanted
to do that, that would be particularly work intensive since warnings
may be specific to a Ruby version etc.
Because of this I believe we should:
- Always run the test suite with warnings enabled.
- Raise an error if a warning is unexpected.
We've been using this pattern for a long time at Shopify both in private
and public repositories.
> Sets readonly attributes for the returned relation.
The current documentation gives the impression that it can manipulate
`attr_readonly`.
Co-authored-by: Petrik de Heus <petrik@deheus.net>
When `column` is passed as an array and the column does not exist,
then `index_exists?` check would fail to return early which would cause
`index_name_for_remove` to raise an error instead of ignoring the
missing column. This change ensures that `defined_for` checks the
column options in addition to the column argument.
Fixes#46196
This fixes the following Rubocop offenses:
```
activerecord/lib/active_record/query_logs.rb:96:9: C: [Correctable] Layout/CaseIndentation: Indent when as deep as case.
when :legacy
^^^^
activerecord/lib/active_record/query_logs.rb:98:9: C: [Correctable] Layout/CaseIndentation: Indent when as deep as case.
when :sqlcommenter
^^^^
```
In Active Record `connection_class` already means "the class in your
application that established the connection" so I would prefer it only
have that one meaning. This change swaps `connection_class` for
`adapter_class` where applicable.
This code block was removed in a previous PR, but it had
a side effect of causing certian timestamp fields to be
changed from `datetime(6)` to `datetime` when performing a schema
migration in the 6.1 version of the migration class. This is due
to `options[:precision]` being set to `nil` and therefore
not being set to the default of 6 later on in the code path.
Instead of hosting the connection logic in the command object, the
database adapter should be responsible for connecting to a console session.
This patch moves #find_cmd_and_exec to the adapter and exposes a new API to
lookup the adapter class without instantiating it.
Co-authored-by: Paarth Madan <paarth.madan@shopify.com>
Most model attribute types try to cast a given value before serializing
it. This allows uncast values to be passed to finder methods and still
be serialized appropriately. However, when persisting a model, this
cast is unnecessary because the value will already have been cast by
`ActiveModel::Attribute#value`.
To eliminate the overhead of a 2nd cast, this commit introduces a
`ActiveModel::Type::SerializeCastValue` module. Types can include this
module, and their `serialize_cast_value` method will be called instead
of `serialize` when serializing an already-cast value.
To preserve existing behavior of any user types that subclass Rails'
types, `serialize_after_cast` will only be called if the type itself
(not a superclass) includes `ActiveModel::Type::SerializeCastValue`.
This also applies to type decorators implemented via `DelegateClass`.
Benchmark script:
```ruby
require "active_model"
require "benchmark/ips"
class ActiveModel::Attribute
alias baseline_value_for_database value_for_database
end
VALUES = {
my_big_integer: "123456",
my_boolean: "true",
my_date: "1999-12-31",
my_datetime: "1999-12-31 12:34:56 UTC",
my_decimal: "123.456",
my_float: "123.456",
my_immutable_string: "abcdef",
my_integer: "123456",
my_string: "abcdef",
my_time: "1999-12-31T12:34:56.789-10:00",
}
TYPES = VALUES.to_h { |name, value| [name, name.to_s.delete_prefix("my_").to_sym] }
class MyModel
include ActiveModel::API
include ActiveModel::Attributes
TYPES.each do |name, type|
attribute name, type
end
end
TYPES.each do |name, type|
$attribute_set ||= MyModel.new(VALUES).instance_variable_get(:@attributes)
attribute = $attribute_set[name.to_s]
puts "=" * 72
Benchmark.ips do |x|
x.report("#{type} before") { attribute.baseline_value_for_database }
x.report("#{type} after") { attribute.value_for_database }
x.compare!
end
end
```
Benchmark results:
```
========================================================================
Warming up --------------------------------------
big_integer before 100.417k i/100ms
big_integer after 260.375k i/100ms
Calculating -------------------------------------
big_integer before 1.005M (± 1.0%) i/s - 5.121M in 5.096498s
big_integer after 2.630M (± 1.0%) i/s - 13.279M in 5.050387s
Comparison:
big_integer after: 2629583.6 i/s
big_integer before: 1004961.2 i/s - 2.62x (± 0.00) slower
========================================================================
Warming up --------------------------------------
boolean before 230.663k i/100ms
boolean after 299.262k i/100ms
Calculating -------------------------------------
boolean before 2.313M (± 0.7%) i/s - 11.764M in 5.085925s
boolean after 3.037M (± 0.6%) i/s - 15.262M in 5.026280s
Comparison:
boolean after: 3036640.8 i/s
boolean before: 2313127.8 i/s - 1.31x (± 0.00) slower
========================================================================
Warming up --------------------------------------
date before 148.821k i/100ms
date after 298.939k i/100ms
Calculating -------------------------------------
date before 1.486M (± 0.6%) i/s - 7.441M in 5.006091s
date after 2.963M (± 0.8%) i/s - 14.947M in 5.045651s
Comparison:
date after: 2962535.3 i/s
date before: 1486459.4 i/s - 1.99x (± 0.00) slower
========================================================================
Warming up --------------------------------------
datetime before 92.818k i/100ms
datetime after 136.710k i/100ms
Calculating -------------------------------------
datetime before 920.236k (± 0.6%) i/s - 4.641M in 5.043355s
datetime after 1.366M (± 0.8%) i/s - 6.836M in 5.003307s
Comparison:
datetime after: 1366294.1 i/s
datetime before: 920236.1 i/s - 1.48x (± 0.00) slower
========================================================================
Warming up --------------------------------------
decimal before 50.194k i/100ms
decimal after 298.674k i/100ms
Calculating -------------------------------------
decimal before 494.141k (± 1.4%) i/s - 2.510M in 5.079995s
decimal after 3.015M (± 1.0%) i/s - 15.232M in 5.052929s
Comparison:
decimal after: 3014901.3 i/s
decimal before: 494141.2 i/s - 6.10x (± 0.00) slower
========================================================================
Warming up --------------------------------------
float before 217.547k i/100ms
float after 298.106k i/100ms
Calculating -------------------------------------
float before 2.157M (± 0.8%) i/s - 10.877M in 5.043292s
float after 2.991M (± 0.6%) i/s - 15.203M in 5.082806s
Comparison:
float after: 2991262.8 i/s
float before: 2156940.2 i/s - 1.39x (± 0.00) slower
========================================================================
Warming up --------------------------------------
immutable_string before
163.287k i/100ms
immutable_string after
298.245k i/100ms
Calculating -------------------------------------
immutable_string before
1.652M (± 0.7%) i/s - 8.328M in 5.040855s
immutable_string after
3.022M (± 0.9%) i/s - 15.210M in 5.033151s
Comparison:
immutable_string after: 3022313.3 i/s
immutable_string before: 1652121.7 i/s - 1.83x (± 0.00) slower
========================================================================
Warming up --------------------------------------
integer before 115.383k i/100ms
integer after 159.702k i/100ms
Calculating -------------------------------------
integer before 1.132M (± 0.8%) i/s - 5.769M in 5.095041s
integer after 1.641M (± 0.5%) i/s - 8.305M in 5.061893s
Comparison:
integer after: 1640635.8 i/s
integer before: 1132381.5 i/s - 1.45x (± 0.00) slower
========================================================================
Warming up --------------------------------------
string before 163.061k i/100ms
string after 299.885k i/100ms
Calculating -------------------------------------
string before 1.659M (± 0.7%) i/s - 8.316M in 5.012609s
string after 2.999M (± 0.6%) i/s - 15.294M in 5.100008s
Comparison:
string after: 2998956.0 i/s
string before: 1659115.6 i/s - 1.81x (± 0.00) slower
========================================================================
Warming up --------------------------------------
time before 98.250k i/100ms
time after 133.463k i/100ms
Calculating -------------------------------------
time before 987.771k (± 0.7%) i/s - 5.011M in 5.073023s
time after 1.330M (± 0.5%) i/s - 6.673M in 5.016573s
Comparison:
time after: 1330253.9 i/s
time before: 987771.0 i/s - 1.35x (± 0.00) slower
```
Commit 37d1429ab1 introduced the DummyERB to avoid loading the environment when
running `rake -T`.
The DummyCompiler simply replaced all output from `<%=` with a fixed string and
removed everything else. This worked okay when it was used for YAML values.
When using `<%=` within a YAML key, it caused an error in the YAML parser,
making it impossible to use ERB as you would expect. For example a
`database.yml` file containing the following should be possible:
development:
<% 5.times do |i| %>
shard_<%= i %>:
database: db/development_shard_<%= i %>.sqlite3
adapter: sqlite3
<% end %>
Instead of using a broken ERB compiler we can temporarily use a
`Rails.application.config` that does not raise an error when configurations are
accessed which have not been set as described in #35468.
This change removes the `DummyCompiler` and uses the standard `ERB::Compiler`.
It introduces the `DummyConfig` which delegates all known configurations to the
real `Rails::Application::Configuration` instance and returns a dummy string for
everything else. This restores the full ERB capabilities without compromising on
speed when generating the rake tasks for multiple databases.
Deprecates `config.active_record.suppress_multiple_database_warning`.
Prior to this change
t.virtual :column_name, type: :datetime
would erroneously produce the same result as
t.virtual :column_name, type: :datetime, precision: nil
This is because the code path for `virtual` skipped the default lookup:
- `t.datetime` is delegated to the `column` method
- `column` sees `type == :datetime` and sets a default `precision` is none was given
- `column` calls `new_column_definition` to add the result to `@columns_hash`
- `t.virtual` is delegated to the `column` method
- `column` sees `type == :virtual`, not `:datetime`, so skips the default lookup
- `column` calls `new_column_definition` to add the result to `@columns_hash`
- `new_column_definition` sees `type == :virtual`, so sets `type = options[:type]`
By moving the default lookup, we get consistent code paths:
- `t.datetime` is delegated to the `column` method
- `column` calls `new_column_definition` to add the result to `@columns_hash`
- `new_column_definition` sees `type == :datetime` and sets a default `precision` is none was given
- `t.virtual` is delegated to the `column` method
- `column` calls `new_column_definition` to add the result to `@columns_hash`
- `new_column_definition` sees `type == :virtual`, so sets `type = options[:type]`
- `new_column_definition` sees `type == :datetime` and sets a default `precision` is none was given
Active Record erroneously uses a precision of `nil` instead of the default
precision, when adding a virtual datetime column.
This tests that behavior ahead of fixing it.
Problem:
Though, the `MessageVerifier` that powers `signed_id` supports both
`expires_in` and `expires_at`, `signed_id` only supports `expires_in`.
Because of this, generating signed_id that expires at a certain time is
somewhat tedious. Imagine issuing a coupon that is valid only for a
day.
Solution:
Add `expires_at` option to `signed_id` to generate signed ids that
expire at the given time.
Building on the work done in #44576 and #44591, we extend the logic that automatically
reconnects broken db connections to take into account a timeout limit. This ensures
that retries + reconnects are slow-query aware, and that we don't retry queries
if a given amount of time has already passed since the query was first tried.
This value will default to 5 seconds, but can be adjusted via the `connection_retry_timeout`
config.
Even if tests are "skipped" by `Minitest::Assertions#skip`
`ensure` is executed because tests methods are Ruby methods.
Adding conditions outside of the test methods, entire test methods are
executed only when they are necessary.
Before f56b4187be this constant was being
loaded in the railtie, but it was removed since we didn't needed it
anymore.
It causes some application to break if they try to reference the
subscriber before `ActiveRecord::Base` is loaded.
This PR calls `ActiveRecord::Base` directly on `establish_connection`
and `connection` rather than delegating. While this doesn't have much
effect right now, I'm working on moving the database tasks away from
their reliance on Base and eventually we'll need to pass a class through
to these adapter tasks. This change prepares these adapter tasks for
that change.
This avoids problems when complex data structures are mutated _after_
being handed to ActiveRecord for processing. For example false hits in
the query cache.
Fixes#46044
In f36c261a44 support for the ssl-mode option was added for MySQL when
using the dbconsole command and MySQLDatabaseTasks.
However, the mysql2 gem uses `ssl_mode` instead of `sslmode`:
ba4d46551d/lib/mysql2/client.rb (L51)
As `ssl_mode` is what should be used in the database.yml configuration
for MySQL we should use it as well for the dbconsole command and
MySQLDatabaseTasks.
Co-authored-by: Chris Gunther <chris@room118solutions.com>
When I moved `SchemaMigration` and `InternalMetadata` away from inheriting
from `ActiveRecord::Base` I accidentally set the logger tags to `self`
rather than `self.class` which meant they were accidentally logging
the instance rather than the class name. While fixing these I realized I
missed a few tags for other queries and have fixed those as well.
According to the MySQL documentation, database connections default to
ssl-mode=PREFERRED. But PREFERRED doesn't verify the server's identity:
The default setting, --ssl-mode=PREFERRED, produces an encrypted
connection if the other default settings are unchanged. However, to
help prevent sophisticated man-in-the-middle attacks, it is
important for the client to verify the server’s identity. The
settings --ssl-mode=VERIFY_CA and --ssl-mode=VERIFY_IDENTITY are a
better choice than the default setting to help prevent this type of
attack. VERIFY_CA makes the client check that the server’s
certificate is valid. VERIFY_IDENTITY makes the client check that
the server’s certificate is valid, and also makes the client check
that the host name the client is using matches the identity in the
server’s certificate.
https://dev.mysql.com/doc/refman/8.0/en/using-encrypted-connections.html
However both the Rails::DBConsole command and the MySQLDatabaseTasks
ignore the ssl-mode option, making the connection fallback to PREFERRED.
Adding ssl-mode to the forwarded options makes sure the expected mode is
passed to the connection.
In https://github.com/rails/rails/pull/45796 I overlooked that
`ActiveRecord::LogSubscriber#sql` wasn't only logging the SQL query
but was also responsible for collecting the `:db_runtime` metric.
Ultimately I think it is cleaner to move this concern to `RuntimeRegistry`.
Since changing internal metadata to no longer inherit from Base
in #45982 I accidentally changed the behavior when a key's value is the
same. Prior to this change the record would not be updated if the
environment key was found an the value had not changed. So instead of
just checking whether we have an entry here we also need to check if the
value should actually be updated, otherwise we should return the entry.
This clean up wraps the messages at 80 characters and clarifies the
changes needed. 99% of apps won't see this warning as they aren't
accessing MigrationContext directly. This is only for libraries or apps
that are passing a custom `SchemaMigration` or `InternalMetadata` class
so they get a more helpful error.
In addition I moved the `NullSchemaMigration` up to the top of the class
to match InternalMetadata.
Following #45924 sometimes these calls will throw deprecation warnings
depending on the test order and whether there are connections left
behind from other tests. I attempted to prevent leaking connections but
when using fixtures it's pretty hard to avoid because the
`teardown_fixture_connections` will often put connections back in place.
It is eaiser to ensure these calls opt into the new behavior to avoid
deprecation warnings.
Followup to #45908 to match the same behavior as SchemaMigration
Previously, InternalMetadata inherited from ActiveRecord::Base. This is
problematic for multiple databases and resulted in building the code in
AbstractAdapter that was previously there. Rather than hacking around
the fact that InternalMetadata inherits from Base, this PR makes
InternalMetadata an independent object. Then each connection can get it's
own InternalMetadata object. This change required defining the methods
that InternalMetadata was depending on ActiveRecord::Base for (ex
create!). I reimplemented only the methods called by the framework as
this class is no-doc's so it doesn't need to implement anything beyond
that. Now each connection gets it's own InternalMetadata object which
stores the connection.
This change also required adding a NullInternalMetadata class for cases
when we don't have a connection yet but still need to copy migrations
from the MigrationContext. Ultimately I think this is a little weird -
we need to do so much work to pick up a set of files? Maybe something to
explore in the future.
Aside from removing the hack we added back in #36439 this change will
enable my work to stop clobbering and depending directly on
Base.connection in the rake tasks. While working on this I discovered
that we always have a ActiveRecord::InternalMetadata because the
connection is always on Base in the rake tasks. This will free us up
to do less hacky stuff in the migrations and tasks.
Both schema migration and internal metadata are blockers to removing
`Base.connection` and `Base.establish_connection` from rake tasks, work
that is required to drop the reliance on `Base.connection` which will
enable more robust (and correct) sharding behavior in Rails..
Previously, only bold text was supported via a positional argument.
This allows for bold, italic, and underline options to be specified
for colored logs.
```ruby
info color("Hello world!", :red, bold: true, underline: true)
```
The issue fixed by the commit that introduced that entry only existed
in the main branch, so it isn't really a released change worthy of a
CHANGELOG entry.
Previously, SchemaMigration inherited from ActiveRecord::Base. This is
problematic for multiple databases and resulted in building the code in
AbstractAdapter that was previously there. Rather than hacking around
the fact that SchemaMigration inherits from Base, this PR makes
SchemaMigration an independent object. Then each connection can get it's
own SchemaMigration object. This change required defining the methods
that SchemaMigration was depending on ActiveRecord::Base for (ex
create!). I reimplemented only the methods called by the framework as
this class is no-doc's so it doesn't need to implement anything beyond
that. Now each connection gets it's own SchemaMigration object which
stores the connection. I also decided to update the method names (create
-> create_version, delete_by -> delete_version, delete_all ->
delete_all_versions) to be more explicit.
This change also required adding a NullSchemaMigraiton class for cases
when we don't have a connection yet but still need to copy migrations
from the MigrationContext. Ultimately I think this is a little weird -
we need to do so much work to pick up a set of files? Maybe something to
explore in the future.
Aside from removing the hack we added back in #36439 this change will
enable my work to stop clobbering and depending directly on
Base.connection in the rake tasks. While working on this I discovered
that we always have a `ActiveRecord::SchemaMigration` because the
connection is always on `Base` in the rake tasks. This will free us up
to do less hacky stuff in the migrations and tasks.
This is a partial fix for #45906 to allocate less objects when iterating
through pool configs. My original attempt to fix this wasn't feasible
due to how the query cache works. The idea to use a dedicated `each_*`
came from jonathanhefner and I incorporated it here.
Using the following script:
```
Benchmark.memory do |x|
x.report("all") do
ActiveRecord::Base.connection_handler.all_connection_pools.each { }
end
x.report("each") do
ActiveRecord::Base.connection_handler.each_connection_pool { }
end
x.compare!
end
```
We can see that less objects are allocated with the each method. This is
even more important now that we are calling `all_connection_pools` in
more placed since #45924 and #45961.
```
Calculating -------------------------------------
all 408.000 memsize ( 0.000 retained)
7.000 objects ( 0.000 retained)
0.000 strings ( 0.000 retained)
each 328.000 memsize ( 0.000 retained)
3.000 objects ( 0.000 retained)
0.000 strings ( 0.000 retained)
Comparison:
each: 328 allocated
all: 408 allocated - 1.24x more
```
Note that if this is run again on this branch `all_connection_pools`
will have even more allocations due to the deprecation warning that was
introduced. However removing that shows similar numbers to the benchmark
here.
Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>
This PR enables `Minitest/AssertRaisesWithRegexpArgument` cop
and it suppresses the new warning below.
```console
% bundle exec rubocop
(snip)
Offenses:
activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb:111:9: C: Minitest/AssertRaisesWithRegexpArgument:
Do not pass regular expression literals to assert_raises. Test the resulting exception.
assert_raises(ActiveRecord::StatementInvalid, /TypeError/) do
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb:628:9: C: Minitest/AssertRaisesWithRegexpArgument:
Do not pass regular expression literals to assert_raises. Test the resulting exception.
assert_raises(ActiveRecord::StatementInvalid, /SQLite3::ReadOnlyException/) do
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
railties/test/application/rake/multi_dbs_test.rb:307:13: C: Minitest/AssertRaisesWithRegexpArgument:
Do not pass regular expression literals to assert_raises. Test the resulting exception.
assert_raises RuntimeError, /You're using a multiple database application/ do
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
railties/test/application/rake/multi_dbs_test.rb:311:13: C: Minitest/AssertRaisesWithRegexpArgument:
Do not pass regular expression literals to assert_raises. Test the resulting exception.
assert_raises RuntimeError, /You're using a multiple database application/ do
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
railties/test/application/rake/multi_dbs_test.rb:336:13: C: Minitest/AssertRaisesWithRegexpArgument:
Do not pass regular expression literals to assert_raises. Test the resulting exception.
assert_raises RuntimeError, /You're using a multiple database application/ do
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
railties/test/application/rake/multi_dbs_test.rb:361:13: C: Minitest/AssertRaisesWithRegexpArgument:
Do not pass regular expression literals to assert_raises. Test the resulting exception.
assert_raises RuntimeError, /You're using a multiple database application/ do
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```
The last argument of `assert_raises` is a custom message string to help explain failures.
So, it's not the argument that `exception.message` is compared to.
`assert_raises` returns a raised exception and can be used to match against a regular expression.
And it updates the dependency version of rubocop-minitest in the Gemfile.
Because `Minitest/AssertRaisesWithRegexpArgument` cop was introduced in minitest 0.22.
https://github.com/rubocop/rubocop-minitest/releases/tag/v0.22.0
Following on #45924 I realized that `all_connection_pools` and
`connection_pool_list` don't make much sense as separate methods and
should follow the same deprecation as the other methods on the handler
here. So this PR deprecates `all_connection_pools` in favor of
`connection_pool_list` with an explicit argument of the role or `:all`.
Passing `nil` will throw a deprecation warning to get applications to
be explicit about behavior they expect.
Previously when I implemented multiple database roles in Rails there
were two handlers so it made sense for the methods
`active_connections?`, `clear_active_connections!`,
`clear_reloadable_connections!`, `clear_all_connections!`, and
`flush_idle_connections!` to only operate on the current (or passed)
role and not all pools regardless of role. When I removed this and moved
all the pools to the handler maintained by a pool manager, I left these
methods as-is to preserve the original behavior.
This made sense because I thought these methods were only called by
applications and not called by Rails. I realized yesterday that some of
these methods (`flush_idle_connections!`, `clear_active_connections!`,
and `clear_reloadable_connections!` are all called on boot by the
Active Record railtie.
Unfortunately this means that applications using multiple databases
aren't getting connections flushed or cleared on boot for any connection
but the writing ones.
The change here continues existing behavior if a role like reading is
passed in directly. Otherwise if the role is `nil` (which is the new
default` we fall back to all connections and issue a deprecation
warning. This will be the new default behavior in the future. In order
to easily allow turning off the deprecation warning I've added an `:all`
argument that will use all pools but no warning. The deprecation warning
will only fire if there is more than one role in the pool manager,
otherwise we assume prior behavior.
This bug would have only affected applications with more than one role
and only when these methods are called outside the context of a
`connected_to` block. These methods no longer consider the set
`current_role` and applications need to be explicit if they don't want
these methods to operate on all pools.
This commit makes two performance tweaks to the dispatch_cache in
Arel::Visitors::Visitor.
First, it interns the method name into a symbol rather than a string.
send expects a symbol, so this avoids Ruby needing to intern that string
itself.
Second, it changes the dispatch cache to an identity hash. Classes are
already compared by identity, but Ruby is able to perform the lookup
more quickly if it is explicit on the hash.
Add ability to use hash with columns and aliases inside #select method.
Post
.joins(:comments)
.select(
posts: { id: :post_id, title: :post_title },
comments: { id: :comment_id, body: :comment_body}
)
instead
Post
.joins(:comments)
.select(
"posts.id as post_id, posts.title as post_title,
comments.id as comment_id, comments.body as comment_body"
)
Co-authored-by: Josef Šimánek <193936+simi@users.noreply.github.com>
Co-authored-by: Jean byroot Boussier <19192189+casperisfine@users.noreply.github.com>
During where clause generation we search for scope type for the model.
We can do this with Module#!= instead as long as we grab the final scope type after the loop.
This is a small, but significant performance improvement.
Co-authored-by: John Hawthorn <jhawthorn@github.com>
Updating the documentation for setting `self.ignored_columns` on an ActiveRecord model to append to the existing value instead of overwriting the existing array.
Appending to the existing value, `+=`, is helpful for the scenario when 2 developers set `ignored_columns` in different parts of the file and the second one overwrites the first one.
When source and target classes have a different set of attributes adapts
attributes such that the extra attributes from target are added.
Fixes#41195
Co-authored-by: SampsonCrowley <sampsonsprojects@gmail.com>
Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>
[`composed_of`](https://api.rubyonrails.org/classes/ActiveRecord/Aggregations/ClassMethods.html#method-i-composed_of)
is a feature that is not widely used and its API is somewhat confusing,
especially for beginners. It was even deprecated for a while, but 10
years after its deprecation, it's still here.
The Rails documentation includes these examples including mapping:
```ruby
composed_of :temperature, mapping: %w(reading celsius)
composed_of :balance, class_name: "Money", mapping: %w(balance amount)
composed_of :address, mapping: [ %w(address_street street),
%w(address_city city) ]
```
Hashes are accepted kind-of accidentally for the `mapping` option. Using
a hash, instead of an array or array of arrays makes the documentation
more beginner-friendly.
```ruby
composed_of :temperature, mapping: { reading: :celsius }
composed_of :balance, class_name: "Money", mapping: { balance: :amount }
composed_of :address, mapping: { address_street: :street, address_city:
:city }
```
Before Ruby 1.9, looping through a hash didn't have deterministic order,
and the mapping order is important, as that's the same order Rails uses
when initializing the value object. Since Ruby 1.9, this isn't an issue
anymore, so we can change the documentation to use hashes instead.
This commit changes the documentation for `composed_of`, clarifying that
any key-value format (both a `Hash` and an `Array`) are accepted for the
`mapping` option. It also adds tests to ensure hashes are also accepted.
While working on another PR I noticed that we have 6 different tasks for
loading the schema; 3 for all dbs, 3 for namespaced dbs. They are:
* `db:schema:load`
* `db:schema:load:namespace`
* `db:test:load_schema`
* `db:test:load_schema:namespace`
* `db:test:load` (calls `db:test:load_schema` only)
* `db:test:load:namespace` (calls `db:test:load_schema:namespace` only)
I've removed the last two because they didn't call anything except for
another task and they were already private. I believe the original
`db:test:load` and `db:test:load:namescace` were added to handle whether
to load sql or ruby schema fules, but we no longer need that (it's
handled by the `load_schema` method directly now).
In addition I removed `:load_config, :check_protected_environments` from
the `db:schema:load:namespace` task because `purge` already calls these
so we don't need to do it twice.
I tried to remove `db:test:load_schema:namespace` and call
`db:schema:load:namespace` instead but we can't do that because
`db:test:load_schema:namespace` needs to set the environment to "test",
not use the current environment. `db:schema:load` also can't be used
instead of `db:test:load_schema` because it tries to load both dev and
test schemas, but it's callers want just the test schema.
This PR is similar to #36439 which moved schema migrations to the
connection and migration context. This PR does the same thing for the
internal metadata class.
Previously the metadata class was only accessible on Base which makes it
a little weird when switching connections. This didn't really cause any
issues but I noticed it when working on a PR to change how connections
work in the database rake tasks.
In a multiple database environment it makes sense to access the
`ar_internal_metadata` on the existing connection rather than on
`Base.connection`. I previously didn't implement this because I felt we
wanted only one place to ask for the metadata information but recently
realized that we were still recording metadata on all the databases, so
we might as well record it correctly.
Applications should notice no change in behavior because this is only
accessed through the rake tasks. Additionally behavior between schema
migrations and internal metadata are now more similar. Eventually I'd
like neither of these to inherit from or rely on Base, but we have a
lot more work to do before that's possible.
This adds support for reading binary column default values before the column data is read from the database.
This makes binary columns behave more like other column types with default values
In Ruby trunk date changed to use the gregorian calendar in
https://github.com/ruby/date/pull/73.
This had the result that dates not in the gregorian calendar can't be
compared to times not in the gregorian calendar. Since this test isn't
testing datetime comparison but rather that bc timestamps can be stored
in the database, we can compare with `Time.new(0) - 1.week` rather than
`Date.new(0) - 1.week`. I think this might be a bug in Ruby but I'm not
sure there's a great fix. For now however, let's get this test passing.
I deprecated `legacy_connection_handling` setter in 7.0 but then removed
it without setting it to false by default upgraded apps might have this
set to false in their config and still be getting an error. This
confusing behavior came up with one of our apps at work.
To make it less confusing I've redefined `legacy_connection_handling`
setter and am raising an argument error if called. I didn't redefine the
getter because that shouldn't be in a config, it's not useful in that
context.
2 small improvemennts to the docs for `ActiveRecord::NestedAttributes`.
- Include a link to `ActionView::Helpers::FormHelper::fields_for`, since you would typically use these tools in tandem I think it makes sense to point you to it.
- Show an example of how to write an integration test when using `ActionView::Helpers::FormHelper::fields_for`. You could argue that this example should also go in Action View, but I think it makes more sense here as the "parent" doc for the Nested Attributes feature.
When passing `nil` to `in_order_of`, the invalid SQL query is generated (because `NULL != NULL`):
```ruby
Book.in_order_of(:format, ["hardcover", "ebook", nil]).to_sql
```
```sql
SELECT "books".* FROM "books" WHERE "books"."format" IN ('hardcover', 'ebook', NULL)
ORDER BY CASE "books"."format" WHEN 'hardcover' THEN 1
WHEN 'ebook' THEN 2 WHEN NULL THEN 3 ELSE 4 END ASC
```
This PR also removes the special order field generation (`ORDER BY FIELD`) for MYSQL, because it is
unable to work with `NULL`s and the default `ORDER BY CASE` works for it (tested on MariaDB and MySQL 5.7+).
The various LogSubscriber subclasses tend to subscribe to events
but then end up doing nothing if the log level is high enough.
But even if we end up not logging, we have to go through the
entire notification path, record timing etc.
By allowing subscribers to dynamically bail out early, we can
save a lot of work if all subscribers are silenced.
This commit does two things.
1. Reverts a recent change that was calling SET DEFAULT NULL
when altering a table. This PR revert that back to instead call DROP
DEFAULT in the case when the column is not nullable.
2. Adds a check to ensure that only nonbinary data types are eligible
for having the `collation` carry through when performing a change_column
migration.
A recent change made it so that when you perform a change column
it will preserve collation on the column rather than nil it out·
which would select the database default. This works fine except in
the case of a binary type. In this case the collation should be
set to nil in the options so that the database default collation
type can be used on the column.
We'll ensure that any consumers that require ddl obtain it by visiting
the schema definition. Consequently, we should also stop visiting the
schema definition in the "schema definition builder" methods -- callers
will need to both build a schema definition, and then visit it using a
SchemaCreation object. This means schema definitions will not be populated
with SQL type information, or any other information that is set when
the definition is visited.
Follow-up to #45735.
Prior to this commit, `drop_enum` could not be reversed when called with
the `if_exists: true` option, because `create_enum` could not accept
options.
This commit fixes that and adds test coverage.
This commit also ensures that reversing `drop_enum` will raise an
`ActiveRecord::IrreversibleMigration` error if no enum values are
specified.
Previously `setup_shared_connection_pool` was called anytime
`setup_fixtures` saw a future connection established. Sometimes those
connections are `nil` but we were setting up the shared pools before
determining that.
This change moves the `setup_shared_connection_pool` into the check for
if we have a connection. The tests that were dealing with these settings
were originally set to not use transacitons. But without this we can't
test the change here so I've updated that as well.
This is an alternate fix to #45769. I realized after opening the PR it
wasn't quite right to skip if we don't have a writing pool config,
because the question is how were we ending up with that anyway? Well the
answer was that we were calling the shared setup in the wrong place.
Fixes#45767
and :to options are not being type cast. For example, for an enum
attribute, attribute_changed? should handle a String, Symbol or Integer
for the :from and :to options.
We recently ran into an issue where we used `.unscoped` as part of a
method chain without realising that it would also unset the scoping of
the association. It feels like this would be useful information to
surface in the documentation so that there's a clear source of truth on
how the method behaves.
The PR #44931 fell off my radar a few months ago so I decided to add a
test myself to ensure this behavior change is tested. While doing that I
decided to refactor the changes from that PR to make the code a little
cleaner and easier to read.
* Improve #configured_migrate_path logic
This solves two issues:
* `migrations_paths` is an Array, which here gets coerced to a string.
This is fine if it's a single path, but otherwise the generated path
is a concatenation of all the paths. Instead, pick the first one.
* If the `--database` flag is not passed in, fall back to the primary
database's `migrations_paths` entry if available before falling back
to the global default migrate path.
* migrations_paths can be a String, Array, or nil
Define APIs on the MySQL and PostgreSQL connection adapters for building
ChangeColumnDefaultDefinition objects. These provide information on what a
column default change would look like, when called with the same arguments as
would be passed to #change_column_default.
The `remove_check_constraint` method now accepts an `if_exists` option. If set
to true an error won't be raised if the check constraint doesn't exist.
This commit is a combination of PR #45726 and #45718 with some
additional changes to improve wording, testing, and implementation.
Usage:
```ruby
remove_check_constraint :products, name: "price_check", if_exists: true
```
Fixes#45634
Co-authored-by: Margaret Parsa <mparsa@actbluetech.com>
Co-authored-by: Aditya Bhutani <adi_bhutani16@yahoo.in>
Aligns the SQL produced for changing a column default between MySQL and PostgreSQL.
MySQL is currently producing an <ALTER TABLE x CHANGE COLUMN y> query just to
change the default for a column, which is less efficient than using the SET DEFAULT syntax.
Additionally, extracts ChangeColumnDefaultDefinition and moves the SQL generation to the visitor class.
Using `create_or_find_by` in codepaths where most of the time
the record already exist is wasteful on several accounts.
`create_or_find_by` should be the method to use when most of the
time the record doesn't already exist, not a race condition safe
version of `find_or_create_by`.
To make `find_or_create_by` race-condition free, we can search
the record again if the creation failed because of an unicity
constraint.
Co-Authored-By: Alex Kitchens <alexcameron98@gmail.com>
Ref: https://github.com/rails/rails/pull/41659
Mutating the attributes hash requires to workaround the mutation
in `find_or_create_by` etc.
One extra Hash dup is not big deal.
This previously made no difference, but it now saves us reconnecting to
run a ROLLBACK: because we're still inside a transaction at that moment,
we won't be #restorable?.
There's no point attempting to rollback in this case; we can instead
just invalidate the now-lost transaction.
Similarly, even though we can't immediately reconnect for a connection
failure mid-transaction, we can and should drop and prior verification
of the connection: the next out-of-transaction query attempt needs to
fix it, not assume it's working.
Define APIs on the MySQL and PostgreSQL connection adapters for building
ChangeColumnDefinition objects. These provide information on what a
column change would look like, when called with the same arguments as
would be passed to #change_column.
Exposes AlterTable schema definition for adding new columns through new API,
and stores ddl on the schema definition. Refactors existing #add_column method to use
this as an intermediate method to build the create add_column definition.
Exposes CreateIndex schema definition through new API, and stores ddl
on the schema definition. Refactors existing #add_index methods to use
this as an intermediate method to build the create index definition.
Print default parent class for controller, job, and model generators.
Before:
[--parent=PARENT] # The parent class for the generated job
After:
[--parent=PARENT] # The parent class for the generated job
# Default: ApplicationJob
The block can't affect reconnectability, because we'll be re-running it
anyway.
Also, only #reconnect! once per query, to avoid squaring the total
number of permitted retries while attempting to query a pathologically
flakey server.
Connectivity issues need a reconnect to be retried, but there are also
in-server errors that can also be retried (lock timeouts and deadlocks).
To handle this better, we move translation into #with_raw_connection:
it's a much better representation of the boundary between the underlying
adapter library and Active Record's abstractions anyway. (It doesn't
know about the query being run, though, so we let #log enrich that into
the exception later.)
We may be restoring a connection after #with_raw_connection has already
passed #materialize_transactions, so it's important we eagerly return
things to exactly the state they had before.
So far this is just infrastructure: no errors are considered retryable,
and no real queries are marked retryable.
Even in this state, we can drop connection verification from checkout,
and defer it until the first query is ready. But the more interesting
within-reach win will be when 'BEGIN' is retryable, and we can recognise
connectivity-related exceptions.
Define API on connection adapter for building a TableDefinition.
The provided TableDefinition object offers insight into what a table
created using the migration method #create_table would look like, including
information on primary keys, column types, etc.
As a part of this PR, we extract TableDefinition#set_primary_key to handle
some of the primary key setting logic currently being performed in #create_table.
The `_dump` task is private so it shouldn't be called by apps. Because
of this were establishing a connection to the db_config twice, once in
`_dump` and once in `dump`. There's no reason to do this since `_dump`
always calls `dump` and apps should not be calling `_dump`.
In Postgres there was an issue when calling `ActiveRecord::ConnectionAdapters::PostgreSQL::ReferentialIntegrity#all_foreign_keys_valid?` if there were multiple schemas with foreign key constraints, because the ALTER TABLE statement did not include the schema.
This commit adds the necessary schema prefix
```
ALTER TABLE [schema].[table_name] VALIDATE CONSTRAINT [constraint]
^^^^^^
```
In #45450 we stopped removing connections when establish_connection was
called and an identical connection existed in the pool. Unfortunately
this broke granular connection swapping for the primary_abstract_class
because the class stored in the pool was incorrect.
What was happening was connection for the writing role were getting
stored with the class ActiveRecord::Base and connections for the reading
role were getting ApplicationRecord (or whatever the app set as their
`primary_abstract_class`. If we made sure that the reading connections
also got ActiveRecord::Base that would break granular swapping for both
writing and reading because Active Record doesn't know whether you
wanted global swapping or granular swapping for prevent writes. This bug
only manifests on prevent writes because it's the only value we actually
lookup on the connection pool (because at the point we need it we don't
have access to self).
To fix this I've decided to update the class on the existing pool if it
doesn't match the owner_name passed. This is kind of gross I admit, but
it's safe because the way we find connections is on the
`connection_name`. The class is only stored so that the connection can
find it's `current_preventing_writes` value. This also required me to
move the ivar to an instance method for connection_class on the pool
because we don't want to cache the value and want to make sure we're
getting it from the pool config directly.
In an effort to find all the internal callers of `connection_handler` I
decided to remove any uses that are either:
1) Unnecessary - we can call what we need on Base or elsewhere without
going through the handler
2) Incorrect - there's a better way call the same thing. For exmaple
avoiding calling the ivar, and instead calling `send` on an existing
method.
Doing this will inform us what cases the connection handler is required
and how we can expand Base to provide everything an application or gem
could need without going through the handler. Reducing calls makes it
easier to replace or refactor in the future.
Fix: https://github.com/rails/rails/issues/45585
There's no benefit in serializing it as HWIA, it requires
to allow that type for YAML safe_load and takes more space.
We can cast it back to a regular hash before serialization.
Bart de Water