Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ARCHITECTURE.md
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@
- **anchors(0.13: `anchor_edges`)の全走査は `compute-anchor-fact-rows.ts` のみに許す** — build は id 範囲パーティションの `AsyncGenerator` で chunk 化し、chunk 跨ぎの `referrer_count` 合算は JS の Map で持ち回らず SQLite の `ON CONFLICT ... DO UPDATE` に委譲する(どちらも大規模 archive の OOM 回避。`derive-external-link-summary-rows.ts`、`upsert-external-link-rows.ts`)
- **`page_url_rank` は URL テキストを複製しない** — read model ファミリーで唯一の例外(`viewer_images` は数百万行規模になりうるため)。順位比較は SQLite BINARY 照合に合わせた `compareUrlBinary` を使う(素朴な JS 文字列比較は補助面文字で食い違う。`query/src/viewer-read-model/build-page-url-rank-map.ts`)
- **fast path の強制 legacy 条件は関数ごとに異なる** — pages/resources は `urlPattern`/`directory`、broken links は `urlPattern`/`includeRedirectSources`、headers/mismatches は明示 `sortBy`、duplicates のみ強制 legacy なし。各 `get-*-fast-path.ts` の JSDoc が正。duplicates の legacy 経路は実 `group_id` と衝突しない負の sentinel `-(index+1)` を採番し、route は `groupId <= 0` を 404 にする
- **既存アーカイブへの列追加は通常 version-gated migration script の役割だが、書き込みパスが単一で nullable な追加列に限り自己修復してよい**
- **`REQUIRED_FORMAT_VERSION` は pkg.version と別値** — crawler が `info.version` に書くのはフォーマット cut であって npm リリース番号ではない。format-breaking な変更はリリース前でも先行 bump し、format cut の無いリリースでは据え置く(`crawler/src/archive/meta/assert-compatible-version.ts`)
- **anchor_edges は `UNIQUE(page_id, href_page_id)` で dedup し、first-wins で集約する** — 同一宛先への複数アンカー(header と footer の同一リンク等)は 1 行に集約され、`count` が観測数・`first_hash` / `first_text_id` が最初の 1 件の識別を持つ。2 件目以降の textContent はスキーマ上保持されない(意図的な設計。`create-entity-tables.ts`)。**per-instance のアンカー明細を前提にする consumer(Sheets の Discrepancies 等)は first-wins の 1 行しか受け取れない**点に注意。`image_items` は逆に UNIQUE 制約なし(1 出現 = 1 行)で再スクレイプ時に置き換える
- **テーブルは「鮮度をどう保証するか」で 3 種に分類できる** — 再クロール(`--append` / `--retry-failed` / `--inventory`)後にどこまで鮮度が保証されるかはこの分類だけで決まる。各項目は「これは何か(言い換え)→ 鮮度の保ち方(特性)→ なぜその設計か(理由)」の順で読む
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,9 @@ describe('analyze-markuplint plugin', () => {
rule: 'attr-duplication',
code: '',
message: 'Duplicate attribute',
url: 'https://example.com/page (5:10)',
url: 'https://example.com/page',
line: 5,
col: 10,
},
],
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,9 @@ export default definePlugin((options: Options) => {
rule: report.ruleId,
code: '',
message: report.message,
url: `${url.href} (${report.line}:${report.col})`,
url: url.href,
line: report.line,
col: report.col,
};
});
return {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,9 @@ describe('analyze-textlint plugin', () => {
rule: 'no-nfd',
code: '-',
message: 'Found NFD character',
url: 'https://example.com/page (3:10)',
url: 'https://example.com/page',
line: 3,
col: 10,
},
]);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,9 @@ export default definePlugin((options: Options) => {
rule: r.ruleId,
code: '-',
message: `${r.message}`,
url: `${report.url} (${r.line}:${r.column})`,
url: report.url,
line: r.line,
col: r.column,
};
});
});
Expand Down
2 changes: 2 additions & 0 deletions packages/@nitpicker/crawler/src/archive/archive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,8 @@ export default class Archive extends ArchiveAccessor {
code?: string | null;
message: string;
url: string;
line?: number | null;
col?: number | null;
}[],
): Promise<void> {
await this.#db.replaceAnalysisViolations(violations);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,12 @@ describe('createAdjunctTables', () => {
expect(await db.schema.hasColumn('page_tags', 'marker')).toBe(true);
});

it('gives analysis_violations nullable line/col columns on fresh creation', async () => {
await createAdjunctTables(db);
expect(await db.schema.hasColumn('analysis_violations', 'line')).toBe(true);
expect(await db.schema.hasColumn('analysis_violations', 'col')).toBe(true);
});

it('enforces the content_items FK on insert (PRAGMA foreign_keys = ON)', async () => {
await db.raw('PRAGMA foreign_keys = ON');
await createAdjunctTables(db);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* - `page_html_blobs` + `page_html_ref` — content-addressable HTML
* snapshots, FK → `content_items(id)`
*
* The DDL is shared between fresh-archive provisioning ({@link initSchema}

Check warning on line 19 in packages/@nitpicker/crawler/src/archive/create-adjunct-tables.ts

View workflow job for this annotation

GitHub Actions / lint

The type 'initSchema' is undefined
* calls this right after `createEntityTables`) and the migration script
* (`scripts/migrate-to-0.13.mjs` calls it before retargeting FK
* declarations and dropping the legacy tables). Keeping the schema in one
Expand Down Expand Up @@ -186,7 +186,9 @@
code_text_id integer references analysis_text_refs(id),
page_url_sort_key text not null,
message_sort_key text not null,
code_sort_key text not null
code_sort_key text not null,
line integer,
col integer
)
`);
await instance.raw(
Expand Down
2 changes: 2 additions & 0 deletions packages/@nitpicker/crawler/src/archive/database.ts
Original file line number Diff line number Diff line change
Expand Up @@ -612,6 +612,8 @@ export class Database extends EventEmitter<DatabaseEvent> {
code?: string | null;
message: string;
url: string;
line?: number | null;
col?: number | null;
}[],
): Promise<void> {
return emitErrorAndRetry(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,97 @@ describe('replaceAnalysisViolations', () => {
).rejects.toThrow(/could not resolve 1 page URL/);
});

it('persists explicit line/col fields as-is without touching the url', async () => {
await seedContentItem(db, 'https://example.com/');
await replaceAnalysisViolations(db, [
{
validator: 'markuplint',
severity: 'error',
rule: 'required-attr',
message: 'Missing alt attribute',
url: 'https://example.com/',
line: 5,
col: 10,
},
]);
const rows = await db('analysis_violations').select('*');
expect(rows).toMatchObject([
{ page_url_sort_key: 'https://example.com/', line: 5, col: 10 },
]);
});

it('repairs a legacy corrupted url by splitting off the trailing (line:col)', async () => {
await seedContentItem(db, 'https://example.com/');
await replaceAnalysisViolations(db, [
{
validator: 'markuplint',
severity: 'error',
rule: 'required-attr',
message: 'Missing alt attribute',
url: 'https://example.com/ (5:10)',
},
]);
const rows = await db('analysis_violations').select('*');
expect(rows).toMatchObject([
{ page_url_sort_key: 'https://example.com/', line: 5, col: 10 },
]);
});

it('leaves violations without line/col or a corrupted-url suffix untouched (e.g. axe)', async () => {
await seedContentItem(db, 'https://example.com/');
await replaceAnalysisViolations(db, [
{
validator: 'axe',
severity: 'error',
rule: 'label',
message: 'Missing label',
url: 'https://example.com/',
},
]);
const rows = await db('analysis_violations').select('*');
expect(rows).toMatchObject([
{ page_url_sort_key: 'https://example.com/', line: null, col: null },
]);
});

it('self-heals an existing analysis_violations table that predates the line/col columns', async () => {
// Simulate an already-0.13 archive whose analysis_violations table was
// provisioned before issue #225 added line/col.
await db.raw('DROP TABLE analysis_violations');
await db.raw(`
CREATE TABLE analysis_violations (
id integer primary key,
page_id integer not null references content_items(id),
validator text not null,
severity text not null,
rule text not null,
message_text_id integer not null references analysis_text_refs(id),
code_text_id integer references analysis_text_refs(id),
page_url_sort_key text not null,
message_sort_key text not null,
code_sort_key text not null
)
`);
await seedContentItem(db, 'https://example.com/');

await replaceAnalysisViolations(db, [
{
validator: 'markuplint',
severity: 'error',
rule: 'required-attr',
message: 'Missing alt attribute',
url: 'https://example.com/ (5:10)',
},
]);

expect(await db.schema.hasColumn('analysis_violations', 'line')).toBe(true);
expect(await db.schema.hasColumn('analysis_violations', 'col')).toBe(true);
const rows = await db('analysis_violations').select('*');
expect(rows).toMatchObject([
{ page_url_sort_key: 'https://example.com/', line: 5, col: 10 },
]);
});

it('deduplicates message and code text through analysis_text_refs', async () => {
await seedContentItem(db, 'https://example.com/');
await seedContentItem(db, 'https://example.com/a');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,62 @@ import { createHash } from 'node:crypto';

import { eachSplitted } from '../../../utils/array/each-splitted.js';

/**
* Matches the position suffix that `@nitpicker/analyze-markuplint` and
* `@nitpicker/analyze-textlint` used to append to `Violation.url` before
* issue #225 (e.g. `https://example.com/page (5:10)`). Anchored to the end
* of the string so a URL that legitimately contains a similar-looking
* substring earlier on is not mistaken for the suffix.
*/
const LEGACY_CORRUPTED_URL_PATTERN = /^(.+) \((\d+):(\d+)\)$/;

/**
* Splits a legacy corrupted `Violation.url` (page URL + trailing
* `" (line:col)"`) back into a clean URL and its position, or returns
* `null` when `url` does not match the corrupted shape (e.g. a clean axe
* URL, or a URL that already carries explicit `line`/`col`).
* @param url - The `Violation.url` value as read from storage or input.
* @returns The recovered `{ url, line, col }`, or `null` when `url` is not corrupted.
*/
function parseLegacyCorruptedUrl(
url: string,
): { url: string; line: number; col: number } | null {
const match = LEGACY_CORRUPTED_URL_PATTERN.exec(url);
if (!match) {
return null;
}
const [, cleanUrl, line, col] = match;
return { url: cleanUrl!, line: Number(line), col: Number(col) };
}

/**
* Adds the `line`/`col` columns to `analysis_violations` when they are
* missing, i.e. when the archive's table was provisioned before issue #225.
*
* `create-adjunct-tables.ts` never mutates a table it finds already
* present — catching up an *existing* table normally requires a
* version-gated migration script (see `scripts/migrate-to-0.13.mjs`). This
* function relies on the exception documented in ARCHITECTURE.md's
* invariants list: a nullable, additive column on a table with a single
* write path may self-heal here without a version bump, because
* `replaceAnalysisViolations` is that single write path for
* `analysis_violations`. Runs before the transaction below so the DDL is
* not mixed with the DML rewrite.
* @param knex - Knex query builder connected to the archive DB.
*/
async function ensureLineColColumns(knex: Knex): Promise<void> {
if (!(await knex.schema.hasColumn('analysis_violations', 'line'))) {
await knex.schema.alterTable('analysis_violations', (table) => {
table.integer('line').nullable();
});
}
if (!(await knex.schema.hasColumn('analysis_violations', 'col'))) {
await knex.schema.alterTable('analysis_violations', (table) => {
table.integer('col').nullable();
});
}
}

/**
* Replaces the stored analysis violations with a freshly generated set.
*
Expand All @@ -12,6 +68,12 @@ import { eachSplitted } from '../../../utils/array/each-splitted.js';
* `analysis_text_refs`, and rewrites `analysis_violations` in one
* transaction. This is the storage-side counterpart of the query-layer
* `getViolations` read path.
*
* Violations whose `url` still carries the pre-#225 corrupted position
* suffix (and that do not already carry explicit `line`/`col`) are repaired
* in place: {@link parseLegacyCorruptedUrl} splits the suffix off before URL
* resolution, so both freshly-analyzed and legacy-JSON-backfilled data end
* up in the same clean shape.
* @param knex - Knex query builder connected to the archive DB.
* @param violations - Flat violation list from the analyze phase.
*/
Expand All @@ -24,16 +86,28 @@ export async function replaceAnalysisViolations(
code?: string | null;
message: string;
url: string;
line?: number | null;
col?: number | null;
}[],
): Promise<void> {
await ensureLineColColumns(knex);
const repaired = violations.map((violation) => {
if (violation.line != null || violation.col != null) {
return violation;
}
const parsed = parseLegacyCorruptedUrl(violation.url);
return parsed
? { ...violation, url: parsed.url, line: parsed.line, col: parsed.col }
: violation;
});
await knex.transaction(async (trx) => {
await trx('analysis_violations').delete();
await trx('analysis_text_refs').delete();
if (violations.length === 0) {
if (repaired.length === 0) {
return;
}

const urls = [...new Set(violations.map((v) => v.url))];
const urls = [...new Set(repaired.map((v) => v.url))];
const pageIdByUrl = new Map<string, number>();
await eachSplitted(urls, 500, async (chunk) => {
const pageRows = await trx('content_items')
Expand Down Expand Up @@ -86,8 +160,10 @@ export async function replaceAnalysisViolations(
page_url_sort_key: string;
message_sort_key: string;
code_sort_key: string;
line: number | null;
col: number | null;
}> = [];
for (const violation of violations) {
for (const violation of repaired) {
const pageId = pageIdByUrl.get(violation.url);
if (!pageId) {
throw new Error(
Expand All @@ -103,6 +179,8 @@ export async function replaceAnalysisViolations(
severity: violation.severity,
rule: violation.rule,
message_text_id: messageTextId,
line: violation.line ?? null,
col: violation.col ?? null,
code_text_id: codeTextId,
page_url_sort_key: violation.url,
message_sort_key: violation.message,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ describe('retargetLegacyFkTables', () => {
{ pageId: 1, kind: 'json-ld', type: 'Article' },
]);
expect(await db('analysis_violations').select('*')).toMatchObject([
{ page_id: 1, validator: 'axe', rule: 'label' },
{ page_id: 1, validator: 'axe', rule: 'label', line: null, col: null },
]);
const htmlRefs = await db('page_html_ref').select('*');
expect(htmlRefs).toHaveLength(1);
Expand Down Expand Up @@ -173,6 +173,20 @@ describe('retargetLegacyFkTables', () => {
).rejects.toThrow(/FOREIGN KEY constraint failed/);
});

it('aborts when a staged table is missing a column outside the nullable-on-retarget allowlist', async () => {
// Unlike analysis_violations.line/col, this column loss is not on the
// allowlist — it must still fail loudly rather than silently null-fill.
await db.raw('ALTER TABLE page_errors DROP COLUMN "createdAt"');
await seedPageAndContentItem(db, 1, 'https://example.com/');
await db('page_errors').insert({ pageId: 1, phase: 'render', message: 'boom' });

await expect(
db.transaction(async (trx) => {
await retargetLegacyFkTables(trx);
}),
).rejects.toThrow(/no such column/);
});

it('produces the same index name set as a fresh archive', async () => {
await db.transaction(async (trx) => {
await retargetLegacyFkTables(trx);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,23 @@ const RETARGET_TABLES: readonly string[] = [
'analysis_violations',
];

/**
* Canonical columns that a staged (pre-migration) table may legitimately
* lack because {@link createAdjunctTables}'s DDL gained them after the
* table was first lazily provisioned on the input archive (`analysis_violations`
* predates its `line`/`col` columns — issue #225). Missing columns listed
* here are copied back as `NULL` instead of aborting the migration; this is
* safe only because the column did not exist when the staged data was
* written, so `NULL` is the only value it could ever have had. Any other
* canonical column absent from the staged table still aborts the copy (see
* {@link retargetLegacyFkTables} JSDoc) — that case must stay loud, since a
* genuine rename/drop cannot be told apart from benign schema growth by
* column existence alone.
*/
const NULLABLE_ON_RETARGET: Readonly<Record<string, readonly string[]>> = {
analysis_violations: ['line', 'col'],
};

/**
* Rewrites the FK declarations of the adjunct tables from the legacy
* `pages(id)` to `content_items(id)` by rebuilding each table. SQLite has
Expand Down Expand Up @@ -80,9 +97,21 @@ export async function retargetLegacyFkTables(trx: Knex): Promise<void> {
const columns: { name: string }[] = await trx
.select('name')
.from(trx.raw('pragma_table_info(?)', [table]));
const columnList = columns.map((column) => `"${column.name}"`).join(', ');
const stagedColumns: { name: string }[] = await trx
.select('name')
.from(trx.raw('pragma_table_info(?)', [`${table}__retarget`]));
const stagedColumnNames = new Set(stagedColumns.map((column) => column.name));
const nullableOnRetarget = NULLABLE_ON_RETARGET[table] ?? [];
const insertColumnList = columns.map((column) => `"${column.name}"`).join(', ');
const selectColumnList = columns
.map((column) =>
!stagedColumnNames.has(column.name) && nullableOnRetarget.includes(column.name)
? `NULL AS "${column.name}"`
: `"${column.name}"`,
)
.join(', ');
await trx.raw(
`INSERT INTO "${table}" (${columnList}) SELECT ${columnList} FROM "${table}__retarget"`,
`INSERT INTO "${table}" (${insertColumnList}) SELECT ${selectColumnList} FROM "${table}__retarget"`,
);
await trx.raw(`DROP TABLE "${table}__retarget"`);
}
Expand Down
Loading
Loading