DRYs the four `Enumerable.Empty<T>().AsQueryable()` returns added in
`2c23ba4` (S1168 fix) into a single `public static IQueryable<T>
GetEmptyQueryable<T>()` helper on `LinqQueryBreakdown`. No behavior
change — all 1180 tests stay green.
Co-Authored-By: Thom Lamb <thomlamb@gmail.com>
Three public methods on the SqlServer-namespaced Markdown generators no
longer touch instance state and now carry the `static` keyword:
- `Markdown.SqlServer.QueryBreakdownGenerator.GenerateMermaidDiagram(QueryBreakdown, string?)`
- `Markdown.SqlServer.SqlStatementGenerator.GenerateSequenceDiagram(ISqlBreakdown, string?)`
- `Markdown.SqlServer.SqlStatementGenerator.GenerateEntityRelationshipDiagram(IEnumerable<string>, string?)`
Plus one private bonus the analyzer caught on the same pass:
- `LinqExpressionVisitor.ExtractSelectExpression` → static (non-breaking).
Internal callers in the Snowflake/LinqToSql/PostgreSql wrapper classes
and in the test fixtures are updated to the type-name form
(`SqlServer.SqlStatementGenerator.GenerateSequenceDiagram(...)`).
The wrappers retain their `_baseGenerator` field for now even though it
is no longer used — that S4487 / unused-field cleanup is its own commit.
BREAKING CHANGE: External NuGet consumers calling
`generatorInstance.GenerateMermaidDiagram(...)`,
`generatorInstance.GenerateSequenceDiagram(...)`, or
`generatorInstance.GenerateEntityRelationshipDiagram(...)` on the
SqlServer-namespaced generators must switch to type-name form, e.g.
`Markdown.SqlServer.SqlStatementGenerator.GenerateSequenceDiagram(...)`.
Calls through the Snowflake / LinqToSql / PostgreSql wrapper classes are
unaffected at the call site.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two private methods that don't touch instance state get the `static`
keyword:
- `LinqExpressionVisitor.ExtractMemberName`
- `Markdown.Expressions.ExpressionGenerator.GenerateMermaidDiagram(Expression)`
Both have only intra-class callers, so this is a non-breaking change —
the call sites continue to work unchanged under C# method-resolution.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces `null` returns from `SqlBreakdownBase.GetQuery<T>()` and its four
overrides (LinqQueryBreakdown, SqlServer/PostgreSql/Snowflake QueryBreakdown)
with `Enumerable.Empty<T>().AsQueryable()`, and tightens the signature from
`IQueryable<T>?` to `IQueryable<T>`. The "we can't reconstruct" semantic
now lives in "the query yields zero rows" rather than a nullable return,
which is what callers in LINQ pipelines actually want.
Three LinqQueryBreakdownTests tests asserting `Is.Null` are renamed and
updated to assert `Is.Empty`.
BREAKING CHANGE: SqlBreakdownBase.GetQuery<T> and the SqlServer / PostgreSql /
Snowflake / LinqToSql QueryBreakdown.GetQuery<T> overrides no longer return
`IQueryable<T>?`; they now return a non-nullable `IQueryable<T>` that is
empty when reconstruction isn't possible. External NuGet consumers null-
checking the result must switch to `.Any()` / `Is.Empty` checks instead.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Re-runs `dotnet format analyzers --diagnostics NUnit2046 NUnit2045` after
the Tier 2 Assert.Multiple wrap, which exposed:
- `Has.Length.EqualTo(n)` rewrites for `string[]`/array `.Length` checks
(the first pass only knew about `.Count`).
- `Is.Not.Empty` rewrites for `Count, Is.GreaterThan(0)`.
- A handful of new NUnit2045 groups that became wrappable once the
initial Multiple blocks settled the surrounding indentation.
Tests still 1180/1180 passing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Promote the `new JsonSerializerOptions { Converters = { ... } }` instance
that `OneTimeSetup` was constructing on each fixture run to a
`private static readonly JsonSerializerOptions _jsonOptions` field, so
the converter list isn't rebuilt per fixture. Strictly cosmetic here
(OneTimeSetup runs once) but it's the change the analyzer wants and the
field is the more idiomatic JsonSerializer pattern.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two inline `new[] { ... }` literals inside TestCaseSource yield-returns
get hoisted to `static readonly string[]` fields with descriptive names
(`monthListValues`, `calendarRange`) matching the surrounding test-case
identifiers. Avoids reconstructing the same array on each call.
Only the test-file CA1861 sites are touched; the three production-code
sites flagged for the same rule are out of scope for this branch and
will land separately.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Driven by `dotnet format analyzers --diagnostics NUnit2045`. The fixer
groups consecutive independent `Assert.That(...)` calls into
`Assert.Multiple(() => { ... })`, so a failing assertion no longer
short-circuits the block — every failure inside the group is reported,
which gives much better diagnostics on multi-property tests.
Audit confirmed no Assert.Throws / Assert.Fail / Assert.Catch / Assert.Pass
/ Assert.DoesNotThrow got pulled inside a Multiple block (those need to
short-circuit). All 1180 tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- NUnit2046: `Assert.That(x.Count, Is.EqualTo(n))` → `Assert.That(x, Has.Count.EqualTo(n))` (or `Is.Empty` when n==0)
- NUnit2011: `Assert.That(s.Contains(x))` → `Assert.That(s, Does.Contain(x))` for richer failure messages
- CA1866: `.StartsWith("$"|"@"|":")` → `.StartsWith('$'|'@'|':')` char overload
Driven by `dotnet format analyzers --diagnostics NUnit2046 NUnit2011 CA1866`
for the cases the Roslyn fixer handles, plus a regex sweep for the remaining
`Count == n` (n>0) cases which the fixer doesn't address. CA1866 had no
associated code fix and was edited by hand (3 sites in 2 files). All tests
green (1180 passing).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Manual sweep of all 42 IDE0028 sites flagged by SonarQube — `dotnet format
analyzers --diagnostics IDE0028` declined to fix these (no .editorconfig
opt-in for `dotnet_style_prefer_collection_expression`), so applied by
hand. The repo already targets `<LangVersion>latest</LangVersion>` on
net8.0, so C# 12 collection expressions are available.
Pattern: `new List<T>()` / `new Dictionary<K,V>()` / `new ArrayList()` /
`new()` -> `[]` for empty; `new List<T> { ... }` -> `[...]` for literal.
24 files touched in src/{EFCore, LinqToSql, Query, Snowflake, SqlBreakdown,
SqlServer}; tests untouched (no IDE0028 sites in test code).
Build clean (35 warnings unchanged from baseline, 0 errors). All tests
remain green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Section A (server inspection / API access) is unchanged. Section B is
rewritten around the tiered model:
- Tier 1 — mechanical: bulk fix multiple rule IDs in one dotnet format
invocation (IDE0028, CA1825, CA1834, etc).
- Tier 2 — judgment: one commit per rule, audit diff before staging.
Calls out CA1822 public-static = binary break, CA1861 cryptic
generated field names + duplicate-field collisions, CA1859 has no
batch fixer.
- Tier 3 — manual / no fixer: csharpsquid:Sxxxx, public-API design
calls. Reference [[sonarqube-wontfix-rules]] memory for triage.
Adds the local-Java-version gotcha: `./scan-sonar.ps1` is blocked here
(system JRE is 8, scanner needs 17); CI handles the upload via
actions/setup-java@v4 with temurin 17.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Applied via `dotnet format analyzers --diagnostics CA1854 --severity info`.
Eliminates the duplicate hash lookup in the
`if (d.ContainsKey(k)) d[k]++ else d[k] = 1` pattern. The fixer rewrites
the conditional to `if (d.TryGetValue(k, out var value)) d[k] = ++value;`
which is semantically identical but does the lookup once.
4 files touched.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Applied via `dotnet format analyzers --diagnostics CA1861 --severity info`,
plus manual cleanup:
- Renamed two cryptic fixer-generated field names:
- QueryBreakdownCollection.stringArray -> SnowflakeFunctionNames (and
inlined the now-redundant local alias)
- ExpressionObjectTests.arg2 -> NotInValues
- Deduped three identical `separator = ['\r','\n']` fields the fixer
emitted in the same test class (kept the first declaration; the other
two test methods now reuse it).
8 files touched.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Applied via `dotnet format analyzers --diagnostics CA1822 --severity info`.
12 files touched. The fixer also updated internal callers in tests to use
the type-name form (e.g. `gen.Method(x)` -> `Generator.Method(x)`); build
and full test suite remain green.
BREAKING CHANGE: two public methods become static and therefore can no
longer be invoked through an instance reference by external consumers:
- Strata.SqlTools.Markdown.LinqToSql.QueryBreakdownGenerator.GenerateMethodChainDiagram
- Strata.SqlTools.Markdown.LinqToSql.SqlStatementGenerator.GenerateLinqPipelineDiagram
Both are stateless utility methods on Generator classes — the static form
is the correct shape; the only callers in this repo already used the
type-name form. External code should change `gen.GenerateMethodChainDiagram(...)`
to `QueryBreakdownGenerator.GenerateMethodChainDiagram(...)`.
All other CA1822 hits in this commit are on private/protected members
(no public-surface impact).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Applied via `dotnet format analyzers --diagnostics CA1510 --severity info`.
Replaces `if (x == null) throw new ArgumentNullException(nameof(x));`
blocks with the one-line `ArgumentNullException.ThrowIfNull(x);` —
same behavior, same parameter name, much less noise.
6 files touched across SqlBreakdown, SqlServer, LinqToSql.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add .sonarqube/ (created by dotnet sonarscanner begin) and scan.log
(from local scan-sonar.ps1 diagnostics) so they don't accidentally
get committed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CS8601 is reported via external_roslyn on the SonarQube server, which does
not expose transitions for external-analyzer issues — so a server-side
Won't Fix is not available. Silence locally with a narrow pragma so the
issue stops appearing in subsequent scans.
Justification: Parameters is Dictionary<string, object> (non-nullable
value annotation), but a SQL parameter value can legitimately be null.
The proper fix is to widen the public dictionary value type to object?,
which ripples through every consumer of QueryBreakdown.Parameters —
deferred to a separate change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codifies (A) how to query the bermudalamb SonarQube 9.9 server's web API
from PowerShell — Basic auth with token-as-username, the
SONARQUBE_URL trailing-slash gotcha, the useful endpoints for triage and
Won't Fix transitions; and (B) the per-rule-group cleanup loop (query →
pick → edit → build → test → commit → scan → verify).
References memory sonarqube-wontfix-rules rather than duplicating the
catalog. Frontmatter follows superpowers:writing-skills (description is
triggering conditions only, no workflow summary).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
S2342 requires [Flags] enums to use plural names. Both enums have zero
references anywhere in the repo today.
- ConstraintType -> ConstraintTypes
- TriggerType -> TriggerTypes
BREAKING CHANGE: external NuGet consumers (if any) referencing these
singular-named types will need to update to the plural names. Internal
codebase has no references.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Snowflake/Breakdowns/ProcedureBreakdown.ParseCallParameters — replace the
foreach over a MatchCollection with a .Cast<Match>().Select(m => m.Groups)
projection. The loop now iterates GroupCollection values directly, indexing
groups[1] and groups[2] for name/value without rebinding the Match.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LinqExpressionVisitor._tableName was only assigned and read inside
VisitConstant immediately before assigning FromClause. Drop the field
entirely and assign FromClause directly from entityType.Name.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Rules/Rule/Groups/With.GetExpressions only called base.GetExpressions(). The
comment 'do some ordering here??' indicates the override is a TODO stub.
Drop the override and preserve the intent as an inline TODO on the class.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SqlServer/Breakdowns/QueryBreakdown.cs:26 — _clausesCacheDirty was both
initialized inline (= true) and re-assigned in the constructor at line 56.
Drop the inline initializer; the ctor remains authoritative.
The four nearby S3604 false-positives on clause backing fields stay
suppressed via #pragma — they are write-through targets of cache-invalidating
property setters.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- WithClause.RecursiveQuery and WithClause.ColumnList had get/set bodies that
only forwarded to private backing fields. Convert both to auto-properties
and remove the now-orphaned _recursiveQuery / _columnList fields.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `[Serializable]` attribute and corresponding `[OnDeserialized]` methods have been removed from various breakdown classes. This eliminates reliance on `BinaryFormatter`, which is a deprecated and insecure serialization mechanism in modern .NET.
This change also resolves SonarQube rule S5766 warnings by removing the context in which they apply, leading to cleaner and more secure code.
Introduce a default regex match timeout across the library to prevent potential ReDoS attacks (SonarQube rule S6444).
Implement `[OnDeserialized]` methods to re-establish object invariants and validate state after deserialization, addressing SonarQube rule S5766.
Refactors the `CalculationFilterGroup` class to utilize C# primary constructors and property initializers, improving conciseness and readability.
The `IsValid()` method's logic is updated. Previously, it returned true if *any* filter in the group was valid. Now, it returns true only if *all* filters are valid, and an empty filter group is considered valid. This adjustment clarifies the group's validity criteria, aligning with common interpretations for `All` operations and addressing related technical debt.
Resolves SonarQube NUnit2045. The three independent parameter-index
assertions now report together instead of short-circuiting on the first
failure. The Is.Not.Null guard in the reflection helper stays outside the
multiple block because it gates the subsequent Invoke.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Moves the byte-identical 19-file ExpressionFactory/Query tree (duplicated
across Snowflake and SqlServer) into the new Strata.SqlTools.Query project
under the flat namespace Strata.SqlTools.Query. Both dialect projects now
reference the shared project; the Snowflake copies are deleted.
Also folds in the IDE0028 fix on CalculationFilterGroup.GetValidFilters
(collection expression []), which resolves both new-code IDE0028 smells
in one place now that there is a single copy.
Eliminates the 63 new duplicate lines flagged on the PR and removes the
largest contributor to the project's 11.1% duplication density. No
behavioral change: the moved types are identical to the originals.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
First step of extracting the duplicated ExpressionFactory/Query model
tree. Adds a BCL-only class library and registers it in the solution;
files are moved in the next commit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Task-by-task plan to scaffold the shared project, move the 19 identical
ExpressionFactory/Query files, rewire dialect references, and clear the 3
remaining new-code SonarQube smells (IDE0028 x2, NUnit2045).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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>
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.
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>
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>
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>
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>
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>
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>
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>
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>
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>
Thom Lambandbermudalamb