Thom LambandClaude Opus 4.7
6b461095c6
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) <noreply@anthropic.com>
77 lines
6.4 KiB
Markdown
77 lines
6.4 KiB
Markdown
# 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<T>.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 <sha>` 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
|