fix(common-utils,api): restore filter sidebar values behind query proxies

.changeset/fix-filter-facets-behind-proxy.md

@@ -0,0 +1,13 @@
+---
+"@hyperdx/common-utils": patch
+---
+
+Fix filter sidebar values disappearing behind query proxies. Batched facet-value
+queries (KV rollup and map text-index lookups) previously bound one query
+parameter per key; with ~100 keys this exceeded the ClickHouse web client's URL
+parameter budget, silently promoting the request to a multipart/form-data body
+that proxy gateways can reject — every LowCardinality-column and map-attribute
+filter then vanished without an error. Keys are now inlined as SQL-escaped
+literals so the query rides the POST body with a constant parameter count. Also
+fixes an operator-precedence bug that applied the KV rollup time filter (and
+notEmpty guard) to only the last OR branch.

packages/api/jest.int.config.js

@@ -1,8 +1,29 @@
+const { createJsWithTsPreset } = require('ts-jest');
+
 const base = require('./jest.config.js');
 
+// http-proxy-middleware v4 and its deps ship ESM-only builds Jest's CJS
+// loader can't parse. jest.setup.ts blanket-mocks the package; the
+// clickhouse-proxy int tests need the real thing, so transpile it here
+// (`allowJs` for the plain-JS sources, `.mjs` for httpxy's entry).
+const esmDepsTransformCfg = createJsWithTsPreset({
+  tsconfig: {
+    rootDir: './src',
+    allowJs: true,
+  },
+});
+
 /** @type {import("jest").Config} **/
 module.exports = {
   ...base,
+  transform: {
+    ...esmDepsTransformCfg.transform,
+    '^.+\\.mjs$': esmDepsTransformCfg.transform['^.+\\.[tj]sx?$'],
+  },
+  moduleFileExtensions: ['js', 'mjs', 'json', 'ts', 'tsx', 'node'],
+  transformIgnorePatterns: [
+    '/node_modules/(?!(http-proxy-middleware|httpxy|is-plain-obj)/)',
+  ],
   testMatch: ['**/__tests__/**/*.int.test.ts?(x)'],
   // Override the unit config's ignore list: it excludes `.int.test.ts`, which
   // would otherwise hide every integration test from this suite.

packages/api/jest.setup.ts

@@ -11,7 +11,8 @@ jest.retryTimes(
 );
 
 // http-proxy-middleware v4 is ESM-only and Jest's CJS module loader cannot
-// load ESM packages. Auto-mock since no test exercises the proxy directly.
+// load ESM packages. Auto-mock it; the clickhouse-proxy int tests opt out via
+// jest.unmock() (jest.int.config.js transpiles the package for them).
 jest.mock('http-proxy-middleware', () => ({
   createProxyMiddleware: jest.fn(() => jest.fn()),
 }));

packages/api/src/routers/api/__tests__/clickhouseProxy.int.test.ts

@@ -0,0 +1,153 @@
+import http from 'http';
+
+import { getLoggedInAgent, getServer } from '@/fixtures';
+import Connection from '@/models/connection';
+
+// Use the real proxy instead of the blanket mock in jest.setup.ts; the int
+// jest config transpiles it (see jest.int.config.js).
+jest.unmock('http-proxy-middleware');
+
+type CapturedRequest = {
+  method: string | undefined;
+  url: string | undefined;
+  headers: http.IncomingHttpHeaders;
+  body: Buffer;
+};
+
+/**
+ * Pins /clickhouse-proxy body forwarding: text/plain re-injection and the
+ * raw-stream passthrough of multipart/form-data bodies, which
+ * `@clickhouse/client-web` emits when query params exceed its URL budget
+ * (regression context: ClickHouse support-escalation #8482).
+ *
+ * Known-latent cases (charset-suffixed JSON, urlencoded) are not covered;
+ * tracked in https://github.com/hyperdxio/hyperdx/issues/2942.
+ */
+describe('clickhouse-proxy body forwarding', () => {
+  const server = getServer();
+
+  let upstream: http.Server;
+  let upstreamUrl: string;
+  let captured: CapturedRequest[] = [];
+
+  beforeAll(async () => {
+    await server.start();
+
+    upstream = http.createServer((req, res) => {
+      const chunks: Buffer[] = [];
+      req.on('data', chunk => chunks.push(chunk));
+      req.on('end', () => {
+        captured.push({
+          method: req.method,
+          url: req.url,
+          headers: req.headers,
+          body: Buffer.concat(chunks),
+        });
+        res.writeHead(200, { 'Content-Type': 'application/json' });
+        res.end(JSON.stringify({ data: [] }));
+      });
+    });
+    await new Promise<void>(resolve => upstream.listen(0, resolve));
+    const address = upstream.address();
+    if (address == null || typeof address === 'string') {
+      throw new Error('Expected upstream server to listen on a TCP port');
+    }
+    upstreamUrl = `http://127.0.0.1:${address.port}`;
+  });
+
+  beforeEach(() => {
+    captured = [];
+  });
+
+  afterEach(async () => {
+    await server.clearDBs();
+  });
+
+  afterAll(async () => {
+    await new Promise<void>((resolve, reject) =>
+      upstream.close(err => (err ? reject(err) : resolve())),
+    );
+    await server.stop();
+  });
+
+  const createConnection = async (teamId: string) => {
+    const connection = await Connection.create({
+      team: teamId,
+      name: 'Upstream Echo',
+      host: upstreamUrl,
+      username: 'default',
+      password: '',
+    });
+    return connection._id.toString();
+  };
+
+  it('forwards a text/plain SQL body verbatim', async () => {
+    const { agent, team } = await getLoggedInAgent(server);
+    const connectionId = await createConnection(team._id.toString());
+
+    const sql = 'SELECT count() FROM system.tables FORMAT JSON';
+    await agent
+      .post('/clickhouse-proxy/?query_id=test-text')
+      .set('x-hyperdx-connection-id', connectionId)
+      .set('Content-Type', 'text/plain')
+      .send(sql)
+      .expect(200);
+
+    expect(captured).toHaveLength(1);
+    expect(captured[0].body.toString()).toBe(sql);
+    // Connection credentials are attached server-side.
+    expect(captured[0].headers['x-clickhouse-user']).toBe('default');
+  });
+
+  it('forwards a large multipart/form-data body byte-for-byte (unparsed stream passthrough)', async () => {
+    const { agent, team } = await getLoggedInAgent(server);
+    const connectionId = await createConnection(team._id.toString());
+
+    // Mirror the client's multipart output: a `query` part plus one
+    // `param_*` part per bound parameter, past the 4096-byte URL budget.
+    const boundary = '----clickhouse-js-test-boundary';
+    const parts: string[] = [
+      `--${boundary}\r\n` +
+        `Content-Disposition: form-data; name="query"\r\n\r\n` +
+        `SELECT Key FROM rollup WHERE Key IN ({p0:String}) FORMAT JSON\r\n`,
+    ];
+    for (let i = 0; i < 120; i++) {
+      parts.push(
+        `--${boundary}\r\n` +
+          `Content-Disposition: form-data; name="param_HYPERDX_PARAM_${i}"\r\n\r\n` +
+          `some.rather.long.map.attribute.key.number.${i}\r\n`,
+      );
+    }
+    parts.push(`--${boundary}--\r\n`);
+    const body = parts.join('');
+    expect(Buffer.byteLength(body)).toBeGreaterThan(4096);
+
+    await agent
+      .post('/clickhouse-proxy/?query_id=test-multipart')
+      .set('x-hyperdx-connection-id', connectionId)
+      .set('Content-Type', `multipart/form-data; boundary=${boundary}`)
+      .send(body)
+      .expect(200);
+
+    expect(captured).toHaveLength(1);
+    expect(captured[0].headers['content-type']).toBe(
+      `multipart/form-data; boundary=${boundary}`,
+    );
+    expect(captured[0].body.toString()).toBe(body);
+    expect(captured[0].headers['content-length']).toBe(
+      String(Buffer.byteLength(body)),
+    );
+  });
+
+  it('rejects requests without a connection id header', async () => {
+    const { agent } = await getLoggedInAgent(server);
+
+    await agent
+      .post('/clickhouse-proxy/?query_id=test-no-conn')
+      .set('Content-Type', 'text/plain')
+      .send('SELECT 1')
+      .expect(400);
+
+    expect(captured).toHaveLength(0);
+  });
+});

packages/common-utils/src/__tests__/metadata.test.ts

@@ -951,14 +951,17 @@ describe('Metadata', () => {
         ]),
       );
 
-      jest
+      const doMVsAggregateSpy = jest
         .spyOn(metadata as any, 'doMetadataMVsAggregateColumn')
         .mockImplementation((...args: any[]) => {
           const columnName = args[1] as string;
           return Promise.resolve(
             columnName === 'ServiceName' || columnName === 'SeverityText',
           );
         });
+
+      // Returned so tests can override the MV-aggregation answer.
+      return { doMVsAggregateSpy };
     };
 
     afterEach(() => {
@@ -1020,8 +1023,101 @@ describe('Metadata', () => {
       expect(sql).toContain('BY ColumnIdentifier, Key');
       expect(sql).not.toContain('mergeTreeTextIndex(');
       expect(Object.values(params)).toContain('otel_logs_kv_rollup_15m');
-      expect(Object.values(params)).toContain('ServiceName');
-      expect(Object.values(params)).toContain('SeverityText');
+      // Keys are inlined as escaped literals, not bound as params — per-key
+      // params would push large batches into multipart bodies (see #8482).
+      expect(sql).toContain("Key IN ('ServiceName','SeverityText')");
+      expect(Object.values(params)).not.toContain('ServiceName');
+      expect(Object.values(params)).not.toContain('SeverityText');
+    });
+
+    it('keeps the KV rollup MV query parameter count independent of the number of keys', async () => {
+      const { doMVsAggregateSpy } = setupDefaultLogsSchema();
+      doMVsAggregateSpy.mockResolvedValue(true);
+
+      const manyKeys = Array.from(
+        { length: 80 },
+        (_, i) => `LogAttributes['some.rather.long.attribute.key.${i}']`,
+      );
+      // Route map keys through the MV (not the text index) so they all land in
+      // one batched rollup query.
+      jest
+        .spyOn(metadata, 'getMapColumnTextIndexes')
+        .mockResolvedValue(new Map());
+
+      await metadata.getAllKeyValues({
+        ...baseArgs,
+        keyExpressions: manyKeys,
+      });
+
+      const mvCall = (mockClickhouseClient.query as jest.Mock).mock.calls.find(
+        (c: any[]) => (c[0].query as string).includes('Key IN ('),
+      );
+      expect(mvCall).toBeDefined();
+      const params = mvCall![0].query_params as Record<string, string>;
+      // A handful of fixed params — never one per key.
+      expect(Object.keys(params).length).toBeLessThan(10);
+      const sql = mvCall![0].query as string;
+      expect(sql).toContain("'some.rather.long.attribute.key.0'");
+      expect(sql).toContain("'some.rather.long.attribute.key.79'");
+    });
+
+    it('SQL-escapes inlined rollup keys containing quotes and backslashes', async () => {
+      const { doMVsAggregateSpy } = setupDefaultLogsSchema();
+      doMVsAggregateSpy.mockResolvedValue(true);
+      jest
+        .spyOn(metadata, 'getMapColumnTextIndexes')
+        .mockResolvedValue(new Map());
+
+      await metadata.getAllKeyValues({
+        ...baseArgs,
+        keyExpressions: ["LogAttributes['it's a key']"],
+      });
+
+      const mvCall = (mockClickhouseClient.query as jest.Mock).mock.calls.find(
+        (c: any[]) => (c[0].query as string).includes('Key IN ('),
+      );
+      expect(mvCall).toBeDefined();
+      const sql = mvCall![0].query as string;
+      // The embedded quote must be escaped, never spliced raw.
+      expect(sql).toContain("Key IN ('it\\'s a key')");
+      expect(sql).not.toContain("Key IN ('it's a key')");
+    });
+
+    it('applies the time filter to every branch of the KV rollup OR chain', async () => {
+      setupDefaultLogsSchema();
+
+      await metadata.getAllKeyValues({
+        ...baseArgs,
+        keyExpressions: ['ServiceName', 'SeverityText'],
+      });
+
+      const mvCall = (mockClickhouseClient.query as jest.Mock).mock.calls.find(
+        (c: any[]) => (c[0].query as string).includes('Key IN ('),
+      );
+      expect(mvCall).toBeDefined();
+      const sql = (mvCall![0].query as string).replace(/\s+/g, ' ');
+      // Without parens, `a OR b AND time` binds the time filter to the
+      // last branch only.
+      expect(sql).toMatch(/WHERE \(\(ColumnIdentifier = /);
+      expect(sql).toMatch(/\)\) AND Timestamp >= /);
+    });
+
+    it('inlines map text index key prefixes as escaped literals, not params', async () => {
+      setupDefaultLogsSchema();
+
+      await metadata.getAllKeyValues({
+        ...baseArgs,
+        keyExpressions: ["LogAttributes['requestId']"],
+      });
+
+      const idxCall = (mockClickhouseClient.query as jest.Mock).mock.calls.find(
+        (c: any[]) => (c[0].query as string).includes('startsWith(token,'),
+      );
+      expect(idxCall).toBeDefined();
+      const sql = idxCall![0].query as string;
+      const params = idxCall![0].query_params as Record<string, string>;
+      expect(sql).toContain("startsWith(token, 'requestId=')");
+      expect(Object.values(params)).not.toContain('requestId=');
     });
 
     it('routes columns without any index or MV entry through the getKeyValues fallback (raw table scan)', async () => {
@@ -1212,11 +1308,12 @@ describe('Metadata', () => {
 
       expect(mapTextIndexCalls.length).toBeGreaterThanOrEqual(2);
 
-      const allParamValues = mapTextIndexCalls.flatMap((c: any[]) =>
-        Object.values(c[0].query_params ?? {}),
-      );
+      // Key prefixes are inlined as escaped literals, not per-key params.
+      const allQueries = mapTextIndexCalls
+        .map((c: any[]) => c[0].query as string)
+        .join('\n');
       for (let i = 0; i < keyCount; i++) {
-        expect(allParamValues).toContain(`k${i}=`);
+        expect(allQueries).toContain(`startsWith(token, 'k${i}=')`);
       }
     });
 

packages/common-utils/src/core/metadata.ts

@@ -1184,9 +1184,15 @@ export class Metadata {
         for (const [columnName, info] of queryOptions.entries()) {
           const orChain = concatChSql(
             ' OR ',
+            // Inline keys as SQL-escaped literals, not bind params: ~100
+            // per-key params exceed the web client's URL param budget and
+            // silently switch the request to a multipart body that proxies
+            // may reject. Keys are ingest-controlled, hence SqlString.escape.
             info.keys.map(
               k =>
-                chSql`startsWith(token, ${{ String: `${k}${info.separator}` }})`,
+                chSql`startsWith(token, ${{
+                  UNSAFE_RAW_SQL: SqlString.escape(`${k}${info.separator}`),
+                }})`,
             ),
           );
           const partsFilter = await this.partsOverlapFilter({
@@ -1358,17 +1364,22 @@ export class Metadata {
         // this should only be one mv... but we have a for loop in case
         const branch: ChSql[] = [];
         for (const [columnName, keys] of entry) {
-          const sql = chSql`(ColumnIdentifier = ${{ String: columnName }} AND Key IN (${concatChSql(
-            ',',
-            keys.map(key => chSql`${{ String: key }}`),
-          )}))`;
+          // Inline keys as SQL-escaped literals, not bind params: ~100
+          // per-key params exceed the web client's URL param budget and
+          // silently switch the request to a multipart body that proxies
+          // may reject. Keys are ingest-controlled, hence SqlString.escape.
+          const sql = chSql`(ColumnIdentifier = ${{ String: columnName }} AND Key IN (${{
+            UNSAFE_RAW_SQL: keys.map(key => SqlString.escape(key)).join(','),
+          }}))`;
           branch.push(sql);
         }
+        // Parenthesize the OR chain: `a OR b AND timeFilter` would bind the
+        // time filter (and notEmpty) to the last branch only.
         const sql = chSql`
           SELECT * FROM (
             SELECT ColumnIdentifier, Key, groupUniqArray(${{ Int32: maxValuesPerKey }})(Value) as Values
             FROM ${tableExpr({ database: databaseName, table: mvName })}
-            WHERE ${concatChSql(' OR ', branch)}
+            WHERE (${concatChSql(' OR ', branch)})
               AND ${timeFilter}
               AND notEmpty(Value)
             GROUP BY ColumnIdentifier, Key