mastofe

commit: a8deb6648bc348e64469cc3451040b46ea057b77
parent: 20a6584d2dd9d5ecaa19a45a0c0c5ffec5a100ff
Author: Eugen Rochko <eugen@zeonfederated.com>
Date:   Wed, 13 Dec 2017 12:15:28 +0100

Fix redundant HTTP request in FetchLinkCardService (#6002)


Diffstat:
M
app/lib/provider_discovery.rb
19
++++++++++++++++---
M
app/services/fetch_link_card_service.rb
58
+++++++++++++++++++++++++++++-----------------------------

2 files changed, 45 insertions(+), 32 deletions(-)
diff --git a/app/lib/provider_discovery.rb b/app/lib/provider_discovery.rb
@@ -2,13 +2,26 @@
 
 class ProviderDiscovery < OEmbed::ProviderDiscovery
   class << self
+    def get(url, **options)
+      provider = discover_provider(url, options)
+
+      options.delete(:html)
+
+      provider.get(url, options)
+    end
+
     def discover_provider(url, **options)
-      res    = Request.new(:get, url).perform
       format = options[:format]
 
-      raise OEmbed::NotFound, url if res.code != 200 || res.mime_type != 'text/html'
+      if options[:html]
+        html = Nokogiri::HTML(options[:html])
+      else
+        res = Request.new(:get, url).perform
+
+        raise OEmbed::NotFound, url if res.code != 200 || res.mime_type != 'text/html'
 
-      html = Nokogiri::HTML(res.to_s)
+        html = Nokogiri::HTML(res.to_s)
+      end
 
       if format.nil? || format == :json
         provider_endpoint ||= html.at_xpath('//link[@type="application/json+oembed"]')&.attribute('href')&.value
diff --git a/app/services/fetch_link_card_service.rb b/app/services/fetch_link_card_service.rb
@@ -40,6 +40,12 @@ class FetchLinkCardService < BaseService
 
     return if res.code != 405 && (res.code != 200 || res.mime_type != 'text/html')
 
+    @response = Request.new(:get, @url).perform
+
+    return if @response.code != 200 || @response.mime_type != 'text/html'
+
+    @html = @response.to_s
+
     attempt_oembed || attempt_opengraph
   end
 
@@ -70,32 +76,32 @@ class FetchLinkCardService < BaseService
   end
 
   def attempt_oembed
-    response = OEmbed::Providers.get(@url)
+    embed = OEmbed::Providers.get(@url, html: @html)
 
-    return false unless response.respond_to?(:type)
+    return false unless embed.respond_to?(:type)
 
-    @card.type          = response.type
-    @card.title         = response.respond_to?(:title)         ? response.title         : ''
-    @card.author_name   = response.respond_to?(:author_name)   ? response.author_name   : ''
-    @card.author_url    = response.respond_to?(:author_url)    ? response.author_url    : ''
-    @card.provider_name = response.respond_to?(:provider_name) ? response.provider_name : ''
-    @card.provider_url  = response.respond_to?(:provider_url)  ? response.provider_url  : ''
+    @card.type          = embed.type
+    @card.title         = embed.respond_to?(:title)         ? embed.title         : ''
+    @card.author_name   = embed.respond_to?(:author_name)   ? embed.author_name   : ''
+    @card.author_url    = embed.respond_to?(:author_url)    ? embed.author_url    : ''
+    @card.provider_name = embed.respond_to?(:provider_name) ? embed.provider_name : ''
+    @card.provider_url  = embed.respond_to?(:provider_url)  ? embed.provider_url  : ''
     @card.width         = 0
     @card.height        = 0
 
     case @card.type
     when 'link'
-      @card.image = URI.parse(response.thumbnail_url) if response.respond_to?(:thumbnail_url)
+      @card.image = URI.parse(embed.thumbnail_url) if embed.respond_to?(:thumbnail_url)
     when 'photo'
-      return false unless response.respond_to?(:url)
-      @card.embed_url = response.url
-      @card.image     = URI.parse(response.url)
-      @card.width     = response.width.presence  || 0
-      @card.height    = response.height.presence || 0
+      return false unless embed.respond_to?(:url)
+      @card.embed_url = embed.url
+      @card.image     = URI.parse(embed.url)
+      @card.width     = embed.width.presence  || 0
+      @card.height    = embed.height.presence || 0
     when 'video'
-      @card.width  = response.width.presence  || 0
-      @card.height = response.height.presence || 0
-      @card.html   = Formatter.instance.sanitize(response.html, Sanitize::Config::MASTODON_OEMBED)
+      @card.width  = embed.width.presence  || 0
+      @card.height = embed.height.presence || 0
+      @card.html   = Formatter.instance.sanitize(embed.html, Sanitize::Config::MASTODON_OEMBED)
     when 'rich'
       # Most providers rely on <script> tags, which is a no-no
       return false
@@ -107,17 +113,11 @@ class FetchLinkCardService < BaseService
   end
 
   def attempt_opengraph
-    response = Request.new(:get, @url).perform
-
-    return if response.code != 200 || response.mime_type != 'text/html'
-
-    html = response.to_s
-
     detector = CharlockHolmes::EncodingDetector.new
     detector.strip_tags = true
 
-    guess = detector.detect(html, response.charset)
-    page  = Nokogiri::HTML(html, nil, guess&.fetch(:encoding, nil))
+    guess = detector.detect(@html, @response.charset)
+    page  = Nokogiri::HTML(@html, nil, guess&.fetch(:encoding, nil))
 
     if meta_property(page, 'twitter:player')
       @card.type   = :video
@@ -134,16 +134,16 @@ class FetchLinkCardService < BaseService
       @card.image_remote_url = meta_property(page, 'og:image') if meta_property(page, 'og:image')
     end
 
-    @card.title            = meta_property(page, 'og:title').presence || page.at_xpath('//title')&.content || ''
-    @card.description      = meta_property(page, 'og:description').presence || meta_property(page, 'description') || ''
+    @card.title       = meta_property(page, 'og:title').presence || page.at_xpath('//title')&.content || ''
+    @card.description = meta_property(page, 'og:description').presence || meta_property(page, 'description') || ''
 
     return if @card.title.blank? && @card.html.blank?
 
     @card.save_with_optional_image!
   end
 
-  def meta_property(html, property)
-    html.at_xpath("//meta[@property=\"#{property}\"]")&.attribute('content')&.value || html.at_xpath("//meta[@name=\"#{property}\"]")&.attribute('content')&.value
+  def meta_property(page, property)
+    page.at_xpath("//meta[@property=\"#{property}\"]")&.attribute('content')&.value || page.at_xpath("//meta[@name=\"#{property}\"]")&.attribute('content')&.value
   end
 
   def lock_options