diff --git a/src/utils/identifiers.ts b/src/utils/identifiers.ts index 07a4a62aa..f96edab70 100644 --- a/src/utils/identifiers.ts +++ b/src/utils/identifiers.ts @@ -31,9 +31,10 @@ export function getQuoteChar( */ /** True when identifiers in generated SQL fragments should be double-quoted (PostgreSQL). */ export function shouldQuoteIdentifiers( - driver: string | null | undefined, + driver: string | PluginManifest | null | undefined, ): boolean { - return driver === "postgres"; + const driverStr = typeof driver === "object" ? driver?.id : driver; + return driverStr === "postgres" || driverStr === "postgresql"; } // PostgreSQL folds unquoted identifiers to lowercase and only needs quotes for @@ -50,7 +51,7 @@ const PG_RESERVED = new Set([ */ export function formatSqlIdentifier( identifier: string, - driver: string | null | undefined, + driver: string | PluginManifest | null | undefined, ): string { if (!shouldQuoteIdentifiers(driver)) return identifier; if (PG_SAFE_IDENTIFIER.test(identifier) && !PG_RESERVED.has(identifier)) { @@ -61,7 +62,7 @@ export function formatSqlIdentifier( export function quoteIdentifier( identifier: string, - driver: string | null | undefined, + driver: string | PluginManifest | null | undefined, ): string { const quote = getQuoteChar(driver); const escaped = @@ -78,7 +79,7 @@ export function quoteIdentifier( */ export function quoteTableRef( table: string, - driver: string | null | undefined, + driver: string | PluginManifest | null | undefined, schema?: string | null, ): string { if (schema) { diff --git a/src/utils/visualQuery.ts b/src/utils/visualQuery.ts index 06a3b2996..2881785ff 100644 --- a/src/utils/visualQuery.ts +++ b/src/utils/visualQuery.ts @@ -68,7 +68,6 @@ function formatTableRef( tableName: string, driver: string | null | undefined, ): string { - if (!driver) return tableName; return tableName .split('.') .map((part) => formatSqlIdentifier(part, driver)) @@ -92,6 +91,48 @@ function formatGeneratedColumnRef( return `${alias}.${formatSqlIdentifier(nameParts.join('.'), driver)}`; } +function formatAggregateArgument( + argument: string, + driver: string | null | undefined, +): string { + const trimmed = argument.trim(); + if ( + trimmed === '*' || + trimmed.includes('"') || + trimmed.includes('`') || + trimmed.includes('/') || + /[\s(),+\-*]/.test(trimmed) + ) { + return argument; + } + + return argument.replace(trimmed, formatGeneratedColumnRef(trimmed, driver)); +} + +function formatHavingColumnRef( + column: string, + driver: string | null | undefined, +): string { + const aggregateMatch = column.match(/^([A-Z_][A-Z0-9_]*)\((.*)\)$/i); + if (!aggregateMatch) return formatGeneratedColumnRef(column, driver); + + const [, aggregateFunction, argument] = aggregateMatch; + const distinctMatch = argument.match(/^(\s*DISTINCT\s+)(.+)$/i); + if (distinctMatch) { + const [, distinctPrefix, distinctArgument] = distinctMatch; + return `${aggregateFunction}(${distinctPrefix}${formatAggregateArgument(distinctArgument, driver)})`; + } + + return `${aggregateFunction}(${formatAggregateArgument(argument, driver)})`; +} + +function formatAlias( + alias: string, + driver: string | null | undefined, +): string { + return formatSqlIdentifier(alias, driver); +} + /** * Collects tables and their aliases from nodes */ @@ -157,7 +198,7 @@ export function collectSelectedColumns( } if (agg?.alias) { - colExpr += ` AS ${agg.alias}`; + colExpr += ` AS ${formatAlias(agg.alias, driver)}`; } if (agg?.order !== undefined) { @@ -167,7 +208,7 @@ export function collectSelectedColumns( nonAggregatedCols.push(columnRef); if (colAlias?.alias) { - colExpr += ` AS ${colAlias.alias}`; + colExpr += ` AS ${formatAlias(colAlias.alias, driver)}`; } if (colAlias?.order !== undefined) { @@ -326,13 +367,16 @@ export function generateGroupByClause( /** * Generates HAVING clause for aggregate conditions */ -export function generateHavingClause(conditions: WhereCondition[]): string { +export function generateHavingClause( + conditions: WhereCondition[], + driver?: string | null, +): string { const aggregateConditions = conditions.filter((c) => c.isAggregate && c.column && c.value); if (aggregateConditions.length === 0) return ''; const clauses = aggregateConditions.map((c, idx) => { - const condition = `${c.column} ${c.operator} ${c.value}`; + const condition = `${formatHavingColumnRef(c.column, driver)} ${c.operator} ${c.value}`; return idx === 0 ? condition : `${c.logicalOperator} ${condition}`; }); @@ -384,7 +428,7 @@ export function generateVisualQuerySQL( sql += generateFromClause(nodes, edges, aliases, driver); sql += generateWhereClause(whereConditions, driver); sql += generateGroupByClause(hasAggregation, nonAggregatedCols, groupBy, driver); - sql += generateHavingClause(whereConditions); + sql += generateHavingClause(whereConditions, driver); sql += generateOrderByClause(orderBy, driver); sql += generateLimitClause(limit); diff --git a/tests/utils/identifiers.test.ts b/tests/utils/identifiers.test.ts index 3348005b3..d744f1905 100644 --- a/tests/utils/identifiers.test.ts +++ b/tests/utils/identifiers.test.ts @@ -5,6 +5,7 @@ import { quoteTableRef, formatSqlIdentifier, } from '../../src/utils/identifiers'; +import type { PluginManifest } from '../../src/types/plugins'; describe('getQuoteChar', () => { it('should return backtick for mysql', () => { @@ -114,6 +115,17 @@ describe('formatSqlIdentifier', () => { expect(formatSqlIdentifier('AccountId', 'postgres')).toBe('"AccountId"'); }); + it('should quote identifiers for postgresql driver ids', () => { + expect(formatSqlIdentifier('AccountId', 'postgresql')).toBe('"AccountId"'); + expect(formatSqlIdentifier('user', 'postgresql')).toBe('"user"'); + }); + + it('should quote identifiers for PostgreSQL plugin manifests', () => { + const manifest = { id: 'postgresql' } as PluginManifest; + + expect(formatSqlIdentifier('AccountId', manifest)).toBe('"AccountId"'); + }); + it('should quote reserved words for postgres', () => { expect(formatSqlIdentifier('select', 'postgres')).toBe('"select"'); expect(formatSqlIdentifier('user', 'postgres')).toBe('"user"'); @@ -138,4 +150,4 @@ describe('formatSqlIdentifier', () => { expect(formatSqlIdentifier('users', 'sqlite')).toBe('users'); expect(formatSqlIdentifier('AccountEventLog', 'sqlite')).toBe('AccountEventLog'); }); -}); \ No newline at end of file +}); diff --git a/tests/utils/visualQuery.test.ts b/tests/utils/visualQuery.test.ts index a37af5ee7..f7d78dbf3 100644 --- a/tests/utils/visualQuery.test.ts +++ b/tests/utils/visualQuery.test.ts @@ -62,6 +62,19 @@ describe('visualQuery utils', () => { expect(result).toEqual(['users t1', 'posts t2']); }); + it('should quote table names that need it for postgres', () => { + const nodes: QueryNode[] = [ + { id: 'n1', data: { label: 'user', columns: [], selectedColumns: {} } }, + { id: 'n2', data: { label: 'AccountEventLog', columns: [], selectedColumns: {} } }, + ]; + const aliases = { n1: 't1', n2: 't2' }; + + expect(generateTableList(nodes, aliases, 'postgres')).toEqual([ + '"user" t1', + '"AccountEventLog" t2', + ]); + }); + it('should include schema-qualified MySQL table references', () => { const nodes: QueryNode[] = [ { id: 'n1', data: { label: 'users', schema: 'db_a', columns: [], selectedColumns: {} } }, @@ -167,6 +180,27 @@ describe('visualQuery utils', () => { expect(result.columns[0].expr).toBe('SUM(t1.total) AS total_sum'); }); + it('should quote aggregation aliases that need it for postgres', () => { + const nodes: QueryNode[] = [ + { + id: 'n1', + data: { + label: 'orders', + columns: [{ name: 'total', type: 'DECIMAL' }], + selectedColumns: { total: true }, + columnAggregations: { + total: { function: 'SUM', alias: 'Total Count' }, + }, + }, + }, + ]; + const aliases = { n1: 't1' }; + + const result = collectSelectedColumns(nodes, aliases, 'postgres'); + + expect(result.columns[0].expr).toBe('SUM(t1.total) AS "Total Count"'); + }); + it('should handle column aliases without aggregation', () => { const nodes: QueryNode[] = [ { @@ -189,6 +223,27 @@ describe('visualQuery utils', () => { expect(result.nonAggregatedCols).toContain('t1.first_name'); }); + it('should quote column aliases that need it for postgres', () => { + const nodes: QueryNode[] = [ + { + id: 'n1', + data: { + label: 'users', + columns: [{ name: 'first_name', type: 'VARCHAR' }], + selectedColumns: { first_name: true }, + columnAliases: { + first_name: { alias: 'Display Name' }, + }, + }, + }, + ]; + const aliases = { n1: 't1' }; + + const result = collectSelectedColumns(nodes, aliases, 'postgres'); + + expect(result.columns[0].expr).toBe('t1.first_name AS "Display Name"'); + }); + it('should handle custom ordering', () => { const nodes: QueryNode[] = [ { @@ -624,6 +679,37 @@ describe('visualQuery utils', () => { it('should return empty string for no aggregate conditions', () => { expect(generateHavingClause([])).toBe(''); }); + + it('should quote generated HAVING column references for postgres', () => { + const conditions: WhereCondition[] = [ + { id: '1', column: 't1.AccountId', operator: '>', value: '0', logicalOperator: 'AND', isAggregate: true }, + ]; + + expect(generateHavingClause(conditions, 'postgres')).toBe( + '\nHAVING\n t1."AccountId" > 0', + ); + }); + + it('should preserve COUNT(*) HAVING expressions for postgres', () => { + const conditions: WhereCondition[] = [ + { id: '1', column: 'COUNT(*)', operator: '>', value: '0', logicalOperator: 'AND', isAggregate: true }, + ]; + + expect(generateHavingClause(conditions, 'postgres')).toBe( + '\nHAVING\n COUNT(*) > 0', + ); + }); + + it('should preserve aggregate HAVING expressions while formatting postgres column refs', () => { + const conditions: WhereCondition[] = [ + { id: '1', column: 'SUM(t1.amount)', operator: '>', value: '1000', logicalOperator: 'AND', isAggregate: true }, + { id: '2', column: 'COUNT(t1.AccountId)', operator: '>', value: '0', logicalOperator: 'AND', isAggregate: true }, + ]; + + expect(generateHavingClause(conditions, 'postgres')).toBe( + '\nHAVING\n SUM(t1.amount) > 1000\n AND COUNT(t1."AccountId") > 0', + ); + }); }); describe('generateOrderByClause', () => { @@ -801,5 +887,48 @@ describe('visualQuery utils', () => { expect(result).toContain('t1."order"'); expect(result).toContain('FROM\n "user" t1'); }); + + it('should quote postgres SQL when the driver id is postgresql', () => { + const nodes: QueryNode[] = [ + { + id: 'n1', + data: { + label: 'AccountEventLog', + columns: [{ name: 'AccountId', type: 'INT' }], + selectedColumns: { AccountId: true }, + }, + }, + ]; + + const result = generateVisualQuerySQL(nodes, [], [], [], [], '', 'postgresql'); + + expect(result).toContain('t1."AccountId"'); + expect(result).toContain('FROM\n "AccountEventLog" t1'); + }); + + it('should generate postgres SQL with quoted HAVING refs and aliases', () => { + const nodes: QueryNode[] = [ + { + id: 'n1', + data: { + label: 'AccountEventLog', + columns: [{ name: 'AccountId', type: 'INT' }], + selectedColumns: { AccountId: true }, + columnAggregations: { + AccountId: { function: 'COUNT', alias: 'Total Count' }, + }, + }, + }, + ]; + const whereConditions: WhereCondition[] = [ + { id: '1', column: 'COUNT(t1.AccountId)', operator: '>', value: '0', logicalOperator: 'AND', isAggregate: true }, + ]; + + const result = generateVisualQuerySQL(nodes, [], whereConditions, [], [], '', 'postgres'); + + expect(result).toContain('COUNT(t1."AccountId") AS "Total Count"'); + expect(result).toContain('FROM\n "AccountEventLog" t1'); + expect(result).toContain('HAVING\n COUNT(t1."AccountId") > 0'); + }); }); });