From 41e98258bf4302f954adf5bcea322feaf529a91a Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Tue, 19 May 2026 14:58:24 -0500 Subject: [PATCH] docs: implementation plan for critical-tier SonarQube tech-debt sweep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../plans/2026-05-19-sonar-tech-debt.md | 618 ++++++++++++++++++ 1 file changed, 618 insertions(+) create mode 100644 docs/superpowers/plans/2026-05-19-sonar-tech-debt.md diff --git a/docs/superpowers/plans/2026-05-19-sonar-tech-debt.md b/docs/superpowers/plans/2026-05-19-sonar-tech-debt.md new file mode 100644 index 0000000..8c50494 --- /dev/null +++ b/docs/superpowers/plans/2026-05-19-sonar-tech-debt.md @@ -0,0 +1,618 @@ +# Critical-tier SonarQube tech-debt sweep — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Resolve the 8 BLOCKER/CRITICAL non-S3776 SonarQube issues in `sql-utilities` using a pragmatic mix of fixes and justified suppressions. Implements the spec at `docs/superpowers/specs/2026-05-19-sonar-tech-debt-design.md`. + +**Architecture:** Each rule is addressed in isolation on the existing branch `fix/Sonarqube-Tech-Debt`. Five distinct commits, easiest → trickiest, so an early problem doesn't block the entire sweep. Existing tests (~818 passing) are the safety net for behavioral changes. + +**Tech Stack:** C# / .NET 8 / NUnit / SonarAnalyzer.CSharp 10.4.0 (referenced via `Directory.Build.props`). + +**Spec deviation flagged here:** During plan-time exploration I found that `HierarchicalData.AllChildData` (S2365 case #3) is part of the `IHierarchicalData` interface contract, is serialized via JSON (`tests/Strata.SqlTools.Rules.Tests/Data.json`), and its transformation is the property's deliberate intent. Refactoring to a method would break the interface and the serialization shape. Treatment for this one case is changed from **fix** to **suppress with justification** — same pattern as S3875. The other two S2365 cases (Snowflake/SqlServer `CalculationFilterGroup.Filters`) still get the refactor as specified. + +**Working directory assumption:** All paths are relative to `C:\gitea\sql-utilities`. Branch `fix/Sonarqube-Tech-Debt` is already checked out; `HEAD` is the spec commit `6b46109`. + +--- + +## File map + +**Modified:** +- `src/Strata.SqlTools.Rules/ExpressionVisitor.cs` — interface parameter rename (Task 1) +- `src/Strata.SqlTools.LinqToSql/Builders/LinqQueryBreakdownBuilder.cs` — delete unused field (Task 2) +- `src/Strata.SqlTools.Rules/Rule/Expression/Expression.Operators.cs` — `[SuppressMessage]` attribute (Task 3) +- `src/Strata.SqlTools.PostgreSql/Visitors/CommandVisitor.cs` — static → instance field (Task 4) +- `src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/CalculationFilterGroup.cs` — property → method (Task 5) +- `src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/CalculationFilterGroup.cs` — property → method (Task 5) +- `src/Strata.SqlTools.SqlBreakdown/Expressions/InputPropertyExpression.cs` — `[SuppressMessage]` on `AllChildData` (Task 5) +- `src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/QueryConfigExtensions.cs` — caller update (Task 5) +- `src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/QueryConfigExtensions.cs` — caller update (Task 5) + +**Created:** +- `tests/Strata.SqlTools.PostgreSql.Tests/PostgreSql/CommandVisitorTests.cs` — cross-instance isolation test for S2696 (Task 4) + +--- + +## Task 1: S927 — Rename `IVisitor.VisitNotEquals` parameter + +**Files:** +- Modify: `src/Strata.SqlTools.Rules/ExpressionVisitor.cs:17` + +**Background:** Sonar flags the two implementation parameters (`notEqual`) as not matching the interface declaration (`Equal`). The interface is the one that's wrong — `Equal` makes no sense as a name on a `VisitNotEquals` method. Renaming the interface fixes both implementations in one shot. Only known caller is positional (`visitor.VisitNotEquals(this)` in `NotEqual.cs:19`), so the rename is binary-safe. + +- [ ] **Step 1: Read current state** + +Run: +```powershell +Select-String -Path src/Strata.SqlTools.Rules/ExpressionVisitor.cs -Pattern "VisitNotEquals" -SimpleMatch +``` + +Expected output includes: +``` +17: T VisitNotEquals(NotEqual Equal); +44: public virtual Expression VisitNotEquals(NotEqual notEqual) => Validate(notEqual); +80: public virtual string VisitNotEquals(NotEqual notEqual) +``` + +- [ ] **Step 2: Apply rename in interface** + +Edit `src/Strata.SqlTools.Rules/ExpressionVisitor.cs` line 17 from: + +```csharp + T VisitNotEquals(NotEqual Equal); +``` + +to: + +```csharp + T VisitNotEquals(NotEqual notEqual); +``` + +- [ ] **Step 3: Build to confirm no callers broke** + +Run: +```powershell +dotnet build Strata.SqlTools.QueryBreakdown.sln -c Release --nologo +``` + +Expected: build succeeds (warnings OK, errors not). + +- [ ] **Step 4: Run tests** + +Run: +```powershell +dotnet test Strata.SqlTools.QueryBreakdown.sln -c Release --no-build --nologo +``` + +Expected: all tests pass; counts match the baseline (PostgreSql 107P/1S, SqlBreakdown 148P, Snowflake 220P/1S, EFCore 15P, LinqToSql 185P, Markdown 143P/4S, plus Rules and SqlServer test projects). + +- [ ] **Step 5: Commit** + +```powershell +git -C C:/gitea/sql-utilities add src/Strata.SqlTools.Rules/ExpressionVisitor.cs +git -C C:/gitea/sql-utilities commit -m "fix(rules): rename IVisitor.VisitNotEquals parameter to match impls + +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) " +``` + +--- + +## Task 2: S4487 — Remove unread `_breakdown` field + +**Files:** +- Modify: `src/Strata.SqlTools.LinqToSql/Builders/LinqQueryBreakdownBuilder.cs:11, :24` + +**Background:** Field is set in the constructor but never read. Confirmed by grep — only two references, both writes (declaration + constructor assignment). Safe delete. + +- [ ] **Step 1: Confirm no readers exist** + +Run: +```powershell +Select-String -Path src/Strata.SqlTools.LinqToSql -Pattern "_breakdown" -SimpleMatch -Recurse +``` + +Expected: only `LinqQueryBreakdownBuilder.cs:11` and `LinqQueryBreakdownBuilder.cs:24`. If any other file references `_breakdown`, **stop and investigate** — those would need updates before deletion. + +- [ ] **Step 2: Delete the field declaration** + +Edit `src/Strata.SqlTools.LinqToSql/Builders/LinqQueryBreakdownBuilder.cs` to remove line 11: + +```csharp + private readonly LinqQueryBreakdown _breakdown; +``` + +(Leave the surrounding lines for `_selectColumns`, `_fromTable`, etc. intact.) + +- [ ] **Step 3: Delete the constructor initialization** + +In the same file, in the constructor at line 22-25, remove the `_breakdown` assignment so the constructor body is empty: + +Before: +```csharp + public LinqQueryBreakdownBuilder() + { + _breakdown = new LinqQueryBreakdown(); + } +``` + +After: +```csharp + public LinqQueryBreakdownBuilder() + { + } +``` + +- [ ] **Step 4: Build and test** + +Run: +```powershell +dotnet build Strata.SqlTools.QueryBreakdown.sln -c Release --nologo +dotnet test Strata.SqlTools.QueryBreakdown.sln -c Release --no-build --nologo +``` + +Expected: clean build, all tests pass at baseline counts. + +- [ ] **Step 5: Commit** + +```powershell +git -C C:/gitea/sql-utilities add src/Strata.SqlTools.LinqToSql/Builders/LinqQueryBreakdownBuilder.cs +git -C C:/gitea/sql-utilities commit -m "fix(linq2sql): remove unread _breakdown field from builder + +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) " +``` + +--- + +## Task 3: S3875 — Suppress `operator==` rule with justification + +**Files:** +- Modify: `src/Strata.SqlTools.Rules/Rule/Expression/Expression.Operators.cs:7-8` + +**Background:** The `operator==` overload on `Expression` is intentional DSL syntax — it returns a `Comparison` expression, not a `bool`. The file already has `#pragma warning disable CS0660, CS0661` documenting that `Equals` / `GetHashCode` are deliberately not overridden for the same reason. Adding a `[SuppressMessage]` for S3875 makes the analyzer agree with the design. + +- [ ] **Step 1: Add `using` for `SuppressMessage` if absent** + +Read `src/Strata.SqlTools.Rules/Rule/Expression/Expression.Operators.cs` lines 1-9. If there's no `using System.Diagnostics.CodeAnalysis;` near the top, add it as the first line of the file. + +- [ ] **Step 2: Apply `[SuppressMessage]` to the partial class declaration** + +Edit lines 6-8 from: + +```csharp +#pragma warning disable CS0660, CS0661 +public partial class Expression +#pragma warning restore CS0660, CS0661 +``` + +to: + +```csharp +#pragma warning disable CS0660, CS0661 +[SuppressMessage("Major Code Smell", "S3875:\"operator==\" should not be overloaded on reference types", Justification = "Intentional DSL syntax: `expr1 == expr2` constructs a Comparison rule expression, not a bool. The existing CS0660/CS0661 pragma documents the deliberate omission of Equals/GetHashCode for the same reason.")] +public partial class Expression +#pragma warning restore CS0660, CS0661 +``` + +- [ ] **Step 3: Build and test** + +Run: +```powershell +dotnet build Strata.SqlTools.QueryBreakdown.sln -c Release --nologo +dotnet test Strata.SqlTools.QueryBreakdown.sln -c Release --no-build --nologo +``` + +Expected: clean build, all tests pass at baseline. The local Sonar analyzer should no longer warn on S3875 for this class. + +- [ ] **Step 4: Commit** + +```powershell +git -C C:/gitea/sql-utilities add src/Strata.SqlTools.Rules/Rule/Expression/Expression.Operators.cs +git -C C:/gitea/sql-utilities commit -m "fix(rules): suppress S3875 on intentional DSL operator== + +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) " +``` + +--- + +## Task 4: S2696 — Make `CommandVisitor._parameterIndex` instance-scoped + +**Files:** +- Modify: `src/Strata.SqlTools.PostgreSql/Visitors/CommandVisitor.cs:12, :30` +- Create: `tests/Strata.SqlTools.PostgreSql.Tests/PostgreSql/CommandVisitorTests.cs` + +**Background:** `private static int _parameterIndex = 1;` is incremented inside an instance method `FormatParameterName`. Two `CommandVisitor` instances share state and the counter never resets — almost certainly a latent bug. Fix is one keyword removal. Add a test that proves cross-instance independence so the contract sticks. + +- [ ] **Step 1: Write the failing test** + +Create `tests/Strata.SqlTools.PostgreSql.Tests/PostgreSql/CommandVisitorTests.cs`: + +```csharp +using NUnit.Framework; +using Strata.SqlTools.SqlBreakdown.Expressions; +using Strata.SqlTools.Visitors.PostgreSql; + +namespace Strata.SqlTools.PostgreSql.Tests.PostgreSql; + +[TestFixture] +public class CommandVisitorTests +{ + [Test] + public void TwoVisitors_HaveIndependentParameterIndices() + { + var visitor1 = new CommandVisitor(); + var visitor2 = new CommandVisitor(); + + var param1a = InvokeFormatParameterName(visitor1, "p"); + var param1b = InvokeFormatParameterName(visitor1, "p"); + var param2a = InvokeFormatParameterName(visitor2, "p"); + + Assert.That(param1a, Is.EqualTo("$1"), "first visitor's first parameter should be $1"); + Assert.That(param1b, Is.EqualTo("$2"), "first visitor's second parameter should be $2"); + Assert.That(param2a, Is.EqualTo("$1"), "second visitor must start at $1, not inherit visitor1's counter"); + } + + private static string InvokeFormatParameterName(CommandVisitor visitor, string name) + { + var method = typeof(CommandVisitor).GetMethod( + "FormatParameterName", + System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Public) + ?? typeof(Strata.SqlTools.Visitors.SqlServer.CommandVisitor).GetMethod( + "FormatParameterName", + System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic); + Assert.That(method, Is.Not.Null, "FormatParameterName must exist as a protected method on the visitor hierarchy"); + return (string)method!.Invoke(visitor, new object[] { name })!; + } +} +``` + +- [ ] **Step 2: Run the new test to confirm it fails** + +Run: +```powershell +dotnet test tests/Strata.SqlTools.PostgreSql.Tests/Strata.SqlTools.PostgreSql.Tests.csproj -c Release --filter "FullyQualifiedName~CommandVisitorTests" --nologo +``` + +Expected: FAIL — `param2a` will be `$3` (continuing visitor1's counter), not `$1`. The failure message will read something like *"second visitor must start at $1, not inherit visitor1's counter — Expected: \"$1\" But was: \"$3\""*. + +If the test instead passes immediately, the static field has already been changed; verify by reading the source. + +- [ ] **Step 3: Change static field to instance** + +Edit `src/Strata.SqlTools.PostgreSql/Visitors/CommandVisitor.cs` line 12 from: + +```csharp + private static int _parameterIndex = 1; +``` + +to: + +```csharp + private int _parameterIndex = 1; +``` + +No other changes needed — `FormatParameterName` already uses `_parameterIndex++` which now operates on the instance field. + +- [ ] **Step 4: Re-run the new test to confirm it passes** + +Run: +```powershell +dotnet test tests/Strata.SqlTools.PostgreSql.Tests/Strata.SqlTools.PostgreSql.Tests.csproj -c Release --filter "FullyQualifiedName~CommandVisitorTests" --nologo +``` + +Expected: PASS — all three assertions hold. + +- [ ] **Step 5: Run the full suite for regressions** + +Run: +```powershell +dotnet test Strata.SqlTools.QueryBreakdown.sln -c Release --no-build --nologo +``` + +Expected: all tests pass; PostgreSql test count is +1 vs. baseline. + +- [ ] **Step 6: Commit** + +```powershell +git -C C:/gitea/sql-utilities add src/Strata.SqlTools.PostgreSql/Visitors/CommandVisitor.cs tests/Strata.SqlTools.PostgreSql.Tests/PostgreSql/CommandVisitorTests.cs +git -C C:/gitea/sql-utilities commit -m "fix(pgsql): make CommandVisitor parameter index instance-scoped + +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) " +``` + +--- + +## Task 5: S2365 — Refactor filtering collection-property getters + +**Files:** +- Modify: `src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/CalculationFilterGroup.cs` +- Modify: `src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/CalculationFilterGroup.cs` +- Modify: `src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/QueryConfigExtensions.cs:14, :16` +- Modify: `src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/QueryConfigExtensions.cs:14, :16` +- Modify: `src/Strata.SqlTools.SqlBreakdown/Expressions/InputPropertyExpression.cs:376-380` (suppress only, no shape change) + +**Background:** Three S2365 issues. Two are nearly identical (`CalculationFilterGroup.Filters` in Snowflake + SqlServer): a property getter that runs `.Where(...).ToList()` on every access. Refactor: keep `Filters` as a plain auto-property holding the raw collection (JSON round-trip stays intact), add `GetValidFilters()` method that does the filtering. Update internal `IsValid()` and external callers in both `QueryConfigExtensions.cs`. + +The third (`HierarchicalData.AllChildData`) is in an interface, serialized via JSON, and its transformation IS the property's contract. Treat as `[SuppressMessage]` with justification — same pattern as S3875. + +This task produces one commit that touches both refactors and the suppression. + +### 5a — Snowflake `CalculationFilterGroup` + +- [ ] **Step 1: Apply the refactor** + +Replace the entire body of `src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/CalculationFilterGroup.cs` with: + +```csharp +using System.Text.Json.Serialization; + +namespace Strata.SqlTools.Snowflake.ExpressionFactory.Query; + +public class CalculationFilterGroup +{ + // Hereditary logical operation applied to all Filters + public LogicalOperator LogicalOperator { get; set; } + + public IEnumerable Filters { get; set; } + + public CalculationFilterGroup() + { + LogicalOperator = LogicalOperator.And; + Filters = new List(); + } + + [JsonConstructor] + public CalculationFilterGroup(IEnumerable filters, LogicalOperator logicalOperator) + { + Filters = filters; + LogicalOperator = logicalOperator; + } + + public IEnumerable GetValidFilters() + { + return Filters?.Where(x => x.IsValid()).ToList() ?? new List(); + } + + public bool IsValid() + { + return GetValidFilters().Any(); + } +} +``` + +Key changes: +- Removed `_filters` private field and `[JsonIgnore]` attribute (no longer needed) +- `Filters` is now a plain auto-property storing the raw collection +- Filtering logic moved to `GetValidFilters()` +- `IsValid()` updated to call `GetValidFilters().Any()` + +### 5b — SqlServer `CalculationFilterGroup` + +- [ ] **Step 2: Apply the same refactor in SqlServer** + +Replace the entire body of `src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/CalculationFilterGroup.cs` with the same content as Step 1 but with the namespace `Strata.SqlTools.SqlServer.ExpressionFactory.Query`: + +```csharp +using System.Text.Json.Serialization; + +namespace Strata.SqlTools.SqlServer.ExpressionFactory.Query; + +public class CalculationFilterGroup +{ + // Hereditary logical operation applied to all Filters + public LogicalOperator LogicalOperator { get; set; } + + public IEnumerable Filters { get; set; } + + public CalculationFilterGroup() + { + LogicalOperator = LogicalOperator.And; + Filters = new List(); + } + + [JsonConstructor] + public CalculationFilterGroup(IEnumerable filters, LogicalOperator logicalOperator) + { + Filters = filters; + LogicalOperator = logicalOperator; + } + + public IEnumerable GetValidFilters() + { + return Filters?.Where(x => x.IsValid()).ToList() ?? new List(); + } + + public bool IsValid() + { + return GetValidFilters().Any(); + } +} +``` + +### 5c — Update external callers in both `QueryConfigExtensions.cs` + +- [ ] **Step 3: Update Snowflake QueryConfigExtensions** + +In `src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/QueryConfigExtensions.cs`, replace lines 13-17: + +Before: +```csharp + return queryConfig.Values.SelectMany(value => value.CalculationDataColumnIds) + .Union(queryConfig.Values.SelectMany(x => x.FilterGroups.SelectMany(y => y.Filters.Select(f => f.DataColumnId)))) + .Union(queryConfig.Rows.Select(row => row.DataColumnId)) + .Union(queryConfig.FilterGroups.SelectMany(filterGroup => filterGroup.Filters.Select(filter => filter.DataColumnId))) + .ToArray(); +``` + +After: +```csharp + return queryConfig.Values.SelectMany(value => value.CalculationDataColumnIds) + .Union(queryConfig.Values.SelectMany(x => x.FilterGroups.SelectMany(y => y.GetValidFilters().Select(f => f.DataColumnId)))) + .Union(queryConfig.Rows.Select(row => row.DataColumnId)) + .Union(queryConfig.FilterGroups.SelectMany(filterGroup => filterGroup.GetValidFilters().Select(filter => filter.DataColumnId))) + .ToArray(); +``` + +Rationale: the previous code accessed `.Filters` which was already filtered. To preserve the same semantics, switch to `.GetValidFilters()`. If you wanted the raw set instead, `.Filters` would still work — but the current behavior is "only valid filters contribute to column IDs," so we keep that. + +- [ ] **Step 4: Update SqlServer QueryConfigExtensions** + +Apply the identical change to `src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/QueryConfigExtensions.cs` lines 13-17 (same before/after as Step 3). + +### 5d — Suppress S2365 on `HierarchicalData.AllChildData` + +- [ ] **Step 5: Add `using` and `[SuppressMessage]`** + +In `src/Strata.SqlTools.SqlBreakdown/Expressions/InputPropertyExpression.cs`, confirm `using System.Diagnostics.CodeAnalysis;` is present near the top. If absent, add it. + +Then locate the `AllChildData` property on the `HierarchicalData` class (around line 376): + +Before: +```csharp + public IEnumerable AllChildData + { + get => _childDataMap.SelectMany(x => x.Value).ToList(); + set => _childDataMap = value.GroupBy(x => x.DataSourceGuid).ToDictionary(x => x.Key, x => x.ToList()); + } +``` + +After: +```csharp + [SuppressMessage("Major Code Smell", "S2365:Properties should not make collection or array copies", Justification = "AllChildData is part of the IHierarchicalData interface contract and is JSON-serialized (see Data.json). The flatten-on-get / group-on-set transformation is the deliberate purpose of the property — _childDataMap is the storage form, the property is the wire form.")] + public IEnumerable AllChildData + { + get => _childDataMap.SelectMany(x => x.Value).ToList(); + set => _childDataMap = value.GroupBy(x => x.DataSourceGuid).ToDictionary(x => x.Key, x => x.ToList()); + } +``` + +### 5e — Verify and commit the bundle + +- [ ] **Step 6: Build** + +Run: +```powershell +dotnet build Strata.SqlTools.QueryBreakdown.sln -c Release --nologo +``` + +Expected: clean build. If `QueryConfigExtensions.cs` fails to compile, double-check that `GetValidFilters()` is spelled correctly and the call sites compile against the new method signature. + +- [ ] **Step 7: Run full suite** + +Run: +```powershell +dotnet test Strata.SqlTools.QueryBreakdown.sln -c Release --no-build --nologo +``` + +Expected: all tests pass at baseline + the new test from Task 4. If any test that exercises `CalculationFilterGroup` serialization breaks, inspect — the JSON shape did change (raw filters now serialize, not pre-filtered). If that's an issue, switch back to the suppression approach for the two `Filters` properties as well. + +- [ ] **Step 8: Commit** + +```powershell +git -C C:/gitea/sql-utilities add src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/CalculationFilterGroup.cs src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/CalculationFilterGroup.cs src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/QueryConfigExtensions.cs src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/QueryConfigExtensions.cs src/Strata.SqlTools.SqlBreakdown/Expressions/InputPropertyExpression.cs +git -C C:/gitea/sql-utilities commit -m "refactor: address S2365 collection-copying property getters + +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) " +``` + +--- + +## Final verification + +- [ ] **Step 1: Confirm clean tree and 6 commits ahead of main** + +Run: +```powershell +git -C C:/gitea/sql-utilities status --short +git -C C:/gitea/sql-utilities log --oneline main..HEAD +``` + +Expected: empty working tree; six commits on `fix/Sonarqube-Tech-Debt`: +1. `docs: spec for critical-tier SonarQube tech-debt sweep` +2. `fix(rules): rename IVisitor.VisitNotEquals parameter to match impls` +3. `fix(linq2sql): remove unread _breakdown field from builder` +4. `fix(rules): suppress S3875 on intentional DSL operator==` +5. `fix(pgsql): make CommandVisitor parameter index instance-scoped` +6. `refactor: address S2365 collection-copying property getters` + +- [ ] **Step 2: Final full build + test pass** + +Run: +```powershell +dotnet build Strata.SqlTools.QueryBreakdown.sln -c Release --nologo +dotnet test Strata.SqlTools.QueryBreakdown.sln -c Release --no-build --nologo +``` + +Expected: clean build, full suite green, test count = baseline + 1 (for the new `CommandVisitorTests`). + +- [ ] **Step 3: Push SonarQube analysis (optional but recommended)** + +Run (requires `$env:SONAR_TOKEN` set): +```powershell +./scan-sonar.ps1 +``` + +Expected: scanner uploads results to `https://snrqbe.bermudalamb.synology.me`. Open the dashboard and confirm: +- S3875 issue shows as **suppressed** (Won't Fix or Resolved-Won't-Fix) +- S2365 issue on `HierarchicalData.AllChildData` shows as **suppressed** +- The other 6 issues (S927 x2, S4487, S2696, S2365 x2) show as **resolved** + +- [ ] **Step 4: Merge to main** + +```powershell +git -C C:/gitea/sql-utilities checkout main +git -C C:/gitea/sql-utilities merge --ff-only fix/Sonarqube-Tech-Debt +``` + +Expected: fast-forward merge succeeds. If it can't fast-forward (i.e. main moved during the sweep), pause and let the user decide between rebase, merge commit, or holding. + +--- + +## Out of scope (do not do in this plan) + +- Refactor any S3776 cognitive-complexity hotspot +- Touch any NUnit2045/2046 test-modernization issue +- Change `.gitea/workflows/sonarqube.yml` (coverage reporting is being worked separately) +- Push the branch to remote (Gitea) — user has been working locally