From c967069bd181969a2c32a5c8056f26ea6280739d Mon Sep 17 00:00:00 2001 From: avionicharshit-byte Date: Wed, 2 Sep 2026 20:04:07 +0530 Subject: [PATCH] fix(db): count distinct profiles so the list and its total agree --- .../db/src/services/profile-list-sql.test.ts | 99 +++++++++++++++++++ packages/db/src/services/profile.service.ts | 86 ++++++++-------- 2 files changed, 141 insertions(+), 44 deletions(-) create mode 100644 packages/db/src/services/profile-list-sql.test.ts diff --git a/packages/db/src/services/profile-list-sql.test.ts b/packages/db/src/services/profile-list-sql.test.ts new file mode 100644 index 000000000..9f9e498f3 --- /dev/null +++ b/packages/db/src/services/profile-list-sql.test.ts @@ -0,0 +1,99 @@ +/** + * SQL-shape tests for the profile list and its count. + * + * Same strategy as profile-metrics-sql.test.ts: string assertions always run; + * `EXPLAIN` validation runs against a locally reachable ClickHouse + * (`pnpm dock:up`) and skips otherwise. + */ +import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'; + +import { ch } from '../clickhouse/client'; +import { buildProfileListCountSql, buildProfileListSql } from './profile.service'; + +const PROJECT_ID = 'test-sql-validation'; + +let chReachable = false; + +beforeAll(async () => { + vi.spyOn(console, 'log').mockImplementation(() => {}); + try { + await ch.command({ query: 'SELECT 1' }); + chReachable = true; + } catch { + chReachable = false; + } +}); + +afterAll(() => { + vi.restoreAllMocks(); +}); + +const itCH = (name: string, fn: () => Promise) => + it(name, async (ctx) => { + if (!chReachable) { + ctx.skip('ClickHouse not reachable at CLICKHOUSE_URL'); + } + await fn(); + }); + +describe('buildProfileListCountSql', () => { + // One profile is several rows until a merge collapses them, so counting rows + // reports a total the FINAL list can never fill. + it('counts profiles, not undeduplicated rows', () => { + const sql = buildProfileListCountSql({ projectId: PROJECT_ID }); + expect(sql).toContain('uniqExact(id) as count'); + expect(sql).not.toContain('count(id)'); + }); + + it('applies the same filters as the list', () => { + const options = { + projectId: PROJECT_ID, + search: 'john', + isExternal: true, + }; + const list = buildProfileListSql({ ...options, take: 50 }); + const count = buildProfileListCountSql(options); + for (const clause of [ + `project_id = '${PROJECT_ID}'`, + 'is_external = true', + "email ILIKE '%john%'", + ]) { + expect(list).toContain(clause); + expect(count).toContain(clause); + } + }); + + it('escapes the project id', () => { + const sql = buildProfileListCountSql({ projectId: "x'--" }); + expect(sql).toContain("'x\\'--'"); + }); + + itCH('parses and resolves against ClickHouse', async () => { + await ch.command({ + query: `EXPLAIN ${buildProfileListCountSql({ projectId: PROJECT_ID })}`, + }); + }); +}); + +describe('buildProfileListSql', () => { + it('pages with offset = cursor * take', () => { + expect(buildProfileListSql({ projectId: PROJECT_ID, take: 50 })).not.toContain( + 'OFFSET', + ); + expect( + buildProfileListSql({ projectId: PROJECT_ID, take: 50, cursor: 2 }), + ).toContain('OFFSET 100'); + }); + + it('reads the deduplicated view of the table', () => { + expect(buildProfileListSql({ projectId: PROJECT_ID, take: 50 })).toContain( + 'FROM profiles FINAL', + ); + }); + + itCH('parses and resolves against ClickHouse', async () => { + await ch.command({ + query: `EXPLAIN ${buildProfileListSql({ projectId: PROJECT_ID, take: 50 })}`, + }); + }); +}); diff --git a/packages/db/src/services/profile.service.ts b/packages/db/src/services/profile.service.ts index 5eddf7ae1..8791e401d 100644 --- a/packages/db/src/services/profile.service.ts +++ b/packages/db/src/services/profile.service.ts @@ -13,7 +13,7 @@ import { toNullIfDefaultMinDate, } from '../clickhouse/client'; import { clix } from '../clickhouse/query-builder'; -import { createSqlBuilder } from '../sql-builder'; +import { type SqlBuilderObject, createSqlBuilder } from '../sql-builder'; import type { IClickhouseEvent } from './event.service'; import { buildFilterWhere } from './filter-where.service'; import type { IClickhouseSession } from './session.service'; @@ -182,21 +182,14 @@ export async function getProfiles(ids: string[], projectId: string) { export const getProfilesCached = cacheable(getProfiles, 60 * 5); -export async function getProfileList({ - take, - cursor, - projectId, - filters, - search, - isExternal, -}: GetProfileListOptions) { - const { sb, getSql } = createSqlBuilder(); - sb.from = `${TABLE_NAMES.profiles} FINAL`; - sb.select.all = '*'; +type ProfileListFilterOptions = Omit; + +/** Where clause shared by the profile list and its count, so the two agree. */ +function applyProfileListWhere( + sb: SqlBuilderObject, + { projectId, filters, search, isExternal }: ProfileListFilterOptions, +) { sb.where.project_id = `project_id = ${sqlstring.escape(projectId)}`; - sb.limit = take; - sb.offset = Math.max(0, (cursor ?? 0) * take); - sb.orderBy.created_at = 'created_at DESC'; const searchClause = profileSearchSql(search); if (searchClause) { sb.where.search = searchClause; @@ -214,39 +207,44 @@ export async function getProfileList({ }), ); } - const data = await chQuery(getSql()); - return data.map(transformProfile); } -export async function getProfileListCount({ - projectId, - filters, - isExternal, - search, -}: Omit) { +export function buildProfileListSql({ + take, + cursor, + ...options +}: GetProfileListOptions) { const { sb, getSql } = createSqlBuilder(); - sb.from = 'profiles'; - sb.select.count = 'count(id) as count'; - sb.where.project_id = `project_id = ${sqlstring.escape(projectId)}`; + sb.from = `${TABLE_NAMES.profiles} FINAL`; + sb.select.all = '*'; + sb.limit = take; + sb.offset = Math.max(0, (cursor ?? 0) * take); + sb.orderBy.created_at = 'created_at DESC'; + applyProfileListWhere(sb, options); + return getSql(); +} + +export function buildProfileListCountSql(options: ProfileListFilterOptions) { + const { sb, getSql } = createSqlBuilder(); + sb.from = TABLE_NAMES.profiles; + // One profile is several rows until a background merge collapses them, so + // counting rows overcounts against the FINAL list. uniqExact deduplicates + // without FINAL, which cannot spill to disk on large projects. + sb.select.count = 'uniqExact(id) as count'; sb.groupBy.project_id = 'project_id'; - const searchClause = profileSearchSql(search); - if (searchClause) { - sb.where.search = searchClause; - } - if (isExternal !== undefined) { - sb.where.external = `is_external = ${isExternal ? 'true' : 'false'}`; - } - if (filters?.length) { - Object.assign( - sb.where, - buildFilterWhere(filters, projectId, { - selfTable: 'profiles', - profileIdExpr: 'id', - groupsExpr: 'groups', - }), - ); - } - const data = await chQuery<{ count: number }>(getSql()); + applyProfileListWhere(sb, options); + return getSql(); +} + +export async function getProfileList(options: GetProfileListOptions) { + const data = await chQuery(buildProfileListSql(options)); + return data.map(transformProfile); +} + +export async function getProfileListCount(options: ProfileListFilterOptions) { + const data = await chQuery<{ count: number }>( + buildProfileListCountSql(options), + ); return data[0]?.count ?? 0; }