agoda-com / agoda-com/Agoda.IoC

Proposal: Your DI container should fail your build, not your deploy

未關閉
#35 1 則留言 0 個 reaction 已指派 0 人 在 GitHub 檢視
主要語言
C#
星號
38
分支
10
PR 合併指標
30 天內沒有已合併 PR

描述

# Proposal: Your DI container should fail your build, not your deploy

## Start with the developer, not the feature

Before talking about analyzers or diagnostics, let's talk about what actually happens to a developer working in a codebase that uses Agoda.IoC today.

They add a constructor parameter. The code compiles. The tests that don't touch that path pass. The PR gets approved. It ships. Then — at startup, or worse, on the first request that hits a lazy resolution path — the application throws:

```
System.InvalidOperationException: Unable to resolve service for type
'IWidgetPricingClient' while attempting to activate 'WidgetService'.
```

The feedback loop for this mistake is measured in **minutes to hours** (build → deploy → observe), when the mistake itself was made in **seconds** and was fully knowable the moment the developer typed it. Every piece of information needed to catch it was sitting in the source code: the attributes on the classes, the constructor signatures. We just don't look at it until runtime.

This is exactly the class of problem Agoda.IoC was created to solve. The original README says it well: developers shouldn't have to dig through giant configuration classes to understand a class's lifecycle. We moved the *declaration* of DI to the class where it belongs. This proposal is the natural next step: move the *verification* of DI to the build, where it belongs.

## The problems, in order of pain

These are the failure modes developers actually hit, roughly ordered by how much they hurt:

### 1. The captive dependency (the silent one)

A `[RegisterSingleton]` service takes a `[RegisterScoped]` dependency in its constructor. Nothing throws. The scoped instance — often a DbContext or per-request context object — gets captured by the singleton at first resolution and lives forever. The symptoms show up far from the cause: stale data, cross-request state bleed, connection exhaustion under load. This is the DI bug that costs the most engineering hours to diagnose, and it is **100% detectable from lifetime metadata alone**. We have that metadata. It's in our attributes.

MS.DI only catches this with `ValidateScopes`, which is enabled by default **only in the Development environment** — precisely where the load patterns that expose the symptom don't exist.

### 2. The missing registration (the loud but late one)

The scenario above: a constructor asks for something nobody registered. Loud, but late. The cost isn't the fix (it's a one-line attribute) — it's the round-trip time and the erosion of trust in refactoring. Developers who've been burned start treating constructor changes as risky, which is exactly the fear-driven behavior we're trying to engineer out of our culture.

### 3. The silent last-wins overwrite

Two classes register against the same interface, neither with `ReplaceService = true` or a `Key`. One of them wins. The developer who wrote the losing registration has no idea their class is never constructed. This frequently survives for months until someone wonders why their bug fix "didn't take."

### 4. Attribute misuse that reads fine and fails at runtime

A `Factory` type that doesn't implement `IImplementationFactory` for the right `T`. A `Mock` type that doesn't implement the registered interface. A `For` type the class doesn't actually implement. Duplicate `Order` values in an `OfCollection` group. All of these are trivially checkable, and all of them currently fail at runtime with errors that don't point at the attribute that caused them.

### 5. The circular dependency with the opaque error

MS.DI detects cycles at first resolution and throws a stack-dive of an exception. A build-time diagnostic that simply names the cycle path (`A → B → C → A`) turns a debugging session into a squiggle.

## What we're proposing

Add a **Roslyn `DiagnosticAnalyzer`** to the existing `Agoda.IoC.Generator` package that validates the dependency graph at compile time.

No new package, no new install step, no adoption campaign. Generators and analyzers ship in the same `analyzers/dotnet/cs` folder of a nupkg — everyone already referencing `Agoda.IoC.Generator` gets graph verification on their next version bump. (A standalone `Agoda.IoC.Analyzers` package for users of the reflection-based `Agoda.IoC.NetCore` is a reasonable follow-up, but shouldn't gate v1.)

### Credit where it's due

This idea is stolen with pride from **[[Koin Annotations](https://insert-koin.io/docs/reference/koin-annotations/start)](https://insert-koin.io/docs/reference/koin-annotations/start)** in the Kotlin ecosystem, which offers a `KOIN_CONFIG_CHECK` compile-safety option: its KSP processor validates at build time that every declared dependency is satisfiable, and fails the compilation if not. Koin proved both the value and the practical shape of this feature — including the crucial insight that verification must be *pragmatic*, with an explicit escape hatch (`@Provided`) for dependencies supplied outside the annotation system. We should also acknowledge **Jab**, **StrongInject**, and **Dagger**, which demonstrate full compile-time graph verification for pure codegen containers, and **SimpleInjector's `Verify()`** for the runtime-at-startup variant.

What none of them offer — and what makes this worth building — is the middle position Agoda.IoC occupies: *attribute-driven registration over vanilla MS.DI, with build-time verification as a lint rather than a lock-in*. After the generator runs, our output is still plain `IServiceCollection` calls. This proposal keeps that property intact: the analyzer adds safety without adding runtime presence.

## Proposed diagnostics

| ID | Severity (default) | What it catches |
|----|-------------------|-----------------|
| `AGIOC001` | Warning | Constructor parameter of a registered type is not resolvable from any registration, the framework allowlist, or an `[ExternallyProvided]` declaration |
| `AGIOC002` | Warning | Captive dependency: longer-lived registration depends on shorter-lived one (singleton → scoped/per-request, singleton → transient-with-state is debatable and out of scope for v1) |
| `AGIOC003` | Warning | Multiple non-collection registrations for the same service type without `ReplaceService = true` or distinct `Key`s |
| `AGIOC004` | Warning | `Factory` type does not implement `IImplementationFactory` for the registered service type |
| `AGIOC005` | Warning | `Mock` type or `For` type is incompatible with the annotated class; duplicate `Order` within an `OfCollection` group |
| `AGIOC006` | Warning | Circular constructor dependency among registered types (diagnostic message names the full cycle path) |

All default to **Warning**. Teams opt into build-breaking behavior per-repo via `.editorconfig`:

```ini
dotnet_diagnostic.AGIOC001.severity = error
dotnet_diagnostic.AGIOC002.severity = error
```

This mirrors Koin's decision to make `KOIN_CONFIG_CHECK` opt-in, and it matters: a verification feature that produces false-positive build failures on day one gets disabled on day two and never re-enabled. Warnings that developers *choose* to promote to errors build trust in the opposite direction.

## "Doesn't `ValidateOnBuild` already do this?"

Partially — and the proposal is stronger for being precise about the overlap. MS.DI ships two validation mechanisms, both enabled **only in the Development environment** by the default host builder:

- **`ValidateOnBuild`** walks every registration at container build and throws if a constructor can't be satisfied. This overlaps with `AGIOC001` — but it skips factory registrations (lambdas are opaque, including everything registered via `IImplementationFactory`), skips open generics, requires actually building the host (F5 or a `WebApplicationFactory` test — a loop that doesn't exist at all for class library projects), and says nothing about lifetimes.
- **`ValidateScopes`** catches captive dependencies (`AGIOC002`) — but only at **resolution time**, on the specific code path that triggers it. A captive dependency on a rarely-exercised path passes local testing and reaches production, where the check is off by default.

Neither mechanism touches `AGIOC003` (last-wins overwrites are *legal* MS.DI behavior), `AGIOC004`/`AGIOC005` (attribute semantics MS.DI knows nothing about), and cycles (`AGIOC006`) surface only on first resolution with an unhelpful error.

The two approaches are complementary, and the recommendation should say so explicitly: **enable `ValidateOnBuild` and `ValidateScopes` in all environments** (they're cheap) *and* add the analyzer. The runtime checks are the safety net; the analyzer is the shorter loop — a red squiggle while typing and a failed `dotnet build` in CI, with no host required.

## Being honest about what this can and cannot verify

An analyzer can only see attribute-driven registrations. Real applications also register services through `AddHttpClient()`, `AddControllers()`, `AddOptions()`, third-party extension methods, and a handful of manual lines in `Program.cs`. **Complete verification is impossible; useful verification is very achievable.** The design has to embrace that honestly rather than pretend otherwise:

1. **Built-in framework allowlist** — types assumed resolvable without registration: `ILogger`, `IOptions` / `IOptionsMonitor` / `IOptionsSnapshot`, `IServiceProvider`, `IServiceScopeFactory`, `IHttpClientFactory`, `IConfiguration`, `IHostEnvironment` / `IWebHostEnvironment`, `IMemoryCache`, `Lazy`, and `IEnumerable` / `IReadOnlyList` where `T` has at least one registration.

2. **An assembly-level escape hatch** for everything else — the direct equivalent of Koin's `@Provided`:

```csharp
[assembly: ExternallyProvided(typeof(IFeatureFlagClient))]
[assembly: ExternallyProvided(typeof(IMessageBus))]
```

This doubles as living documentation: it makes the manually-registered surface of an application *explicit and reviewable*, which is a small win on its own.

3. **Cross-assembly awareness.** Attributes live in assembly metadata, so the analyzer can see `[RegisterSingleton]` types in referenced class libraries via metadata symbols — validating the *composed* graph across a solution, matching the multi-assembly `AutoWireAssembly(new[]{...})` reality of real services.

Note that `AGIOC002` (captive dependency) and `AGIOC003`–`AGIOC006` don't suffer from the "unknown registrations" problem at all — they're computed purely from information the attributes fully own. If we shipped *only* AGIOC002, the proposal would already pay for itself.

## Implementation sketch

Compilation-level analyzer, because graph properties are global:

```csharp
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public class IoCGraphAnalyzer : DiagnosticAnalyzer
{
public override void Initialize(AnalysisContext context)
{
context.EnableConcurrentExecution();
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);

context.RegisterCompilationStartAction(start =>
{
var registrations = new ConcurrentBag();

// Pass 1: collect [Register*]-attributed types,
// including from referenced assemblies via metadata symbols
start.RegisterSymbolAction(
ctx => CollectIfRegistered(ctx, registrations),
SymbolKind.NamedType);

// Pass 2: whole-graph validation once collection completes
start.RegisterCompilationEndAction(
ctx => ValidateGraph(ctx, BuildServiceMap(registrations)));
});
}
}
```

One Roslyn behavior worth designing around up front: `CompilationEndAction` diagnostics don't surface during live typing in the IDE unless full-solution analysis is enabled — they reliably appear on **build** and in **CI**. That's acceptable (it matches Koin, whose KSP check is also build-time) but it shapes the architecture: the *local* rules (`AGIOC004`, `AGIOC005`) should be ordinary symbol actions so they squiggle live in the editor, and only the whole-graph rules (`AGIOC001`, `AGIOC002`, `AGIOC003`, `AGIOC006`) live in compilation-end.

Testing uses `Microsoft.CodeAnalysis.Testing`, which makes analyzer test suites declarative and pleasant — every diagnostic gets positive cases, negative cases, and escape-hatch cases.

## Rollout plan

1. **Phase 1 — the certain rules.** Ship `AGIOC002`–`AGIOC005` as warnings. Zero false positives possible; immediate value; builds trust.
2. **Phase 2 — the graph rules.** Ship `AGIOC001` and `AGIOC006` as warnings, with the framework allowlist and `[ExternallyProvided]`. Dogfood on a couple of real internal services first, because the allowlist will accrete cases from real-world usage (open generics matching, keyed resolution patterns) and it's better to harden it before external users hit the gaps.
3. **Phase 3 — documentation and the error story.** Document the `.editorconfig` promotion path and recommend it as the end state for services: DI misconfiguration should fail the build in CI, not the health check in production.

## What success looks like

Not "we shipped an analyzer." Success is:

- A developer adds a constructor parameter for an unregistered service and finds out **from a red squiggle**, before the PR even exists.
- A captive dependency becomes a build warning instead of a week-long production investigation.
- Constructor refactoring stops being scary, because the safety net is at compile time.
- The manually-registered surface of every service is explicit, reviewable, and honest.

The measure of a DevEx improvement isn't feature count — it's how many minutes of feedback loop we delete and how much fear we remove from ordinary changes. This deletes the worst feedback loop the library currently has.

## Open questions for discussion

1. Should `AGIOC002` treat singleton → transient as a captive dependency? (Technically it is; practically most transients are stateless and this would be noisy. Proposal: out of scope for v1, revisit with an opt-in strictness knob.)
2. Should the framework allowlist be extensible via config (e.g., `.editorconfig` keys or an MSBuild property) in addition to `[ExternallyProvided]`? Useful for org-wide shared conventions.
3. Do we want a standalone `Agoda.IoC.Analyzers` package for `Agoda.IoC.NetCore` (reflection-mode) users, or is analyzer coverage a carrot to migrate them to the Generator?
4. Keyed registrations: `IKeyedComponentFactory` resolution is a runtime lookup by string — should `AGIOC001` attempt to verify key existence where the key is a constant, or treat keyed resolution as always-satisfied in v1?

---

*Inspiration: [[Koin Annotations](https://insert-koin.io/docs/reference/koin-annotations/start)](https://insert-koin.io/docs/reference/koin-annotations/start) (`KOIN_CONFIG_CHECK`) by the Koin team — the proof that annotation-driven DI and build-time verification belong together. Prior art consulted: Jab, StrongInject, Dagger, SimpleInjector `Verify()`, MS.DI `ValidateOnBuild`/`ValidateScopes`.*

貢獻指南

開啟貢獻指南

評估

這個 Issue 還沒有評估資料。

把新 issue 寄到你的電子郵件信箱

精選適合新手參與的 GitHub issue 摘要。