diff --git a/doc/BASIC_DESIGN.md b/doc/BASIC_DESIGN.md index ff69ef3..75a14eb 100644 --- a/doc/BASIC_DESIGN.md +++ b/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 diff --git a/doc/PLUGINS.md b/doc/PLUGINS.md index 2b4f033..b2db031 100644 --- a/doc/PLUGINS.md +++ b/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 diff --git a/plugins/filter/full_feed.rb b/plugins/filter/full_feed.rb index 3027abc..1235f68 100644 --- a/plugins/filter/full_feed.rb +++ b/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 diff --git a/spec/plugins/filter/full_feed_spec.rb b/spec/plugins/filter/full_feed_spec.rb index ce36e15..c4b05aa 100644 --- a/spec/plugins/filter/full_feed_spec.rb +++ b/spec/plugins/filter/full_feed_spec.rb @@ -5,7 +5,7 @@ # License:: The GPL version 3, or LGPL version 3 (Dual License). # Contact:: idnanashi@gmail.com # Created:: Jan 24, 2013 -# Updated:: Aug 14, 2026 +# Updated:: Aug 15, 2026 # Copyright:: Copyright (c) 2012-2026 Automatic Ruby Developers. require File.expand_path(File.dirname(__FILE__) + '../../../spec_helper') @@ -18,6 +18,252 @@ require 'filter/full_feed' require 'fileutils' require 'tmpdir' +require 'json' +require 'stringio' + +# The contexts below reach no network: the siteinfo is written into a +# temporary HOME, and Automatic::Http.open yields what open-uri would have +# yielded. Everything this plugin gets wrong, it gets wrong between a link and +# a description, which is exactly what a local double can hold still. +module FullFeedSpec + module_function + + # A siteinfo file of the shape the LDRFullFeed database has, in a temporary + # ~/.automatic/assets/siteinfo, which is where the plugin looks first. + def write_siteinfo(home, records) + dir = File.join(home, '.automatic', 'assets', 'siteinfo') + FileUtils.mkdir_p(dir) + File.write(File.join(dir, 'test.json'), JSON.dump(records)) + end + + def record(url, xpath, enc = nil) + { 'data' => { 'type' => 'IND', 'url' => url, 'xpath' => xpath, 'enc' => enc.to_s } } + end + + # What open-uri hands a caller: the bytes of the page, tagged with the + # encoding open-uri settled on, which is the charset the response declared + # or UTF-8 where it declared none. The distinction is the point of several + # of these examples, so the double keeps it. + def response(body, content_type) + charset = content_type[/charset\s*=\s*([\w-]+)/i, 1] + io = StringIO.new(body.dup.force_encoding(charset || 'UTF-8')) + io.define_singleton_method(:meta) { { 'content-type' => content_type } } + io + end +end + +describe Automatic::Plugin::FilterFullFeed, 'without a network' do + let(:home) { Dir.mktmpdir('automatic-spec-home') } + let(:link) { 'http://example.com/article' } + let(:content_type) { 'text/html; charset=UTF-8' } + let(:page) do + '' \ + '

the whole article

' + end + + # One item, one record matching it by default. An example that wants + # something else overrides the `let` it needs. + let(:records) { [FullFeedSpec.record('^http://example\.com/', '//div[@class="entry"]')] } + let(:item) { subject.instance_variable_get(:@pipeline)[0].items[0] } + + # The pipeline generator's block is instance_eval'd, so `link` has to be a + # local here rather than the example group's method. + subject { + url = link + Automatic::Plugin::FilterFullFeed.new( + { 'siteinfo' => 'test.json' }, + AutomaticSpec.generate_pipeline { + feed { item url, 'a title', 'the summary the feed gave' } + }) + } + + before do + @real_home = ENV['HOME'] + ENV['HOME'] = home + FullFeedSpec.write_siteinfo(home, records) + Automatic::Http.stub(:open) { |_url, &block| block.call(FullFeedSpec.response(page, content_type)) } + end + + after do + ENV['HOME'] = @real_home + FileUtils.remove_entry(home) + end + + context "when the link matches a record" do + it "replaces the summary with the article" do + subject.run + item.description.should == '

the whole article

' + end + end + + # The database was last updated in 2013 and nearly every record in it is + # anchored on ^http://, while a feed today hands out https links. Matching + # under one scheme only is the difference between this plugin working and + # doing nothing at all. + context "when the link is https and the record is anchored on http" do + let(:link) { 'https://example.com/article' } + + it "still matches the record" do + subject.run + item.description.should == '

the whole article

' + end + end + + context "when the link is http and the record is anchored on https" do + let(:records) { [FullFeedSpec.record('^https://example\.com/', '//div[@class="entry"]')] } + + it "still matches the record" do + subject.run + item.description.should == '

the whole article

' + end + end + + # A record whose site has been redesigned selects nothing. Assigning that + # empty result is worse than doing nothing: the item loses the summary it + # arrived with and the feed goes out with an empty body. + context "when the record matches but its XPath selects nothing" do + let(:records) { [FullFeedSpec.record('^http://example\.com/', '//div[@class="gone"]')] } + + it "keeps the summary the feed gave" do + subject.run + item.description.should == 'the summary the feed gave' + end + end + + context "when the page cannot be read" do + before { Automatic::Http.stub(:open).and_raise(Errno::ECONNREFUSED) } + + it "keeps the summary the feed gave" do + subject.run + item.description.should == 'the summary the feed gave' + end + end + + context "when no record matches the link" do + let(:records) { [FullFeedSpec.record('^http://elsewhere\.example/', '//div')] } + + it "keeps the summary the feed gave" do + subject.run + item.description.should == 'the summary the feed gave' + end + end + + # An empty pattern matches every link, so a record without one would put its + # own XPath over the whole feed. + context "with a record that has no URL pattern" do + let(:records) do + [FullFeedSpec.record('', '//div[@class="entry"]'), + FullFeedSpec.record('^http://example\.com/', '//div[@class="entry"]')] + end + + it "ignores it" do + subject.instance_variable_get(:@siteinfo).size.should == 1 + end + end + + context "with a record whose pattern is not a regular expression" do + let(:records) { [FullFeedSpec.record('^http://example\.com/broken(', '//div')] } + + it "ignores it" do + subject.instance_variable_get(:@siteinfo).should be_empty + end + end + + context "with a record that has no XPath" do + let(:records) { [FullFeedSpec.record('^http://example\.com/', '')] } + + it "ignores it" do + subject.instance_variable_get(:@siteinfo).should be_empty + end + end + + # Encoding. open-uri tags a page with the charset of the response, and with + # UTF-8 when the response named none -- which is not the same as the page + # having said so. Handing the parser that tag instead of the page is how a + # site that declares its charset in a meta tag turns into mojibake. + context "when the page declares its charset only in a meta tag" do + let(:content_type) { 'text/html' } + let(:page) do + ('' \ + '
日本語の本文
').encode('Shift_JIS') + end + + it "reads the page in the encoding the page names" do + subject.run + item.description.should == '
日本語の本文
' + item.description.encoding.should == Encoding::UTF_8 + end + end + + context "when the page declares no charset anywhere" do + let(:content_type) { 'text/html' } + let(:page) { '
日本語の本文
'.encode('EUC-JP') } + let(:records) { [FullFeedSpec.record('^http://example\.com/', '//div[@class="entry"]', 'EUC-JP')] } + + it "falls back to the encoding the record names" do + subject.run + item.description.should == '
日本語の本文
' + end + end + + # A record's `enc` was written in 2013. A site that has moved to UTF-8 since + # says so in the page, and the page is the more recent of the two. + context "when the record names an encoding the page contradicts" do + let(:records) { [FullFeedSpec.record('^http://example\.com/', '//div[@class="entry"]', 'EUC-JP')] } + let(:page) do + '' \ + '
日本語の本文
' + end + + it "believes the page" do + subject.run + item.description.should == '
日本語の本文
' + end + end + + context "when the record names an encoding that does not exist" do + let(:records) { [FullFeedSpec.record('^http://example\.com/', '//div[@class="entry"]', 'NoSuchEncoding')] } + + it "ignores it rather than failing the run" do + subject.run + item.description.should == '

the whole article

' + end + end + + # Whatever comes out of here is put into a feed by a publish plugin, which + # has no way to recover from a string it cannot encode. The parser returns + # the page in whatever encoding it settled on, so the conversion happens + # here whether or not the page turned out to be readable. + context "when the page's bytes do not match the charset it declares" do + let(:page) do + '
日本語の本文
'.encode('Shift_JIS') + end + + it "still hands on valid UTF-8" do + subject.run + item.description.encoding.should == Encoding::UTF_8 + item.description.valid_encoding?.should be true + end + end + + describe "#run" do + it "passes the feeds on" do + subject.run.size.should == 1 + end + + it "leaves an item with no link alone" do + subject.instance_variable_get(:@pipeline)[0].items[0].link = nil + lambda { subject.run }.should_not raise_error + end + end + + describe "the siteinfo setting" do + it "is required" do + lambda { Automatic::Plugin::FilterFullFeed.new({}, []) }. + should raise_error(ArgumentError, /siteinfo/) + end + end +end describe Automatic::Plugin::FilterFullFeed do context "It should be matched by siteinfo", :network do