Skip to content

Commit

Permalink
Prevent spurious SQL query execution via ActiveSupport::Cache events
Browse files Browse the repository at this point in the history
When using a simple Rails view template like

```erb
<% cache User.all do %>
  some content here
<% end %>
```

we noticed that a spurious SQL query

```sql
SELECT "users".* FROM "users"
```

was executed both on cache miss and on cache hit.

Since the very point of [fragment caching][1] is to prevent this kind
of queries, we investigated and found out that they were caused by
`meta_request`.

Rather than just stating the cache key used, events from `ActiveSupport::Cache`
provide all original objects which contributed to the calculation of
the cache key. This included `User.all` from the template.

When `meta_request` checks whether this object is encodable to JSON

```ruby
User.all.as_json(methods: [:duration])
```

unintentionally triggers the spurious SQL query.

We fix this by sanitizing the event payload and replacing the inputs
of the cache key calculation with their final result.

[1]: https://guides.rubyonrails.org/caching_with_rails.html#fragment-caching
  • Loading branch information
leoarnold committed Aug 2, 2023
1 parent de081f7 commit 554626d
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 1 deletion.
4 changes: 4 additions & 0 deletions meta_request/.rubocop.yml
Original file line number Diff line number Diff line change
@@ -1 +1,5 @@
inherit_from: .rubocop_todo.yml

Metrics/BlockLength:
ExcludedMethods:
- describe
12 changes: 11 additions & 1 deletion meta_request/lib/meta_request/event.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# frozen_string_literal: true

require 'active_support'
require 'active_support/cache'
require 'active_support/json'
require 'active_support/core_ext'

Expand All @@ -13,6 +14,7 @@ class Event < ActiveSupport::Notifications::Event
attr_reader :duration

def initialize(name, start, ending, transaction_id, payload)
@name = name
super(name, start, ending, transaction_id, json_encodable(payload))
@duration = 1000.0 * (ending - start)
end
Expand All @@ -37,7 +39,7 @@ def self.events_for_exception(exception_wrapper)
def json_encodable(payload)
return {} unless payload.is_a?(Hash)

transform_hash(payload, deep: true) do |hash, key, value|
transform_hash(sanitize_hash(payload), deep: true) do |hash, key, value|
if value.class.to_s == 'ActionDispatch::Http::Headers'
value = value.to_h.select { |k, _| k.upcase == k }
elsif not_encodable?(value)
Expand All @@ -54,6 +56,14 @@ def json_encodable(payload)
end.with_indifferent_access
end

def sanitize_hash(payload)
if @name =~ /\Acache_\w+\.active_support\z/
payload[:key] = ActiveSupport::Cache::Store.new.send(:normalize_key, payload[:key])
end

payload
end

def not_encodable?(value)
(defined?(ActiveRecord) && value.is_a?(ActiveRecord::ConnectionAdapters::AbstractAdapter)) ||
(defined?(ActionDispatch) &&
Expand Down
49 changes: 49 additions & 0 deletions meta_request/spec/meta_request/event_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# frozen_string_literal: true

require 'spec_helper'

RSpec.describe MetaRequest::Event do
describe '.new' do
context 'when transforming an event from ActiveSupport::Cache' do
let(:active_support_cache_events) do
%w[
cache_read.active_support
cache_write.active_support
]
end

let(:payload) do
{
key: [
'views',
'users/show:118c2da025706846afc6874e76b33a5c',
active_record_relation
]
}
end

let(:active_record_relation) do
double('ActiveRecord::Relation', cache_key: 'users/query-e06f359ccb226f3021b50c0c7e457f79')
end

let(:full_cache_key) do
'views/users/show:118c2da025706846afc6874e76b33a5c/users/query-e06f359ccb226f3021b50c0c7e457f79'
end

it 'replaces the cache key with its String representation and does not trigger SQL queries' do
# This would result in an SQL query being executed
expect(active_record_relation).to_not receive(:as_json)

event = described_class.new(
active_support_cache_events.sample,
Time.parse('2023-08-02 23:10:00.759479575 +0200'),
Time.parse('2023-08-02 23:10:00.761230028 +0200'),
SecureRandom.hex(10),
payload
)

expect(event.payload[:key]).to eq(full_cache_key)
end
end
end
end

0 comments on commit 554626d

Please sign in to comment.