also looking at this
fix(orm): scope destructive statements the way reads are scoped
#1116
2 files
+210
-7
| @@ -2672,6 +2672,33 @@ class ModelQueryBuilder< | ||
| 2672 | 2672 | * the soft-delete predicate. User clauses are parenthesised when both are |
| 2673 | 2673 | * present so a top-level `OR` can't escape the soft-delete filter. |
| 2674 | 2674 | */ |
| 2675 | /** | |
| 2676 | * Refuse a destructive statement that carries clauses it will not emit. | |
| 2677 | * | |
| 2678 | * `delete()`, `update()` and `increment()` read none of `_limit`, `_offset` | |
| 2679 | * or `_orderBy`, so `query().where(...).orderBy('id').limit(1).delete()` | |
| 2680 | * quietly deleted every matching row rather than one — the caller's cap was | |
| 2681 | * dropped on the floor. LIMIT on an UPDATE/DELETE is not portable (Postgres | |
| 2682 | * has no such form), so these cannot simply be emitted; being loud is the | |
| 2683 | * honest option. See #1111. | |
| 2684 | */ | |
| 2685 | private assertNoUnappliedClauses(method: string): void { | |
| 2686 | const ignored: string[] = [] | |
| 2687 | if (this._limit !== undefined) | |
| 2688 | ignored.push('limit()') | |
| 2689 | if (this._offset !== undefined) | |
| 2690 | ignored.push('offset()') | |
| 2691 | if (this._orderBy.length > 0) | |
| 2692 | ignored.push('orderBy()') | |
| 2693 | ||
| 2694 | if (ignored.length > 0) { | |
| 2695 | throw new TypeError( | |
| 2696 | `[orm] ${method}() cannot apply ${ignored.join(', ')} — an UPDATE/DELETE takes no ORDER BY or LIMIT on every supported dialect. ` | |
| 2697 | + `Previously these were ignored, so the statement affected every matching row. Narrow it with where() instead.`, | |
| 2698 | ) | |
| 2699 | } | |
| 2700 | } | |
| 2701 | ||
| 2675 | 2702 | private composeWhere(params: unknown[]): string { |
| 2676 | 2703 | const userClause = this._wheres.length > 0 ? this.buildWhereClauses(params) : '' |
| 2677 | 2704 | const sd = this.softDeleteClause() |
| @@ -3020,6 +3047,7 @@ class ModelQueryBuilder< | ||
| 3020 | 3047 | */ |
| 3021 | 3048 | async increment<K extends NumericColumns<TDef>>(column: K, amount = 1): Promise<number> { |
| 3022 | 3049 | assertValidIdentifier(column, 'increment(column)') |
| 3050 | this.assertNoUnappliedClauses('increment') | |
| 3023 | 3051 | const exec = getExecutor() |
| 3024 | 3052 | const params: unknown[] = [amount] |
| 3025 | 3053 | |
| @@ -3031,9 +3059,11 @@ class ModelQueryBuilder< | ||
| 3031 | 3059 | params.push(formatNow()) |
| 3032 | 3060 | } |
| 3033 | 3061 | |
| 3034 | if (this._wheres.length > 0) { | |
| 3035 | const clauses = this.buildWhereClauses(params) | |
| 3036 | sql += ` WHERE ${clauses}` | |
| 3062 | // See delete(): composeWhere is the only path that adds the soft-delete | |
| 3063 | // predicate, so a scoped counter bump has to go through it. #1111. | |
| 3064 | const whereBody = this.composeWhere(params) | |
| 3065 | if (whereBody) { | |
| 3066 | sql += ` WHERE ${whereBody}` | |
| 3037 | 3067 | } |
| 3038 | 3068 | |
| 3039 | 3069 | return (await exec.run(sql, params)).changes |
| @@ -3185,18 +3215,26 @@ class ModelQueryBuilder< | ||
| 3185 | 3215 | } |
| 3186 | 3216 | |
| 3187 | 3217 | async delete(): Promise<number> { |
| 3218 | this.assertNoUnappliedClauses('delete') | |
| 3188 | 3219 | const exec = getExecutor() |
| 3189 | 3220 | const params: unknown[] = [] |
| 3190 | 3221 | let sql = `DELETE FROM ${this._definition.table}` |
| 3191 | 3222 | |
| 3192 | if (this._wheres.length > 0) { | |
| 3193 | sql += ` WHERE ${this.buildWhereClauses(params)}` | |
| 3223 | // composeWhere, not buildWhereClauses: the soft-delete predicate is added | |
| 3224 | // only by composeWhere, so building the WHERE here meant a scope that | |
| 3225 | // exists purely as that predicate contributed nothing to the statement. | |
| 3226 | // `onlyTrashed().delete()` — purge the trash — emitted a bare DELETE and | |
| 3227 | // removed every row, live ones included. See #1111. | |
| 3228 | const whereBody = this.composeWhere(params) | |
| 3229 | if (whereBody) { | |
| 3230 | sql += ` WHERE ${whereBody}` | |
| 3194 | 3231 | } |
| 3195 | 3232 | |
| 3196 | 3233 | return (await exec.run(sql, params)).changes |
| 3197 | 3234 | } |
| 3198 | 3235 | |
| 3199 | 3236 | async update(data: Partial<Pick<InferModelAttributes<TDef>, FillableKeys<TDef>>>): Promise<number> { |
| 3237 | this.assertNoUnappliedClauses('update') | |
| 3200 | 3238 | const exec = getExecutor() |
| 3201 | 3239 | const entries = Object.entries(data) |
| 3202 | 3240 | const sets = entries.map(([k]) => `${sqlColumn(k)} = ?`).join(', ') |
| @@ -3208,8 +3246,11 @@ class ModelQueryBuilder< | ||
| 3208 | 3246 | |
| 3209 | 3247 | let sql = `UPDATE ${this._definition.table} SET ${sets}${timestampsEnabled(this._definition) ? ', updated_at = ?' : ''}` |
| 3210 | 3248 | |
| 3211 | if (this._wheres.length > 0) { | |
| 3212 | sql += ` WHERE ${this.buildWhereClauses(params)}` | |
| 3249 | // See delete(): composeWhere is the only path that adds the soft-delete | |
| 3250 | // predicate, so a scoped update has to go through it. #1111. | |
| 3251 | const whereBody = this.composeWhere(params) | |
| 3252 | if (whereBody) { | |
| 3253 | sql += ` WHERE ${whereBody}` | |
| 3213 | 3254 | } |
| 3214 | 3255 | |
| 3215 | 3256 | return (await exec.run(sql, params)).changes |