From b6f5e9459cecc0db9208d764efa2b29471f447e4 Mon Sep 17 00:00:00 2001 From: KeithVoels Date: Fri, 20 Mar 2026 18:50:33 -0500 Subject: [PATCH] fix: record deserialization with $ref metadata, bump to v0.21.3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NeatooJsonSerializer's global ReferenceHandler.Preserve caused ObjectWithParameterizedCtorRefMetadataNotSupported when Interface Factory methods returned records with parameterized constructors. Fix: dual JsonSerializerOptions — Neatoo types keep ReferenceHandler for $id/$ref tracking, plain records/DTOs use clean options without reference metadata. Type classification via IsNeatooType(). Breaking: non-Neatoo types no longer emit $id/$ref in JSON wire format. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../record-deserialization-ref-metadata.md | 656 ++++++++++++++++++ docs/release-notes/index.md | 2 + docs/release-notes/v0.21.2.md | 2 +- docs/release-notes/v0.21.3.md | 43 ++ docs/serialization.md | 6 + .../record-deserialization-ref-metadata.md | 181 +++++ .../RemoteFactory/references/anti-patterns.md | 29 + .../references/interface-factory.md | 20 + src/Design/CLAUDE-DESIGN.md | 48 ++ .../FactoryPatterns/AllPatterns.cs | 30 +- .../FactoryTests/InterfaceFactoryTests.cs | 37 + src/Directory.Build.props | 2 +- .../Internal/NeatooJsonSerializer.cs | 98 ++- .../InterfaceFactoryRecordTargets.cs | 97 +++ ...nterfaceFactoryRecordSerializationTests.cs | 141 ++++ 15 files changed, 1375 insertions(+), 17 deletions(-) create mode 100644 docs/plans/completed/record-deserialization-ref-metadata.md create mode 100644 docs/release-notes/v0.21.3.md create mode 100644 docs/todos/completed/record-deserialization-ref-metadata.md create mode 100644 src/Tests/RemoteFactory.IntegrationTests/TestTargets/TypeSerialization/InterfaceFactoryRecordTargets.cs create mode 100644 src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/InterfaceFactoryRecordSerializationTests.cs diff --git a/docs/plans/completed/record-deserialization-ref-metadata.md b/docs/plans/completed/record-deserialization-ref-metadata.md new file mode 100644 index 00000000..3a3b4ca0 --- /dev/null +++ b/docs/plans/completed/record-deserialization-ref-metadata.md @@ -0,0 +1,656 @@ +# Fix Record Deserialization with $ref Metadata + +**Date:** 2026-03-20 +**Related Todo:** [Record Deserialization Fails with $ref Metadata](../todos/record-deserialization-ref-metadata.md) +**Status:** Documentation Complete +**Last Updated:** 2026-03-20 (developer deliverables completed) + +--- + +## Overview + +When an Interface Factory method returns a plain record type (no `[Factory]` attribute), the `NeatooJsonSerializer` emits `$id`/`$ref` metadata via `ReferenceHandler.Preserve`. System.Text.Json cannot deserialize types with parameterized constructors when `$ref` metadata is present, causing `ObjectWithParameterizedCtorRefMetadataNotSupported` errors on the client. + +The fix introduces a dual-options approach: the serializer maintains two `JsonSerializerOptions` instances -- one with `ReferenceHandler.Preserve` for Neatoo types (which handle `$id`/`$ref` manually via custom converters), and one without reference handling for non-Neatoo fallback serialization. The server response path determines which options to use based on whether the return type is a Neatoo type. + +--- + +## Business Requirements Context + +**Source:** [Todo Requirements Review](../todos/record-deserialization-ref-metadata.md#requirements-review) + +### Relevant Existing Requirements + +#### Serialization Architecture + +- **NeatooJsonSerializer (`src/RemoteFactory/Internal/NeatooJsonSerializer.cs:68-74`):** Applies `ReferenceHandler.Preserve` globally via `NeatooReferenceHandler`. All serialization/deserialization passes through a single `JsonSerializerOptions` instance. -- Relevance: This is the root cause. The single options instance forces `$id`/`$ref` onto all types including records that cannot handle it. + +- **Custom Converter Chain (`NeatooJsonSerializer.cs:76-85`):** Three converter factories registered in order: `NeatooOrdinalConverterFactory` (for `IOrdinalSerializable`), then `NeatooJsonConverterFactory` subclasses (currently only `NeatooInterfaceJsonConverterFactory`). Types not matched by any converter fall through to STJ built-in handling. -- Relevance: The converter chain correctly handles Neatoo types. The problem is only in the STJ fallback path for non-Neatoo types. + +- **NeatooInterfaceJsonTypeConverter (`src/RemoteFactory/Internal/NeatooInterfaceJsonTypeConverter.cs:44-48`):** Manages `$id`/`$ref` manually for interface-typed Neatoo properties using `$type`/`$value` wrapping. Calls `options.ReferenceHandler.CreateResolver().AddReference()` explicitly. -- Relevance: This converter already handles reference semantics itself. It does not rely on STJ's built-in `$id`/`$ref` emission; it uses its own `$type`/`$value` envelope. + +- **NeatooOrdinalConverterFactory (`src/RemoteFactory/Internal/NeatooOrdinalConverterFactory.cs:60-69`):** Only converts types implementing `IOrdinalSerializable` when format is `Ordinal`. Writes arrays, bypassing `$id`/`$ref` entirely. -- Relevance: Records with `[Factory]` get `IOrdinalSerializable` generated code and use ordinal format. The problem only affects records WITHOUT `[Factory]`. + +#### Interface Factory Return Types + +- **Design Source of Truth (`src/Design/Design.Domain/FactoryPatterns/AllPatterns.cs:204-220`):** Interface Factory methods can return arbitrary non-Neatoo types. The existing `ExampleDto` is a class with public setters and a default constructor. -- Relevance: Establishes that non-Neatoo return types are a supported pattern. The class-based `ExampleDto` works because STJ can populate it via property setters even with `$id`/`$ref`. Records with primary constructors cannot. + +#### Server Response Serialization + +- **HandleRemoteDelegateRequest (`src/RemoteFactory/HandleRemoteDelegateRequest.cs:126-141`):** Extracts the delegate's return type and calls `serializer.Serialize(result, returnType)`. The return type is known at this point. -- Relevance: This is the key integration point where the serialization strategy can be varied per return type. + +#### Client Response Deserialization + +- **MakeRemoteDelegateRequest (`src/RemoteFactory/Internal/MakeRemoteDelegateRequest.cs:69-93`):** Client calls `_neatooJsonSerializer.DeserializeRemoteResponse(result)` which calls `Deserialize(json)`. The `T` is the expected return type. -- Relevance: Deserialization must use matching options (no `ReferenceHandler`) to correctly parse JSON that was serialized without `$id`/`$ref`. + +#### Existing Tests + +- **RecordSerializationTests (`src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/RecordSerializationTests.cs`):** Tests records WITH `[Factory]` through client-server round-trips. These use ordinal format and are NOT affected by this bug. -- Relevance: Must continue to pass. + +- **InterfaceFactoryTests (`src/Tests/RemoteFactory.IntegrationTests/FactoryGenerator/InterfaceFactory/InterfaceFactoryTests.cs`):** Tests Interface Factory with `bool` and `List` return types. These primitive/collection types are not affected by the `$ref` issue. -- Relevance: Must continue to pass. + +- **CoverageGapSerializationTests (`src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/CoverageGapSerializationTests.cs`):** Tests dictionary, enum, and large object types through round-trips. All use `[Factory]`-decorated classes. -- Relevance: Must continue to pass. + +### Gaps + +1. **No test for Interface Factory returning a record with parameterized constructor.** The existing Interface Factory tests return `bool` and `List`. The record serialization tests use `[Factory]`-decorated records (ordinal path). No test covers the intersection: an Interface Factory method returning a plain record. + +2. **No documented anti-pattern for mixing Neatoo types with records in return types.** Clarification A3 establishes this as a new anti-pattern. + +3. **No documented scope for reference preservation.** Published docs describe it for Neatoo domain objects but don't state whether it applies to all types. + +4. **No Design project example of Interface Factory returning a record.** `AllPatterns.cs` uses `ExampleDto` (a class). + +### Contradictions + +None. + +### Recommendations for Architect + +1. Scope the fix to the serializer layer only. +2. Preserve reference handling for Neatoo types. +3. Add a Design project example after implementation. +4. Document the new anti-pattern (Clarification A3). +5. Test with the two DI container pattern. +6. Consider the HandleRemoteDelegateRequest response path as the key integration point. +7. Verify on both net9.0 and net10.0. + +--- + +## Business Rules (Testable Assertions) + +1. WHEN a non-Neatoo type (no `[Factory]`, no `IOrdinalSerializable`) is serialized by `NeatooJsonSerializer`, THEN the output JSON does NOT contain `$id` or `$ref` metadata properties. -- Source: NEW (Gap 1, Clarification A2) + +2. WHEN a Neatoo type (implements `IOrdinalSerializable` or is matched by `NeatooInterfaceJsonConverterFactory`) is serialized by `NeatooJsonSerializer`, THEN reference handling (`$id`/`$ref` for named format, or ordinal array format) continues to work as before. -- Source: NeatooInterfaceJsonTypeConverter lines 44-48, NeatooOrdinalConverterFactory lines 60-69 + +3. WHEN an Interface Factory method returns a record with a primary constructor, THEN the full client-server-client round-trip succeeds and the record is correctly deserialized on the client with all property values preserved. -- Source: NEW (Gap 1, todo Problem section) + +4. WHEN an Interface Factory method returns a record containing a collection (e.g., `IReadOnlyList`), THEN the collection is correctly deserialized with all elements preserved after the round-trip. -- Source: NEW (Gap 1, matches the minimal repro in the todo) + +5. WHEN an Interface Factory method returns null for a nullable record return type, THEN the client receives null without error. -- Source: NEW (defensive rule for null handling) + +6. WHEN an Interface Factory method returns a class with public setters (e.g., `ExampleDto`), THEN the round-trip continues to work as before (no regression). -- Source: Design Source of Truth AllPatterns.cs:204-220 + +7. WHEN a Neatoo [Factory]-decorated record with primary constructor is used (e.g., `SimpleRecord`), THEN it continues to serialize/deserialize via ordinal format without error. -- Source: RecordSerializationTests (existing passing tests) + +8. WHEN a Neatoo [Factory]-decorated class is fetched remotely, THEN it continues to serialize/deserialize with full reference handling without error. -- Source: Existing round-trip tests (RemoteFetchRoundTripTests, etc.) + +### Test Scenarios + +| # | Scenario | Inputs / State | Rule(s) | Expected Result | +|---|----------|---------------|---------|-----------------| +| 1 | Interface Factory returns simple record | `record MyResult(string Name, int Value)` returned from server | Rule 1, 3 | Record deserialized on client with `Name` and `Value` intact | +| 2 | Interface Factory returns record with collection | `record MyResult(string Name, IReadOnlyList Items)` where `MyItem` is a record | Rule 1, 3, 4 | Record deserialized with collection containing all items | +| 3 | Interface Factory returns record with nested record | `record Outer(string Name, Inner Child)` where `Inner` is also a record | Rule 1, 3 | Both records deserialized with all properties intact | +| 4 | Interface Factory returns null for nullable record | Method returns `Task`, server returns null | Rule 5 | Client receives null | +| 5 | Interface Factory returns class with public setters (regression) | `ExampleDto` class returned from Interface Factory | Rule 6 | Class deserialized correctly, same as before | +| 6 | [Factory]-decorated record round-trip (regression) | `[Factory] public partial record SimpleRecord(string Name, int Value)` | Rule 7 | Ordinal serialization/deserialization works as before | +| 7 | [Factory]-decorated class remote fetch (regression) | Class with `[Factory]`, `[Fetch]`, `[Remote]` | Rule 8 | Full round-trip with reference handling works as before | +| 8 | Non-Neatoo type JSON has no $id/$ref | Serialize a plain record and inspect JSON output | Rule 1 | JSON string does not contain `"$id"` or `"$ref"` | + +--- + +## Approach + +### Strategy: Dual JsonSerializerOptions + +The serializer will maintain TWO `JsonSerializerOptions` instances: + +1. **Neatoo Options** (existing): `ReferenceHandler = NeatooReferenceHandler`, with all custom converters. Used for Neatoo types that handle `$id`/`$ref` manually. + +2. **Plain Options** (new): No `ReferenceHandler`, with the same `TypeInfoResolver` and converter chain. Used for non-Neatoo types (records, DTOs, primitives, collections) where `$id`/`$ref` metadata would break parameterized constructor deserialization. + +### Type Classification + +A type is "Neatoo" if it meets ANY of these criteria: +- Implements `IOrdinalSerializable` (records/classes with `[Factory]`) +- Is an interface/abstract type recognized by `NeatooInterfaceJsonConverterFactory` (has entries in `IServiceAssemblies`) + +All other types (plain records, classes, primitives, collections) use the plain options. + +### Classification Location + +The classification happens in `NeatooJsonSerializer` at the `Serialize(object? target, Type targetType)` and `Deserialize(string json, Type type)` methods. The `targetType` parameter is already available. The check is: + +``` +bool isNeatooType = typeof(IOrdinalSerializable).IsAssignableFrom(targetType) + || (targetType.IsInterface && serviceAssemblies.HasType(targetType)); +``` + +For generic wrapper types like `Task`, `IReadOnlyList`, etc., the `returnType` extracted in `HandleRemoteDelegateRequest` is already unwrapped to the inner `T`, so the check works directly. + +### Circular Reference Assessment (Clarification A2) + +Supporting circular references in plain records/DTOs would require implementing a custom `JsonConverter` that manually handles `$id`/`$ref` for arbitrary types with parameterized constructors -- essentially reimplementing STJ's reference resolution with constructor-aware deserialization. This is NOT trivial; it would require: +- Buffering the JSON to read ahead for `$ref` targets +- Constructing objects in two passes (allocate, then populate) +- Handling all constructor parameter patterns STJ supports + +**Verdict: Not trivial. Skip per Clarification A2.** Plain records/DTOs are serialized without reference handling. If users need circular references, they should use Neatoo types. + +--- + +## Design + +### Modified Files + +#### 1. `src/RemoteFactory/Internal/NeatooJsonSerializer.cs` + +**Changes:** +- Add a second `JsonSerializerOptions` instance (`PlainOptions`) without `ReferenceHandler`, sharing the same `TypeInfoResolver` and converter chain +- Add a private helper method `IsNeatooType(Type type)` that determines whether a type should use Neatoo options (with reference handling) or plain options +- Modify `Serialize(object? target, Type targetType)` to select options based on `IsNeatooType(targetType)` +- Modify `Deserialize(string json, Type type)` to select options based on `IsNeatooType(type)` +- Modify `Serialize(object? target)` (no type parameter) to use `IsNeatooType(target.GetType())` +- Modify `Deserialize(string json)` to use `IsNeatooType(typeof(T))` + +**Design detail -- IsNeatooType logic:** +``` +private bool IsNeatooType(Type type) +{ + // Unwrap nullable value types + var underlying = Nullable.GetUnderlyingType(type) ?? type; + + // IOrdinalSerializable = [Factory]-decorated type with generated ordinal support + if (typeof(IOrdinalSerializable).IsAssignableFrom(underlying)) + return true; + + // Interface/abstract types registered in IServiceAssemblies = Neatoo interface factory types + if ((underlying.IsInterface || underlying.IsAbstract) && serviceAssemblies.HasType(underlying)) + return true; + + return false; +} +``` + +**Design detail -- PlainOptions construction:** +``` +this.PlainOptions = new JsonSerializerOptions +{ + // No ReferenceHandler -- this is the key difference + TypeInfoResolver = neatooDefaultJsonTypeInfoResolver, + WriteIndented = serializationOptions.Format == SerializationFormat.Named, + IncludeFields = true +}; + +// Add the same converters (they will CanConvert=false for non-Neatoo types anyway) +if (serializationOptions.Format == SerializationFormat.Ordinal) +{ + this.PlainOptions.Converters.Add(new NeatooOrdinalConverterFactory(serializationOptions)); +} +foreach (var factory in neatooJsonConverterFactories) +{ + this.PlainOptions.Converters.Add(factory); +} +``` + +The converters are shared because they only match Neatoo types (`CanConvert` returns false for non-Neatoo types), so they are inert in the plain options. The critical difference is the absence of `ReferenceHandler`, which prevents STJ from emitting/expecting `$id`/`$ref` metadata. + +**Design detail -- Serialize method selection:** + +In `Serialize(object? target, Type targetType)`: +``` +var options = IsNeatooType(targetType) ? this.Options : this.PlainOptions; +// For Neatoo types, set up the reference resolver as before +if (options == this.Options) +{ + using var rr = new NeatooReferenceResolver(); + this.ReferenceHandler.ReferenceResolver.Value = rr; +} +var result = JsonSerializer.Serialize(target, targetType, options); +``` + +Same pattern for `Deserialize`. + +#### 2. New Test Targets: `src/Tests/RemoteFactory.IntegrationTests/TestTargets/TypeSerialization/InterfaceFactoryRecordTargets.cs` + +New file containing: +- `record InterfaceRecordResult(string Name, int Value)` -- simple record +- `record InterfaceRecordItem(int Id, string Description)` -- item for collection +- `record InterfaceRecordWithCollection(string Name, IReadOnlyList Items)` -- record with collection (the repro scenario) +- `record InterfaceNestedOuter(string Label, InterfaceNestedInner Child)` and `record InterfaceNestedInner(int Id)` -- nested records +- `[Factory] public interface IRecordReturnService` with methods returning these types +- `public class RecordReturnService : IRecordReturnService` -- server implementation + +#### 3. New Test File: `src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/InterfaceFactoryRecordSerializationTests.cs` + +Tests covering all scenarios in the Test Scenarios table. Uses `ClientServerContainers.Scopes(configureServer: ...)` to register the `IRecordReturnService` implementation. + +### What Does NOT Change + +- `NeatooReferenceHandler.cs` -- Unchanged +- `NeatooReferenceResolver.cs` -- Unchanged +- `NeatooInterfaceJsonConverterFactory.cs` -- Unchanged +- `NeatooInterfaceJsonTypeConverter.cs` -- Unchanged +- `NeatooOrdinalConverterFactory.cs` -- Unchanged +- `HandleRemoteDelegateRequest.cs` -- Unchanged (it already passes `returnType` to `Serialize`) +- `MakeRemoteDelegateRequest.cs` -- Unchanged (it calls `DeserializeRemoteResponse`) +- Generator code -- No changes needed +- All existing tests -- Must continue to pass + +### Data Flow (After Fix) + +``` +Server: HandleRemoteDelegateRequest + | + +-- serializer.Serialize(result, returnType) + | + +-- IsNeatooType(returnType)? + | + YES --> Use Options (with ReferenceHandler.Preserve, $id/$ref emitted) + | | + | +-- Custom converters handle $type/$value wrapping + | + NO --> Use PlainOptions (no ReferenceHandler, no $id/$ref) + | + +-- STJ built-in serialization, clean JSON + +Client: MakeRemoteDelegateRequest + | + +-- serializer.DeserializeRemoteResponse(result) + | + +-- serializer.Deserialize(json) + | + +-- IsNeatooType(typeof(T))? + | + YES --> Use Options (with ReferenceHandler, resolves $id/$ref) + | + NO --> Use PlainOptions (no ReferenceHandler, no $id/$ref expected) + | + +-- STJ built-in deserialization, works with parameterized ctors +``` + +--- + +## Implementation Steps + +### Phase 1: Serializer Dual-Options (Core Fix) + +1. Add `PlainOptions` field to `NeatooJsonSerializer` -- a second `JsonSerializerOptions` without `ReferenceHandler` +2. Add `IsNeatooType(Type type)` private method +3. Update `Serialize(object? target)` to select options based on type +4. Update `Serialize(object? target, Type targetType)` to select options based on type +5. Update `Deserialize(string json)` to select options based on `typeof(T)` +6. Update `Deserialize(string json, Type type)` to select options based on type +7. Build and verify all existing tests pass + +### Phase 2: Test Targets and Tests + +1. Create `InterfaceFactoryRecordTargets.cs` with record types, interface factory, and server implementation +2. Create `InterfaceFactoryRecordSerializationTests.cs` with tests for all 8 scenarios +3. Register `IRecordReturnService` -> `RecordReturnService` in test container setup +4. Run all tests -- new tests should pass, existing tests should continue to pass +5. Verify on both net9.0 and net10.0 + +--- + +## Acceptance Criteria + +- [ ] Interface Factory returning `record(string Name, int Value)` completes full client-server round-trip +- [ ] Interface Factory returning `record(string Name, IReadOnlyList Items)` completes full round-trip with collection intact +- [ ] Interface Factory returning null for nullable record returns null on client +- [ ] Existing `[Factory]`-decorated record tests pass (ordinal serialization unaffected) +- [ ] Existing `[Factory]`-decorated class tests pass (reference handling preserved) +- [ ] Existing Interface Factory tests pass (`bool`, `List` return types) +- [ ] All existing integration tests pass +- [ ] All existing unit tests pass +- [ ] Tests pass on both net9.0 and net10.0 +- [ ] JSON output for non-Neatoo types does not contain `$id` or `$ref` + +--- + +## Dependencies + +- System.Text.Json (already a dependency, no version change needed) +- No new NuGet packages required +- No generator changes required + +--- + +## Risks / Considerations + +1. **Type classification edge cases.** The `IsNeatooType` check must correctly handle: + - Generic collection types wrapping Neatoo types (e.g., `IReadOnlyList`) -- these serialize as arrays, not objects, so the collection itself does not get `$id`. The contained Neatoo interface items are handled by `NeatooInterfaceJsonConverterFactory` which manages references internally. The collection type itself is NOT a Neatoo type, so it uses `PlainOptions`. + - Nullable types (`T?`) -- unwrap via `Nullable.GetUnderlyingType()` + - Primitive types (`int`, `string`, `bool`) -- not Neatoo types, use `PlainOptions`. These have always worked because they don't use parameterized constructors with `$ref`. + +2. **Parameters serialized via `ToObjectJson`.** The `ToObjectJson` method calls `Serialize(target)` (no type parameter), which will use `target.GetType()`. For record parameters passed to Interface Factory methods, this is correct -- they are serialized without `$id`/`$ref`. However, these parameters are also deserialized server-side via `FromObjectJson`, which calls `Deserialize(json, type)`. Both sides must agree on whether reference handling is used. Since `IsNeatooType` is deterministic based on the type, they will agree. + +3. **Breaking change to wire format.** Non-Neatoo types that previously had `$id`/`$ref` in their JSON will no longer have it. Per Clarification A4, this is acceptable (v0, full breaking change tolerance). The `$id`/`$ref` for these types was not useful anyway -- it caused failures rather than enabling features. + +4. **Two `JsonSerializerOptions` memory cost.** Minimal impact. `JsonSerializerOptions` caches type metadata internally, so there will be two caches. This is expected and acceptable. + +5. **NeatooInterfaceJsonConverterFactory shared across both options.** The factory is registered as `Transient` in DI, but the `NeatooJsonSerializer` constructor receives the factories via `IEnumerable` and adds the same instances to both options. The `CanConvert` method on these factories only returns `true` for interface/abstract types in `IServiceAssemblies`, so they are inert for non-Neatoo types in `PlainOptions`. However, the fact that they are added to `PlainOptions.Converters` means STJ will call `CanConvert` on them for each type -- this is a minor performance consideration but is consistent with the existing behavior. + +--- + +## Architectural Verification + +**Scope Table:** + +| Pattern/Feature | Affected? | Current Status | After Fix | +|----------------|-----------|---------------|-----------| +| Interface Factory returning record (primary ctor) | YES | Broken ($ref error) | Fixed (PlainOptions, no $ref) | +| Interface Factory returning class (public setters) | NO (regression check) | Working | Unchanged (PlainOptions, no $ref -- but was working with $ref too) | +| Interface Factory returning primitive/collection | NO (regression check) | Working | Unchanged | +| [Factory]-decorated record (ordinal) | NO | Working | Unchanged (uses NeatooOptions, IOrdinalSerializable) | +| [Factory]-decorated class (ordinal) | NO | Working | Unchanged (uses NeatooOptions, IOrdinalSerializable) | +| [Factory]-decorated class (named format) | NO | Working | Unchanged (uses NeatooOptions, ReferenceHandler active) | +| Neatoo interface serialization ($type/$value) | NO | Working | Unchanged (NeatooInterfaceJsonConverterFactory handles) | + +**Verification Evidence:** +- Interface Factory returning record: Needs Implementation (no test exists today) +- All other patterns: Verified by existing passing tests + +**Breaking Changes:** Yes -- JSON wire format changes for non-Neatoo types (no `$id`/`$ref` emitted). Acceptable per Clarification A4 (v0). + +**Codebase Analysis:** +- `NeatooJsonSerializer.cs`: 343 lines, well-structured. Adding a second options instance and type classification is clean. +- `HandleRemoteDelegateRequest.cs`: Already extracts `returnType` and passes it to `Serialize(result, returnType)`. No changes needed. +- `MakeRemoteDelegateRequest.cs`: Calls `DeserializeRemoteResponse` which calls `Deserialize`. No changes needed. +- Converter chain: All converters use `CanConvert` to self-select. Adding them to PlainOptions is safe. + +--- + +## Agent Phasing + +| Phase | Agent Type | Fresh Agent? | Rationale | Dependencies | +|-------|-----------|-------------|-----------|--------------| +| Phase 1: Serializer Dual-Options | developer | Yes | Core runtime library change, focused scope (1 file) | None | +| Phase 2: Test Targets and Tests | developer | No (continue) | Same context needed, tests validate Phase 1 changes | Phase 1 | + +**Parallelizable phases:** None. Phase 2 depends on Phase 1. + +**Notes:** Both phases are small enough to fit in a single agent invocation. The developer can implement Phase 1, run existing tests to verify no regression, then implement Phase 2 and run all tests. + +--- + +## Developer Review + +**Status:** Approved +**Reviewed:** 2026-03-20 + +### Assertion Trace Verification + +| Rule # | Implementation Path (method/condition) | Expected Result | Matches Rule? | Notes | +|--------|---------------------------------------|-----------------|---------------|-------| +| 1 | `NeatooJsonSerializer.Serialize(object? target, Type targetType)` calls `IsNeatooType(targetType)`. For a plain record (no `[Factory]`), `typeof(IOrdinalSerializable).IsAssignableFrom(recordType)` is `false` and `recordType.IsInterface` is `false`, so `IsNeatooType` returns `false`. Method selects `PlainOptions` (no `ReferenceHandler`). STJ serializes without `$id`/`$ref`. | JSON output has no `$id`/`$ref` | Yes | Verified: `HandleRemoteDelegateRequest.cs:141` calls `serializer.Serialize(result, returnType)` where `returnType` is unwrapped from `Task` at line 132. The unwrapped type for a record is the concrete record type. | +| 2 | `NeatooJsonSerializer.Serialize(object? target, Type targetType)` calls `IsNeatooType(targetType)`. For a Neatoo type implementing `IOrdinalSerializable`, `typeof(IOrdinalSerializable).IsAssignableFrom(neatooType)` is `true`, so `IsNeatooType` returns `true`. Method selects `Options` (with `ReferenceHandler = NeatooReferenceHandler`). For interface-typed Neatoo properties, `NeatooInterfaceJsonConverterFactory.CanConvert` returns `true` (line 23: `typeToConvert.IsInterface && !typeToConvert.IsGenericType && serviceAssemblies.HasType(typeToConvert)`), routing to `NeatooInterfaceJsonTypeConverter` which handles `$type`/`$value` wrapping and `$id`/`$ref` manually (lines 44-48). For ordinal format, `NeatooOrdinalConverterFactory.CanConvert` returns `true` (line 69: `typeof(IOrdinalSerializable).IsAssignableFrom(typeToConvert)`), writing arrays. | Reference handling works as before (ordinal arrays or named with `$id`/`$ref`) | Yes | Both converter paths are unchanged. The `Options` instance is identical to the current single instance. | +| 3 | Server: `HandleRemoteDelegateRequest` at line 141 calls `serializer.Serialize(result, returnType)` where `returnType` is the unwrapped record type. `IsNeatooType(recordType)` returns `false` (not `IOrdinalSerializable`, not interface/abstract). Uses `PlainOptions` -- STJ serializes with parameterized ctor properties as standard JSON. Client: `MakeRemoteDelegateRequest.ForDelegateNullable` at line 93 calls `DeserializeRemoteResponse` which calls `Deserialize(json)`. `IsNeatooType(typeof(T))` returns `false` for the record. Uses `PlainOptions` -- STJ deserializes via parameterized constructor without `$ref` metadata interference. | Full round-trip succeeds, record properties preserved | Yes | Symmetric classification on both sides ensures matching options. | +| 4 | Same path as Rule 3. The record type `record MyResult(string Name, IReadOnlyList Items)` is not `IOrdinalSerializable`, not interface/abstract. `IsNeatooType` returns `false`. With `PlainOptions` (no `ReferenceHandler`), STJ serializes `IReadOnlyList` as a standard JSON array. `MyItem` is also a plain record, so STJ handles it with built-in constructor-based serialization. No `$ref` metadata is emitted for collection elements. | Collection deserialized with all elements intact | Yes | STJ natively handles `IReadOnlyList` and records with primary ctors when no `ReferenceHandler` is active. | +| 5 | Server: `NeatooJsonSerializer.Serialize(object? target, Type targetType)` at line 124 -- when `target == null`, returns `null` immediately (before `IsNeatooType` is even called). `HandleRemoteDelegateRequest` at line 141 creates `RemoteResponseDto(null)`. Client: `DeserializeRemoteResponse` at line 323 checks `remoteResponse?.Json == null`, returns `default` (which is `null` for a nullable record). | Client receives null without error | Yes | The null path short-circuits before any options selection. No serialization/deserialization of the null value occurs. | +| 6 | Server: `Serialize(result, returnType)` where `returnType` is `ExampleDto` (a class). `IsNeatooType(typeof(ExampleDto))` returns `false` -- not `IOrdinalSerializable`, not interface/abstract. Uses `PlainOptions`. STJ serializes as standard JSON object (no `$id`/`$ref`). Client: `Deserialize` where `T` is `ExampleDto`. `IsNeatooType` returns `false`. Uses `PlainOptions`. STJ deserializes via public setters. | Class deserialized correctly | Yes | Wire format changes (no `$id`/`$ref` where there was before), but deserialization still works because public-setter classes do not need `$ref` resolution. Acceptable per Clarification A4. | +| 7 | Server: `Serialize(result, returnType)` where `returnType` is a `[Factory]`-decorated record. `typeof(IOrdinalSerializable).IsAssignableFrom(recordType)` is `true` (generator implements `IOrdinalSerializable`). `IsNeatooType` returns `true`. Uses `Options` (with `ReferenceHandler`). `NeatooOrdinalConverterFactory.CanConvert` returns `true`, serializes as array. | Ordinal serialization works as before | Yes | The `Options` instance is identical to the current code. No change to the Neatoo type path. | +| 8 | Same path as Rule 2. All `[Factory]`-decorated classes implement `IOrdinalSerializable`. `IsNeatooType` returns `true`. `Options` (with `ReferenceHandler`) is used. Custom converters handle `$id`/`$ref`/`$type`/`$value` as before. | Full round-trip with reference handling works | Yes | No change to the Neatoo type serialization path. | + +### Concerns + +**Non-blocking observations (no changes to plan required):** + +1. **`HasType` semantics vs. plan description.** The plan's `IsNeatooType` description says `serviceAssemblies.HasType(underlying)` checks if the type is "recognized by NeatooInterfaceJsonConverterFactory (has entries in IServiceAssemblies)." The actual `HasType` implementation (`ServiceAssemblies.cs:49`) checks `this.Assemblies.Contains(type.Assembly)` -- it checks whether the type's assembly is registered, not whether the specific type is a Neatoo type. However, this does not affect correctness because the `IsNeatooType` check guards the `HasType` call with `underlying.IsInterface || underlying.IsAbstract`. Plain record types are concrete, so they never reach the `HasType` check. The `IsNeatooType` classification is correct for the problem being solved. + +2. **Generic interface Neatoo types.** The plan's `IsNeatooType` checks `(underlying.IsInterface || underlying.IsAbstract) && serviceAssemblies.HasType(underlying)` but does not include the `!underlying.IsGenericType` check that `NeatooInterfaceJsonConverterFactory.CanConvert` includes (line 23). This means a generic Neatoo interface would be classified as Neatoo by `IsNeatooType` (using `Options` with `ReferenceHandler`) but NOT matched by the converter's `CanConvert`, falling through to STJ built-in handling with `ReferenceHandler.Preserve` active. In practice this is unlikely to occur -- Interface Factory return types are not generic interfaces -- and the behavior is no worse than the current code (which also uses `ReferenceHandler.Preserve` for everything). Noting for completeness only. + +3. **`ToObjectJson`/`FromObjectJson` parameter serialization.** The plan correctly identifies (Risk 2) that `ToObjectJson` calls `Serialize(target)` (no type parameter), which the plan modifies to use `IsNeatooType(target.GetType())`. The server-side `FromObjectJson` calls `Deserialize(json, type)` where `type` comes from `serviceAssemblies.FindType()`. Both sides will use `IsNeatooType` on the same concrete type, so classification is symmetric. No issue. + +4. **Agent Phasing is practical.** Both phases are small enough for a single agent invocation. Phase 2 depends on Phase 1. The "No (continue)" decision for Phase 2 is correct -- the developer needs the Phase 1 context to write tests against the changes. + +**Verdict: Approved.** The plan is complete, correct, and implementable. All assertion traces produce results consistent with the business rules. The design is minimal and surgical -- one file modified, two new test files created. The dual-options approach is clean and the `IsNeatooType` classification is correct for all traced scenarios. + +--- + +## Implementation Contract + +**Created:** 2026-03-20 +**Approved by:** developer agent (remotefactory-developer) + +### Verification Acceptance Criteria + +No pre-existing failing acceptance criteria from the architect. All existing tests must continue to pass. + +### Test Scenario Mapping + +| Scenario # | Test Method | Notes | +|------------|-------------|-------| +| 1 | `InterfaceFactoryRecordSerializationTests.InterfaceFactory_SimpleRecord_RoundTrip` | Simple record with primary ctor through client-server | +| 2 | `InterfaceFactoryRecordSerializationTests.InterfaceFactory_RecordWithCollection_RoundTrip` | Record containing `IReadOnlyList` of records | +| 3 | `InterfaceFactoryRecordSerializationTests.InterfaceFactory_NestedRecord_RoundTrip` | Record containing another record as a property | +| 4 | `InterfaceFactoryRecordSerializationTests.InterfaceFactory_NullableRecord_ReturnsNull` | Nullable record method returns null | +| 5 | Covered by existing `InterfaceFactoryTests` (bool, List) + no-regression gate | ExampleDto class regression covered by existing Design tests | +| 6 | Covered by existing `RecordSerializationTests` (all passing) | [Factory]-decorated record regression | +| 7 | Covered by existing round-trip tests (RemoteFetchRoundTripTests, etc.) | [Factory]-decorated class regression | +| 8 | `InterfaceFactoryRecordSerializationTests.InterfaceFactory_NonNeatooType_NoRefMetadata` | Direct JSON inspection for absence of `$id`/`$ref` | + +### In Scope + +- [ ] `src/RemoteFactory/Internal/NeatooJsonSerializer.cs`: Add `PlainOptions` field, `IsNeatooType(Type)` method, modify all 4 Serialize/Deserialize methods to select options +- [ ] `src/Tests/RemoteFactory.IntegrationTests/TestTargets/TypeSerialization/InterfaceFactoryRecordTargets.cs`: New file with record types, `IRecordReturnService` interface factory, and server implementation +- [ ] `src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/InterfaceFactoryRecordSerializationTests.cs`: New file with 5+ tests covering scenarios 1-4 and 8 +- [ ] Checkpoint: After Phase 1, run `dotnet test` on full solution -- all existing tests must pass +- [ ] Checkpoint: After Phase 2, run `dotnet test` on full solution -- all existing + new tests must pass on both net9.0 and net10.0 + +### Out of Scope + +- `NeatooReferenceHandler.cs` -- No changes +- `NeatooReferenceResolver.cs` -- No changes +- `NeatooInterfaceJsonConverterFactory.cs` -- No changes +- `NeatooInterfaceJsonTypeConverter.cs` -- No changes +- `NeatooOrdinalConverterFactory.cs` -- No changes +- `HandleRemoteDelegateRequest.cs` -- No changes +- `MakeRemoteDelegateRequest.cs` -- No changes +- Generator code -- No changes +- All existing test files -- Must NOT be modified +- Documentation files (CLAUDE-DESIGN.md, Design project examples, release notes) -- Handled in Step 9 + +### Verification Gates + +1. After Phase 1 (serializer changes): `dotnet test src/Neatoo.RemoteFactory.sln` -- all existing tests pass (zero failures) +2. After Phase 2 (new tests): `dotnet test src/Neatoo.RemoteFactory.sln` -- all existing + new tests pass on both net9.0 and net10.0 +3. Final: Full solution builds and tests pass with zero warnings from new code + +### Stop Conditions + +If any occur, STOP and report: +- Any existing test failure after Phase 1 changes (indicates regression in serializer) +- `NeatooInterfaceJsonConverterFactory` or `NeatooOrdinalConverterFactory` behavior changes (converters must remain untouched) +- Out-of-scope test failure +- Architectural contradiction discovered (e.g., a Neatoo type that does not implement `IOrdinalSerializable` and is not interface/abstract) + +--- + +## Implementation Progress + +**Started:** 2026-03-20 +**Developer:** remotefactory-developer + +### Phase 1: Serializer Dual-Options (Core Fix) + +- [x] Added `PlainOptions` field to `NeatooJsonSerializer` -- a second `JsonSerializerOptions` without `ReferenceHandler`, sharing the same `TypeInfoResolver` and converter chain +- [x] Added `IsNeatooType(Type type)` private method with nullable unwrapping, `IOrdinalSerializable` check, and interface/abstract + `serviceAssemblies.HasType` check +- [x] Updated `Serialize(object? target)` to select options based on `target.GetType()` +- [x] Updated `Serialize(object? target, Type targetType)` to select options based on `targetType` +- [x] Updated `Deserialize(string? json)` to select options based on `typeof(T)` +- [x] Updated `Deserialize(string? json, Type type)` to select options based on `type` +- [x] Fixed CA1851 (multiple enumeration of IEnumerable) by materializing `neatooJsonConverterFactories` to a list before iterating twice +- [x] Build: 0 errors, 0 new warnings +- [x] All existing tests pass: 490 unit (net9.0) + 490 unit (net10.0) + 481 integration (net9.0) + 481 integration (net10.0) + +### Phase 2: Test Targets and Tests + +- [x] Created `InterfaceFactoryRecordTargets.cs` with 5 record types, `IRecordReturnService` interface factory, and `RecordReturnService` server implementation +- [x] Created `InterfaceFactoryRecordSerializationTests.cs` with 6 tests covering all scenarios: + - `InterfaceFactory_SimpleRecord_RoundTrip` (Scenario 1, Rules 1/3) + - `InterfaceFactory_RecordWithCollection_RoundTrip` (Scenario 2, Rules 1/3/4) + - `InterfaceFactory_NestedRecord_RoundTrip` (Scenario 3, Rules 1/3) + - `InterfaceFactory_NullableRecord_ReturnsNull` (Scenario 4, Rule 5) + - `InterfaceFactory_NullableRecord_ReturnsValue` (additional coverage for non-null nullable path) + - `InterfaceFactory_NonNeatooType_NoRefMetadata` (Scenario 8, Rule 1 -- direct JSON inspection) +- [x] All tests pass on both net9.0 and net10.0 + +--- + +## Completion Evidence + +**Reported:** 2026-03-20 + +- **Tests Passing:** + - `RemoteFactory.UnitTests.dll (net9.0)`: 490 passed, 0 failed + - `RemoteFactory.UnitTests.dll (net10.0)`: 490 passed, 0 failed + - `RemoteFactory.IntegrationTests.dll (net9.0)`: 487 passed, 0 failed, 3 skipped (pre-existing ShowcasePerformanceTests) + - `RemoteFactory.IntegrationTests.dll (net10.0)`: 487 passed, 0 failed, 3 skipped (pre-existing ShowcasePerformanceTests) + - Integration test count increased from 481 to 487 (6 new tests added) +- **Verification Resources Pass:** N/A (no Design project changes in implementation scope) +- **All Contract Items:** Confirmed 100% complete + +### Contract Checklist + +- [x] `src/RemoteFactory/Internal/NeatooJsonSerializer.cs`: `PlainOptions` field, `IsNeatooType(Type)` method, all 4 Serialize/Deserialize methods updated +- [x] `src/Tests/RemoteFactory.IntegrationTests/TestTargets/TypeSerialization/InterfaceFactoryRecordTargets.cs`: New file with record types, `IRecordReturnService`, and `RecordReturnService` +- [x] `src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/InterfaceFactoryRecordSerializationTests.cs`: New file with 6 tests covering scenarios 1-4 and 8 +- [x] Checkpoint 1 (after Phase 1): All existing tests pass -- 0 failures +- [x] Checkpoint 2 (after Phase 2): All existing + new tests pass on both net9.0 and net10.0 -- 0 failures + +### Out-of-Scope Files NOT Modified + +- `NeatooReferenceHandler.cs` -- Not modified +- `NeatooReferenceResolver.cs` -- Not modified +- `NeatooInterfaceJsonConverterFactory.cs` -- Not modified +- `NeatooInterfaceJsonTypeConverter.cs` -- Not modified +- `NeatooOrdinalConverterFactory.cs` -- Not modified +- `HandleRemoteDelegateRequest.cs` -- Not modified +- `MakeRemoteDelegateRequest.cs` -- Not modified +- Generator code -- Not modified +- All existing test files -- Not modified +- Documentation files -- Not modified (handled in Step 9) + +### Stop Conditions Encountered + +None. No existing test failures, no out-of-scope modifications needed, no architectural contradictions discovered. + +--- + +## Documentation + +**Agent:** business-requirements-documenter, documentation-writer, remotefactory-developer +**Completed:** 2026-03-20 +**Status:** Complete (all deliverables fulfilled) + +### Files Directly Updated + +1. **`src/Design/CLAUDE-DESIGN.md`:** + - Added Anti-Pattern 9: Mixing Neatoo types with records in Interface Factory return types (per Clarification A3, Gap 2). Includes WRONG/RIGHT examples and explanation of the serialization mismatch. + - Added item 10 to Common Mistakes summary: "Mixing Neatoo types with records in Interface Factory return types" + - Added Quick Decisions entry: "Can Interface Factory return a record?" -- Yes, plain records/DTOs without Neatoo types + - Added Design Completeness Checklist item: "Interface Factory returning a record type" (unchecked -- awaiting Design project example) + +2. **`docs/serialization.md`:** + - Added "Scope: Neatoo Types Only" subsection under Reference Preservation. Documents that `$id`/`$ref` applies only to Neatoo types, plain records/DTOs are serialized without reference handling, and mixing the two in a single return type is an anti-pattern. + +### Developer Deliverables + +These require source code changes and cannot be made by the documentation agent. + +1. **[COMPLETED]** **`src/Design/Design.Domain/FactoryPatterns/AllPatterns.cs`:** Added `ExampleRecordResult` record type with primary constructor `(int Id, string Name)`. Added `GetRecordByIdAsync(int id)` method to `IExampleRepository` interface returning `Task`. Added implementation in `ExampleRepository` and `NullReturningRepository`. Checked off the Design Completeness Checklist item in `CLAUDE-DESIGN.md`. + +2. **[COMPLETED]** **`src/Design/Design.Tests/FactoryTests/InterfaceFactoryTests.cs`:** Added `InterfaceFactory_GetRecordByIdAsync_ReturnsRecordFromServer` test demonstrating the Interface Factory record return type pattern works through a full client-server round-trip using `DesignClientServerContainers.Scopes()`. Test passes on both net9.0 and net10.0. + +3. **[COMPLETED]** **Skill updates:** Updated `skills/RemoteFactory/references/interface-factory.md` with a "Record Return Types" section showing hand-written examples of records as Interface Factory return types plus the anti-pattern note. Updated `skills/RemoteFactory/references/anti-patterns.md` with Anti-Pattern 14: Mixing Neatoo Types with Records in Interface Factory Return Types, and added it to the Summary Table. No mdsnippets run needed -- all additions are hand-written content (anti-pattern examples and guidance), not compiled code snippets. + +4. **Release notes:** Create `docs/release-notes/vX.Y.Z.md` documenting this as a bug fix (patch version bump). The fix resolves `ObjectWithParameterizedCtorRefMetadataNotSupported` errors when Interface Factory methods return record types with primary constructors. + +### General Documentation (documentation-writer) + +**Agent:** documentation-writer +**Completed:** 2026-03-20 + +1. **`docs/release-notes/v0.21.3.md`:** Created release notes for patch version 0.21.3. Documents the bug fix, wire format breaking change for non-Neatoo types, migration guide, and commit summary. +2. **`docs/release-notes/index.md`:** Added v0.21.3 to both the Highlights table and All Releases list. +3. **`docs/release-notes/v0.21.2.md`:** Bumped nav_order from 1 to 2 to make room for v0.21.3. +4. **`src/Directory.Build.props`:** Updated `PackageVersion` from `0.21.2` to `0.21.3`. + +--- + +## Architect Verification + +**Verified:** 2026-03-20 +**Verdict:** VERIFIED + +**Independent build results:** +- `dotnet build src/Neatoo.RemoteFactory.sln`: 0 errors, 1 warning (pre-existing Blazor WASM NativeFileReference warning, unrelated) + +**Independent test results:** +- `RemoteFactory.UnitTests.dll (net9.0)`: 490 passed, 0 failed, 0 skipped +- `RemoteFactory.UnitTests.dll (net10.0)`: 490 passed, 0 failed, 0 skipped +- `RemoteFactory.IntegrationTests.dll (net9.0)`: 487 passed, 0 failed, 3 skipped (pre-existing ShowcasePerformanceTests) +- `RemoteFactory.IntegrationTests.dll (net10.0)`: 487 passed, 0 failed, 3 skipped (pre-existing ShowcasePerformanceTests) + +**Design match:** Yes -- implementation matches the original plan in all respects. + +1. **`PlainOptions` field (lines 98-113):** Second `JsonSerializerOptions` without `ReferenceHandler`, shares `TypeInfoResolver` and converter chain. The developer also addressed CA1851 by materializing `neatooJsonConverterFactories` to a list before iterating twice (line 74). Matches plan. +2. **`IsNeatooType(Type type)` method (lines 123-136):** Nullable unwrapping via `Nullable.GetUnderlyingType`, `IOrdinalSerializable.IsAssignableFrom` check, then `IsInterface || IsAbstract` with `serviceAssemblies.HasType` check. Exact match to plan's design detail. +3. **All four Serialize/Deserialize methods updated:** Each selects options via `IsNeatooType` and guards `NeatooReferenceResolver` setup with `if (options == this.Options)`. Matches plan. +4. **Test targets (`InterfaceFactoryRecordTargets.cs`):** 5 record types, `[Factory] IRecordReturnService` interface, `RecordReturnService` implementation. Matches plan. +5. **Test file (`InterfaceFactoryRecordSerializationTests.cs`):** 6 tests covering scenarios 1-4, 8, plus bonus non-null nullable path. Uses `ClientServerContainers.Scopes` with `configureServer` for server-side DI registration. Matches plan with additional coverage. +6. **Scope discipline:** `git status` confirms only `NeatooJsonSerializer.cs` modified, 2 new test files added. No out-of-scope files touched (no converters, no handlers, no generator code, no existing test files). + +**Issues found:** None + +--- + +## Requirements Verification + +**Reviewer:** business-requirements-reviewer +**Verified:** 2026-03-20 +**Verdict:** REQUIREMENTS SATISFIED + +### Requirements Compliance + +| Requirement | Status | Evidence | +|-------------|--------|----------| +| Serialization Architecture -- NeatooJsonSerializer applies ReferenceHandler.Preserve globally (root cause) | Satisfied | Verified in `NeatooJsonSerializer.cs`: `Options` (line 76-82) retains `ReferenceHandler = this.ReferenceHandler` for Neatoo types. New `PlainOptions` (lines 98-113) omits `ReferenceHandler`. The `IsNeatooType` method (lines 123-136) correctly routes types. Non-Neatoo types no longer receive `$id`/`$ref` metadata. | +| Custom Converter Chain -- NeatooOrdinalConverterFactory and NeatooInterfaceJsonConverterFactory unchanged | Satisfied | Verified: `NeatooOrdinalConverterFactory.cs` and `NeatooInterfaceJsonConverterFactory.cs` were not modified (confirmed via git status and file reading). Both converters are added to `PlainOptions` (lines 105-113) but are inert for non-Neatoo types because their `CanConvert` methods return `false` for plain records. | +| NeatooInterfaceJsonTypeConverter manages $id/$ref manually (lines 44-48) | Satisfied | Verified: `NeatooInterfaceJsonTypeConverter.cs` was not modified. It calls `options.ReferenceHandler!.CreateResolver().AddReference(id, result)` at line 46-47. This converter only runs for interface/abstract types matched by `NeatooInterfaceJsonConverterFactory.CanConvert` (line 23: `typeToConvert.IsInterface && !typeToConvert.IsGenericType && serviceAssemblies.HasType(typeToConvert)`). All such types pass `IsNeatooType` and route to `Options` (which has `ReferenceHandler`), so the converter always has a valid `ReferenceHandler` available. | +| NeatooOrdinalConverterFactory handles [Factory]-decorated records via IOrdinalSerializable | Satisfied | Verified: `NeatooOrdinalConverterFactory.CanConvert` (line 60-69) checks `typeof(IOrdinalSerializable).IsAssignableFrom(typeToConvert)`. `IsNeatooType` also checks `IOrdinalSerializable.IsAssignableFrom` first (line 128), so all ordinal types route to `Options`. No change to ordinal serialization path. | +| Interface Factory return types can be arbitrary non-Neatoo types (AllPatterns.cs:204-220) | Satisfied | Verified: The implementation makes this pattern work for records with parameterized constructors, not just classes with public setters. `IsNeatooType` returns `false` for concrete non-Neatoo types (records and classes alike), routing them to `PlainOptions`. Test `InterfaceFactory_SimpleRecord_RoundTrip` demonstrates the record round-trip through `ClientServerContainers.Scopes`. | +| HandleRemoteDelegateRequest extracts returnType and calls Serialize(result, returnType) | Satisfied | Verified: `HandleRemoteDelegateRequest.cs` was not modified (confirmed via git status). Line 141 calls `serializer.Serialize(result, returnType)` where `returnType` is unwrapped from `Task` at lines 130-133. The serializer now classifies this via `IsNeatooType(targetType)`. | +| MakeRemoteDelegateRequest calls DeserializeRemoteResponse on client | Satisfied | Verified: `MakeRemoteDelegateRequest.cs` was not modified. Client-side deserialization calls `Deserialize(json)` which now uses `IsNeatooType(typeof(T))` -- symmetric classification with the server side. | +| Existing RecordSerializationTests (ordinal format) must continue to pass | Satisfied | Completion evidence: 487 integration tests pass on both net9.0 and net10.0. [Factory]-decorated records implement `IOrdinalSerializable`, so `IsNeatooType` returns `true`, routing to `Options` with full reference handling -- identical to pre-change behavior. | +| Existing InterfaceFactoryTests (bool, List) must continue to pass | Satisfied | Completion evidence: 487 integration tests pass. Primitive and collection types are not Neatoo types, so they route to `PlainOptions`. These types never had parameterized constructor issues, so removing `$id`/`$ref` is a wire format change only (acceptable per Clarification A4). | +| Design Debt -- no deliberately deferred features implemented | Satisfied | Verified: The 5 design debt items in `CLAUDE-DESIGN.md` (lines 685-691) are: private setters, OR logic for AspAuthorize, automatic Remote detection, collection factory injection, IEnumerable serialization. None are implemented by this change. The fix is scoped to the serializer's options selection logic. | +| Anti-Patterns 1-8 -- no violations | Satisfied | Verified: All 8 anti-patterns in `CLAUDE-DESIGN.md` (lines 156-372) are unrelated to serializer options selection. The new test targets follow correct patterns: `[Factory]` on the interface, no operation attributes on interface methods, `public` interface with server implementation. | + +### Unintended Side Effects + +1. **Wire format change for non-Neatoo types (acceptable).** Non-Neatoo types (e.g., `ExampleDto` class returned from Interface Factories) previously had `$id`/`$ref` in their JSON. After this change, they do not. This is a breaking wire format change. Per Clarification A4, this is acceptable (v0, full breaking change tolerance). The `$id`/`$ref` metadata was never useful for these types and caused failures for records with parameterized constructors. + +2. **ToObjectJson/FromObjectJson parameter path (no issue).** `ToObjectJson` (line 329-339) calls `this.Serialize(target)`, which now uses `IsNeatooType(target.GetType())`. `FromObjectJson` (line 401-411) calls `this.Deserialize(objectTypeJson.Json, targetType)`, which uses `IsNeatooType(type)`. Both sides classify the same concrete type identically, so serialization/deserialization are symmetric. For record parameters passed to Interface Factory methods, both sides will use `PlainOptions`. For Neatoo types, both sides will use `Options`. No issue. + +3. **PlainOptions shares TypeInfoResolver with Options (no issue).** The `NeatooJsonTypeInfoResolver` overrides `CreateObject` for DI-registered types. For non-Neatoo record types, the resolver does not find them in DI and falls back to STJ's default constructor-based deserialization. This is the correct behavior for records with primary constructors. + +4. **NeatooInterfaceJsonConverterFactory registered in PlainOptions (no issue).** The factory is added to `PlainOptions.Converters` (lines 110-113), but its `CanConvert` method only returns `true` for non-generic interface/abstract types in registered assemblies. Plain records are concrete types and never match. The factory is inert in `PlainOptions`. Minor performance cost from STJ calling `CanConvert` on each type, consistent with existing behavior. + +5. **HasType semantics (no issue).** `ServiceAssemblies.HasType` (line 49) checks assembly membership, not specific type identity. However, `IsNeatooType` guards the `HasType` call with `underlying.IsInterface || underlying.IsAbstract` (line 132), so concrete record types in registered assemblies never reach the `HasType` check. Classification is correct. + +### Issues Found + +None. The implementation satisfies all documented requirements, follows the established serialization architecture, does not violate any anti-pattern, and does not implement any deliberately deferred feature. The scope is minimal (one production file modified, two new test files) and all existing tests pass on both net9.0 and net10.0. diff --git a/docs/release-notes/index.md b/docs/release-notes/index.md index 1efdcea8..ddbb650c 100644 --- a/docs/release-notes/index.md +++ b/docs/release-notes/index.md @@ -16,6 +16,7 @@ Releases with new features, breaking changes, or bug fixes. | Version | Date | Highlights | |---------|------|------------| +| [v0.21.3](v0.21.3.md) | 2026-03-20 | Fix record deserialization with `$id`/`$ref` metadata | | [v0.21.2](v0.21.2.md) | 2026-03-08 | Trimming-safe factory registration for all factory types | | [v0.21.1](v0.21.1.md) | 2026-03-08 | Fix factories trimmed when first overload has no callers | | [v0.21.0](v0.21.0.md) | 2026-03-08 | [Remote] requires internal (breaking), Can* auth-driven guards, trimming fixes | @@ -45,6 +46,7 @@ Releases with new features, breaking changes, or bug fixes. ## All Releases +- [v0.21.3](v0.21.3.md) - 2026-03-20 - Fix record deserialization with reference metadata - [v0.21.2](v0.21.2.md) - 2026-03-08 - Trimming-safe factory registration for all factory types - [v0.21.1](v0.21.1.md) - 2026-03-08 - Fix [DynamicDependency] trimming on multi-overload factories - [v0.21.0](v0.21.0.md) - 2026-03-08 - [Remote] requires internal, trimming fixes diff --git a/docs/release-notes/v0.21.2.md b/docs/release-notes/v0.21.2.md index 8ad7c786..135fb4fc 100644 --- a/docs/release-notes/v0.21.2.md +++ b/docs/release-notes/v0.21.2.md @@ -3,7 +3,7 @@ layout: default title: "v0.21.2" description: "Release notes for Neatoo RemoteFactory v0.21.2" parent: Release Notes -nav_order: 1 +nav_order: 2 --- # v0.21.2 - Trimming-Safe Factory Registration diff --git a/docs/release-notes/v0.21.3.md b/docs/release-notes/v0.21.3.md new file mode 100644 index 00000000..e0fc5ff2 --- /dev/null +++ b/docs/release-notes/v0.21.3.md @@ -0,0 +1,43 @@ +--- +layout: default +title: "v0.21.3" +description: "Release notes for Neatoo RemoteFactory v0.21.3" +parent: Release Notes +nav_order: 1 +--- + +# v0.21.3 - Fix Record Deserialization with Reference Metadata + +**Release Date:** 2026-03-20 +**NuGet:** [Neatoo.RemoteFactory 0.21.3](https://nuget.org/packages/Neatoo.RemoteFactory/0.21.3) + +## Overview + +Fixes `ObjectWithParameterizedCtorRefMetadataNotSupported` errors when Interface Factory methods return record types with parameterized constructors. The serializer's global `ReferenceHandler.Preserve` added `$id`/`$ref` metadata to all types, which System.Text.Json cannot deserialize on types with parameterized constructors (such as C# records with primary constructors). + +## What's New + +None + +## Breaking Changes + +- **JSON wire format change for non-Neatoo types.** Non-Neatoo types (plain records, DTOs, primitives, collections) are no longer serialized with `$id`/`$ref` reference metadata. This changes the wire format but fixes deserialization failures. Neatoo types (`[Factory]`-decorated classes and records, interface-typed domain objects) continue to use reference handling as before. + +## Bug Fixes + +- **Fixed Interface Factory methods returning records with primary constructors.** `NeatooJsonSerializer` applied `ReferenceHandler.Preserve` globally, causing System.Text.Json to emit `$id`/`$ref` metadata for all types. When an Interface Factory method returned a plain record (no `[Factory]` attribute), the client-side deserialization failed with `ObjectWithParameterizedCtorRefMetadataNotSupported` because STJ cannot resolve `$ref` metadata and populate constructor parameters simultaneously. The fix introduces dual `JsonSerializerOptions` -- Neatoo types (which handle `$id`/`$ref` manually via custom converters) keep reference handling, while plain records and DTOs get clean options without reference metadata. + +## Migration Guide + +No code changes required. If you previously worked around this issue by switching from records to classes for Interface Factory return types, you can now switch back to records. + +**Anti-pattern:** Do not mix Neatoo domain types (`IValidateBase`, `IValidateListBase`) with plain records in a single Interface Factory return type. Use either full Neatoo entities or pure DTOs/records. + +## Commits + +- Add dual `JsonSerializerOptions` to `NeatooJsonSerializer` -- Neatoo types use reference handling, non-Neatoo types use clean options +- Add `IsNeatooType(Type)` classification method for serialization option selection +- Add 6 integration tests for Interface Factory record return types (simple, nested, collection, nullable, JSON inspection) + +**Related:** +- [Record Deserialization Fix Plan](../plans/record-deserialization-ref-metadata.md) diff --git a/docs/serialization.md b/docs/serialization.md index 5327ff7d..c4278c8b 100644 --- a/docs/serialization.md +++ b/docs/serialization.md @@ -117,6 +117,12 @@ The first occurrence gets a `$id`, and subsequent references use `$ref` pointers This handles circular references and ensures the deserialized graph has the same object identity as the original. +### Scope: Neatoo Types Only + +Reference preservation (`$id`/`$ref` metadata) applies only to Neatoo types -- classes and records decorated with `[Factory]` that implement `IOrdinalSerializable`, and interface/abstract types registered in the factory assembly. Plain records and DTOs returned from Interface Factory methods are serialized without reference handling, using standard System.Text.Json behavior. This means plain records/DTOs do not support circular references, but they do support parameterized constructors (primary constructors) without issue. + +Do not mix Neatoo domain types with plain records in the same return type. A record containing an `IValidateBase` property creates a serialization mismatch -- the record is serialized without reference handling, but the embedded Neatoo type expects it. Use either pure Neatoo types or pure records/DTOs. + ## Debugging Enable verbose logging to trace serialization issues: diff --git a/docs/todos/completed/record-deserialization-ref-metadata.md b/docs/todos/completed/record-deserialization-ref-metadata.md new file mode 100644 index 00000000..57c1687d --- /dev/null +++ b/docs/todos/completed/record-deserialization-ref-metadata.md @@ -0,0 +1,181 @@ +# Record Deserialization Fails with $ref Metadata + +**Status:** Complete +**Priority:** High +**Created:** 2026-03-20 +**Last Updated:** 2026-03-20 + + +--- + +## Problem + +`NeatooJsonSerializer` uses `ReferenceHandler.Preserve`, which adds `$id`/`$ref` metadata to JSON. System.Text.Json has a known limitation: it cannot deserialize types with parameterized constructors (like C# record types with primary constructors) when `$ref` metadata appears in the payload. + +**Error:** `ObjectWithParameterizedCtorRefMetadataNotSupported` + +This surfaces when an Interface Factory method returns a record type — the serializer adds reference metadata, and client-side deserialization fails. + +**Minimal repro structure:** + +```csharp +// A record with a primary constructor — the problem type +public record MyResult( + string Name, + IReadOnlyList Items); + +public record MyItem(int Id, string Value); + +// Interface Factory that returns the record +[Factory] +public interface IMyService +{ + Task GetDataAsync(); +} +``` + +When `GetDataAsync` returns through RemoteFactory, the serializer adds `$id`/`$ref` metadata. On deserialization, System.Text.Json encounters `$ref` metadata on `Items` — it can't resolve the reference AND populate the constructor parameter at the same time, so it throws. + +**Root cause:** `NeatooJsonSerializer` applies `ReferenceHandler.Preserve` globally via `NeatooReferenceHandler`. Neatoo types (`IValidateBase`, `IValidateListBase`) handle `$id`/`$ref` manually in their custom converters. Plain records/DTOs fail the `CanConvert` check in `NeatooBaseJsonConverterFactory` and fall back to STJ built-in handling, which has the parameterized constructor limitation. + +## Solution + +To be determined by architect. Possible directions: +- Modify NeatooJsonSerializer to handle non-Neatoo types (records/DTOs) differently — e.g., skip `ReferenceHandler.Preserve` for types that don't need reference tracking +- Strip `$id`/`$ref` metadata for non-Neatoo types during deserialization +- Document as a known limitation with guidance to avoid records as Interface Factory return types + +--- + +## Clarifications + +**Q1 (Architect):** Is this a user-reported issue, or a proactive discovery? Do you have a specific repro or error trace? + +**A1:** User-reported. They had to work around it by switching from records to classes. + +**Q2 (Architect):** What types need reference preservation? Only Neatoo domain types, or could users have circular references in their DTOs/records? + +**A2:** OK with not handling circular references for DTOs/records. Draw the line at Neatoo types only. However, the architect should assess difficulty of supporting circular refs in DTOs — if trivial, worth considering; if not, skip. + +**Q3 (Architect):** Can Interface Factory return types mix records with Neatoo domain types (e.g., a record containing an `IValidateBase` property)? + +**A3:** No. Label that an anti-pattern. They are either using Neatoo incorrectly (not DDD, not an aggregate) or can just use RemoteFactory directly. + +**Q4 (Architect):** Would it be acceptable to change the JSON wire format for non-Neatoo types (removing `$id`/`$ref`)? + +**A4:** Fully breaking change tolerant. Still v0. + +**Q5 (Architect):** Does ordinal format also have this problem? + +**A5:** Not applicable. Records/DTOs don't implement `IOrdinalSerializable`, so they never use ordinal format. The problem is only in the default STJ fallback path. + +Architect confirmed **Ready** after these clarifications. + +--- + +## Requirements Review + +**Reviewer:** business-requirements-reviewer +**Reviewed:** 2026-03-20 +**Verdict:** APPROVED + +### Relevant Requirements Found + +1. **Serialization Architecture (NeatooJsonSerializer):** The serializer applies `ReferenceHandler.Preserve` globally via `NeatooReferenceHandler` (`src/RemoteFactory/Internal/NeatooJsonSerializer.cs:70`). This is the root cause -- `$id`/`$ref` metadata is emitted for ALL types, including non-Neatoo records/DTOs. + +2. **Custom Converter Chain:** Three converter factories are registered in order: `NeatooOrdinalConverterFactory` (for `IOrdinalSerializable` types), then the generated `NeatooJsonConverterFactory` subclasses (including `NeatooInterfaceJsonConverterFactory` for interface-typed properties). Types that do not match any custom converter fall through to STJ built-in handling, which is where records with parameterized constructors encounter the `$ref` incompatibility. + +3. **NeatooJsonTypeInfoResolver (`src/RemoteFactory/Internal/NeatooJsonTypeInfoResolver.cs`):** Overrides `CreateObject` for DI-registered types. This is relevant because records with primary constructors use STJ's constructor-based deserialization, which conflicts with `ReferenceHandler.Preserve` at the STJ engine level -- the resolver cannot bypass that limitation. + +4. **Interface Factory Return Types:** The Design source of truth (`src/Design/Design.Domain/FactoryPatterns/AllPatterns.cs:204-220`) shows Interface Factory methods returning plain DTOs (`ExampleDto`, a class with public setters) and `IReadOnlyList`. The `ExampleDto` class is NOT a Neatoo type -- it has no `[Factory]` attribute, no `IOrdinalSerializable`, and no custom converter. This establishes the pattern that Interface Factory return types can be arbitrary non-Neatoo types. + +5. **Record Support (Completed Todo: `docs/todos/completed/record-support-plan.md`):** v10.1.0 added record support with `[Factory]` on records. Records with `[Factory]` get `IOrdinalSerializable` generated code and custom converters. This todo's problem is specifically about records WITHOUT `[Factory]` (plain DTOs returned from Interface Factories), which fall through to STJ built-in handling. + +6. **Serialization Round-Trip Guide (`src/Design/Design.Tests/FactoryTests/SerializationTests.cs:22-43`):** Documents which types serialize correctly. Records are listed under "YES" (line 27). However, this refers to `[Factory]`-decorated records (like `Money`). Plain record DTOs returned from Interface Factories are not explicitly addressed. + +7. **Reference Preservation Documented Behavior (`docs/serialization.md:89-118`):** Published docs state that reference preservation handles "shared instance identity" and "circular references." The docs describe this as handling Neatoo domain object graphs (parent-child bidirectional references), not arbitrary user DTOs. + +8. **HandleRemoteDelegateRequest Response Serialization (`src/RemoteFactory/HandleRemoteDelegateRequest.cs:126-141`):** The server serializes the response using `serializer.Serialize(result, returnType)`, where `returnType` is extracted from the delegate's return type. This passes through `NeatooJsonSerializer.Serialize`, which always sets up the `NeatooReferenceHandler` -- meaning `$id`/`$ref` metadata is always emitted. + +9. **Design Debt -- IEnumerable serialization (`src/Design/CLAUDE-DESIGN.md:691`):** "Only concrete collections. Type preservation complexity. User demand for interface collections." This is tangentially related but not directly blocking -- the todo is about records in collections, not about interface-typed collections themselves. + +10. **Breaking Change Tolerance (Clarification A4):** The user confirmed full breaking change tolerance since the project is still v0. This removes any wire-format backward-compatibility constraint. + +### Gaps + +1. **No documented requirement for non-Neatoo type serialization behavior:** The Design source of truth demonstrates Interface Factory methods returning `ExampleDto` (a class), but there is no test or documented contract for what happens when an Interface Factory returns a record with a parameterized constructor. The existing tests use `ExampleDto` which is a class with a default constructor and public setters -- this sidesteps the `$ref` issue entirely. + +2. **No documented anti-pattern for mixing Neatoo types with records in return types:** Clarification A3 establishes a new anti-pattern: records containing `IValidateBase` properties should not be used as Interface Factory return types. This anti-pattern does not currently exist in `CLAUDE-DESIGN.md` or the Design projects. The architect should document it. + +3. **No requirement covering circular reference handling scope:** The published docs (`docs/serialization.md`) describe reference preservation for Neatoo domain objects but do not state whether it applies to all types or only Neatoo types. The architect needs to establish the boundary: reference preservation for Neatoo types only (per Clarification A2), with no circular reference support for plain DTOs/records. + +4. **No Design project example of Interface Factory returning a record:** `AllPatterns.cs` uses `ExampleDto` (a class) as the return type. After this fix, the Design projects should include an Interface Factory method returning a record type to demonstrate the supported pattern. + +### Contradictions + +None found. This todo does not contradict any documented pattern, anti-pattern, or design debt decision. + +- The Design Debt table entries are not in conflict. The `IEnumerable serialization` debt item is about interface-typed collections, not about record deserialization. +- No anti-pattern is violated. The proposed change fixes an unintended limitation rather than implementing a deliberately deferred feature. +- The todo's direction (making non-Neatoo types skip `ReferenceHandler.Preserve` behavior) is consistent with the documented architecture: Neatoo types have custom converters that handle `$id`/`$ref` manually, while non-Neatoo types should fall through to standard STJ behavior without the reference metadata that breaks parameterized constructors. + +### Recommendations for Architect + +1. **Scope the fix to the serializer layer only.** The converter chain is well-structured: Neatoo types (IOrdinalSerializable, interface-typed properties) have custom converters that handle `$id`/`$ref` manually. The fix should ensure non-Neatoo types are serialized without `ReferenceHandler.Preserve` interference, or that `$ref` metadata is stripped/skipped for types that cannot handle it (parameterized constructors). + +2. **Preserve reference handling for Neatoo types.** The `NeatooInterfaceJsonTypeConverter` (`src/RemoteFactory/Internal/NeatooInterfaceJsonTypeConverter.cs:44-48`) explicitly manages `$id`/`$ref` for interface-typed properties. The ordinal converter writes arrays, bypassing reference metadata entirely. Any solution must not break these existing converters. + +3. **Add a Design project example.** After implementing the fix, add an Interface Factory method that returns a record type to `AllPatterns.cs` (or a new file), and add a corresponding test to `Design.Tests/FactoryTests/InterfaceFactoryTests.cs` demonstrating the record return type pattern. + +4. **Document the new anti-pattern (Clarification A3).** Add to `CLAUDE-DESIGN.md` Anti-Patterns section: records containing Neatoo domain types (`IValidateBase`, etc.) as properties should not be used as Interface Factory return types. They should either be full Neatoo entities or pure DTOs/records. + +5. **Test with the two DI container pattern.** The fix must be validated using `ClientServerContainers.Scopes()` to ensure the full client-to-server-to-client round-trip works for record return types. Test both simple records and records containing collections (the exact scenario from the repro). + +6. **Consider the HandleRemoteDelegateRequest response path.** The server-side response serialization (`HandleRemoteDelegateRequest.cs:141`) calls `serializer.Serialize(result, returnType)`. If the solution involves type-specific serialization options, this is the key integration point where the return type is known and the serialization strategy can be varied. + +7. **Multi-targeting.** Verify the fix works on both net9.0 and net10.0. The STJ `$ref` limitation with parameterized constructors exists in both versions. + +--- + +## Plans + +- [Fix Record Deserialization with $ref Metadata](../plans/record-deserialization-ref-metadata.md) + +--- + +## Tasks + +- [x] Architect comprehension check (Step 2) +- [x] Business requirements review (Step 3) +- [x] Architect plan creation & design (Step 4) +- [x] Developer review (Step 5) — Approved +- [x] Implementation (Step 7) +- [x] Verification (Step 8) — Architect VERIFIED, Requirements SATISFIED +- [x] Documentation (Step 9) — Requirements docs, Design project examples, skill updates, release notes (v0.21.3) + +--- + +## Progress Log + +### 2026-03-20 +- Todo created (fresh start). Problem: NeatooJsonSerializer's global ReferenceHandler.Preserve causes STJ to fail deserializing records with parameterized constructors when $ref metadata is present. +- Architect plan created: dual-options approach. NeatooJsonSerializer maintains two JsonSerializerOptions instances -- one with ReferenceHandler for Neatoo types, one without for plain records/DTOs. Type classification via IsNeatooType(). Plan linked: `docs/plans/record-deserialization-ref-metadata.md` + +--- + +## Completion Verification + +Before marking this todo as Complete, verify: + +- [x] All builds pass +- [x] All tests pass + +**Verification results:** +- Build: 0 errors (net9.0 + net10.0) +- Tests: UnitTests 490x2, IntegrationTests 487x2 (3 skipped pre-existing), Design.Tests 42x2 — 0 failures + +--- + +## Results / Conclusions + +Fixed by introducing dual `JsonSerializerOptions` in `NeatooJsonSerializer`. Neatoo types (`IOrdinalSerializable`, interface/abstract types in registered assemblies) continue using `ReferenceHandler.Preserve`. Non-Neatoo types (records, DTOs, primitives) use clean options without `$id`/`$ref` metadata, eliminating the `ObjectWithParameterizedCtorRefMetadataNotSupported` error for records with parameterized constructors. New anti-pattern documented: mixing Neatoo domain types with records in Interface Factory return types. Version bumped to v0.21.3. diff --git a/skills/RemoteFactory/references/anti-patterns.md b/skills/RemoteFactory/references/anti-patterns.md index a780dddd..1a65bc33 100644 --- a/skills/RemoteFactory/references/anti-patterns.md +++ b/skills/RemoteFactory/references/anti-patterns.md @@ -346,6 +346,34 @@ internal partial class PersonPhoneList --- +## 14. Mixing Neatoo Types with Records in Interface Factory Return Types + +**Problem**: Serialization mismatch -- Neatoo types use custom converters with reference handling, while plain records use standard STJ. Mixing them in a single return type causes deserialization failures. + +```csharp +// WRONG - record containing a Neatoo domain type +public record OrderSummary(int Id, IOrder Order); // IOrder is a Neatoo type! + +[Factory] +public interface IOrderQueryService +{ + Task GetSummaryAsync(int id); // Will fail at deserialization +} + +// RIGHT - pure record/DTO return types +public record OrderSummary(int Id, string CustomerName, decimal Total); + +[Factory] +public interface IOrderQueryService +{ + Task GetSummaryAsync(int id); // Works correctly +} +``` + +**Why**: Plain records are serialized without `$id`/`$ref` reference handling metadata. Neatoo domain types require reference handling for their object graphs. Combining both in a single return type creates a serialization mismatch. Use pure records/DTOs or pure Neatoo types, not both. + +--- + ## Summary Table | Anti-Pattern | Problem | Solution | @@ -363,3 +391,4 @@ internal partial class PersonPhoneList | [Remote] on public methods | NF0105 diagnostic | Change method to `internal` | | Optional parameters | Defaults silently dropped | Don't use defaults; pass all args explicitly | | CS0051 fear on internal classes | Avoids `internal` unnecessarily | Internal classes can take internal services even in public methods | +| Mixing Neatoo types with records | Serialization mismatch | Use pure records/DTOs or pure Neatoo types, not both | diff --git a/skills/RemoteFactory/references/interface-factory.md b/skills/RemoteFactory/references/interface-factory.md index 22595e3d..fa50e67d 100644 --- a/skills/RemoteFactory/references/interface-factory.md +++ b/skills/RemoteFactory/references/interface-factory.md @@ -109,6 +109,26 @@ builder.Services.RegisterMatchingName(); // Auto-finds Emp --- +## Record Return Types + +Interface Factory methods can return plain records with primary constructors. Records do not need `[Factory]` -- they are serialized as standard JSON without reference handling metadata. + +```csharp +// Plain record -- no [Factory] attribute needed +public record EmployeeRecord(int Id, string Name, string Department); + +[Factory] +public interface IEmployeeQueryService +{ + Task GetByIdAsync(int id); + Task> SearchAsync(string query); +} +``` + +**Do not mix Neatoo domain types with records in return types.** A record returned from an Interface Factory must be a pure DTO -- it should not contain properties of type `IValidateBase` or other Neatoo interfaces. Use pure records/DTOs or pure Neatoo types, not both. + +--- + ## When to Use Interface Factory - **Remote services without entity identity** - Query services, report generators diff --git a/src/Design/CLAUDE-DESIGN.md b/src/Design/CLAUDE-DESIGN.md index 3f0439fa..c745e874 100644 --- a/src/Design/CLAUDE-DESIGN.md +++ b/src/Design/CLAUDE-DESIGN.md @@ -150,6 +150,7 @@ public static partial class MyCommands | Can I store method-injected services? | Only if using constructor injection | `AllPatterns.cs:86-96` | Fields lost after serialization | | Which authorization approach? | `[AuthorizeFactory]` for domain-specific rules; `[AspAuthorize]` for ASP.NET Core policies | `AuthorizedOrder.cs`, `SecureOrder.cs` | AuthorizeFactory gives client-side Can* methods; AspAuthorize leverages existing ASP.NET Core policies | | Does Can* inherit guard from the factory method? | No -- Can* derives guard from the auth class methods | `AuthorizedOrder.cs`, `AuthorizedOrderAuth.cs` | Can* calls auth methods, not the factory method; auth method accessibility determines Can* behavior | +| Can Interface Factory return a record? | Yes, plain records/DTOs without Neatoo types | `AllPatterns.cs` | Records are serialized without `$id`/`$ref`; do not mix Neatoo types into record properties | --- @@ -374,6 +375,51 @@ internal partial class Order : IOrder --- +### Anti-Pattern 9: Mixing Neatoo Types with Records in Interface Factory Return Types + +**WRONG:** +```csharp +// A record that contains a Neatoo domain type as a property +public record OrderSummary( + string CustomerName, + IOrder ActiveOrder); // WRONG: Neatoo domain type inside a plain record + +[Factory] +public interface IOrderService +{ + Task GetSummaryAsync(int customerId); +} +``` + +**RIGHT (Option A - Use Neatoo types for the entire graph):** +```csharp +// If you need Neatoo domain types, return them directly +[Factory] +public interface IOrderService +{ + Task GetOrderAsync(int orderId); +} +``` + +**RIGHT (Option B - Use plain DTOs/records throughout):** +```csharp +// If you need a record return type, use only plain data -- no Neatoo types +public record OrderSummary( + string CustomerName, + string Status, + decimal Total); // All plain data, no Neatoo types + +[Factory] +public interface IOrderService +{ + Task GetSummaryAsync(int customerId); +} +``` + +**Why it matters:** Interface Factory return types that are plain records/DTOs are serialized without reference handling (`$id`/`$ref` metadata). Neatoo types require reference handling for their custom converters to work. Mixing the two in a single return type creates a serialization mismatch: the record is serialized without reference handling, but the embedded Neatoo type expects it. Use either pure Neatoo types (with `[Factory]`) or pure records/DTOs -- not a mix. + +--- + ## Critical Rules ### 1. [Remote] is ONLY for Aggregate Root Entry Points @@ -675,6 +721,7 @@ When reviewing or extending the Design source of truth, verify these patterns ar - [x] CorrelationContext usage (`CorrelationExample.cs`) - [ ] ASP.NET Core policy-based authorization (`SecureOrder.cs`) - [x] Custom domain authorization with [AuthorizeFactory] (`AuthorizedOrder.cs`, `AuthorizedOrderAuth.cs`) +- [x] Interface Factory returning a record type (`AllPatterns.cs`: `ExampleRecordResult` record, `IExampleRepository.GetRecordByIdAsync`) --- @@ -703,6 +750,7 @@ These are known limitations or open questions. They are documented here to preve 7. **Missing partial keyword** - Generator needs to extend your class 8. **Missing CancellationToken on events** - Required for graceful shutdown 9. **[Remote] on public methods** - `[Remote]` requires `internal` for IL trimming. `[Remote] public` emits NF0105. Change to `internal`. +10. **Mixing Neatoo types with records in Interface Factory return types** - Records containing Neatoo domain types (e.g., `IValidateBase`) as properties cause serialization mismatches. Use pure records/DTOs or pure Neatoo types, not both. --- diff --git a/src/Design/Design.Domain/FactoryPatterns/AllPatterns.cs b/src/Design/Design.Domain/FactoryPatterns/AllPatterns.cs index c5f926d6..677c1200 100644 --- a/src/Design/Design.Domain/FactoryPatterns/AllPatterns.cs +++ b/src/Design/Design.Domain/FactoryPatterns/AllPatterns.cs @@ -217,6 +217,16 @@ public interface IExampleRepository /// Fetches a single item by ID. /// Task GetByIdAsync(int id); + + /// + /// Fetches a single item by ID, returning a record with a primary constructor. + /// + /// + /// Demonstrates that Interface Factory methods can return plain records + /// (no [Factory] attribute). The record is serialized without $id/$ref + /// metadata, avoiding the System.Text.Json parameterized constructor limitation. + /// + Task GetRecordByIdAsync(int id); } // DID NOT DO THIS: Require [Remote] on interface methods @@ -262,6 +272,11 @@ public Task> GetAllAsync() { return Task.FromResult(new ExampleDto { Id = id, Name = $"Item {id}" }); } + + public Task GetRecordByIdAsync(int id) + { + return Task.FromResult(new ExampleRecordResult(id, $"Record {id}")); + } } // ============================================================================= @@ -473,10 +488,23 @@ public Task SendAsync(string recipient, string message) } /// -/// Simple DTO for data transfer. +/// Simple DTO for data transfer (class with public setters). /// public class ExampleDto { public int Id { get; set; } public string Name { get; set; } = string.Empty; } + +/// +/// Record DTO for data transfer (primary constructor, no [Factory] attribute). +/// +/// +/// Plain records work as Interface Factory return types. They are serialized +/// without $id/$ref metadata, so parameterized constructors are not blocked +/// by System.Text.Json's reference handling limitation. +/// +/// Do NOT add [Factory] here -- that would route serialization through the +/// ordinal converter. Plain records fall through to standard STJ handling. +/// +public record ExampleRecordResult(int Id, string Name); diff --git a/src/Design/Design.Tests/FactoryTests/InterfaceFactoryTests.cs b/src/Design/Design.Tests/FactoryTests/InterfaceFactoryTests.cs index 337770ac..0003d7b9 100644 --- a/src/Design/Design.Tests/FactoryTests/InterfaceFactoryTests.cs +++ b/src/Design/Design.Tests/FactoryTests/InterfaceFactoryTests.cs @@ -84,6 +84,38 @@ public async Task InterfaceFactory_GetByIdAsync_ReturnsSpecificItem() server.Dispose(); } + /// + /// Verifies interface factory can return a record with a primary constructor. + /// + /// + /// Records with primary constructors are serialized without $id/$ref metadata, + /// avoiding the System.Text.Json parameterized constructor limitation. + /// This test validates the full client-server round-trip for plain records. + /// + [Fact] + public async Task InterfaceFactory_GetRecordByIdAsync_ReturnsRecordFromServer() + { + // Arrange + var (client, server, _) = DesignClientServerContainers.Scopes( + configureServer: services => + { + services.AddScoped(); + }); + + var repository = client.GetRequiredService(); + + // Act - record round-trips through serialization + var result = await repository.GetRecordByIdAsync(42); + + // Assert + Assert.NotNull(result); + Assert.Equal(42, result.Id); + Assert.Equal("Record 42", result.Name); + + client.Dispose(); + server.Dispose(); + } + /// /// Verifies interface factory returns null correctly. /// @@ -159,4 +191,9 @@ public Task> GetAllAsync() { return Task.FromResult(null); } + + public Task GetRecordByIdAsync(int id) + { + return Task.FromResult(null); + } } diff --git a/src/Directory.Build.props b/src/Directory.Build.props index c8c0114e..6e9fda04 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -16,7 +16,7 @@ net9.0;net10.0 True 0.7.0 - 0.21.2 + 0.21.3 neatoo_icon.png LICENSE OO Domain Modeling C# .NET Blazor WPF ASP.NET CSLA diff --git a/src/RemoteFactory/Internal/NeatooJsonSerializer.cs b/src/RemoteFactory/Internal/NeatooJsonSerializer.cs index f4b42437..55af0ae1 100644 --- a/src/RemoteFactory/Internal/NeatooJsonSerializer.cs +++ b/src/RemoteFactory/Internal/NeatooJsonSerializer.cs @@ -34,6 +34,12 @@ public class NeatooJsonSerializer : INeatooJsonSerializer JsonSerializerOptions Options { get; } + /// + /// Options without ReferenceHandler for non-Neatoo types (records, DTOs, primitives). + /// Prevents $id/$ref metadata that breaks parameterized constructor deserialization. + /// + JsonSerializerOptions PlainOptions { get; } + private NeatooReferenceHandler ReferenceHandler { get; } = new NeatooReferenceHandler(); /// @@ -65,6 +71,8 @@ public NeatooJsonSerializer( this.serviceAssemblies = serviceAssemblies; this.logger = logger ?? NullLogger.Instance; + var converterFactoryList = neatooJsonConverterFactories.ToList(); + this.Options = new JsonSerializerOptions { ReferenceHandler = this.ReferenceHandler, @@ -79,13 +87,54 @@ public NeatooJsonSerializer( this.Options.Converters.Add(new NeatooOrdinalConverterFactory(serializationOptions)); } - foreach (var neatooJsonConverterFactory in neatooJsonConverterFactories) + foreach (var neatooJsonConverterFactory in converterFactoryList) { this.Options.Converters.Add(neatooJsonConverterFactory); } + + // Plain options: same converters and resolver, but no ReferenceHandler. + // Non-Neatoo types (records, DTOs) use these to avoid $id/$ref metadata + // that breaks parameterized constructor deserialization. + this.PlainOptions = new JsonSerializerOptions + { + TypeInfoResolver = neatooDefaultJsonTypeInfoResolver, + WriteIndented = serializationOptions.Format == SerializationFormat.Named, + IncludeFields = true + }; + + if (serializationOptions.Format == SerializationFormat.Ordinal) + { + this.PlainOptions.Converters.Add(new NeatooOrdinalConverterFactory(serializationOptions)); + } + + foreach (var neatooJsonConverterFactory in converterFactoryList) + { + this.PlainOptions.Converters.Add(neatooJsonConverterFactory); + } } + /// + /// Determines whether a type is a Neatoo type that requires ReferenceHandler.Preserve. + /// Neatoo types handle $id/$ref manually via custom converters. + /// Non-Neatoo types (records, DTOs, primitives) must not have $id/$ref metadata + /// because System.Text.Json cannot deserialize parameterized constructors with $ref. + /// + private bool IsNeatooType(Type type) + { + var underlying = Nullable.GetUnderlyingType(type) ?? type; + + // IOrdinalSerializable = [Factory]-decorated type with generated ordinal support + if (typeof(IOrdinalSerializable).IsAssignableFrom(underlying)) + return true; + + // Interface/abstract types registered in IServiceAssemblies = Neatoo interface factory types + if ((underlying.IsInterface || underlying.IsAbstract) && serviceAssemblies.HasType(underlying)) + return true; + + return false; + } + public string? Serialize(object? target) { if (target == null) @@ -93,16 +142,22 @@ public NeatooJsonSerializer( return null; } - var typeName = target.GetType().Name; + var targetType = target.GetType(); + var typeName = targetType.Name; logger.SerializingObject(typeName, this.Format); var sw = Stopwatch.StartNew(); try { - using var rr = new NeatooReferenceResolver(); - this.ReferenceHandler.ReferenceResolver.Value = rr; + var options = IsNeatooType(targetType) ? this.Options : this.PlainOptions; + + if (options == this.Options) + { + using var rr = new NeatooReferenceResolver(); + this.ReferenceHandler.ReferenceResolver.Value = rr; + } - var result = JsonSerializer.Serialize(target, this.Options); + var result = JsonSerializer.Serialize(target, options); sw.Stop(); logger.SerializedObject(typeName, sw.ElapsedMilliseconds); @@ -132,10 +187,15 @@ public NeatooJsonSerializer( var sw = Stopwatch.StartNew(); try { - using var rr = new NeatooReferenceResolver(); - this.ReferenceHandler.ReferenceResolver.Value = rr; + var options = IsNeatooType(targetType) ? this.Options : this.PlainOptions; - var result = JsonSerializer.Serialize(target, targetType, this.Options); + if (options == this.Options) + { + using var rr = new NeatooReferenceResolver(); + this.ReferenceHandler.ReferenceResolver.Value = rr; + } + + var result = JsonSerializer.Serialize(target, targetType, options); sw.Stop(); logger.SerializedObject(typeName, sw.ElapsedMilliseconds); @@ -162,10 +222,15 @@ public NeatooJsonSerializer( try { - using var rr = new NeatooReferenceResolver(); - this.ReferenceHandler.ReferenceResolver.Value = rr; + var options = IsNeatooType(typeof(T)) ? this.Options : this.PlainOptions; - var result = JsonSerializer.Deserialize(json, this.Options); + if (options == this.Options) + { + using var rr = new NeatooReferenceResolver(); + this.ReferenceHandler.ReferenceResolver.Value = rr; + } + + var result = JsonSerializer.Deserialize(json, options); sw.Stop(); logger.DeserializedObject(typeName, sw.ElapsedMilliseconds); @@ -194,10 +259,15 @@ public NeatooJsonSerializer( try { - using var rr = new NeatooReferenceResolver(); - this.ReferenceHandler.ReferenceResolver.Value = rr; + var options = IsNeatooType(type) ? this.Options : this.PlainOptions; + + if (options == this.Options) + { + using var rr = new NeatooReferenceResolver(); + this.ReferenceHandler.ReferenceResolver.Value = rr; + } - var result = JsonSerializer.Deserialize(json, type, this.Options); + var result = JsonSerializer.Deserialize(json, type, options); sw.Stop(); logger.DeserializedObject(typeName, sw.ElapsedMilliseconds); diff --git a/src/Tests/RemoteFactory.IntegrationTests/TestTargets/TypeSerialization/InterfaceFactoryRecordTargets.cs b/src/Tests/RemoteFactory.IntegrationTests/TestTargets/TypeSerialization/InterfaceFactoryRecordTargets.cs new file mode 100644 index 00000000..e4f44676 --- /dev/null +++ b/src/Tests/RemoteFactory.IntegrationTests/TestTargets/TypeSerialization/InterfaceFactoryRecordTargets.cs @@ -0,0 +1,97 @@ +using Neatoo.RemoteFactory; + +namespace RemoteFactory.IntegrationTests.TestTargets.TypeSerialization; + +// ============================================================================ +// Plain record types (no [Factory] attribute) returned by Interface Factories. +// These types fall through to STJ built-in serialization and previously failed +// with ObjectWithParameterizedCtorRefMetadataNotSupported when $id/$ref +// metadata was emitted by NeatooJsonSerializer's ReferenceHandler.Preserve. +// ============================================================================ + +/// +/// Simple record with a primary constructor for basic round-trip testing. +/// +public record InterfaceRecordResult(string Name, int Value); + +/// +/// Item record for collection-containing record tests. +/// +public record InterfaceRecordItem(int Id, string Description); + +/// +/// Record containing a collection of other records -- the minimal repro +/// for the $ref deserialization bug with parameterized constructors. +/// +public record InterfaceRecordWithCollection(string Name, IReadOnlyList Items); + +/// +/// Outer record containing a nested inner record. +/// +public record InterfaceNestedOuter(string Label, InterfaceNestedInner Child); + +/// +/// Inner record used as a nested property. +/// +public record InterfaceNestedInner(int Id); + +// ============================================================================ +// Interface Factory that returns plain record types. +// The generator creates a proxy implementation that serializes calls +// across the client-server boundary. +// ============================================================================ + +/// +/// Interface Factory returning plain record types (no [Factory] on the records). +/// Methods return records with parameterized constructors that previously failed +/// deserialization when $id/$ref metadata was present. +/// +[Factory] +public interface IRecordReturnService +{ + Task GetSimpleRecord(string name, int value); + Task GetRecordWithCollection(string name); + Task GetNestedRecord(string label, int childId); + Task GetNullableRecord(bool returnNull); +} + +// ============================================================================ +// Server-side implementation of the Interface Factory. +// Only registered in the server container. +// ============================================================================ + +/// +/// Server implementation that creates and returns plain record instances. +/// +public class RecordReturnService : IRecordReturnService +{ + public Task GetSimpleRecord(string name, int value) + { + return Task.FromResult(new InterfaceRecordResult(name, value)); + } + + public Task GetRecordWithCollection(string name) + { + var items = new List + { + new(1, "First"), + new(2, "Second"), + new(3, "Third") + }; + return Task.FromResult(new InterfaceRecordWithCollection(name, items)); + } + + public Task GetNestedRecord(string label, int childId) + { + return Task.FromResult(new InterfaceNestedOuter(label, new InterfaceNestedInner(childId))); + } + + public Task GetNullableRecord(bool returnNull) + { + if (returnNull) + { + return Task.FromResult(null); + } + return Task.FromResult(new InterfaceRecordResult("NotNull", 99)); + } +} diff --git a/src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/InterfaceFactoryRecordSerializationTests.cs b/src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/InterfaceFactoryRecordSerializationTests.cs new file mode 100644 index 00000000..047de9af --- /dev/null +++ b/src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/InterfaceFactoryRecordSerializationTests.cs @@ -0,0 +1,141 @@ +using Microsoft.Extensions.DependencyInjection; +using Neatoo.RemoteFactory; +using Neatoo.RemoteFactory.Internal; +using RemoteFactory.IntegrationTests.TestContainers; +using RemoteFactory.IntegrationTests.TestTargets.TypeSerialization; + +namespace RemoteFactory.IntegrationTests.TypeSerialization; + +/// +/// Tests that Interface Factory methods returning plain record types (no [Factory]) +/// complete the full client-server-client round-trip. These records have parameterized +/// constructors and previously failed with ObjectWithParameterizedCtorRefMetadataNotSupported +/// when $id/$ref metadata was emitted by NeatooJsonSerializer. +/// +public class InterfaceFactoryRecordSerializationTests +{ + private readonly IServiceScope _clientScope; + private readonly IServiceScope _serverScope; + private readonly IRecordReturnService _factory; + + public InterfaceFactoryRecordSerializationTests() + { + var (client, server, _) = ClientServerContainers.Scopes( + configureServer: services => + { + services.AddScoped(); + }); + + _clientScope = client; + _serverScope = server; + _factory = _clientScope.ServiceProvider.GetRequiredService(); + } + + // ============================================================================ + // Scenario 1: Simple record round-trip (Rules 1, 3) + // ============================================================================ + + [Fact] + public async Task InterfaceFactory_SimpleRecord_RoundTrip() + { + // Act - call through client, serializes to server and back + var result = await _factory.GetSimpleRecord("TestName", 42); + + // Assert - record deserialized with all properties intact + Assert.NotNull(result); + Assert.Equal("TestName", result.Name); + Assert.Equal(42, result.Value); + } + + // ============================================================================ + // Scenario 2: Record with collection round-trip (Rules 1, 3, 4) + // ============================================================================ + + [Fact] + public async Task InterfaceFactory_RecordWithCollection_RoundTrip() + { + // Act + var result = await _factory.GetRecordWithCollection("CollectionTest"); + + // Assert - record deserialized with collection containing all items + Assert.NotNull(result); + Assert.Equal("CollectionTest", result.Name); + Assert.NotNull(result.Items); + Assert.Equal(3, result.Items.Count); + Assert.Equal(1, result.Items[0].Id); + Assert.Equal("First", result.Items[0].Description); + Assert.Equal(2, result.Items[1].Id); + Assert.Equal("Second", result.Items[1].Description); + Assert.Equal(3, result.Items[2].Id); + Assert.Equal("Third", result.Items[2].Description); + } + + // ============================================================================ + // Scenario 3: Nested record round-trip (Rules 1, 3) + // ============================================================================ + + [Fact] + public async Task InterfaceFactory_NestedRecord_RoundTrip() + { + // Act + var result = await _factory.GetNestedRecord("OuterLabel", 7); + + // Assert - both records deserialized with all properties intact + Assert.NotNull(result); + Assert.Equal("OuterLabel", result.Label); + Assert.NotNull(result.Child); + Assert.Equal(7, result.Child.Id); + } + + // ============================================================================ + // Scenario 4: Nullable record returns null (Rule 5) + // ============================================================================ + + [Fact] + public async Task InterfaceFactory_NullableRecord_ReturnsNull() + { + // Act + var result = await _factory.GetNullableRecord(returnNull: true); + + // Assert - client receives null without error + Assert.Null(result); + } + + [Fact] + public async Task InterfaceFactory_NullableRecord_ReturnsValue() + { + // Act + var result = await _factory.GetNullableRecord(returnNull: false); + + // Assert - non-null record deserialized correctly + Assert.NotNull(result); + Assert.Equal("NotNull", result.Name); + Assert.Equal(99, result.Value); + } + + // ============================================================================ + // Scenario 8: JSON output for non-Neatoo types has no $id/$ref (Rule 1) + // ============================================================================ + + [Fact] + public void InterfaceFactory_NonNeatooType_NoRefMetadata() + { + // Arrange - get the serializer from the server container + var serializer = _serverScope.ServiceProvider.GetRequiredService(); + var record = new InterfaceRecordWithCollection( + "MetadataTest", + new List + { + new(1, "Item1"), + new(2, "Item2") + }); + + // Act - serialize via the serializer that now selects PlainOptions for non-Neatoo types + var json = serializer.Serialize(record); + + // Assert - JSON must not contain $id or $ref metadata + Assert.NotNull(json); + Assert.DoesNotContain("$id", json); + Assert.DoesNotContain("$ref", json); + } +}