Write valid URLs, hreflang codes and dates in every sitemap

README.md

@@ -69,6 +69,10 @@ Then tell crawlers where the index is, in `public/robots.txt`:
 Sitemap: https://www.example.com/sitemap.xml
 ```
 
+When the sitemaps are served from elsewhere (`config.sitemaps_url`, see [Storage](#storage)), that line names
+the index there: a sitemap on another host, or in a folder of the site, may list the pages of the site only
+when its `robots.txt` points to it.
+
 ## Sources
 
 A source is a class in `app/sitemaps`. Its blocks run in an object that has your route helpers, already set to
@@ -94,7 +98,10 @@ end
 ```
 
 A page in one language gets no `hreflang` link. A page that does not exist in the `x_default` language gets
-no `x-default` link.
+no `x-default` link. A Rails locale such as `pt_BR` is written `hreflang="pt-BR"`, and two languages that share
+a URL list it once.
+
+A URL your block writes by hand is percent-encoded: spaces, accents and control characters cannot break a file.
 
 ### When the date depends on more than one column
 
@@ -205,9 +212,15 @@ cannot be reached is a warning, never a failed build.
 bin/rails mapitaine:check
 ```
 
-It renders every sitemap without publishing or submitting anything, and fails on a URL outside `config.host`,
-a URL listed twice in a source, or a `lastmod` in the future. A build reports the same, for the files it
-rewrites, in `warnings`.
+It renders every sitemap without publishing or submitting anything, and fails on:
+
+- a URL outside `config.host`, listed twice in a source, or longer than 2,048 characters, and a page whose
+  languages all share one URL;
+- an `hreflang` that is not a language code (`en`, `pt-BR`, `zh-Hant`, `es-419`);
+- a `lastmod` in the future, or one that is not a W3C date, which is then left out;
+- an index with more than the 50,000 files it may hold.
+
+A build reports the same, for the files it rewrites, in `warnings`.
 
 ## Instrumentation
 

docs/guide.html

@@ -61,6 +61,7 @@ <h2 id="installation-title">Installation</h2>
 <span class="k">end</span></code></pre>
         <p>Then tell crawlers where the index is, in <code>public/robots.txt</code>:</p>
 <pre><code class="language-text">Sitemap: https://www.example.com/sitemap.xml</code></pre>
+        <p>When the sitemaps are served from elsewhere (<code>config.sitemaps_url</code>, see <a href="#storage">Storage</a>), that line names the index there: a sitemap on another host, or in a folder of the site, may list the pages of the site only when its <code>robots.txt</code> points to it.</p>
       </div>
     </section>
 
@@ -98,7 +99,8 @@ <h2 id="languages-title">Languages</h2>
   <span class="n">url</span> <span class="p">{</span> <span class="o">|</span><span class="n">product</span><span class="p">,</span> <span class="n">locale</span><span class="o">|</span> <span class="n">product_url</span><span class="p">(</span><span class="n">product</span><span class="p">,</span> <span class="n">locale</span><span class="p">:)</span> <span class="p">}</span>
   <span class="n">locales</span> <span class="p">{</span> <span class="o">|</span><span class="n">product</span><span class="o">|</span> <span class="n">product</span><span class="p">.</span><span class="nf">translated_locales</span> <span class="p">}</span>
 <span class="k">end</span></code></pre>
-        <p>Each <code>&lt;url&gt;</code> then lists every language of its page, itself included, as <code>hreflang</code> links. A page in one language gets no link. A page that does not exist in the <code>x_default</code> language gets no <code>x-default</code> link.</p>
+        <p>Each <code>&lt;url&gt;</code> then lists every language of its page, itself included, as <code>hreflang</code> links. A page in one language gets no link. A page that does not exist in the <code>x_default</code> language gets no <code>x-default</code> link. A Rails locale such as <code>pt_BR</code> is written <code>hreflang="pt-BR"</code>, and two languages that share a URL list it once.</p>
+        <p>A URL your block writes by hand is percent-encoded: spaces, accents and control characters cannot break a file.</p>
       </div>
     </section>
 
@@ -163,7 +165,7 @@ <h3>Through Active Storage</h3>
   <span class="n">config</span><span class="p">.</span><span class="nf">sitemaps_url</span> <span class="o">=</span> <span class="s2">"https://cdn.example.com/sitemaps"</span>   <span class="c1"># where those files are served from</span>
 <span class="k">end</span></code></pre>
         <p>No blob is created: the files keep their names as keys, and nothing is written to the database. Any service works that serves a key at a stable URL: S3, Google Cloud Storage, Azure, a mirror of them. The Disk service does not, it scatters keys in subfolders: keep the default for files on disk.</p>
-        <p>Sitemaps served from another host than the pages they list are accepted when <code>robots.txt</code> names the index.</p>
+        <p>Sitemaps served from another host than the pages they list are accepted when the site's <code>robots.txt</code> names the index there.</p>
         <h3>Your own</h3>
         <p>A storage is any object with <code>read(name)</code>, <code>write(name, path)</code> and <code>delete(name)</code>.</p>
       </div>
@@ -183,7 +185,14 @@ <h2 id="indexnow-title">IndexNow</h2>
       <h2 id="checking-title">Checking</h2>
       <div class="prose">
 <pre><code class="language-sh">bin/rails mapitaine:check</code></pre>
-        <p>It renders every sitemap without publishing or submitting anything, and fails on a URL outside <code>config.host</code>, a URL listed twice in a source, or a <code>lastmod</code> in the future. A build reports the same in <code>warnings</code>, for the files it rewrites.</p>
+        <p>It renders every sitemap without publishing or submitting anything, and fails on:</p>
+        <ul>
+          <li>a URL outside <code>config.host</code>, listed twice in a source, or longer than 2,048 characters, and a page whose languages all share one URL;</li>
+          <li>an <code>hreflang</code> that is not a language code (<code>en</code>, <code>pt-BR</code>, <code>zh-Hant</code>, <code>es-419</code>);</li>
+          <li>a <code>lastmod</code> in the future, or one that is not a W3C date, which is then left out;</li>
+          <li>an index with more than the 50,000 files it may hold.</li>
+        </ul>
+        <p>A build reports the same in <code>warnings</code>, for the files it rewrites.</p>
         <p>Mapitaine never requests a page of the site. Whether a URL answers 200 or carries a <code>noindex</code> is what Search Console reports, from the visits crawlers make anyway.</p>
       </div>
     </section>

lib/mapitaine.rb

@@ -4,10 +4,12 @@
 require "active_support/core_ext/object/blank"
 require "active_support/core_ext/string/inflections"
 require "active_support/notifications"
+require "date"
 require "digest"
 require "fileutils"
 require "json"
 require "net/http"
+require "time"
 require "tmpdir"
 require "uri"
 require "zlib"

lib/mapitaine/builder.rb

@@ -8,6 +8,12 @@ def changed?
       end
     end
 
+    # https://www.sitemaps.org/protocol.html
+    MAX_URL_CHARACTERS = 2_048
+    MAX_FILES_PER_INDEX = 50_000
+    # A language, then maybe a script and a region: en, pt-BR, zh-Hant-TW, es-419.
+    HREFLANG = /\A[a-z]{2,3}(-[a-z]{4})?(-([a-z]{2}|\d{3}))?\z/i
+
     Warning = Data.define(:source, :url, :message) do
       def to_s
         "#{source}: #{url} #{message}"
@@ -37,6 +43,9 @@ def build_in(directory, full:)
       @kept = []
       @warnings = []
       @changed_urls = []
+      @checked_hreflangs = Set.new
+      @reported = Set.new
+      @x_default = Xml.hreflang(@configuration.x_default)
 
       @configuration.sources.each { |source_class| build_source(source_class.new(@configuration)) }
       publish_index
@@ -71,7 +80,7 @@ def render(source, shard, fingerprint, stored, seen_urls)
       ActiveSupport::Notifications.instrument("shard.mapitaine", source: source.slug, shard:) do
         source.each_page(shard) do |page|
           inspect_page(source, page, seen_urls)
-          writer.add(Xml.url_entries(page, x_default: @configuration.x_default), Xml.timestamp(page.lastmod))
+          writer.add(Xml.url_entries(page, x_default: @x_default), page.lastmod)
           changed_urls.concat(page.urls) if changed_since_last_build?(page)
         end
         writer.close
@@ -96,10 +105,12 @@ def publish(source, shard, fingerprint, files)
     end
 
     def publish_index
+      name = "#{@configuration.filename}.xml"
       entries = @manifest.files.map do |file|
         Xml.sitemap_entry("#{@configuration.sitemaps_url}/#{file["name"]}", file["lastmod"])
       end
-      upload("#{@configuration.filename}.xml", "#{Xml::INDEX_OPEN}#{entries.join}#{Xml::INDEX_CLOSE}")
+      inspect_index("#{@configuration.sitemaps_url}/#{name}", entries.size)
+      upload(name, "#{Xml::INDEX_OPEN}#{entries.join}#{Xml::INDEX_CLOSE}")
     end
 
     def upload(name, content)
@@ -116,19 +127,43 @@ def inspect_page(source, page, seen_urls)
       page.urls.each do |url|
         warn(source, url, "is outside #{@configuration.host!}") unless url.start_with?("#{@configuration.host!}/") || url == @configuration.host!
         warn(source, url, "is listed twice") unless seen_urls.add?(url)
+        warn(source, url, "is longer than #{MAX_URL_CHARACTERS} characters") if url.length > MAX_URL_CHARACTERS
+      end
+      page.url_by_locale.each do |hreflang, url|
+        warn(source, url, "has hreflang #{hreflang.inspect}, not a language code") if @checked_hreflangs.add?(hreflang) && !hreflang.match?(HREFLANG)
+      end
+      if page.url_by_locale.size > 1 && page.urls.one?
+        warn_once(source, :one_url, page.urls.first, "is the URL of every language of its page: does the url block use the locale?")
+      end
+      inspect_lastmod(source, page)
+    end
+
+    def inspect_lastmod(source, page)
+      if page.rejected_lastmod
+        warn_once(source, :rejected_lastmod, page.urls.first, "has a lastmod that is not a W3C date (#{page.rejected_lastmod.inspect}), left out")
+      elsif page.modified_at && page.modified_at > @manifest.built_at
+        warn(source, page.urls.first, "has a lastmod in the future (#{page.lastmod})")
       end
-      modified_at = page.modified_at
-      warn(source, page.urls.first, "has a lastmod in the future (#{Xml.timestamp(page.lastmod)})") if modified_at && modified_at > @manifest.built_at
+    end
+
+    def inspect_index(url, file_count)
+      return if file_count <= MAX_FILES_PER_INDEX
+
+      warn("index", url, "lists #{file_count} files, more than #{MAX_FILES_PER_INDEX}: raise shard_size")
+    end
+
+    # For a mistake every page of a source would repeat.
+    def warn_once(source, kind, url, message)
+      warn(source, url, message) if @reported.add?([source.slug, kind])
     end
 
     def warn(source, url, message)
-      @warnings << Warning.new(source: source.slug, url:, message:)
+      @warnings << Warning.new(source: source.is_a?(Source) ? source.slug : source, url:, message:)
     end
 
     # The first build has no "before": submitting a whole site at once is not what IndexNow is for.
     def changed_since_last_build?(page)
-      modified_at = page.modified_at
-      @previous.built_at && modified_at && modified_at >= @previous.built_at
+      @previous.built_at && page.modified_at && page.modified_at >= @previous.built_at
     end
 
     def submit_changed_urls

lib/mapitaine/page.rb

@@ -1,14 +1,11 @@
 # frozen_string_literal: true
 
 module Mapitaine
-  # One page of the site: the same content at one URL per language.
-  Page = Data.define(:url_by_locale, :lastmod, :images) do
+  # One page of the site: the same content at one URL per language, keyed by hreflang. lastmod is a W3C date;
+  # rejected_lastmod what a source gave that was not one.
+  Page = Data.define(:url_by_locale, :lastmod, :modified_at, :images, :rejected_lastmod) do
     def urls
-      url_by_locale.values
-    end
-
-    def modified_at
-      lastmod.to_time if lastmod.respond_to?(:to_time)
+      url_by_locale.values.uniq
     end
   end
 end

lib/mapitaine/source.rb

@@ -16,6 +16,11 @@ class Source
     SLUG_FORMAT = /\A[a-z0-9][a-z0-9_-]*\z/
     # A source that is not an Active Record relation is one shard, rendered at each build.
     WHOLE = "all"
+    # What RFC 3986 lets a URL hold as is. Anything else is percent-encoded, byte by byte.
+    URL_CHARACTERS = "A-Za-z0-9\\-._~:/?#\\[\\]@!$&'()*+,;=%"
+    UNSAFE = "[^#{URL_CHARACTERS}]|%(?![0-9A-Fa-f]{2})"
+    UNSAFE_CHARACTER = Regexp.new(UNSAFE)
+    UNSAFE_BYTE = Regexp.new(UNSAFE, Regexp::NOENCODING)
 
     class << self
       def inherited(subclass)
@@ -144,13 +149,29 @@ def page_for(record)
       locales = definitions[:locales] ? Array(instance_exec(record, &definitions[:locales])) : configuration.locales
       url_by_locale = locales.filter_map do |locale|
         location = instance_exec(record, locale, &definitions.fetch(:url))
-        [locale, absolute(location)] if location
+        [Xml.hreflang(locale), absolute(location)] if location
       end
-      Page.new(url_by_locale: url_by_locale.to_h, lastmod: lastmod_of(record), images: images_of(record))
+      given_lastmod = lastmod_of(record)
+      lastmod = Xml.timestamp(given_lastmod)
+      Page.new(url_by_locale: url_by_locale.to_h, lastmod:, modified_at: lastmod && Xml.time(lastmod), images: images_of(record),
+        rejected_lastmod: (given_lastmod if lastmod.nil? && given_lastmod.present?))
     end
 
     def absolute(location)
-      location.start_with?("/") ? "#{configuration.host!}#{location}" : location
+      location = location.to_s
+      if location.start_with?("//")
+        location = "#{configuration.host!.split("//", 2).first}#{location}"
+      elsif location.start_with?("/")
+        location = "#{configuration.host!}#{location}"
+      end
+      encoded(location)
+    end
+
+    # Route helpers encode their URLs already; a URL written by hand may hold spaces or accents.
+    def encoded(url)
+      return url if url.ascii_only? && !url.match?(UNSAFE_CHARACTER)
+
+      url.b.gsub(UNSAFE_BYTE) { |byte| format("%%%02X", byte.ord) }.force_encoding(Encoding::UTF_8)
     end
 
     def lastmod_of(record)

lib/mapitaine/tasks.rake

@@ -17,7 +17,7 @@ namespace :mapitaine do
     report.call(Mapitaine.build(full: true))
   end
 
-  desc "Render every sitemap without publishing it, and fail on duplicate, foreign or future-dated URLs"
+  desc "Render every sitemap without publishing it, and fail on what looks wrong in it"
   task check: :environment do
     warnings = Mapitaine.check
     abort warnings.join("\n") if warnings.any?

lib/mapitaine/xml.rb

@@ -11,6 +11,9 @@ module Xml
     INDEX_CLOSE = "</sitemapindex>\n"
     # Google reads no more than 1,000 images per URL.
     MAX_IMAGES_PER_URL = 1_000
+    # https://www.w3.org/TR/NOTE-datetime: a year, a month, a day, or a time with its zone.
+    W3C_DATE = /\A(\d{4})(?:-(\d{2})(?:-(\d{2}))?)?\z/
+    W3C_TIME = /\A(\d{4})-(\d{2})-(\d{2})T(\d{2}):(\d{2})(?::(\d{2})(?:\.\d+)?)?(Z|[+-]\d{2}:\d{2})\z/
 
     module_function
 
@@ -25,27 +28,58 @@ def sitemap_entry(url, lastmod)
       "<sitemap><loc>#{escape(url)}</loc>#{lastmod_tag(lastmod)}</sitemap>\n"
     end
 
+    # The W3C date or time of a lastmod, in UTC, or nil when it is not one.
     def timestamp(value)
-      return if value.nil?
-      return value.utc.iso8601 if value.respond_to?(:utc)
+      case value
+      when nil then nil
+      when DateTime then value.to_time.getutc.iso8601
+      when Date then value.iso8601
+      when String then w3c_timestamp(value)
+      else value.getutc.iso8601 if value.respond_to?(:getutc)
+      end
+    end
+
+    def w3c_timestamp(text)
+      if (date = W3C_DATE.match(text))
+        year, month, day = date.captures
+        text if Date.valid_date?(year.to_i, (month || 1).to_i, (day || 1).to_i)
+      elsif (time = W3C_TIME.match(text))
+        year, month, day, hour, minute, second, zone = time.captures
+        return unless Date.valid_date?(year.to_i, month.to_i, day.to_i)
+
+        Time.new(year.to_i, month.to_i, day.to_i, hour.to_i, minute.to_i, second.to_i, (zone == "Z") ? "UTC" : zone).getutc.iso8601
+      end
+    rescue ArgumentError
+      nil
+    end
+
+    # A date alone is the day as it starts first, at UTC+14: no time zone has it in the future after that.
+    def time(stamp)
+      return Time.iso8601(stamp) if stamp.include?("T")
+
+      year, month, day = stamp.split("-").map(&:to_i)
+      Time.new(year, month || 1, day || 1, 0, 0, 0, "+14:00")
+    end
 
-      value.respond_to?(:iso8601) ? value.iso8601 : value.to_s
+    # Rails locales spell regions with an underscore (pt_BR), hreflang with a hyphen (pt-BR).
+    def hreflang(locale)
+      locale.to_s.tr("_", "-")
     end
 
     def escape(text)
       text.to_s.encode(xml: :text).gsub('"', "&quot;")
     end
 
-    def lastmod_tag(value)
-      stamp = timestamp(value)
+    def lastmod_tag(stamp)
       stamp ? "<lastmod>#{escape(stamp)}</lastmod>" : ""
     end
 
     def alternates(page, x_default)
       return "" if page.url_by_locale.size < 2
 
       links = page.url_by_locale.map { |locale, url| alternate(locale, url) }
-      links << alternate("x-default", page.url_by_locale[x_default]) if page.url_by_locale.key?(x_default)
+      default_url = page.url_by_locale[x_default]
+      links << alternate("x-default", default_url) if default_url
       links.join
     end