From 554626d6262bdf8d02ba5b05256eb1b625cf7bb4 Mon Sep 17 00:00:00 2001
From: Leo Arnold <github@leoarnold.de>
Date: Wed, 2 Aug 2023 23:56:38 +0200
Subject: [PATCH] Prevent spurious SQL query execution via ActiveSupport::Cache
 events

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
---
 meta_request/.rubocop.yml                    |  4 ++
 meta_request/lib/meta_request/event.rb       | 12 ++++-
 meta_request/spec/meta_request/event_spec.rb | 49 ++++++++++++++++++++
 3 files changed, 64 insertions(+), 1 deletion(-)
 create mode 100644 meta_request/spec/meta_request/event_spec.rb

diff --git a/meta_request/.rubocop.yml b/meta_request/.rubocop.yml
index cc32da4..69158a8 100644
--- a/meta_request/.rubocop.yml
+++ b/meta_request/.rubocop.yml
@@ -1 +1,5 @@
 inherit_from: .rubocop_todo.yml
+
+Metrics/BlockLength:
+  ExcludedMethods:
+    - describe
diff --git a/meta_request/lib/meta_request/event.rb b/meta_request/lib/meta_request/event.rb
index d305afc..739df32 100644
--- a/meta_request/lib/meta_request/event.rb
+++ b/meta_request/lib/meta_request/event.rb
@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require 'active_support'
+require 'active_support/cache'
 require 'active_support/json'
 require 'active_support/core_ext'
 
@@ -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
@@ -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)
@@ -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) &&
diff --git a/meta_request/spec/meta_request/event_spec.rb b/meta_request/spec/meta_request/event_spec.rb
new file mode 100644
index 0000000..8eb5f7a
--- /dev/null
+++ b/meta_request/spec/meta_request/event_spec.rb
@@ -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