Skip to content

fix: correct precedence parentheses for SQLFragment joins #476

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wschurman merged 1 commit into from
Feb 27, 2026
Merged

fix: correct precedence parentheses for SQLFragment joins #476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
45 changes: 29 additions & 16 deletions packages/entity-database-adapter-knex/src/SQLOperator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,19 +48,18 @@ export class SQLFragment {
* Combine SQL fragments
*/
append(other: SQLFragment): SQLFragment {
return SQLFragment.join([this, other], ' ');
return joinSQLFragments([this, other], ' ');
}

/**
* Join multiple SQL fragments into a single fragment with a separator.
* Join multiple SQL fragments with a comma separator.
* Useful for combining column lists, value lists, etc.
*
* @param fragments - Array of SQL fragments to join
* @param separator - Separator string (default: ', ')
* @returns - A new SQLFragment with the fragments joined by a comma and space
*/
static join(fragments: readonly SQLFragment[], separator = ', '): SQLFragment {
return new SQLFragment(
fragments.map((f) => f.sql).join(separator),
fragments.flatMap((f) => f.bindings),
);
static joinWithCommaSeparator(...fragments: readonly SQLFragment[]): SQLFragment {
return joinSQLFragments(fragments, ', ');
}

/**
Expand All @@ -75,8 +74,8 @@ export class SQLFragment {
* // Generates: "SELECT * FROM users WHERE age > ? ORDER BY name"
* ```
*/
static concat(...fragments: SQLFragment[]): SQLFragment {
return SQLFragment.join(fragments, ' ');
static concat(...fragments: readonly SQLFragment[]): SQLFragment {
return joinSQLFragments(fragments, ' ');
}

/**
Expand Down Expand Up @@ -432,34 +431,40 @@ export const SQLFragmentHelpers = {
* JSON path extraction helper (-\>)
*/
jsonPath(column: string, path: string): SQLFragment {
return new SQLFragment(`"${column}"->'${path}'`, []);
return sql`${identifier(column)}->${path}`;
},

/**
* JSON path text extraction helper (-\>\>)
*/
jsonPathText(column: string, path: string): SQLFragment {
return new SQLFragment(`"${column}"->>'${path}'`, []);
return sql`${identifier(column)}->>${path}`;
},

/**
* Logical AND of multiple fragments
*/
and(...conditions: SQLFragment[]): SQLFragment {
and(...conditions: readonly SQLFragment[]): SQLFragment {
if (conditions.length === 0) {
return sql`1 = 1`;
}
return SQLFragment.join(conditions, ' AND ');
return joinSQLFragments(
conditions.map((c) => SQLFragmentHelpers.group(c)),
' AND ',
);
},

/**
* Logical OR of multiple fragments
*/
or(...conditions: SQLFragment[]): SQLFragment {
or(...conditions: readonly SQLFragment[]): SQLFragment {
if (conditions.length === 0) {
return sql`1 = 0`;
}
return SQLFragment.join(conditions, ' OR ');
return joinSQLFragments(
conditions.map((c) => SQLFragmentHelpers.group(c)),
' OR ',
);
},

/**
Expand All @@ -476,3 +481,11 @@ export const SQLFragmentHelpers = {
return new SQLFragment('(' + condition.sql + ')', condition.bindings);
},
};

// Internal helper function to join SQL fragments with a specified separator
function joinSQLFragments(fragments: readonly SQLFragment[], separator: string): SQLFragment {
return new SQLFragment(
fragments.map((f) => f.sql).join(separator),
fragments.flatMap((f) => f.bindings),
);
}
4 changes: 2 additions & 2 deletions .../entity-database-adapter-knex/src/__integration-tests__/PostgresEntityIntegration-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { setTimeout } from 'timers/promises';
import { PaginationSpecification } from '../AuthorizationResultBasedKnexEntityLoader';
import { NullsOrdering, OrderByOrdering } from '../BasePostgresEntityDatabaseAdapter';
import { PaginationStrategy } from '../PaginationStrategy';
import { raw, sql, SQLFragment, SQLFragmentHelpers } from '../SQLOperator';
import { raw, sql, SQLFragmentHelpers } from '../SQLOperator';
import {
PostgresTestEntity,
PostgresTestEntityFields,
Expand Down Expand Up @@ -695,7 +695,7 @@ describe('postgres entity integration', () => {
sql`(has_a_cat = ${true} AND has_a_dog = ${true})`,
];
const joinedResults = await PostgresTestEntity.knexLoader(vc1)
.loadManyBySQL(SQLFragment.join(conditions, ' OR '))
.loadManyBySQL(SQLFragmentHelpers.or(...conditions))
.orderBy('name', OrderByOrdering.ASCENDING)
.executeAsync();

Expand Down
52 changes: 16 additions & 36 deletions packages/entity-database-adapter-knex/src/__tests__/SQLOperator-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,45 +109,25 @@ describe('SQLOperator', () => {
});
});

describe(SQLFragment.join, () => {
it('joins fragments with custom separator', () => {
const fragments = [
new SQLFragment('name = ?', [{ type: 'value', value: 'Alice' }]),
new SQLFragment('age = ?', [{ type: 'value', value: 30 }]),
new SQLFragment('city = ?', [{ type: 'value', value: 'NYC' }]),
];
const joined = SQLFragment.join(fragments, ' AND ');

expect(joined.sql).toBe('name = ? AND age = ? AND city = ?');
expect(joined.getKnexBindings()).toEqual(['Alice', 30, 'NYC']);
});

describe(SQLFragment.joinWithCommaSeparator, () => {
it('handles empty array in join', () => {
const joined = SQLFragment.join([]);
const joined = SQLFragment.joinWithCommaSeparator();

expect(joined.sql).toBe('');
expect(joined.getKnexBindings()).toEqual([]);
});

it('joins SQL fragments with default separator', () => {
it('joins SQL fragments with comma', () => {
const columns = [sql`name`, sql`age`, sql`email`];
const joined = SQLFragment.join(columns);
const joined = SQLFragment.joinWithCommaSeparator(...columns);

expect(joined.sql).toBe('name, age, email');
expect(joined.getKnexBindings()).toEqual([]);
});

it('joins SQL fragments with custom separator', () => {
const conditions = [sql`age > ${18}`, sql`status = ${'active'}`, sql`verified = ${true}`];
const joined = SQLFragment.join(conditions, ' AND ');

expect(joined.sql).toBe('age > ? AND status = ? AND verified = ?');
expect(joined.getKnexBindings()).toEqual([18, 'active', true]);
});

it('handles single fragment', () => {
const single = [sql`name = ${'Alice'}`];
const joined = SQLFragment.join(single);
const joined = SQLFragment.joinWithCommaSeparator(...single);

expect(joined.sql).toBe('name = ?');
expect(joined.getKnexBindings()).toEqual(['Alice']);
Expand Down Expand Up @@ -191,7 +171,7 @@ describe('SQLOperator', () => {
filters.push(sql`category = ${'electronics'}`);

if (filters.length > 0) {
fragments.push(sql`WHERE ${SQLFragment.join(filters, ' AND ')}`);
fragments.push(sql`WHERE ${SQLFragmentHelpers.and(...filters)}`);
}

// Add ORDER BY
Expand All @@ -203,7 +183,7 @@ describe('SQLOperator', () => {
const query = SQLFragment.concat(...fragments);

expect(query.sql).toBe(
'SELECT * FROM products WHERE price > ? AND category = ? ORDER BY created_at DESC LIMIT ?',
'SELECT * FROM products WHERE (price > ?) AND (category = ?) ORDER BY created_at DESC LIMIT ?',
);
expect(query.getKnexBindings()).toEqual([100, 'electronics', 10]);
});
Expand Down Expand Up @@ -632,17 +612,17 @@ describe('SQLOperator', () => {
it('generates JSON path access', () => {
const fragment = SQLFragmentHelpers.jsonPath('data', 'user');

expect(fragment.sql).toBe(`"data"->'user'`);
expect(fragment.getKnexBindings()).toEqual([]);
expect(fragment.sql).toBe(`??->?`);
expect(fragment.getKnexBindings()).toEqual(['data', 'user']);
});
});

describe(SQLFragmentHelpers.jsonPathText, () => {
it('generates JSON path text access', () => {
const fragment = SQLFragmentHelpers.jsonPathText('data', 'email');

expect(fragment.sql).toBe(`"data"->>'email'`);
expect(fragment.getKnexBindings()).toEqual([]);
expect(fragment.sql).toBe(`??->>?`);
expect(fragment.getKnexBindings()).toEqual(['data', 'email']);
});
});

Expand All @@ -652,15 +632,15 @@ describe('SQLOperator', () => {
const cond2 = sql`status = ${'active'}`;
const fragment = SQLFragmentHelpers.and(cond1, cond2);

expect(fragment.sql).toBe('age >= ? AND status = ?');
expect(fragment.sql).toBe('(age >= ?) AND (status = ?)');
expect(fragment.getKnexBindings()).toEqual([18, 'active']);
});

it('handles single condition in AND', () => {
const cond = sql`age >= ${18}`;
const fragment = SQLFragmentHelpers.and(cond);

expect(fragment.sql).toBe('age >= ?');
expect(fragment.sql).toBe('(age >= ?)');
expect(fragment.getKnexBindings()).toEqual([18]);
});

Expand All @@ -678,15 +658,15 @@ describe('SQLOperator', () => {
const cond2 = sql`status = ${'pending'}`;
const fragment = SQLFragmentHelpers.or(cond1, cond2);

expect(fragment.sql).toBe('status = ? OR status = ?');
expect(fragment.sql).toBe('(status = ?) OR (status = ?)');
expect(fragment.getKnexBindings()).toEqual(['active', 'pending']);
});

it('handles single condition in OR', () => {
const cond = sql`status = ${'active'}`;
const fragment = SQLFragmentHelpers.or(cond);

expect(fragment.sql).toBe('status = ?');
expect(fragment.sql).toBe('(status = ?)');
expect(fragment.getKnexBindings()).toEqual(['active']);
});

Expand Down Expand Up @@ -729,7 +709,7 @@ describe('SQLOperator', () => {
);

expect(fragment.sql).toBe(
'?? BETWEEN ? AND ? AND (?? IN (?, ?) OR role = ?) AND ?? IS NOT NULL',
'(?? BETWEEN ? AND ?) AND (((?? IN (?, ?)) OR (role = ?))) AND (?? IS NOT NULL)',
);
expect(fragment.getKnexBindings()).toEqual([
'age',
Expand Down
22 changes: 12 additions & 10 deletions packages/entity-database-adapter-knex/src/internal/EntityKnexDataManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,7 +594,7 @@ export class EntityKnexDataManager<
);

// Build left side of comparison (current row's computed values)
const leftSide = SQLFragment.join(postgresCursorFieldIdentifiers, ', ');
const leftSide = SQLFragment.joinWithCommaSeparator(...postgresCursorFieldIdentifiers);

// Build right side using subquery to get computed values for cursor entity.
// For field names, qualify with the cursor row alias. For SQL fragments,
Expand All @@ -605,7 +605,7 @@ export class EntityKnexDataManager<

// Build SELECT fields for subquery
const rightSideSubquery = sql`
SELECT ${SQLFragment.join(postgresCursorRowFieldIdentifiers, ', ')}
SELECT ${SQLFragment.joinWithCommaSeparator(...postgresCursorRowFieldIdentifiers)}
FROM ${identifier(tableName)} AS ${raw(CURSOR_ROW_TABLE_ALIAS)}
WHERE ${raw(CURSOR_ROW_TABLE_ALIAS)}.${identifier(idField)} = ${decodedExternalCursorEntityID}
`;
Expand Down Expand Up @@ -637,15 +637,15 @@ export class EntityKnexDataManager<
tableAlias?: typeof CURSOR_ROW_TABLE_ALIAS,
): SQLFragment {
const ilikeConditions = this.buildILikeConditions(search, tableAlias);
return sql`CASE WHEN ${SQLFragment.join(ilikeConditions, ' OR ')} THEN 1 ELSE 0 END`;
return sql`CASE WHEN ${SQLFragmentHelpers.or(...ilikeConditions)} THEN 1 ELSE 0 END`;
}

private buildTrigramSimilarityGreatestExpression(
search: DataManagerSearchSpecification<TFields>,
tableAlias?: typeof CURSOR_ROW_TABLE_ALIAS,
): SQLFragment {
const similarityExprs = this.buildTrigramSimilarityExpressions(search, tableAlias);
return sql`GREATEST(${SQLFragment.join(similarityExprs, ', ')})`;
return sql`GREATEST(${SQLFragment.joinWithCommaSeparator(...similarityExprs)})`;
}

private buildTrigramCursorCondition(
Expand All @@ -671,9 +671,11 @@ export class EntityKnexDataManager<
extraOrderByFields?.map((f) => this.resolveSearchFieldToSQLFragment(f)) ?? [];

// Build left side of comparison (current row's computed values)
const leftSide = SQLFragment.join(
[exactMatchExpr, similarityExpr, ...extraFields, sql`${identifier(idField)}`],
', ',
const leftSide = SQLFragment.joinWithCommaSeparator(
exactMatchExpr,
similarityExpr,
...extraFields,
sql`${identifier(idField)}`,
);

// Build right side using subquery to get computed values for cursor entity
Expand Down Expand Up @@ -702,7 +704,7 @@ export class EntityKnexDataManager<
];

const rightSideSubquery = sql`
SELECT ${SQLFragment.join(selectFields, ', ')}
SELECT ${SQLFragment.joinWithCommaSeparator(...selectFields)}
FROM ${identifier(this.entityConfiguration.tableName)} AS ${raw(CURSOR_ROW_TABLE_ALIAS)}
WHERE ${raw(CURSOR_ROW_TABLE_ALIAS)}.${identifier(idField)} = ${decodedExternalCursorEntityID}
`;
Expand Down Expand Up @@ -733,7 +735,7 @@ export class EntityKnexDataManager<
];

return {
searchWhere: conditions.length > 0 ? SQLFragment.join(conditions, ' OR ') : sql`1 = 0`,
searchWhere: conditions.length > 0 ? SQLFragmentHelpers.or(...conditions) : sql`1 = 0`,
searchOrderByClauses,
};
}
Expand Down Expand Up @@ -777,7 +779,7 @@ export class EntityKnexDataManager<
];

return {
searchWhere: SQLFragment.join(allConditions, ' OR '),
searchWhere: SQLFragmentHelpers.or(...allConditions),
searchOrderByClauses,
};
}
Expand Down