From 6b461095c66cd9f8e2b89331f124a7e74c7e5011 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Tue, 19 May 2026 14:23:37 -0500 Subject: [PATCH] docs: spec for critical-tier SonarQube tech-debt sweep Design doc covering the 8 BLOCKER/CRITICAL non-S3776 issues across six projects. Approach is a pragmatic mix: fix where the rule reflects a real defect, suppress with justification where the rule conflicts with deliberate design (DSL operator==). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../2026-05-19-sonar-tech-debt-design.md | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 docs/superpowers/specs/2026-05-19-sonar-tech-debt-design.md diff --git a/docs/superpowers/specs/2026-05-19-sonar-tech-debt-design.md b/docs/superpowers/specs/2026-05-19-sonar-tech-debt-design.md new file mode 100644 index 0000000..414f5dd --- /dev/null +++ b/docs/superpowers/specs/2026-05-19-sonar-tech-debt-design.md @@ -0,0 +1,76 @@ +# Critical-tier SonarQube tech-debt sweep — 2026-05-19 + +## Context + +The `sql-utilities` SonarQube project (https://snrqbe.bermudalamb.synology.me, key `sql-utilities`) is in passing state overall (quality gate OK, 0 bugs, 0 vulnerabilities) but carries **809 open code smells**. The vast majority are mechanical noise — `external_roslyn:NUnit2045/2046` (487 combined), style/perf analyzer suggestions (~150) — concentrated in test files. + +This spec covers a **first, focused pass** at the high-impact tier: the **8 non-cognitive-complexity issues** at BLOCKER/CRITICAL severity, all in production code. They map cleanly to a handful of files across six projects. The 11 S3776 cognitive-complexity hotspots are deferred to a separate plan. + +Coverage reporting (currently 0% in SonarQube despite ~818 passing tests) is also out of scope here — it's being worked separately via the `.gitea/workflows/sonarqube.yml` iteration visible in `git log`. + +## Goal + +Resolve the 8 high-impact issues with changes that respect the code's intent — fix where the rule reflects a genuine defect, suppress with justification where the rule conflicts with deliberate design. + +## Scope (the 8 issues) + +| Rule | Severity | File | Treatment | +|---|---|---|---| +| S3875 | BLOCKER | `src/Strata.SqlTools.Rules/Rule/Expression/Expression.Operators.cs:28` | **Suppress** — the `operator==` returning a `Comparison` is intentional DSL syntax; the file already has `#pragma warning disable CS0660, CS0661` for the same reason. | +| S927 | CRITICAL | `src/Strata.SqlTools.Rules/ExpressionVisitor.cs:44, :80` | **Fix the interface, not the impls.** Rename the `IVisitor.VisitNotEquals` parameter from `Equal` (wrong) to `notEqual`. Implementations are already correct. | +| S2365 | CRITICAL | `src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/CalculationFilterGroup.cs:15` | **Fix.** Convert filtering getter to method `GetValidFilters()`; keep raw `Filters` as a plain auto-property for JSON round-trip. Update callers (currently `IsValid()`). | +| S2365 | CRITICAL | `src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/CalculationFilterGroup.cs:15` | **Fix.** Same pattern as above (likely a near-clone). | +| S2365 | CRITICAL | `src/Strata.SqlTools.SqlBreakdown/Expressions/InputPropertyExpression.cs:378` | **Fix.** Convert `AllChildData` getter (`_childDataMap.SelectMany(...).ToList()`) to method `GetAllChildData()`. Update callers. | +| S2696 | CRITICAL | `src/Strata.SqlTools.PostgreSql/Visitors/CommandVisitor.cs:12` | **Fix (latent bug).** Change `private static int _parameterIndex = 1;` to instance field. The static field is a cross-instance shared counter that never resets — almost certainly unintended. | +| S4487 | CRITICAL | `src/Strata.SqlTools.LinqToSql/Builders/LinqQueryBreakdownBuilder.cs:11` | **Fix.** Delete the unread `_breakdown` field and its constructor initialization. | + +## Approach + +Approach **B — pragmatic mix** (selected from three options during brainstorming): + +- **A — Strict "do what Sonar says"** was rejected because it would remove the DSL `==` operator (real regression) and rename correctly-named implementation parameters to match a wrong interface name. +- **C — Suppress all 8** was rejected because two of the issues (S2696 static field, S4487 unused field) are genuine defects and S2365 reflects a real per-access allocation cost. +- **B** treats each rule as advice: fix where it reflects a real defect, suppress with justification where it conflicts with deliberate design. + +## Commit plan + +Branch: `fix/Sonarqube-Tech-Debt` off `main` (already created — the spec commit is the first commit on it). + +Five commits, ordered easiest → trickiest so any blocking issue surfaces late and can be deferred without losing the earlier wins: + +1. `fix(rules): rename IVisitor.VisitNotEquals parameter to match impls` — S927 x2 +2. `fix(linq2sql): remove unread _breakdown field from builder` — S4487 +3. `fix(rules): suppress S3875 on intentional DSL operator==` — S3875 +4. `fix(pgsql): make CommandVisitor parameter index instance-scoped` — S2696 +5. `refactor: convert filtering collection-property getters to methods` — S2365 x3 + +## Verification + +Per commit: + +- `dotnet build Strata.SqlTools.QueryBreakdown.sln -c Release` — clean build +- `dotnet test Strata.SqlTools.QueryBreakdown.sln -c Release --no-build` — all tests must remain green; skip count must not increase beyond the current baseline (PostgreSql 1 skipped, Snowflake 1 skipped, Markdown 4 skipped) +- If a commit's verification fails: fix inside the same commit, or revert before moving on + +Final: fast-forward merge to `main` (matches existing direct-to-main pattern in `git log`). Run `./scan-sonar.ps1` to push a fresh analysis; confirm the 8 issues clear in SonarQube (S3875 will show as suppressed; the other 7 should resolve outright). + +## Risks & mitigations + +| Risk | Likelihood | Impact | Mitigation | +|---|---|---|---| +| S2365 property→method breaks consumers of `Filters` / `AllChildData` | Medium | Medium — compile errors in callers | Grep call sites before each commit; update in the same commit. For JSON round-trip, keep `Filters` as auto-property holding raw collection, move filtering logic into `GetValidFilters()`. | +| S2696 static→instance changes numbering | Low | Medium if any downstream consumer relies on cross-instance counter | Tests don't depend on it. Document in commit message: parameter indices now reset per visitor instance — this is the intended Postgres behavior. | +| S927 interface rename breaks named-arg callers | Low | Low — C# parameter names are not in the binary contract | Grep confirms no `VisitNotEquals(Equal: …)` callers in repo. External NuGet consumers using named args would need a one-line update. | +| S3875 suppression hides a future real bug in `Expression` equality | Low | Low | Justification string makes intent explicit. The existing `#pragma warning disable CS0660, CS0661` already documents the deliberate omission of `Equals`/`GetHashCode`. | +| Tests stay 0% covered in SonarQube | N/A | N/A | Pre-existing, out of scope. Tracked separately via `.gitea/workflows/sonarqube.yml` iteration. | + +**Rollback:** each commit is self-contained → `git revert ` restores the prior state for any single fix. Worst case, abandon the branch. + +## Out of scope + +- The 11 S3776 cognitive-complexity hotspots (separate follow-up plan) +- The ~487 NUnit modernization issues in test files +- The ~150 style/perf analyzer issues in test files +- Coverage reporting (0% in SonarQube) +- Duplication (11.1%) +- 73 security hotspots