Altinn / Altinn/altinn-authentication

Cleanup: remove compiler warnings, enable nullable for all projects and prevent new warnings from being added

Open
#488 5 comments 0 reactions 0 assignees View on GitHub
Etter19JuniRelease status/draft
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.