diff --git a/packages/db/src/accounts.js b/packages/db/src/accounts.js index d0ed394..fb86ac0 100644 --- a/packages/db/src/accounts.js +++ b/packages/db/src/accounts.js @@ -1,4 +1,5 @@ import { newId, nowIso } from './client.js'; +import { topicLabelSql } from './topicLabel.js'; /** * Everything the accounts layer reads and writes. @@ -518,7 +519,7 @@ export async function followedTopics(db, userId, limit = 200) { -- The topic's own spelling, so a page can say "AI" rather than -- "ai". Left join: a topic whose feeds have all died still has -- a follow, and the slug is a serviceable fallback. - (select min(k.keyword) from feed_keywords k where k.slug = tf.slug) as keyword + ${topicLabelSql('tf.slug')} as keyword from topic_follows tf where tf.user_id = ? order by tf.created_at desc diff --git a/packages/db/src/alerts.js b/packages/db/src/alerts.js index f68cd2c..5b852d0 100644 --- a/packages/db/src/alerts.js +++ b/packages/db/src/alerts.js @@ -1,5 +1,6 @@ import { newId, nowIso } from './client.js'; import { normalizeSegment } from './accounts.js'; +import { topicLabelSql } from './topicLabel.js'; /** * Alerts: the reads and writes behind being told about a new post. @@ -196,7 +197,7 @@ export async function alertingFollows(db, userId) { }), db.execute({ sql: `select tf.slug, tf.segment, - (select min(k.keyword) from feed_keywords k where k.slug = tf.slug) as keyword + ${topicLabelSql('tf.slug')} as keyword from topic_follows tf where tf.user_id = ? and tf.alerts = 1 order by tf.created_at desc`, @@ -628,7 +629,7 @@ export async function newItemsFromAlertedFeeds(db, userId, cursor, limit = 50) { export async function alertedTopics(db, userId, limit = 50) { const { rows } = await db.execute({ sql: `select tf.slug, tf.segment, - (select min(k.keyword) from feed_keywords k where k.slug = tf.slug) as keyword + ${topicLabelSql('tf.slug')} as keyword from topic_follows tf where tf.user_id = ? and tf.alerts = 1 order by tf.created_at desc diff --git a/packages/db/src/queries.js b/packages/db/src/queries.js index 9c7a007..29e64cd 100644 --- a/packages/db/src/queries.js +++ b/packages/db/src/queries.js @@ -1,6 +1,7 @@ import { clusterKey, dedupeItems, topicSlug } from '@rssamplifier/feed'; import { newId, nowIso } from './client.js'; +import { topicLabelSql } from './topicLabel.js'; /** * Every query the app needs, in one place. @@ -1092,12 +1093,10 @@ export async function keywordsForFeed(db, feedId, limit = 12) { export async function topicBySlug(db, slug) { const { rows } = await db.execute({ sql: `select k.slug, - -- Any spelling will do as the display name: extraction - -- lowercases its keywords and categories are lowercased before - -- they are stored, so the rows under one slug differ only in - -- ways the slug already erased. Same rule as the rollup, so - -- the index and the page always agree on the title. - min(k.keyword) as keyword, + -- The most-used spelling, not any spelling: see topicLabel.js. + -- Same rule as the rollup, so the index and the page always + -- agree on the title. + ${topicLabelSql('k.slug')} as keyword, count(*) as feed_count from feed_keywords k join feeds f on f.id = k.feed_id and f.status <> 'dead' @@ -1454,6 +1453,11 @@ export async function countTopics(db, minFeeds = 2, query = null) { * blog's vocabulary, they are the overwhelming majority of the rows, and a page * listing one feed is not a topic page. * + * The displayed keyword is the most-used spelling of the slug, chosen by the + * shared rule in `topicLabel.js` rather than by this query — the topic page, + * the alert queries and the followed-topics list all have to agree with it, and + * they did not when each picked its own. + * * @param {Client} db * @param {number} [minFeeds] * @returns {Promise} topics in the rollup @@ -1467,7 +1471,7 @@ export async function refreshTopics(db, minFeeds = 2) { { sql: `insert into topics (slug, keyword, feed_count, refreshed_at) select k.slug, - min(k.keyword), + ${topicLabelSql('k.slug')}, count(distinct k.feed_id), ? from feed_keywords k diff --git a/packages/db/src/topicLabel.js b/packages/db/src/topicLabel.js new file mode 100644 index 0000000..2073c05 --- /dev/null +++ b/packages/db/src/topicLabel.js @@ -0,0 +1,54 @@ +/** + * The one place that decides what a topic is *called*. + * + * Many raw category strings slug down to the same topic, so something has to + * pick the label from among them. Five queries used to pick it independently + * with `min(keyword)` — the rollup, the topic page, both alert queries and the + * followed-topics list — and all five were wrong in the same way. + * + * `min()` is a *lexicographic* minimum, and in ASCII `!`(0x21) `"`(0x22) + * `*`(0x2A) `.`(0x2E) `/`(0x2F) `[`(0x5B) and `_`(0x5F) all sort before + * lowercase letters. So whenever one publisher wrote a malformed `` + * tag, their spelling won the label for everybody. Measured on prod: + * + * slug `news` displayed as `! news` — 1 feed, against 10,310 spelling it `news` + * slug `en` displayed as `["en"]` — 1 feed, against 13,130 + * slug `ai` displayed as `"ai"` — 1 feed, against 11,051 + * slug `post` displayed as `_posts` — 1 feed, against 11,774 + * slug `life` displayed as `/life` — 4 feeds, against 8,985 + * + * The old comment on `topicBySlug` explained the choice as "any spelling will + * do … the rows under one slug differ only in ways the slug already erased". + * That is the premise that made this invisible, and it is false in exactly one + * direction: `topicSlug` strips punctuation, `feed_keywords.keyword` keeps it, + * so the surviving differences are precisely the ugly ones. + * + * Ranking by `count(distinct feed_id)` lets the directory's own usage decide. + * It needs no stoplist, it cannot be captured by a single feed, and it improves + * on its own as feeds are added. The tie-breaks handle what a count cannot: + * shortest first, so `ai` beats `ai,` and `ai:` at equal counts, then + * lexicographic so the result is deterministic. + * + * This is a display concern only. `topicSlug` already normalised these away, so + * URLs, grouping and counts were right the whole time — only the label on top + * of them was wrong. + */ + +/** + * A correlated scalar subquery returning the most-used spelling of one slug. + * + * `slugExpr` is interpolated as SQL, so it must be a column reference the + * caller controls (`k.slug`, `tf.slug`) — never a value, and never anything + * derived from user input. Values still go through bound parameters. + * + * @param {string} slugExpr SQL expression naming the slug to label + * @returns {string} SQL scalar subquery + */ +export function topicLabelSql(slugExpr) { + return `(select lk.keyword + from feed_keywords lk + where lk.slug = ${slugExpr} + group by lk.keyword + order by count(distinct lk.feed_id) desc, length(lk.keyword) asc, lk.keyword asc + limit 1)`; +} diff --git a/packages/db/test/topic-label.test.js b/packages/db/test/topic-label.test.js new file mode 100644 index 0000000..a5f55fc --- /dev/null +++ b/packages/db/test/topic-label.test.js @@ -0,0 +1,66 @@ +/** + * The label a topic wears is the spelling most feeds actually use. + * + * Regression test for the `min(keyword)` rollup: a lexicographic minimum let + * one publisher's malformed `` tag rename a topic carried by + * thousands of blogs, because `!`, `"`, `/`, `[` and `_` all sort before + * lowercase letters in ASCII. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtemp, rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { connect } from '../src/client.js'; +import { migrate } from '../src/migrate.js'; +import * as q from '../src/queries.js'; + +/** Add `n` feeds that all spell the same slug the same way. */ +async function feedsSpelling(db, slug, keyword, n, tag) { + for (let i = 0; i < n; i += 1) { + const feed = await q.insertFeed(db, { + slug: `${tag}-${i}`, + feed_url: `https://${tag}${i}.example/feed.xml`, + title: `${tag} ${i}`, + kind: 'blog', + }); + await q.replaceFeedKeywords(db, feed.id, [ + { slug, keyword, words: 1, count: 3, source: 'category' }, + ]); + } +} + +test('a punctuation-prefixed spelling from one feed cannot rename a topic', async () => { + const dir = await mkdtemp(join(tmpdir(), 'rssamp-topiclabel-')); + const db = connect({ url: `file:${join(dir, 'label.db')}` }); + await migrate(db); + + // The real prod shape, scaled down: many feeds say "news", exactly one says + // "! news", and both slug to `news`. + await feedsSpelling(db, 'news', 'news', 5, 'plain'); + await feedsSpelling(db, 'news', '! news', 1, 'punct'); + + await q.refreshTopics(db); + + const topic = await q.topicBySlug(db, 'news'); + assert.equal(topic.keyword, 'news', 'the majority spelling wins the label'); + assert.equal(topic.feedCount, 6, 'every spelling still counts toward the topic'); + + await rm(dir, { recursive: true, force: true }); +}); + +test('trailing punctuation loses to the bare word on a tie-break', async () => { + const dir = await mkdtemp(join(tmpdir(), 'rssamp-topictie-')); + const db = connect({ url: `file:${join(dir, 'tie.db')}` }); + await migrate(db); + + // Equal feed counts, so the count cannot decide: shortest must. + await feedsSpelling(db, 'ai', 'ai:', 2, 'colon'); + await feedsSpelling(db, 'ai', 'ai', 2, 'bare'); + + await q.refreshTopics(db); + assert.equal((await q.topicBySlug(db, 'ai')).keyword, 'ai'); + + await rm(dir, { recursive: true, force: true }); +});