Altinn / Altinn/altinn-authentication
Cleanup: remove compiler warnings, enable nullable for all projects and prevent new warnings from being added
- Dominant language
- C#
- Stars
- 8
- Forks
- 5
- Avg merge
- 3d 6h
- Merged PRs (30d)
- 18
Description
# Cleanup: remove compiler warnings, enable nullable, enforce WarnAsError
## Goal
Get to **0 compiler warnings**, **nullable reference types enabled on every project**, and **CI failing on any new warning** (`TreatWarningsAsErrors`).
## Current state (updated 2026-07-07, after #2102)
Nullable reference types are now **enabled on all projects**. `TreatWarningsAsErrors` is still off and there is no `Directory.Build.props` yet. A full `Release` solution build now surfaces **~1896 warnings** (down from ~1932 after OIDC e2e test consolidation (#2105); up from ~998 pre-nullable — enabling nullable exposed the real `CS86xx` warnings that were previously hidden).
**Per project**
| Project | Warnings | Nullable |
|---|---|---|
| `Tests` | 790 | ✅ (enabled in #2102) |
| `Authentication` (host) | 506 | ✅ (enabled in #2102) |
| `Core` | 410 | ✅ |
| `Integration` | 64 | ✅ |
| `SystemIntegrationTests` | 32 | ✅ |
| `Persistance` | 0 ✅ locked (WarnAsError, #2106) | ✅ |
**Top warning codes now**
| Code | Count | Meaning |
|---|---|---|
| CS8618 | 648 | non-nullable member uninitialized |
| CS8600 | 400 | null literal / possible null → non-nullable |
| CS8604 | 298 | possible null argument |
| CS8602 | 122 | dereference of possibly-null |
| CS8625 | 120 | cannot convert null literal to non-nullable |
| CS0219 | 92 | unused local |
| CS8603 | 64 | possible null return |
| CS8601 / CS8619 | 26 / 24 | null assignment / nullability mismatch |
| ASPDEPR004 | 18 | `WebHostBuilder` deprecated |
| CS4014 | 16 | async not awaited |
## Warning categories & fix strategy
| Category | Codes (count) | Strategy |
|---|---|---|
| **Nullable — uninitialized members** | CS8618 (402) | Dominant. Mostly `Core` DTOs/models with non-nullable `string`/ref properties never initialized. Fix with `required` (preferred for real required fields), `= null!` (deserialization targets), constructor init, or make the property nullable. |
| **Nullable — flow warnings** | CS8604 (122), CS8600 (58), CS8602 (44), CS8603 (28), CS8625 (22), CS8619 (10), CS8601 (10), CS8613 (6), CS8629 (2), CS8509 (2) | Per-usage nullability decisions: add null checks/guards, make signatures nullable where null is valid, or `!` where an invariant guarantees non-null (with a comment). |
| **Nullable annotation in disabled context** | CS8632 (90) | Files already use `?` annotations in the 3 nullable-**disabled** projects. **Resolved automatically** by enabling nullable on those projects (which then surfaces CS86xx to fix). |
| **Dead code** | CS0219 (92), CS0169 (10), CS0168 (4), CS9113 (4) | Mechanical, low risk: delete unused locals/fields/vars and unread primary-ctor params. |
| **Deprecated / obsolete APIs** | ASPDEPR004 `WebHostBuilder` (18), SYSLIB0057 `X509Certificate2(..)` (8), ASPDEPR005 `IPNetwork` (4), CS0618 (8) | API migrations (see below). |
| **Async not awaited** | CS4014 (16) | Fire-and-forget calls (e.g. audit-event logging). Either `await`, or intentionally discard with a justifying comment. Needs care — don't change timing semantics blindly. |
| **XML docs** | CS1573 (10), CS1574 (8), CS1587 (6), CS1570 (2) | Fix/remove malformed or mismatched doc comments. Low risk. |
| **NuGet redundant refs** | NU1510 (8) | Remove `PackageReference`s already provided transitively / by the framework. |
| **Equality** | CS0659 (2) | Add `GetHashCode` where `Equals` is overridden. |
### Deprecated-API migration detail
- `WebHostBuilder` → `WebApplicationFactory` / `HostBuilder` (ASPDEPR004, mostly in test setup).
- `new X509Certificate2(...)` → `X509CertificateLoader` (SYSLIB0057).
- `Microsoft.AspNetCore.HttpOverrides.IPNetwork` → `System.Net.IPNetwork`, and `AltinnClusterInfo.ClusterNetwork` → `TrustedProxies` (ASPDEPR005 + CS0618) — **coordinate with #2100** (client-IP / forwarded-headers).
- `UrnEncoded.TryUnescape` → `TryParse` (CS0618).
- Testcontainers `new PostgreSqlBuilder()` → ctor with image arg (CS0618).
## Recommended approach
1. **Land the low-risk sweeps first** (dead code, XML docs, NU1510, CS0659, deprecated-API migrations) so they don't collide with the larger nullable work.
2. **Enable nullable project-by-project**, leaf → root, fixing that project's warnings in the same PR: `Core` first (largest, foundational), then `Integration`/`Persistance` (already enabled — just clear warnings), then the host, jwtcookie, and finally the test projects.
3. **Enforce as you go**: once a project is clean, set `true` on it so it can't regress while the others are being cleaned.
4. **Centralize at the end**: add a root `Directory.Build.props` with `enable` + `true`, remove per-project overrides, and update the CI **Build and Test** job so any warning fails the build.
## Tasks
**Quick mechanical sweeps (low risk)**
- [ ] Remove dead code — CS0219 / CS0169 / CS0168 / CS9113 (~110)
- [ ] Fix XML doc warnings — CS1573 / CS1574 / CS1587 / CS1570 (~26)
- [x] Remove redundant PackageReferences — NU1510 (8) — suppressed solution-wide via root `Directory.Build.props` (#2107), per team guidance to suppress rather than remove
- [ ] Add missing `GetHashCode` — CS0659 (2)
**Deprecated-API migrations**
- [ ] `WebHostBuilder` → `WebApplicationFactory`/`HostBuilder` — ASPDEPR004 (18)
- [ ] `X509Certificate2` ctor → `X509CertificateLoader` — SYSLIB0057 (8)
- [ ] `IPNetwork` → `System.Net.IPNetwork` + `ClusterNetwork` → `TrustedProxies` — ASPDEPR005 + CS0618 (coordinate with #2100)
- [ ] `UrnEncoded.TryUnescape` → `TryParse`; testcontainers `PostgreSqlBuilder` ctor — CS0618
**Async correctness**
- [ ] Resolve not-awaited async calls — CS4014 (16)
**Nullable enablement (flag)**
- [x] Enable `enable` on all projects — #2102 (host, jwtcookie, Tests; the rest were already on). Model-binding gotcha it surfaced (`IntrospectionRequest`) fixed at the model level.
**Clear the surfaced nullable warnings (per project)** — ~1934 warnings now surfaced
- [ ] `Core` — CS8618 + flow warnings (~410)
- [x] `Persistance` — done + `TreatWarningsAsErrors` enabled (#2106)
- [x] `Integration` — cleared + locked (#2126); surfaced 4 latent NREs in the host, fixed in the same PR
- [ ] `Authentication` (host)
- [ ] `Altinn.Common.Authentication` (jwtcookie)
- [x] `Tests` + `SystemIntegrationTests` — both cleaned and locked (`SystemIntegrationTests` #2110; `Tests` over #2111 → #2113 → #2120 → #2121)
**Enforcement**
- [ ] Add root `Directory.Build.props` with `Nullable=enable` + `TreatWarningsAsErrors=true`
- [ ] Update CI **Build and Test** to fail on any warning (`-warnaserror`) and remove per-project overrides
## Acceptance criteria
- `dotnet build Altinn.Platform.Authentication.sln -c Release` produces **0 warnings**.
- Every project (including test projects) has nullable reference types enabled.
- CI fails the build if any new warning is introduced.
---
## Guidance from review (@Alxandr) + gotchas found while enabling nullable
**Nullable fix conventions**
- `= null!` is only for members that are **always** set by post-construction initialization (deserialization, async init in tests, etc.). Every use must carry a short comment explaining why it's safe. Don't use it to silence CS8618 on genuinely-optional members — make those nullable (`?`) instead; use `required` for genuinely-required ones.
**Dead code (CS0219/CS0169/CS9113) — not always dead**
- Some "unused" fields/properties are read by **reflection** (serialization, model binding, DI, config binding, test frameworks). Verify each is truly unreferenced (including reflection/attribute usage) before removing. When in doubt, keep it.
**`NU1510`**
- These should be **silenced/suppressed**, not "fixed" by removing package references. (e.g. `NU1510` or the appropriate MSBuild suppression.)
**⚠️ Gotcha: enabling nullable changes model-binding validation (found via a broken test)**
- Enabling nullable on the **host** made ASP.NET Core treat non-nullable reference properties on request models as implicitly `[Required]`. A request omitting such a property now returns **400** instead of binding it as null. This broke `IntrospectionControllerTest.ValidateToken_TokenHintNone_EFormidlingServiceCalled` (`IntrospectionRequest.TokenTypeHint` is optional per RFC 7662 but became required).
- Fix approach (per @Alxandr): annotate the affected model **explicitly** rather than globally suppressing. `IntrospectionRequest` was the only model-bound request type defined in the host (all others live in Core, already nullable); its `Token`/`TokenTypeHint` were made `string?` (token presence is validated in the controller; `token_type_hint` is optional per RFC 7662). Avoid the global `SuppressImplicitRequiredAttributeForNonNullableReferenceTypes` knob.
- **Follow-up task:** audit remaining request/binding models (mostly in Core) and express required-ness **explicitly** (`[Required]` / deliberate non-nullable) vs optional (`string?`).
**Task additions**
- [ ] Audit request/binding models for correct required vs optional annotations (explicit `[Required]` / `string?`)
- [ ] When removing dead code, confirm no reflection/serialization/DI/model-binding usage first
- [x] Suppress `NU1510` rather than removing package references — done repo-wide in root `Directory.Build.props` (#2107)
---
## Learnings from the first ratchet PR (Persistance, #2106)
Enabling `` on the first project surfaced several things that change the plan for the remaining projects:
1. **CI builds `Debug`, which turns on StyleCop analysers.** `TreatWarningsAsErrors` therefore promotes **StyleCop `SA*` warnings to errors**, not just `CS*`/nullable. Each project's ratchet PR must clear its **pre-existing StyleCop violations** too (Persistance had `SA1025`, `SA1508` sitting as warnings). The nullable/`CS*` counts earlier **undercount** the real per-project effort — budget for `SA*` on the big projects (host, Tests). Decision to make:
- **(a)** fix StyleCop as part of each ratchet (genuinely-clean projects), or
- **(b)** scope enforcement to specific codes via `CS8600;CS8602;…` instead of blanket `TreatWarningsAsErrors`, deferring StyleCop to a later pass.
2. **A green local `Release` build is not enough** — it doesn't run StyleCop. Validate each ratchet PR with a **`Debug`** build (`dotnet build ` with no `-c Release`) to match CI before pushing.
3. **`NU1510` bites every project under `TreatWarningsAsErrors`.** Silence it (per @Alxandr) — but do it **once, repo-wide**, via a root `Directory.Build.props` `$(NoWarn);NU1510` rather than per-project, so each new ratchet project doesn't rediscover it. (The host's `NU1510` warning is harmless noise until that project gets WarnAsError.)
4. **`= null!` / `!` still need a justification comment** — but watch StyleCop: a comment on its own line trips `SA1515`; prefer a **trailing** comment.
**Suggested adjustment to the plan:** land the repo-root `Directory.Build.props` with the shared `NoWarn` (NU1510) *early* (before the next ratchet project), and decide (a) vs (b) on StyleCop up front so the big-project PRs have a consistent target.
---
## StyleCop scope + enforcement decision (2026-07-07)
Measured `SA*` from a clean **Debug** build (StyleCop only runs in Debug):
| Project | Total (Debug) | of which `SA*` | `CS`/nullable |
|---|---|---|---|
| Tests | 978 | 190 | ~788 |
| Authentication (host) | 532 | 26 | ~506 |
| Core | 410 | 0 | 410 |
| Integration | 60 | 0 | 60 |
| SystemIntegrationTests | 32 | 0 | 32 |
| Persistance | 0 | 0 | 0 ✅ locked |
Only **Tests (190)** and **host (26)** have StyleCop violations; the rest are SA-clean. The SA violations are almost entirely trivial formatting — SA1005 (122, comment must start with a space), SA1512/SA1001/SA1400/SA1508 and misc spacing — all bulk-fixable with `dotnet format`/IDE code-fixes.
**Decision: approach (a) — fix StyleCop per project** (not (b) scoped-`CS`-codes enforcement). SA is small (216), confined to 2 projects, and auto-fixable; keeping projects genuinely clean is the better end state and matches Persistance. For Tests/host, do a one-time `dotnet format` SA sweep (reviewable as "formatting only") before/with the CS/nullable pass, then flip `TreatWarningsAsErrors`.
**Ratchet order (leaf → root):** Persistance ✅ → **Integration (60)** → SystemIntegrationTests (32) → Core (410) → host (532) → Tests (978).
Contributor guide
No contributing guide indexed for this repository
Research direction
Start by running `dotnet build Altinn.Platform.Authentication.sln` in both Debug and Release, then use the per-project warning tables to identify the next cleanup area. Read the root `Directory.Build.props` and the CI `Build and Test` job as they are updated. Done means zero solution warnings, nullable enabled everywhere, and CI failing on newly introduced warnings.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- build-system, ci-cd
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100