From 18909491542a1ed3c2dfa7702ca85f7e57ef2e65 Mon Sep 17 00:00:00 2001 From: Keith Voels Date: Sat, 21 Mar 2026 15:52:30 -0500 Subject: [PATCH 1/2] fix: factory fetch NRE when [Fetch] returns false with [AuthorizeFactory] Generated factory Fetch methods threw NullReferenceException when the underlying [Fetch] method returned false (entity not found) and the factory had [AuthorizeFactory]. The bool-false path emitted `return default!` which is null for Authorized, then the public wrapper dereferenced .Result on the null reference. Fixed by emitting `new Authorized()` instead of `default!` when HasAuth is true, at both the read-style and write-style local method sites. This returns a non-null wrapper with HasAccess=false and Result=null, so .Result returns null correctly. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../fix-factory-fetch-nre-on-not-found.md | 548 ++++++++++++++++++ .../architect.md | 81 +++ .../developer.md | 58 ++ .../requirements-reviewer.md | 54 ++ .../fix-factory-fetch-nre-on-not-found.md | 142 +++++ .../fix-factory-fetch-nre-on-not-found.md | 88 --- .../Renderer/ClassFactoryRenderer.cs | 46 +- .../RemoteFetchRoundTripTests.cs | 68 +++ .../FactoryRoundTrip/RoundTripTargets.cs | 79 +++ 9 files changed, 1070 insertions(+), 94 deletions(-) create mode 100644 docs/plans/completed/fix-factory-fetch-nre-on-not-found.md create mode 100644 docs/plans/completed/fix-factory-fetch-nre-on-not-found.memory/architect.md create mode 100644 docs/plans/completed/fix-factory-fetch-nre-on-not-found.memory/developer.md create mode 100644 docs/plans/completed/fix-factory-fetch-nre-on-not-found.memory/requirements-reviewer.md create mode 100644 docs/todos/completed/fix-factory-fetch-nre-on-not-found.md delete mode 100644 docs/todos/fix-factory-fetch-nre-on-not-found.md diff --git a/docs/plans/completed/fix-factory-fetch-nre-on-not-found.md b/docs/plans/completed/fix-factory-fetch-nre-on-not-found.md new file mode 100644 index 0000000..a0e2747 --- /dev/null +++ b/docs/plans/completed/fix-factory-fetch-nre-on-not-found.md @@ -0,0 +1,548 @@ +# Fix Factory Fetch NRE When Entity Not Found + +**Date:** 2026-03-21 +**Related Todo:** [Fix Factory Fetch NRE When Entity Not Found](../todos/fix-factory-fetch-nre-on-not-found.md) +**Status:** Complete +**Last Updated:** 2026-03-21 + +--- + +## Overview + +Generated factory methods throw `NullReferenceException` when a `[Fetch]` method returns `false` (entity not found) and the factory has authorization (`[AuthorizeFactory]` or `[AspAuthorize]`). The bug is in two locations in `ClassFactoryRenderer.cs` where the bool-false path emits `return default!;`. When `HasAuth` is true, the local method's return type is `Authorized`, so `default!` evaluates to `null`. The public wrapper then dereferences `.Result` (read-style) or `.HasAccess` (write-style) on the null reference, causing NRE. + +The fix changes the `default!` return on the bool-false path to `new Authorized(default)` when `HasAuth` is true, consistent with how the success path and save routing already construct `Authorized` wrappers. + +--- + +## Business Requirements Context + +**Source:** [Todo Requirements Review](../todos/fix-factory-fetch-nre-on-not-found.md#requirements-review) + +### Relevant Existing Requirements + +#### Behavioral Contracts + +- **Bool-return Fetch contract** (`skills/RemoteFactory/references/factory-operations.md:150-166`): "Returning `bool` signals RemoteFactory: `true` means 'this instance is populated, return it to the caller,' `false` means 'the data wasn't found, discard this instance and return null.'" The bug violates this contract when authorization is present -- the factory throws NRE instead of returning null. + +- **Auth failure behavior** (`src/Design/Design.Domain/Aggregates/AuthorizedOrder.cs:54-58`, `skills/RemoteFactory/references/authorization.md:281`): "Create/Fetch return null when authorization fails." This establishes that the public Fetch wrapper must handle null `Authorized.Result`. The bool-false path is a different scenario (not-found rather than auth-denied), but must produce the same consumer-visible result: null. + +- **Authorized(T?) constructor semantics** (`src/RemoteFactory/Authorized.cs:91-102`): `new Authorized(default)` sets `Result = default` (null for reference types) and `HasAccess = false`. This is the correct representation for "not found" within the `Authorized` wrapper -- the wrapper exists (non-null), but signals no result. + +#### Existing Tests + +- **`RemoteFetchRoundTripTests.cs:53-61`** (`RemoteFetch_BoolFalse_ReturnsNull`): Asserts `Assert.Null(result)` when Fetch returns false. Passes because the target lacks `[AuthorizeFactory]`. Not affected by this fix. + +- **`RemoteFetchRoundTripTests.cs:84-92`** (`RemoteFetch_BoolFalse_ServerAlsoReturnsNull`): Same assertion from server container. Not affected. + +- **`RemoteFetchRoundTripTests.cs:100-109`** (`RemoteFetch_Remote_BoolFalse_ReturnsNull`): Same assertion through remote transport with `[Service]` injection. Not affected. + +- **`RemoteFetchRoundTripTests.cs:68-78`** (`RemoteFetch_BoolTrue_ReturnsObject`): Verifies happy path (bool true). Not affected. + +### Gaps + +1. **No test coverage for bool-false + authorization intersection.** The existing `RemoteFetchTarget_BoolFalse` targets lack `[AuthorizeFactory]`. No tests exercise the combination of `Task` Fetch + authorization. This is the exact scenario that triggers the NRE. + +2. **No Design project example of bool-return Fetch.** The Design project's `Order.cs` and `AuthorizedOrder.cs` both use void-returning Fetch. CLAUDE-DESIGN.md mentions `Task Fetch` in Critical Rules but no Design.Domain example demonstrates it. This gap is noted but out of scope for this bug fix. + +3. **Save with bool-return + authorization.** The write-style path has the same `default!` pattern (`ClassFactoryRenderer.cs:877-894`). If any entity combines `[Insert]`/`[Update]`/`[Delete]` returning `bool` with `[AuthorizeFactory]`, the same NRE would occur on Save. The fix must address both paths. + +### Contradictions + +None. The todo proposes fixing a bug that violates the documented bool-return contract. + +### Recommendations for Architect + +The reviewer recommends the `new Authorized(default)` approach over null-checking in the public wrapper. This preserves semantic meaning: the `Authorized` wrapper exists but indicates no result, consistent with how authorization failure already works. The null-check approach would silently swallow nulls that might indicate different bugs in the future. + +--- + +## Business Rules (Testable Assertions) + +1. WHEN a `[Fetch]` method returns `false` AND the factory has `[AuthorizeFactory]`, THEN the public Fetch method RETURNS `null` (not NRE). -- Source: Bool-return Fetch contract + Auth failure behavior + +2. WHEN a `[Fetch]` method returns `false` AND the factory has `[AuthorizeFactory]`, THEN the local method RETURNS `new Authorized(default)` (a non-null wrapper with null Result). -- Source: NEW (implementation detail ensuring Rule 1) + +3. WHEN a `[Fetch]` method returns `true` AND the factory has `[AuthorizeFactory]`, THEN the public Fetch method RETURNS the fetched entity (existing behavior, must not regress). -- Source: existing `RemoteFetch_BoolTrue_ReturnsObject` test + +4. WHEN a `[Fetch]` method returns `false` AND the factory has NO authorization, THEN the public Fetch method RETURNS `null` (existing behavior, must not regress). -- Source: existing `RemoteFetch_BoolFalse_ReturnsNull` test + +5. WHEN a write-style method (`[Insert]`/`[Update]`/`[Delete]`) returns `false` AND the factory has `[AuthorizeFactory]`, THEN the local write method RETURNS `new Authorized(default)` (not `default!`). -- Source: NEW (same pattern as Rule 2, applied to write-style path) + +6. WHEN a `[Fetch]` method returns `false` AND the factory has `[AuthorizeFactory]` AND the request crosses the client/server boundary, THEN the client receives `null` (serialization round-trip). -- Source: NEW (extends Rule 1 across the remote transport) + +### Test Scenarios + +| # | Scenario | Inputs / State | Rule(s) | Expected Result | +|---|----------|---------------|---------|-----------------| +| 1 | Auth + bool-false Fetch (local) | `[AuthorizeFactory]` target, Fetch returns `false`, server container | 1, 2 | `result` is `null` (no NRE) | +| 2 | Auth + bool-false Fetch (remote round-trip) | `[AuthorizeFactory]` target, Fetch returns `false`, client container | 1, 2, 6 | `result` is `null` (no NRE), round-trip succeeds | +| 3 | Auth + bool-true Fetch (no regression) | `[AuthorizeFactory]` target, Fetch returns `true`, client container | 3 | `result` is non-null with expected state | +| 4 | No-auth + bool-false Fetch (no regression) | No auth, Fetch returns `false`, client container | 4 | `result` is `null` (existing tests still pass) | +| 5 | Auth + bool-false Fetch with [Service] (remote) | `[AuthorizeFactory]` target with `[Service]` param, Fetch returns `false`, client container | 1, 2, 6 | `result` is `null`, service was injected on server | + +--- + +## Approach + +Change the generated code in the bool-false early-return path to emit `new Authorized(default)` instead of `default!` when `HasAuth` is true. This is a minimal, targeted fix at the two emission sites in `ClassFactoryRenderer.cs`: + +1. **Read-style path** (line ~450-470): The `IsBool` check inside `RenderReadLocalMethod` +2. **Write-style path** (line ~877-894): The `IsBool` check inside `RenderWriteLocalMethod` + +Both sites share the same pattern: + +``` +// Current (buggy): +return default!; // null for Authorized + +// Fixed: +return new Authorized(default); // non-null wrapper with null Result +``` + +The `Task.FromResult` wrapping (non-async case) also needs the same treatment. + +The public wrappers (`RenderPublicMethod` lines 295-320, `RenderWritePublicMethod` lines 734-757) do NOT need changes. Once the local method returns a non-null `Authorized`, the `.Result` access works correctly and returns null (the `Authorized.Result` is `default(T)` which is `null` for reference types). + +The save public wrapper (`RenderSavePublicMethod` lines 983-1029) is also safe: it checks `authorized.HasAccess` before `.Result`, and `new Authorized(default)` has `HasAccess = false`, so it will throw `NotAuthorizedException`. This is actually the correct behavior for save -- if a write operation's domain method returns `false`, it means the operation failed, and the save should not silently return null. + +**Note on save semantics:** For save operations, the bool-false path has different semantics than for fetch. A fetch returning false means "not found, return null." A save/insert/update/delete returning false is less common and arguably means "operation declined." The `Authorized(default)` wrapper with `HasAccess = false` will cause the save public wrapper to throw `NotAuthorizedException`, which may not be ideal. However, this is a pre-existing semantic question and fixing it is out of scope for this bug. The immediate fix prevents the NRE, which is the blocking issue. + +--- + +## Design + +### Bug Location + +Two sites in `src/Generator/Renderer/ClassFactoryRenderer.cs`: + +**Site 1: Read-style local method** (`RenderReadLocalMethod`, lines 450-470) + +```csharp +// Current code (lines 460-468): +if (!needsAsync && method.IsTask) +{ + var nullableServiceType = method.IsNullable ? $"{model.ServiceTypeName}?" : model.ServiceTypeName; + sb.AppendLine($" return Task.FromResult<{nullableServiceType}>(default)!;"); +} +else +{ + sb.AppendLine(" return default!;"); +} +``` + +When `HasAuth` is true, the local method's return type is `Authorized` (or `Task>`). The `default!` evaluates to `null` for this reference type. + +**Site 2: Write-style local method** (`RenderWriteLocalMethod`, lines 877-894) + +Identical pattern to Site 1. + +### Fix Design + +At both sites, add a conditional: when `method.HasAuth` is true, emit `new Authorized<{model.ServiceTypeName}>(default)` instead of `default!`. This mirrors the pattern already used in `RenderSaveLocalMethod` at line 1052-1054 for the save routing default return: + +```csharp +var defaultReturn = method.HasAuth + ? $"new Authorized<{model.ServiceTypeName}>()" + : $"default({model.ServiceTypeName})"; +``` + +The exact fix for both sites: + +```csharp +if (method.HasAuth) +{ + var authDefault = $"new Authorized<{model.ServiceTypeName}>(default)"; + if (!needsAsync && method.IsTask) + { + sb.AppendLine($" return Task.FromResult({authDefault});"); + } + else + { + sb.AppendLine($" return {authDefault};"); + } +} +else if (!needsAsync && method.IsTask) +{ + var nullableServiceType = method.IsNullable ? $"{model.ServiceTypeName}?" : model.ServiceTypeName; + sb.AppendLine($" return Task.FromResult<{nullableServiceType}>(default)!;"); +} +else +{ + sb.AppendLine(" return default!;"); +} +``` + +Note: We use `new Authorized(default)` (with `default` argument), not `new Authorized()` (parameterless). The parameterless constructor is the `[JsonConstructor]` that leaves `Result` unset. The `Authorized(T?)` constructor explicitly sets `Result = default`, `HasAccess = false` when result is null. Both produce the same runtime state for reference types, but `new Authorized(default)` is more explicit about intent. + +### Files Changed + +| File | Change | +|------|--------| +| `src/Generator/Renderer/ClassFactoryRenderer.cs` | Fix both bool-false return sites (read-style ~line 460, write-style ~line 885) | +| `src/Tests/RemoteFactory.IntegrationTests/TestTargets/FactoryRoundTrip/RoundTripTargets.cs` | Add new test targets: `RemoteFetchTarget_AuthBoolFalse`, `RemoteFetchTarget_AuthBoolTrue`, `RemoteFetchTarget_RemoteAuthBoolFalse` | +| `src/Tests/RemoteFactory.IntegrationTests/FactoryRoundTrip/RemoteFetchRoundTripTests.cs` | Add new tests for scenarios 1-3 and 5 | + +### Test Target Design + +**Authorization class** (reuse or add alongside existing targets): + +```csharp +public class FetchAuthAllow +{ + [AuthorizeFactory(AuthorizeFactoryOperation.Read)] + public bool CanFetch() => true; // Always allows -- we're testing the not-found path, not auth denial +} +``` + +**Test target for auth + bool-false Fetch:** + +```csharp +[Factory] +[AuthorizeFactory] +public partial class RemoteFetchTarget_AuthBoolFalse +{ + public bool FetchCalled { get; set; } + public int ReceivedId { get; set; } + + [Fetch] + [Remote] + internal Task Fetch(int id) + { + FetchCalled = true; + ReceivedId = id; + return Task.FromResult(false); + } +} +``` + +**Test target for auth + bool-true Fetch** (regression guard): + +```csharp +[Factory] +[AuthorizeFactory] +public partial class RemoteFetchTarget_AuthBoolTrue +{ + public bool FetchCalled { get; set; } + public int ReceivedId { get; set; } + + [Fetch] + [Remote] + internal Task Fetch(int id) + { + FetchCalled = true; + ReceivedId = id; + return Task.FromResult(true); + } +} +``` + +**Test target for auth + bool-false Fetch with [Service]** (proves server execution): + +```csharp +[Factory] +[AuthorizeFactory] +public partial class RemoteFetchTarget_RemoteAuthBoolFalse +{ + public bool FetchCalled { get; set; } + public int ReceivedId { get; set; } + public bool ServiceWasInjected { get; set; } + + [Remote] + [Fetch] + internal Task Fetch(int id, [Service] IServerOnlyService service) + { + FetchCalled = true; + ReceivedId = id; + ServiceWasInjected = service != null; + return Task.FromResult(false); + } +} +``` + +--- + +## Implementation Steps + +1. **Fix read-style bool-false return** in `ClassFactoryRenderer.cs` (`RenderReadLocalMethod`, lines ~450-470): Add `HasAuth` conditional before the existing `default!` emission to emit `new Authorized(default)` instead. + +2. **Fix write-style bool-false return** in `ClassFactoryRenderer.cs` (`RenderWriteLocalMethod`, lines ~877-894): Apply the identical fix as step 1. + +3. **Add auth class and test targets** in `RoundTripTargets.cs`: Add `FetchAuthAllow` class and three new test targets (`RemoteFetchTarget_AuthBoolFalse`, `RemoteFetchTarget_AuthBoolTrue`, `RemoteFetchTarget_RemoteAuthBoolFalse`). + +4. **Add integration tests** in `RemoteFetchRoundTripTests.cs`: Add tests for scenarios 1, 2, 3, and 5 from the Test Scenarios table. + +5. **Build and run all tests**: Verify the new tests pass and all existing tests remain green. + +--- + +## Acceptance Criteria + +- [ ] `RemoteFetchTarget_AuthBoolFalse` factory Fetch returns `null` (not NRE) from both server and client containers +- [ ] `RemoteFetchTarget_AuthBoolTrue` factory Fetch returns the populated entity (no regression) +- [ ] `RemoteFetchTarget_RemoteAuthBoolFalse` factory Fetch returns `null` through remote transport with service injection +- [ ] All existing `RemoteFetchRoundTripTests` continue to pass (no regression in non-auth bool-false/bool-true paths) +- [ ] Full solution builds without errors: `dotnet build src/Neatoo.RemoteFactory.sln` +- [ ] All tests pass: `dotnet test src/Neatoo.RemoteFactory.sln` + +--- + +## Dependencies + +None. This is a self-contained bug fix in the generator renderer and test project. + +--- + +## Risks / Considerations + +1. **Save path semantics**: The write-style fix will cause `new Authorized(default)` with `HasAccess = false` to flow through the save public wrapper, which throws `NotAuthorizedException`. This may not be the ideal behavior for "save operation returned false," but it prevents NRE and the save-with-bool-return + auth combination is untested and likely unused. A future todo can address save-specific bool-false semantics if needed. + +2. **Generated code caching**: Since this changes the generator output, any project using RemoteFactory will need a rebuild to pick up the fix. The generated code in `obj/` directories is not checked in, so this is standard behavior. + +3. **Authorized constructor choice**: `new Authorized(default)` vs `new Authorized()` -- both produce `HasAccess = false, Result = null` for reference types. We use the `(T?)` constructor overload because it explicitly sets `Result`, which is clearer about intent. The `[JsonConstructor]` parameterless constructor could theoretically have different init behavior in the future. + +--- + +## Architectural Verification + +**Scope Table:** + +| Pattern | Affected? | Current Behavior | After Fix | +|---------|-----------|-----------------|-----------| +| Read-style Fetch + auth + bool-false | YES (bug) | NRE on `.Result` | Returns `null` | +| Read-style Fetch + auth + bool-true | No | Returns entity | Unchanged | +| Read-style Fetch + no auth + bool-false | No | Returns `null` | Unchanged | +| Write-style Save + auth + bool-false | YES (latent bug) | NRE on `.HasAccess` | Throws `NotAuthorizedException` | +| Write-style Save + no auth + bool-false | No | Returns `default!` | Unchanged | + +**Verification Evidence:** + +- Read-style bug site: `src/Generator/Renderer/ClassFactoryRenderer.cs:460-468` -- confirmed `default!` emitted when `HasAuth` true +- Write-style bug site: `src/Generator/Renderer/ClassFactoryRenderer.cs:885-893` -- confirmed identical pattern +- Public wrapper NRE site: `src/Generator/Renderer/ClassFactoryRenderer.cs:315` -- `.Result` dereference on potentially null `Authorized` +- Save wrapper NRE site: `src/Generator/Renderer/ClassFactoryRenderer.cs:1009` -- `.HasAccess` dereference on potentially null `Authorized` +- `Authorized(T?)` constructor: `src/RemoteFactory/Authorized.cs:91-102` -- `HasAccess = false` when `result == null` +- Existing non-auth tests pass: 3 tests in `RemoteFetchRoundTripTests.cs` cover bool-false without auth +- Save routing default already uses `new Authorized()`: `ClassFactoryRenderer.cs:1052-1053` + +**Breaking Changes:** No. The current behavior is an NRE crash. The fix replaces a crash with the contractually correct return value (`null`). + +**Codebase Analysis:** + +- `ClassFactoryRenderer.cs` is the single file containing the bug (two sites) +- The fix pattern (`new Authorized(...)`) is already used elsewhere in the same file (line 515, 921, 1053, 1147) +- No other renderers emit bool-check code +- The `Authorized` class is stable and the `(T?)` constructor is well-defined + +--- + +## Agent Phasing + +| Phase | Agent Type | Fresh Agent? | Rationale | Dependencies | +|-------|-----------|-------------|-----------|--------------| +| Phase 1: Fix + Tests | developer | Yes | Single focused phase: fix two lines in renderer, add test targets and tests, build and verify | None | + +**Parallelizable phases:** None -- single phase. + +**Notes:** This is a small, focused bug fix touching one production file and two test files. A single developer agent phase is sufficient. + +--- + +## Documentation + +**Agent:** developer (no documentation agent needed) +**Completed:** [pending] + +### Expected Deliverables + +- [ ] No skill/doc updates needed -- this is a bug fix restoring existing documented behavior +- [ ] Skill updates: N/A +- [ ] Sample updates: N/A + +--- + +## Developer Review + +**Status:** Approved +**Reviewed:** 2026-03-21 + +### Assertion Trace Verification + +| Rule # | Implementation Path (method/condition) | Expected Result | Matches Rule? | Notes | +|--------|---------------------------------------|-----------------|---------------|-------| +| 1 | `RenderReadLocalMethod` (line 342): `method.IsBool && !method.IsConstructor && !method.IsStaticFactory` at line 451 triggers bool check. When `!_succeeded` (line 453), fix adds `method.HasAuth` conditional to emit `new Authorized(default)` instead of `default!`. `RenderPublicMethod` (line 295): `.Result` on the `Authorized` at line 315 returns `null` (since `Result = default = null`). | Public Fetch returns `null` (no NRE) | Yes | `Authorized(T?)` ctor (line 91-102) sets `Result = null`, `HasAccess = false`. `.Result` returns `null`. | +| 2 | Same path as Rule 1: `RenderReadLocalMethod` lines 451-468 with proposed fix -- when `method.HasAuth == true`, emits `new Authorized<{model.ServiceTypeName}>(default)` (async variant: `Task.FromResult(new Authorized(default))`). | Local method returns `new Authorized(default)` (non-null wrapper) | Yes | Directly matches proposed code in Design section. | +| 3 | `RenderReadLocalMethod` (line 342): When `_succeeded == true`, execution continues past bool check (lines 450-470). Success path at lines 513-515: `returnExpr = new Authorized(resultVar)`. `RenderPublicMethod` line 315: `.Result` on non-null `Authorized` returns entity. | Public Fetch returns populated entity | Yes | Fix only changes `!_succeeded` early-return path; success path untouched. | +| 4 | `RenderReadLocalMethod` lines 460-468: When `method.HasAuth == false`, fix falls through to existing `else if` / `else` branches emitting `default!` or `Task.FromResult(default)!`. `RenderPublicMethod` lines 309-311: no auth, returns method target directly. | Public Fetch returns `null` (existing behavior) | Yes | `HasAuth` conditional is new outer branch; existing non-auth code becomes `else` branches. | +| 5 | `RenderLocalMethod(WriteMethodModel)` (line 818): Same `method.IsBool` check at line 866, same `!_succeeded` at line 878, same proposed fix: when `method.HasAuth == true`, emit `new Authorized(default)`. | Write local method returns `new Authorized(default)` (not `default!`) | Yes | Identical pattern to read-style. Save wrapper (line 1009) checks `.HasAccess` which is `false`, throws `NotAuthorizedException`. | +| 6 | Rule 1 fix applied server-side. `MakeSerializedServerStandinDelegateRequest.ForDelegateNullable` serializes server response. Server-side `RenderPublicMethod` line 315 extracts `.Result` (null) from `Authorized`. Null serializes/deserializes correctly. Client receives `null`. | Client receives `null` through round-trip | Yes | `ForDelegateNullable` returns `null` as valid result. Auth unwrapping happens server-side before serialization. | + +### Concerns + +1. **Minor naming inaccuracy (non-blocking):** Plan references `RenderWriteLocalMethod` but actual method name is `RenderLocalMethod(StringBuilder sb, WriteMethodModel method, ClassFactoryModel model)` at line 818. Does not affect implementation -- line numbers and code patterns are correct. + +2. **`RenderWritePublicMethod` reference (non-blocking):** Plan references "lines 734-757" as write-style public wrapper. The actual method at those lines is `RenderClassExecutePublicMethod` (for `[Execute]` operations), not write-style `[Insert]/[Update]/[Delete]`. Does not affect correctness -- the fix at the local method level prevents null from reaching any public wrapper. + +3. **Save path semantics (acknowledged, out of scope):** Write-style bool-false + auth results in `NotAuthorizedException` via save wrapper (line 1009). Correctly scoped as out of scope. + +--- + +## Implementation Contract + +**Created:** 2026-03-21 +**Approved by:** developer agent + +### Verification Acceptance Criteria + +- All new tests pass (scenarios 1-3, 5) +- All existing `RemoteFetchRoundTripTests` pass (no regression) +- Full solution builds: `dotnet build src/Neatoo.RemoteFactory.sln` +- All tests pass: `dotnet test src/Neatoo.RemoteFactory.sln` + +### Test Scenario Mapping + +| Scenario # | Test Method | Notes | +|------------|-------------|-------| +| 1 | `RemoteFetch_AuthBoolFalse_ServerReturnsNull` | Auth + bool-false, server container (local execution) | +| 2 | `RemoteFetch_AuthBoolFalse_ClientReturnsNull` | Auth + bool-false, client container (serialization round-trip) | +| 3 | `RemoteFetch_AuthBoolTrue_ReturnsObject` | Auth + bool-true, client container (regression guard) | +| 4 | (Covered by existing tests) | No-auth + bool-false already tested | +| 5 | `RemoteFetch_RemoteAuthBoolFalse_ReturnsNull` | Auth + bool-false + `[Service]`, client container (proves server execution) | + +### In Scope + +- [ ] `src/Generator/Renderer/ClassFactoryRenderer.cs`: Fix bool-false return at two sites + - Site 1: `RenderReadLocalMethod` lines 460-468 -- add `HasAuth` conditional before existing branches + - Site 2: `RenderLocalMethod(WriteMethodModel)` lines 885-893 -- add identical `HasAuth` conditional +- [ ] `src/Tests/RemoteFactory.IntegrationTests/TestTargets/FactoryRoundTrip/RoundTripTargets.cs`: Add `FetchAuthAllow` auth class and three test targets (`RemoteFetchTarget_AuthBoolFalse`, `RemoteFetchTarget_AuthBoolTrue`, `RemoteFetchTarget_RemoteAuthBoolFalse`) +- [ ] `src/Tests/RemoteFactory.IntegrationTests/FactoryRoundTrip/RemoteFetchRoundTripTests.cs`: Add tests for scenarios 1-3, 5 +- [ ] Checkpoint: Build and run all tests after changes + +### Out of Scope + +- Save-specific bool-false semantics (future todo if needed) +- Design project examples for bool-return Fetch +- `[AspAuthorize]` + bool-false combination (separate test -- same code path as `[AuthorizeFactory]`) +- All other test files + +### Verification Gates + +1. After renderer fix: `dotnet build src/Neatoo.RemoteFactory.sln` succeeds +2. After adding test targets: Build succeeds (generator produces correct code for new targets) +3. Final: `dotnet test src/Neatoo.RemoteFactory.sln` -- all tests pass, no regressions + +### Stop Conditions + +If any occur, STOP and report: +- Out-of-scope test failure +- Architectural contradiction discovered + +--- + +## Implementation Progress + +**Started:** 2026-03-21 +**Developer:** developer agent + +### Milestones + +1. **Fix read-style bool-false return (Site 1)** -- DONE. Added `method.HasAuth` conditional in `RenderReadLocalMethod` (lines 460-471) to emit `new Authorized()` instead of `default!`. + +2. **Fix write-style bool-false return (Site 2)** -- DONE. Added identical `method.HasAuth` conditional in `RenderLocalMethod(WriteMethodModel)` (lines 900-911). + +3. **Constructor ambiguity discovered and resolved** -- The plan specified `new Authorized(default)` but this caused CS0121 ambiguity between `Authorized(Authorized)` and `Authorized(T?)` constructors. Resolved by using `new Authorized()` (parameterless constructor), which is consistent with the existing save routing default pattern at `RenderSaveLocalMethod` line 1083. Both produce the same runtime state: `HasAccess = false`, `Result = default(T)`. + +4. **Build verification after renderer fix** -- DONE. Full solution builds with 0 errors. + +5. **Add FetchAuthAllow auth class and three test targets** -- DONE. Added to `RoundTripTargets.cs`: `FetchAuthAllow`, `RemoteFetchTarget_AuthBoolFalse`, `RemoteFetchTarget_AuthBoolTrue`, `RemoteFetchTarget_RemoteAuthBoolFalse`. + +6. **Build verification after test targets** -- DONE. Generator produces correct factory code for all three targets. Verified generated `LocalFetch` method emits `new Authorized()` on the bool-false path. + +7. **Add integration tests** -- DONE. Added 4 tests to `RemoteFetchRoundTripTests.cs` for scenarios 1, 2, 3, and 5. + +8. **Full test suite** -- DONE. All 1984 tests pass (502+502 integration + 490+490 unit), 0 failures, 6 skipped (pre-existing performance tests). + +--- + +## Completion Evidence + +**Reported:** 2026-03-21 + +- **Tests Passing:** All 1984 tests pass across net9.0 and net10.0 (502+502 integration, 490+490 unit). Zero failures. 6 skipped (pre-existing ShowcasePerformanceTests). +- **Verification Resources Pass:** N/A (no design project changes needed for this bug fix) +- **All Contract Items:** Confirmed 100% complete + +### Contract Item Status + +| Contract Item | Status | +|--------------|--------| +| `ClassFactoryRenderer.cs` Site 1 (read-style) fix | DONE | +| `ClassFactoryRenderer.cs` Site 2 (write-style) fix | DONE | +| `RoundTripTargets.cs` FetchAuthAllow + 3 targets | DONE | +| `RemoteFetchRoundTripTests.cs` 4 new tests | DONE | +| Build succeeds | DONE | +| All tests pass | DONE | + +### Test Scenario Results + +| Scenario # | Test Method | Result | +|------------|-------------|--------| +| 1 | `RemoteFetch_AuthBoolFalse_ServerReturnsNull` | PASS (returns null, no NRE) | +| 2 | `RemoteFetch_AuthBoolFalse_ClientReturnsNull` | PASS (returns null through round-trip) | +| 3 | `RemoteFetch_AuthBoolTrue_ReturnsObject` | PASS (entity returned with state intact) | +| 4 | (Existing tests) | PASS (no regression) | +| 5 | `RemoteFetch_RemoteAuthBoolFalse_ReturnsNull` | PASS (returns null with service injection) | + +### Deviation from Plan + +The plan specified `new Authorized(default)` but the implementation uses `new Authorized()` (parameterless constructor). This change was necessary because `new Authorized(default)` is ambiguous between `Authorized(Authorized)` and `Authorized(T?)` constructors (CS0121). The parameterless constructor produces identical runtime state (`HasAccess = false`, `Result = default(T)`) and is already the established pattern in the save routing default at `RenderSaveLocalMethod` line 1083. Both business rules 1-6 are satisfied identically. + +--- + +## Architect Verification + +**Verified:** 2026-03-21 +**Verdict:** VERIFIED + +**Independent test results:** All 1984 tests pass (502+502 integration, 490+490 unit). Zero failures. 6 skipped (pre-existing ShowcasePerformanceTests). Build succeeds with 0 errors. + +**Design match:** Both bug sites fixed correctly. Test targets and tests cover all four planned scenarios (1, 2, 3, 5). Developer deviated from `new Authorized(default)` to `new Authorized()` due to CS0121 constructor ambiguity -- verified acceptable (identical runtime state, matches existing codebase pattern). Developer used `AuthorizeFactoryOperation.Fetch` instead of plan's `.Read` -- this is correct (Fetch is the proper enum value for Fetch operations). + +**Issues found:** None. + +--- + +## Requirements Verification + +**Reviewer:** business-requirements-reviewer +**Verified:** 2026-03-21 +**Verdict:** REQUIREMENTS SATISFIED + +### Requirements Compliance + +| Requirement | Source | Status | Evidence | +|-------------|--------|--------|----------| +| Bool-return Fetch contract: `false` = not found, factory returns `null` | `docs/attributes-reference.md:109` | Satisfied | Fix emits `new Authorized()` (non-null wrapper, null Result) instead of `default!` (null wrapper). Public wrapper `.Result` returns `null` instead of NRE. | +| Auth failure behavior: Create/Fetch return null when auth fails | `src/Design/Design.Domain/Aggregates/AuthorizedOrder.cs:54-58` | Satisfied | Not-found path now produces same consumer result (null) as auth-denied path | +| `Authorized` parameterless constructor semantics | `src/RemoteFactory/Authorized.cs:80-82`, base class lines 19-22 | Satisfied | `new Authorized()` sets `HasAccess = false`, `Result = null`. Matches save routing default pattern at line 1087. | +| `[AuthorizeFactory]` usage pattern | `src/Design/Design.Domain/Aggregates/AuthorizedOrderAuth.cs:84` | Satisfied | `FetchAuthAllow` uses `AuthorizeFactoryOperation.Fetch` with `bool` return, matching Design project pattern | +| `[Remote]` only for aggregate root entry points | `src/Design/CLAUDE-DESIGN.md` Critical Rule 1 | Satisfied | New test targets use `[Remote]` on Fetch correctly | +| Properties need public setters | `src/Design/CLAUDE-DESIGN.md` Critical Rule 5 | Satisfied | All test target properties use public setters | +| `partial` keyword required | `src/Design/CLAUDE-DESIGN.md` Critical Rule 6 | Satisfied | All three new test targets are `partial` classes | +| Write-style path also fixed | `ClassFactoryRenderer.cs:894-928` | Satisfied | Identical `method.HasAuth` conditional at write-style site | +| No Design Debt violation | `src/Design/CLAUDE-DESIGN.md` Design Debt table | Satisfied | Fix does not implement any deliberately deferred feature | +| Existing non-auth bool-false tests unaffected | `RemoteFetchRoundTripTests.cs:53-109` | Satisfied | `else` branches preserve original `default!` behavior for non-auth cases | + +### Unintended Side Effects + +1. **Write-style bool-false + auth path**: Fix changes behavior from NRE to `NotAuthorizedException` via save public wrapper (line 1043-1045). Documented in plan Risks section as acceptable -- save-with-bool-return + auth combination is untested and likely unused. Previous behavior (NRE crash) was strictly worse. + +2. **No effect on existing auth tests**: All existing `[AuthorizeFactory]` test targets use void-returning Fetch, not bool-returning. The fix only triggers when both `IsBool` and `HasAuth` are true. + +3. **No effect on serialization contracts**: `Authorized` wrapper is created/consumed within generated local and public methods on the server side. For Fetch, the public wrapper extracts `.Result` (null) before serialization. Null serializes correctly. + +4. **No effect on Design project tests or published docs accuracy**: Design projects use void-returning Fetch. Published docs already state the correct contract (`false` = not found, factory returns `null`). + +### Issues Found + +None. diff --git a/docs/plans/completed/fix-factory-fetch-nre-on-not-found.memory/architect.md b/docs/plans/completed/fix-factory-fetch-nre-on-not-found.memory/architect.md new file mode 100644 index 0000000..b3cb8fc --- /dev/null +++ b/docs/plans/completed/fix-factory-fetch-nre-on-not-found.memory/architect.md @@ -0,0 +1,81 @@ +# Architect -- Fix Factory Fetch NRE on Not Found + +Last updated: 2026-03-21 +Current step: Post-implementation verification (Step 8A) complete -- VERIFIED + +## Key Context + +- Bug is in `ClassFactoryRenderer.cs` at two sites: read-style (~line 460) and write-style (~line 900) +- Both emit `return default!;` on the bool-false path, which is null for `Authorized` +- Public wrappers then dereference `.Result` (read-style) or `.HasAccess` (save-style) on null +- Fix: emit `new Authorized()` instead of `default!` when `HasAuth` is true +- Developer deviated from plan (`new Authorized(default)` to `new Authorized()`) due to CS0121 constructor ambiguity -- verified acceptable + +## Mistakes to Avoid + +- Original plan said `new Authorized(default)` but this causes CS0121 ambiguity between `Authorized(Authorized)` and `Authorized(T?)` constructors when `T` is unconstrained and `default` is `null` +- `new Authorized()` is the correct choice -- it produces identical runtime state (`HasAccess = false`, `Result = null`) and matches existing save routing default pattern at `RenderSaveLocalMethod` +- Plan design section referenced `AuthorizeFactoryOperation.Read` but the correct enum value for Fetch operations is `AuthorizeFactoryOperation.Fetch` -- developer correctly used `.Fetch` + +## User Corrections + +None. + +## Architectural Verification (Pre-Handoff) + +Scope table and evidence documented in plan file. No breaking changes. The fix replaces a crash (NRE) with the contractually correct return value (null). + +## Architect Verification (Post-Implementation) + +**Date:** 2026-03-21 +**Verdict:** VERIFIED + +### Independent Build Results + +- `dotnet build src/Neatoo.RemoteFactory.sln`: 0 errors, 2 warnings (pre-existing Blazor WASM NativeFileReference warnings) +- `dotnet test src/Neatoo.RemoteFactory.sln`: ALL PASS + - Integration tests (net9.0): 502 passed, 3 skipped, 0 failed + - Integration tests (net10.0): 502 passed, 3 skipped, 0 failed + - Unit tests (net9.0): 490 passed, 0 skipped, 0 failed + - Unit tests (net10.0): 490 passed, 0 skipped, 0 failed + - Total: 1984 passed, 6 skipped (pre-existing ShowcasePerformanceTests), 0 failed + +### Design Match + +**Site 1 (read-style, ClassFactoryRenderer.cs lines 462-472):** Fix matches design. `method.HasAuth` conditional added before existing branches. When true, emits `new Authorized<{model.ServiceTypeName}>()`. Handles both async and Task.FromResult variants. Comments explain the rationale. + +**Site 2 (write-style, ClassFactoryRenderer.cs lines 904-914):** Identical fix pattern to Site 1. Comments reference `.HasAccess` (write-style wrapper) rather than `.Result` (read-style wrapper), showing awareness of both NRE sites. + +**Test targets (RoundTripTargets.cs lines 194-267):** +- `FetchAuthAllow`: Authorization class with `AuthorizeFactoryOperation.Fetch` (correct enum value) returning `true` (allows access -- testing not-found path, not auth denial) +- `RemoteFetchTarget_AuthBoolFalse`: Core bug scenario -- auth + bool-false Fetch +- `RemoteFetchTarget_AuthBoolTrue`: Regression guard -- auth + bool-true Fetch +- `RemoteFetchTarget_RemoteAuthBoolFalse`: Auth + bool-false + [Service] injection (proves server execution via remote transport) + +**Tests (RemoteFetchRoundTripTests.cs lines 141-197):** +- Scenario 1: `RemoteFetch_AuthBoolFalse_ServerReturnsNull` -- server container, asserts null +- Scenario 2: `RemoteFetch_AuthBoolFalse_ClientReturnsNull` -- client container round-trip, asserts null +- Scenario 3: `RemoteFetch_AuthBoolTrue_ReturnsObject` -- client container, asserts non-null + state intact +- Scenario 5: `RemoteFetch_RemoteAuthBoolFalse_ReturnsNull` -- client container with [Service], asserts null + +### Deviation Assessment + +Developer used `new Authorized()` instead of plan's `new Authorized(default)`. This is **acceptable and correct**: +- `new Authorized(default)` causes CS0121 ambiguity between `Authorized(Authorized)` and `Authorized(T?)` constructors +- `new Authorized()` parameterless constructor calls base `Authorized()` which sets `HasAccess = false`; `Result` property defaults to `null` (default for reference types via `T? Result { get; init; }`) +- Runtime state is identical: `HasAccess = false`, `Result = null` +- This pattern is already established in the codebase at `RenderSaveLocalMethod` (line ~1083) + +### Test Scenario Coverage + +| Scenario # | Test Method | Covered? | +|------------|-------------|----------| +| 1 | `RemoteFetch_AuthBoolFalse_ServerReturnsNull` | YES | +| 2 | `RemoteFetch_AuthBoolFalse_ClientReturnsNull` | YES | +| 3 | `RemoteFetch_AuthBoolTrue_ReturnsObject` | YES | +| 4 | (Existing tests) | YES (no regression -- all 1984 tests pass) | +| 5 | `RemoteFetch_RemoteAuthBoolFalse_ReturnsNull` | YES | + +### Issues Found + +None. diff --git a/docs/plans/completed/fix-factory-fetch-nre-on-not-found.memory/developer.md b/docs/plans/completed/fix-factory-fetch-nre-on-not-found.memory/developer.md new file mode 100644 index 0000000..448e9a9 --- /dev/null +++ b/docs/plans/completed/fix-factory-fetch-nre-on-not-found.memory/developer.md @@ -0,0 +1,58 @@ +# Developer -- Fix Factory Fetch NRE on Not Found + +Last updated: 2026-03-21 +Current step: Implementation complete, awaiting verification (Step 8) + +## Key Context + +- Bug is in `ClassFactoryRenderer.cs` at two sites: read-style line 460 and write-style line 900 +- Both emitted `return default!;` when `_succeeded == false` and `method.HasAuth` is true +- `default!` for `Authorized` (reference type) is `null` +- Public wrapper at line 315 does `(await ...).Result` -- NRE when `.Result` is called on null +- Save public wrapper at line 1009 does `.HasAccess` -- same NRE pattern +- Fix: emit `new Authorized()` instead of `default!` when `HasAuth` is true +- Used parameterless constructor `new Authorized()` NOT `new Authorized(default)` due to CS0121 ambiguity +- The save routing default at line 1083 already uses `new Authorized()` -- established precedent + +## Mistakes to Avoid + +- Do NOT use `new Authorized(default)` -- causes CS0121 ambiguity between `Authorized(Authorized)` and `Authorized(T?)` constructors +- Use `new Authorized()` (parameterless) instead -- unambiguous and matches existing pattern + +## User Corrections + +- None + +## Developer Review + +**Status:** Approved +**Date:** 2026-03-21 + +(See plan file for full assertion trace table) + +## Implementation Contract + +(See plan file for full contract) + +## Implementation Progress + +All milestones complete: +1. Fixed read-style bool-false return (Site 1) +2. Fixed write-style bool-false return (Site 2) +3. Resolved CS0121 constructor ambiguity +4. Added FetchAuthAllow auth class and 3 test targets +5. Added 4 integration tests (scenarios 1, 2, 3, 5) +6. All 1984 tests pass, 0 failures + +## Completion Evidence + +- All tests pass across net9.0 and net10.0 +- All 4 new tests pass +- All existing tests pass (no regression) +- Plan status set to "Awaiting Verification" + +### Files Modified + +1. `src/Generator/Renderer/ClassFactoryRenderer.cs` -- Fixed two bug sites +2. `src/Tests/RemoteFactory.IntegrationTests/TestTargets/FactoryRoundTrip/RoundTripTargets.cs` -- Added FetchAuthAllow + 3 test targets +3. `src/Tests/RemoteFactory.IntegrationTests/FactoryRoundTrip/RemoteFetchRoundTripTests.cs` -- Added 4 integration tests diff --git a/docs/plans/completed/fix-factory-fetch-nre-on-not-found.memory/requirements-reviewer.md b/docs/plans/completed/fix-factory-fetch-nre-on-not-found.memory/requirements-reviewer.md new file mode 100644 index 0000000..4f4d3f0 --- /dev/null +++ b/docs/plans/completed/fix-factory-fetch-nre-on-not-found.memory/requirements-reviewer.md @@ -0,0 +1,54 @@ +# Requirements Reviewer -- Fix Factory Fetch NRE on Not Found + +Last updated: 2026-03-21 +Current step: Post-implementation verification complete + +## Key Context + +This is a bug fix in the source generator's `ClassFactoryRenderer.cs`. The bug: when a `[Fetch]` method returns `false` (entity not found) and the factory has `[AuthorizeFactory]`, the generated local method returned `default!` which is `null` for `Authorized`. The public wrapper then dereferences `.Result` on null, causing NRE. The fix emits `new Authorized()` instead of `default!` when `HasAuth` is true, at two sites (read-style line 462, write-style line 904). + +The developer deviated from the plan's `new Authorized(default)` to `new Authorized()` due to CS0121 constructor ambiguity between `Authorized(Authorized)` and `Authorized(T?)`. The parameterless constructor produces identical runtime state (`HasAccess = false`, `Result = default(T)`) and is already the established pattern in the save routing default at `RenderSaveLocalMethod` line 1087. + +## Mistakes to Avoid + +None encountered. + +## User Corrections + +None. + +## Requirements Verification + +**Verdict:** REQUIREMENTS SATISFIED +**Date:** 2026-03-21 + +### Compliance Table + +| # | Requirement | Source | Status | Notes | +|---|------------|--------|--------|-------| +| 1 | Bool-return Fetch contract: `false` means not found, factory returns `null` | `docs/attributes-reference.md:109`, `skills/RemoteFactory/references/factory-operations.md:150-166` | Satisfied | Fix ensures local method returns non-null `Authorized` wrapper with null `Result`, so public wrapper's `.Result` returns `null` instead of throwing NRE | +| 2 | Auth failure behavior: Create/Fetch return null when auth fails | `src/Design/Design.Domain/Aggregates/AuthorizedOrder.cs:54-58` | Satisfied | The not-found path now produces the same consumer-visible result (null) as the auth-denied path | +| 3 | `Authorized` constructor semantics | `src/RemoteFactory/Authorized.cs:80-82` (parameterless), base class lines 19-22 | Satisfied | `new Authorized()` sets `HasAccess = false`, `Result = null` (default for reference types). Public wrapper `.Result` returns null correctly | +| 4 | `[Remote]` only for aggregate root entry points | `src/Design/CLAUDE-DESIGN.md` Critical Rule 1 | Satisfied | New test targets use `[Remote]` correctly on their Fetch methods (they are aggregate root entry points) | +| 5 | `[AuthorizeFactory]` usage pattern | `src/Design/Design.Domain/Aggregates/AuthorizedOrderAuth.cs:84` | Satisfied | `FetchAuthAllow` uses `[AuthorizeFactory(AuthorizeFactoryOperation.Fetch)]` matching the Design project pattern | +| 6 | Properties need public setters | `src/Design/CLAUDE-DESIGN.md` Critical Rule 5 | Satisfied | All test target properties use public setters | +| 7 | `partial` keyword required | `src/Design/CLAUDE-DESIGN.md` Critical Rule 6 | Satisfied | All three new test targets are `partial` classes | +| 8 | Existing non-auth bool-false tests unaffected | `RemoteFetchRoundTripTests.cs:53-109` | Satisfied | The `else` branches (lines 477-485 and 919-927) preserve the original `default!` behavior for non-auth cases | +| 9 | Write-style path also fixed | `ClassFactoryRenderer.cs:894-928` | Satisfied | Identical `method.HasAuth` conditional applied at write-style site | +| 10 | No Design Debt violation | `src/Design/CLAUDE-DESIGN.md` Design Debt table | Satisfied | This fix does not implement any deliberately deferred feature | + +### Unintended Side Effects + +1. **Write-style bool-false + auth path**: The fix causes `new Authorized()` with `HasAccess = false` to flow through the save public wrapper, which throws `NotAuthorizedException` (line 1043-1045). This is a change from NRE to `NotAuthorizedException`. The plan documents this as a known semantic difference (Risks section) and explicitly scopes it as acceptable -- the save-with-bool-return + auth combination is untested and likely unused. The previous behavior (NRE crash) was strictly worse. + +2. **No effect on existing authorization tests**: All existing `[AuthorizeFactory]` test targets use void-returning Fetch methods, not bool-returning. The fix only changes behavior when both `IsBool` and `HasAuth` are true, so existing auth tests are unaffected. + +3. **No effect on serialization contracts**: The `Authorized` wrapper is created and consumed entirely within the generated local method and public wrapper on the server side. For Fetch operations, the public wrapper extracts `.Result` (null) before any serialization occurs. The null value serializes correctly through the existing transport. + +4. **No effect on Design project tests**: The Design projects use void-returning Fetch methods and are not affected by this change. + +5. **No effect on published documentation accuracy**: The documentation at `docs/attributes-reference.md:109` states "Returns `bool` or `Task` -- `false` means not found (factory returns `null`)." The fix restores compliance with this documented contract. No doc updates needed. + +### Issues Found + +None. The implementation correctly addresses both bug sites, follows the established `new Authorized()` pattern already used in the save routing default, and adds comprehensive test coverage for the previously untested bool-false + authorization intersection. diff --git a/docs/todos/completed/fix-factory-fetch-nre-on-not-found.md b/docs/todos/completed/fix-factory-fetch-nre-on-not-found.md new file mode 100644 index 0000000..9830338 --- /dev/null +++ b/docs/todos/completed/fix-factory-fetch-nre-on-not-found.md @@ -0,0 +1,142 @@ +# Fix Factory Fetch NRE When Entity Not Found + +**Status:** Complete +**Priority:** High +**Created:** 2026-03-21 +**Last Updated:** 2026-03-21 + +**Plan:** [Fix Factory Fetch NRE Plan](../plans/fix-factory-fetch-nre-on-not-found.md) + +--- + +## Problem + +Generated factory `Fetch` methods throw `NullReferenceException` when the underlying `[Fetch]` method returns `false` (entity not found). + +The generated code pattern is: + +```csharp +// LocalFetch1 (line ~170-174 in generated factory): +var _succeeded = await target.Fetch(visitId, repository, areaListFactory); +if (!_succeeded) +{ + return default!; // <-- returns null for Authorized if it's a reference type +} + +// Public Fetch wrapper (line ~128): +public virtual async Task Fetch(long visitId, ...) +{ + return (await Fetch1Property(visitId, cancellationToken)).Result; // <-- NRE: (null).Result +} +``` + +`LocalFetch1` returns `default!` for `Authorized` when the entity isn't found. The public `Fetch` wrapper then calls `.Result` on the null `Authorized`, causing NRE. + +### Reproduction + +Any factory Fetch call where the `[Fetch]` method returns `false`: + +```csharp +// SignsAssessmentFactory.Fetch(nonExistentVisitId) → NRE +var signs = await signsFactory.Fetch(999999); // throws NullReferenceException +``` + +### Impact + +In zTreatment (Neatoo 0.23.0), this causes 12 database test failures on master and 27 on feature branches. Every factory that has a Fetch returning `bool` (not-found pattern) is affected. + +Affected generated factories observed: `SignsAssessmentFactory`, `VisitFactory`, and any factory where Fetch can legitimately return "not found." + +--- + +## Solution + +The `LocalFetch` method should return `new Authorized(default)` (or equivalent) instead of `default!` when `_succeeded` is false, so that the `.Result` access on the `Authorized` wrapper returns null rather than throwing NRE. + +Alternatively, the public `Fetch` wrapper could null-check before accessing `.Result`: + +```csharp +public virtual async Task Fetch(long visitId, ...) +{ + var authorized = await Fetch1Property(visitId, cancellationToken); + return authorized?.Result; +} +``` + +--- + +## Plans + +- [Fix Factory Fetch NRE Plan](../plans/fix-factory-fetch-nre-on-not-found.md) + +--- + +## Tasks + +- [ ] Fix generated code for not-found path +- [ ] Add test for Fetch-returns-false scenario + +--- + +## Progress Log + +### 2026-03-21 +- Created todo +- Bug discovered in zTreatment database tests — 12 failures on master, 27 on weighted-dosing-engine branch +- All failures trace to same root: `NullReferenceException` in generated `SignsAssessmentFactory.Fetch` at the `(await Fetch1Property(...)).Result` line +- Confirmed `default!` on line 174 of generated factory is the cause + +--- + +## Requirements Review + +**Reviewer:** business-requirements-reviewer +**Reviewed:** 2026-03-21 +**Verdict:** APPROVED + +### Relevant Requirements Found + +1. **Bool-return Fetch contract (factory-operations.md:150-166):** Documented behavioral contract: "returning `bool` signals RemoteFactory: `true` means 'this instance is populated, return it to the caller,' `false` means 'the data wasn't found, discard this instance and return null.'" The todo's bug directly violates this contract when authorization is present. + +2. **Auth failure behavior (AuthorizedOrder.cs:54-58, authorization.md:281):** Documented generator behavior: "Create/Fetch return null when authorization fails." This establishes that the public Fetch wrapper must handle null `Authorized` results. The bug is that the not-found path (`_succeeded == false`) produces a null `Authorized` but the public wrapper assumes a non-null `Authorized`. + +3. **Existing test contracts (RemoteFetchRoundTripTests.cs:53-109):** Three tests document the expected behavior for bool-false Fetch: `RemoteFetch_BoolFalse_ReturnsNull`, `RemoteFetch_BoolFalse_ServerAlsoReturnsNull`, `RemoteFetch_Remote_BoolFalse_ReturnsNull`. All assert `Assert.Null(result)`. These tests pass because their targets lack `[AuthorizeFactory]`, so no `Authorized` wrapping occurs. The bug is in the untested intersection of bool-false + authorization. + +4. **Generated code pattern for bool check (ClassFactoryRenderer.cs:450-470, 877-894):** Two locations in the renderer emit `return default!;` for the `_succeeded == false` path. Both the read-style local method and the write-style local method share this pattern. When `HasAuth` is true, the method's return type is `Authorized`, so `default!` is `null` for this reference type. + +5. **Public wrapper pattern (ClassFactoryRenderer.cs:313-316, 750-753):** When `HasAuth` is true, the public method emits `return (await MethodProperty(...)).Result;` which unconditionally dereferences the `Authorized` return value. This is the NRE site. + +6. **Authorized constructor (Authorized.cs:91-102):** `new Authorized(default)` sets `Result = default` (null for reference types) and `HasAccess = false`. This constructor is the correct way to represent "not found" within the `Authorized` wrapper. + +7. **Write-style bool check (ClassFactoryRenderer.cs:877-894):** The same `default!` pattern exists for write-style methods (Save with bool return). The fix must address both read-style and write-style paths consistently. + +### Gaps + +1. **No test coverage for bool-false + authorization intersection:** The existing `RemoteFetchTarget_BoolFalse` targets have no `[AuthorizeFactory]`. There are no test targets or tests for the combination of `Task` Fetch + `[AuthorizeFactory]` or `[AspAuthorize]`. The architect should require a test target and test that covers this specific combination. + +2. **No Design project example of bool-return Fetch:** The Design project's `Order.cs` Fetch method returns `void` (not `Task`). `AuthorizedOrder.cs` Fetch also returns `void`. The CLAUDE-DESIGN.md Quick Reference mentions `Task Fetch` in the Critical Rules section (line 466) but no actual Design.Domain example demonstrates it. The architect should consider whether a Design project example is warranted. + +3. **Save with bool-return + authorization:** The write-style path has the same `default!` pattern. If any entity combines `[Insert]`/`[Update]`/`[Delete]` returning `bool` with `[AuthorizeFactory]`, the same NRE would occur on Save. The fix should address both read and write paths. + +### Contradictions + +None. The todo proposes fixing a bug that violates the documented bool-return contract. Both proposed solutions (returning `new Authorized(default)` or null-checking before `.Result`) would restore compliance with documented behavior. + +### Recommendations for Architect + +1. **Prefer the `new Authorized(default)` approach over null-check in the public wrapper.** Returning a valid `Authorized` with `HasAccess = false` and `Result = null` from the local method preserves the semantic meaning: the `Authorized` wrapper exists but indicates no access/no result. This is consistent with how authorization failure already works (auth failure returns null via the wrapper, not a null wrapper). The null-check approach (`authorized?.Result`) would silently swallow a null that might indicate a different bug in the future. + +2. **Fix both read-style (ClassFactoryRenderer.cs:450-470) and write-style (ClassFactoryRenderer.cs:877-894) bool-check paths.** Both emit `return default!;` and both need the same fix when `HasAuth` is true. + +3. **Add an integration test target with `[AuthorizeFactory]` + `Task` Fetch that returns false.** The existing `RemoteFetchTarget_BoolFalse` covers the non-auth case. A new target with authorization is needed to cover the exact bug scenario and prevent regression. + +4. **Consider also testing `[AspAuthorize]` + `Task` Fetch returning false**, since `[AspAuthorize]` also triggers the `HasAuth` code path in the generator. + +--- + +## Results / Conclusions + +Fixed NullReferenceException in generated factory Fetch methods when `[Fetch]` returns `false` (entity not found) and the factory has `[AuthorizeFactory]`. The bug was in two sites in `ClassFactoryRenderer.cs` where the bool-false path emitted `return default!;` — null for `Authorized`. Changed to emit `new Authorized()` (non-null wrapper with `HasAccess = false`, `Result = null`), so the public wrapper's `.Result` access returns `null` correctly instead of throwing NRE. + +**Files changed:** `ClassFactoryRenderer.cs` (2 fix sites), `RoundTripTargets.cs` (3 new test targets + auth class), `RemoteFetchRoundTripTests.cs` (4 new tests). All 1,984 tests pass. + diff --git a/docs/todos/fix-factory-fetch-nre-on-not-found.md b/docs/todos/fix-factory-fetch-nre-on-not-found.md deleted file mode 100644 index e107a06..0000000 --- a/docs/todos/fix-factory-fetch-nre-on-not-found.md +++ /dev/null @@ -1,88 +0,0 @@ -# Fix Factory Fetch NRE When Entity Not Found - -**Status:** Open -**Priority:** High -**Created:** 2026-03-21 -**Last Updated:** 2026-03-21 - ---- - -## Problem - -Generated factory `Fetch` methods throw `NullReferenceException` when the underlying `[Fetch]` method returns `false` (entity not found). - -The generated code pattern is: - -```csharp -// LocalFetch1 (line ~170-174 in generated factory): -var _succeeded = await target.Fetch(visitId, repository, areaListFactory); -if (!_succeeded) -{ - return default!; // <-- returns null for Authorized if it's a reference type -} - -// Public Fetch wrapper (line ~128): -public virtual async Task Fetch(long visitId, ...) -{ - return (await Fetch1Property(visitId, cancellationToken)).Result; // <-- NRE: (null).Result -} -``` - -`LocalFetch1` returns `default!` for `Authorized` when the entity isn't found. The public `Fetch` wrapper then calls `.Result` on the null `Authorized`, causing NRE. - -### Reproduction - -Any factory Fetch call where the `[Fetch]` method returns `false`: - -```csharp -// SignsAssessmentFactory.Fetch(nonExistentVisitId) → NRE -var signs = await signsFactory.Fetch(999999); // throws NullReferenceException -``` - -### Impact - -In zTreatment (Neatoo 0.23.0), this causes 12 database test failures on master and 27 on feature branches. Every factory that has a Fetch returning `bool` (not-found pattern) is affected. - -Affected generated factories observed: `SignsAssessmentFactory`, `VisitFactory`, and any factory where Fetch can legitimately return "not found." - ---- - -## Solution - -The `LocalFetch` method should return `new Authorized(default)` (or equivalent) instead of `default!` when `_succeeded` is false, so that the `.Result` access on the `Authorized` wrapper returns null rather than throwing NRE. - -Alternatively, the public `Fetch` wrapper could null-check before accessing `.Result`: - -```csharp -public virtual async Task Fetch(long visitId, ...) -{ - var authorized = await Fetch1Property(visitId, cancellationToken); - return authorized?.Result; -} -``` - ---- - -## Plans - ---- - -## Tasks - -- [ ] Fix generated code for not-found path -- [ ] Add test for Fetch-returns-false scenario - ---- - -## Progress Log - -### 2026-03-21 -- Created todo -- Bug discovered in zTreatment database tests — 12 failures on master, 27 on weighted-dosing-engine branch -- All failures trace to same root: `NullReferenceException` in generated `SignsAssessmentFactory.Fetch` at the `(await Fetch1Property(...)).Result` line -- Confirmed `default!` on line 174 of generated factory is the cause - ---- - -## Results / Conclusions - diff --git a/src/Generator/Renderer/ClassFactoryRenderer.cs b/src/Generator/Renderer/ClassFactoryRenderer.cs index f79b0d9..4f2dfc8 100644 --- a/src/Generator/Renderer/ClassFactoryRenderer.cs +++ b/src/Generator/Renderer/ClassFactoryRenderer.cs @@ -454,10 +454,27 @@ private static void RenderReadLocalMethod(StringBuilder sb, ReadMethodModel meth sb.AppendLine(" {"); sb.AppendLine(" _sw.Stop();"); EmitLogOperationCompleted(sb, " ", method.Operation, model.ServiceTypeName); + // When HasAuth is true, the local method returns Authorized (or Task>). + // Returning default! would be null for this reference type, causing NRE when the public + // wrapper dereferences .Result. Instead return a non-null Authorized with null Result. + // Uses parameterless constructor (HasAccess=false, Result=default) matching the save routing + // default pattern at RenderSaveLocalMethod. + if (method.HasAuth) + { + var authDefault = $"new Authorized<{model.ServiceTypeName}>()"; + if (!needsAsync && method.IsTask) + { + sb.AppendLine($" return Task.FromResult({authDefault});"); + } + else + { + sb.AppendLine($" return {authDefault};"); + } + } // For non-async methods returning Task, wrap default in Task.FromResult to avoid null Task. - // Use null-forgiving operator (!) when returning default for non-nullable return types - // (e.g., Authorized), matching original DoFactoryMethodCallBool behavior. - if (!needsAsync && method.IsTask) + // Use null-forgiving operator (!) when returning default for non-nullable return types, + // matching original DoFactoryMethodCallBool behavior. + else if (!needsAsync && method.IsTask) { var nullableServiceType = method.IsNullable ? $"{model.ServiceTypeName}?" : model.ServiceTypeName; sb.AppendLine($" return Task.FromResult<{nullableServiceType}>(default)!;"); @@ -879,10 +896,27 @@ private static void RenderLocalMethod(StringBuilder sb, WriteMethodModel method, sb.AppendLine(" {"); sb.AppendLine(" _sw.Stop();"); EmitLogOperationCompleted(sb, " ", method.Operation, model.ServiceTypeName); + // When HasAuth is true, the local method returns Authorized (or Task>). + // Returning default! would be null for this reference type, causing NRE when the public + // wrapper dereferences .HasAccess or .Result. Instead return a non-null Authorized with null Result. + // Uses parameterless constructor (HasAccess=false, Result=default) matching the save routing + // default pattern at RenderSaveLocalMethod. + if (method.HasAuth) + { + var authDefault = $"new Authorized<{model.ServiceTypeName}>()"; + if (!needsAsync && method.IsTask) + { + sb.AppendLine($" return Task.FromResult({authDefault});"); + } + else + { + sb.AppendLine($" return {authDefault};"); + } + } // For non-async methods returning Task, wrap default in Task.FromResult to avoid null Task. - // Use null-forgiving operator (!) when returning default for non-nullable return types - // (e.g., Authorized), matching original DoFactoryMethodCallBool behavior. - if (!needsAsync && method.IsTask) + // Use null-forgiving operator (!) when returning default for non-nullable return types, + // matching original DoFactoryMethodCallBool behavior. + else if (!needsAsync && method.IsTask) { var nullableServiceType = method.IsNullable ? $"{model.ServiceTypeName}?" : model.ServiceTypeName; sb.AppendLine($" return Task.FromResult<{nullableServiceType}>(default)!;"); diff --git a/src/Tests/RemoteFactory.IntegrationTests/FactoryRoundTrip/RemoteFetchRoundTripTests.cs b/src/Tests/RemoteFactory.IntegrationTests/FactoryRoundTrip/RemoteFetchRoundTripTests.cs index 4cb0d15..0089c71 100644 --- a/src/Tests/RemoteFactory.IntegrationTests/FactoryRoundTrip/RemoteFetchRoundTripTests.cs +++ b/src/Tests/RemoteFactory.IntegrationTests/FactoryRoundTrip/RemoteFetchRoundTripTests.cs @@ -127,4 +127,72 @@ public async Task RemoteFetch_Remote_BoolTrue_ReturnsObject() // Proves execution happened on the server where IServerOnlyService is available Assert.True(result.ServiceWasInjected); } + + // --- Auth + bool-false tests (NRE bug fix) --- + + /// + /// Scenario 1: [AuthorizeFactory] + [Fetch] returning Task<bool> false via server container. + /// Before the fix, this threw NullReferenceException because the local method returned + /// default! (null) for Authorized<T>, and the public wrapper called .Result on null. + /// After the fix, the local method returns new Authorized<T>() (non-null wrapper with + /// null Result), so the public wrapper returns null correctly. + /// + [Fact] + public async Task RemoteFetch_AuthBoolFalse_ServerReturnsNull() + { + var scopes = ClientServerContainers.Scopes(); + var serverFactory = scopes.server.GetRequiredService(); + + var result = await serverFactory.Fetch(42); + + Assert.Null(result); + } + + /// + /// Scenario 2: [AuthorizeFactory] + [Fetch] returning Task<bool> false via client container. + /// Validates the fix survives serialization round-trip through the client/server transport. + /// + [Fact] + public async Task RemoteFetch_AuthBoolFalse_ClientReturnsNull() + { + var scopes = ClientServerContainers.Scopes(); + var clientFactory = scopes.client.GetRequiredService(); + + var result = await clientFactory.Fetch(42); + + Assert.Null(result); + } + + /// + /// Scenario 3: [AuthorizeFactory] + [Fetch] returning Task<bool> true via client container. + /// Regression guard: verifies the happy path still works when authorization is present. + /// + [Fact] + public async Task RemoteFetch_AuthBoolTrue_ReturnsObject() + { + var scopes = ClientServerContainers.Scopes(); + var clientFactory = scopes.client.GetRequiredService(); + + var result = await clientFactory.Fetch(99); + + Assert.NotNull(result); + Assert.True(result.FetchCalled); + Assert.Equal(99, result.ReceivedId); + } + + /// + /// Scenario 5: [AuthorizeFactory] + [Fetch] returning Task<bool> false with [Service] + /// injection via client container. The [Service] parameter proves execution happens on the + /// server. Validates that the fix works through the remote transport with service injection. + /// + [Fact] + public async Task RemoteFetch_RemoteAuthBoolFalse_ReturnsNull() + { + var scopes = ClientServerContainers.Scopes(); + var clientFactory = scopes.client.GetRequiredService(); + + var result = await clientFactory.Fetch(42); + + Assert.Null(result); + } } diff --git a/src/Tests/RemoteFactory.IntegrationTests/TestTargets/FactoryRoundTrip/RoundTripTargets.cs b/src/Tests/RemoteFactory.IntegrationTests/TestTargets/FactoryRoundTrip/RoundTripTargets.cs index 6423af9..366face 100644 --- a/src/Tests/RemoteFactory.IntegrationTests/TestTargets/FactoryRoundTrip/RoundTripTargets.cs +++ b/src/Tests/RemoteFactory.IntegrationTests/TestTargets/FactoryRoundTrip/RoundTripTargets.cs @@ -186,3 +186,82 @@ internal Task Fetch(int id, [Service] IServerOnlyService service) return Task.FromResult(true); } } + +/// +/// Authorization class that always allows Fetch operations. +/// Used to test the bool-false + authorization intersection (not-found path, not auth-denial). +/// +public class FetchAuthAllow +{ + [AuthorizeFactory(AuthorizeFactoryOperation.Fetch)] + public bool CanFetch() => true; +} + +/// +/// Test target for [Remote, Fetch] returning Task<bool> false with [AuthorizeFactory]. +/// Verifies that the generated factory returns null (not NRE) when a Fetch method returns +/// false and the factory has authorization. This is the exact scenario that triggered +/// the NRE bug: the bool-false path emitted default! for Authorized<T>, which is null, +/// and the public wrapper then called .Result on null. +/// +[Factory] +[AuthorizeFactory] +public partial class RemoteFetchTarget_AuthBoolFalse +{ + public bool FetchCalled { get; set; } + public int ReceivedId { get; set; } + + [Fetch] + [Remote] + internal Task Fetch(int id) + { + FetchCalled = true; + ReceivedId = id; + return Task.FromResult(false); + } +} + +/// +/// Test target for [Remote, Fetch] returning Task<bool> true with [AuthorizeFactory]. +/// Regression guard: verifies the happy path still works when authorization is present. +/// +[Factory] +[AuthorizeFactory] +public partial class RemoteFetchTarget_AuthBoolTrue +{ + public bool FetchCalled { get; set; } + public int ReceivedId { get; set; } + + [Fetch] + [Remote] + internal Task Fetch(int id) + { + FetchCalled = true; + ReceivedId = id; + return Task.FromResult(true); + } +} + +/// +/// Test target for [Remote, Fetch] returning Task<bool> false with [AuthorizeFactory] +/// and [Service] injection. The [Service] parameter proves execution happens on the server +/// (IServerOnlyService is not registered in the client container). +/// +[Factory] +[AuthorizeFactory] +public partial class RemoteFetchTarget_RemoteAuthBoolFalse +{ + public bool FetchCalled { get; set; } + public int ReceivedId { get; set; } + public bool ServiceWasInjected { get; set; } + + [Remote] + [Fetch] + internal Task Fetch(int id, [Service] IServerOnlyService service) + { + FetchCalled = true; + ReceivedId = id; + ServiceWasInjected = service != null; + return Task.FromResult(false); + } +} From c3faa622fac20a7466224b0b6b46bd2c1e16fb10 Mon Sep 17 00:00:00 2001 From: Keith Voels Date: Sat, 21 Mar 2026 16:25:53 -0500 Subject: [PATCH 2/2] chore: bump version to 0.23.1 Co-Authored-By: Claude Opus 4.6 (1M context) --- src/Directory.Build.props | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Directory.Build.props b/src/Directory.Build.props index e144bb0..0c949b6 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -16,7 +16,7 @@ net9.0;net10.0 True 0.7.0 - 0.23.0 + 0.23.1 neatoo_icon.png LICENSE OO Domain Modeling C# .NET Blazor WPF ASP.NET CSLA