diff --git a/CHANGELOG.md b/CHANGELOG.md index cdc97bd..590ccaa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **New rule `DOL041` — planner settings overridden in raw SQL.** Application + code that toggles planner GUCs (`enable_*`, `plan_cache_mode`, `jit*`) takes + the plan choice away from the Postgres planner — connection-wide for `SET` / + `SET SESSION`, transaction-scoped for `SET LOCAL` — so a query that passes + review and tests can collapse into a forced nested-loop join as its row set + grows. The line-oriented rule sees the GUC name even when the value is a + bind parameter (`SET enable_seqscan = %s`), flags `SET` / `SET LOCAL` / + `SET SESSION` alike, and stays quiet on non-planner session settings + (`search_path`, `statement_timeout`, `work_mem`, …). Default severity + `warning`, applicability `unsafe` — the safe repair is a query or index + change, so no QuickFix. Refs #110. + - **Simplified Chinese rule index.** The existing `DOL021` and `DOL022` translations now have a dedicated partial-locale index linked from the Chinese README. Refs #81. diff --git a/README.md b/README.md index 11239d5..a8fa2a0 100644 --- a/README.md +++ b/README.md @@ -273,9 +273,9 @@ Django's own check needs a working settings module, an importable app registry a -### 🎯 Inline QuickFixes (17 rules) +### 🎯 Inline diagnostics & QuickFixes (18 rules) -Static analysis over `.py` files with Ruff-style codes (`DOL001`..`DOL032`), Clippy-style `Applicability`, and per-rule severity overrides. `.count() > 0` → `.exists()`, `null=True` on `CharField`, missing `on_delete`, `datetime.now()` → `timezone.now()` and a dozen more. +Static analysis over `.py` files with Ruff-style codes (`DOL001`..`DOL041`), Clippy-style `Applicability`, and per-rule severity overrides. `.count() > 0` → `.exists()`, `null=True` on `CharField`, missing `on_delete`, `datetime.now()` → `timezone.now()`, planner-GUC overrides in raw SQL (`enable_*`, `plan_cache_mode`, `jit*`; diagnostic-only), and a dozen more. Suppress inline with `# django-orm-lens-disable-next-line DOL007`. @@ -636,7 +636,7 @@ The defaults are opinionated and sensible. If you need to tweak: ## 🔬 Rule catalogue -Sixteen editor-side checks (`DOL001`–`DOL032`) with Ruff-style codes, per-rule severity, and Clippy-style applicability — plus fifteen CLI-side migration-risk rules and the static N+1 analyzer. **Every rule now has its own documentation page.** +Eighteen editor-side checks (`DOL001`–`DOL041`) with Ruff-style codes, per-rule severity, and Clippy-style applicability — plus fifteen CLI-side migration-risk rules and the static N+1 analyzer. **Every rule now has its own documentation page.** | Category | Rules | Examples | |---|---|---| @@ -644,6 +644,7 @@ Sixteen editor-side checks (`DOL001`–`DOL032`) with Ruff-style codes, per-rule | [Model definition](https://github.com/FROWNINGdev/django-orm-lens/blob/main/docs/rules/README.md) | `DOL011`–`DOL015` | `ForeignKey` without `on_delete`, `null=True` on string fields | | [Datetime](https://github.com/FROWNINGdev/django-orm-lens/blob/main/docs/rules/README.md) | `DOL021`–`DOL022` | `datetime.now()` → `timezone.now()` | | [Forms / views](https://github.com/FROWNINGdev/django-orm-lens/blob/main/docs/rules/README.md) | `DOL031`–`DOL032` | `locals()` in `render()`, `Meta.fields = '__all__'` | +| [Raw SQL](https://github.com/FROWNINGdev/django-orm-lens/blob/main/docs/rules/DOL041.md) | `DOL041` | `SET enable_hashjoin = off`, `plan_cache_mode`, `jit` overrides in app code | | [Migration risks](https://github.com/FROWNINGdev/django-orm-lens/blob/main/docs/rules/migrations.md) | 16 rules | NOT NULL add without default, table-locking index builds, irreversible data migrations | | [Static N+1](https://github.com/FROWNINGdev/django-orm-lens/blob/main/docs/rules/nplusone.md) | 1 analyzer | FK/M2M access in loops without `select_related` / `prefetch_related` | diff --git a/docs/rules/DOL041.md b/docs/rules/DOL041.md new file mode 100644 index 0000000..cc801b3 --- /dev/null +++ b/docs/rules/DOL041.md @@ -0,0 +1,64 @@ +# DOL041 — Planner setting overridden in raw SQL + +**Default severity:** warning · **Applicability:** unsafe · **Category:** raw SQL + +Detects `SET` / `SET LOCAL` / `SET SESSION` of a planner-toggling Postgres GUC inside application code — `enable_*` (hashjoin, mergejoin, nestloop, seqscan, indexscan, …), `plan_cache_mode`, and `jit*`. The check is pure text shape, so it sees the GUC name even when the value is a bind parameter (`%s` or `%(name)s`): `cursor.execute('SET enable_seqscan = %s', ['off'])` is flagged exactly like a literal. + +## Why this is a warning and not a style nit + +`SET enable_hashjoin = off` does not make a query faster. It removes the planner's ability to choose otherwise — and the planner is choosing per query, with row estimates the call site cannot see. The failure mode is asymmetric: + +- On a small dataset the forced plan looks fine. Review and tests pass. +- As the filtered row set grows, a forced nested-loop join can be orders of magnitude slower than the join the planner would have picked, and the query that "always worked" becomes a production incident. + +A plain `SET` is also **connection-scoped**. With connection pooling, the override leaks into whatever runs next on that same connection. `SET LOCAL` scopes it to the transaction and is the only form with an excuse — but even that is a judgement call a reviewer should see, so it is flagged too. + +## What is flagged + +```python +cursor.execute('SET enable_seqscan = %s; SET enable_bitmapscan = %s', ['off', 'off']) +cursor.execute("SET LOCAL enable_hashjoin = off") +connection.cursor().execute("SET SESSION enable_bitmapscan TO 'off'") +cursor.execute("SET plan_cache_mode = force_generic_plan") +cursor.execute("SET jit = off") +``` + +Each GUC in a multi-statement string is reported separately, and both the `=` and `TO` forms match. + +## What is not flagged + +`SET search_path`, `SET timezone`, `SET statement_timeout`, `SET work_mem` and the rest of the session-settings surface — those have legitimate per-request uses, and flagging them would drown the rule. Only the family that exists to overrule the planner is in scope. + +Comment text is ignored as well: Python `#` comments, SQL `--` comments and SQL `/* ... */` blocks (including ones spanning several lines) do not produce findings, so commented-out SQL stays quiet. + +## Bad + +```python +def open_tickets(connection): + # every later query on this pooled connection inherits the override + connection.cursor().execute("SET enable_hashjoin = off") + return Ticket.objects.raw(TICKET_REPORT_SQL) +``` + +## Good + +```python +def open_tickets(): + return Ticket.objects.raw(TICKET_REPORT_SQL) +``` + +Fix the query or the index instead — or, if an override is truly required, scope it with `SET LOCAL` inside a transaction and keep it auditable: + +```python +with transaction.atomic(), connection.cursor() as cur: + cur.execute("SET LOCAL enable_seqscan = off") + ... +``` + +## Suppress + +```python +# django-orm-lens-disable-next-line DOL041 +``` + +Or per-workspace in `.vscode/settings.json`: `{"djangoOrmLens.rules": {"DOL041": "off"}}`. diff --git a/docs/rules/README.md b/docs/rules/README.md index a7307ab..cb2cb42 100644 --- a/docs/rules/README.md +++ b/docs/rules/README.md @@ -5,7 +5,7 @@ Django ORM Lens ships two rule surfaces: -1. **Editor rules (`DOL###`)** — 16 line-oriented static checks that run inside the VS Code extension on every `.py` file. Findings appear in the Problems panel under the source `Django ORM Lens`, link to these pages from the diagnostic code, and — where a fix is safe to express as a text edit — carry a QuickFix lightbulb. Detection is regex-based with bounded windows; no Python process is involved. +1. **Editor rules (`DOL###`)** — 18 line-oriented static checks that run inside the VS Code extension on every `.py` file. Findings appear in the Problems panel under the source `Django ORM Lens`, link to these pages from the diagnostic code, and — where a fix is safe to express as a text edit — carry a QuickFix lightbulb. Detection is regex-based with bounded windows; no Python process is involved. 2. **CLI / CI analyzers** — AST-based checks in the Python package (`pip install django-orm-lens`) for terminals and pipelines: [`migration-risk`](migrations.md), [`nplusone`](nplusone.md), [`blast-radius`](blast-radius.md) — which joins migration risks with the code that still references what they change — and [`drift`](drift.md), a `makemigrations --check` that needs no Django boot. ## Severity and applicability @@ -41,6 +41,7 @@ Applicability follows Clippy's semantics. It is a property of each individual fi | [DOL022](DOL022.md) | `datetime.utcnow()` is deprecated | datetime | warning | suggestion | | [DOL031](DOL031.md) | `render()` with `locals()` as context | forms | warning | suggestion | | [DOL032](DOL032.md) | `fields = '__all__'` in Meta | forms | warning | unsafe | +| [DOL041](DOL041.md) | Planner setting overridden in raw SQL | raw SQL | warning | unsafe | ### Suppressing findings inline diff --git a/src/rules/index.ts b/src/rules/index.ts index 917ab7a..ecb9296 100644 --- a/src/rules/index.ts +++ b/src/rules/index.ts @@ -3,6 +3,7 @@ import { querysetRules } from './queryset'; import { modelRules } from './models'; import { datetimeRules } from './datetime'; import { formsRules } from './forms'; +import { rawSqlRules } from './rawsql'; import { ALL_FIXERS, findFixersForCode } from './fixers'; import { WorkspaceIndex } from '../types'; import { @@ -39,12 +40,13 @@ import { * for the rest of the file. */ -/** Canonical rule catalogue in stable order (queryset, model, datetime, forms). */ +/** Canonical rule catalogue in stable order (queryset, model, datetime, forms, raw SQL). */ export const ALL_RULES: Rule[] = [ ...querysetRules, ...modelRules, ...datetimeRules, ...formsRules, + ...rawSqlRules, ]; /** Re-exports so callers only need `from './rules'`. */ diff --git a/src/rules/rawsql.ts b/src/rules/rawsql.ts new file mode 100644 index 0000000..d746128 --- /dev/null +++ b/src/rules/rawsql.ts @@ -0,0 +1,137 @@ +import { Finding, Rule, RuleContext } from './types'; + +/** + * Raw SQL / database-session rules. + * + * Codes DOL041..DOL050 are reserved for this group. Codes are stable public + * surface; do not renumber. When a rule is removed, its code stays retired. + * + * Like the queryset rules these are line-oriented (regex only), so the pass + * stays O(lineCount) and works without a Python parser. The signal lives in + * the SQL text itself — a GUC name survives parameterization, so + * `cursor.execute('SET enable_seqscan = %s', ['off'])` is as visible as a + * literal `off`. + */ + +const DOCS_BASE = + 'https://github.com/FROWNINGdev/django-orm-lens/blob/main/docs/rules'; + +/** + * Planner-toggling GUCs only. + * + * Deliberately not the whole `SET` surface. `search_path`, `timezone`, + * `statement_timeout` and `work_mem` have legitimate per-request uses and + * flagging them would drown the rule. The family below exists for one + * purpose: telling the planner which plan shapes it may consider, which is + * exactly the decision the planner exists to make. + */ +const RE_PLANNER_OVERRIDE = + /\bSET\s+(?:(LOCAL|SESSION)\s+)?((?:enable_[a-z_]+)|plan_cache_mode|jit(?:_[a-z_]+)?)\s*(?:=|TO)\s*('[^']*'|%\([A-Za-z_][A-Za-z0-9_]*\)s|[A-Za-z0-9_%.]+)/gi; + +/** + * Blank out comment text on a line, preserving length so finding ranges still + * point at the original columns. Handles Python `#` comments (outside string + * literals), SQL `--` comments, and SQL block comments, whose state carries + * across lines through `state.inBlockComment`. + */ +function maskComments( + text: string, + state: { inBlockComment: boolean }, +): string { + let out = ''; + let quote: string | null = null; + for (let i = 0; i < text.length; i++) { + const ch = text[i]; + if (state.inBlockComment) { + if (ch === '*' && text[i + 1] === '/') { + state.inBlockComment = false; + out += ' '; + i++; + } else { + out += ' '; + } + continue; + } + if (ch === '#' && quote === null) { + return out + ' '.repeat(text.length - i); + } + if (ch === '-' && text[i + 1] === '-') { + return out + ' '.repeat(text.length - i); + } + if (ch === '/' && text[i + 1] === '*') { + state.inBlockComment = true; + out += ' '; + i++; + continue; + } + if (quote !== null) { + if (ch === '\\') { + out += ch + (text[i + 1] ?? ''); + i++; + continue; + } + if (ch === quote) quote = null; + out += ch; + continue; + } + if (ch === '"' || ch === "'") { + quote = ch; + } + out += ch; + } + return out; +} + +/** + * DOL041 — raw SQL in application code overriding the query planner. + * + * `SET enable_hashjoin = off` (and friends) does not make a query faster; + * it removes the planner's ability to choose otherwise. On a small dataset + * the forced plan may look fine and then collapse as the filtered row set + * grows — the classic case being a forced nested-loop join over a multi-level + * `IN` chain. Worse, a plain `SET` is connection-scoped: with connection + * pooling it leaks into whatever runs next on that connection. `SET LOCAL` + * scopes the override to the transaction, which is the only form that has an + * excuse, and even that is a judgement call the reviewer should see. + * + * No QuickFix on purpose: the safe repair is a query/index change, not a + * mechanical text edit, so the finding is `unsafe`. + */ +const DOL041: Rule = { + meta: { + code: 'DOL041', + title: 'Planner setting overridden in raw SQL', + category: 'performance', + defaultSeverity: 'warning', + docsUrl: `${DOCS_BASE}/DOL041.md`, + since: '0.19.0', + messages: { + connection: + 'Planner setting {guc} = {value} is overridden here — this forces the query planner for every query on the connection and can turn a fast plan into a nested-loop scan as data grows. Fix the query or index instead.', + local: + 'Planner setting {guc} = {value} is overridden here with SET LOCAL — it only affects the current transaction, but it still takes the plan choice away from the planner. Make sure it is intentional and auditable.', + }, + }, + /** Report every planner-GUC override on the line, connection- or transaction-scoped. */ + check(ctx: RuleContext): Finding[] { + const out: Finding[] = []; + const blockState = { inBlockComment: false }; + for (let i = 0; i < ctx.lineCount; i++) { + const text = maskComments(ctx.lineAt(i), blockState); + RE_PLANNER_OVERRIDE.lastIndex = 0; + let m: RegExpExecArray | null; + while ((m = RE_PLANNER_OVERRIDE.exec(text)) !== null) { + out.push({ + code: 'DOL041', + messageId: m[1]?.toUpperCase() === 'LOCAL' ? 'local' : 'connection', + args: { guc: m[2], value: m[3] }, + range: { line: i, startCol: m.index, endCol: m.index + m[0].length }, + applicability: 'unsafe', + }); + } + } + return out; + }, +}; + +export const rawSqlRules: Rule[] = [DOL041]; diff --git a/test/rules/rawsql.test.js b/test/rules/rawsql.test.js new file mode 100644 index 0000000..02bafc0 --- /dev/null +++ b/test/rules/rawsql.test.js @@ -0,0 +1,165 @@ +const assert = require('node:assert/strict'); +const Module = require('node:module'); +const test = require('node:test'); + +// Shim the `vscode` module — rules only touch it via type-only imports at +// build time; at runtime the plain `{}` shim is sufficient because none of +// the rule-check code paths call into a vscode.* runtime member. +const originalLoad = Module._load; +Module._load = function (request, parent, isMain) { + if (request === 'vscode') return {}; + return originalLoad.call(this, request, parent, isMain); +}; + +const { rawSqlRules } = require('../../out/rules/rawsql'); + +test.after(() => { + Module._load = originalLoad; +}); + +/** Build a fake `RuleContext` from a source string. */ +function makeCtx(source) { + const lines = source.split(/\r?\n/); + return { + document: null, + lineCount: lines.length, + lineAt(i) { + return lines[i] ?? ''; + }, + windowBefore(i, n) { + return lines.slice(Math.max(0, i - n), i); + }, + windowAfter(i, n) { + return lines.slice(i + 1, Math.min(lines.length, i + 1 + n)); + }, + }; +} + +/** Look up a rule by code, failing loudly when it is not registered. */ +function ruleByCode(code) { + const r = rawSqlRules.find((r) => r.meta.code === code); + assert.ok(r, `rule ${code} must exist`); + return r; +} + +test('DOL041 flags both GUCs in a parameterized multi-statement SET', () => { + const rule = ruleByCode('DOL041'); + const findings = rule.check( + makeCtx( + "cursor.execute('SET enable_seqscan = %s; SET enable_bitmapscan = %s', ['off', 'off'])", + ), + ); + assert.equal(findings.length, 2); + assert.equal(findings[0].code, 'DOL041'); + assert.equal(findings[0].applicability, 'unsafe'); + assert.equal(findings[0].args.guc, 'enable_seqscan'); + assert.equal(findings[0].args.value, '%s'); + assert.equal(findings[1].args.guc, 'enable_bitmapscan'); +}); + +test('DOL041 flags SET LOCAL and SESSION variants', () => { + const rule = ruleByCode('DOL041'); + const local = rule.check(makeCtx('SET LOCAL enable_seqscan = off')); + assert.equal(local.length, 1); + assert.equal(local[0].messageId, 'local'); + const session = rule.check( + makeCtx('cur.execute("SET SESSION enable_bitmapscan = off")'), + ); + assert.equal(session.length, 1); + assert.equal(session[0].messageId, 'connection'); +}); + +test('DOL041 captures named DB-API placeholders completely', () => { + const rule = ruleByCode('DOL041'); + const findings = rule.check(makeCtx('SET enable_seqscan = %(planner)s')); + assert.equal(findings.length, 1); + assert.equal(findings[0].args.value, '%(planner)s'); +}); + +test('DOL041 accepts the TO form and quoted values', () => { + const rule = ruleByCode('DOL041'); + const findings = rule.check( + makeCtx("SET enable_nestloop TO 'off'"), + ); + assert.equal(findings.length, 1); + assert.equal(findings[0].args.guc, 'enable_nestloop'); +}); + +test('DOL041 flags plan_cache_mode and jit overrides', () => { + const rule = ruleByCode('DOL041'); + const findings = rule.check( + makeCtx('SET plan_cache_mode = force_generic_plan\nSET jit = off'), + ); + assert.equal(findings.length, 2); + assert.equal(findings[0].args.guc, 'plan_cache_mode'); + assert.equal(findings[1].args.guc, 'jit'); +}); + +test('DOL041 matches without spaces around the equals sign', () => { + const rule = ruleByCode('DOL041'); + const findings = rule.check(makeCtx('SET enable_hashjoin=off')); + assert.equal(findings.length, 1); + assert.equal(findings[0].args.value, 'off'); +}); + +test('DOL041 ignores non-planner session settings', () => { + const rule = ruleByCode('DOL041'); + assert.equal(rule.check(makeCtx("SET statement_timeout = '5s'")).length, 0); + assert.equal(rule.check(makeCtx("SET search_path = 'public'")).length, 0); + assert.equal(rule.check(makeCtx('SET work_mem = 65536')).length, 0); + assert.equal( + rule.check(makeCtx('SET TRANSACTION ISOLATION LEVEL SERIALIZABLE')).length, + 0, + ); +}); + +test('DOL041 ignores comment lines', () => { + const rule = ruleByCode('DOL041'); + assert.equal(rule.check(makeCtx('# SET enable_seqscan = off legacy')).length, 0); + assert.equal(rule.check(makeCtx('-- SET enable_seqscan = off')).length, 0); +}); + +test('DOL041 ignores trailing Python and inline SQL comments', () => { + const rule = ruleByCode('DOL041'); + assert.equal( + rule.check(makeCtx('cursor.execute("SELECT 1") # SET enable_seqscan = off')) + .length, + 0, + ); + assert.equal( + rule.check(makeCtx('cursor.execute("SELECT 1 -- SET enable_seqscan = off")')) + .length, + 0, + ); + assert.equal( + rule.check(makeCtx('cursor.execute("/* SET enable_seqscan = off */ SELECT 1")')) + .length, + 0, + ); +}); + +test('DOL041 tracks SQL block comments across lines', () => { + const rule = ruleByCode('DOL041'); + const source = + 'cursor.execute("""/* force index")\nSET enable_seqscan = off\n*/ SELECT 1""")'; + assert.equal(rule.check(makeCtx(source)).length, 0); +}); + +test('DOL041 still flags SET text before a trailing SQL comment', () => { + const rule = ruleByCode('DOL041'); + const findings = rule.check( + makeCtx('cursor.execute("SET enable_seqscan = off -- force index")'), + ); + assert.equal(findings.length, 1); + assert.equal(findings[0].args.value, 'off'); +}); + +test('DOL041 stays quiet on ordinary SQL and ORM code', () => { + const rule = ruleByCode('DOL041'); + const findings = rule.check( + makeCtx( + 'qs = Order.objects.filter(customer__in=ids)\nrows = list(qs[:100])', + ), + ); + assert.equal(findings.length, 0); +});