diff --git a/server/api/collections/by-author/[login]/[slug]/index.get.ts b/server/api/collections/by-author/[login]/[slug]/index.get.ts index a8362435..9b835e33 100644 --- a/server/api/collections/by-author/[login]/[slug]/index.get.ts +++ b/server/api/collections/by-author/[login]/[slug]/index.get.ts @@ -24,13 +24,13 @@ export default defineApiHandler({ if (!collection) throw createError({ statusCode: 404, message: 'Collection not found' }) + // The CTE is limited to the collection's repositories. Unfiltered, it ranked + // every Skill in the registry and ran the correlated repo count for each, + // about 1.69M rows read per call. `rn` partitions by repository, so the + // filter leaves every rank unchanged. const skillsRes = await platform.db.prepare( `WITH ranked_skills AS ( SELECT s.owner, s.repo, s.name, s.display_name, - (SELECT COUNT(*) FROM skills repo_skills - WHERE repo_skills.owner = s.owner - AND repo_skills.repo = s.repo - AND repo_skills.source_resolved = 1) AS repo_skill_count, ROW_NUMBER() OVER ( PARTITION BY s.owner, s.repo ORDER BY s.modified_at DESC, s.name ASC @@ -39,9 +39,15 @@ export default defineApiHandler({ WHERE (r.broken_since IS NULL OR r.broken_since > unixepoch() - 604800) AND s.source_resolved = 1 AND s.rendered_status = 'ok' + AND (s.owner, s.repo) IN ( + SELECT owner, repo FROM collection_skills_v2 WHERE collection_id = ?1 + ) ) SELECT cs.position, cs.owner, cs.repo, rs.name, rs.display_name, cs.reason, - rs.repo_skill_count + (SELECT COUNT(*) FROM skills repo_skills + WHERE repo_skills.owner = cs.owner + AND repo_skills.repo = cs.repo + AND repo_skills.source_resolved = 1) AS repo_skill_count FROM collection_skills_v2 cs JOIN ranked_skills rs ON rs.owner = cs.owner @@ -50,7 +56,7 @@ export default defineApiHandler({ (cs.name IS NOT NULL AND rs.name = cs.name) OR (cs.name IS NULL AND rs.rn = 1) ) - WHERE cs.collection_id = ? + WHERE cs.collection_id = ?1 ORDER BY cs.position ASC`, ).bind(collection.id).all() diff --git a/server/api/collections/by-author/[login]/[slug]/manifest.get.ts b/server/api/collections/by-author/[login]/[slug]/manifest.get.ts index a836bd01..711e3d8e 100644 --- a/server/api/collections/by-author/[login]/[slug]/manifest.get.ts +++ b/server/api/collections/by-author/[login]/[slug]/manifest.get.ts @@ -41,6 +41,10 @@ export default defineApiHandler({ WHERE (r.broken_since IS NULL OR r.broken_since > unixepoch() - 604800) AND s.source_resolved = 1 AND s.rendered_status = 'ok' + -- Rank only the collection's repositories; \`rn\` is per repository. + AND (s.owner, s.repo) IN ( + SELECT owner, repo FROM collection_skills_v2 WHERE collection_id = ?1 + ) ) SELECT cs.owner, cs.repo, rs.name, cs.position, rs.target_package diff --git a/server/api/collections/index.get.ts b/server/api/collections/index.get.ts index 59da17ab..eb7d2b01 100644 --- a/server/api/collections/index.get.ts +++ b/server/api/collections/index.get.ts @@ -103,6 +103,10 @@ async function loadSkillLabels(db: D1Database, rows: CollectionIndexRow[]) { WHERE (r.broken_since IS NULL OR r.broken_since > unixepoch() - 604800) AND s.source_resolved = 1 AND s.rendered_status = 'ok' + -- Rank only the listed collections' repositories; \`rn\` is per repository. + AND (s.owner, s.repo) IN ( + SELECT owner, repo FROM collection_skills_v2 WHERE collection_id IN (${placeholders}) + ) ) SELECT cs.collection_id, cs.owner, cs.repo, rs.name, rs.display_name FROM collection_skills_v2 cs @@ -115,7 +119,7 @@ async function loadSkillLabels(db: D1Database, rows: CollectionIndexRow[]) { ) WHERE cs.collection_id IN (${placeholders}) ORDER BY cs.collection_id ASC, cs.position ASC`, - ).bind(...ids).all() + ).bind(...ids, ...ids).all() for (const skill of skillsRes.results ?? []) { const labels = skillsByCollection.get(skill.collection_id) ?? [] diff --git a/test/unit/collection-detail.test.ts b/test/unit/collection-detail.test.ts new file mode 100644 index 00000000..47b2feff --- /dev/null +++ b/test/unit/collection-detail.test.ts @@ -0,0 +1,110 @@ +import type { EventHandler, H3Event } from 'h3' +import Database from 'better-sqlite3' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +describe('collection detail endpoint', () => { + let sqlite: Database.Database + let event: H3Event + let detailHandler: EventHandler + + beforeEach(async () => { + vi.resetModules() + vi.stubGlobal('defineEventHandler', (handler: EventHandler) => handler) + sqlite = new Database(':memory:') + sqlite.exec(` + CREATE TABLE users (id INTEGER PRIMARY KEY, login TEXT NOT NULL, name TEXT, avatar TEXT); + CREATE TABLE collections_v2 ( + id INTEGER PRIMARY KEY, author_user_id INTEGER, slug TEXT NOT NULL, name TEXT NOT NULL, + preamble TEXT, featured INTEGER DEFAULT 0, created_at INTEGER, updated_at INTEGER, deleted_at INTEGER + ); + CREATE TABLE collection_skills_v2 ( + collection_id INTEGER NOT NULL, position INTEGER NOT NULL, + owner TEXT NOT NULL, repo TEXT NOT NULL, name TEXT, reason TEXT + ); + CREATE TABLE skills ( + owner TEXT NOT NULL, repo TEXT NOT NULL, name TEXT NOT NULL, display_name TEXT, + modified_at INTEGER, source_resolved INTEGER NOT NULL DEFAULT 0, rendered_status TEXT + ); + CREATE TABLE repos (owner TEXT NOT NULL, repo TEXT NOT NULL, broken_since INTEGER); + + INSERT INTO users VALUES (1, 'harlan', 'Harlan', NULL); + INSERT INTO collections_v2 VALUES (1, 1, 'stack', 'Stack', NULL, 0, 1, 1, NULL); + INSERT INTO collections_v2 VALUES (2, 1, 'other', 'Other', NULL, 0, 1, 1, NULL); + -- Collection 1 names one Skill and one whole repository. + INSERT INTO collection_skills_v2 VALUES (1, 1, 'acme', 'kit', 'older', 'named'); + INSERT INTO collection_skills_v2 VALUES (1, 2, 'solo', 'hub', NULL, 'repo'); + INSERT INTO collection_skills_v2 VALUES (2, 1, 'elsewhere', 'repo', 'x', NULL); + INSERT INTO skills VALUES ('acme', 'kit', 'older', 'Older', 100, 1, 'ok'); + INSERT INTO skills VALUES ('acme', 'kit', 'newer', 'Newer', 200, 1, 'ok'); + INSERT INTO skills VALUES ('solo', 'hub', 'stale', 'Stale', 100, 1, 'ok'); + INSERT INTO skills VALUES ('solo', 'hub', 'fresh', 'Fresh', 300, 1, 'ok'); + INSERT INTO skills VALUES ('solo', 'hub', 'unresolved', 'Unresolved', 900, 0, 'ok'); + INSERT INTO skills VALUES ('elsewhere', 'repo', 'x', 'X', 999, 1, 'ok'); + INSERT INTO repos VALUES ('acme', 'kit', NULL); + INSERT INTO repos VALUES ('solo', 'hub', NULL); + INSERT INTO repos VALUES ('elsewhere', 'repo', NULL); + `) + + event = { + method: 'GET', + context: { + platform: { db: wrapSqlite(sqlite) }, + params: { login: 'harlan', slug: 'stack' }, + }, + node: { req: { headers: {} } }, + } as unknown as H3Event + vi.stubGlobal('getUserSession', () => Promise.resolve(null)) + vi.stubGlobal('getRouterParam', (_event: H3Event, key: string) => event.context.params?.[key]) + detailHandler = (await import('../../server/api/collections/by-author/[login]/[slug]/index.get')).default + }) + + afterEach(() => { + sqlite.close() + vi.unstubAllGlobals() + }) + + it('resolves named Skills and repository entries with resolved repo counts', async () => { + const result = await detailHandler(event) as { skills: Array> } + + expect(result.skills.map(skill => ({ + position: skill.position, + owner: skill.owner, + repo: skill.repo, + name: skill.name, + displayName: skill.displayName, + registryPath: skill.registryPath, + }))).toEqual([ + { position: 1, owner: 'acme', repo: 'kit', name: 'older', displayName: 'Older', registryPath: '/gh/acme/kit/older' }, + { position: 2, owner: 'solo', repo: 'hub', name: 'fresh', displayName: 'Fresh', registryPath: '/gh/solo/hub/fresh' }, + ]) + }) +}) + +function wrapSqlite(sqlite: Database.Database) { + return { + prepare(sql: string) { + return { + bind(...params: unknown[]) { + const prepared = expandNumberedPlaceholders(sql, params) + return { + async all() { + return { results: sqlite.prepare(prepared.sql).all(...prepared.params) as T[] } + }, + async first() { + return (sqlite.prepare(prepared.sql).get(...prepared.params) as T | undefined) ?? null + }, + } + }, + } + }, + } +} + +function expandNumberedPlaceholders(sql: string, params: unknown[]): { sql: string, params: unknown[] } { + const expanded: unknown[] = [] + const nextSql = sql.replace(/\?(\d+)/g, (_, raw: string) => { + expanded.push(params[Number(raw) - 1]) + return '?' + }) + return expanded.length ? { sql: nextSql, params: expanded } : { sql, params } +}