diff --git a/.changeset/pink-pugs-tickle.md b/.changeset/pink-pugs-tickle.md new file mode 100644 index 000000000..b7893a571 --- /dev/null +++ b/.changeset/pink-pugs-tickle.md @@ -0,0 +1,7 @@ +--- +"@hyperdx/common-utils": patch +"@hyperdx/api": patch +"@hyperdx/app": patch +--- + +chore(eslint): enable @typescript-eslint/no-unsafe-type-assertion rule (warn) diff --git a/.changeset/tough-owls-check.md b/.changeset/tough-owls-check.md new file mode 100644 index 000000000..bea5cfa4a --- /dev/null +++ b/.changeset/tough-owls-check.md @@ -0,0 +1,5 @@ +--- +"@hyperdx/common-utils": patch +--- + +refactor(common-utils): improve type safety and linting for type assertions diff --git a/packages/api/eslint.config.mjs b/packages/api/eslint.config.mjs index 588abee90..5c926d422 100644 --- a/packages/api/eslint.config.mjs +++ b/packages/api/eslint.config.mjs @@ -42,6 +42,7 @@ export default [ '@typescript-eslint/no-empty-object-type': 'warn', '@typescript-eslint/no-explicit-any': 'off', '@typescript-eslint/no-floating-promises': 'error', + '@typescript-eslint/no-unsafe-type-assertion': 'warn', '@typescript-eslint/no-namespace': 'warn', '@typescript-eslint/no-unused-vars': [ 'warn', diff --git a/packages/app/eslint.config.mjs b/packages/app/eslint.config.mjs index 3257e9091..2342ee250 100644 --- a/packages/app/eslint.config.mjs +++ b/packages/app/eslint.config.mjs @@ -21,6 +21,7 @@ export default [ 'next-env.d.ts', 'playwright-report/**', '.next/**', + '.storybook/**', 'node_modules/**', 'out/**', 'build/**', @@ -32,6 +33,8 @@ export default [ '**/*.config.mjs', 'eslint.config.mjs', 'public/__ENV.js', + 'global-setup.js', + 'scripts/**', ], }, { @@ -50,6 +53,7 @@ export default [ '@typescript-eslint/no-empty-function': 'warn', '@typescript-eslint/no-explicit-any': 'off', '@typescript-eslint/no-unsafe-function-type': 'warn', + '@typescript-eslint/no-unsafe-type-assertion': 'warn', '@typescript-eslint/no-unused-expressions': 'warn', '@typescript-eslint/no-unused-vars': [ 'warn', @@ -94,6 +98,7 @@ export default [ ecmaFeatures: { jsx: true, }, + project: './tsconfig.json', tsconfigRootDir: import.meta.dirname, }, globals: { @@ -119,6 +124,13 @@ export default [ }, }, }, + { + // Disable type-checked rules for JS files (not part of TypeScript project) + files: ['**/*.{js,jsx}'], + rules: { + '@typescript-eslint/no-unsafe-type-assertion': 'off', + }, + }, { files: ['tests/e2e/**/*.{ts,js}'], ...playwrightPlugin.configs['flat/recommended'], diff --git a/packages/common-utils/eslint.config.mjs b/packages/common-utils/eslint.config.mjs index 822c0877a..1f6b617bb 100644 --- a/packages/common-utils/eslint.config.mjs +++ b/packages/common-utils/eslint.config.mjs @@ -37,6 +37,7 @@ export default [ '@typescript-eslint/no-empty-object-type': 'warn', '@typescript-eslint/no-explicit-any': 'off', '@typescript-eslint/no-floating-promises': 'error', + '@typescript-eslint/no-unsafe-type-assertion': 'error', '@typescript-eslint/no-namespace': 'warn', '@typescript-eslint/no-unused-vars': [ 'warn', @@ -90,5 +91,12 @@ export default [ }, }, }, + { + // Disable unsafe type assertion rule for test files (mocking requires type assertions) + files: ['src/**/*.test.ts', 'src/**/__tests__/**/*.ts'], + rules: { + '@typescript-eslint/no-unsafe-type-assertion': 'off', + }, + }, ]; diff --git a/packages/common-utils/src/clickhouse/browser.ts b/packages/common-utils/src/clickhouse/browser.ts index 494cac7e7..b56a2cb0d 100644 --- a/packages/common-utils/src/clickhouse/browser.ts +++ b/packages/common-utils/src/clickhouse/browser.ts @@ -98,6 +98,7 @@ export class ClickhouseClient extends BaseClickhouseClient { protected async __query({ query, + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- default generic value format = 'JSON' as Format, query_params = {}, abort_signal, @@ -123,6 +124,7 @@ export class ClickhouseClient extends BaseClickhouseClient { : {}), }; + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- client library type mismatch return this.getClient().query({ query, query_params, diff --git a/packages/common-utils/src/clickhouse/index.ts b/packages/common-utils/src/clickhouse/index.ts index e5ee8d902..0bc35573a 100644 --- a/packages/common-utils/src/clickhouse/index.ts +++ b/packages/common-utils/src/clickhouse/index.ts @@ -470,11 +470,10 @@ export abstract class BaseClickhouseClient { debugSql = query; } - // eslint-disable-next-line no-console console.debug('--------------------------------------------------------'); - // eslint-disable-next-line no-console + console.debug('Sending Query:', debugSql); - // eslint-disable-next-line no-console + console.debug('--------------------------------------------------------'); } @@ -742,6 +741,7 @@ export function chSqlToAliasMap( replaceJsonExpressions(sql); const parser = new SQLParser.Parser(); + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- astify returns union type const ast = parser.astify(sqlWithReplacements, { database: 'Postgresql', parseOptions: { includeLocations: true }, @@ -795,9 +795,10 @@ export function filterColumnMetaByType( meta: Array, types: JSDataType[], ): Array | undefined { - return meta.filter(column => - types.includes(convertCHDataTypeToJSType(column.type) as JSDataType), - ); + return meta.filter(column => { + const jsType = convertCHDataTypeToJSType(column.type); + return jsType != null && types.includes(jsType); + }); } export function inferTimestampColumn( diff --git a/packages/common-utils/src/clickhouse/node.ts b/packages/common-utils/src/clickhouse/node.ts index 57dc6baa3..116789e1e 100644 --- a/packages/common-utils/src/clickhouse/node.ts +++ b/packages/common-utils/src/clickhouse/node.ts @@ -25,6 +25,7 @@ export class ClickhouseClient extends BaseClickhouseClient { protected async __query({ query, + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- default generic value format = 'JSON' as Format, query_params = {}, abort_signal, @@ -38,6 +39,7 @@ export class ClickhouseClient extends BaseClickhouseClient { ); // TODO: Custom error handling + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- client library type mismatch return this.getClient().query({ query, query_params, diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index fa17c2722..e0630d7e9 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -30,7 +30,7 @@ export class MetadataCache { async getOrFetch(key: string, query: () => Promise): Promise { // Check if value exists in cache - const cachedValue = this.cache.get(key) as T | undefined; + const cachedValue: T | undefined = this.cache.get(key); if (cachedValue != null) { return cachedValue; } @@ -194,9 +194,9 @@ export class Metadata { connectionId, clickhouse_settings: this.getClickHouseSettings(), }) - .then(res => res.json()) + .then(res => res.json()) .then(d => d.data); - return columns as ColumnMeta[]; + return columns; }, ); } @@ -353,13 +353,15 @@ export class Metadata { max_rows_to_read: '0', }, }) - .then(res => res.json>()) + .then(res => res.json<{ keysArr?: string[]; key?: string }>()) .then(d => { let output: string[]; if (strategy === 'groupUniqArrayArray') { - output = d.data[0].keysArr as string[]; + output = d.data[0].keysArr ?? []; } else { - output = d.data.map(row => row.key) as string[]; + output = d.data + .map(row => row.key) + .filter((k): k is string => Boolean(k)); } return output.filter(r => r); @@ -496,8 +498,8 @@ export class Metadata { ...this.getClickHouseSettings(), }, }) - .then(res => res.json>()) - .then(d => d.data.map(row => row.value as string)); + .then(res => res.json<{ value: string }>()) + .then(d => d.data.map(row => row.value)); return values; }); } @@ -765,10 +767,11 @@ export class Metadata { }) .then(res => res.json()); - // TODO: Fix type issues mentioned in HDX-1548. value is not acually a + // TODO: Fix type issues mentioned in HDX-1548. value is not actually a // string[], sometimes it's { [key: string]: string; } return Object.entries(json?.data?.[0]).map(([key, value]) => ({ key: keys[parseInt(key.replace('param', ''))], + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- intentional, see HDX-1548 value: (value as string[])?.filter(Boolean), // remove nulls })); }, diff --git a/packages/common-utils/src/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index 7f1695139..e630e0415 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -168,6 +168,7 @@ const fastifySQL = ({ // Parse the SQL AST try { const parser = new SQLParser.Parser(); + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- astify returns union type, we expect Select const ast = parser.astify(rawSQL, { database: 'Postgresql', }) as SQLParser.Select; @@ -187,9 +188,11 @@ const fastifySQL = ({ } let colExpr; + switch (node.type) { case 'column_ref': { // FIXME: handle 'Value' type? + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion const _n = node as ColumnRef; // @ts-ignore if (typeof _n.column !== 'string') { @@ -199,6 +202,7 @@ const fastifySQL = ({ break; } case 'binary_expr': { + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion const _n = node as SQLParser.Expr; if (Array.isArray(_n.left)) { for (const left of _n.left) { @@ -218,6 +222,7 @@ const fastifySQL = ({ break; } case 'function': { + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion const _n = node as SQLParser.Function; if (_n.args?.type === 'expr_list') { @@ -231,6 +236,7 @@ const fastifySQL = ({ _n.args?.value?.[0]?.type === 'column_ref' && _n.args?.value?.[1]?.type === 'single_quote_string' ) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- incomplete library types colExpr = `${_n.name?.name?.[0]?.value}(${(_n.args?.value?.[0] as any)?.column.expr.value}, '${_n.args?.value?.[1]?.value}')`; } } @@ -250,6 +256,7 @@ const fastifySQL = ({ if (colExpr) { const materializedField = materializedFields.get(colExpr); if (materializedField) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion const _n = node as ColumnRef; // reset the node ref for (const key in _n) { @@ -971,7 +978,8 @@ async function renderWith( // results in schema conformance. const resolvedSql = sql ? sql - : await renderChartConfig(chartConfig as ChartConfig, metadata); + : // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- intentional, see comment above + await renderChartConfig(chartConfig as ChartConfig, metadata); if (clause.isSubquery === false) { return chSql`(${resolvedSql}) AS ${{ Identifier: clause.name }}`; @@ -1269,6 +1277,7 @@ async function translateMetricChartConfig( // Render the various clauses from the user input so they can be woven into the CTE queries. The dateRange // is manipulated to search forward/back one bucket window to ensure that there is enough data to compute // a reasonable value on the ends of the series. + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion const cteChartConfig = { ...chartConfig, from: { @@ -1374,6 +1383,7 @@ export async function renderChartConfig( chSql`${orderBy?.sql ? chSql`ORDER BY ${orderBy}` : ''}`, //chSql`${fill?.sql ? chSql`WITH FILL ${fill}` : ''}`, chSql`${limit?.sql ? chSql`LIMIT ${limit}` : ''}`, + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- settings type narrowing chSql`${'settings' in chartConfig ? chSql`SETTINGS ${chartConfig.settings as ChSql}` : []}`, ]); } diff --git a/packages/common-utils/src/queryParser.ts b/packages/common-utils/src/queryParser.ts index cfedec730..18edbcef4 100644 --- a/packages/common-utils/src/queryParser.ts +++ b/packages/common-utils/src/queryParser.ts @@ -29,6 +29,29 @@ export function parse(query: string): lucene.AST { const IMPLICIT_FIELD = ''; +// Type guards for lucene AST types +function isNodeTerm(node: lucene.Node | lucene.AST): node is lucene.NodeTerm { + return 'term' in node && node.term != null; +} + +function isNodeRangedTerm( + node: lucene.Node | lucene.AST, +): node is lucene.NodeRangedTerm { + return 'inclusive' in node && node.inclusive != null; +} + +function isBinaryAST(ast: lucene.AST | lucene.Node): ast is lucene.BinaryAST { + return 'right' in ast && ast.right != null; +} + +function isLeftOnlyAST( + ast: lucene.AST | lucene.Node, +): ast is lucene.LeftOnlyAST { + return ( + 'left' in ast && ast.left != null && !('right' in ast && ast.right != null) + ); +} + const CLICK_HOUSE_JSON_NUMBER_TYPES = [ 'Int8', 'Int16', @@ -641,8 +664,8 @@ async function nodeTerm( const isImplicitField = node.field === IMPLICIT_FIELD; // NodeTerm - if ((node as lucene.NodeTerm).term != null) { - const nodeTerm = node as lucene.NodeTerm; + if (isNodeTerm(node)) { + const nodeTerm = node; let term = decodeSpecialTokens(nodeTerm.term); // We should only negate the search for negated bare terms (ex. '-5') // This meeans the field is implicit and the prefix is - @@ -714,8 +737,8 @@ async function nodeTerm( // TODO: Handle regex, similarity, boost, prefix } // NodeRangedTerm - if ((node as lucene.NodeRangedTerm).inclusive != null) { - const rangedTerm = node as lucene.NodeRangedTerm; + if (isNodeRangedTerm(node)) { + const rangedTerm = node; return serializer.range( field, rangedTerm.term_min, @@ -760,18 +783,18 @@ async function serialize( // Node Scenarios: // 1. NodeTerm: Single term ex. "foo:bar" // 2. NodeRangedTerm: Two terms ex. "foo:[bar TO qux]" - if ((ast as lucene.NodeTerm).term != null) { - return await nodeTerm(ast as lucene.NodeTerm, serializer, context); + if (isNodeTerm(ast)) { + return await nodeTerm(ast, serializer, context); } - if ((ast as lucene.NodeRangedTerm).inclusive != null) { - return await nodeTerm(ast as lucene.NodeTerm, serializer, context); + if (isNodeRangedTerm(ast)) { + return await nodeTerm(ast, serializer, context); } // AST Scenarios: // 1. BinaryAST: Two terms ex. "foo:bar AND baz:qux" // 2. LeftOnlyAST: Single term ex. "foo:bar" - if ((ast as lucene.BinaryAST).right != null) { - const binaryAST = ast as lucene.BinaryAST; + if (isBinaryAST(ast)) { + const binaryAST = ast; const operator = serializer.operator(binaryAST.operator, context); const parenthesized = binaryAST.parenthesized; @@ -786,8 +809,8 @@ async function serialize( return serialized; } - if ((ast as lucene.LeftOnlyAST).left != null) { - const leftOnlyAST = ast as lucene.LeftOnlyAST; + if (isLeftOnlyAST(ast)) { + const leftOnlyAST = ast; const parenthesized = leftOnlyAST.parenthesized; const newContext = createSerializerContext(context, leftOnlyAST); diff --git a/packages/common-utils/src/types.ts b/packages/common-utils/src/types.ts index 70d01633a..9419a938a 100644 --- a/packages/common-utils/src/types.ts +++ b/packages/common-utils/src/types.ts @@ -31,6 +31,7 @@ export const MetricTableSchema = z ...acc, [key]: z.string().optional(), }), + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- reduce builds complete object at runtime {} as Record, ), )