Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/pink-pugs-tickle.md
Original file line number Diff line number Diff line change
@@ -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)
5 changes: 5 additions & 0 deletions .changeset/tough-owls-check.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@hyperdx/common-utils": patch
---

refactor(common-utils): improve type safety and linting for type assertions
1 change: 1 addition & 0 deletions packages/api/eslint.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
12 changes: 12 additions & 0 deletions packages/app/eslint.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export default [
'next-env.d.ts',
'playwright-report/**',
'.next/**',
'.storybook/**',
'node_modules/**',
'out/**',
'build/**',
Expand All @@ -32,6 +33,8 @@ export default [
'**/*.config.mjs',
'eslint.config.mjs',
'public/__ENV.js',
'global-setup.js',
'scripts/**',
],
},
{
Expand All @@ -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',
Expand Down Expand Up @@ -94,6 +98,7 @@ export default [
ecmaFeatures: {
jsx: true,
},
project: './tsconfig.json',
tsconfigRootDir: import.meta.dirname,
},
globals: {
Expand All @@ -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'],
Expand Down
8 changes: 8 additions & 0 deletions packages/common-utils/eslint.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type assertion will error in common-utils pkg going forward

'@typescript-eslint/no-namespace': 'warn',
'@typescript-eslint/no-unused-vars': [
'warn',
Expand Down Expand Up @@ -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',
},
},
];

2 changes: 2 additions & 0 deletions packages/common-utils/src/clickhouse/browser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ export class ClickhouseClient extends BaseClickhouseClient {

protected async __query<Format extends DataFormat>({
query,
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- default generic value
format = 'JSON' as Format,
query_params = {},
abort_signal,
Expand All @@ -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,
Expand Down
13 changes: 7 additions & 6 deletions packages/common-utils/src/clickhouse/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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('--------------------------------------------------------');
}

Expand Down Expand Up @@ -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 },
Expand Down Expand Up @@ -795,9 +795,10 @@ export function filterColumnMetaByType(
meta: Array<ColumnMetaType>,
types: JSDataType[],
): Array<ColumnMetaType> | 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(
Expand Down
2 changes: 2 additions & 0 deletions packages/common-utils/src/clickhouse/node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export class ClickhouseClient extends BaseClickhouseClient {

protected async __query<Format extends DataFormat>({
query,
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- default generic value
format = 'JSON' as Format,
query_params = {},
abort_signal,
Expand All @@ -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,
Expand Down
21 changes: 12 additions & 9 deletions packages/common-utils/src/core/metadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export class MetadataCache {

async getOrFetch<T>(key: string, query: () => Promise<T>): Promise<T> {
// 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;
}
Expand Down Expand Up @@ -194,9 +194,9 @@ export class Metadata {
connectionId,
clickhouse_settings: this.getClickHouseSettings(),
})
.then(res => res.json())
.then(res => res.json<ColumnMeta>())
.then(d => d.data);
return columns as ColumnMeta[];
return columns;
},
);
}
Expand Down Expand Up @@ -353,13 +353,15 @@ export class Metadata {
max_rows_to_read: '0',
},
})
.then(res => res.json<Record<string, unknown>>())
.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);
Expand Down Expand Up @@ -496,8 +498,8 @@ export class Metadata {
...this.getClickHouseSettings(),
},
})
.then(res => res.json<Record<string, unknown>>())
.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;
});
}
Expand Down Expand Up @@ -765,10 +767,11 @@ export class Metadata {
})
.then(res => res.json<any>());

// 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
}));
},
Expand Down
12 changes: 11 additions & 1 deletion packages/common-utils/src/core/renderChartConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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') {
Expand All @@ -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) {
Expand All @@ -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') {
Expand All @@ -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}')`;
}
}
Expand All @@ -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) {
Expand Down Expand Up @@ -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 }}`;
Expand Down Expand Up @@ -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: {
Expand Down Expand Up @@ -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}` : []}`,
]);
}
Expand Down
47 changes: 35 additions & 12 deletions packages/common-utils/src/queryParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,29 @@ export function parse(query: string): lucene.AST {

const IMPLICIT_FIELD = '<implicit>';

// 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',
Expand Down Expand Up @@ -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 -
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;

Expand All @@ -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);
Expand Down
1 change: 1 addition & 0 deletions packages/common-utils/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<MetricsDataType, z.ZodString>,
),
)
Expand Down
Loading