diff --git a/layers/registry/server/api/clusters/[slug].get.ts b/layers/registry/server/api/clusters/[slug].get.ts index d96b809c..4c1d1dfc 100644 --- a/layers/registry/server/api/clusters/[slug].get.ts +++ b/layers/registry/server/api/clusters/[slug].get.ts @@ -1,7 +1,7 @@ import type { Cluster } from '../../data/clusters' import { getDB } from '#server/utils/db' import { CLUSTER_BY_SLUG } from '../../data/clusters' -import { clusterMembersSql } from '../../utils/cluster-membership' +import { clusterPageSql } from '../../utils/cluster-membership' import { curateClusterSkills, parseClusterSkillKeys } from '../../utils/cluster-skill-curation' function clusterMeta(cluster: Cluster) { @@ -57,33 +57,20 @@ export default defineCachedEventHandler(async (event) => { } // Membership is abstract-first with a capped backfill; see cluster-membership. - const members = clusterMembersSql( + const statements = clusterPageSql( 'owner, name, repo, display_name, description, stars, modified_at, is_abstract', cluster.categories, - pinnedSkills.map(skill => skill.key), + pinnedSkills, + { limit, offset }, ) - const countRow = await db.prepare(`SELECT COUNT(*) AS n FROM (${members.sql})`) - .bind(...members.params) + const countRow = await db.prepare(statements.countSql) + .bind(...statements.countParams) .first<{ n: number }>() const total = countRow?.n ?? 0 - // `CASE` with no `WHEN` is a syntax error, so a category that ranks purely on - // stars drops the pinned-order arm rather than emitting an empty CASE. - const pinnedOrderSql = pinnedSkills.length - ? `CASE - ${pinnedSkills.map((_, index) => `WHEN owner = ? AND name = ? THEN ${index}`).join('\n')} - ELSE ${pinnedSkills.length} - END,` - : '' - const pinnedOrderParams = pinnedSkills.flatMap(skill => [skill.owner, skill.name]) - const listSql = ` - SELECT * FROM (${members.sql}) - ORDER BY ${pinnedOrderSql} is_abstract DESC, stars DESC, modified_at DESC, name ASC - LIMIT ? OFFSET ? - ` - const res = await db.prepare(listSql) - .bind(...members.params, ...pinnedOrderParams, limit, offset) + const res = await db.prepare(statements.listSql) + .bind(...statements.listParams) .all() const rankedItems = (res.results ?? []).map(s => ({ diff --git a/layers/registry/server/utils/cluster-index-rows.ts b/layers/registry/server/utils/cluster-index-rows.ts index 5b93d176..4545c75c 100644 --- a/layers/registry/server/utils/cluster-index-rows.ts +++ b/layers/registry/server/utils/cluster-index-rows.ts @@ -1,3 +1,5 @@ +import { D1_BOUND_PARAMETER_LIMIT } from './cluster-membership' + interface ClusterIndexRowIdentity { owner: string repo: string @@ -11,9 +13,6 @@ export type ClusterIndexQuery = ( values: string[], ) => Promise -// D1 accepts at most 100 bound parameters in one query. -const D1_BOUND_PARAMETER_LIMIT = 100 - function chunks(values: T[], size: number): T[][] { return Array.from( { length: Math.ceil(values.length / size) }, diff --git a/layers/registry/server/utils/cluster-membership.ts b/layers/registry/server/utils/cluster-membership.ts index b073a633..d3d6bb46 100644 --- a/layers/registry/server/utils/cluster-membership.ts +++ b/layers/registry/server/utils/cluster-membership.ts @@ -20,6 +20,11 @@ * clear the indexability bar. */ +import type { ParsedClusterSkillKey } from './cluster-skill-curation' + +/** D1 rejects a statement that binds more than this many parameters. */ +export const D1_BOUND_PARAMETER_LIMIT = 100 + /** Backfill depth per classifier category, not per track. */ export const CLUSTER_BACKFILL_PER_CATEGORY = 40 @@ -113,3 +118,57 @@ export function clusterMembersSql( return { sql, params } } + +export interface ClusterPageWindow { + limit: number + offset: number +} + +export interface ClusterPageSql { + countSql: string + countParams: (string | number)[] + listSql: string + listParams: (string | number)[] +} + +/** + * The two statements one `/skills/` page sends. + * + * They live here rather than in the route because the membership fragment is + * not what D1 counts. A pinned key is bound once in the SELECT `CASE`, once in + * the WHERE arm, and twice more in the ORDER BY `CASE`, so a guard on the + * fragment reads a quarter of the real parameter count. + */ +export function clusterPageSql( + columns: string, + categories: string[], + pinnedSkills: ParsedClusterSkillKey[], + pageWindow: ClusterPageWindow, +): ClusterPageSql { + const members = clusterMembersSql(columns, categories, pinnedSkills.map(skill => skill.key)) + + // `CASE` with no `WHEN` is a syntax error, so a category that ranks purely on + // stars drops the pinned-order arm rather than emitting an empty CASE. + const pinnedOrderSql = pinnedSkills.length + ? `CASE + ${pinnedSkills.map((_, index) => `WHEN owner = ? AND name = ? THEN ${index}`).join('\n')} + ELSE ${pinnedSkills.length} + END,` + : '' + + return { + countSql: `SELECT COUNT(*) AS n FROM (${members.sql})`, + countParams: [...members.params], + listSql: ` + SELECT * FROM (${members.sql}) + ORDER BY ${pinnedOrderSql} is_abstract DESC, stars DESC, modified_at DESC, name ASC + LIMIT ? OFFSET ? + `, + listParams: [ + ...members.params, + ...pinnedSkills.flatMap(skill => [skill.owner, skill.name]), + pageWindow.limit, + pageWindow.offset, + ], + } +} diff --git a/test/unit/cluster-page-sql.test.ts b/test/unit/cluster-page-sql.test.ts new file mode 100644 index 00000000..f52889ad --- /dev/null +++ b/test/unit/cluster-page-sql.test.ts @@ -0,0 +1,49 @@ +import { describe, expect, it } from 'vitest' +import { CLUSTERS } from '../../layers/registry/server/data/clusters' +import { clusterPageSql, D1_BOUND_PARAMETER_LIMIT } from '../../layers/registry/server/utils/cluster-membership' +import { parseClusterSkillKeys } from '../../layers/registry/server/utils/cluster-skill-curation' + +const COLUMNS = 'owner, name, repo, display_name, description, stars, modified_at, is_abstract' + +/** + * `/api/clusters/` sends two statements built from the membership + * fragment, and the fragment is not what D1 counts. The list statement binds + * every pinned key three times: once for the SELECT `CASE`, once for the WHERE + * arm, and twice more for the ORDER BY `CASE`, plus limit and offset. Guarding + * only the fragment let SKILLD-X reach production at 173 events. + */ +describe('cluster page sql', () => { + it('binds one param per placeholder in both statements', () => { + const page = clusterPageSql(COLUMNS, ['testing'], parseClusterSkillKeys(['obra/tdd']), { limit: 60, offset: 0 }) + + expect(page.countSql.match(/\?/g)).toHaveLength(page.countParams.length) + expect(page.listSql.match(/\?/g)).toHaveLength(page.listParams.length) + }) + + it('orders pinned skills ahead of the star ranking', () => { + const page = clusterPageSql(COLUMNS, [], parseClusterSkillKeys(['obra/tdd', 'shadcn/shadcn']), { limit: 10, offset: 20 }) + + expect(page.listParams.slice(-6)).toEqual(['obra', 'tdd', 'shadcn', 'shadcn', 10, 20]) + }) + + it('drops the pinned ORDER BY arm when a cluster has no pins', () => { + const page = clusterPageSql(COLUMNS, ['testing'], [], { limit: 60, offset: 0 }) + + expect(page.listSql).not.toContain('WHEN owner = ?') + expect(page.listParams).toEqual(['testing', 60, 0]) + }) + + it('keeps every live cluster page under the D1 parameter cap', () => { + for (const cluster of CLUSTERS) { + const page = clusterPageSql( + COLUMNS, + cluster.categories, + parseClusterSkillKeys(cluster.pinnedExamples), + { limit: 120, offset: 0 }, + ) + + expect(page.countParams.length, `${cluster.slug} count`).toBeLessThanOrEqual(D1_BOUND_PARAMETER_LIMIT) + expect(page.listParams.length, `${cluster.slug} list`).toBeLessThanOrEqual(D1_BOUND_PARAMETER_LIMIT) + } + }) +})