Skip to content

Commit

Permalink
feat(query-builder): validate modification of finalized QB
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@B4nan
B4nan committed Oct 9, 2022
1 parent 4bbe355 commit b23f015
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 2 deletions.
37 changes: 36 additions & 1 deletion packages/knex/src/query/QueryBuilder.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ export class QueryBuilder<T extends object = AnyEntity> {
}

select(fields: Field<T> | Field<T>[], distinct = false): SelectQueryBuilder<T> {
this.ensureNotFinalized();
this._fields = Utils.asArray(fields);

if (distinct) {
Expand All @@ -144,6 +145,8 @@ export class QueryBuilder<T extends object = AnyEntity> {
}

addSelect(fields: Field<T> | Field<T>[]): SelectQueryBuilder<T> {
this.ensureNotFinalized();

if (this.type && this.type !== QueryType.SELECT) {
return this as SelectQueryBuilder<T>;
}
Expand All @@ -152,11 +155,13 @@ export class QueryBuilder<T extends object = AnyEntity> {
}

distinct(): SelectQueryBuilder<T> {
this.ensureNotFinalized();
return this.setFlag(QueryFlag.DISTINCT) as SelectQueryBuilder<T>;
}

/** postgres only */
distinctOn(fields: string | string[]): SelectQueryBuilder<T> {
this.ensureNotFinalized();
this._distinctOn = Utils.asArray(fields);
return this as SelectQueryBuilder<T>;
}
Expand Down Expand Up @@ -238,13 +243,16 @@ export class QueryBuilder<T extends object = AnyEntity> {
}

withSubQuery(subQuery: Knex.QueryBuilder, alias: string): this {
this.ensureNotFinalized();
this.subQueries[alias] = subQuery.toString();
return this;
}

where(cond: QBFilterQuery<T>, operator?: keyof typeof GroupOperator): this;
where(cond: string, params?: any[], operator?: keyof typeof GroupOperator): this;
where(cond: QBFilterQuery<T> | 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';
Expand Down Expand Up @@ -300,6 +308,7 @@ export class QueryBuilder<T extends object = AnyEntity> {
}

orderBy(orderBy: QBQueryOrderMap<T> | QBQueryOrderMap<T>[]): this {
this.ensureNotFinalized();
this._orderBy = [];
Utils.asArray(orderBy).forEach(o => {
const processed = QueryHelper.processWhere({
Expand All @@ -318,11 +327,14 @@ export class QueryBuilder<T extends object = AnyEntity> {
}

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) };
}
Expand All @@ -332,6 +344,7 @@ export class QueryBuilder<T extends object = AnyEntity> {
}

onConflict(fields: Field<T> | Field<T>[] = []): this {
this.ensureNotFinalized();
this._onConflict = this._onConflict || [];
this._onConflict.push({ fields: Utils.asArray(fields).map(f => f.toString()) });
return this;
Expand Down Expand Up @@ -359,6 +372,7 @@ export class QueryBuilder<T extends object = AnyEntity> {
* @internal
*/
populate(populate: PopulateOptions<T>[], populateWhere?: ObjectQuery<T> | PopulateHint): this {
this.ensureNotFinalized();
this._populate = populate;
this._populateWhere = populateWhere;

Expand All @@ -380,6 +394,7 @@ export class QueryBuilder<T extends object = AnyEntity> {
}

limit(limit?: number, offset = 0): this {
this.ensureNotFinalized();
this._limit = limit;

if (offset) {
Expand All @@ -390,17 +405,21 @@ export class QueryBuilder<T extends object = AnyEntity> {
}

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();
}
Expand All @@ -412,21 +431,25 @@ export class QueryBuilder<T extends object = AnyEntity> {
}

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;
}
Expand All @@ -435,6 +458,7 @@ export class QueryBuilder<T extends object = AnyEntity> {
* Adds index hint to the FROM clause.
*/
indexHint(sql: string): this {
this.ensureNotFinalized();
this._indexHint = sql;
return this;
}
Expand All @@ -446,6 +470,8 @@ export class QueryBuilder<T extends object = AnyEntity> {
from<T extends AnyEntity<T> = AnyEntity>(target: QueryBuilder<T>, aliasName?: string): QueryBuilder<T>;
from<T extends AnyEntity<T> = AnyEntity>(target: EntityName<T>): QueryBuilder<T>;
from<T extends AnyEntity<T> = AnyEntity>(target: EntityName<T> | QueryBuilder<T>, aliasName?: string): QueryBuilder<T> {
this.ensureNotFinalized();

if (target instanceof QueryBuilder) {
this.fromSubQuery(target, aliasName);
} else {
Expand Down Expand Up @@ -721,6 +747,7 @@ export class QueryBuilder<T extends object = AnyEntity> {
}

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);
Expand Down Expand Up @@ -843,6 +870,7 @@ export class QueryBuilder<T extends object = AnyEntity> {
}

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)) {
Expand Down Expand Up @@ -973,7 +1001,6 @@ export class QueryBuilder<T extends object = AnyEntity> {
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()) {
Expand All @@ -987,6 +1014,8 @@ export class QueryBuilder<T extends object = AnyEntity> {
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 {
Expand Down Expand Up @@ -1109,6 +1138,12 @@ export class QueryBuilder<T extends object = AnyEntity> {
}
}

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<T extends object> extends Omit<QueryBuilder<T>, 'getResult' | 'getSingleResult' | 'getResultList' | 'where'> {
Expand Down
2 changes: 1 addition & 1 deletion tests/EntityManager.mysql.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});

Expand Down
3 changes: 3 additions & 0 deletions tests/QueryBuilder.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down

0 comments on commit b23f015

Please sign in to comment.