Fix FilterFullFeed to handle HTTPS/HTTP scheme mismatches and encoding

doc/BASIC_DESIGN.md

@@ -244,8 +244,11 @@ what is fetched.
 The first two use Ruby's bundled `rss` library. That is the reason the pipeline
 value has the shape it has.
 
-- `Http.read(url)` fetches a URL and returns the body; `Http.uri(url)` returns
-  a validated URI and `Http.fetchable?(url)` answers whether there is one.
+- `Http.read(url)` fetches a URL and returns the body and `Http.open(url)`
+  yields the stream, for a caller — an HTML parser, which detects a page's
+  encoding for itself — that would rather not be handed a decoded string;
+  `Http.uri(url)` returns a validated URI and `Http.fetchable?(url)` answers
+  whether there is one.
 
 `Automatic::Http` exists because the decisions a fetch implies — which schemes
 are allowed, how long to wait, how many redirects to follow, what to send as a

doc/PLUGINS.md

@@ -415,10 +415,19 @@ A plugin that fetches over HTTP calls `Automatic::Http`:
 
 ```ruby
 body = Automatic::Http.read(url)          # the body, or an exception
+Automatic::Http.open(url) { |io| ... }    # the stream, for a caller that wants it
 Automatic::Http.uri(url)                  # a validated URI, or an exception
 Automatic::Http.fetchable?(url)           # for skipping an item rather than failing
 ```
 
+`read` returns a string that `open-uri` has already applied an encoding to,
+whether or not the response declared one: a page served as `text/html` with no
+charset comes back tagged UTF-8 because that is the fallback, not because the
+page said so. A plugin that hands the body to an HTML parser wants `open`
+instead, because a parser given the stream reads the `meta` charset for itself
+and a parser given the string believes the tag. `FilterFullFeed` is the worked
+example; the difference there was a whole article in mojibake.
+
 It is a helper and not a client: it opens the URL through `open-uri` with the
 scheme restricted to HTTP and HTTPS, a connect and a read timeout, a bounded
 redirect chain and this project named as the agent. A URL string carrying
@@ -870,6 +879,33 @@ works; how well it works depends on whether the sites you read are in that
 snapshot and still laid out the same way. Supplying your own file in
 `~/.automatic/assets/siteinfo/` is the way to keep it useful.
 
+Three things follow from the database being that old, and the plugin now
+accounts for each:
+
+- **A link matches under either scheme.** 3,448 of the 3,504 usable records
+  anchor on a scheme and all but twenty of those say `^http://`. The sites they
+  name have since moved to HTTPS, which is what a feed hands over, so matching
+  the link as it stands matched almost nothing and the filter quietly did
+  nothing at all. A record describes a site's layout, not how it is
+  transported, so the link is tried under both. Only the match is rewritten;
+  the page is fetched from the link the feed gave.
+- **A record that selects nothing leaves the summary alone.** A site redesigned
+  since its XPath was written selects no nodes, and putting that empty result
+  into the item replaced a perfectly good summary with an empty description.
+  The item keeps what it arrived with, and the miss is logged at `warn` with
+  the XPath that missed.
+- **The page's own encoding is believed before the record's.** The page is
+  parsed from the stream, so a charset in a `meta` tag is read even when the
+  response declared none. A record's `enc` is the fallback for a page that
+  declares nothing anywhere — 1,186 records carry one, mostly EUC-JP and
+  Shift_JIS — and an `enc` naming an encoding Ruby does not have is ignored
+  rather than raised. What comes out is UTF-8 either way.
+
+A record with no URL pattern, no XPath, or a pattern that is not a regular
+expression is dropped when the file is loaded rather than being allowed to fail
+a match later; an empty pattern would otherwise match every link in the feed.
+The remaining patterns are compiled once, not once per item.
+
 #### FilterGithubFeed — **Supported**
 
 `filter/github_feed.rb`. Converts Atom entries — where `title`, `id` and

plugins/filter/full_feed.rb

@@ -12,9 +12,17 @@ module Automatic::Plugin
   class FilterFullFeed
     Automatic.require_optional('nokogiri', needed_by: 'FilterFullFeed')
     require 'json'
+    require 'stringio'
 
     SITEINFO_TYPES = %w[SBM INDIVIDUAL IND SUBGENERAL SUB GENERAL GEN].freeze
 
+    # One siteinfo record, reduced to the four things a match needs and with
+    # its URL pattern compiled once. The database ships with 3,504 usable
+    # records, so compiling them per item -- which is what matching against
+    # the raw JSON did -- was several thousand `Regexp.new` calls for every
+    # link in every feed.
+    Entry = Struct.new(:url, :pattern, :xpath, :encoding)
+
     def initialize(config, pipeline = [])
       @config   = config || {}
       @pipeline = pipeline
@@ -39,9 +47,25 @@ def siteinfo
       raise ArgumentError, 'FilterFullFeed needs a siteinfo file name' if name.empty?
 
       Automatic::Log.puts('info', "Loading siteinfo from #{name}")
-      entries = JSON.parse(File.read(File.join(assets_dir, name), encoding: 'UTF-8'))
-      entries.select { |info| SITEINFO_TYPES.include?(info['data']['type']) }
-             .sort_by { |info| SITEINFO_TYPES.index(info['data']['type']) }
+      records = JSON.parse(File.read(File.join(assets_dir, name), encoding: 'UTF-8'))
+      entries = records.select { |info| SITEINFO_TYPES.include?(info['data']['type']) }
+                       .sort_by { |info| SITEINFO_TYPES.index(info['data']['type']) }
+                       .filter_map { |info| entry(info['data']) }
+      Automatic::Log.puts('info', "Loaded #{entries.size} siteinfo entries")
+      entries
+    end
+
+    # A record is dropped here rather than failing a match later. A record
+    # without a URL pattern would match every link -- an empty pattern matches
+    # everything -- and one without an XPath has nothing to select with.
+    def entry(data)
+      url   = data['url'].to_s
+      xpath = data['xpath'].to_s.strip
+      return nil if url.empty? || xpath.empty?
+
+      Entry.new(url, Regexp.new(url), xpath, charset(data['enc']))
+    rescue RegexpError
+      nil
     end
 
     def assets_dir
@@ -54,29 +78,106 @@ def assets_dir
     def fulltext(item)
       return if item.link.nil?
 
-      info = @siteinfo.find { |entry| matches?(entry, item.link) }
-      if info.nil?
+      record = match(item.link)
+      if record.nil?
         Automatic::Log.puts('info', "Fulltext SITEINFO not found: #{item.link}")
         return
       end
 
-      Automatic::Log.puts('info', "Siteinfo matched: #{info['data']['url']}")
-      item.description = body(item.link, info['data']['xpath'])
+      Automatic::Log.puts('info', "Siteinfo matched: #{record.url}")
+      html = body(item.link, record)
+      if html.nil?
+        # The page was read but the XPath selected nothing, which is what a
+        # site that has been redesigned since its record was written looks
+        # like. Assigning the empty result here is what used to replace a
+        # perfectly good summary with an empty description.
+        Automatic::Log.puts('warn', "Fulltext XPath selected nothing on #{item.link}: #{record.xpath}")
+        return
+      end
+
+      item.description = html
     rescue StandardError => e
       # An unreadable page leaves the item's own summary in place, which is
       # what this filter is an improvement on rather than a replacement for.
       Automatic::Log.puts('warn', "Failed to read fulltext for #{item.link}: #{e.message}")
     end
 
-    def matches?(entry, link)
-      link.match?(entry['data']['url'].to_s)
-    rescue RegexpError
-      false
+    def match(link)
+      links = schemes(link)
+      @siteinfo.find { |record| links.any? { |candidate| record.pattern.match?(candidate) } }
+    end
+
+    # The database was last updated in 2013 and 3,448 of its 3,504 records
+    # anchor on a scheme, nearly all of them `^http://`. The sites they name
+    # have since moved to HTTPS, so an https link out of a feed matches none of
+    # them and the filter silently does nothing. Matching the link under either
+    # scheme is what keeps those records reachable; a record is about a site's
+    # layout, not about how it is transported. Only the match is rewritten --
+    # the page is fetched from the link the feed gave.
+    def schemes(link)
+      case link
+      when %r{\Ahttps://} then [link, link.sub(%r{\Ahttps://}, 'http://')]
+      when %r{\Ahttp://}  then [link, link.sub(%r{\Ahttp://}, 'https://')]
+      else [link]
+      end
+    end
+
+    # Returns the selected body as UTF-8, or nil when the XPath selects
+    # nothing, so that the caller can tell an article apart from an empty
+    # result and keep the summary it already had.
+    def body(link, entry)
+      nodes = document(link, entry).xpath(entry.xpath)
+      return nil if nodes.empty?
+
+      # Normalised to UTF-8, and scrubbed rather than raised on. The parser
+      # returns a string in whatever encoding it settled on for the page, and
+      # a page whose declared charset is not the one it is written in is
+      # common enough in a database this old; what leaves here goes on to a
+      # publish plugin, which has no way to recover from a string it cannot
+      # encode. `invalid:` as well as `undef:`, because converting UTF-8 to
+      # UTF-8 leaves invalid bytes alone unless they are named.
+      html = nodes.to_html.encode('UTF-8', invalid: :replace, undef: :replace)
+      html.strip.empty? ? nil : html
     end
 
-    def body(link, xpath)
-      document = Nokogiri::HTML.parse(Automatic::Http.read(link))
-      document.xpath(xpath).to_html.encode('UTF-8', undef: :replace)
+    # Read the page and parse it under the encoding it is actually written in.
+    #
+    # The page is handed to the parser as a stream rather than as a decoded
+    # string. open-uri applies an encoding to what it returns whether or not
+    # the response declared one -- a page served as `text/html` with no charset
+    # comes back tagged UTF-8 -- and a decoded string tells the parser to
+    # believe that tag and never look at the meta tag underneath it. The
+    # database is full of sites that declare their charset only in a meta tag,
+    # and for those the difference is the whole article in mojibake.
+    #
+    # A record's own `enc` is the last resort, for a page that declares nothing
+    # anywhere: it was recorded in 2013, and trusting it ahead of what the page
+    # says would break every site that has changed encoding since.
+    def document(link, entry)
+      page, declared = Automatic::Http.open(link) { |io| [io.read, declared_charset?(io)] }
+      parsed = Nokogiri::HTML.parse(StringIO.new(page))
+      return parsed if declared || parsed.meta_encoding || entry.encoding.nil?
+
+      Nokogiri::HTML.parse(StringIO.new(page), nil, entry.encoding)
+    end
+
+    # Whether the response itself named a charset, as opposed to open-uri
+    # having settled on one in the absence of an answer.
+    def declared_charset?(io)
+      return false unless io.respond_to?(:meta)
+
+      io.meta['content-type'].to_s.match?(/;\s*charset\s*=/i)
+    end
+
+    # An encoding named by a siteinfo record. An unknown name is ignored rather
+    # than raised, and the page is left to speak for itself.
+    def charset(name)
+      string = name.to_s.strip
+      return nil if string.empty?
+
+      Encoding.find(string).name
+    rescue ArgumentError
+      nil
     end
   end
 end