FEATURE: Oneboxer cache response body (PR #12562)

[jbrw]

Some oneboxes may fail if when making excessive and/or odd requests against the target domains. This change provides a simple mechanism to cache the results of succesful GET requests as part of the oneboxing process, with the goal of reducing repeated requests and ultimately improving the rate of successful oneboxing.

To enable:

Set SiteSetting.cache_onebox_response_body to true

Add the domains you’re interesting in caching to SiteSetting. cache_onebox_response_body_domains e.g. example.com|example.org|example.net

Optionally set SiteSetting.cache_onebox_user_agent to a user agent string of your choice to use when making requests against domains in the above list.

GitHub

[ZogStriP]

lib/final_destination.rb

@@ -38,6 +38,7 @@ def initialize(url, opts = nil)
     @force_get_hosts = @opts[:force_get_hosts] || []
     @preserve_fragment_url_hosts = @opts[:preserve_fragment_url_hosts] || []
     @force_custom_user_agent_hosts = @opts[:force_custom_user_agent_hosts] || []
+    @default_user_agent = @opts[:default_user_agent] || "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/14.0 Safari/605.1.15"

Can you extract this user agent string into a constant?

[ZogStriP]

lib/final_destination.rb

@@ -16,7 +16,7 @@ def self.clear_https_cache!(domain)
 
   def self.cache_https_domain(domain)
     key = redis_https_key(domain)
-    Discourse.redis.without_namespace.setex(key, "1", 1.day.to_i).present?
+    Discourse.redis.without_namespace.setex(key, 1.day.to_i, "1").present?

Wow, we were calling it with wrong arguments?!

[ZogStriP]

lib/oneboxer.rb

@@ -80,6 +84,33 @@ def self.invalidate(url)
     Discourse.cache.delete(onebox_failed_cache_key(url))
   end
 
+  def self.cache_response_body?(uri)
+    uri = URI.parse(uri) if uri.is_a?(String)
+
+    if SiteSetting.cache_onebox_response_body?
+      SiteSetting.cache_onebox_response_body_domains.split("|").map { |domain| uri.hostname.ends_with?(domain) }.any?

I think you can pass a block to any?

      SiteSetting.cache_onebox_response_body_domains.split("|").any? { |domain| uri.hostname.ends_with?(domain) }

[ZogStriP]

lib/oneboxer.rb

@@ -80,6 +84,33 @@ def self.invalidate(url)
     Discourse.cache.delete(onebox_failed_cache_key(url))
   end
 
+  def self.cache_response_body?(uri)
+    uri = URI.parse(uri) if uri.is_a?(String)
+
+    if SiteSetting.cache_onebox_response_body?
+      SiteSetting.cache_onebox_response_body_domains.split("|").map { |domain| uri.hostname.ends_with?(domain) }.any?
+    end
+  end
+
+  def self.cache_response_body(uri, response)
+    key = redis_cached_response_body_key(uri)
+    Discourse.redis.without_namespace.setex(key, 1.minutes.to_i, response).present?

Do we need .present?? Is the return value ever used?

    Discourse.redis.without_namespace.setex(key, 1.minutes.to_i, response)

[jbrw]

The upshot was we were setting a non-zero value for 1 second, rather than setting a non-zero value for 1 day. Meaning we have have been doing a few more GETs than necessary, although with most sites defaulting to https this probably wasn’t a huge issue.