diff --git a/.claude/settings.local.json b/.claude/settings.local.json new file mode 100644 index 0000000..3970ede --- /dev/null +++ b/.claude/settings.local.json @@ -0,0 +1,19 @@ +{ + "permissions": { + "allow": [ + "PowerShell($url = 'https://snrqbe.bermudalamb.synology.me'; $tok = 'squ_49ed6268ca76f64072ff5d6a98ab21bdc352a3ef'; $pair = \"${tok}:\"; $b64 = [Convert]::ToBase64String\\([Text.Encoding]::ASCII.GetBytes\\($pair\\)\\); $h = @{ Authorization = \"Basic $b64\" }; $v = Invoke-WebRequest -Uri \"$url/api/server/version\" -Headers $h -SkipHttpErrorCheck; \"Version: $\\($v.Content\\)\"; $s = Invoke-WebRequest -Uri \"$url/api/system/status\" -Headers $h -SkipHttpErrorCheck; \"Status: $\\($s.Content\\)\")", + "PowerShell($url = 'https://snrqbe.bermudalamb.synology.me'; $tok = 'squ_49ed6268ca76f64072ff5d6a98ab21bdc352a3ef'; $b64 = [Convert]::ToBase64String\\([Text.Encoding]::ASCII.GetBytes\\(\"${tok}:\"\\)\\); $h = @{ Authorization = \"Basic $b64\" }; $p = Invoke-RestMethod -Uri \"$url/api/projects/search?ps=100\" -Headers $h; \"Projects \\($\\($p.paging.total\\)\\):\"; $p.components | ForEach-Object { \" - $\\($_.key\\) [$\\($_.name\\)] visibility=$\\($_.visibility\\) lastAnalysis=$\\($_.lastAnalysisDate\\)\" })", + "PowerShell($url='https://snrqbe.bermudalamb.synology.me'; $tok='squ_49ed6268ca76f64072ff5d6a98ab21bdc352a3ef'; $b64=[Convert]::ToBase64String\\([Text.Encoding]::ASCII.GetBytes\\(\"${tok}:\"\\)\\); $h=@{Authorization=\"Basic $b64\"}; $p='sql-utilities'; $iv=Invoke-RestMethod -Uri \"$url/api/issues/search?componentKeys=$p&resolved=false&ps=1&facets=severities,types,rules,files\" -Headers $h; \"=== Severity ===\"; \\($iv.facets | ? property -eq severities\\).values | Format-Table val, count -AutoSize; \"=== Type ===\"; \\($iv.facets | ? property -eq types\\).values | Format-Table val, count -AutoSize; \"=== Top 15 rules ===\"; \\($iv.facets | ? property -eq rules\\).values | Select-Object -First 15 | Format-Table val, count -AutoSize; \"=== Top 15 files ===\"; \\($iv.facets | ? property -eq files\\).values | Select-Object -First 15 | Format-Table val, count -AutoSize; \"Total open issues: $\\($iv.total\\)\")", + "PowerShell($url='https://snrqbe.bermudalamb.synology.me'; $tok='squ_49ed6268ca76f64072ff5d6a98ab21bdc352a3ef'; $b64=[Convert]::ToBase64String\\([Text.Encoding]::ASCII.GetBytes\\(\"${tok}:\"\\)\\); $h=@{Authorization=\"Basic $b64\"}; $p='sql-utilities'; \"=== BLOCKER + CRITICAL \\(showing rule + file\\) ===\"; $iv=Invoke-RestMethod -Uri \"$url/api/issues/search?componentKeys=$p&resolved=false&severities=BLOCKER,CRITICAL&ps=50\" -Headers $h; $iv.issues | Group-Object rule | Sort-Object Count -Descending | ForEach-Object { \"$\\($_.Count\\) x $\\($_.Name\\)\" }; \"\"; \"=== S3776 \\(cognitive complexity\\) hotspots ===\"; $s=Invoke-RestMethod -Uri \"$url/api/issues/search?componentKeys=$p&resolved=false&rules=csharpsquid:S3776&ps=20\" -Headers $h; $s.issues | ForEach-Object { \"$\\($_.component\\) line $\\($_.line\\) - $\\($_.message\\)\" })", + "Bash(dotnet test *)", + "PowerShell($url='https://snrqbe.bermudalamb.synology.me'; $tok='squ_49ed6268ca76f64072ff5d6a98ab21bdc352a3ef'; $b64=[Convert]::ToBase64String\\([Text.Encoding]::ASCII.GetBytes\\(\"${tok}:\"\\)\\); $h=@{Authorization=\"Basic $b64\"}; $p='sql-utilities'; $iv=Invoke-RestMethod -Uri \"$url/api/issues/search?componentKeys=$p&resolved=false&severities=BLOCKER,CRITICAL&rules=csharpsquid:S2365,csharpsquid:S927,csharpsquid:S2696,csharpsquid:S3875,csharpsquid:S4487&ps=50\" -Headers $h; \"Total: $\\($iv.total\\)\"; \"\"; $iv.issues | Sort-Object rule, component | ForEach-Object { $f = $_.component -replace '^sql-utilities:',''; \"[$\\($_.severity\\)] $\\($_.rule\\)`n $\\($f\\):$\\($_.line\\)`n $\\($_.message\\)`n\" })", + "Bash(git -C C:/gitea/sql-utilities status --short)", + "Bash(git -C C:/gitea/sql-utilities log --oneline -3)", + "Bash(git *)", + "PowerShell(git *)", + "Bash(dotnet build *)", + "Bash(command -v gh)", + "Bash(command -v tea)" + ] + } +} diff --git a/Directory.Build.props b/Directory.Build.props new file mode 100644 index 0000000..6bfd7b9 --- /dev/null +++ b/Directory.Build.props @@ -0,0 +1,10 @@ + + + + + all + runtime; build; native; contentfiles; analyzers; buildtransitive + + + + diff --git a/coverlet.runsettings b/coverlet.runsettings new file mode 100644 index 0000000..3df384b --- /dev/null +++ b/coverlet.runsettings @@ -0,0 +1,14 @@ + + + + + + + opencover + [*.Tests]*,[*.Tests.*]* + **/bin/**/*.cs,**/obj/**/*.cs + + + + + 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 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 diff --git a/scan-sonar.ps1 b/scan-sonar.ps1 new file mode 100644 index 0000000..5c425bb --- /dev/null +++ b/scan-sonar.ps1 @@ -0,0 +1,52 @@ +#!/usr/bin/env pwsh +# Runs a local SonarQube scan with coverage. +# Requires: $env:SONAR_TOKEN (and optionally $env:SONAR_HOST_URL). + +$ErrorActionPreference = 'Stop' +Set-Location $PSScriptRoot + +if (-not $env:SONAR_TOKEN) { + throw "Set `$env:SONAR_TOKEN before running (generate at /account/security)." +} + +$sonarHost = if ($env:SONAR_HOST_URL) { $env:SONAR_HOST_URL } else { 'https://snrqbe.bermudalamb.synology.me' } +$projectKey = 'sql-utilities' +$solution = 'Strata.SqlTools.QueryBreakdown.sln' + +if (-not (Get-Command dotnet-sonarscanner -ErrorAction SilentlyContinue)) { + Write-Host "Installing dotnet-sonarscanner..." -ForegroundColor Cyan + dotnet tool install --global dotnet-sonarscanner +} + +Write-Host "Cleaning previous coverage artifacts..." -ForegroundColor Cyan +Get-ChildItem -Path tests -Directory -Filter TestResults -Recurse -ErrorAction SilentlyContinue | + Remove-Item -Recurse -Force -ErrorAction SilentlyContinue + +Write-Host "sonarscanner begin..." -ForegroundColor Cyan +dotnet sonarscanner begin ` + /k:$projectKey ` + /d:sonar.host.url=$sonarHost ` + /d:sonar.token=$env:SONAR_TOKEN ` + /d:sonar.cs.opencover.reportsPaths="tests/**/TestResults/**/coverage.opencover.xml" ` + /d:sonar.exclusions="**/bin/**,**/obj/**" ` + /d:sonar.coverage.exclusions="tests/**,**/*.Tests/**" ` + /d:sonar.scanner.scanAll=false +if ($LASTEXITCODE -ne 0) { throw "sonarscanner begin failed ($LASTEXITCODE)" } + +Write-Host "dotnet build..." -ForegroundColor Cyan +dotnet build $solution --configuration Release +if ($LASTEXITCODE -ne 0) { throw "build failed ($LASTEXITCODE)" } + +Write-Host "dotnet test (with coverage)..." -ForegroundColor Cyan +dotnet test $solution ` + --configuration Release --no-build ` + --settings coverlet.runsettings ` + --collect "XPlat Code Coverage" +# don't throw on test failures — we still want the analysis to upload +if ($LASTEXITCODE -ne 0) { Write-Warning "some tests failed (exit $LASTEXITCODE); continuing so issues still upload" } + +Write-Host "sonarscanner end..." -ForegroundColor Cyan +dotnet sonarscanner end /d:sonar.token=$env:SONAR_TOKEN +if ($LASTEXITCODE -ne 0) { throw "sonarscanner end failed ($LASTEXITCODE)" } + +Write-Host "Done. Open $sonarHost/dashboard?id=$projectKey" -ForegroundColor Green diff --git a/src/Strata.SqlTools.LinqToSql/Builders/LinqQueryBreakdownBuilder.cs b/src/Strata.SqlTools.LinqToSql/Builders/LinqQueryBreakdownBuilder.cs index 73f644e..c9d0377 100644 --- a/src/Strata.SqlTools.LinqToSql/Builders/LinqQueryBreakdownBuilder.cs +++ b/src/Strata.SqlTools.LinqToSql/Builders/LinqQueryBreakdownBuilder.cs @@ -8,7 +8,6 @@ namespace Strata.SqlTools.Builders.LinqToSql; /// public class LinqQueryBreakdownBuilder { - private readonly LinqQueryBreakdown _breakdown; private readonly List _selectColumns = new(); private string? _fromTable; private string? _whereClause; @@ -21,7 +20,6 @@ public class LinqQueryBreakdownBuilder /// public LinqQueryBreakdownBuilder() { - _breakdown = new LinqQueryBreakdown(); } /// diff --git a/src/Strata.SqlTools.PostgreSql/Visitors/CommandVisitor.cs b/src/Strata.SqlTools.PostgreSql/Visitors/CommandVisitor.cs index 957bf69..0d3c687 100644 --- a/src/Strata.SqlTools.PostgreSql/Visitors/CommandVisitor.cs +++ b/src/Strata.SqlTools.PostgreSql/Visitors/CommandVisitor.cs @@ -9,7 +9,7 @@ namespace Strata.SqlTools.Visitors.PostgreSql; /// public class CommandVisitor : SqlServerCommandVisitor { - private static int _parameterIndex = 1; + private int _parameterIndex = 1; /// /// Formats an identifier for PostgreSQL using double-quote quoting. diff --git a/src/Strata.SqlTools.Rules/ExpressionVisitor.cs b/src/Strata.SqlTools.Rules/ExpressionVisitor.cs index 845f38a..2c46eea 100644 --- a/src/Strata.SqlTools.Rules/ExpressionVisitor.cs +++ b/src/Strata.SqlTools.Rules/ExpressionVisitor.cs @@ -14,7 +14,7 @@ public interface IVisitor T VisitLiteral(Literal literalRule); T VisitEquals(Equal Equal); - T VisitNotEquals(NotEqual Equal); + T VisitNotEquals(NotEqual notEqual); T VisitGreaterThan(GreaterThan GreaterThan); T VisitAnd(And And); diff --git a/src/Strata.SqlTools.Rules/Rule/Expression/Expression.Operators.cs b/src/Strata.SqlTools.Rules/Rule/Expression/Expression.Operators.cs index 1ba379c..25c1b2d 100644 --- a/src/Strata.SqlTools.Rules/Rule/Expression/Expression.Operators.cs +++ b/src/Strata.SqlTools.Rules/Rule/Expression/Expression.Operators.cs @@ -1,9 +1,12 @@ +using System.Diagnostics.CodeAnalysis; + namespace Strata.SqlTools.Rules.Rule.Expression; /// /// Provides implicit conversion operators and comparison operators for rule expressions. /// #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 { diff --git a/src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/CalculationFilterGroup.cs b/src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/CalculationFilterGroup.cs index fb48dc8..ce0f011 100644 --- a/src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/CalculationFilterGroup.cs +++ b/src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/CalculationFilterGroup.cs @@ -4,33 +4,31 @@ namespace Strata.SqlTools.Snowflake.ExpressionFactory.Query; public class CalculationFilterGroup { - [JsonIgnore] - private IEnumerable _filters; - // Hereditary logical operation applied to all Filters public LogicalOperator LogicalOperator { get; set; } - public IEnumerable Filters - { - get => _filters?.Where(x => x.IsValid()).ToList() ?? new List(); - set => _filters = value; - } + public IEnumerable Filters { get; set; } public CalculationFilterGroup() { LogicalOperator = LogicalOperator.And; - _filters = new List(); + Filters = new List(); } [JsonConstructor] public CalculationFilterGroup(IEnumerable filters, LogicalOperator logicalOperator) { - _filters = filters; + Filters = filters; LogicalOperator = logicalOperator; } + public IEnumerable GetValidFilters() + { + return Filters?.Where(x => x.IsValid()).ToList() ?? new List(); + } + public bool IsValid() { - return Filters != null && Filters.Any(); + return Filters != null && Filters.Any(x => x.IsValid()); } } diff --git a/src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/QueryConfigExtensions.cs b/src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/QueryConfigExtensions.cs index fd0c34c..7d62951 100644 --- a/src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/QueryConfigExtensions.cs +++ b/src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/QueryConfigExtensions.cs @@ -11,8 +11,9 @@ public static class QueryConfigExtensions public static int[] GetAllColumnIds(this QueryConfig queryConfig) { return queryConfig.Values.SelectMany(value => value.CalculationDataColumnIds) - .Union(queryConfig.Values.SelectMany(x => x.FilterGroups.SelectMany(y => y.Filters.Select(f => f.DataColumnId)))) + .Union(queryConfig.Values.SelectMany(x => x.FilterGroups.SelectMany(y => y.GetValidFilters().Select(f => f.DataColumnId)))) .Union(queryConfig.Rows.Select(row => row.DataColumnId)) + // FilterGroup.Filters is pre-filtered at construction (see FilterGroup.cs JsonConstructor); no GetValidFilters() equivalent is needed here. .Union(queryConfig.FilterGroups.SelectMany(filterGroup => filterGroup.Filters.Select(filter => filter.DataColumnId))) .ToArray(); } diff --git a/src/Strata.SqlTools.SqlBreakdown/Expressions/InputPropertyExpression.cs b/src/Strata.SqlTools.SqlBreakdown/Expressions/InputPropertyExpression.cs index 70ae99e..f984759 100644 --- a/src/Strata.SqlTools.SqlBreakdown/Expressions/InputPropertyExpression.cs +++ b/src/Strata.SqlTools.SqlBreakdown/Expressions/InputPropertyExpression.cs @@ -1,6 +1,7 @@ using Strata.SqlTools.SqlBreakdown.Interfaces.Core; using System.Collections; +using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.Text.Json; using System.Text.Json.Serialization; @@ -373,6 +374,7 @@ public class HierarchicalData : IHierarchicalData public IFlatData Data { get; set; } + [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(); diff --git a/src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/CalculationFilterGroup.cs b/src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/CalculationFilterGroup.cs index ca56ae4..ce4f1b1 100644 --- a/src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/CalculationFilterGroup.cs +++ b/src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/CalculationFilterGroup.cs @@ -4,33 +4,31 @@ namespace Strata.SqlTools.SqlServer.ExpressionFactory.Query; public class CalculationFilterGroup { - [JsonIgnore] - private IEnumerable _filters; - // Hereditary logical operation applied to all Filters public LogicalOperator LogicalOperator { get; set; } - public IEnumerable Filters - { - get => _filters?.Where(x => x.IsValid()).ToList() ?? new List(); - set => _filters = value; - } + public IEnumerable Filters { get; set; } public CalculationFilterGroup() { LogicalOperator = LogicalOperator.And; - _filters = new List(); + Filters = new List(); } [JsonConstructor] public CalculationFilterGroup(IEnumerable filters, LogicalOperator logicalOperator) { - _filters = filters; + Filters = filters; LogicalOperator = logicalOperator; } + public IEnumerable GetValidFilters() + { + return Filters?.Where(x => x.IsValid()).ToList() ?? new List(); + } + public bool IsValid() { - return Filters != null && Filters.Any(); + return Filters != null && Filters.Any(x => x.IsValid()); } } diff --git a/src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/QueryConfigExtensions.cs b/src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/QueryConfigExtensions.cs index 7cafd14..33f623d 100644 --- a/src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/QueryConfigExtensions.cs +++ b/src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/QueryConfigExtensions.cs @@ -11,8 +11,9 @@ public static class QueryConfigExtensions public static int[] GetAllColumnIds(this QueryConfig queryConfig) { return queryConfig.Values.SelectMany(value => value.CalculationDataColumnIds) - .Union(queryConfig.Values.SelectMany(x => x.FilterGroups.SelectMany(y => y.Filters.Select(f => f.DataColumnId)))) + .Union(queryConfig.Values.SelectMany(x => x.FilterGroups.SelectMany(y => y.GetValidFilters().Select(f => f.DataColumnId)))) .Union(queryConfig.Rows.Select(row => row.DataColumnId)) + // FilterGroup.Filters is pre-filtered at construction (see FilterGroup.cs JsonConstructor); no GetValidFilters() equivalent is needed here. .Union(queryConfig.FilterGroups.SelectMany(filterGroup => filterGroup.Filters.Select(filter => filter.DataColumnId))) .ToArray(); } diff --git a/tests/Strata.SqlTools.PostgreSql.Tests/PostgreSql/CommandVisitorTests.cs b/tests/Strata.SqlTools.PostgreSql.Tests/PostgreSql/CommandVisitorTests.cs new file mode 100644 index 0000000..1b3896a --- /dev/null +++ b/tests/Strata.SqlTools.PostgreSql.Tests/PostgreSql/CommandVisitorTests.cs @@ -0,0 +1,33 @@ +using Strata.SqlTools.Visitors.PostgreSql; + +namespace Strata.SqlTools.SqlBreakdown.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); + Assert.That(method, Is.Not.Null, "FormatParameterName must exist as a protected override on CommandVisitor"); + return (string)method!.Invoke(visitor, new object[] { name })!; + } +}