Previously, it was very easy to accidentally leak a database password in
production logs if an error ends up calling inspect on a ConnectionPool
or an individual connection (Adapter). This is due to the default
`#inspect` output for Pools and Adapters being unnecessarily large, and
both currently including passwords (through the DatabaseConfig of a
Pool, and the internal configuration of an Adapter).
This commit addresses these issues by defining a custom `#inspect` for
ConnectionPool, AbstractAdapter, and DatabaseConfig. The condensed
`#inspect` only includes a few valuable fields instead of all of the
internals, which prevents both the large output and passwords from being
included.
Currently, there is no (simple) way to ask a model if it connects to a
single database or to multiple shards. Furthermore, without looping
through a model's connections, I don't believe there's an easy way to
return a list of shards a model can connect to.
This commit adds a `@shard_keys` ivar that's set whenever `.connects_to`
is called. It sets the ivar to the result of `shards.keys`. `shards` in
`.connects_to` defaults to an empty hash and therefore when calling
`connects_to database: {...}` `@shard_keys` will be set to an empty array.
`@shard_keys` is set _before_ the following lines:
```
if shards.empty?
shards[:default] = database
end
```
This conditional sets the one and only shard (`:default`) to the value of `database`
that we pass to `.connects_to`. This allows for calling
`connected_to(shard: :default)` on models configured to only connect to
a database e.g.:
```ruby
class UnshardedBase < ActiveRecord::Base
self.abstract_class = true
connects_to database: { writing: :primary }
end
class UnshardedModel < UnshardedBase
end
UnshardedBase.connected_to(shard: :default) {
UnshardedBase.connection_pool.db_config.name } => primary
```
This is ultimately still an _unsharded_ model which is why `@shard_keys`
gets set before the conditional.
With the new `@shard_keys` ivar we need a way for descendants of the
abstract AR model to return that same value. For that we leverage the
existing `.connection_class_for_self` method. That method returns the
ancestor of the model where `.connects_to` was called, or returns self if
it's the connection class:
```ruby
class UnshardedBase < ActiveRecord::Base
self.abstract_class = true
connects_to database: { writing: :primary }
end
class UnshardedModel < UnshardedBase
end
ActiveRecord::Base.connection_class_for_self => ActiveRecord::Base
UnshardedBase.connection_class_for_self => UnshardedBase(abstract)
UnshardedModel.connection_class_for_self => UnshardedBase(abstract)
```
The new `.shard_keys` method is a getter which returns the value of
`@shard_keys` from the connection class or it returns an empty array.
The empty array is necessary in cases where `connects_to` was never
called.
Finally, I've added an `.connected_to_all_shards` method which takes all of the
arguments for `.connected_to` except for `shard`. Instead, it loops through
every shard key and then delegates everything else to `.connected_to`. I've
used `.map` instead of `.each` so that we can collect the results of each block.
Fix: #52111
Fix: 5dbc7b4
The above commit caused the size of the `CodeGenerator` method cache
to explode, because the dynamic namespace is way too granular.
But there is actually a much better fix for that, since `alias_attribute`
is now generating exactly the same code as the attribute it's aliasing,
we can generated it as the canonical method in the cache, and then just
define it in the model as the aliased name.
This prevent the cache from growing a lot, and even reduce memory
usage further as the original attribute and its alias now share
the same method cache.
Based on https://github.com/rails/rails/pull/52017
One concern raised by Xavier is users holding on the return value
of `.current_transaction` beyond the point where it is committed /
rolled back / invalidated.
I believe this is an invalid use of the API, just like holding
`ActiveRecord::Base.connection` beyond the scope of a request is.
However we can be more explicit about it, so I changed the callback
registration methods to raise an error when called on a finalized
transaction.
Another concern was the usability of the null-object in the Active
Record notification payloads, and I agree that while the null-object
make sense when calling `Model.current_transaction`, it doesn't make
sense to include it in the payload of events. The goal of the
`.current_transaction` API is to allow implementing transaction aware
code in a streamlined way. The goal of the `:transaction` in events
however it to allow logging whether a query was inside a transaction
or not, so it's much more ergonomic for it to be nilable.
So I kept Matthew's change that passes `transaction: nil` in `sql.active_record` events
when not inside a transaction. I also added test coverage to make
sure it behaves consistently whether we're inside a transactional
test or not.
I also kept the separation between internal and "user" transaction
objects, as I think it's a nice way to limit the effectively exposed
API, and prevent users from abusing that API too much.
Co-Authored-By: Jean Boussier <jean.boussier@gmail.com>
