Eventually we'd like to get rid of this class altogether but for now
this PR reduces the surface area by removing methods from the class and
moving classes out into their own files.
* `adapter_method` was moved into database configurations
* `initialize_dup` was removed because it was only used in tests
* Resolver is now it's own class under connection adapters
* ConnectionUrlResolver, only used by the configurations, is in a class
under DatabaseConfigurations
Co-authored-by: John Crepezzi <john.crepezzi@gmail.com>
This is part 1 of removing the need to pass hashes around inside
connection management. This PR creates database config objects every
time when passing a hash, string, or symbol and sends that object to
connection specification. ConnectionSpecification reaches into the
config hash on db_config when needed, but eventually we'll get rid of
that and ConnectionSpecification since it's doing duplicate work with
the DatabaseConfigurations.
We also chose to change the keys to strings because that's what the
database.yml would create and what apps currently expect. While symbols
are nicer, we'd end up having to deprecate the string behavior first.
Co-authored-by: eileencodes <eileencodes@gmail.com>
Fixes a bug where has_one through relations with non-integer primary
keys were incorrectly raising an implementation error even though a
reflectable association was present.
Previously in some places we used symbol keys, and in some places we used
string keys. That made it pretty confusing to figure out in a particular
place what type of configuration object you were working with.
Now internally, all configuration hashes are keyed by symbols and
converted to such on the way in.
A few exceptions:
- `DatabaseConfigurations#to_h` still returns strings for backward compatibility
- Same for `legacy_hash`
- `default_hash` previously could return strings, but the associated
comment mentions it returns symbol-key `Hash` and now it always does
Because this is a change in behavior, a few method renames have happened:
- `DatabaseConfig#config` is now `DatabaseConfig#configuration_hash` and returns a symbol-key `Hash`
- `ConnectionSpecification#config` is now `ConnectionSpecification#underlying_configuration_hash` and returns the `Hash` of the underlying `DatabaseConfig`
- `DatabaseConfig#config` was added back, returns `String`-keys for backward compatibility, and is deprecated in favor of the new `configuration_hash`
Co-authored-by: eileencodes <eileencodes@gmail.com>
When this case was his previously, an error would be thrown like:
`<NoMethodError: "undefined method config for...">` because we were
erroneously calling `config` on a `Hash`.
This commit adds a test for this case, and fixes the hash access.
These tests are causing the Ruby master build to fail in CI. The Docker
image we use is based on Ubuntu Bionic which provides SQLite 3.22.0, and
the features required to use `insert_all` and `upsert_all` are not
available in that version.
Current Ruby 2.7 implementation behaves differently from 2.6 (by design)
where receiving an empty keyword arguments.
Thus, for now we need this if branch everywhere we're asserting such a method call
in order for the CI to pass.
We could revert this workaround if Ruby 2.7 decides to revert back to the 2.6 behavior,
or maybe we'll need to fix Minitest::Mock once the 2.7 spec became stable.
Before this patch, column names could only be passed as a positional
argument when no other options were supplied:
remove_index :reports, :report_id
Passing column names positionally along with other options would fail:
remove_index :reports, :report_id, unique: true
# => ArgumentError: wrong number of arguments (given 3, expected 1..2)
In 3809c80cd55ac2838f050346800889b6f8e041ef, adding an index with a
name that's already in use was changed from an error to a warning, to
allow other statements in the same migration to complete successfully.
In 55d0d57bfc72c0bdbc81ae5d95c99729f16899af this decision was reversed,
but instead of allowing the statement to execute and raise an adapter-
specific error as it did before, an `ArgumentError` was raised instead.
This interferes with a legitimate use case: on MySQL, it's possible to
drop an index and add another one with the same name in a single `ALTER`
statement. Right now an `ArgumentError` is raised when trying to do so,
even though the resulting statement would execute successfully.
There's no corresponding `ArgumentError` raised when attempting to add a
duplicate column, so I think we can safely remove the check and allow
the adapter to raise its own error about duplicate indexes again.
The `InsertAll` class currently calls `exec_query`, which doesn't give
the query cache enough information to know that it needs to be cleared.
By adding an `exec_insert_all` method that calls `exec_query` internally
we can configure the query cache to clear when that method is called.
Since 32b03b46150b0161eba2321ccac7678511e3d58e, records which have had
`inspect` called on them can't be marshaled, because of this anonymous
`DelegateClass`. We can fix this by giving the class a concrete name.
The methods with tests added here is for the query methods that uses
`check_if_method_has_arguments!` in `ActiveRecord::QueryMethods`.
However, some of these query methods had no tests.
- Ensure that explicit method call `connected_to` with `prevent_writes: false`
turns off 'preventing writes' in the passed block.
- Ensure that after explicit call method call `connected_to` with `prevent_writes: false`
'preventing writes' is retained
Related to https://github.com/rails/rails/pull/37065
If a user is using the middleware for swapping database connections and
manually calling `connected_to` in a controller/model/etc without
calling `while_preventing_writes(false)` there is potential for a race
condition where writes will be blocked.
While the user could _just_ call `while_preventing_writes` in the same
place they call `connected_to` this would mean that all cases need to
call two methods.
This PR changes `connected_to` to call `while_preventing_writes`
directly. By default we'll assume you don't want to prevent writes, but
if called with `connected_to(role: :writing, prevent_writes: true)` or
from the middleware (which calls `connected_to` this way) the writes
will be blocked.
For replicas, apps should use readonly users to enforce not writing
rather than `while_preventing_writes` directly.
Should fix the remaining issues in
https://github.com/rails/rails/issues/36830
After forking, the connection handler will discard any connection pools that belongs to the parent process. However, it will continue to hold a reference to the connection pools of the parent process which is used for retrieval of the connection pool in the child process. Therefore, the `weakref_alive?` check in the connection pool reaper is insufficient since the connection pools of the parent process is never GCed.
This broke in 3e2e8ee where it switched to one reaper thread per
process. However, the implementation will only spawn the reaping thread
if the reaping frequency has not been seen. Since the reaping
frequencies are stored in a class instance variable, that variable is
copied when the process is forked. As such, a forked process will not
spawn a new reaping thread since the class instance variable would have
contain the reaping frequency that was seen in the parent process.
This commit tracks threads separately and checks that they both have
been spawned and are currently alive.
This adds a failing test to reaper_test.rb, based on the previous test
without forking. It also improves the test to return an failure instead
of hanging forever if the reaper is not started.
Co-authored-by: Guo Xiang Tan <tgx_world@hotmail.com>
This fixes a race condition in system tests where prepared
statements can be incorrectly parameterized when multiple
threads observe the mutation of the @prepared_statements
instance variable on the connection.
Fixes#36763
Since 2f8b397258b66581409b0e6537f98ea9b56e9f19, `ActiveRecord::Base` and
`ApplicationRecord` use the same default `connection_specification_name`
so that database connections configured on `ApplicationRecord` also
apply to `ActiveRecord::Base`.
However, when resolving the connection for `ApplicationRecord` we should
continue to fall back to `ActiveRecord::Base` when there's no connection
explicitly configured, so that setting `connection_specification_name`
on `ActiveRecord::Base` affects `ApplicationRecord` and its descendants
in this case as it did in previous versions.
Actually, `first` taking non-deterministic result is considered a bug to
me. But changing the behavior should not be happened in rc2.
I've restored the behavior for 6.0, and then will deprecate that in 6.1.
Fixes#36802.
c9e4c848 has one performance optimization for `aggregate_alias` to early
returning by `aggregate_alias.match?(/\A\w+\z/)`, but it is caused a
regression that failing deduplication for non word chars #36867.
I've quited the optimization and add a test to prevent a future
regression.
Fixes#36867.
As demonstrated in the test added and in #36830 the code that prevents
writes wasn't thread safe. If one thread does a read, then another does
a write, and then another does a read the second read will cause the
first write to be unwriteable.
This change removes the instance variable and instead uses a
getter/setter on Thread.current[:prevent_writes] for the connection
handler to set whether writes are allowed.
Fixes#36830
```ruby
$ cd activerecord
$ bin/test test/cases/dirty_test.rb:494
... snip ...
DEPRECATED: Use assert_nil if expecting nil from /home/yahonda/git/rails/activerecord/test/cases/dirty_test.rb:494. This will fail in Minitest 6.
DEPRECATED: Use assert_nil if expecting nil from /home/yahonda/git/rails/activerecord/test/cases/dirty_test.rb:511. This will fail in Minitest 6.
.
Finished in 0.061593s, 16.2356 runs/s, 795.5428 assertions/s.
1 runs, 49 assertions, 0 failures, 0 errors, 0 skips
$
```
Refer seattlerb/minitest#666rails/rails#27712
