The Gitea Actions sonarqube workflow was running `dotnet build` and
sonarscanner begin/end without ever invoking `dotnet test`, so Sonar
was reporting coverage = 0% on the server despite a 1180-test suite
already exercising the code locally. `scan-sonar.ps1` (the local
equivalent) has had the coverage flags + test step the whole time —
this aligns CI with it.
Changes:
- `sonarscanner begin` now passes
`/d:sonar.cs.opencover.reportsPaths="tests/**/TestResults/**/coverage.opencover.xml"`
so the scanner picks up the per-project OpenCover reports coverlet
emits, and
`/d:sonar.coverage.exclusions="tests/**,**/*.Tests/**"` so the tests
themselves don't count toward the coverage denominator.
- The dormant step 8 (a disabled `if: false` curl-debug block from an
earlier auth-troubleshooting session) is replaced with the actual
test run: `dotnet test ... --no-build --settings
coverlet.runsettings --collect "XPlat Code Coverage"`.
- Step 9 (`sonarscanner end`) unchanged — it now has coverage data
available to upload.
Once this lands, the next scan will surface the *real* coverage number
for sql-utilities. Until we see that number we don't know which files
actually need test investment vs. were already covered by the existing
suite.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After the CA1822 cascade in #15 left every method on the eight
Markdown generator classes static, the classes themselves were
instantiable shells that consumers couldn't usefully `new`. This
commit flips the `class` modifier to `static class` on all eight:
- `Markdown.SqlServer.QueryBreakdownGenerator`
- `Markdown.SqlServer.SqlStatementGenerator`
- `Markdown.LinqToSql.QueryBreakdownGenerator`
- `Markdown.LinqToSql.SqlStatementGenerator`
- `Markdown.PostgreSql.QueryBreakdownGenerator`
- `Markdown.PostgreSql.SqlStatementGenerator`
- `Markdown.Snowflake.QueryBreakdownGenerator`
- `Markdown.Snowflake.SqlStatementGenerator`
Test fixtures drop the now-meaningless `_generator = new …()` field
and `[SetUp]` (kept the existing Setup body where unrelated state was
also initialized — `QueryMarkdownGenerationTests` and
`LinqToSql.QueryBreakdownGeneratorTests`).
BREAKING CHANGE: External NuGet consumers can no longer write
`new Markdown.Snowflake.SqlStatementGenerator()` (or any of the other
seven classes above) — the type is now a static container and may only
be referenced by name, e.g.
`Markdown.Snowflake.SqlStatementGenerator.GenerateSequenceDiagram(…)`.
The call syntax for the static methods is unchanged.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- `SqlUtils.cs:43` and `StatementParser.cs:254,285` (CA1861 ×3): inline
`new[] { ' ' }` and `new[] { ';' }` Split delimiters hoisted to
`static readonly char[]` fields next to the existing `separator`
field. Distinct names (`spaceSeparator`, `semicolonSeparator`) avoid
collision.
- `SqlUtils.Filters.cs:27` (CA1806): `double.TryParse(value, out var
dblValue)` had its return value silently discarded — intentional
(downstream switch branches use `dblValue` only when relevant and
rely on the default `0.0` on failure). Now uses `_ =` to make the
discard explicit and extends the comment.
- `FilterCondition.cs:10` and `With.cs:10` (S1135 ×2): rewrite the
`todo:` / `TODO:` markers as plain "Future:" notes. Both comments
documented deliberate design choices ("inherit base for now",
"could be indexed later") rather than tracked work, so the marker
was misleading anyway.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- `SqlParseException.cs:90` (CA1846): `sql.Substring(0, 197)` →
`sql.AsSpan(0, 197)` in the truncated-SQL diagnostic message. Avoids
an allocation in an already cold exception path.
- `Snowflake/QueryBreakdown.cs:103` (CA1853): drop the redundant
`Parameters.ContainsKey(...)` guard around `Parameters.Remove(...)`.
`Dictionary<TKey,TValue>.Remove` is a no-op if the key is absent, so
the guard only doubled the work and computed the key string twice.
- `LinqQueryBreakdown.cs:194` (S2219): collapse the now-stub
`GetQuery<T>()` (every branch returned `GetEmptyQueryable<T>()` after
the S1168 cleanup) to a single expression-bodied member. Updates the
XML doc to describe the actual current behavior.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Commit 5202d93 made the SqlServer-namespaced Markdown generator methods
static, which left the LinqToSql / PostgreSql / Snowflake wrapper
classes' instance methods delegating to nothing but a static call.
SonarQube re-flagged those 10 wrapper methods as CA1822 on the next
scan.
This sweep:
- Makes all 10 wrapper instance methods `static` (`dotnet format` driven).
- Makes `Markdown.LinqToSql.QueryBreakdownGenerator.GenerateCombinedDiagram`
static preemptively — it composes two static helpers and would otherwise
be the next-iteration cascade flag.
- Removes the now-dead `_baseGenerator` field and its initializing
constructor from all six dialect wrappers (LinqToSql / PostgreSql /
Snowflake × QueryBreakdownGenerator + SqlStatementGenerator). The
classes keep their implicit parameterless constructor so `new
Snowflake.QueryBreakdownGenerator()` still compiles.
- Updates the one test call site (`GenerateCombinedDiagram`) the fixer
didn't catch to use type-name form.
BREAKING CHANGE: External NuGet consumers calling
`instance.Generate*Diagram(...)` on `Markdown.LinqToSql.*`,
`Markdown.PostgreSql.*`, or `Markdown.Snowflake.*` generators must
switch to type-name form, e.g. `Markdown.Snowflake.SqlStatementGenerator.GenerateSequenceDiagram(...)`.
The class types and parameterless constructors remain — only the call
syntax for these methods changes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
bermudalamb