Prevent theses and items sitemap views from using the same cache key #3361

[jefferya]

Addresses #3361

• adds a distinct cache key to the partial used by both the theses and items sitemap view

Context

The theses and item sitemap views use the same cached partial with the same cache key thus, depending on timing and if caching is enabled (disabled in dev) the theses sitemap sometimes contains item or the items sitemap contains theses. The experience is intermittent if there are multiple load-balanced app servers each with their own in-memory cache store.

What's New

This change adds a unique cache key to the partial to differentiate between the theses and item sitemap view calls to the _object partial.

How to test

#3361 (comment)

1. Setup a local dev environment with seeds
2. Enable caching

diff --git a/config/environments/development.rb b/config/environments/development.rb
index b5f7947c..3bbaf5ab 100644
--- a/config/environments/development.rb
+++ b/config/environments/development.rb
@@ -30,9 +30,9 @@ Rails.application.configure do
       'Cache-Control' => "public, max-age=#{2.days.to_i}"
     }
   else
-    config.action_controller.perform_caching = false
+    config.action_controller.perform_caching = true
 
-    config.cache_store = :null_store
+    config.cache_store = :memory_store
   end

3. Start the Rails Console bundle exec rails console -e dev --sandbox
4. Test the Theses route app.get "http://era.lvh.me/sitemap-theses.xml"

...
  Rendered sitemap/_object.xml.builder (Duration: 33817.6ms | Allocations: 29713703) [cache miss]
...

4.a. notice the [cache miss] on the sitemap/_object.xml.builder

5. What does the cache key list look like? Rails.cache.instance_variable_get(:@data).keys

=> ["read_only_mode.first.enabled", "views/sitemap/_object:29b55f4c76b6c3fb21553304686aae88/sitemap"]

5.a. Old: notice that nothing uniquely identifies that the theses route was used to build the cache key
5.b PR: adds a unique key element `sitemap-theses`

6. Testing the items route: app.get "http://era.lvh.me/sitemap-items.xml"

Started GET "/sitemap-items.xml" for 127.0.0.1 at 2024-02-29 21:46:33 +0000
...
...
  Rendered sitemap/_object.xml.builder (Duration: 14.1ms | Allocations: 117) [cache hit]
...

6.a. Old: notice the [cache hit] on the sitemap/_object.xml.builder -- this should be a cache miss as this is a new route but the previous _object.xml.builder cached content (theses content) is being used to populate items
6.b PR: because of the unique cache key this should be a [cache miss].

7. Let's view the content of the cache again Rails.cache.instance_variable_get(:@data).keys

=> ["read_only_mode.first.enabled", "views/sitemap/_object:29b55f4c76b6c3fb21553304686aae88/sitemap"]

7.a. Old: key list is unchanged
7.b PR: a new cache key entry with `sitemap-items`

8. To test contents: document = Nokogiri::XML(@response.body)
   8.a. Old: theses in the items sitemap even though the request was for items
   8.b. PR: items in the items sitemap (no theses)

[jefferya]

Ensure a unique cache key is generated depending on whether to theses sitemap view or the item sitemap view is rendering the partial.

I'm unsure how to write a test to capture the issue... is one needed? Options I've tried:

1. Enable caching in the config/environments/test.rb however there is a possibility for unintended consequences Rails cache prefix is not properly configured for isolated caching during parallel tests. rails/rails#48341

config.action_controller.perform_caching = true
config.cache_store = :memory_store

2. A sitemap_test.rb runtime, dynamically enable the cache during setup and disable during the teardown. My limited understanding has found a successful approach. I was unable to get an approach like this https://stackoverflow.com/questions/14249954/how-to-set-perform-caching-dynamically-in-rails to work.

[murny]

Nice investigate work 👍. The solution makes sense to me.

Regarding testing this, I am kind of in the camp that maybe it's not required? Even if you found a way to test this, the solution would probably introduce a ton of complexity that might not be worth it?