From f5251cf3951e90a03c96efac6614f761bccd6e9f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 11:21:05 +0000 Subject: [PATCH] Make FilterFullFeed work against the sites it names The filter did nothing on almost every feed, and where it did something it could make the item worse than it found it. Three separate causes, all of them consequences of the shipped siteinfo being a 2013 snapshot of a database that can no longer be refreshed. A link was matched against a record's pattern exactly as the feed gave it. 3,448 of the 3,504 usable records anchor on a scheme and all but twenty of those say `^http://`, while the sites they name have since moved to HTTPS, so an ordinary feed today matched nothing at all and every item was logged as "Fulltext SITEINFO not found". A record describes a site's layout, not how it is transported, so the link is now tried under either scheme; only the match is rewritten, and the page is still fetched from the link the feed gave. Against the shipped database, https links to gigazine.net, blog.livedoor.jp and webcre8.jp reach their records again instead of missing. Where a record did match but its XPath selected nothing -- a site redesigned since the XPath was written -- the empty result was assigned anyway, so the item lost the summary it arrived with and the feed went out with an empty body. The summary is now kept, and the miss is logged at warn with the XPath that missed. Page encoding was read from a decoded string. `Automatic::Http.read` returns what 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 -- and a parser given that string believes the tag and never looks at the meta charset underneath it, which is a whole article in mojibake for the many sites in this database that declare their charset only in a meta tag. The page is now handed to the parser as a stream, through `Automatic::Http.open`, so the parser detects the encoding for itself. A record's own `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. The result is converted to UTF-8 with invalid bytes replaced as well as undefined ones, because converting UTF-8 to UTF-8 leaves invalid bytes alone unless they are named, and what leaves here goes on to a publish plugin that cannot recover from a string it cannot encode. Records are now compiled when the file is loaded rather than per item, which was several thousand `Regexp.new` calls for every link in every feed, and a record with no URL pattern, no XPath or a pattern that is not a regular expression is dropped there -- an empty pattern matches every link, so such a record would have put its own XPath over the whole feed. The spec reached the network for the one case that mattered and pointed at a host that has not served what it expected for years, so none of the above was covered. Sixteen local examples cover it now: they write a siteinfo file into a temporary HOME and stand in for open-uri, and each of the fixes fails at least one of them when reverted. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RSLxro7DeKVdunYdYMyjvY --- doc/BASIC_DESIGN.md | 7 +- doc/PLUGINS.md | 36 ++++ plugins/filter/full_feed.rb | 129 ++++++++++++-- spec/plugins/filter/full_feed_spec.rb | 248 +++++++++++++++++++++++++- 4 files changed, 403 insertions(+), 17 deletions(-) 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