Resolve 8 critical SonarQube issues (BLOCKER + CRITICAL non-S3776) #2

2026-05-19T17:23:45-05:00

bermudalamb commented

2026-05-19 17:23:45 -05:00

Summary

Resolves the 8 BLOCKER/CRITICAL SonarQube issues in sql-utilities that aren't cognitive-complexity (S3776). Pragmatic mix: fix where the rule reflects a real defect, suppress with justification where it conflicts with deliberate design.

S3875 (BLOCKER) — Suppress on Expression.Operators.cs. The operator== overload returns a Comparison expression (DSL semantics), not a bool. File already has #pragma warning disable CS0660, CS0661 documenting the same intent.
S927 x2 — Rename IVisitor<T>.VisitNotEquals interface parameter from Equal → notEqual. The interface was the wrong one; implementations were already correct.
S4487 — Delete the unread _breakdown field from LinqQueryBreakdownBuilder.cs.
S2696 — Make _parameterIndex instance-scoped in PostgreSql.CommandVisitor. Latent bug fix: the static field was mutated from an instance method, causing every new visitor to inherit the previous instance's counter. Adds CommandVisitorTests.TwoVisitors_HaveIndependentParameterIndices to lock in the contract.
S2365 x3 — Two refactored (CalculationFilterGroup.Filters → plain auto-property + GetValidFilters() method in both Snowflake and SqlServer; callers in QueryConfigExtensions updated). One suppressed (HierarchicalData.AllChildData is an interface contract member tied to JSON shape; suppression with justification is documented in code and PR description).

Behavior changes

CalculationFilterGroup JSON output now serializes the raw filter collection rather than the pre-filtered one. Round-trip is preserved; callers needing the filtered view must call GetValidFilters().
PostgreSql.CommandVisitor parameter indices now restart at $1 per visitor instance instead of accumulating across instances. This is the intended PostgreSQL behavior.

Out of scope

11 S3776 cognitive-complexity hotspots (separate follow-up plan)
~487 NUnit modernization + ~150 style/perf issues in test files
Coverage reporting (0% in SonarQube — being worked separately via the .gitea/workflows/sonarqube.yml iteration)

Test plan

dotnet build Strata.SqlTools.QueryBreakdown.sln -c Release — clean
dotnet test Strata.SqlTools.QueryBreakdown.sln -c Release --no-build — 1,180 passed, 7 skipped, 0 failed (baseline + 1 new CommandVisitorTests.TwoVisitors_HaveIndependentParameterIndices)
Run ./scan-sonar.ps1 after merge to confirm the 8 issues clear in SonarQube (S3875 + S2365 on AllChildData will show as suppressed; the other 6 should resolve outright)

Spec & plan

Spec: docs/superpowers/specs/2026-05-19-sonar-tech-debt-design.md
Plan: docs/superpowers/plans/2026-05-19-sonar-tech-debt.md

🤖 Generated with Claude Code

## Summary Resolves the 8 BLOCKER/CRITICAL SonarQube issues in `sql-utilities` that aren't cognitive-complexity (S3776). Pragmatic mix: fix where the rule reflects a real defect, suppress with justification where it conflicts with deliberate design. - **S3875 (BLOCKER)** — Suppress on `Expression.Operators.cs`. The `operator==` overload returns a `Comparison` expression (DSL semantics), not a `bool`. File already has `#pragma warning disable CS0660, CS0661` documenting the same intent. - **S927 x2** — Rename `IVisitor<T>.VisitNotEquals` interface parameter from `Equal` → `notEqual`. The interface was the wrong one; implementations were already correct. - **S4487** — Delete the unread `_breakdown` field from `LinqQueryBreakdownBuilder.cs`. - **S2696** — Make `_parameterIndex` instance-scoped in `PostgreSql.CommandVisitor`. **Latent bug fix:** the static field was mutated from an instance method, causing every new visitor to inherit the previous instance's counter. Adds `CommandVisitorTests.TwoVisitors_HaveIndependentParameterIndices` to lock in the contract. - **S2365 x3** — Two refactored (`CalculationFilterGroup.Filters` → plain auto-property + `GetValidFilters()` method in both Snowflake and SqlServer; callers in `QueryConfigExtensions` updated). One suppressed (`HierarchicalData.AllChildData` is an interface contract member tied to JSON shape; suppression with justification is documented in code and PR description). ## Behavior changes - `CalculationFilterGroup` JSON output now serializes the **raw** filter collection rather than the pre-filtered one. Round-trip is preserved; callers needing the filtered view must call `GetValidFilters()`. - `PostgreSql.CommandVisitor` parameter indices now restart at `$1` per visitor instance instead of accumulating across instances. This is the intended PostgreSQL behavior. ## Out of scope - 11 S3776 cognitive-complexity hotspots (separate follow-up plan) - ~487 NUnit modernization + ~150 style/perf issues in test files - Coverage reporting (0% in SonarQube — being worked separately via the `.gitea/workflows/sonarqube.yml` iteration) ## Test plan - [x] `dotnet build Strata.SqlTools.QueryBreakdown.sln -c Release` — clean - [x] `dotnet test Strata.SqlTools.QueryBreakdown.sln -c Release --no-build` — **1,180 passed, 7 skipped, 0 failed** (baseline + 1 new `CommandVisitorTests.TwoVisitors_HaveIndependentParameterIndices`) - [ ] Run `./scan-sonar.ps1` after merge to confirm the 8 issues clear in SonarQube (S3875 + S2365 on `AllChildData` will show as suppressed; the other 6 should resolve outright) ## Spec & plan - Spec: `docs/superpowers/specs/2026-05-19-sonar-tech-debt-design.md` - Plan: `docs/superpowers/plans/2026-05-19-sonar-tech-debt.md` 🤖 Generated with [Claude Code](https://claude.com/claude-code)

bermudalamb added 10 commits 2026-05-19 17:23:46 -05:00

docs: spec for critical-tier SonarQube tech-debt sweep 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>

docs: implementation plan for critical-tier SonarQube tech-debt sweep 41e98258bf

Five-task plan implementing the 2026-05-19 spec: per-rule commits on the
existing fix/Sonarqube-Tech-Debt branch with build+test verification
between each. Notes one spec deviation discovered during plan-time
exploration (HierarchicalData.AllChildData is suppressed rather than
refactored — it's an interface contract member tied to JSON shape).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

fix(rules): rename IVisitor.VisitNotEquals parameter to match impls 986fdfc503

Resolves SonarQube S927 x2 in ExpressionVisitor.cs.

The interface parameter was named ``Equal`` on a ``VisitNotEquals`` method,
which is semantically wrong and forced implementations to mismatch.
Implementations already used ``notEqual``; rename the interface to match.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

fix(linq2sql): remove unread _breakdown field from builder af2c3e054f

Resolves SonarQube S4487 in LinqQueryBreakdownBuilder.cs.

The field was assigned in the constructor but never read. Grep across
the project confirmed no external references.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

fix(rules): suppress S3875 on intentional DSL operator== e2bfe3906a

Resolves SonarQube S3875 (BLOCKER) in Expression.Operators.cs.

The operator== returns a Comparison expression (DSL semantics), not a
bool. The existing CS0660/CS0661 pragma already documents this design;
the new SuppressMessage attribute makes the Sonar analyzer agree.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

fix(pgsql): make CommandVisitor parameter index instance-scoped acfe29ee98

Resolves SonarQube S2696 in CommandVisitor.cs.

The static _parameterIndex field was mutated from an instance method,
causing every new CommandVisitor to inherit the previous instance's
counter and never reset. Parameter indices now restart at $1 per
visitor, which is the intended PostgreSQL behavior.

Adds CommandVisitorTests.TwoVisitors_HaveIndependentParameterIndices to
lock in the contract via reflection (FormatParameterName is protected).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

test(pgsql): align CommandVisitorTests namespace and drop dead fallback f96161a664

Code review follow-up on acfe29e:
- Switch namespace to Strata.SqlTools.SqlBreakdown.Tests.PostgreSql to
  match the dominant convention in the sibling test folder.
- Remove the unreachable SqlServer.CommandVisitor fallback in the
  reflection helper. The Assert.That guard alone is enough to surface
  a future regression.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

refactor: address S2365 collection-copying property getters 6f85358f63

Resolves SonarQube S2365 x3.

CalculationFilterGroup (Snowflake + SqlServer): the `Filters` getter
ran .Where(...).ToList() on every access. Convert to a plain
auto-property holding the raw collection plus a `GetValidFilters()`
method that does the filtering. Update internal IsValid() and the two
QueryConfigExtensions callers to use the new method.

HierarchicalData.AllChildData: part of the IHierarchicalData interface
and JSON-serialized; the transformation IS the property's contract.
Suppress S2365 with justification rather than refactor -- same pattern
as S3875.

Behavior change: CalculationFilterGroup JSON output now serializes the
raw filter collection rather than the pre-filtered one. Round-trip is
preserved; callers needing the filtered view must call GetValidFilters().

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

perf+docs: fix IsValid allocation regression and document FilterGroup asymmetry e01cfae359

Code review follow-up on 6f85358:

- CalculationFilterGroup.IsValid() previously called GetValidFilters().Any(),
  which materialized a full filtered List<CalculationFilter> just to check
  existence. Reimplement directly as Filters.Any(x => x.IsValid()) so
  IsValid() recovers its pre-refactor O(1) early-exit behavior without
  allocations. GetValidFilters() remains for callers that need the full
  materialized list.

- QueryConfigExtensions.GetAllColumnIds line 16 uses FilterGroup (not
  CalculationFilterGroup), which pre-filters in its JsonConstructor and
  has no GetValidFilters() method. Add a comment to document the
  intentional asymmetry between line 14 and line 16.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

feat(sonar): Add SonarQube static analysis and code coverage setup

SonarQube Analysis / sonarqube (pull_request) Successful in 5m37s

Details

30f451d80c

Establishes tooling to systematically analyze and address technical debt.

This includes:
- `scan-sonar.ps1`: An orchestration script for local SonarQube scans with coverage.
- `Directory.Build.props`: Integrates SonarAnalyzer.CSharp for static analysis during build.
- `coverlet.runsettings`: Configures code coverage collection using Coverlet.
- `.claude/settings.local.json`: Adds permissions for AI to query SonarQube and local dev status.

bermudalamb merged commit 988c6d9cb9 into main

2026-05-19 17:26:28 -05:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Lambda-Associates/sql-utilities#2