Thom LambandClaude Opus 4.7
55ab737888
Designs the extraction of the byte-identical 19-file ExpressionFactory/Query tree (duplicated across Snowflake + SqlServer) into a new Strata.SqlTools.Query project under a clean-break namespace, and folds in the 3 remaining new-code SonarQube smells (IDE0028 x2 on CalculationFilterGroup, NUnit2045 on CommandVisitorTests). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
82 lines
5.5 KiB
Markdown
82 lines
5.5 KiB
Markdown
# Extract `Strata.SqlTools.Query` shared project + clear 3 new-code smells — Design
|
||
|
||
**Date:** 2026-05-20
|
||
**Branch:** `fix/Sonarqube-Tech-Debt`
|
||
**Author:** Thom Lamb (with Claude)
|
||
|
||
## Goal
|
||
|
||
Resolve the SonarQube tech debt introduced by yesterday's sweep:
|
||
|
||
1. **3 new-code code smells** (the only issues in SonarQube's new-code period):
|
||
- `IDE0028` — `src/Strata.SqlTools.Snowflake/ExpressionFactory/Query/CalculationFilterGroup.cs:27`
|
||
- `IDE0028` — `src/Strata.SqlTools.SqlServer/ExpressionFactory/Query/CalculationFilterGroup.cs:27`
|
||
- `NUnit2045` — `tests/Strata.SqlTools.PostgreSql.Tests/PostgreSql/CommandVisitorTests.cs:18`
|
||
2. **63 new duplicate lines** flagged on the PR — caused by the `CalculationFilterGroup` rewrite landing in two byte-identical copies (Snowflake + SqlServer).
|
||
|
||
## Root cause of the duplication
|
||
|
||
Snowflake and SqlServer each carry a **byte-identical 19-file `ExpressionFactory/Query/` tree** (verified: every file diffs to zero non-namespace lines). Project-wide duplication density is 11.1% (3,147 duplicated lines / 99 blocks); this tree is a major contributor. The `CalculationFilterGroup` change merely tipped the new-code duplication over threshold.
|
||
|
||
The two flagged files cannot be extracted in isolation: their transitive type closure pulls in 14 of the 19 types (`QueryConfigExtensions → QueryConfig → Value → CalculationFilterGroup → CalculationFilter → Filter → …`). The remaining 4 (`AggregationType`, `ColumnQueryConfig`, `SqlResponse`, `ValueFilter`) are also identical duplicates. Therefore the coherent unit of extraction is the **whole 19-file tree**.
|
||
|
||
## Approach
|
||
|
||
Create a new shared class library and move the entire identical tree into it under a single new namespace.
|
||
|
||
### New project
|
||
|
||
- `src/Strata.SqlTools.Query/Strata.SqlTools.Query.csproj`
|
||
- `net8.0` (matches all existing projects)
|
||
- `PackageId = Strata.SqlTools.Query`, `GeneratePackageOnBuild = false` (matches dialect packages)
|
||
- No project references (the 19 types depend only on BCL: `System.Text.Json.Serialization`, `System.ComponentModel.DataAnnotations`)
|
||
- Added to `Strata.SqlTools.QueryBreakdown.sln`.
|
||
|
||
### Namespace (clean break)
|
||
|
||
- All 19 types move from `Strata.SqlTools.{Snowflake,SqlServer}.ExpressionFactory.Query` to the single flat namespace **`Strata.SqlTools.Query`**.
|
||
- File names are unchanged. One physical copy of each type.
|
||
- Rationale for clean break: both dialect packages have `GeneratePackageOnBuild = false` (not currently published), so the breaking namespace change has no live external consumers. If backward compat is later needed, `[TypeForwardedTo]` can be added without touching call sites.
|
||
|
||
### Wiring
|
||
|
||
- `Strata.SqlTools.Snowflake` and `Strata.SqlTools.SqlServer` both add a `ProjectReference` to `Strata.SqlTools.Query`.
|
||
- Delete all 38 dialect copies (19 in Snowflake, 19 in SqlServer).
|
||
- EFCore / LinqToSql / Markdown obtain the types transitively (they already reference the dialect projects).
|
||
|
||
### Consumer migration
|
||
|
||
Consumers reference these types either via a `using` directive or via relative resolution through the dialect's ancestor namespace. After the move they need `using Strata.SqlTools.Query;`. Known explicit-`using` sites to update:
|
||
|
||
- `src/Strata.SqlTools.SqlServer/ExpressionFactory/ExpressionFactory.cs`
|
||
- `tests/Strata.SqlTools.SqlBreakdown.Tests/ExpressionTests/ExpressionFactoryFilterTests.cs`
|
||
- `tests/Strata.SqlTools.SqlBreakdown.Tests/ExpressionTests/ExpressionTestsBase.cs`
|
||
|
||
Any remaining relative references break the build; the compiler enumerates them precisely. The fix is mechanical: add the `using` (or a per-project `global using Strata.SqlTools.Query;`).
|
||
|
||
**Collision safety:** No single source file references *both* dialect Query namespaces (verified via set intersection), so merging the two type families into one shared type cannot create ambiguous-overload or duplicate-signature errors.
|
||
|
||
## Code smell fixes (folded into the same branch)
|
||
|
||
1. **IDE0028 ×2 → ×1, then fixed.** After extraction there is one `CalculationFilterGroup.cs`. Change line 27 `Filters?.Where(x => x.IsValid()).ToList() ?? new List<CalculationFilter>()` to use the collection expression `?? []`. One edit resolves both original issues.
|
||
2. **NUnit2045.** In `CommandVisitorTests.cs`, wrap the three independent `Assert.That` assertions (lines 18–20) in `Assert.Multiple(() => { … });` so all three report together. (The `Assert.That(method, Is.Not.Null, …)` guard in the helper stays as-is — it gates a subsequent `Invoke`, so it is correctly *not* inside a multiple block.)
|
||
|
||
## Out of scope
|
||
|
||
- The other 798 project-wide code smells (S3776 cognitive complexity, CA/IDE info-level, etc.).
|
||
- Extracting non-identical dialect code (visitors, breakdowns, statement parsers).
|
||
- Publishing/packaging changes beyond the new project's csproj.
|
||
- Pushing to the Gitea remote (user works locally; merge decision deferred).
|
||
|
||
## Verification
|
||
|
||
1. `dotnet build Strata.SqlTools.QueryBreakdown.sln -c Release` — clean (errors fail; pre-existing warnings OK).
|
||
2. `dotnet test Strata.SqlTools.QueryBreakdown.sln -c Release --no-build` — all tests green at baseline (~819, including the Task 4 `CommandVisitorTests`).
|
||
3. Optional: `./scan-sonar.ps1` then confirm on the dashboard that the 3 new-code smells are gone and new-code duplicated lines drop to 0.
|
||
|
||
## Success criteria
|
||
|
||
- New-code period shows **0** code smells and **0** duplicated lines.
|
||
- Project-wide `duplicated_lines_density` drops measurably (the identical tree no longer double-counts).
|
||
- Build clean, full suite green, no behavioral change to query-config semantics.
|