From b23f01526a9287dd07f9eb42e1b68e41cf568f40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ad=C3=A1mek?= Date: Sun, 9 Oct 2022 18:26:49 +0200 Subject: [PATCH] feat(query-builder): validate modification of finalized QB Once QB is finalized, it can't be modified as it could lead to invalid state. This is now validated and disallowed, to modify a finalized QB, first clone it. Closes #3534 --- packages/knex/src/query/QueryBuilder.ts | 37 ++++++++++++++++++++++++- tests/EntityManager.mysql.test.ts | 2 +- tests/QueryBuilder.test.ts | 3 ++ 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/packages/knex/src/query/QueryBuilder.ts b/packages/knex/src/query/QueryBuilder.ts index 465af76b9ffe..633792eeaa71 100644 --- a/packages/knex/src/query/QueryBuilder.ts +++ b/packages/knex/src/query/QueryBuilder.ts @@ -134,6 +134,7 @@ export class QueryBuilder { } select(fields: Field | Field[], distinct = false): SelectQueryBuilder { + this.ensureNotFinalized(); this._fields = Utils.asArray(fields); if (distinct) { @@ -144,6 +145,8 @@ export class QueryBuilder { } addSelect(fields: Field | Field[]): SelectQueryBuilder { + this.ensureNotFinalized(); + if (this.type && this.type !== QueryType.SELECT) { return this as SelectQueryBuilder; } @@ -152,11 +155,13 @@ export class QueryBuilder { } distinct(): SelectQueryBuilder { + this.ensureNotFinalized(); return this.setFlag(QueryFlag.DISTINCT) as SelectQueryBuilder; } /** postgres only */ distinctOn(fields: string | string[]): SelectQueryBuilder { + this.ensureNotFinalized(); this._distinctOn = Utils.asArray(fields); return this as SelectQueryBuilder; } @@ -238,6 +243,7 @@ export class QueryBuilder { } withSubQuery(subQuery: Knex.QueryBuilder, alias: string): this { + this.ensureNotFinalized(); this.subQueries[alias] = subQuery.toString(); return this; } @@ -245,6 +251,8 @@ export class QueryBuilder { where(cond: QBFilterQuery, operator?: keyof typeof GroupOperator): this; where(cond: string, params?: any[], operator?: keyof typeof GroupOperator): this; where(cond: QBFilterQuery | string, params?: keyof typeof GroupOperator | any[], operator?: keyof typeof GroupOperator): this { + this.ensureNotFinalized(); + if (Utils.isString(cond)) { cond = { [`(${cond})`]: Utils.asArray(params) }; operator = operator || '$and'; @@ -300,6 +308,7 @@ export class QueryBuilder { } orderBy(orderBy: QBQueryOrderMap | QBQueryOrderMap[]): this { + this.ensureNotFinalized(); this._orderBy = []; Utils.asArray(orderBy).forEach(o => { const processed = QueryHelper.processWhere({ @@ -318,11 +327,14 @@ export class QueryBuilder { } groupBy(fields: (string | keyof T) | readonly (string | keyof T)[]): this { + this.ensureNotFinalized(); this._groupBy = Utils.asArray(fields); return this; } having(cond: QBFilterQuery | string = {}, params?: any[]): this { + this.ensureNotFinalized(); + if (Utils.isString(cond)) { cond = { [`(${cond})`]: Utils.asArray(params) }; } @@ -332,6 +344,7 @@ export class QueryBuilder { } onConflict(fields: Field | Field[] = []): this { + this.ensureNotFinalized(); this._onConflict = this._onConflict || []; this._onConflict.push({ fields: Utils.asArray(fields).map(f => f.toString()) }); return this; @@ -359,6 +372,7 @@ export class QueryBuilder { * @internal */ populate(populate: PopulateOptions[], populateWhere?: ObjectQuery | PopulateHint): this { + this.ensureNotFinalized(); this._populate = populate; this._populateWhere = populateWhere; @@ -380,6 +394,7 @@ export class QueryBuilder { } limit(limit?: number, offset = 0): this { + this.ensureNotFinalized(); this._limit = limit; if (offset) { @@ -390,17 +405,21 @@ export class QueryBuilder { } offset(offset?: number): this { + this.ensureNotFinalized(); this._offset = offset; return this; } withSchema(schema?: string): this { + this.ensureNotFinalized(); this._schema = schema; return this; } setLockMode(mode?: LockMode, tables?: string[]): this { + this.ensureNotFinalized(); + if (mode != null && mode !== LockMode.OPTIMISTIC && !this.context) { throw ValidationError.transactionRequired(); } @@ -412,21 +431,25 @@ export class QueryBuilder { } setFlushMode(flushMode?: FlushMode): this { + this.ensureNotFinalized(); this.flushMode = flushMode; return this; } setFlag(flag: QueryFlag): this { + this.ensureNotFinalized(); this.flags.add(flag); return this; } unsetFlag(flag: QueryFlag): this { + this.ensureNotFinalized(); this.flags.delete(flag); return this; } cache(config: boolean | number | [string, number] = true): this { + this.ensureNotFinalized(); this._cache = config; return this; } @@ -435,6 +458,7 @@ export class QueryBuilder { * Adds index hint to the FROM clause. */ indexHint(sql: string): this { + this.ensureNotFinalized(); this._indexHint = sql; return this; } @@ -446,6 +470,8 @@ export class QueryBuilder { from = AnyEntity>(target: QueryBuilder, aliasName?: string): QueryBuilder; from = AnyEntity>(target: EntityName): QueryBuilder; from = AnyEntity>(target: EntityName | QueryBuilder, aliasName?: string): QueryBuilder { + this.ensureNotFinalized(); + if (target instanceof QueryBuilder) { this.fromSubQuery(target, aliasName); } else { @@ -721,6 +747,7 @@ export class QueryBuilder { } private joinReference(field: string, alias: string, cond: Dictionary, type: 'leftJoin' | 'innerJoin' | 'pivotJoin', path?: string): EntityProperty { + this.ensureNotFinalized(); const [fromAlias, fromField] = this.helper.splitField(field); const entityName = this._aliases[fromAlias]?.entityName; const meta = this.metadata.get(entityName); @@ -843,6 +870,7 @@ export class QueryBuilder { } private init(type: QueryType, data?: any, cond?: any): this { + this.ensureNotFinalized(); this.type = type; if ([QueryType.UPDATE, QueryType.DELETE].includes(type) && Utils.hasObjectKeys(this._cond)) { @@ -973,7 +1001,6 @@ export class QueryBuilder { QueryHelper.processObjectParams(this._data); QueryHelper.processObjectParams(this._cond); QueryHelper.processObjectParams(this._having); - this.finalized = true; // automatically enable paginate flag when we detect to-many joins if (!this.flags.has(QueryFlag.DISABLE_PAGINATE) && this.hasToManyJoins()) { @@ -987,6 +1014,8 @@ export class QueryBuilder { if (meta && (this.flags.has(QueryFlag.UPDATE_SUB_QUERY) || this.flags.has(QueryFlag.DELETE_SUB_QUERY))) { this.wrapModifySubQuery(meta); } + + this.finalized = true; } private hasToManyJoins(): boolean { @@ -1109,6 +1138,12 @@ export class QueryBuilder { } } + private ensureNotFinalized(): void { + if (this.finalized) { + throw new Error('This QueryBuilder instance is already finalized, clone it first if you want to modify it.'); + } + } + } export interface RunQueryBuilder extends Omit, 'getResult' | 'getSingleResult' | 'getResultList' | 'where'> { diff --git a/tests/EntityManager.mysql.test.ts b/tests/EntityManager.mysql.test.ts index 3e16adff7494..02d0cb97bf52 100644 --- a/tests/EntityManager.mysql.test.ts +++ b/tests/EntityManager.mysql.test.ts @@ -861,7 +861,7 @@ describe('EntityManagerMySql', () => { const qb2 = orm.em.createQueryBuilder(Book2); const res2 = await qb2.select('*').where({ title: 'not exists' }).getSingleResult(); expect(res2).toBeNull(); - const res3 = await qb1.select('*').getResult(); + const res3 = await qb1.clone().select('*').getResult(); expect(res3).toHaveLength(1); }); diff --git a/tests/QueryBuilder.test.ts b/tests/QueryBuilder.test.ts index fdd77a50bf0c..35dd1b1e41dc 100644 --- a/tests/QueryBuilder.test.ts +++ b/tests/QueryBuilder.test.ts @@ -43,6 +43,9 @@ describe('QueryBuilder', () => { .where({ name: 'test 123', type: PublisherType.GLOBAL }) .orderBy({ [`(point(location_latitude, location_longitude) <@> point(${53.46}, ${9.90}))`]: 'ASC' }); expect(qb2.getFormattedQuery()).toBe('select `e0`.* from `publisher2` as `e0` where `e0`.`name` = \'test 123\' and `e0`.`type` = \'global\' order by (point(location_latitude, location_longitude) <@> point(53.46, 9.9)) asc'); + + // trying to modify finalized QB will throw + expect(() => qb2.where('foo = 123')).toThrowError('This QueryBuilder instance is already finalized, clone it first if you want to modify it.'); }); test('select query picks read replica', async () => {