diff --git a/web/src/utils/alerts/aggregationPreviewQuery.spec.ts b/web/src/utils/alerts/aggregationPreviewQuery.spec.ts index ed5466eb00..799d6a4665 100644 --- a/web/src/utils/alerts/aggregationPreviewQuery.spec.ts +++ b/web/src/utils/alerts/aggregationPreviewQuery.spec.ts @@ -14,7 +14,11 @@ // along with this program. If not, see . import { describe, expect, it } from "vitest"; -import { cleanAggregationQuery, withCompositeGroupLabel } from "./aggregationPreviewQuery"; +import { + buildCountChartQuery, + cleanAggregationQuery, + withCompositeGroupLabel, +} from "./aggregationPreviewQuery"; describe("cleanAggregationQuery", () => { it("drops the HAVING clause on a plain query", () => { @@ -122,3 +126,63 @@ describe("withCompositeGroupLabel", () => { expect(out).toMatch(/GROUP BY zo_sql_key, zo_group_label/i); }); }); + +describe("buildCountChartQuery", () => { + it("rewrites a plain custom SQL query into a count-over-time query", () => { + const chartQuery = buildCountChartQuery('SELECT _timestamp FROM "bugtest"'); + + expect(chartQuery).toBe( + 'SELECT histogram(_timestamp) AS zo_sql_key, count(*) AS zo_sql_num FROM "bugtest" GROUP BY zo_sql_key', + ); + }); + + it("does not mistake a line comment's 'from' for the statement's real FROM", () => { + const chartQuery = buildCountChartQuery( + 'SELECT _timestamp -- pick the from column\nFROM "bugtest"', + ); + + expect(chartQuery).toBe( + 'SELECT histogram(_timestamp) AS zo_sql_key, count(*) AS zo_sql_num FROM "bugtest" GROUP BY zo_sql_key', + ); + }); + + it("does not mistake a block comment's 'from' for the statement's real FROM", () => { + const chartQuery = buildCountChartQuery('SELECT _timestamp /* from here */ FROM "bugtest"'); + + expect(chartQuery).toBe( + 'SELECT histogram(_timestamp) AS zo_sql_key, count(*) AS zo_sql_num FROM "bugtest" GROUP BY zo_sql_key', + ); + }); + + it("returns null rather than a broken query for a query that starts with a CTE", () => { + const chartQuery = buildCountChartQuery( + 'WITH x AS (SELECT _timestamp FROM "bugtest") SELECT _timestamp FROM x', + ); + + expect(chartQuery).toBeNull(); + }); + + it("returns null rather than a broken query for a UNION", () => { + const chartQuery = buildCountChartQuery( + 'SELECT _timestamp FROM "bugtest" UNION ALL SELECT _timestamp FROM "bugtest"', + ); + + expect(chartQuery).toBeNull(); + }); + + it("returns null rather than a broken query for a JOIN", () => { + const chartQuery = buildCountChartQuery( + 'SELECT a._timestamp FROM "bugtest" a JOIN "bugtest" b ON a.svc = b.svc', + ); + + expect(chartQuery).toBeNull(); + }); + + it("still ignores a quoted filter value containing a real comment marker", () => { + const chartQuery = buildCountChartQuery( + "SELECT _timestamp FROM \"bugtest\" WHERE (note = '-- not a comment')", + ); + + expect(chartQuery).toContain("WHERE (note = '-- not a comment')"); + }); +}); diff --git a/web/src/utils/alerts/aggregationPreviewQuery.ts b/web/src/utils/alerts/aggregationPreviewQuery.ts index fad4fecde3..f02a24e82a 100644 --- a/web/src/utils/alerts/aggregationPreviewQuery.ts +++ b/web/src/utils/alerts/aggregationPreviewQuery.ts @@ -25,6 +25,57 @@ const maskStringLiterals = (sql: string): string => (m) => m[0] + "x".repeat(m.length - 2) + m[m.length - 1], ); +/** + * Blank out SQL line and block comments, quote-aware in both directions: a + * comment marker inside a string literal isn't mistaken for a real comment, + * and a quote inside a comment isn't mistaken for the start of a string + * literal. Run before maskStringLiterals — a regex pass can't safely tell + * the two apart in one direction only. + */ +const maskComments = (sql: string): string => { + let out = ""; + let i = 0; + while (i < sql.length) { + const ch = sql[i]; + if (ch === "'" || ch === '"') { + out += ch; + i++; + while (i < sql.length) { + out += sql[i]; + if (sql[i] === ch) { + if (sql[i + 1] === ch) { + out += sql[i + 1]; + i += 2; + continue; + } + i++; + break; + } + i++; + } + continue; + } + if (ch === "-" && sql[i + 1] === "-") { + let j = i; + while (j < sql.length && sql[j] !== "\n") j++; + out += "x".repeat(j - i); + i = j; + continue; + } + if (ch === "/" && sql[i + 1] === "*") { + let j = i + 2; + while (j < sql.length && !(sql[j] === "*" && sql[j + 1] === "/")) j++; + j = Math.min(j + 2, sql.length); + out += "x".repeat(j - i); + i = j; + continue; + } + out += ch; + i++; + } + return out; +}; + /** * Blank out everything inside parentheses, nesting-aware, so a keyword used * inside a function call or subquery — e.g. the FROM in EXTRACT(EPOCH FROM @@ -186,7 +237,7 @@ export default cleanAggregationQuery; */ export const buildCountChartQuery = (query: string): string | null => { if (!query) return null; - const masked = maskForKeywordSearch(query); + const masked = maskForKeywordSearch(maskComments(query)); if (!/^\s*SELECT\b/i.test(masked)) return null; const fromMatch = masked.match(/\bFROM\b/i); if (!fromMatch) return null; @@ -197,6 +248,11 @@ export const buildCountChartQuery = (query: string): string | null => { const maskedTail = masked.slice(fromIndex); const cutMatch = maskedTail.match(/\s+(?:ORDER\s+BY|LIMIT|GROUP\s+BY|HAVING)\b/i); const cutIndex = cutMatch ? fromIndex + (cutMatch.index as number) : undefined; + const maskedRowQuery = masked.slice(0, cutIndex); + // UNION/JOIN change what a single "count of matching rows" even means — + // rewriting the projection around them would either fail to parse or chart + // something other than the alert's own row count. No chart beats a wrong one. + if (/\b(?:UNION|JOIN)\b/i.test(maskedRowQuery)) return null; const tail = query.slice(fromIndex, cutIndex).trim(); if (!tail) return null; diff --git a/web/src/views/OnCall/OnCallResponses.spec.ts b/web/src/views/OnCall/OnCallResponses.spec.ts index 790393b040..e29680a045 100644 --- a/web/src/views/OnCall/OnCallResponses.spec.ts +++ b/web/src/views/OnCall/OnCallResponses.spec.ts @@ -13,8 +13,8 @@ // You should have received a copy of the GNU Affero General Public License // along with this program. If not, see . -import { flushPromises, mount } from "@vue/test-utils"; -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { flushPromises, mount, VueWrapper } from "@vue/test-utils"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import i18n from "@/locales"; import destinationService from "@/services/alert_destination"; @@ -201,11 +201,23 @@ const team = { updated_at: 0, }; +// Nothing here ever unmounts on its own account, so every mount from every +// test would otherwise sit alive (watchers, listeners) through the rest of +// the 70+ tests in this file — the accumulation is exactly what pushed the +// last few tests in the file past the default 5s timeout under load. +const mountedWrappers: VueWrapper[] = []; + function render() { - return mount(OnCallResponses, { global: { plugins: [i18n, store], stubs } }); + const wrapper = mount(OnCallResponses, { global: { plugins: [i18n, store], stubs } }); + mountedWrappers.push(wrapper); + return wrapper; } describe("OnCallResponses", () => { + afterEach(() => { + for (const wrapper of mountedWrappers.splice(0)) wrapper.unmount(); + }); + beforeEach(() => { vi.clearAllMocks(); for (const key of Object.keys(routeQuery)) delete routeQuery[key];