This patch just changes the inspect method on test case instances.
Seeing test instance internals probably isn't helpful when an exception
is raised (for example a `NoMethodError`).
This isn't as good as #45122, but should fix#45121
This change incorporates to Rails a feature called error_highlight that
has been available since Ruby 3.1. This allow Rails' error report screen
to display the fine-grained location where an error occurred (not only a
line number but also beginning and end column numbers of the code
fragment).
For ErrorHighlight, see https://bugs.ruby-lang.org/issues/17930 in
detail.
To use error_highlight, ExceptionWrapper now prefers
`Exception#backtrace_locations` (since Ruby 2.1), which returns an array
of `Thread::Backtrace::Location`s, instead of `Exception#backtrace`.
This is because error_highlight requires `Thread::Backtrace::Location`
to locate the column where an error was raised.
Co-Authored-By: John Hawthorn <john@hawthorn.email>
Co-Authored-By: Jean Boussier <jean.boussier@gmail.com>
The driver being used is an implementation detail of `redis-rb`.
If somehow something break on one driver but not the other, it should
be reported to redis-rb and fixed there.
Also `redis-rb` `5.0` has a totally new client and hiredis binding
so all this code no longer works with redis-rb 5.0.
When working in a large rails app, I noticed in a flamegraph of a particular request that ~68ms was spent in the `xml_name_escape` method. I also ran an allocation tracer, which showed this method at the top of the list for String allocations.
This patch updates this method to avoid 3 String allocations:
- 2 allocations saved by using gsub! instead of gsub
- 1 allocation saved by concatenating to an existing string instead of allocating a new output string
For a rough benchmark in our rails app, I wrote a test with an allocation tracer around rendering a small view component.
- 244 String allocations before this change
- 228 String allocations from switching to gsub!
- 220 String allocations with this full patch
A ~10% reduction in String allocations in a real-world example seemed like a good justification for this small change.
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 moves some of the documentation from the `ErrorReporter` class
directly to the `handle` and `record` methods, and fleshes those out.
This also tweaks the documentation of the other `ErrorReporter` methods.
For redis cache store, running `Rails.cache.delete_multi(["foo", "bar"])`
will lead to an error when redis is down. Other existing cache store methods
already implement failure safety.
Instead we lazily check for pipelining support. `Redis::Distributed`
immediately raise when `pipelined` is called.
Also instead of using `mset` we use a pipelined. The performance
difference is extremly minimal, but it allows us to re-use `write_entry`
hence more shared code and we can speed up `write_multi` with an
expiry.
In #30893, the `credentials:edit` command was changed to prevent saving
invalid YAML:
```yaml
# some_invalid_yaml.yml
secret_key_base: ...
- new_key: new value
```
```console
$ EDITOR='cp some_invalid_yaml.yml' bin/rails credentials:edit
ruby-3.1.2/lib/ruby/3.1.0/psych.rb:455:in `parse': (<unknown>): did not find expected key while parsing a block mapping at line 3 column 1 (Psych::SyntaxError)
$ bin/rails credentials:show
secret_key_base: ...
```
However, throwing away user input is not ideal. Such behavior can be
particularly troublesome when copying secrets from ephemeral sources.
This commit changes the `credentials:edit` command to always save user
input, and display a helpful warning when saving invalid YAML:
```console
$ EDITOR='cp some_invalid_yaml.yml' bin/rails credentials:edit
File encrypted and saved.
WARNING: Invalid YAML in '/path/to/app/config/credentials.yml.enc'.
(/path/to/app/config/credentials.yml.enc): did not find expected key while parsing a block mapping at line 3 column 1
Your application will not be able to load 'config/credentials.yml.enc' until the error has been fixed.
$ bin/rails credentials:show
# some_invalid_yaml.yml
secret_key_base: ...
- new_key: new value
```
This commit also applies the same fix to the `encrypted:edit` command.
Prior to this commit, if `config.require_master_key` was set to true in
`config/application.rb`, the `credentials:edit` command could not
automatically generate a master key file:
```ruby
# In config/application.rb
config.require_master_key = true
```
```console
# No previously existing credentials
$ rm config/master.key config/credentials.yml.enc
```
```console
$ bin/rails credentials:edit
activesupport/lib/active_support/encrypted_file.rb:118:in `handle_missing_key': Missing encryption key to decrypt file with. Ask your team for your master key and write it to config/master.key or put it in the ENV['RAILS_MASTER_KEY']. (ActiveSupport::EncryptedFile::MissingKeyError)
```
This commit adds a `EncryptedFile#key?` method that can be used to check
for the existence of a key without raising `MissingKeyError`, and the
`credentials:edit` command now uses this method:
```console
$ bin/rails credentials:edit
Adding config/master.key to store the encryption key: ...
Save this in a password manager your team can access.
If you lose the key, no one, including you, can access anything encrypted with it.
create config/master.key
```
This commit also applies the same fix to the `encrypted:edit` command.
Prior to this commit, `EncryptedFile` would internally memoize when a
key file was non-existent. This meant that a key file generated after
checking `encrypted_file.key.nil?` would not be recognized, unless a new
`EncryptedFile` instance was used. (Though setting the key in a
designated environment variable instead would entirely bypass this
memoization.)
This commit changes `EncryptedFile` to only memoize when the key file
has been read. Consequently, this commit memoizes the `EncryptedFile`
(or, specifically, `EncryptedConfiguration`) instance used by
`CredentialsCommand`.
This commit also adds more test coverage around key file generation for
`CredentialsCommand`.
We should not need to check defined? here because we are only interested
in whether @html_safe is truthy or falsy.
We can use an aliased attr_reader to make this even faster by skipping
both method dispatch.
Co-authored-by: Jean Boussier <jean.boussier@gmail.com>
Although Ruby provides `Base64.urlsafe_encode64` and
`Base64.urlsafe_decode64` methods, the technical term is "URL-safe",
with "URL" and "safe" as separate words.
For better readability, this commit renames the `:urlsafe` option for
`MessageEncryptor` and `MessageVerifier` to `:url_safe`.
with some additional changes made later:
1. Flip the condition for backward compatibility
Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>
2. Improve custom behavior test
Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>
3. Fix indentation
This adds a `:urlsafe` option to the `MessageEncryptor` constructor.
When enabled, this option ensures that messages use a URL-safe encoding.
This matches the `MessageVerifier` `:urlsafe` option added in #45419.
Follow-up to #43924.
This commit refactors the logic around assembling and extracting the
parts of a message (namely: the encrypted data, the IV, and the auth
tag). It also provides a small but reproducible performance increase
for a roundtrip.
Benchmark:
```ruby
require "benchmark/ips"
require "active_support/message_encryptor"
DATA = "x" * 100
ENCRYPTOR = ActiveSupport::MessageEncryptor.new(SecureRandom.random_bytes(32))
Benchmark.ips do |x|
x.report("roundtrip") do
ENCRYPTOR.decrypt_and_verify(ENCRYPTOR.encrypt_and_sign(DATA))
end
end
```
Before:
```
Warming up --------------------------------------
roundtrip 1.342k i/100ms
Calculating -------------------------------------
roundtrip 13.525k (± 1.5%) i/s - 68.442k in 5.061532s
```
After:
```
Warming up --------------------------------------
roundtrip 1.409k i/100ms
Calculating -------------------------------------
roundtrip 14.125k (± 1.4%) i/s - 71.859k in 5.088419s
```
The `test_urlsafe` test could not fail when `urlsafe: false` because the
serialized input data did not contain a bit sequence that would encode
to a non-URL-safe character. The `test_no_padding` *could* fail when
`urlsafe: false`, except when the default serializer uses JSON, because
the serialized input data would be a multiple of 3 bytes, thus not
requiring any padding.
This commit replaces those tests with a falsifiable test using a
carefully chosen input string.
In minitest/minitest@6e06ac9 minitest changed such that it now accepts
`kwargs` instead of requiring kwargs to be shoved into the args array.
This is a good change but required some updates to our test code to get
the new version of minitest passing.
Changes are as follows:
1) Lock minitest to 5.15 for Ruby 2.7. We don't love this change but
it's pretty difficult to get 2.7 and 3.0 to play nicely together with
the new kwargs changes. Dropping 2.7 support isn't an option right
now for Rails. This is safe because all of the code changes here are
internal methods to Rails like assert_called_with. Applications
shouldn't be consuming them as they are no-doc'd.
2) Update the `assert_called_with` method to take any kwargs but also
the returns kwarg.
3) Update callers of `assert_called_with` to move the kwargs outside the
args array.
4) Update the message from marshaled exceptions. In 5.16 the exception
message is "result not reported" instead of "Wrapped undumpable
exception".
Co-authored-by: Matthew Draper <matthew@trebex.net>
urlsafe option was introduced to MessageVerifier in
09c3f36a96 but it can generate strings
containing padding character ("=") which is not urlsafe.
Fix not to pad when base64 encode.
MessageVerifier uses Base64.strict_encode64 and generated strings are
not urlsafe. Though the goal is to make MessageVerifier generated
strings urlsafe, We can not simply switch to Base64.urlsafe_encode64
because it will be a breaking change. Thus, as a first step, urlsafe
option is added to the MessageVerifier initializer.
The `assert_called_with` helper allows passing a multi-dimensional array to
mock multiple calls to the same method for a given block. This works
fine now, but when adding support for real kwargs arguments to line up with
recent upgrades in Minitest, this approach is no longer workable because
we can't pass multiple sets of differing kwargs.
Rather than complicated this method further, this commit removes the
multi-call form of `assert_called_with` and modifies the tests that
currently make use of that functionality to just use the underlying
`Minitest::Mock` calls.
Co-authored-by: Eileen M. Uchitelle <eileencodes@gmail.com>
The logic for shortening long keys in MemCacheStore
is broken since the time it stopped using MD5 checksumming.
Memcached supports keys up to 250 chars length. The reason
things worked I believe is because the underlying Dalli
library is doing it's own key shortening. Which uses its own
hashing function and in fact limits keys to 249 chars, see
petergoldstein/dalli@74b2625f11
This patch in a similar way properly truncates keys to 250
characters and avoids double hashing on Dalli side. Also
makes key name more predictable and independent from the
underlying Dalli version.
Previously, including/prepending/extending one of these deprecated
constants would silently succeed, since it is a module.
This adds a defintion for append_features/prepend_features/extended so
that it can forward the inclusion onto the target module.
This commit aims to improve ActiveSupport::Notifications::Fanout. There
are three main goals here: backwards compatibility, safety, and
performance.
* Backwards compatibility
This ActiveSupport::Notifications is an old and well used interface.
Over time it has collected a lot of features and flexibility, much of
which I suspect is not used anywhere by anyone, but it is hard to know
specifics and we would at minimum need a deprecation cycle.
For this reason this aims to fully maintain compatibility. This includes
both the ability to use an alternate notification implementation instead
of Fanout, the signatures received by all types of listeners, and the
interface used on the Instrumenter and Fanout itself (including the
sometimes problematic start/finish).
* Safety
There have been issues (both recent and past) with the "timestacks"
becoming invalid, particularly when subscribing and unsubscribing within
events. This is an issue when topics are subscribed/unsubscribed to
while they are in flight.
The previous implementation would record a separate timestamp or event
object for each listener in a thread local stack. This meant that it was
essential that the listeners to start and finish were identical.
This issue is avoided by passing the listeners used to `start` the event
to `finish` (`finish_with_state` in the Instrumenter), to ensure they
are the same set in `start`/`finish`.
This commit further avoids this issue. Instead of pushing individual
times onto a stack, we now push a single object, `Handle`, onto the
stack for an event. This object holds all the subscribers (recorded at
start time) and all their associated data. This means that as long as
start/stop calls are not interleaved.
This commit also exposes `build_handle` as a public interface. This
returns the Handle object which can have start/stop called at any time
and any order safely. The one reservation I have with making this public
is that existing "evented" listeners (those receiving start/stop) may
not be ready for that (ex. if they maintain an internal thread-local
stack).
* Performance
This aims to be faster and make fewer allocations then the existing
implementation.
For time-based and event-object-based listeners, the previous
implementation created a separate object for each listener, pushing
and popping it on a thread-local stack. This is slower both because we
need to access the thread local repeatedly (hash lookups) and because
we're allocating duplicate objects.
The new implementation works by grouping similar types of listeners
together and shares either the `Event` or start/stop times between all
of them. The grouping was done so that we didn't need to allocate Events
or Times for topics which did have a listener of that type.
This implementation is significantly faster for all cases, except for
evented, which is slower.
For topics with 10 subscriptions:
*main*:
timed 66.739k (± 2.5%) i/s - 338.800k in 5.079883s
timed_monotonic 138.265k (± 0.6%) i/s - 699.261k in 5.057575s
event_object 48.650k (± 0.2%) i/s - 244.250k in 5.020614s
evented 366.559k (± 1.0%) i/s - 1.851M in 5.049727s
unsubscribed 3.696M (± 0.5%) i/s - 18.497M in 5.005335s
*This branch*:
timed 259.031k (± 0.6%) i/s - 1.302M in 5.025612s
timed_monotonic 327.439k (± 1.7%) i/s - 1.665M in 5.086815s
event_object 228.991k (± 0.3%) i/s - 1.164M in 5.083539s
evented 296.057k (± 0.3%) i/s - 1.501M in 5.070315s
unsubscribed 3.670M (± 0.3%) i/s - 18.376M in 5.007095s
Co-authored-by: John Crepezzi <john.crepezzi@gmail.com>
Co-authored-by: Theo Julienne <theojulienne@github.com>
Because these strings contain no HTML elements and the basic entities
are escaped, they are safe to be included as-is as PCDATA in HTML
content. Tagging them as html-safe avoids double-escaping entities
when being concatenated to a SafeBuffer during rendering.
Fixes https://github.com/rails/rails-html-sanitizer/issues/124
An ArgumentError was being raised when methods were called with (proc,
options) inside a with_options block:
def my_method(arg1, **kwargs)
[arg1, kwargs]
end
# this would raise instead of merging options
with_options(hello: "world") do
my_method(proc {}, {fizz: "buzz"})
end
Fixes#45183
The previous documentation hints that the return value if `other` `acts_like` time is seconds with the example, but it doesn't explicitly say "seconds" anywhere. Just trying to make it more obvious that the value is seconds, and not something like milliseconds or nanoseconds, etc.
Without some help, failures in a forked process make for some noise in
the output, but won't fail the build.
Instead of trying to transfer the whole exception back, I've gone for a
simpler solution of just sending _something_ (the exception class name)
back so we'll fail; the full failure will be visible in the child
process's stderr output.
Vue.js, alpinejs, and potentially other JS libraries support tags
starting with `@` symbols. This was broken by the recent security release in
649516ce0f
I've only added `@` to the list even though there are potentially other
safe characters. We can add more if necessary (and if safe).
Fixes:
* #45014
* #44972
Clarify to users what objects may be cached, and highlight the option used to cache default non-serializable data.
Purpose: Improving new-to-Rails users' experience, as this detail may not be obvious, costing them time and effort spent debugging.
Co-authored-by: Hartley McGuire <skipkayhil@gmail.com>
Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>
Add the method ERB::Util.xml_name_escape to escape dangerous characters
in names of tags and names of attributes, following the specification of
XML.
Use that method in the tag helpers of ActionView::Helpers. Rename the option
:escape_attributes to :escape, to simplify by applying the option to the whole
tag.
The stdlib Logger::new allows passing a :formatter keyword argument to
set the logger's formatter. ActiveSupport::Logger::new ignores this
argument by always setting the formatter to an instance of
SimpleFormatter. Instead, we should only set it when none is yet set.
Every time I write `config.cache_classes` I have to pause for a moment to make
sure I get it right. It makes you think.
On the other hand, if you read `config.enable_reloading = true`, does the
application reload? You do not need to spend 1 cycle of brain CPU to nod.
The default message would not tell you what the actual value is, just
what it expected it to have changed to or from.
It now tells you what the actual value is, similar to the output you'd
get from a matcher such as `assert_equal`
- `#first` raises an error when called on a beginless range.
Using `#begin` to get the first element instead.
- Adding additional equality condition to cover the case when both
ranges are beginless
With Ruby 2.4+ the default for +to_time+ changed from converting to the
local system time to preserving the offset of the receiver. At the time
Rails supported older versions of Ruby so a compatibility layer was
added to assist in the migration process. From Rails 5.0 new applications
have defaulted to the Ruby 2.4+ behavior and since Rails 7.0 now only
supports Ruby 2.7+ this compatibility layer can be safely removed.
To minimize any noise generated the deprecation warning only appears when
the setting is configured to `false` as that is the only scenario where
the removal of the compatibility layer has any effect.
The tests for the redis cache store are hard-coded to use redis://localhost:6379/0
and redis://localhost:6379/1. This prevents them from running within a docker compose
environment where typically the url would be redis://redis:6379/0.
- This PR fixes two issues with the Tagged Logging feature in
conjunction with broadcasting logs.
For the sake of clarity I'll define the "main logger" and
the "broadcasted logger" in this snippet:
```ruby
main_logger = ActiveSupport::TaggedLogging.new(ActiveSupport::Logger.new(io))
broadcaster_logger = ActiveSupport::TaggedLogging.new(ActiveSupport::Logger.new(io))
main_logger.extend(Activesupport::Logger.broadcast(broadcasted_logger))
```
1) The first issue that this PR fixes, is that the tags on the "main logger"
don't propagate to the "broadcasted logger" when you pass in a block.
```ruby
main_logger.tagged("FOO") { |logger| logger.info("Hello") }
# Outputs:
# [Foo] Hello <- By the main logger
# Hello <- By the broadcasted logger
```
A fix was made in 70af536b5d
but that only works for the non block version
2) It's quite common for the "broadcasted logger" to have a diffent
log formatter that the "main logger". In example you'd want to
output JSON logs in one and raw text in the other.
That wasn't possible before. All loggers had to have the same
instance of the formatter. The formatter was set on all loggers
thanks to [this](3fc9d12875/activesupport/lib/active_support/logger.rb (L45-L48)) and it's [associated test](3fc9d12875/activesupport/test/broadcast_logger_test.rb (L58-L64))
This requirement was needed to make the Tagged Logging feature
work; the tags being set in a thread variable whose name
uses the `object_id` 3fc9d12875/activesupport/lib/active_support/tagged_logging.rb (L59)
(different formatter instance -> different object_id -> different variables)
In this PR, I have removed the code that sets the same formatter
instance on all logger. The "broadcaster logger" just need to
have the `current_tags` point to the `current_tags` of the
"main logger", I'm doing that by redefing the `current_tags`
method each time the "main logger" uses a different formatter.
The advantages by doing so is that custom made formatter
can now call this `current_tags` method, which will return
all the tags and process them the way they want.
```ruby
class JSONLogFormatter
def call(_, _, _, msg)
tags = current_tags # Can now retrieve the tags
{ message: msg, tags: tags }.to_json
end
end
broadcasted_logger = Logger.new(io)
broadcaster_logger.formatter = JSONLogFormatter.new
main_logger = Logger.new(io)
main_logger.extend(ActiveSupport::Logger.broadcast(broadcasted_logger))
```
The behavior remains the same as before if a logger uses the
Rails vanilla formatter or the Tagged Logging formatter.
activerecord/lib/active_record/connection_adapters/postgresql/column.rb
- usage added in 64fd666
- unneeded because of active_support/rails: 8f58d6e
railties/lib/rails/rack/logger.rb
- usage added in c83d9a1
- usage removed in c131211
activesupport/lib/active_support/number_helper/number_converter.rb
- the NumberHelper was split into multiple classes in 2da9d67, however
the require was left in NumberConverter even though
NumberToPhoneConverter is the only class where it's used
activesupport/lib/active_support/duration/iso8601_serializer.rb
- usage added in 04c512d
- usage removed in 51e991f
Ref: https://github.com/rails/rails/pull/43625#discussion_r809532572
It can be used by error reporting service integration when they wish
to handle the error higher in the stack.
For instance Sidekiq has its own error handling interface with a
little bit of extra context information.
Ref: https://github.com/aws/aws-sdk-ruby/pull/2670
Some gems like aws-sdk-core use `Object#extend(Enumerable)`.
It's not a very good pattern, but it's somehwat handled ok by Ruby.
However if Enumerable has constants, then any time the module is
extended, the global constant cache is flushed and this has a very
negative impact on performance for the virtual machine, and even
worse for JITs.
We no longer need to define instance variables to avoid uninitialized
instance variable warnings.
Also rather than to keep a hash of `constant -> path` forever, we
can simply keep a list of constants and call `const_get` for each of
them.
And finally we can clear the list we kept, as it's just wasted memory.
core_ext/array/wrap
- added in b1164adda1
- usage removed in fa986ae0ca
core_ext/numeric/time
- added in ee51b51b60, but usage was only
in mem_cache_store so moved require there
RDoc will automatically format and link API references as long as they
are not already marked up as inline code.
This commit removes markup from various API references so that those
references will link to the relevant API docs.
Aaron Patterson