I was reading this source code and made a few edits on passing to ensure
style details agree within the file itself and Rails.
The addition of "# noop" makes the method body look similar to other
analogous ones in the same file (not shown in the diff).
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.
When there are no fields:
* Omit blank line in migration prior to "t.timestamps"
* Omit leading and trailing spaced in empty hashes in
create and update controller and api functional tests
Co-authored-by: zzak <zzakscott@gmail.com>
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>
The documentation on ActiveSupport::MessageVerifier used the “sensitive data” string as an example; that wording might induce the developer to think we’re dealing with encryption, while the payload is actually only Base64 encoded and is not protected at all.
We also improve the documentation on ActiveRecord::SignedId, which uses MessageVerifier and thereby will also expose the ID as encoded cleartext, making explicit that it’s not encryption, only signing.
Lastly, we refer the developer to MessageEncryptor if the payload needs to be encrypted.
* Fix typo in `global_executor_concurrency` error message
* removed `using` and replace multi_thread_pool with `:multi_thread_pool`
Co-authored-by: Petrik de Heus <petrik@deheus.net>
---------
Co-authored-by: Petrik de Heus <petrik@deheus.net>
Co-authored-by: Rafael Mendonça França <rafael@rubyonrails.org>
If the value is already a local time, there's no difference, so no need
to warn.
Correspondingly, avoid calling to_time in the handful of places we were
using it internally: it's easy to do, and we know we don't care about
the zone.
Joshua Young