| Commit message (Collapse) | Author | Age | Files | Lines |
... | |
| | | | | |
|
|/ / / /
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | | |
Currently, almost all "Dangerous query method" warnings are false alarm.
As long as almost all the warnings are false alarm, developers think
"Let's ignore the warnings by using `Arel.sql()`, it actually is false
alarm in practice.", so I think we should effort to reduce false alarm
in order to make the warnings valuable.
This allows column name with function (e.g. `length(title)`) as safe SQL
string, which is very common false alarm pattern, even in the our
codebase.
Related 6c82b6c99, 6607ecb2a, #36420.
Fixes #32995.
|
| | | | |
|
| | | |
| | | |
| | | |
| | | |
| | | | |
`split(/\s*,\s*/)` to order args and then `permit.match?` one by one is
much slower than `permit.match?` once.
|
| | | |
| | | |
| | | |
| | | | |
Method added in https://github.com/rails/rails/pull/36416
|
|\ \ \ \
| | | | |
| | | | | |
Fix preloading on AR::Relation where records are duplicated by a join
|
| | | | | |
|
|\ \ \ \ \
| | | | | |
| | | | | | |
Allow quoted identifier string as safe SQL string
|
| | | | | |
| | | | | |
| | | | | |
| | | | | |
| | | | | |
| | | | | |
| | | | | |
| | | | | |
| | | | | |
| | | | | |
| | | | | |
| | | | | |
| | | | | | |
Currently `posts.title` is regarded as a safe SQL string, but
`"posts"."title"` (it is a result of `quote_table_name("posts.title")`)
is regarded as an unsafe SQL string even though a result of
`quote_table_name` should obviously be regarded as a safe SQL string,
since the column name matcher doesn't respect quotation, it is a little
annoying.
This changes the column name matcher to allow quoted identifiers as safe
SQL string, now all results of the `quote_table_name` are regarded as
safe SQL string.
|
|/ / / / /
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | |
| | | | | |
This PR proposes moving the schema cache from the connection to the pool
so the connection can ask the pool for the cache. In a future PR our
goal is to be able to read the yaml file from the pool so we can get
rid of the `active_record.check_schema_cache_dump` initializer. This
will fix the issues surrounding dumping the schema cache and mulitple
databases.
Why do we want to get rid of the initializer you ask?
Well I was looking at #34449 and trying to make it work for our usecase
and it revealed A LOT of problems. There are a few issues that I will
fix in remaining PRs with SchemaMigration, but there's a big glaring
issue with this initializer.
When you have an application with multiple databases we'll need to loop
through all the configurations and set the schema cache on those
connections. The problem is on initialization we only have one
connection - the one for Ar::Base. This is fine in a single db
application but not fine in multi-db. If we follow the pattern in #34449
and establish a connection to those other dbs we will end up setting the
cache on the _connection object_ rather than on all connections that
connect for that config.
So even though we looped through the configs and assigned the cache the
cache will not be set (or will be set wrong) once the app is booted
because the connection objects after boot are _different_ than the
connection objects we assigned the cache to.
After trying many different ways to set the schema cache `@tenderlove`
and I came to the conclusion that the initializer is problematic, as is
setting the schema cache twice.
This is part 1 to move the cache to the pool so the cache can read from
the schema cache yaml file instead of setting it when initializing the
app.
To do this we have created a `NullPool` that initializes an empty cache. I
put the `get_schema_cache` and `set_schema_cache` in an `AbstractPool`
so we can share code between `ConnectionPool` and `NullPool` instead of
duplicating code.
Now we only need to set the schema_cache on the pool rather than the
connection. In `discard!` we need to unset the connection from the
schema_cache - we still want the cache just not the connection.
|
|\ \ \ \ \
| | | | | |
| | | | | | |
Fixed db:prepare task for multiple databases.
|
| | |_|_|/
| |/| | |
| | | | |
| | | | |
| | | | | |
When one database existed already, but not the other,
during setup of missing one, existing database was wiped out.
|
|\ \ \ \ \
| |/ / / /
|/| | | | |
Treat ActiveRecord::Base and ApplicationRecord as "primary"
|
| |/ / /
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | | |
When someone has a multi-db application their `ApplicationRecord` will
look like:
```ruby
class ApplicationRecord < ActiveRecord::Base
self.abstract_class = true
connects_to database: { writing: :primary, reading: :replica }
end
```
This will cause us to open 2 connections to ActiveRecord::Base's
database when we actually only want 1. This is because Rails sees
`ApplicationRecord` and thinks it's a new connection, not the existing
`ActiveRecord::Base` connection because the
`connection_specification_name` is different.
This PR changes `ApplicationRecord` classes to consider themselves the
same as the "primary" connection.
Fixes #36382
|
|/ / /
| | |
| | |
| | |
| | |
| | | |
If an sqlite3 table contains a decimal column behind columns with a collation
definition, then parsing the collation of all preceeding columns will fail --
the collation will be missed without notice.
|
|\ \ \
| |/ /
|/| |
| | |
| | | |
guigs/fix-invalid-schema-when-pk-column-has-comment
Fix invalid schema dump when primary key column has a comment
|
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | | |
Before this fix it would either generate an invalid schema, passing `comment` option twice to `create_table`, or it move the comment from primary key column to the table if table had no comment when the dump was generated.
The situation now is that a comment on primary key will be ignored (not present on schema).
Fixes #29966
|
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | | |
`create_table` and `t.column` have the same named options (e.g.
`:comment`, `:primary_key`), so it should be separated table options
from column options.
Related #36373.
|
|/ /
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
Each `save` calls `all_timestamp_attributes_in_model` to fill timestamp
columns. Allthough the `all_timestamp_attributes_in_model` returns the
same value every time, the `all_timestamp_attributes_in_model` makes
extra 5 arrays every time.
This avoids the making extra 5 arrays by memoizing the result, it makes
`save` economical and a bit faster.
https://gist.github.com/kamipo/1ddad2235073f508637bf9a72d64bb83
Before (2a015f6c0be0593a624b0c800e5335319ac4c660):
```
{["~/rails/activerecord/lib/active_record/timestamp.rb",
76,
:T_ARRAY]=>[1000, 0, 341, 0, 1, 13640],
["~/rails/activerecord/lib/active_record/timestamp.rb",
64,
:T_ARRAY]=>[1000, 0, 341, 0, 1, 13640],
["~/rails/activerecord/lib/active_record/timestamp.rb",
80,
:T_ARRAY]=>[1000, 0, 341, 0, 1, 13640],
["~/rails/activerecord/lib/active_record/timestamp.rb",
68,
:T_ARRAY]=>[1000, 0, 341, 0, 1, 13640],
["~/rails/activerecord/lib/active_record/timestamp.rb",
73,
:T_ARRAY]=>[1000, 0, 341, 0, 1, 13640]}
Warming up --------------------------------------
User.create * 10 36.000 i/100ms
Calculating -------------------------------------
User.create * 10 353.644 (± 7.4%) i/s - 1.764k in 5.021876s
```
After (this change):
```
{["~/rails/activerecord/lib/active_record/timestamp.rb",
83,
:T_ARRAY]=>[1, 0, 1, 1, 1, 40],
["~/rails/activerecord/lib/active_record/timestamp.rb",
87,
:T_ARRAY]=>[1, 0, 1, 1, 1, 40],
["~/rails/activerecord/lib/active_record/timestamp.rb",
64,
:T_ARRAY]=>[1, 1, 1, 1, 1, 0],
["~/rails/activerecord/lib/active_record/timestamp.rb",
69,
:T_ARRAY]=>[1, 1, 1, 1, 1, 0],
["~/rails/activerecord/lib/active_record/timestamp.rb",
74,
:T_ARRAY]=>[1, 1, 1, 1, 1, 0]}
Warming up --------------------------------------
User.create * 10 37.000 i/100ms
Calculating -------------------------------------
User.create * 10 380.063 (± 7.1%) i/s - 1.924k in 5.097917s
```
|
| | |
|
| |
| |
| |
| |
| | |
`cache_version`
`ActiveRecord::Base.cache_versioning` it `true` by default since Rails 5.2 as stated correctly in the documentation for the `ActiveRecord::Base.cache_versioning` class attribute. Remove the wrong and duplicated documentation of the default value for `cache_versioning` from `cache_version`.
|
| | |
|
|\ \
| | |
| | | |
Allow symbol (i.e. quoted identifier) as safe SQL string
|
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | | |
`pluck(:id)` / `order(:id)` are very common use case, and passed symbol
(i.e. quoted identifier) is obviously safe argument, but
`:id.to_s.split(/\s*,\s*/).all? { |part| permit.match?(part) }` is
useless and a bit expensive operation for each such safe symbols (will
make extra 2 mutable strings, 1 array, 1 proc).
This avoids the expensive operation to such safe symbols, it makes
`pluck(:id)` / `order(:id)` itself about 9% faster.
https://gist.github.com/kamipo/11d428b57f3629a72ae89c6f21721326
Before (93e640735e9363672b770b8d1c5a35f9e464f806):
```
Warming up --------------------------------------
users.pluck(:id) 1.217k i/100ms
users.order(:id).to_sql 1.848k i/100ms
Calculating -------------------------------------
users.pluck(:id) 12.239k (± 8.2%) i/s - 60.850k in 5.013839s
users.order(:id).to_sql 19.111k (± 7.5%) i/s - 96.096k in 5.064450s
```
After (this change):
```
Warming up --------------------------------------
users.pluck(:id) 1.293k i/100ms
users.order(:id).to_sql 2.036k i/100ms
Calculating -------------------------------------
users.pluck(:id) 13.257k (± 6.9%) i/s - 65.943k in 5.002568s
users.order(:id).to_sql 20.957k (± 7.6%) i/s - 105.872k in 5.086102s
```
|
|/ /
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
Prior to 3e2e8eeb9ea552bd4782538cf9348455f3d0e14a the Reaper thread
would hold a reference to connection pools indefinitely, preventing the
connection pool from being garbage collected, and also leaking the
Thread.
Since 3e2e8eeb9ea552bd4782538cf9348455f3d0e14a, there is only one Reaper
Thread for all pools, however all pools are still stored in a class
variable, preventing them from being garbage collected.
This commit instead holds reference to the pools through a WeakRef. This
way, connection pools referenced elsewhere will be reaped, any others
will be able to be garbage collected.
I don't love resorting to WeakRef to solve this, but I believe it's the
simplest way to accomplish the the desired behaviour.
|
| | |
|
|\ \
| | |
| | | |
[CI skip] Put :nodoc: on method that raises NoMethodError
|
| | |
| | |
| | |
| | |
| | | |
This method is not intended to be used so I think we should
remove it from the docs.
|
| | |
| | |
| | |
| | |
| | | |
Almost all database statements methods except `explain` was moved into
`DatabaseStatements` at #35922. This moves the last one method.
|
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | | |
Unfortunately, a11a8ff had no effect as long as using bind param, and
was not tested.
This ensures making the intent of a11a8ff, which fall back to type
casting from the connection adapter.
Fixes #35205.
```
% ARCONN=postgresql bundle exec ruby -w -Itest test/cases/relation/where_test.rb -n test_type_casting_nested_joins
Using postgresql
Run options: -n test_type_casting_nested_joins --seed 55730
# Running:
E
Error:
ActiveRecord::WhereTest#test_type_casting_nested_joins:
ActiveRecord::StatementInvalid: PG::InvalidTextRepresentation: ERROR: invalid input syntax for integer: "2-foo"
rails test test/cases/relation/where_test.rb:30
Finished in 0.245778s, 4.0687 runs/s, 0.0000 assertions/s.
1 runs, 0 assertions, 0 failures, 1 errors, 0 skips
```
|
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | | |
#36293 was an issue for through association with `joins` for a long
time, but since #35864 through association with `left_joins` would also
be affected by the issue.
Implicit through table joins should be appeared before user supplied
joins, otherwise loading through association with joins will cause a
statement invalid error.
Fixes #36293.
```
% ARCONN=postgresql bundle exec ruby -w -Itest test/cases/associations/has_many_through_associations_test
.rb -n test_through_association_with_joins
Using postgresql
Run options: -n test_through_association_with_joins --seed 7116
# Running:
E
Error:
HasManyThroughAssociationsTest#test_through_association_with_joins:
ActiveRecord::StatementInvalid: PG::UndefinedTable: ERROR: missing FROM-clause entry for table "posts"
LINE 1: ... "comments_posts" ON "comments_posts"."post_id" = "posts"."i...
^
: SELECT "comments".* FROM "comments" INNER JOIN "comments" "comments_posts" ON "comments_posts"."post_id" = "posts"."id" INNER JOIN "posts" ON "comments"."post_id" = "posts"."id" WHERE "posts"."author_id" = $1
rails test test/cases/associations/has_many_through_associations_test.rb:61
Finished in 0.388657s, 2.5730 runs/s, 0.0000 assertions/s.
1 runs, 0 assertions, 0 failures, 1 errors, 0 skips
```
|
|\ \ \
| | | |
| | | | |
Use a single thread for all ConnectionPool Reapers
|
| | |/
| |/|
| | |
| | |
| | | |
Previously we would spawn one thread per connection pool, which ends up
being wasteful for apps with several connection pools.
|
|\ \ \
| | | |
| | | | |
Fix eager loading associations with string joins not to raise NoMethodError
|
| | | |
| | | |
| | | |
| | | | |
Fixes #34456.
|
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | | |
This partly reverts the effect of d1107f4d.
d1107f4d makes `touch` tracks the mutation whether the `touch` is
occurred by explicit or not.
Existing apps expects that the previous changes tracks only the changes
which is explicit action by users.
I'd revert the implicit `touch` mutation tracking since I'd not like to
break existing apps.
Fixes #36219.
|
|\ \ \ \
| | | | |
| | | | | |
Remove SQLite version support caveats [ci skip]
|
| | |/ /
| |/| |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | |
| | | | |
Since d1a74c1e012ed96f7179e53b9190b7da0a369e11, Active Record requires
SQLite version 3.8.0 or greater, so savepoints and partial indexes are
always available.
That commit also added a runtime version check, so we can remove the
minimum version requirement from the internal adapter documentation.
|
|\ \ \ \
| |/ / /
|/| | | |
document update_counters on relation [ci skip]
|
| | | | |
|
| |/ /
|/| |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | | |
This commit adds "TRANSACTION" to savepoint and commit, rollback statements
because none of savepoint statements were removed by #36153 since they are not "SCHEMA" statements.
Although, only savepoint statements can be labeled as "TRANSACTION"
I think all of transaction related method should add this label.
Follow up #36153
|
| | |
| | |
| | |
| | |
| | | |
The initializer receives `nil` for these options when no cofigurations were given:
https://github.com/rails/rails/blob/v6.0.0.rc1/activerecord/lib/active_record/railtie.rb#L91-L97
|
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | | |
transaction
Currently, `committed!`/`rolledback!` will only be attempted for the
first enrolled record in the transaction, that will cause some
problematic behaviors.
The first one problem, `clear_transaction_record_state` won't be called
even if the transaction is finalized except the first enrolled record.
This means that de-duplicated records in the transaction won't refer
latest state (e.g. won't happen rolling back record state).
The second one problem, the enrolled order is not always the same as the
order in which the actions actually happened, the first enrolled record
may succeed no actions (e.g. `destroy` has already succeeded on another
record during `before_destroy`), it will lose to fire any transactional
callbacks.
To avoid both problems, we should attempt `committed!`/`rolledback!` to
all enrolled records in the transaction.
|
| | | |
|
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | | |
* Make scope arity check consistent
* Add test for arity change
[Rob Trame + Rafael Mendonça França]
|
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | | |
If the same id's records are saved and/or destroyed in the transaction,
commit callbackes will only run for the first enrolled record.
https://github.com/rails/rails/blob/a023e2180093ebc517a642aaf21f3c7241c67657/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb#L115-L119
The regression #36132 is caused due to #35920 changed the enrollment
order that the first action's record will be enrolled to last in the
transaction.
We could not change the the enrollment order as long as someone depends
on the enrollment order.
Fixes #36132.
|
| | | |
|
| | | |
|
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | |
| | | |
This fixes a regression for #35864.
Usually, stashed joins (mainly eager loading) are performed as LEFT
JOINs.
But the case of merging joins/left_joins of different class, that
(stashed) joins are performed as the same `join_type` as the parent
context for now.
Since #35864, both (joins/left_joins) stashed joins might be contained
in `joins_values`, so each stashed joins should maintain its own
`join_type` context.
Fixes #36103.
|
|\ \ \
| | | |
| | | | |
Model error as object
|