From 2edb077cee4420cae604a8890ed72d82d834bb84 Mon Sep 17 00:00:00 2001 From: Duke - Duc Dinh Date: Fri, 18 Sep 2026 21:05:32 +0700 Subject: [PATCH 1/3] feat(rules): add DOL041 planner-override rule New editor rule flagging SET/SET LOCAL/SET SESSION of planner-toggling Postgres GUCs (enable_*, plan_cache_mode, jit*) in application code. Line-oriented, so parameterized values (%s) are detected the same as literals. Non-planner session settings stay unflagged. Refs #110 --- CHANGELOG.md | 12 ++++ README.md | 7 ++- docs/rules/DOL041.md | 62 +++++++++++++++++++ docs/rules/README.md | 3 +- src/rules/index.ts | 4 +- src/rules/rawsql.ts | 84 ++++++++++++++++++++++++++ test/rules/rawsql.test.js | 122 ++++++++++++++++++++++++++++++++++++++ 7 files changed, 289 insertions(+), 5 deletions(-) create mode 100644 docs/rules/DOL041.md create mode 100644 src/rules/rawsql.ts create mode 100644 test/rules/rawsql.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index cdc97bd..7c6c8e3 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*`) at the + session level takes the plan choice away from the Postgres planner for the + whole connection, 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..11f837f 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 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 GUCs forced off in raw SQL, 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..257735c --- /dev/null +++ b/docs/rules/DOL041.md @@ -0,0 +1,62 @@ +# 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: `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. + +## 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..1c81996 --- /dev/null +++ b/src/rules/rawsql.ts @@ -0,0 +1,84 @@ +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-z0-9_%.]+)/gi; + +function isCommentLine(text: string): boolean { + return text.trimStart().startsWith('#'); +} + +/** + * 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: { + default: + '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; scope with SET LOCAL only when intentional.', + }, + }, + check(ctx: RuleContext): Finding[] { + const out: Finding[] = []; + for (let i = 0; i < ctx.lineCount; i++) { + const text = ctx.lineAt(i); + if (isCommentLine(text)) continue; + RE_PLANNER_OVERRIDE.lastIndex = 0; + let m: RegExpExecArray | null; + while ((m = RE_PLANNER_OVERRIDE.exec(text)) !== null) { + out.push({ + code: 'DOL041', + messageId: 'default', + 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..7f47caf --- /dev/null +++ b/test/rules/rawsql.test.js @@ -0,0 +1,122 @@ +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)); + }, + }; +} + +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'); + assert.equal( + rule.check(makeCtx('SET LOCAL enable_seqscan = off')).length, + 1, + ); + assert.equal( + rule.check(makeCtx('cur.execute("SET SESSION enable_bitmapscan = off")')) + .length, + 1, + ); +}); + +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); +}); + +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); +}); From b5f93d095f2d6b9637e434ad8c5a459cd3f6d734 Mon Sep 17 00:00:00 2001 From: Duke - Duc Dinh Date: Fri, 18 Sep 2026 23:12:17 +0700 Subject: [PATCH 2/3] fix(rules): address DOL041 review feedback - README: rename section to 'Inline diagnostics & QuickFixes' and mark DOL041 as diagnostic-only, covering plan_cache_mode/jit* overrides - rawsql: capture named DB-API placeholders (%(name)s) whole - rawsql: skip SQL -- comment lines inside multiline query strings - rawsql: split diagnostic message so SET LOCAL is described as transaction-scoped instead of claiming connection persistence - tests: regression coverage for all of the above; docstrings for touched helpers --- CHANGELOG.md | 20 ++++++++++---------- README.md | 4 ++-- docs/rules/DOL041.md | 2 +- src/rules/rawsql.ts | 19 ++++++++++++++----- test/rules/rawsql.test.js | 24 ++++++++++++++++-------- 5 files changed, 43 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c6c8e3..590ccaa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,16 +10,16 @@ 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*`) at the - session level takes the plan choice away from the Postgres planner for the - whole connection, 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. + 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 diff --git a/README.md b/README.md index 11f837f..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 (18 rules) +### ๐ŸŽฏ Inline diagnostics & QuickFixes (18 rules) -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 GUCs forced off in raw SQL, 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`. diff --git a/docs/rules/DOL041.md b/docs/rules/DOL041.md index 257735c..35b45fd 100644 --- a/docs/rules/DOL041.md +++ b/docs/rules/DOL041.md @@ -2,7 +2,7 @@ **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: `cursor.execute('SET enable_seqscan = %s', ['off'])` is flagged exactly like a literal. +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 diff --git a/src/rules/rawsql.ts b/src/rules/rawsql.ts index 1c81996..5da8495 100644 --- a/src/rules/rawsql.ts +++ b/src/rules/rawsql.ts @@ -26,10 +26,16 @@ const DOCS_BASE = * 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-z0-9_%.]+)/gi; + /\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; +/** + * True for a line that is entirely a comment โ€” Python `#` or a SQL `--` + * line inside a multiline query string. Both shapes put planner-looking SQL + * in front of the regex without meaning to execute it. + */ function isCommentLine(text: string): boolean { - return text.trimStart().startsWith('#'); + const trimmed = text.trimStart(); + return trimmed.startsWith('#') || trimmed.startsWith('--'); } /** @@ -56,10 +62,13 @@ const DOL041: Rule = { docsUrl: `${DOCS_BASE}/DOL041.md`, since: '0.19.0', messages: { - default: - '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; scope with SET LOCAL only when intentional.', + 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[] = []; for (let i = 0; i < ctx.lineCount; i++) { @@ -70,7 +79,7 @@ const DOL041: Rule = { while ((m = RE_PLANNER_OVERRIDE.exec(text)) !== null) { out.push({ code: 'DOL041', - messageId: 'default', + 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', diff --git a/test/rules/rawsql.test.js b/test/rules/rawsql.test.js index 7f47caf..fd9e38e 100644 --- a/test/rules/rawsql.test.js +++ b/test/rules/rawsql.test.js @@ -35,6 +35,7 @@ function makeCtx(source) { }; } +/** 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`); @@ -58,15 +59,21 @@ test('DOL041 flags both GUCs in a parameterized multi-statement SET', () => { test('DOL041 flags SET LOCAL and SESSION variants', () => { const rule = ruleByCode('DOL041'); - assert.equal( - rule.check(makeCtx('SET LOCAL enable_seqscan = off')).length, - 1, - ); - assert.equal( - rule.check(makeCtx('cur.execute("SET SESSION enable_bitmapscan = off")')) - .length, - 1, + 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', () => { @@ -109,6 +116,7 @@ test('DOL041 ignores non-planner session settings', () => { 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 stays quiet on ordinary SQL and ORM code', () => { From 2aef69b41280482070cb9f7132241d958ca41df2 Mon Sep 17 00:00:00 2001 From: Duke - Duc Dinh Date: Fri, 18 Sep 2026 23:22:59 +0700 Subject: [PATCH 3/3] fix(rules): ignore comment text before DOL041 matching Mask Python trailing # comments, inline SQL -- comments and SQL /* */ block comments (state carried across lines) before running the planner-GUC regex. Masking preserves line length so finding ranges still point at the original columns. Commented-out SQL no longer produces findings, while SET text before a trailing comment still does. --- docs/rules/DOL041.md | 2 ++ src/rules/rawsql.ts | 60 +++++++++++++++++++++++++++++++++------ test/rules/rawsql.test.js | 35 +++++++++++++++++++++++ 3 files changed, 89 insertions(+), 8 deletions(-) diff --git a/docs/rules/DOL041.md b/docs/rules/DOL041.md index 35b45fd..cc801b3 100644 --- a/docs/rules/DOL041.md +++ b/docs/rules/DOL041.md @@ -29,6 +29,8 @@ Each GUC in a multi-statement string is reported separately, and both the `=` an `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 diff --git a/src/rules/rawsql.ts b/src/rules/rawsql.ts index 5da8495..d746128 100644 --- a/src/rules/rawsql.ts +++ b/src/rules/rawsql.ts @@ -29,13 +29,57 @@ 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; /** - * True for a line that is entirely a comment โ€” Python `#` or a SQL `--` - * line inside a multiline query string. Both shapes put planner-looking SQL - * in front of the regex without meaning to execute it. + * 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 isCommentLine(text: string): boolean { - const trimmed = text.trimStart(); - return trimmed.startsWith('#') || trimmed.startsWith('--'); +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; } /** @@ -71,9 +115,9 @@ const DOL041: Rule = { /** 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 = ctx.lineAt(i); - if (isCommentLine(text)) continue; + const text = maskComments(ctx.lineAt(i), blockState); RE_PLANNER_OVERRIDE.lastIndex = 0; let m: RegExpExecArray | null; while ((m = RE_PLANNER_OVERRIDE.exec(text)) !== null) { diff --git a/test/rules/rawsql.test.js b/test/rules/rawsql.test.js index fd9e38e..02bafc0 100644 --- a/test/rules/rawsql.test.js +++ b/test/rules/rawsql.test.js @@ -119,6 +119,41 @@ test('DOL041 ignores comment lines', () => { 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(