Azure / Azure/Connectors-NET-SDK
Enable CA1062 (validate arguments of public methods) to catch missing null-argument guards at build time
- Dominant language
- C#
- Stars
- 3
- Forks
- 5
- Avg merge
- 1d 15h
- Merged PRs (30d)
- 7
Description
## Summary
Enable **CA1062** ("Validate arguments of externally visible methods") as a build-enforced analyzer rule so missing null-argument guards on public/protected APIs are caught deterministically at build time instead of ad hoc during code review.
## Motivation
Missing `ArgumentNullException` guards on public entry points are a **recurring manual review nit** (including from the Copilot PR reviewer). Each occurrence currently costs a review round-trip: reviewer spots it, author adds `ArgumentNullException.ThrowIfNull(...)`, re-review. CA1062 catches the entire class at compile time, consistently, before a PR is ever opened.
This rule is **disabled by default** in the SDK's current analysis mode (`AnalysisMode=Default`), so it never fires today even though `EnableNETAnalyzers=true` and `TreatWarningsAsErrors=true` are set in `eng/build/Engineering.props`.
## Evidence (from a local `AnalysisMode=All` build)
CA1062 fires on hand-written production code that skips argument validation, e.g. in `ConnectorClientBase`:
- `CallConnectorAsync(HttpMethod method, ...)` — `method` not validated before use.
- `CallConnectorAsync(HttpMethod method, ...)` (no-body overload) — same.
New code such as `ConnectorTriggerPayload` already validates via `ArgumentNullException.ThrowIfNull`, so it is clean — which shows the rule aligns with the direction the codebase is already moving.
## Scope / considerations
- **Generated code is in scope.** The generated clients are produced by our own CodefulSdkGenerator (BPM repo) and are a core part of the value proposition; they must not ship missing guards. Enabling CA1062 will surface any generated entry points lacking validation — if it flags generated code, the fix belongs in the generator's emit templates, not a per-file suppression.
- **Severity:** start as `warning` (which `TreatWarningsAsErrors` escalates to an error) or `suggestion` for a soft rollout, then promote.
- **Rollout:** enable via `.editorconfig` (`dotnet_diagnostic.CA1062.severity = warning`) rather than raising the whole `AnalysisMode`, to keep the change targeted and avoid the ~17k unrelated diagnostics that `AnalysisMode=All` produces.
- **Note:** CA1062 has known limitations (it may not track validation performed in a called helper), so expect a few false positives to suppress with justification.
## Acceptance criteria
- [ ] `dotnet_diagnostic.CA1062.severity` set in `.editorconfig`.
- [ ] Existing hand-written violations fixed (add `ArgumentNullException.ThrowIfNull`).
- [ ] Generated-code violations (if any) fixed at the generator, or a documented, justified suppression policy agreed.
- [ ] Build stays green with the rule enforced.
_Filed from analysis on PR #196; not part of that PR's scope._
Contributor guide
Assessment
This issue has not been assessed yet.