From 217d63ca1fed03b78f00a790df488cc26a7ef6f7 Mon Sep 17 00:00:00 2001 From: KeithVoels Date: Sat, 21 Mar 2026 13:36:43 -0500 Subject: [PATCH] feat: shared reference handling for mutable types, bump to v0.23.0 Restores $id/$ref reference tracking for mutable reference types (Dictionary, List, plain classes) while keeping records safe from STJ's parameterized-constructor limitation. Two new components: - NeatooPreserveReferenceHandler: bridges STJ built-in converters to NeatooReferenceResolver.Current - RecordBypassConverterFactory: claims parameterized-constructor types, delegates to inner options without ReferenceHandler (DDD: records are value objects, identity doesn't matter) Both paths share the same NeatooReferenceResolver instance for cross-type reference identity. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../business-requirements-documenter.md | 78 ++- .../agents/business-requirements-reviewer.md | 70 ++- .claude/agents/remotefactory-architect.md | 56 +- .claude/agents/remotefactory-developer.md | 96 +++- .claude/agents/remotefactory-docs-writer.md | 44 +- docs/appendix/record-reference-handling.md | 152 ++++++ docs/appendix/serialization.md | 4 +- .../shared-reference-handling-plan.md | 472 ++++++++++++++++ .../architect.md | 182 +++++++ .../developer.md | 509 ++++++++++++++++++ .../requirements-documenter.md | 64 +++ .../requirements-reviewer.md | 75 +++ docs/release-notes/index.md | 1 + docs/release-notes/v0.22.0.md | 2 +- docs/release-notes/v0.23.0.md | 60 +++ docs/serialization.md | 16 +- ...red-reference-handling-non-custom-types.md | 242 +++++++++ src/Design/CLAUDE-DESIGN.md | 6 +- .../FactoryTests/SerializationTests.cs | 5 +- src/Directory.Build.props | 2 +- .../Internal/NeatooJsonSerializer.cs | 9 + .../NeatooPreserveReferenceHandler.cs | 41 ++ .../Internal/RecordBypassConverterFactory.cs | 134 +++++ .../SharedReferenceTargets.cs | 66 +++ .../SharedReferenceExplorationTests.cs | 359 ++++++++++++ .../TypeSerialization/SharedReferenceTests.cs | 262 +++++++++ 26 files changed, 2961 insertions(+), 46 deletions(-) create mode 100644 docs/appendix/record-reference-handling.md create mode 100644 docs/plans/completed/shared-reference-handling-plan.md create mode 100644 docs/plans/completed/shared-reference-handling-plan.memory/architect.md create mode 100644 docs/plans/completed/shared-reference-handling-plan.memory/developer.md create mode 100644 docs/plans/completed/shared-reference-handling-plan.memory/requirements-documenter.md create mode 100644 docs/plans/completed/shared-reference-handling-plan.memory/requirements-reviewer.md create mode 100644 docs/release-notes/v0.23.0.md create mode 100644 docs/todos/completed/shared-reference-handling-non-custom-types.md create mode 100644 src/RemoteFactory/Internal/NeatooPreserveReferenceHandler.cs create mode 100644 src/RemoteFactory/Internal/RecordBypassConverterFactory.cs create mode 100644 src/Tests/RemoteFactory.IntegrationTests/TestTargets/TypeSerialization/SharedReferenceTargets.cs create mode 100644 src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/SharedReferenceExplorationTests.cs create mode 100644 src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/SharedReferenceTests.cs diff --git a/.claude/agents/business-requirements-documenter.md b/.claude/agents/business-requirements-documenter.md index 96e37ea2..a4e5e55d 100644 --- a/.claude/agents/business-requirements-documenter.md +++ b/.claude/agents/business-requirements-documenter.md @@ -3,10 +3,10 @@ name: business-requirements-documenter description: | Use this agent to update RemoteFactory's business requirements documentation after a verified implementation is complete. Reads the plan's Business Requirements Context and Business Rules, compares to what was implemented, and updates the project's requirements docs (Design projects, CLAUDE-DESIGN.md, published docs) with new rules, changed rules, and resolved gaps. - This is the project-specific version that understands RemoteFactory's code-based requirements structure. It operates at Step 8 Part A of the project-todos workflow, after both architect verification and requirements verification have passed (Step 7). + This is the project-specific version that understands RemoteFactory's code-based requirements structure. It operates at Step 9 Part A of the project-todos workflow, after both architect verification (Step 8A) and requirements verification (Step 8B) have passed. - Context: The orchestrator is running the project-todos workflow. Step 7 has passed — both VERIFIED and REQUIREMENTS SATISFIED. A new factory attribute was added. The documenter needs to update CLAUDE-DESIGN.md, Design project comments, and published docs. + Context: The orchestrator is running the project-todos workflow. Step 8 has passed — both VERIFIED (8A) and REQUIREMENTS SATISFIED (8B). A new factory attribute was added. The documenter needs to update CLAUDE-DESIGN.md, Design project comments, and published docs. user: "Verification passed. Update the docs." assistant: "Both verifications confirmed. I'll invoke the business-requirements-documenter to update CLAUDE-DESIGN.md with the new attribute pattern, add it to the Design Completeness Checklist, and update the published attribute reference." @@ -104,15 +104,71 @@ When updating the quick reference, know what goes where: --- +## Agent Memory File + +Write all documentation tracking and deliverables to your agent memory file at `docs/plans/{plan-name}.memory/requirements-documenter.md`. The plan file contains only design — do NOT write documentation tracking to the plan. + +**Create the memory file** using the Write tool the first time you need to write. The directory is created automatically. + +**Do NOT read other agents' memory files.** The orchestrator relays cross-agent information (e.g., the developer's completion evidence, the reviewer's verification verdict) in your spawn prompt. + +### Memory File Structure + +```markdown +# Requirements Documenter — [Plan Name] + +Last updated: YYYY-MM-DD +Current step: [what this agent is doing or last did] + +## Key Context +[Curated summary — decisions, corrections, discoveries +that matter for the next fresh run of THIS agent] + +## Mistakes to Avoid +[Things this agent got wrong and was corrected on] + +## User Corrections +[Direct quotes/paraphrases of user overrides] + +## Documentation Tracking + +### Expected Deliverables +[List from the plan's acceptance criteria or known documentation needs] + +### Requirements Documentation Updated +| File | What Changed | Why | +|------|-------------|-----| +| [path] | [description] | [traced to which business rule] | + +### Developer Deliverables +[Source code changes the developer agent must make — the orchestrator routes these] +- [ ] [File path]: [What to change] — Reason: [Why] + +### Step 9 Part B Needed? +[State whether non-requirements documentation deliverables exist: release notes, README, migration guide, architecture docs. If none: "No general documentation deliverables identified — Step 9 Part B can be skipped."] +``` + +### Key Rules + +1. **Plan = shared design.** All agents read it. Contains ONLY design content. +2. **Memory = private notes.** Only this agent and the orchestrator read it. +3. **Never read other agents' memory files.** Orchestrator mediates. +4. **Create directory on first write.** The Write tool handles this automatically. +5. **Curated, not append-only.** Rewrite each run with only relevant content. + +--- + ## Process -### Step 1: Read the Plan +### Step 1: Read the Plan and Relayed Evidence Read the plan file to understand: 1. **Business Requirements Context** — what requirements existed before 2. **Business Rules (Testable Assertions)** — what the implementation satisfies. Note NEW vs traced assertions. -3. **Completion Evidence** — what was actually built -4. **Requirements Verification** — confirmation that requirements are satisfied. **If this section is absent or shows REQUIREMENTS VIOLATION, STOP immediately and report to the orchestrator.** + +Review the developer's **completion evidence** (relayed in your spawn prompt by the orchestrator — do NOT read `developer.md`) to understand what was actually built. + +The orchestrator confirms that both verifications passed before invoking you. **If the spawn prompt does not confirm VERIFIED and REQUIREMENTS SATISFIED, STOP immediately and report to the orchestrator.** ### Step 2: Categorize Changes @@ -139,24 +195,26 @@ For each business rule assertion in the plan: - Design Completeness Checklist item to check off - Skill reference-app code changes + `mdsnippets` run -### Step 4: Record Work in Plan +### Step 4: Record Work in Memory File -Update the plan's **Documentation** section: +Write all documentation tracking to your **agent memory file**: 1. List each requirements file created or updated with what changed 2. List each Developer Deliverable with specific instructions -3. Set plan status to **"Requirements Documented"** +3. State whether Step 9 Part B is needed (non-requirements documentation deliverables) +4. Set plan status to **"Requirements Documented"** ### Step 5: Report to Orchestrator Return a structured summary: - Files directly updated (with brief description of changes) - Developer Deliverables listed (source code changes the developer agent must make) -- **Step 8 Part B needed?** — State whether non-requirements documentation deliverables exist: +- **Step 9 Part B needed?** — State whether non-requirements documentation deliverables exist: - Release notes updates - README changes - Migration guide needed - Architecture docs updates - - If none: "No general documentation deliverables identified — Step 8 Part B can be skipped." + - If none: "No general documentation deliverables identified — Step 9 Part B can be skipped." +- Report: "Documentation tracking in my memory file at `docs/plans/{plan-name}.memory/requirements-documenter.md`" --- diff --git a/.claude/agents/business-requirements-reviewer.md b/.claude/agents/business-requirements-reviewer.md index b1be7352..6f5a96eb 100644 --- a/.claude/agents/business-requirements-reviewer.md +++ b/.claude/agents/business-requirements-reviewer.md @@ -4,11 +4,11 @@ description: | Use this agent to review existing business requirements documentation against a proposed todo or implementation plan for RemoteFactory. This is the project-specific version that understands RemoteFactory's code-based requirements (Design projects, tests, and published docs). Identifies relevant existing rules, patterns, anti-patterns, design debt decisions, gaps, and contradictions. Has veto power when a proposed change contradicts documented requirements. This agent operates in two modes: - 1. Pre-design review (Step 2): Analyze a todo against existing requirements before the architect begins - 2. Post-implementation verification (Step 7B): Confirm the implementation satisfies documented requirements + 1. Pre-design review (Step 3): Analyze a todo against existing requirements before the architect begins + 2. Post-implementation verification (Step 8B): Confirm the implementation satisfies documented requirements - Context: The orchestrator has created a todo for adding a new factory attribute. It is now at Step 2 (Business Requirements Review) and needs to check the todo against RemoteFactory's documented patterns before the architect begins. + Context: The orchestrator has created a todo for adding a new factory attribute. It is now at Step 3 (Business Requirements Review) and needs to check the todo against RemoteFactory's documented patterns before the architect begins. user: "I want to add a [RemoteValidate] attribute that generates validation endpoints" assistant: "The todo is created. Before the architect designs anything, I'll invoke the business-requirements-reviewer to check for contradictions with RemoteFactory's documented patterns, anti-patterns, and design decisions." @@ -17,7 +17,7 @@ description: | - Context: The architect and developer have completed work on a serialization change. The architect has independently verified all builds and tests pass (Step 7A: VERIFIED). The orchestrator must now run requirements verification (Step 7B). + Context: The architect and developer have completed work on a serialization change. The architect has independently verified all builds and tests pass (Step 8A: VERIFIED). The orchestrator must now run requirements verification (Step 8B). user: "Architect says builds and tests are all green." assistant: "Part A is verified. I'll invoke the business-requirements-reviewer for Part B — requirements verification against the Design projects and documented patterns." @@ -104,7 +104,7 @@ These are the most commonly relevant rules. Always check these against any propo --- -## Mode 1: Pre-Design Review (Step 2) +## Mode 1: Pre-Design Review (Step 3) ### Step 0: Check for an Existing Review @@ -179,14 +179,67 @@ Return a structured summary to the orchestrator. --- -## Mode 2: Post-Implementation Verification (Step 7B) +## Mode 2: Post-Implementation Verification (Step 8B) When invoked after the architect's technical verification (builds pass, tests pass), verify that the implementation respects RemoteFactory's documented requirements. +### Agent Memory File (Mode 2 Only) + +In Mode 2, write all verification findings to your agent memory file at `docs/plans/{plan-name}.memory/requirements-reviewer.md`. The plan file contains only design — do NOT write verification results to the plan. + +**Create the memory file** using the Write tool the first time you need to write. The directory is created automatically. + +**Do NOT read other agents' memory files.** The orchestrator relays cross-agent information (e.g., the developer's completion evidence) in your spawn prompt. + +#### Memory File Structure + +```markdown +# Requirements Reviewer — [Plan Name] + +Last updated: YYYY-MM-DD +Current step: [what this agent is doing or last did] + +## Key Context +[Curated summary — decisions, corrections, discoveries +that matter for the next fresh run of THIS agent] + +## Mistakes to Avoid +[Things this agent got wrong and was corrected on] + +## User Corrections +[Direct quotes/paraphrases of user overrides] + +## Requirements Verification + +**Verdict:** REQUIREMENTS SATISFIED | REQUIREMENTS VIOLATION +**Date:** YYYY-MM-DD + +### Compliance Table + +| # | Requirement | Source | Status | Notes | +|---|------------|--------|--------|-------| +| 1 | [Rule/pattern] | [File:location] | Satisfied/Violated | [Details] | + +### Unintended Side Effects +[Changes that technically work but alter behavior governed by other business rules] + +### Issues Found +[Specific violations with citations to Design projects or docs] +``` + +#### Key Rules + +1. **Plan = shared design.** All agents read it. Contains ONLY design content. +2. **Memory = private notes.** Only this agent and the orchestrator read it. +3. **Never read other agents' memory files.** Orchestrator mediates. +4. **Report verdict location.** Tell the orchestrator: "Verdict in my memory file at `docs/plans/{plan-name}.memory/requirements-reviewer.md`" + +**Note:** Mode 1 (pre-design review) is unchanged — it writes to the todo's Requirements Review section, which is not a plan section. + ### Process 1. Read the plan's **Business Requirements Context** section -2. Read the plan's **Completion Evidence** and **Implementation Progress** sections. Extract the list of modified files. **If no file list exists, STOP and report to the orchestrator.** +2. Review the developer's **completion evidence** (relayed in your spawn prompt by the orchestrator — do NOT read `developer.md`). Extract the list of modified files. **If no file list exists, STOP and report to the orchestrator.** 3. **Use Read and Grep to trace through the actual implementation source code.** Do not rely solely on the plan text. 4. For each requirement marked as relevant: - Trace through the implementation to verify it's satisfied @@ -196,7 +249,8 @@ When invoked after the architect's technical verification (builds pass, tests pa - Does the change affect serialization contracts? - Does the change affect the Design project tests? (They must still demonstrate correct patterns) - Does the change affect published docs accuracy? -6. Fill in the **Requirements Verification** section of the plan with the compliance table +6. **Write verification findings to your agent memory file** — compliance table, unintended side effects, issues found +7. Report to orchestrator: "Verdict in my memory file at `docs/plans/{plan-name}.memory/requirements-reviewer.md`" ### Verdict diff --git a/.claude/agents/remotefactory-architect.md b/.claude/agents/remotefactory-architect.md index 7ae8f2bf..8cd694e8 100644 --- a/.claude/agents/remotefactory-architect.md +++ b/.claude/agents/remotefactory-architect.md @@ -176,6 +176,50 @@ Once you have sufficient context: --- +## Agent Memory File + +When participating in the project-todos workflow, write all workflow state to your agent memory file at `docs/plans/{plan-name}.memory/architect.md`. The plan file contains only design — do NOT write verification results or workflow state to the plan. + +**Create the memory file** using the Write tool the first time you need to write. The directory is created automatically. + +**Do NOT read other agents' memory files.** The orchestrator relays cross-agent information in your spawn prompt. For example, the developer's completion evidence is included in your spawn prompt — do not open `developer.md`. + +### Memory File Structure + +```markdown +# Architect — [Plan Name] + +Last updated: YYYY-MM-DD +Current step: [what this agent is doing or last did] + +## Key Context +[Curated summary — decisions, corrections, discoveries +that matter for the next fresh run of THIS agent] + +## Mistakes to Avoid +[Things this agent got wrong and was corrected on] + +## User Corrections +[Direct quotes/paraphrases of user overrides] + +## Architectural Verification (Pre-Handoff) +[Written during Step 4/5 — scope table, evidence from verification resources, breaking changes identified] + +## Architect Verification (Post-Implementation) +[Written during Step 8A — verdict, test results, design match, test scenario coverage] +``` + +### Key Rules + +1. **Plan = shared design.** All agents read it. Contains ONLY design content. +2. **Memory = private notes.** Only this agent and the orchestrator read it. +3. **Never read other agents' memory files.** Orchestrator mediates. +4. **Create directory on first write.** The Write tool handles this automatically. +5. **Curated, not append-only.** Rewrite each run with only relevant content. +6. **Report verdict location.** Tell the orchestrator: "Verdict in my memory file at `docs/plans/{plan-name}.memory/architect.md`" + +--- + ## Core Responsibilities 1. **Architectural Vision**: Define and maintain the overall architecture of RemoteFactory, ensuring new features align with existing patterns and the project's philosophy of eliminating DTOs and manual factories @@ -420,13 +464,23 @@ When architectural analysis results in a concrete design: 3. Structure the plan with clear phases and acceptance criteria 4. Include test strategy using ClientServerContainers pattern 5. Reference any related todos +6. If verification resources exist (Design projects, sample projects), verify scope claims and write pre-handoff verification notes to your agent memory file ### Handoff to Developer When designs are ready for implementation: 1. Summarize the final architectural decision 2. List specific files to create/modify 3. Define acceptance criteria and test requirements -4. Explicitly recommend: "Use the `remotefactory-developer` agent for implementation" +4. Write pre-handoff verification notes (scope table, evidence, breaking changes) to your agent memory file — the orchestrator relays key findings to the developer +5. Explicitly recommend: "Use the `remotefactory-developer` agent for implementation" + +### Post-Implementation Verification (Step 8A) +When invoked to verify a completed implementation: +1. Review the developer's completion evidence (relayed in your spawn prompt — do NOT read `developer.md`) +2. **Independently run all builds and tests** — do NOT trust the developer's reported results +3. Cross-check every test scenario from the plan against actual passing tests +4. **Write verification verdict and evidence to your agent memory file** — not to the plan +5. Report to orchestrator: "Verdict in my memory file at `docs/plans/{plan-name}.memory/architect.md`" ### Returning Bug Analysis When bug diagnosis is complete: diff --git a/.claude/agents/remotefactory-developer.md b/.claude/agents/remotefactory-developer.md index 773dc082..d808720e 100644 --- a/.claude/agents/remotefactory-developer.md +++ b/.claude/agents/remotefactory-developer.md @@ -169,6 +169,57 @@ Once you have sufficient clarity: --- +## Agent Memory File + +When participating in the project-todos workflow, write all workflow state to your agent memory file at `docs/plans/{plan-name}.memory/developer.md`. The plan file contains only design — do NOT write reviews, contracts, progress, or evidence to the plan. + +**Create the memory file** using the Write tool the first time you need to write. The directory is created automatically. + +**Do NOT read other agents' memory files.** The orchestrator relays cross-agent information in your spawn prompt. For example, the architect's pre-handoff verification notes are included in your spawn prompt — do not open `architect.md`. + +**On resume:** Read your own memory file first for prior context, corrections, and mistakes to avoid. + +### Memory File Structure + +```markdown +# Developer — [Plan Name] + +Last updated: YYYY-MM-DD +Current step: [what this agent is doing or last did] + +## Key Context +[Curated summary — decisions, corrections, discoveries +that matter for the next fresh run of THIS agent] + +## Mistakes to Avoid +[Things this agent got wrong and was corrected on] + +## User Corrections +[Direct quotes/paraphrases of user overrides] + +## Developer Review +[Written during Step 5 — assertion trace table, concerns, verdict] + +## Implementation Contract +[Written during Step 6 — scope, out-of-scope, verification gates, stop conditions, test scenario mapping] + +## Implementation Progress +[Written during Step 7 — milestones, current state] + +## Completion Evidence +[Written when implementation is done — test results, contract status, test scenario mapping] +``` + +### Key Rules + +1. **Plan = shared design.** All agents read it. Contains ONLY design content. +2. **Memory = private notes.** Only this agent and the orchestrator read it. +3. **Never read other agents' memory files.** Orchestrator mediates. +4. **Create directory on first write.** The Write tool handles this automatically. +5. **Curated, not append-only.** Rewrite each run with only relevant content. + +--- + ## Test Preservation Is Sacred **Existing tests must never be "gutted" to make them pass.** What counts as gutting (NEVER do these to out-of-scope tests): @@ -192,12 +243,13 @@ Once you have sufficient clarity: 5. Wait for answers before proceeding ### Phase 2: Implementation Contract -After clarification, create a contract listing: +After clarification, **write the contract to your agent memory file**: - All files to be created/modified (with specific changes) - Tests to be added (with scenarios described) - Tests that must NOT be modified (out-of-scope) - Verification checkpoints - Rollback points +- If verification resources have failing acceptance criteria, list them in the contract Get user confirmation on the contract before implementing. @@ -209,6 +261,8 @@ Follow a checklist-driven approach: - [ ] Verify serialization round-trip for any new types - [ ] Add comprehensive tests using ClientServerContainers pattern - [ ] Run full test suite before marking complete +- [ ] **Write progress milestones to your agent memory file** as you complete them +- [ ] **Do NOT update documentation markdown** — skill markdown, user-facing docs, and release notes are handled by the documenter agent in Step 9. Code comments (XML docs) on modified code are in scope. ### Phase 4: Validation - [ ] All new tests pass @@ -216,6 +270,8 @@ Follow a checklist-driven approach: - [ ] Code follows existing patterns in the codebase - [ ] No reflection added without approval - [ ] Generated code (if any) is clean and follows conventions +- [ ] **Write Completion Evidence to your agent memory file** (test results, contract status, test scenario mapping) +- [ ] Set plan status to "Awaiting Verification", then **STOP** — do NOT mark the todo or plan as Complete --- @@ -341,12 +397,23 @@ When stopping to ask, provide: ### When Reviewing a Plan +**Write all review findings to your agent memory file** (assertion trace, concerns, verdict). The output below goes to the memory file's Developer Review section: + ```markdown -## Plan Review: [Plan Name] +## Developer Review + +**Status:** Approved | Concerns Raised +**Date:** YYYY-MM-DD ### Summary Brief description of what the plan intends to accomplish. +### Assertion Trace Verification + +| # | Business Rule | Implementation Path | Expected Result | Verified? | +|---|--------------|--------------------|-----------------|-----------| +| 1 | WHEN X, THEN Y | ClassName.Method — condition | value | Yes/No | + ### Gaps and Questions #### Critical (Must Answer Before Implementation) @@ -368,27 +435,32 @@ Brief description of what the plan intends to accomplish. ### When Starting Implementation +**Write the contract to your agent memory file's** Implementation Contract section: + ```markdown -## Implementation Contract: [Feature Name] +## Implementation Contract -### Files to Create -- `path/to/new/File.cs` - Description +### Scope +- `path/to/new/File.cs` - Description (create) +- `path/to/existing/File.cs` - What changes (modify) -### Files to Modify -- `path/to/existing/File.cs` - What changes +### Out of Scope +- `path/to/OutOfScope/Tests.cs` - Reason it's out of scope ### Tests to Add - `path/to/Tests.cs` - Test scenarios -### Tests NOT to Modify -- `path/to/OutOfScope/Tests.cs` - Reason it's out of scope +### Test Scenario Mapping +| # | Plan Scenario | Test Method | File | +|---|--------------|-------------|------| +| 1 | [Scenario name] | [TestMethodName] | [path] | -### Verification Checkpoints +### Verification Gates 1. After step X, run Y tests 2. After step Z, verify W -### Rollback Points -- If X fails, revert to Y +### Stop Conditions +- If X fails, STOP and report ``` --- diff --git a/.claude/agents/remotefactory-docs-writer.md b/.claude/agents/remotefactory-docs-writer.md index d690173b..f9987ddd 100644 --- a/.claude/agents/remotefactory-docs-writer.md +++ b/.claude/agents/remotefactory-docs-writer.md @@ -1,12 +1,12 @@ --- name: remotefactory-docs-writer description: | - Use this agent for general (non-requirements) documentation work on the RemoteFactory project: published Jekyll docs, release notes, README updates, migration guides, architecture docs, and getting-started content. This is the documentation agent for Step 8 Part B of the project-todos workflow. + Use this agent for general (non-requirements) documentation work on the RemoteFactory project: published Jekyll docs, release notes, README updates, migration guides, architecture docs, and getting-started content. This is the documentation agent for Step 9 Part B of the project-todos workflow. Does NOT handle business requirements documentation (that's the business-requirements-documenter). Does NOT handle skill files directly (those require MarkdownSnippets workflow through the developer agent). - Context: Step 8 Part A is complete. The documenter identified that docs/serialization.md needs a new section about the feature, and release notes need creating. + Context: Step 9 Part A is complete. The documenter identified that docs/serialization.md needs a new section about the feature, and release notes need creating. user: "Requirements docs are updated. The documenter says we need to update serialization docs and create release notes." assistant: "I'll invoke the remotefactory-docs-writer to update the serialization documentation and draft the release notes." @@ -119,7 +119,41 @@ Individual version files following the project's conventions. --- -## Process for Step 8 Part B +## Agent Memory File + +When participating in the project-todos workflow (Step 9 Part B), write documentation tracking to your agent memory file at `docs/plans/{plan-name}.memory/docs-writer.md`. The plan file contains only design — do NOT write documentation tracking to the plan. + +**Create the memory file** using the Write tool the first time you need to write. The directory is created automatically. + +**Do NOT read other agents' memory files.** The orchestrator relays cross-agent information in your spawn prompt. + +### Memory File Structure + +```markdown +# Docs Writer — [Plan Name] + +Last updated: YYYY-MM-DD +Current step: [what this agent is doing or last did] + +## Documentation Tracking + +### Files Updated +| File | What Changed | +|------|-------------| +| [path] | [description] | + +### Files Created +| File | Purpose | +|------|---------| +| [path] | [description] | + +### Deliverables Skipped (N/A) +[Any identified deliverables that turned out not to be needed, with reason] +``` + +--- + +## Process for Step 9 Part B When invoked as part of the project-todos workflow: @@ -171,9 +205,9 @@ Follow the release notes process from CLAUDE.md: 4. Update `docs/release-notes/index.md` (highlights table + all releases list) 5. Adjust nav_order for existing release pages -### Step 5: Record Work in Plan +### Step 5: Record Work in Memory File -Update the plan's **Documentation** section with what was written/updated. If this completes all documentation deliverables, note that Step 8 is complete. +Write documentation tracking to your **agent memory file** — list each file created or updated with what changed. If this completes all documentation deliverables, set plan status to "Documentation Complete." --- diff --git a/docs/appendix/record-reference-handling.md b/docs/appendix/record-reference-handling.md new file mode 100644 index 00000000..167429de --- /dev/null +++ b/docs/appendix/record-reference-handling.md @@ -0,0 +1,152 @@ +# Appendix: Record Reference Handling and Value Object Semantics + +This appendix explains **why** records bypass reference tracking (`$id`/`$ref`) during serialization and **how** RemoteFactory's three-path serialization architecture handles reference identity for different type categories. For user-facing serialization documentation, see [Serialization](../serialization.md). For serialization internals (type resolution, DI integration), see [Serialization Internals](serialization.md). + +## The Problem: Reference Tracking vs. Parameterized Constructors + +System.Text.Json's `ReferenceHandler.Preserve` adds `$id`/`$ref` metadata to track shared object identity across a serialization graph. This works for mutable types (classes with default constructors, dictionaries, lists), but **fails for types with parameterized constructors**: + +``` +System.NotSupportedException: + Deserialization of reference type 'MyRecord' with parameterized constructor + is not supported. (ObjectWithParameterizedCtorRefMetadataNotSupported) +``` + +STJ deserializes parameterized-constructor types by reading the entire JSON payload, then passing all values to the constructor at once. A `$ref` pointer requires the referenced object to already exist -- but it cannot exist yet because construction has not completed. Microsoft has closed this as NOT_PLANNED ([dotnet/runtime#73302](https://github.com/dotnet/runtime/issues/73302)). + +This creates a tension: RemoteFactory needs `ReferenceHandler` for shared-instance identity on mutable types, but cannot have it for records. + +## The DDD Resolution: Value Objects Have No Identity to Track + +The resolution is not a workaround -- it follows directly from DDD value object semantics. + +**Records are value objects.** A value object is defined by its values, not by its reference identity. Two `Address("123 Main", "Springfield")` instances with the same state are interchangeable. There is no meaningful distinction between "the same Address object" and "two Address objects with identical state." + +**Reference tracking is semantically wrong for value objects.** Tracking identity with `$id`/`$ref` implies the object's reference matters -- that the consumer needs to know "this is the same instance I saw earlier." For entities, that is true. For value objects, it is not. Serializing a record twice (one copy per property) produces the correct result: two independent value objects with identical state. + +**Nested reference types within a record are part of its state.** A `Dictionary` inside a record's constructor parameter is part of the value object's definition. If the same `Dictionary` instance is also referenced from an entity property elsewhere in the graph, the entity's reference participates in `$id`/`$ref` tracking (it has identity), while the record's copy is logically independent (it is state). `ReferenceEquals` between them returns `false` after round-trip. This is correct. + +```csharp +// The Dictionary is part of the value object's state definition +record ProductSpec(string Name, Dictionary Attributes); + +// The same Dictionary instance assigned to both an entity property and a record: +var attrs = new Dictionary { ["color"] = "red" }; +entity.Metadata = attrs; // Entity property -- tracked with $id/$ref +entity.Spec = new ProductSpec("Widget", attrs); // Value object -- independent copy + +// After round-trip: +// entity.Metadata participates in reference tracking (shared identity preserved) +// entity.Spec.Attributes is an independent copy (value object semantics) +// ReferenceEquals(entity.Metadata, entity.Spec.Attributes) is false -- correct +``` + +## Three-Path Serialization Architecture + +RemoteFactory routes types through three serialization paths based on which converter claims the type. All three paths share the same `NeatooReferenceResolver` instance (per-operation, via `AsyncLocal`), so reference IDs are unique across the entire graph. + +### Path 1: Neatoo Custom Converters (Entities and Lists) + +Types with Neatoo custom converters (`NeatooBaseJsonTypeConverter`, `NeatooListBaseJsonTypeConverter`) are serialized by Neatoo's converters. These converters access `NeatooReferenceResolver.Current` directly to emit `$id`/`$ref` metadata under their own control. + +- **Types:** Neatoo entities (`IEditBase`, `IValidateBase`, etc.) and Neatoo lists (`IEditListBase`, etc.) +- **Reference tracking:** Yes -- converter-level, using `NeatooReferenceResolver.Current` directly +- **Unchanged from v0.22.0** + +### Path 2: STJ Built-in Converters with ReferenceHandler (Mutable Types) + +Types without a custom converter and with a default (parameterless) constructor are handled by STJ's built-in converters. `NeatooPreserveReferenceHandler` is set on `JsonSerializerOptions`, which tells STJ to call `CreateResolver()` and use the returned resolver for `$id`/`$ref` tracking. + +`NeatooPreserveReferenceHandler.CreateResolver()` returns `NeatooReferenceResolver.Current` -- the same resolver instance that Neatoo's custom converters use. This means STJ's built-in converters and Neatoo's custom converters share a single ID space. + +- **Types:** `Dictionary`, `List`, plain classes with default constructors, other mutable reference types +- **Reference tracking:** Yes -- serializer-level, via `ReferenceHandler` on options delegating to `NeatooReferenceResolver.Current` +- **Handles:** Shared instances (same object in two properties), circular references (A.Next = B, B.Next = A) + +### Path 3: Record Bypass Converter (Value Objects) + +Types with parameterized constructors (no public parameterless constructor) are claimed by `RecordBypassConverterFactory`. This converter delegates serialization to an inner `JsonSerializerOptions` that is identical to the outer options except `ReferenceHandler` is `null` and `RecordBypassConverterFactory` is removed (to prevent recursion). + +The record and its entire subtree serialize without `$id`/`$ref` metadata. + +- **Types:** C# records with primary constructors, classes with only parameterized constructors +- **Reference tracking:** No -- records are value objects; identity is defined by values, not reference +- **Prevents:** STJ's `ObjectWithParameterizedCtorRefMetadataNotSupported` exception + +### Converter Priority + +STJ checks converters in registration order. The order in `NeatooJsonSerializer` is: + +1. `NeatooOrdinalConverterFactory` (if Ordinal format) -- claims `IOrdinalSerializable` types +2. Neatoo custom converter factories -- claim Neatoo entities and lists +3. `RecordBypassConverterFactory` -- claims types with parameterized constructors +4. STJ built-in converters -- handle everything else (with `ReferenceHandler` active) + +A type claimed by an earlier converter is never seen by later ones. Neatoo types are always handled by Neatoo converters (Path 1), regardless of whether they have parameterized constructors. `RecordBypassConverterFactory` only sees non-Neatoo types. + +### Reference Flow + +``` +NeatooJsonSerializer.Serialize() + | + +-- Creates NeatooReferenceResolver, sets .Current via AsyncLocal + | + +-- JsonSerializer.Serialize(target, OuterOptions) + | + +-- Neatoo entity/list property: + | Path 1: Neatoo converter reads .Current directly + | Emits $id/$ref under converter control + | + +-- Record property: + | Path 3: RecordBypassConverterFactory claims it + | Delegates to InnerOptions (ReferenceHandler = null) + | No $id/$ref in the record's subtree + | + +-- Dictionary/List/plain class property: + | Path 2: STJ built-in converter + | ReferenceHandler calls CreateResolver() -> .Current + | STJ emits $id/$ref using the shared resolver + | + +-- All three paths share the same resolver and ID counter + | + +-- Clears .Current in finally block +``` + +## Detection Rule: Parameterized Constructors + +`RecordBypassConverterFactory` uses a simple detection rule: claim any type that has no public parameterless constructor but has at least one public constructor with parameters. This matches STJ's own heuristic for parameterized-constructor deserialization. + +This rule is intentionally broad. It claims all C# records with primary constructors, but also non-record classes with only parameterized constructors. In the RemoteFactory ecosystem, such non-record classes are rare. The simpler rule avoids fragile heuristics that attempt to distinguish `record class Foo(int X)` from `class Foo(int X)` -- STJ cannot distinguish them either, and both have the same parameterized-constructor limitation. + +Types with **both** a parameterless and a parameterized constructor are NOT claimed. STJ uses the parameterless constructor for those types, so they work with `ReferenceHandler` and participate in `$id`/`$ref` tracking. + +## What This Means in Practice + +| Type Category | Reference Tracked? | Example | +|---|---|---| +| Neatoo entity | Yes (Path 1) | `Employee`, `IEditBase` implementations | +| Neatoo list | Yes (Path 1) | `EmployeeList`, `IEditListBase` implementations | +| Dictionary, List, plain class | Yes (Path 2) | `Dictionary`, `List`, `class Holder { ... }` | +| C# record | No (Path 3) | `record Address(string Street, string City)` | +| Class with only parameterized ctor | No (Path 3) | `class Foo { public Foo(int x) { } }` | +| Primitives, enums, strings | N/A | Not reference types; no tracking needed | + +**For most domain models, this works transparently.** Entities get reference tracking. Value objects (records) get correct value-based serialization. Collections and dictionaries shared across properties preserve identity. + +## The STJ Limitation in Detail + +System.Text.Json's `ReferenceHandler.Preserve` adds `$id` to the first occurrence of every reference type and `$ref` to subsequent occurrences. On deserialization, STJ resolves `$ref` pointers to previously-deserialized objects. + +For types with parameterized constructors, STJ must read all properties from the JSON before calling the constructor. But `$id`/`$ref` metadata must be processed during reading -- and the object does not exist until after construction. From [Microsoft's documentation](https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/preserve-references#immutable-types-and-records): + +> This feature can't be used to preserve value types or immutable types. On deserialization, the instance of an immutable type is created after the entire payload is read. So it would be impossible to deserialize the same instance if a reference to it appears within the JSON payload. + +This is not a bug to be fixed -- it is a fundamental constraint of how parameterized-constructor deserialization works. The `$id` token must be processed when the object is first encountered, but the object cannot be constructed until all its properties are read. STJ throws `NotSupportedException` rather than silently producing incorrect results. + +`RecordBypassConverterFactory` prevents this exception by removing `ReferenceHandler` from the options used for record serialization. Since records are value objects (no identity to track), this produces the correct result. + +## Related Documentation + +- [Serialization](../serialization.md) -- User-facing serialization documentation +- [Serialization Internals](serialization.md) -- Type resolution pipeline and DI integration +- [Client-Server Architecture](../client-server-architecture.md) -- How serialization fits in the remote call lifecycle diff --git a/docs/appendix/serialization.md b/docs/appendix/serialization.md index 22c15284..2b3f3d74 100644 --- a/docs/appendix/serialization.md +++ b/docs/appendix/serialization.md @@ -52,7 +52,9 @@ RemoteFactory's deserialization resolves instances from the DI container (`Servi ### 3. Shared Object Identity Is Lost -When two properties reference the same object instance (common in aggregate root / child entity relationships), STJ duplicates it — creating two independent copies. RemoteFactory preserves identity using `$id` / `$ref` pointers, maintaining the object graph structure across the wire. +When two properties reference the same object instance (common in aggregate root / child entity relationships), STJ duplicates it -- creating two independent copies. RemoteFactory preserves identity using `$id` / `$ref` pointers, maintaining the object graph structure across the wire. + +This applies to Neatoo types (via custom converters) and mutable reference types like `Dictionary`, `List`, and plain classes with default constructors (via `NeatooPreserveReferenceHandler` on `JsonSerializerOptions`). Types with parameterized constructors (records, immutable types) are intentionally excluded from reference tracking -- they are DDD value objects where duplication on round-trip is semantically correct. See [Appendix: Record Reference Handling](record-reference-handling.md) for the rationale. ## Type Resolution Pipeline diff --git a/docs/plans/completed/shared-reference-handling-plan.md b/docs/plans/completed/shared-reference-handling-plan.md new file mode 100644 index 00000000..895265d2 --- /dev/null +++ b/docs/plans/completed/shared-reference-handling-plan.md @@ -0,0 +1,472 @@ +# Shared Reference Handling for Non-Custom Types + +**Date:** 2026-03-20 +**Related Todo:** [Shared Reference Handling for Non-Custom Types](../todos/completed/shared-reference-handling-non-custom-types.md) +**Status:** Complete +**Last Updated:** 2026-03-21 + +--- + +## Overview + +RemoteFactory v0.22.0 moved reference handling (`$id`/`$ref`) to a converter-level concern, removing `ReferenceHandler` from `JsonSerializerOptions`. This means only types with custom converters (Neatoo entities/lists) participate in reference tracking. Non-custom types (Dictionary, List, plain classes) serialized by STJ's built-in converters lose shared-reference identity after round-trip. + +This plan addresses the gap in two phases: +1. **Phase 1 (Reproduction):** Create RemoteFactory-internal tests that reproduce all three problem scenarios -- shared Dictionary identity, records with parameterized constructors, and circular references in non-custom types -- using ONLY RemoteFactory's serialization (no Neatoo). **Phase 1 is complete.** It confirmed that custom `ReferenceHandler` works for mutable types, and that STJ's parameterized constructor limitation is permanent and extends to ANY `$id`/`$ref` metadata on reference-type constructor parameters. +2. **Phase 2 (Fix):** Implement a two-component solution: `NeatooPreserveReferenceHandler` (already built in Phase 1) wired into `NeatooJsonSerializer`'s options for mutable reference types, plus a new `RecordBypassConverterFactory` that claims types with parameterized constructors and delegates to inner options without `ReferenceHandler`. Records serialize without `$id`/`$ref` metadata entirely -- this is semantically correct because records are DDD value objects whose identity is defined by their values, not by reference. + +--- + +## Business Requirements Context + +**Source:** [Todo Requirements Review](../todos/shared-reference-handling-non-custom-types.md#requirements-review) + +### Published Documentation Promises + +- **`docs/serialization.md:10`:** "Shared instance identity -- When the same object is referenced by two properties (e.g., a parent-child bidirectional reference), System.Text.Json duplicates it. RemoteFactory tracks object identity and serializes shared references as `$ref` pointers, preserving the graph structure." This claim is NOT qualified as "Neatoo types only." The current implementation under-delivers on this published promise. + +- **`docs/appendix/serialization.md:53-55`:** "When two properties reference the same object instance (common in aggregate root / child entity relationships), STJ duplicates it -- creating two independent copies. RemoteFactory preserves identity using `$id` / `$ref` pointers, maintaining the object graph structure across the wire." Again, no qualification. + +- **`docs/client-server-architecture.md:3`:** "RemoteFactory lets you write your domain model as if it runs in a single process." Losing shared-identity for non-custom types violates this abstraction. User clarification A1 reinforces: "RemoteFactory is billing itself as abstracting away the client/server physical layer. We should try as hard as we can to do that." + +### Current Architecture (v0.22.0) + +- **`docs/serialization.md:120-124`:** v0.22.0 documents "Scope: Converter-Level, Not Serializer-Level." Plain records and DTOs have no custom converter, so they serialize without reference metadata. This is the documented current-state limitation that this todo proposes changing. + +- **`docs/release-notes/v0.22.0.md:16-17, 24`:** Confirms no `ReferenceHandler` on options is the current design. This was a deliberate breaking change from v0.21.3. + +- **Anti-Pattern 9 (`src/Design/CLAUDE-DESIGN.md:378-419`):** Technical explanation references the converter-level mechanism. If this todo restores `ReferenceHandler` on options, Anti-Pattern 9's explanation needs updating. The user-facing rule (do not mix Neatoo types with records) may or may not change. + +### STJ Limitation + +- **`ObjectWithParameterizedCtorRefMetadataNotSupported`:** STJ cannot deserialize types with parameterized constructors when `$ref` metadata appears in the payload. Microsoft docs confirm: "This feature can't be used to preserve value types or immutable types. On deserialization, the instance of an immutable type is created after the entire payload is read. So it would be impossible to deserialize the same instance if a reference to it appears within the JSON payload." Records with primary constructors fall into this category. + +### Infrastructure + +- **`NeatooReferenceResolver`** (`src/RemoteFactory/Internal/NeatooReferenceResolver.cs`): Already provides the full reference tracking API -- `GetReference`, `AddReference`, `ResolveReference` with `ReferenceEqualityComparer.Instance`. It extends STJ's `ReferenceResolver` base class and is accessible via `AsyncLocal`. + +- **`NeatooInterfaceJsonTypeConverter`** (`src/RemoteFactory/Internal/NeatooInterfaceJsonTypeConverter.cs`): Does NOT call `NeatooReferenceResolver` at all (dead code was removed in v0.22.0). Only handles `$type`/`$value` wrapping. + +### Existing Tests + +- **`InterfaceFactory_NonNeatooType_NoRefMetadata`** (`src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/InterfaceFactoryRecordSerializationTests.cs:121-140`): Asserts `Assert.DoesNotContain("$id", json)` and `Assert.DoesNotContain("$ref", json)` for a serialized record. This test guards against the v0.21.3 record bug. If the implementation adds `ReferenceHandler` globally, this test will fail. **Intent analysis:** The test's purpose is to verify records are not corrupted by `$id`/`$ref` -- not to assert that reference tracking is universally absent. If the new design can handle records without corruption, the test's core intent (records work) is preserved even if the assertion changes. + +- **Design project `SerializationTests.cs`** (`src/Design/Design.Tests/FactoryTests/SerializationTests.cs`): Seven tests verify round-trip for Create, Fetch, ValueObject, Collection, Nullable, Modified, SaveMeta. None test shared object identity. + +- **No RemoteFactory test infrastructure for shared-reference scenarios** exists. The only test is in the Neatoo repository (currently [Ignore]d). + +### Gaps + +1. No documented contract for what "shared instance identity" means for non-Neatoo types -- which types participate, cross-entity vs. within-entity scope. +2. No documented approach for handling the record/parameterized-constructor conflict alongside `ReferenceHandler`. +3. No RemoteFactory test infrastructure for shared-reference scenarios. +4. No documented requirement for circular reference handling in non-custom types. +5. No analysis of ordinal format interaction with reference metadata. + +### Contradictions + +No hard contradictions with the Design Debt table or documented anti-patterns. The key **tension** is with the v0.22.0 "converter-level, not serializer-level" principle. Setting `options.ReferenceHandler` would partially reverse v0.22.0's direction. The architect addresses this tension in the Approach section. + +--- + +## Business Rules (Testable Assertions) + +### Phase 1: Reproduction Rules (exploration -- these establish the baseline) + +1. WHEN a `Dictionary` is assigned to two properties of a plain class and serialized/deserialized through `NeatooJsonSerializer` (no `ReferenceHandler` on options), THEN the two properties after deserialization are NOT the same object instance (`ReferenceEquals` returns `false`). -- Source: Current behavior per v0.22.0 design (Finding 4). This rule documents the CURRENT broken state that Phase 1 must reproduce. + +2. WHEN `NeatooJsonSerializer` serializes a record with a parameterized constructor (current v0.22.0 -- no `ReferenceHandler`), THEN deserialization succeeds without error. -- Source: v0.22.0 release notes (Finding 13), test `InterfaceFactory_SimpleRecord_RoundTrip`. + +3. WHEN `options.ReferenceHandler = ReferenceHandler.Preserve` is set AND a record with a parameterized constructor is serialized/deserialized, THEN STJ throws `NotSupportedException` containing `ObjectWithParameterizedCtorRefMetadataNotSupported` on deserialization. -- Source: Original v0.21.3 bug (Finding 7). Phase 1 must reproduce this to confirm it is an inherent STJ limitation. + +4. WHEN a plain class has a circular reference (A.Child = B, B.Parent = A) and is serialized through `NeatooJsonSerializer` without `ReferenceHandler`, THEN serialization throws `JsonException` (stack depth exceeded) or produces invalid output. -- Source: `src/Design/Design.Tests/FactoryTests/SerializationTests.cs:38` ("Circular references without proper handling" listed as NO). Phase 1 must reproduce this. + +5. WHEN a `Dictionary` is assigned to two properties of a plain class AND `options.ReferenceHandler = ReferenceHandler.Preserve` is set, THEN after deserialization the two properties ARE the same object instance (`ReferenceEquals` returns `true`). -- Source: STJ `ReferenceHandler.Preserve` documented behavior. Phase 1 must confirm this works as the "happy path" when `ReferenceHandler` is present. + +6. WHEN `options.ReferenceHandler` uses a custom `ReferenceHandler` subclass that delegates to `NeatooReferenceResolver.Current`, THEN STJ's built-in converters for mutable reference types (Dictionary, List, plain classes) emit `$id`/`$ref` metadata AND shared identity is preserved on deserialization. -- Source: NEW. Phase 1 must confirm this approach works. + +### Phase 2: Solution Rules + +7. WHEN a mutable reference type (Dictionary, List, plain class with default/settable constructor) is assigned to two properties of any object in the same serialization graph AND serialized/deserialized through `NeatooJsonSerializer`, THEN after deserialization the two properties reference the same object instance (`ReferenceEquals` returns `true`). -- Source: Published docs promise (Finding 1, 2). NEW implementation requirement. + +8. WHEN a type with a parameterized constructor (records, classes with constructor parameters) is serialized/deserialized through `NeatooJsonSerializer`, THEN deserialization succeeds without error and all property values are preserved. The `RecordBypassConverterFactory` claims the type and delegates to inner options without `ReferenceHandler`, so no `$id`/`$ref` metadata appears in the JSON for these types. -- Source: v0.22.0 behavior preservation, existing test `InterfaceFactory_SimpleRecord_RoundTrip`. DDD justification: records are value objects; identity is defined by values, not reference. + +9. WHEN a mutable reference type has a circular reference (A.Child = B, B.Parent = A) AND is serialized/deserialized through `NeatooJsonSerializer`, THEN the circular reference is preserved after deserialization (`A.Child.Parent` is the same instance as `A`). -- Source: User clarification A5 ("Both [shared identity and circular references]"). NEW. + +10. WHEN a Neatoo type with a custom converter is serialized alongside non-custom types in the same object graph, THEN both Neatoo's converter-level `$id`/`$ref` AND STJ's built-in `$id`/`$ref` use the SAME `NeatooReferenceResolver` instance, so cross-type shared references are tracked correctly. -- Source: User clarification A2 ("Yes, at least start with that scope"). NEW. + +11. WHEN a type with a parameterized constructor appears in a graph with shared mutable references, THEN the `RecordBypassConverterFactory` claims the parameterized-constructor type and serializes it WITHOUT `$id`/`$ref` metadata. Mutable references elsewhere in the graph (outside the parameterized-constructor type's subtree) still participate in `ReferenceHandler`-based tracking. A mutable type (e.g., Dictionary) nested inside a record's constructor parameters is serialized as an independent copy -- this is correct DDD behavior because records are value objects and their internal state is logically independent. -- Source: User decision (DDD value object semantics). NEW. + +12. WHEN serialization uses Ordinal format (`SerializationFormat.Ordinal`), THEN types with `IOrdinalSerializable` are serialized as arrays by `NeatooOrdinalConverter` and reference handling for those types continues to be managed by Neatoo's custom converters (not by `ReferenceHandler` on options). Non-ordinal types in the same graph still participate in `ReferenceHandler`-based reference tracking. -- Source: Ordinal converter design. NEW. Ordinal and `ReferenceHandler` coexist because the ordinal converter claims only `IOrdinalSerializable` types. + +13. WHEN all existing tests in `RemoteFactory.IntegrationTests` and `Design.Tests` are run, THEN zero tests fail. The `InterfaceFactory_NonNeatooType_NoRefMetadata` test continues to pass unchanged because `RecordBypassConverterFactory` claims the record type before `ReferenceHandler` can add metadata -- records still produce JSON without `$id`/`$ref`. -- Source: Sacred tests rule -- intent preserved; bypass converter ensures records never get reference metadata. + +### Test Scenarios + +| # | Scenario | Inputs / State | Rule(s) | Expected Result | +|---|----------|---------------|---------|-----------------| +| 1 | Shared Dictionary -- current behavior (no fix) | Plain class with `PropA` and `PropB` both set to same `Dictionary` instance; serialize/deserialize via `NeatooJsonSerializer` (v0.22.0 options) | Rule 1 | `ReferenceEquals(result.PropA, result.PropB)` is `false` -- shared identity lost | +| 2 | Record round-trip -- current behavior | `InterfaceRecordWithCollection("Test", items)` serialize/deserialize via `NeatooJsonSerializer` | Rule 2 | Deserialization succeeds, `Name == "Test"`, `Items.Count == 3` | +| 3 | Record with ReferenceHandler.Preserve | Same record, but `options.ReferenceHandler = ReferenceHandler.Preserve` | Rule 3 | `NotSupportedException` thrown on deserialization | +| 4 | Circular reference -- no handler | Plain class `Node { string Name; Node? Next; }` with `a.Next = b; b.Next = a`; serialize via `NeatooJsonSerializer` (no handler) | Rule 4 | `JsonException` thrown (max depth exceeded) | +| 5 | Shared Dictionary with ReferenceHandler.Preserve | Same as Scenario 1 but with `ReferenceHandler.Preserve` on a bare `JsonSerializerOptions` | Rule 5 | `ReferenceEquals(result.PropA, result.PropB)` is `true` | +| 6 | Custom ReferenceHandler delegating to NeatooReferenceResolver | Same as Scenario 5 but using custom `ReferenceHandler` subclass that returns `NeatooReferenceResolver.Current` from `CreateResolver()` | Rule 6 | `ReferenceEquals(result.PropA, result.PropB)` is `true` -- confirms custom handler works | +| 7 | Shared Dictionary -- after fix | Same as Scenario 1 but with `NeatooPreserveReferenceHandler` on options and `RecordBypassConverterFactory` in converters | Rule 7 | `ReferenceEquals(result.PropA, result.PropB)` is `true` | +| 8 | Record round-trip -- after fix | Record with parameterized constructor containing reference-type params (e.g., `IReadOnlyList`); serialize/deserialize with both components active | Rule 8 | Deserialization succeeds, all properties intact, JSON contains no `$id`/`$ref` for the record | +| 9 | Circular reference -- after fix | Same as Scenario 4 but with both components active | Rule 9 | `result.Next.Next` is same instance as `result` (`ReferenceEquals` is `true`) | +| 10 | Cross-type shared reference (Neatoo + non-Neatoo) | Neatoo entity with a `Dictionary` property; same Dictionary also referenced from a sibling property | Rule 10 | Both properties reference the same Dictionary instance after round-trip | +| 11 | Record with nested mutable type in mixed graph | Graph containing a record `Foo(string Name, Dictionary Data)` AND a plain class property referencing the SAME Dictionary instance; both components active | Rule 11 | Record deserializes correctly with its own independent copy of `Data`; the plain class property's Dictionary participates in `$id`/`$ref` tracking; no error. `ReferenceEquals` between the record's Data and the plain class's Data is `false` -- correct DDD behavior | +| 12 | Ordinal format -- existing behavior preserved | Existing ordinal serialization tests pass without change | Rule 12 | All ordinal tests pass | +| 13 | Existing test suite -- no regressions | Run all `RemoteFactory.IntegrationTests` and `Design.Tests` | Rule 13 | Zero failures; `InterfaceFactory_NonNeatooType_NoRefMetadata` passes unchanged because bypass converter prevents `$id`/`$ref` on records | + +--- + +## Approach + +### The Core Insight + +The v0.22.0 decision to make reference handling a "converter-level concern" was correct for Neatoo's custom converters -- they have full control over their serialization format. But it left a gap: non-custom types handled by STJ's built-in converters have no converter to add `$id`/`$ref`. The only way to get STJ's built-in converters to participate in reference tracking is through `options.ReferenceHandler`. + +This is not reverting v0.22.0. It is completing the picture: + +- **v0.22.0 established:** Neatoo converters access `NeatooReferenceResolver.Current` directly (converter-level). +- **This work adds:** STJ's built-in converters access the SAME resolver through `options.ReferenceHandler` (serializer-level for non-custom types). +- **`RecordBypassConverterFactory` prevents STJ's parameterized-constructor limitation** from affecting records. +- **Both reference tracking paths use the same resolver instance**, so cross-type reference identity is maintained. + +### The Record Problem and DDD Resolution + +Phase 1 confirmed that STJ's parameterized-constructor limitation is permanent and comprehensive: STJ throws `NotSupportedException` for ANY reference metadata (`$id` or `$ref`) on reference-type constructor parameters -- not just `$ref` on the record itself. This limitation is closed as NOT_PLANNED in dotnet/runtime#73302. + +Phase 2 attempted the "optimistic path" (hoping STJ would skip records as immutable) and discovered it does not. STJ adds `$id` to ALL reference types when `ReferenceHandler` is set, regardless of constructor type. + +**The resolution is architectural, not a workaround -- it follows DDD value object semantics:** + +Records are value objects. Value objects are defined by their values, not their identity. Two value objects with the same state are interchangeable. Therefore: + +1. **Reference tracking is semantically wrong for records.** Tracking identity implies the object's reference matters. For value objects, it does not. Duplicating a record's internal state (including nested Lists/Dictionaries) on round-trip is the correct DDD behavior. + +2. **Nested reference types within records are part of the value object's state.** A Dictionary inside a record constructor parameter is part of the value object's definition. If that same Dictionary instance is also referenced from an entity property elsewhere in the graph, the entity's reference participates in tracking (via the outer options with `ReferenceHandler`), while the record's copy is logically independent. This is not a limitation -- it is correct. Entities have identity; value objects do not. + +3. **The bypass converter enforces this semantic boundary.** `RecordBypassConverterFactory` claims types with parameterized constructors and serializes them without reference metadata. This is not "skipping" reference handling -- it is applying the correct serialization behavior for value objects. + +### Two-Component Architecture + +**Component 1: `NeatooPreserveReferenceHandler`** (already built in Phase 1) +- Custom `ReferenceHandler` subclass wired into `NeatooJsonSerializer`'s `JsonSerializerOptions` +- Delegates `CreateResolver()` to `NeatooReferenceResolver.Current` +- Gives STJ's built-in converters `$id`/`$ref` for mutable reference types (Dictionary, List, plain classes with default constructors) + +**Component 2: `RecordBypassConverterFactory`** (new, ~50-80 lines) +- `JsonConverterFactory` that claims types with parameterized constructors +- Detection rule: type has a constructor with parameters AND is NOT already claimed by a Neatoo converter +- Delegates serialization to a cached inner `JsonSerializerOptions` identical to the outer options except `ReferenceHandler = null` and no `RecordBypassConverterFactory` (prevents recursion) +- Records and their entire subtree serialize without `$id`/`$ref` + +**Why "all types with parameterized constructors" is the correct detection rule:** +- Records with primary constructors are the primary target +- Non-record classes with parameterized constructors are rare in the RemoteFactory ecosystem +- Even if such classes exist, not having reference tracking on them is low-risk -- the simpler rule avoids fragile heuristics that attempt to distinguish `record class Foo(int X)` from `class Foo(int X)` (STJ cannot distinguish them) +- The rule is easy to explain and document + +### Two-Phase Strategy + +**Phase 1: Reproduction and exploration** (COMPLETE) -- Created `SharedReferenceExplorationTests` in `RemoteFactory.IntegrationTests` with targeted tests. Created `NeatooPreserveReferenceHandler`. Confirmed custom `ReferenceHandler` works for mutable types. Confirmed STJ's parameterized-constructor limitation is permanent and comprehensive. + +**Phase 2: Implementation** -- Build on Phase 1 artifacts: +1. Create `RecordBypassConverterFactory` (~50-80 lines) that claims parameterized-constructor types and delegates to inner options without `ReferenceHandler`. +2. Wire both `NeatooPreserveReferenceHandler` (already built) and `RecordBypassConverterFactory` (new) into `NeatooJsonSerializer`'s options. +3. Create Phase 2 acceptance tests (Scenarios 7-11). +4. Verify all existing tests pass unchanged -- `InterfaceFactory_NonNeatooType_NoRefMetadata` should pass without modification because the bypass converter claims the record before `ReferenceHandler` can add metadata. +5. Update documentation. + +--- + +## Domain Model Behavioral Design + +Not applicable. This work is in the serialization infrastructure layer, not in domain model behavior. No computed properties, reactive rules, or validation rules are involved. + +--- + +## Design + +### Component 1: NeatooPreserveReferenceHandler (already built) + +``` +NeatooPreserveReferenceHandler : ReferenceHandler + CreateResolver() => NeatooReferenceResolver.Current + ?? throw InvalidOperationException("No active serialization") +``` + +This bridges STJ's `ReferenceHandler` API to the existing `NeatooReferenceResolver.Current` `AsyncLocal`. The resolver is created and managed by `NeatooJsonSerializer` (unchanged from v0.22.0). Already built and validated in Phase 1. + +### Component 2: RecordBypassConverterFactory (new) + +``` +RecordBypassConverterFactory : JsonConverterFactory + + CanConvert(Type typeToConvert): + - Return false if typeToConvert is already claimed by a Neatoo converter + (implements a Neatoo marker interface or is handled by NeatooInterfaceJsonTypeConverter, etc.) + - Return true if typeToConvert has a constructor with parameters + (STJ's own heuristic: [JsonConstructor] or single public ctor with params) + - Return false otherwise (mutable types go through STJ built-in + ReferenceHandler) + + CreateConverter(Type typeToConvert, JsonSerializerOptions options): + - Return new RecordBypassConverter(innerOptions) + - innerOptions is lazily created and cached (one per outer options instance) + +RecordBypassConverter : JsonConverter + + Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options): + - return JsonSerializer.Deserialize(ref reader, _innerOptions) + + Write(Utf8JsonWriter writer, T value, JsonSerializerOptions options): + - JsonSerializer.Serialize(writer, value, _innerOptions) +``` + +### Inner Options Construction + +The inner `JsonSerializerOptions` is the critical piece. It must be identical to the outer options except: + +1. **`ReferenceHandler = null`** -- no `$id`/`$ref` for the record's subtree +2. **`Converters` list excludes `RecordBypassConverterFactory`** -- prevents infinite recursion (record converter claims record, delegates to inner options, inner options have record converter, which claims record again...) +3. **All other settings preserved** -- `TypeInfoResolver`, `WriteIndented`, `IncludeFields`, other Neatoo converters + +Construction approach using the .NET 9+ copy constructor: + +``` +var innerOptions = new JsonSerializerOptions(outerOptions); +innerOptions.ReferenceHandler = null; +// Remove RecordBypassConverterFactory from Converters +// (iterate and copy all except self) +``` + +The inner options are created once per outer options instance and cached. Thread safety is handled by the `AsyncLocal` scope of `NeatooReferenceResolver.Current` (each serialization operation is isolated). + +### Modified NeatooJsonSerializer Constructor + +``` +Options = new JsonSerializerOptions +{ + TypeInfoResolver = neatooDefaultJsonTypeInfoResolver, + ReferenceHandler = new NeatooPreserveReferenceHandler(), // Component 1 + WriteIndented = ..., + IncludeFields = true, + Converters = { + new RecordBypassConverterFactory(), // Component 2 -- BEFORE other converters + // ... existing Neatoo converters + } +}; +``` + +Converter ordering matters: `RecordBypassConverterFactory` must be added before Neatoo converters so it is checked first. However, its `CanConvert` returns `false` for Neatoo types (they have their own converters), so there is no conflict. + +### Reference Flow After Change + +``` +NeatooJsonSerializer.Serialize() + | + +-- Creates NeatooReferenceResolver, sets .Current via AsyncLocal + | + +-- JsonSerializer.Serialize(target, OuterOptions) + | + +-- For types with Neatoo custom converters (entities/lists): + | Neatoo converter reads NeatooReferenceResolver.Current directly + | Emits $id/$ref under its own control + | (unchanged from v0.22.0) + | + +-- For types with parameterized constructors (records, etc.): + | RecordBypassConverterFactory.CanConvert() returns true + | RecordBypassConverter delegates to InnerOptions (no ReferenceHandler) + | STJ serializes the record and its entire subtree normally + | NO $id/$ref metadata anywhere in the record's subtree + | (DDD correct: value objects have no identity to track) + | + +-- For mutable types without custom converters (Dictionary, List, plain classes): + | STJ built-in converter handles serialization + | ReferenceHandler is active (OuterOptions has NeatooPreserveReferenceHandler) + | STJ calls CreateResolver() -> NeatooReferenceResolver.Current + | STJ emits $id/$ref using that resolver + | + +-- Neatoo converters and STJ built-in converters share the SAME resolver + | Cross-type references share the same ID space + | + +-- Clears .Current in finally block (unchanged) +``` + +### Why InterfaceFactory_NonNeatooType_NoRefMetadata Passes Unchanged + +The existing test serializes a record with a parameterized constructor and asserts `DoesNotContain("$id")` and `DoesNotContain("$ref")`. With the bypass converter: + +1. STJ encounters the record type +2. `RecordBypassConverterFactory.CanConvert()` returns `true` (parameterized constructor) +3. `RecordBypassConverter` serializes using inner options (no `ReferenceHandler`) +4. No `$id`/`$ref` metadata appears in the JSON +5. Test assertions pass unchanged + +The test's original intent (records are not corrupted by reference metadata) is preserved by design, not by coincidence. + +### File Changes (Phase 2) + +| File | Change | +|------|--------| +| `src/RemoteFactory/Internal/NeatooPreserveReferenceHandler.cs` | Already exists from Phase 1 -- no change needed | +| `src/RemoteFactory/Internal/RecordBypassConverterFactory.cs` | NEW -- `JsonConverterFactory` + `JsonConverter` that bypasses reference handling for parameterized-constructor types (~50-80 lines) | +| `src/RemoteFactory/Internal/NeatooJsonSerializer.cs` | Add `ReferenceHandler = new NeatooPreserveReferenceHandler()` and `new RecordBypassConverterFactory()` to Options constructor. NOTE: `NeatooPreserveReferenceHandler` may already be wired from Phase 1 -- verify and add `RecordBypassConverterFactory` | +| `src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/SharedReferenceTests.cs` | NEW -- Phase 2 acceptance tests (Scenarios 7-11) | +| `src/Tests/RemoteFactory.IntegrationTests/TestTargets/TypeSerialization/SharedReferenceTargets.cs` | May need additional target types for Phase 2 scenarios (e.g., record with nested Dictionary for Scenario 11) | +| `src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/InterfaceFactoryRecordSerializationTests.cs` | NO change expected -- bypass converter preserves existing behavior | +| `docs/serialization.md` | Update "Scope" section to reflect expanded reference handling (Step 9 documentation deliverable) | +| `docs/appendix/serialization.md` | No change needed (already promises universal shared identity) | +| `docs/appendix/record-reference-handling.md` | NEW -- Appendix doc explaining DDD rationale for bypass converter (Step 9 documentation deliverable) | +| `src/Design/CLAUDE-DESIGN.md` | Update Anti-Pattern 9 technical explanation (Step 9 documentation deliverable) | + +--- + +## Implementation Steps + +### Phase 1: Reproduction and Exploration + +1. Create `SharedReferenceTargets.cs` with test target types: + - `SharedDictionaryHolder` -- plain class with two `Dictionary` properties + - `CircularNode` -- plain class with `Name` and `Next` properties for circular reference testing + - Reuse existing `InterfaceRecordWithCollection` for record testing + +2. Create `SharedReferenceExplorationTests.cs` with tests for Scenarios 1-6: + - Tests that document current behavior (expected failures that prove the problem exists) + - Tests that explore `ReferenceHandler.Preserve` with records (confirm STJ limitation) + - Tests that explore custom `ReferenceHandler` delegating to `NeatooReferenceResolver.Current` + - Each test clearly documents what it proves and whether the result was expected + +3. Run Phase 1 tests, analyze results, and document findings in developer memory file. + +### Phase 2: Implementation (building on Phase 1 artifacts) + +Phase 1 is complete. `NeatooPreserveReferenceHandler` is already built. Phase 2 adds the bypass converter and wires both components into `NeatooJsonSerializer`. + +4. Create `RecordBypassConverterFactory.cs` in `src/RemoteFactory/Internal/`: + - `RecordBypassConverterFactory : JsonConverterFactory` -- `CanConvert` returns `true` for types with parameterized constructors, `false` for types already claimed by Neatoo converters + - `RecordBypassConverter : JsonConverter` -- delegates Read/Write to `JsonSerializer.Serialize/Deserialize` using cached inner `JsonSerializerOptions` with `ReferenceHandler = null` and no `RecordBypassConverterFactory` + - Inner options created lazily, cached per outer options instance + - Estimated ~50-80 lines total + +5. Wire both components into `NeatooJsonSerializer`'s `JsonSerializerOptions`: + - Verify `NeatooPreserveReferenceHandler` is already on `Options.ReferenceHandler` (from Phase 1). If not, add it. + - Add `new RecordBypassConverterFactory()` to `Options.Converters` list, BEFORE existing Neatoo converters. + +6. Create Phase 2 acceptance tests in `SharedReferenceTests.cs`: + - Scenario 7: Shared Dictionary identity preserved after round-trip + - Scenario 8: Record with reference-type constructor params round-trips with both components active + - Scenario 9: Circular reference in plain class preserved after round-trip + - Scenario 10: Cross-type shared reference (Neatoo entity + Dictionary) + - Scenario 11: Record with nested mutable type in mixed graph -- record gets independent copy, mutable type outside record is tracked + +7. Run full test suite: + - All existing tests must pass with zero failures + - `InterfaceFactory_NonNeatooType_NoRefMetadata` should pass unchanged (bypass converter prevents `$id`/`$ref` on records) + - If this test unexpectedly fails, investigate -- the bypass converter should prevent this + +8. Documentation updates are Step 9 deliverables (see Acceptance Criteria). + +--- + +## Acceptance Criteria + +### Phase 1 (COMPLETE) +- [x] Phase 1 exploration tests exist and document current behavior for all three scenarios +- [x] Phase 1 findings are documented (inherent STJ limitation confirmed -- permanent, comprehensive) +- [x] `NeatooPreserveReferenceHandler` built and validated + +### Phase 2 (Implementation) +- [ ] `RecordBypassConverterFactory` created (~50-80 lines) claiming all types with parameterized constructors +- [ ] Both components wired into `NeatooJsonSerializer`'s `JsonSerializerOptions` +- [ ] Shared Dictionary identity is preserved after round-trip through `NeatooJsonSerializer` (Scenario 7) +- [ ] Records with parameterized constructors (including reference-type params) continue to deserialize without error (Scenario 8) +- [ ] Records produce JSON without `$id`/`$ref` metadata (Scenario 8) +- [ ] Circular references in plain classes are handled (no infinite loop, identity preserved) (Scenario 9) +- [ ] Cross-type shared references (Neatoo entity + plain Dictionary) work (Scenario 10) +- [ ] Record with nested mutable type in mixed graph: record gets independent copy, mutable type outside record is tracked (Scenario 11) +- [ ] Ordinal format serialization is unaffected (Scenario 12) +- [ ] All existing tests pass (zero failures) (Scenario 13) +- [ ] `InterfaceFactory_NonNeatooType_NoRefMetadata` test passes unchanged -- bypass converter prevents `$id`/`$ref` on records + +**Documentation deliverables** (for Step 9): +- [x] NEW: `docs/appendix/record-reference-handling.md` -- Appendix-style document explaining the DDD rationale for why records bypass reference handling. Covers: value object semantics, three-path serialization architecture, nested reference types within records, the STJ parameterized-constructor limitation (dotnet/runtime#73302), detection rule, practical type category table, and reference flow diagram. +- [x] Update published docs (`docs/serialization.md`) -- Requirements documenter updated the "Scope" section to "Reference Handling by Type Category" with the three-path table and appendix cross-reference. Docs writer fixed duplicate sentence in the anti-pattern guidance. +- [x] Update `docs/appendix/serialization.md` -- Requirements documenter updated section 3 ("Shared Object Identity Is Lost") with expanded scope and cross-reference to the new appendix. +- [x] Update `CLAUDE-DESIGN.md` Anti-Pattern 9 technical explanation -- updated by requirements documenter: Anti-Pattern 9 explanation, Quick Decisions Table record row, and Common Mistakes #10 summary +- [ ] Release notes for the version bump -- separate deliverable + +--- + +## Requirements Documentation (Step 9) + +### Files Directly Updated + +1. **`src/Design/CLAUDE-DESIGN.md`** -- Three updates: + - Anti-Pattern 9 "Why it matters" explanation: replaced stale "no ReferenceHandler set" text with two-path strategy description (`NeatooPreserveReferenceHandler` for mutable types, `RecordBypassConverterFactory` for records) + - Quick Decisions Table: updated record row rationale from "serialized without `$id`/`$ref`" to "bypass reference handling (`RecordBypassConverterFactory`)" + - Common Mistakes #10: updated description to reference bypass converter behavior + +2. **`docs/serialization.md`** -- Replaced "Scope: Converter-Level, Not Serializer-Level" section (lines 120-124) with "Reference Handling by Type Category" section containing: three-path type table, shared resolver explanation, DDD value object rationale for nested mutable types in records, and cross-link to the new appendix + +3. **`docs/appendix/serialization.md`** -- Updated section 3 ("Shared Object Identity Is Lost") with expanded scope clarifying which types participate in reference tracking and cross-reference to the new record-reference-handling appendix + +4. **`docs/appendix/record-reference-handling.md`** -- Already existed (created by prior agent). Verified content matches implementation. No changes needed. + +### Developer Deliverables (Source Code) + +1. **`src/Design/Design.Tests/FactoryTests/SerializationTests.cs:38`** -- Update the header comment. Line 38 currently says "Circular references without proper handling" under the "NO" list. After this implementation, circular references in mutable types ARE handled (via `NeatooPreserveReferenceHandler`). Change to: "Circular references in types with parameterized constructors (records)" or remove the line and add a note under "PARTIAL" explaining that circular references work for mutable types but not records. + +2. **(Optional) Design.Tests shared-reference test** -- Consider adding a test to `SerializationTests.cs` that exercises shared object identity for a non-Neatoo mutable type (e.g., same Dictionary assigned to two properties on a `[Factory]` class). This would close Gap 10 from the requirements review ("None test shared object identity"). Not urgent since `SharedReferenceTests.cs` in IntegrationTests covers this thoroughly. + +--- + +## Agent Phasing + +| Phase | Agent Type | Fresh Agent? | Rationale | Dependencies | +|-------|-----------|-------------|-----------|--------------| +| Phase 1: Reproduction Tests | developer | Yes | COMPLETE -- exploration tests built, findings documented | None | +| Phase 2: Implementation | developer | Yes | Clean context; implements bypass converter and wires both components. Phase 1 findings and architect revision are known. | Phase 1 (complete) | + +**Parallelizable phases:** None -- Phase 2 depends on Phase 1 artifacts. + +**Notes:** +- Phase 1 is complete. Key findings to relay to Phase 2 developer: + - `NeatooPreserveReferenceHandler` is already built and validated + - STJ's parameterized-constructor limitation is permanent and comprehensive (throws for `$id` on reference-type constructor params, not just `$ref`) + - Custom `ReferenceHandler` works correctly for mutable types (Dictionary, List, plain classes) + - The only viable path for records is a custom converter that bypasses reference handling entirely +- Phase 2 developer receives the plan (this file) and the architect's memory file context (relayed by orchestrator). The implementation is straightforward: ~50-80 lines for the bypass converter, wiring into serializer options, and Phase 2 acceptance tests. +- No architect revision is expected during Phase 2 -- the design is decided and complete. + +--- + +## Dependencies + +- **STJ `ReferenceHandler` API:** The custom subclass approach relies on STJ calling `CreateResolver()` for each serialization operation. This is well-documented public API. +- **`NeatooReferenceResolver.Current` lifecycle:** Must remain created/disposed by `NeatooJsonSerializer` per serialize/deserialize call. No changes to lifecycle management. +- **Neatoo converters:** Must continue to work with the shared resolver. Since Neatoo converters access `NeatooReferenceResolver.Current` directly (not through `options.ReferenceHandler`), they are unaffected by adding `ReferenceHandler` to options. + +--- + +## Risks / Considerations + +1. **Inner options caching and thread safety:** The bypass converter creates a cached inner `JsonSerializerOptions`. The cache key is the outer options instance. Since `NeatooJsonSerializer` creates `Options` once in the constructor and reuses it, there is exactly one inner options instance per serializer. Thread safety is handled by `AsyncLocal` scoping of `NeatooReferenceResolver.Current` -- each serialization operation is isolated. Risk is LOW but the developer should verify the caching strategy does not cause issues under concurrent serialization. + +2. **Converter ordering:** `RecordBypassConverterFactory` must be checked BEFORE Neatoo converters in the `Converters` list. If Neatoo converters are checked first and one claims a type that also has a parameterized constructor, the bypass converter is never reached. This is CORRECT behavior (Neatoo types should use Neatoo's converter), but the ordering must be intentional. Risk is LOW if the developer adds the factory first in the list. + +3. **Detection over-claiming:** The bypass converter claims ALL types with parameterized constructors, not just C# `record` types. This means a non-record class like `class Foo { public Foo(int x) { X = x; } public int X { get; set; } }` would also bypass reference handling. In the RemoteFactory ecosystem, such types are rare. The user explicitly accepted this tradeoff: simpler detection rule avoids fragile heuristics. Risk is LOW. + +4. **Performance:** Two sources of overhead: (a) `ReferenceHandler` causes STJ to check every mutable reference type instance against the resolver's dictionary -- same cost as `ReferenceHandler.Preserve`, a standard STJ feature. (b) The bypass converter delegates to a second `JsonSerializer.Serialize/Deserialize` call with inner options. This is a pass-through, not double serialization -- STJ writes directly to the same `Utf8JsonWriter`. Risk is LOW. + +5. **Neatoo converter interaction:** Neatoo converters and STJ built-in converters will both write to the same `NeatooReferenceResolver`. The `$id` numbering will be interleaved (Neatoo gets some IDs, STJ gets others). This is correct behavior -- the resolver tracks all objects in the graph regardless of which converter serialized them. + +6. **v0.22.0 principle tension:** Adding `ReferenceHandler` to options partially reverses v0.22.0's "no `ReferenceHandler` on options" decision. The resolution is clean: v0.22.0 was correct that Neatoo converters should not depend on `options.ReferenceHandler` (they access the resolver directly). The `ReferenceHandler` is for STJ's built-in converters only. The bypass converter extends the converter-level principle to records. All three paths (Neatoo converters, bypass converter, STJ built-in) coexist without conflict. + +7. **Multi-targeting:** Must verify on both net9.0 and net10.0. The `JsonSerializerOptions` copy constructor is available in both. The STJ `ReferenceHandler` API is available in both. + +8. **Ordinal format:** `NeatooOrdinalConverterFactory` only claims types implementing `IOrdinalSerializable`. Non-ordinal types in the same graph fall through to STJ built-in handling and will participate in `ReferenceHandler`-based reference tracking. The ordinal converter serializes as arrays -- STJ's `ReferenceHandler` does not interfere with custom converters that fully control their write output. + +9. **Nested reference types within records (the DDD resolution):** A Dictionary inside a record constructor parameter is serialized without `$id`/`$ref` (part of the record's bypass subtree). If the same Dictionary instance is also referenced from a plain class property elsewhere in the graph, the plain class's reference participates in tracking but the record's copy does not. This means `ReferenceEquals` between them is `false` after round-trip. This is intentional and correct per DDD value object semantics -- but it should be documented clearly (see appendix deliverable) to prevent future sessions from treating it as a bug. diff --git a/docs/plans/completed/shared-reference-handling-plan.memory/architect.md b/docs/plans/completed/shared-reference-handling-plan.memory/architect.md new file mode 100644 index 00000000..1831687f --- /dev/null +++ b/docs/plans/completed/shared-reference-handling-plan.memory/architect.md @@ -0,0 +1,182 @@ +# Architect -- Shared Reference Handling Plan + +Last updated: 2026-03-21 +Current step: Phase 2 design revised and finalized. Plan updated to "Under Review (Developer)" for developer review (Step 5). + +## Key Context + +### The Decided Approach: Simple Bypass Converter with DDD Justification + +After Phase 1 confirmed that STJ's parameterized-constructor limitation is permanent and comprehensive, and after extensive discussion of viable approaches (custom converter factory, two-pass serialization, reduced scope, options stripping), the user decided on the simplest viable path: + +**Two-component architecture:** + +1. **`NeatooPreserveReferenceHandler`** (already built in Phase 1) -- wired into `NeatooJsonSerializer`'s `JsonSerializerOptions`. Gives STJ's built-in converters `$id`/`$ref` for mutable reference types. + +2. **`RecordBypassConverterFactory`** (new, ~50-80 lines) -- claims ALL types with parameterized constructors. Delegates to cached inner `JsonSerializerOptions` identical to outer except `ReferenceHandler = null` and no `RecordBypassConverterFactory`. Records and their entire subtree serialize without `$id`/`$ref`. + +### DDD Justification (Core Design Decision) + +Records are value objects. Value objects are defined by their values, not their identity. This is not a compromise or limitation -- it is the correct semantic model: + +- **Reference tracking is semantically wrong for records.** Tracking identity implies the object's reference matters. For value objects, it does not. +- **Nested reference types within records are part of the value object's state.** A Dictionary inside a record is part of the value object's definition. Duplicating it on round-trip is correct DDD behavior. +- **The entity/value object boundary is the serialization boundary.** Entities (mutable, identity-tracked via `$id`/`$ref`) and value objects (immutable, duplicated) serialize differently because they ARE different. + +The user explicitly wants this documented in an appendix-style document (`docs/appendix/record-reference-handling.md`) to prevent future sessions from re-flagging this behavior as a bug. + +### Detection Rule + +Claim ALL types with parameterized constructors (simplest, safest). The user explicitly accepted this: +- Non-record classes with parameterized constructors are rare in the RemoteFactory ecosystem +- Even if such classes exist, not having reference tracking on them is low-risk +- The simpler rule avoids fragile heuristics that attempt to distinguish `record class` from `class` (STJ cannot distinguish them) + +### v0.22.0 Principle Preserved + +This does NOT revert v0.22.0. Three paths coexist: +- **Neatoo converters:** Access `NeatooReferenceResolver.Current` directly (unchanged from v0.22.0) +- **Bypass converter:** Serializes records without reference handling (new, extends converter-level principle) +- **STJ built-in:** Uses `ReferenceHandler` on options for mutable types only (new, fills the gap) + +All three paths share the same `NeatooReferenceResolver` instance when applicable. + +### Phase 1 Findings Summary (for relaying to Phase 2 developer) + +1. `NeatooPreserveReferenceHandler` is built and validated -- works correctly for mutable types +2. STJ's parameterized-constructor limitation is permanent (dotnet/runtime#73302 closed NOT_PLANNED) +3. The limitation extends to ANY `$id`/`$ref` metadata on reference-type constructor parameters, not just `$ref` on the record itself +4. `ReferenceResolver.GetReference()` cannot suppress `$id` emission -- there is no per-type opt-out at the resolver level +5. The ONLY mechanism in STJ that bypasses built-in reference handling is custom converters (`JsonConverterFactory.CanConvert()` returning `true`) +6. The `JsonSerializerOptions` copy constructor (available in .NET 9+) makes creating inner options straightforward + +### InterfaceFactory_NonNeatooType_NoRefMetadata Test + +This test passes UNCHANGED with the bypass converter design. The bypass converter claims the record before `ReferenceHandler` can add metadata. The test's original intent (records are not corrupted by reference metadata) is preserved by design, not by coincidence. + +## Mistakes to Avoid + +- The original plan's "optimistic path" (hoping STJ would skip records as immutable) was WRONG. STJ adds `$id` to ALL reference types when `ReferenceHandler` is set, regardless of constructor type. +- The "filtering resolver" contingency also fails because `ReferenceResolver.GetReference()` cannot suppress `$id` emission. +- Do NOT attempt per-type `ReferenceHandler` control -- STJ does not support it. The only path is custom converters. +- Do NOT try to use `JsonTypeInfo.CreateObject` to change STJ's converter selection -- it does not work (converter is baked in at `JsonTypeInfo` creation time). + +## User Corrections + +- User reframe (critical): "Stop thinking about this as Neatoo vs non-Neatoo types. The issue is about standard .NET records. Records are value objects by definition -- identity doesn't matter, duplication is fine. The right model is: ReferenceHandler for mutable reference types (Dictionary, List, plain classes), skip it for records." +- User on detection rule: Claim ALL types with parameterized constructors. Simplest, safest. Non-record classes with parameterized constructors are rare. +- User on nested reference types: Acceptable. DDD justification resolves the concern. +- User on documentation: "Document this in its own appendix style/level of documentation." New file: `docs/appendix/record-reference-handling.md`. +- A1: "RemoteFactory is billing itself as abstracting away the client/server physical layer. We should try as hard as we can to do that." +- A2: Cross-entity sharing scope confirmed ("Yes, at least start with that scope"). +- A3: RemoteFactory owns this, not Neatoo. +- A5: Both shared identity AND circular references. + +## Post-Implementation Verification (Step 8A) + +**Date:** 2026-03-21 +**Verdict:** VERIFIED + +### Build Verification + +- **Solution build (Release):** PASSED -- zero errors, 3 pre-existing WASM warnings (unrelated SQLite interop) +- **All projects compiled successfully** including Generator (netstandard2.0), RemoteFactory (net9.0/net10.0), AspNetCore, all test projects, Design project, Examples + +### Test Results -- All Projects, Both Frameworks + +| Project | Framework | Total | Passed | Failed | Skipped | Result | +|---------|-----------|-------|--------|--------|---------|--------| +| RemoteFactory.UnitTests | net9.0 | 490 | 490 | 0 | 0 | PASS | +| RemoteFactory.UnitTests | net10.0 | 490 | 490 | 0 | 0 | PASS | +| RemoteFactory.IntegrationTests | net9.0 | 501 | 498 | 0 | 3 | PASS | +| RemoteFactory.IntegrationTests | net10.0 | 501 | 498 | 0 | 3 | PASS | +| Design.Tests | net9.0+net10.0 | 42 | 42 | 0 | 0 | PASS | + +**Zero failures across all projects and both frameworks.** + +The 3 skipped integration tests are `ShowcasePerformanceTests` (pre-existing, completely unrelated to this work). + +### Test Scenario Cross-Check + +For EACH numbered test scenario in the plan's Test Scenarios table, I verified a corresponding test method exists and passes: + +| Scenario | Rule(s) | Test Method | Test File | Passes? | +|----------|---------|-------------|-----------|---------| +| 1 | Rule 1->7 | `Scenario1_SharedDictionary_IdentityPreserved` | SharedReferenceExplorationTests.cs | YES (both TFMs) | +| 2 | Rule 2 | `Scenario2_RecordRoundTrip_CurrentBehavior_Succeeds` | SharedReferenceExplorationTests.cs | YES (both TFMs) | +| 3 | Rule 3 | `Scenario3_RecordWithReferenceHandlerPreserve_ThrowsOnDeserialization` | SharedReferenceExplorationTests.cs | YES (both TFMs) | +| 4 | Rule 4->9 | `Scenario4_CircularReference_Handled` | SharedReferenceExplorationTests.cs | YES (both TFMs) | +| 5 | Rule 5 | `Scenario5_SharedDictionary_ReferenceHandlerPreserve_IdentityPreserved` | SharedReferenceExplorationTests.cs | YES (both TFMs) | +| 6 | Rule 6 | `Scenario6_CustomReferenceHandler_NeatooReferenceResolver_IdentityPreserved` | SharedReferenceExplorationTests.cs | YES (both TFMs) | +| 7 | Rule 7 | `Scenario7_SharedDictionary_AfterFix_IdentityPreserved` | SharedReferenceTests.cs | YES (both TFMs) | +| 8 | Rule 8 | `Scenario8_RecordRoundTrip_AfterFix_Succeeds` | SharedReferenceTests.cs | YES (both TFMs) | +| 9 | Rule 9 | `Scenario9_CircularReference_AfterFix_IdentityPreserved` | SharedReferenceTests.cs | YES (both TFMs) | +| 10 | Rule 10 | `Scenario10_CrossTypeSharedReference_DictionaryWithMixedProperties` | SharedReferenceTests.cs | YES (both TFMs) | +| 11 | Rule 11 | `Scenario11_RecordInGraphWithSharedMutableRefs` | SharedReferenceTests.cs | YES (both TFMs) | +| 12 | Rule 12 | Existing ordinal tests (no dedicated test) | OrdinalSerializationTests.cs, OrdinalMetadataTests.cs | YES -- all ordinal tests pass unchanged | +| 13 | Rule 13 | `InterfaceFactory_NonNeatooType_NoRefMetadata` + full suite | InterfaceFactoryRecordSerializationTests.cs | YES -- test passes UNCHANGED (confirmed via git diff) | + +**Coverage: 13 of 13 test scenarios verified with passing tests.** + +### Critical Test Verification + +`InterfaceFactory_NonNeatooType_NoRefMetadata` passes UNCHANGED: +- **git diff confirms:** zero modifications to `InterfaceFactoryRecordSerializationTests.cs` +- **Test assertions unchanged:** `Assert.DoesNotContain("$id", json)` and `Assert.DoesNotContain("$ref", json)` +- **Passes on both net9.0 and net10.0** +- **Mechanism:** `RecordBypassConverterFactory` claims the record type (parameterized constructor) before `ReferenceHandler` can add metadata, so records still produce clean JSON + +### Implementation Design Match + +**New files created (matching plan's File Changes table):** +- `src/RemoteFactory/Internal/RecordBypassConverterFactory.cs` -- 134 lines, includes both `RecordBypassConverterFactory` and `RecordBypassConverter` +- `src/RemoteFactory/Internal/NeatooPreserveReferenceHandler.cs` -- 41 lines (from Phase 1, unchanged) +- `src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/SharedReferenceTests.cs` -- Phase 2 acceptance tests (Scenarios 7-11) +- `src/Tests/RemoteFactory.IntegrationTests/TestTargets/TypeSerialization/SharedReferenceTargets.cs` -- target types + +**Modified files:** +- `src/RemoteFactory/Internal/NeatooJsonSerializer.cs` -- Added `ReferenceHandler = new NeatooPreserveReferenceHandler()` on options; added `new RecordBypassConverterFactory()` to converters list + +**Unchanged (confirmed via git diff):** +- `src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/InterfaceFactoryRecordSerializationTests.cs` + +### Design Conformance + +1. **`RecordBypassConverterFactory` detection rule:** `!hasParameterlessCtor && hasParameterizedCtor` -- matches plan specification exactly ("type has no public parameterless constructor AND has at least one public constructor with parameters") +2. **Inner options construction:** Uses `new JsonSerializerOptions(outerOptions)` copy constructor, sets `ReferenceHandler = null`, removes `RecordBypassConverterFactory` from converters to prevent recursion -- matches plan design +3. **Caching:** Double-checked locking with `_innerOptions` field -- correct thread-safe lazy initialization +4. **Null handling:** `RecordBypassConverter` handles null values in both Read and Write -- good defensive coding not in the plan but correct + +### Deliberate Deviation from Plan (Converter Ordering) + +The plan specified `RecordBypassConverterFactory` BEFORE Neatoo converters (plan lines 257, 356). The implementation places it AFTER Neatoo converters (per user decision for defense-in-depth). + +**This deviation is functionally correct** because: +- Neatoo converters claim their types first (interfaces, abstract types, IOrdinalSerializable) +- `RecordBypassConverterFactory.CanConvert()` does not explicitly check for Neatoo types -- it relies on ordering +- When AFTER Neatoo converters, Neatoo types are already claimed before the bypass factory is consulted +- The result is equivalent: Neatoo types use Neatoo converters, records use the bypass converter, mutable types use STJ built-in with ReferenceHandler +- All tests pass, confirming the ordering works correctly + +### Exploration Tests (Phase 1) Updated + +Scenarios 1 and 4 were updated from documenting broken behavior to asserting the fix works: +- Scenario 1: Changed from `Assert.False(ReferenceEquals(...))` (identity lost) to `Assert.True(ReferenceEquals(...))` (identity preserved) +- Scenario 4: Changed from `Assert.Throws` (max depth) to asserting circular reference round-trip works + +These updates are correct -- the exploration tests now document the fixed behavior rather than the broken state. The original assertions were Phase 1 baselines that existed only to prove the problem; updating them to reflect the fix is appropriate. + +### Summary + +All acceptance criteria from the plan's Phase 2 section are satisfied: +- RecordBypassConverterFactory created (134 lines) +- Both components wired into NeatooJsonSerializer's JsonSerializerOptions +- Shared Dictionary identity preserved (Scenario 7) +- Records with parameterized constructors deserialize without error (Scenario 8) +- Records produce JSON without $id/$ref metadata (verified by unchanged InterfaceFactory_NonNeatooType_NoRefMetadata test) +- Circular references handled (Scenario 9) +- Cross-type shared references work (Scenario 10) +- Record with nested mutable type coexists correctly (Scenario 11) +- Ordinal format unaffected (all ordinal tests pass) +- All existing tests pass with zero failures +- InterfaceFactory_NonNeatooType_NoRefMetadata passes UNCHANGED diff --git a/docs/plans/completed/shared-reference-handling-plan.memory/developer.md b/docs/plans/completed/shared-reference-handling-plan.memory/developer.md new file mode 100644 index 00000000..1ebae41a --- /dev/null +++ b/docs/plans/completed/shared-reference-handling-plan.memory/developer.md @@ -0,0 +1,509 @@ +# Developer -- Shared Reference Handling Plan + +Last updated: 2026-03-21 +Current step: Phase 2 REVISED -- Implementation Complete, Awaiting Verification + +## Key Context + +### Codebase Verification Summary + +- **NeatooReferenceResolver.cs** (verified): Extends `ReferenceResolver`, uses `ReferenceEqualityComparer.Instance`, has `AsyncLocal` `Current` property with public getter / internal setter. Contains `GetReference`, `AddReference`, `ResolveReference`, `AlreadyExists`. Reference counter starts at 0, increments per new reference. Implements `IDisposable`. +- **NeatooJsonSerializer.cs** (verified): Creates a new `NeatooReferenceResolver` per Serialize/Deserialize call using `using var rr = new NeatooReferenceResolver()`, sets `Current = rr` before calling `JsonSerializer.Serialize/Deserialize`, clears `Current = null` in `finally`. Four methods follow this pattern (two Serialize, two Deserialize). The `JsonSerializerOptions` is constructed in the constructor with NO `ReferenceHandler` -- confirmed. Contains a comment (lines 71-76) documenting the Phase 2 revert and the reason. +- **NeatooInterfaceJsonTypeConverter.cs** (verified): Only handles `$type`/`$value` wrapping. Does NOT call `NeatooReferenceResolver` anywhere. Confirmed no dead code remains. +- **NeatooInterfaceJsonConverterFactory.cs** (verified): `CanConvert()` checks `typeToConvert.IsInterface || typeToConvert.IsAbstract` AND `!typeToConvert.IsGenericType` AND `serviceAssemblies.HasType(typeToConvert)`. Only claims interfaces/abstract types known to RemoteFactory. +- **NeatooJsonConverterFactory.cs** (verified): Abstract marker base class extending `JsonConverterFactory`. Empty -- no methods. Provides type identity for filtering (e.g., `converter is NeatooJsonConverterFactory`). +- **NeatooOrdinalConverterFactory.cs** (verified): `CanConvert()` checks `options.Format == SerializationFormat.Ordinal` AND `typeof(IOrdinalSerializable).IsAssignableFrom(typeToConvert)`. Only claims IOrdinalSerializable types in Ordinal mode. +- **InterfaceFactory_NonNeatooType_NoRefMetadata test** (verified at line 120-140): Serializes `InterfaceRecordWithCollection` (a record with primary constructor) and asserts `DoesNotContain("$id")` and `DoesNotContain("$ref")`. +- **InterfaceRecordWithCollection** (verified): `public record InterfaceRecordWithCollection(string Name, IReadOnlyList Items)` -- primary constructor, `Items` is a reference-type constructor parameter. +- **NeatooPreserveReferenceHandler.cs** (verified): Already built. `CreateResolver()` returns `NeatooReferenceResolver.Current` with null check throwing `InvalidOperationException`. +- **NeatooJsonSerializer constructor converter ordering** (verified lines 66-90): Ordinal converter added first (if Ordinal format), then Neatoo converter factories from DI. The `RecordBypassConverterFactory` must be inserted BEFORE ordinal and Neatoo converters. + +### Timing Analysis for Custom ReferenceHandler + +The proposed `NeatooPreserveReferenceHandler.CreateResolver()` returns `NeatooReferenceResolver.Current`. The call sequence is: + +1. `NeatooJsonSerializer.Serialize()` creates `rr = new NeatooReferenceResolver()` +2. Sets `NeatooReferenceResolver.Current = rr` +3. Calls `JsonSerializer.Serialize(target, this.Options)` +4. STJ internally calls `Options.ReferenceHandler.CreateResolver()` which returns `NeatooReferenceResolver.Current` (= `rr`) +5. STJ uses `rr` for built-in converter `$id`/`$ref` tracking +6. Meanwhile, Neatoo custom converters also read `NeatooReferenceResolver.Current` (= `rr`) +7. Both paths share the same resolver instance and ID counter +8. `finally` clears `Current = null` + +Timing is correct. STJ calls `CreateResolver()` once at the start of serialize/deserialize, AFTER `Current` has been set. + +### Phase 1 Key Findings (from prior implementation) + +- F1: `ReferenceHandler.Preserve` throws for records with `$ref` (only when same instance in two properties) +- F2: Custom `ReferenceHandler` delegating to `NeatooReferenceResolver.Current` works for mutable types +- F3: STJ does NOT skip `$id` for records -- they get `$id` even though docs suggest immutable types are skipped +- F4 (Phase 2 discovery): The limitation is broader -- ANY reference-type constructor parameter gets `$id` and throws `NotSupportedException` during deserialization. `InterfaceRecordWithCollection(string Name, IReadOnlyList Items)` fails because `Items` (IReadOnlyList) gets `$id`. + +### Limitation Not In Plan Scope (Pre-Existing) + +`ToRemoteDelegateRequest` serializes each parameter individually -- each `Serialize` call creates a NEW resolver. Shared references across parameters in a remote request are NOT tracked. This is pre-existing behavior and out of scope. + +## Mistakes to Avoid + +- Phase 1 Finding F1 was INCOMPLETE. STJ throws not just for `$ref` on records but for `$id` on ANY reference-type constructor parameter. The revised plan accounts for this with the bypass converter. +- Do not assume STJ treats records as "immutable types" for reference-metadata purposes. It does not. +- The `NeatooJsonSerializer` constructor builds the converter list in a specific order (ordinal first, then Neatoo factories from DI). `RecordBypassConverterFactory` must be inserted before all of them, not appended. +- The Phase 1 test `Scenario3_RecordWithReferenceHandlerPreserve_ThrowsOnDeserialization` must assign the same record to TWO properties to trigger `$ref`. A single-occurrence record will only get `$id`. + +## User Corrections + +- User decided: "all types with parameterized constructors" as detection rule (simplest, avoids fragile heuristics) +- User decided: DDD justification resolves the nested-reference-type concern -- records are value objects, duplication is correct +- User decided: `InterfaceFactory_NonNeatooType_NoRefMetadata` should pass UNCHANGED (bypass converter claims records before ReferenceHandler can add metadata) + +--- + +## Assertion Trace Verification -- Phase 2 Revised Plan + +For this review, I re-trace ALL 13 rules against the REVISED implementation (bypass converter approach). Rules 1-6 are Phase 1 (already implemented and passing). Rules 7-13 are Phase 2. + +### Rules 1-6 (Phase 1 -- already verified and implemented) + +These rules are unchanged from the original review. Phase 1 tests exist and pass. + +| Rule # | Rule Summary | Status | +|--------|-------------|--------| +| 1 | Shared Dictionary, no handler -> identity lost | CONFIRMED -- Phase 1 test passes | +| 2 | Record round-trip succeeds (no handler) | CONFIRMED -- Phase 1 test passes | +| 3 | Record + ReferenceHandler.Preserve -> NotSupportedException | CONFIRMED -- Phase 1 test passes | +| 4 | Circular reference, no handler -> JsonException | CONFIRMED -- Phase 1 test passes | +| 5 | Shared Dictionary + ReferenceHandler.Preserve -> identity preserved | CONFIRMED -- Phase 1 test passes | +| 6 | Custom ReferenceHandler + NeatooReferenceResolver -> identity preserved | CONFIRMED -- Phase 1 test passes | + +### Rules 7-13 (Phase 2 -- revised bypass converter approach) + +| Rule # | Rule Summary | Implementation Path | Verdict | +|--------|-------------|-------------------|---------| +| 7 | Shared Dictionary identity preserved after fix | `NeatooJsonSerializer` constructor gets `ReferenceHandler = new NeatooPreserveReferenceHandler()` on `Options` (line 68-79 area). `RecordBypassConverterFactory` is added to `Options.Converters` first. When serializing `SharedDictionaryHolder`, STJ encounters `Dictionary` properties. `RecordBypassConverterFactory.CanConvert(typeof(Dictionary))` returns `false` because `Dictionary` has a default constructor (no parameterized constructor that STJ uses). STJ's built-in converter handles it. `ReferenceHandler` is active, so STJ emits `$id` on first dictionary, `$ref` on second. After deserialization, `ReferenceEquals` is `true`. | CONFIRMED -- `Dictionary` has no parameterized constructor. Bypass converter does not claim it. STJ built-in converter + `ReferenceHandler` handles it correctly. | +| 8 | Records with parameterized constructors still deserialize correctly | STJ encounters `InterfaceRecordWithCollection(string Name, IReadOnlyList Items)`. `RecordBypassConverterFactory.CanConvert()` returns `true` because the type has a parameterized constructor. `RecordBypassConverter.Write()` delegates to `JsonSerializer.Serialize(writer, value, _innerOptions)` where `_innerOptions` has `ReferenceHandler = null`. No `$id`/`$ref` emitted. On deserialization, `RecordBypassConverter.Read()` delegates to `JsonSerializer.Deserialize(ref reader, _innerOptions)`. No reference metadata in the JSON, so STJ constructs the record normally via its constructor. | CONFIRMED -- The inner options have no `ReferenceHandler`, so the entire record subtree serializes clean. This is functionally identical to the current v0.22.0 behavior for records. | +| 9 | Circular references in mutable types preserved | `CircularNode` has properties `Name` (string) and `Next` (CircularNode?). `CircularNode` has a default constructor (no parameterized constructor). `RecordBypassConverterFactory.CanConvert(typeof(CircularNode))` returns `false`. STJ built-in converter handles it. `ReferenceHandler` is active. STJ calls `GetReference` on `nodeA`, gets `$id="1"`, `alreadyExists=false`. Serializes normally. Encounters `nodeB` in `Next` property, gets `$id="2"`. Inside `nodeB`, encounters `nodeA` again in `Next`, `GetReference` returns `alreadyExists=true`, emits `$ref="1"`. On deserialization, `$ref="1"` resolves to the already-constructed `nodeA`. Circular graph restored. | CONFIRMED -- Mutable classes with default constructors go through STJ built-in path with `ReferenceHandler`. Standard STJ cycle detection applies. | +| 10 | Cross-type shared references use same resolver | Both Neatoo custom converters (reading `NeatooReferenceResolver.Current` directly) and STJ built-in converters (accessing resolver via `NeatooPreserveReferenceHandler.CreateResolver()`) get the SAME `NeatooReferenceResolver` instance. The `_objectToReferenceIdMap` and `_referenceIdToObjectMap` are shared. IDs are globally unique because `_referenceCount` increments within the single resolver. Cross-type identity is tracked. For the Phase 2 test (Scenario 10), the test uses only non-Neatoo types (plain classes + Dictionary), which is a proxy for the real cross-converter scenario (requires Neatoo repository). The test verifies the resolver handles mixed-type graphs correctly. | CONFIRMED -- The resolver sharing mechanism is sound. The test is a reasonable proxy. Full cross-converter testing requires Neatoo. | +| 11 | Records in mixed graphs: record gets independent copy, mutable types tracked | `RecordWithSharedDictionaryHolder` has properties `Record` (InterfaceRecordItem -- a record), `DictionaryA`, `DictionaryB` (both pointing to same dictionary). STJ encounters the holder (mutable, default constructor). `ReferenceHandler` is active. STJ serializes `Record` property: `RecordBypassConverterFactory.CanConvert(typeof(InterfaceRecordItem))` returns `true` (parameterized constructor). `RecordBypassConverter` serializes using inner options (no `ReferenceHandler`). No `$id` on the record. STJ serializes `DictionaryA`: bypass converter returns `false`. STJ built-in converter handles it. `ReferenceHandler` emits `$id` on first dictionary. STJ serializes `DictionaryB`: same dictionary instance, `GetReference` returns `alreadyExists=true`, emits `$ref`. After deserialization: record deserialized correctly via inner options, dictionaries share identity via `$ref`. `ReferenceEquals(result.DictionaryA, result.DictionaryB)` is `true`. The record's data is an independent copy. | CONFIRMED -- The bypass converter cleanly separates the record's subtree from the reference-tracked graph. DDD semantics are preserved. | +| 12 | Ordinal format unaffected | `NeatooOrdinalConverterFactory.CanConvert()` (line 60-69) checks `options.Format == SerializationFormat.Ordinal` AND `typeof(IOrdinalSerializable).IsAssignableFrom(typeToConvert)`. Ordinal converter claims only `IOrdinalSerializable` types. `RecordBypassConverterFactory` checks for parameterized constructors. No conflict: types implementing `IOrdinalSerializable` are Neatoo types generated with ordinal metadata -- they are not records with parameterized constructors. Even if there were overlap, the ordinal converter would be added after the bypass converter in the list, but the bypass converter's `CanConvert` would return `false` for Neatoo types (see detection rule below). Non-ordinal types in the same graph fall through to STJ built-in handling with `ReferenceHandler`. | CONFIRMED -- Ordinal and bypass converters have non-overlapping `CanConvert` predicates. Wait -- I need to verify this. See Concern C6. | +| 13 | All existing tests pass; `InterfaceFactory_NonNeatooType_NoRefMetadata` passes unchanged | The test (line 120-140) serializes `InterfaceRecordWithCollection` via `NeatooJsonSerializer`. With the bypass converter: `RecordBypassConverterFactory.CanConvert(typeof(InterfaceRecordWithCollection))` returns `true` (parameterized constructor: `string Name, IReadOnlyList Items`). `RecordBypassConverter` delegates to inner options with `ReferenceHandler = null`. JSON output has no `$id`/`$ref`. Test assertions `DoesNotContain("$id")` and `DoesNotContain("$ref")` pass. | CONFIRMED -- The bypass converter intercepts the record BEFORE STJ's built-in converter sees it. Since the bypass converter delegates to inner options without `ReferenceHandler`, no reference metadata appears. The test passes unchanged. This is the correct behavior by design, not a coincidence. | + +## Concern Analysis -- Revised Phase 2 + +### C6: RecordBypassConverterFactory CanConvert Detection Rule -- How Exactly? (Clarifying, not blocking) + +The plan says: "Return false if typeToConvert is already claimed by a Neatoo converter (implements a Neatoo marker interface or is handled by NeatooInterfaceJsonTypeConverter, etc.)" and "Return true if typeToConvert has a constructor with parameters." + +The "already claimed by a Neatoo converter" check needs clarification. Looking at the actual Neatoo converters in RemoteFactory: + +1. `NeatooInterfaceJsonConverterFactory` -- claims `typeToConvert.IsInterface || typeToConvert.IsAbstract` (with additional checks). Records are concrete classes, not interfaces/abstracts. This converter would NOT claim a record. No conflict. + +2. `NeatooOrdinalConverterFactory` -- claims `typeof(IOrdinalSerializable).IsAssignableFrom(typeToConvert)` in Ordinal mode. Records do not implement `IOrdinalSerializable`. No conflict. + +3. Neatoo repository converters (`NeatooBaseJsonTypeConverter`, `NeatooListBaseJsonTypeConverter`) -- these are registered in the Neatoo repo, not RemoteFactory. They extend `NeatooJsonConverterFactory`. They claim Neatoo entity/list types. + +The detection rule "has a constructor with parameters" is simple: check if the type has any public constructor with parameters. The question is: does `RecordBypassConverterFactory` need to explicitly filter out Neatoo types, or does converter ordering handle it? + +**Converter ordering in STJ**: STJ checks converters in list order. The first converter whose `CanConvert` returns `true` wins. If `RecordBypassConverterFactory` is added FIRST (before Neatoo converters), it would be checked first. But its `CanConvert` only returns `true` for types with parameterized constructors. Neatoo entity types typically have their concrete classes created by generated code -- do they have parameterized constructors? + +Looking at the interface converter: it claims only interfaces/abstracts. The concrete types (generated by Neatoo) are serialized INSIDE the `$type`/`$value` wrapper by calling `JsonSerializer.Serialize(writer, value, value.GetType(), options)`. At that point, STJ looks for a converter for the concrete type. The Neatoo downstream converters (`NeatooBaseJsonTypeConverter`) are registered for the concrete types in the Neatoo repo. Those converters are `NeatooJsonConverterFactory` subclasses, which are added to the converter list after `RecordBypassConverterFactory`. + +If a Neatoo entity type happens to have a parameterized constructor, `RecordBypassConverterFactory` (checked first) would claim it, and Neatoo's converter would never be reached. This could be a problem. + +**However**, in practice: Neatoo entities are generated classes with default constructors (DI-constructed). They are not records with primary constructors. The risk is low but not zero -- a future change to Neatoo code generation could introduce parameterized constructors. + +**Mitigation**: The plan's design says "Return false if typeToConvert is already claimed by a Neatoo converter." The simplest check: if any converter in the outer options' `Converters` list is a `NeatooJsonConverterFactory` subclass AND its `CanConvert` returns `true` for the type, skip it. But this would require iterating converters, which is expensive. + +A simpler mitigation: check if the type is in the `IServiceAssemblies` registry (Neatoo types are registered). But `RecordBypassConverterFactory` does not have access to `IServiceAssemblies`. + +The SIMPLEST mitigation (and what I recommend): `RecordBypassConverterFactory` does NOT filter Neatoo types explicitly. Instead, rely on converter list ordering: add `RecordBypassConverterFactory` AFTER Neatoo converters, not before. Wait -- this contradicts the plan. + +Actually, re-reading the plan more carefully: the plan says to add `RecordBypassConverterFactory` BEFORE Neatoo converters. The rationale is: "its `CanConvert` returns false for Neatoo types (they have their own converters)." But the bypass converter does not CHECK whether a Neatoo converter exists -- it only checks for parameterized constructors. + +**My recommendation**: Add `RecordBypassConverterFactory` AFTER Neatoo converters in the list. This way, Neatoo converters get first priority. The bypass converter only picks up types that no Neatoo converter claimed. This is simpler and safer than adding explicit Neatoo-type detection logic to the bypass converter. + +Wait -- but Neatoo converters (`NeatooInterfaceJsonConverterFactory`) only claim interfaces/abstracts. The concrete Neatoo types (entities) would NOT be claimed by the interface converter. They are claimed by downstream Neatoo converters from the Neatoo repository. Those downstream converters ARE `NeatooJsonConverterFactory` subclasses and ARE in the converter list (added via DI). + +So the ordering would be: +1. Ordinal converter (if ordinal format) +2. Neatoo converters from DI (NeatooInterfaceJsonConverterFactory + downstream Neatoo converters) +3. RecordBypassConverterFactory + +This ordering ensures Neatoo converters claim their types first. Remaining types with parameterized constructors (i.e., records not claimed by any Neatoo converter) are claimed by the bypass converter. Types without parameterized constructors (Dictionary, List, plain classes) fall through to STJ built-in handling with `ReferenceHandler`. + +**IMPORTANT**: This changes the plan's stated ordering ("BEFORE other converters"). The plan says to add bypass converter first. I believe adding it LAST (after Neatoo converters) is safer. This is a Clarifying concern -- the implementation is the same either way if Neatoo types never have parameterized constructors, but the ordering provides defense-in-depth. + +**Resolution**: Either ordering works today because Neatoo entity types do not have parameterized constructors. The plan's ordering (first) works if the `CanConvert` check is robust. My suggested ordering (last) provides implicit safety. I will note this in the implementation contract and let the user decide. + +### C7: Inner Options Construction -- JsonSerializerOptions Copy Constructor Mutability (Clarifying) + +The plan shows: +``` +var innerOptions = new JsonSerializerOptions(outerOptions); +innerOptions.ReferenceHandler = null; +// Remove RecordBypassConverterFactory from Converters +``` + +The `JsonSerializerOptions` copy constructor in .NET 9+ creates a mutable copy. After `JsonSerializer.Serialize/Deserialize` is called with an options instance, STJ "locks" the options (makes them immutable). The inner options must be created BEFORE the first serialize/deserialize call that uses them, or the copy must be made from the outer options before they are locked. + +In the plan's design, `RecordBypassConverter` lazily creates inner options on first use. At that point, the outer options have already been passed to `JsonSerializer.Serialize` (which locks them). However, `new JsonSerializerOptions(outerOptions)` copies from a locked instance, which is fine -- the COPY is mutable until it is locked by its own first use. + +But there is a subtlety: after creating the copy, we need to modify it (remove `RecordBypassConverterFactory` from Converters, set `ReferenceHandler = null`). These modifications must happen before the inner options are used for serialization. Since the inner options are lazily created and then immediately used, the sequence is: + +1. Create copy: `new JsonSerializerOptions(outerOptions)` -- mutable copy +2. Modify: `innerOptions.ReferenceHandler = null` -- fine, still mutable +3. Remove converter: iterate and remove `RecordBypassConverterFactory` -- fine, still mutable +4. First use: `JsonSerializer.Serialize(writer, value, innerOptions)` -- locks inner options + +This sequence is correct. The concern is theoretical only. + +**One additional consideration**: The `Converters` list on the copy. Does `new JsonSerializerOptions(outerOptions)` deep-copy the converters list? The .NET documentation says the copy constructor copies "all configuration properties." The `Converters` collection is copied. Removing an item from the copy's list does not affect the outer options' list. + +Verdict: Non-issue. The construction sequence is sound. + +### C8: Inner Options Caching Strategy (Clarifying) + +The plan says inner options are "created once per outer options instance and cached." Since `NeatooJsonSerializer` creates `Options` once in its constructor and reuses it for all operations, there is exactly one outer options instance per serializer, and therefore exactly one inner options instance per serializer. + +The cache could be as simple as a field on `RecordBypassConverterFactory`. But the factory creates typed converters (`RecordBypassConverter`) per type. The inner options should be shared across all typed converters (since they are identical regardless of T). The factory should own the cache, not the individual converters. + +Implementation: `RecordBypassConverterFactory` has a `Lazy` or similar, initialized from the outer options. Each `RecordBypassConverter` receives the cached inner options from the factory. + +Wait -- `CreateConverter(Type typeToConvert, JsonSerializerOptions options)` receives the `options` parameter. The factory can create the inner options from this parameter on the first call to `CreateConverter` and cache it. + +Thread safety: `NeatooJsonSerializer` is scoped per DI scope. Multiple threads could call Serialize concurrently within the same scope. The inner options creation should be thread-safe. A `Lazy` or a `volatile` field with double-check locking would work. + +However, since `JsonSerializerOptions` itself is thread-safe once locked (after first use), and the inner options are locked on first use, the cache is safe. The only race condition is during creation -- two threads might create inner options simultaneously. Using `Lazy` prevents double creation. + +Verdict: Minor implementation detail. The plan's "lazily created and cached" approach is correct. Recommend `Lazy` or a simple null-check with lock. + +### C9: What Counts as "Parameterized Constructor" for CanConvert? (Clarifying) + +The plan says: "Return true if typeToConvert has a constructor with parameters (STJ's own heuristic: [JsonConstructor] or single public ctor with params)." + +The bypass converter should match STJ's own detection heuristic for parameterized constructors. STJ's rules are: +1. If `[JsonConstructor]` is present on a constructor, use that constructor. +2. If no `[JsonConstructor]`, use the single public constructor if it has parameters. +3. If there is a public parameterless constructor, STJ uses it (not parameterized). + +The bypass converter's `CanConvert` should return `true` when STJ WOULD use a parameterized constructor. The simplest approximation: + +```csharp +// Type has no public parameterless constructor AND has at least one public constructor with parameters +// OR type has [JsonConstructor] on a parameterized constructor +``` + +A simpler (slightly over-inclusive) check: +```csharp +// Does NOT have a public parameterless constructor +!typeToConvert.GetConstructors().Any(c => c.GetParameters().Length == 0) +&& typeToConvert.GetConstructors().Any(c => c.GetParameters().Length > 0) +``` + +This catches records (which have a primary constructor and typically no parameterless constructor). It also catches any class without a parameterless constructor. This aligns with the user's decision: "all types with parameterized constructors." + +For types that have BOTH a parameterless constructor and a parameterized one, STJ uses the parameterless constructor by default. These types would NOT get reference metadata issues (STJ uses the parameterless constructor, which supports `$id`/`$ref`). So `CanConvert` should return `false` for these types. The check above handles this correctly: if a parameterless constructor exists, `CanConvert` returns `false`. + +Edge case: `[JsonConstructor]` on a parameterized constructor in a type that also has a parameterless constructor. In this case, STJ uses the parameterized constructor despite the parameterless one existing. The simple check above would return `false` (parameterless constructor exists), and the type would go through STJ's built-in path with `ReferenceHandler`, which would fail. + +**Risk assessment**: This edge case (explicit `[JsonConstructor]` on a parameterized constructor alongside a parameterless constructor) is extremely rare in the RemoteFactory ecosystem. Records never have this pattern. The risk is LOW and the simple check is correct for the stated scope. + +Verdict: The simple "no parameterless constructor" check is sufficient. Document the `[JsonConstructor]` edge case as a known limitation. + +### C10: Scenario 11 Test Design -- Record Property Serialization Order (Non-blocking) + +In Scenario 11, the `RecordWithSharedDictionaryHolder` has properties: `Record` (InterfaceRecordItem), `DictionaryA`, `DictionaryB`. The test assumes the bypass converter handles `Record` before STJ assigns `$id` to the Dictionary. Since STJ serializes properties in order, and `Record` comes before `DictionaryA` in the property list, the bypass converter claims `InterfaceRecordItem` first. When STJ reaches `DictionaryA`, it is the first time it sees that dictionary instance, so it assigns `$id`. Then `DictionaryB` gets `$ref`. This is correct. + +But what if the Dictionary appeared INSIDE the record's constructor? The plan's Rule 11 states: "A mutable type (e.g., Dictionary) nested inside a record's constructor parameters is serialized as an independent copy." The bypass converter delegates to inner options (no `ReferenceHandler`), so the Dictionary inside the record would not participate in reference tracking. If the same Dictionary instance appears both as a record constructor parameter AND as a standalone property, the standalone property gets `$id`/`$ref` but the record's copy is independent. This is the intended DDD behavior. + +The current Scenario 11 test uses `InterfaceRecordItem(int Id, string Description)` -- constructor params are value types. It does NOT test a record with a Dictionary constructor parameter. The plan's Rule 11 description mentions this case but the test scenario does not exercise it. + +**Recommendation**: The test as written is valid for the stated scenario. A more thorough test with a record that has a Dictionary constructor parameter would be a nice-to-have but is not blocking. The DDD justification covers this case architecturally. + +Verdict: Non-blocking. The test is sufficient for the stated acceptance criteria. + +## Test Scenario Verification -- Revised Phase 2 + +| Scenario | Rule(s) | Expected Result | Trace Verdict | +|----------|---------|-----------------|---------------| +| 7: Shared Dictionary after fix | R7 | `ReferenceEquals` is `true` | PASS -- Dictionary has default constructor, bypass converter skips it, STJ built-in + ReferenceHandler handles it. | +| 8: Record round-trip after fix | R8 | Deserialization succeeds, no `$id`/`$ref` in JSON | PASS -- Bypass converter claims record (parameterized constructor), delegates to inner options without ReferenceHandler. Functionally identical to v0.22.0 for records. | +| 9: Circular reference after fix | R9 | Circular reference preserved | PASS -- `CircularNode` has default constructor, bypass converter skips it, STJ built-in + ReferenceHandler handles cycles. | +| 10: Cross-type shared reference | R10 | Same Dictionary instance | PASS -- Both converter paths share the same resolver. Test uses non-Neatoo types as proxy. | +| 11: Record in graph with shared mutable refs | R11 | Record works, Dictionary shared | PASS -- Bypass converter serializes record without metadata. STJ built-in handles Dictionary with `$id`/`$ref`. Record and Dictionary coexist. | +| 12: Ordinal format preserved | R12 | All ordinal tests pass | PASS -- Ordinal converter claims IOrdinalSerializable types (not records). Bypass converter claims records (not IOrdinalSerializable). Non-overlapping. | +| 13: Existing tests pass; NoRefMetadata unchanged | R13 | Zero failures | PASS -- Bypass converter claims record before ReferenceHandler can add metadata. JSON output is clean. | + +## Requirements Context Check + +The revised plan's approach respects all documented requirements: + +1. **Published docs promise** (docs/serialization.md:10, docs/appendix/serialization.md:53-55) -- The bypass converter approach delivers shared-instance identity for mutable types while correctly excluding records (value objects). The docs promise shared identity for "when the same object is referenced by two properties" -- records as value objects are semantically not "the same object" even when they share a reference. The implementation is consistent with the promise when interpreted through DDD semantics. + +2. **v0.22.0 principle** (docs/serialization.md:120-124) -- The "converter-level, not serializer-level" section described the pre-fix state. This fix adds `ReferenceHandler` to options (serializer-level) for STJ's built-in converters, while keeping Neatoo converters unchanged (converter-level). The bypass converter extends the converter-level principle to records. This is a principled extension, not a revert. + +3. **Existing test intent** (InterfaceFactoryRecordSerializationTests.cs:120-140) -- The `InterfaceFactory_NonNeatooType_NoRefMetadata` test guards against record corruption by `$id`/`$ref`. The bypass converter preserves this guarantee by design. The test passes unchanged. + +4. **Anti-Pattern 9** -- The user-facing rule (do not mix Neatoo types with records) is unchanged. Only the technical explanation needs updating (docs deliverable). + +5. **STJ limitation** -- Fully addressed. The bypass converter prevents STJ from ever seeing reference metadata on parameterized-constructor types. The limitation cannot manifest. + +## Agent Phasing Review + +The revised plan's phasing is straightforward: + +- **Phase 1** (complete): Exploration tests and `NeatooPreserveReferenceHandler` built. +- **Phase 2** (revised): Single fresh agent. Creates `RecordBypassConverterFactory`, wires both components into `NeatooJsonSerializer`, creates Phase 2 acceptance tests, verifies all tests pass. + +The phasing is practical. Phase 2 is self-contained: one new file (~50-80 lines), one file modification (NeatooJsonSerializer constructor), test updates (remove Skip attributes, add Scenario 8 JSON assertion). No parallel phases needed. + +## Verdict + +**Approved.** The revised bypass converter approach cleanly resolves the Phase 2 blocker (STJ parameterized-constructor limitation). All 13 business rules trace through the implementation correctly. The detection rule ("all types with parameterized constructors") is simple and defensible. The DDD justification for record behavior is sound. The claim that `InterfaceFactory_NonNeatooType_NoRefMetadata` passes unchanged is correct -- the bypass converter intercepts records before `ReferenceHandler` can add metadata. + +Concerns C6-C10 are all Clarifying, not blocking: +- C6 (converter ordering) -- either ordering works today; adding bypass converter AFTER Neatoo converters is slightly safer +- C7 (inner options mutability) -- non-issue, construction sequence is correct +- C8 (caching strategy) -- minor implementation detail, Lazy recommended +- C9 (parameterized constructor detection) -- "no parameterless public constructor" check is sufficient +- C10 (Scenario 11 completeness) -- test covers the stated scenario; record-with-Dictionary-param is a nice-to-have + +No logic errors found. No blocking concerns. Requirements context is consistent with the design. + +--- + +## Implementation Contract -- Phase 2 (Revised) + +### Scope + +#### Files to Create +- `src/RemoteFactory/Internal/RecordBypassConverterFactory.cs` -- `JsonConverterFactory` + `JsonConverter` that bypasses reference handling for types with parameterized constructors (~50-80 lines) + +#### Files to Modify +- `src/RemoteFactory/Internal/NeatooJsonSerializer.cs` -- Add `ReferenceHandler = new NeatooPreserveReferenceHandler()` to Options constructor. Add `new RecordBypassConverterFactory()` to `Options.Converters` list. Remove the revert comment (lines 71-76). +- `src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/SharedReferenceTests.cs` -- Remove `Skip` attributes from Scenarios 7, 9, 10, 11. Update Scenario 8 to also assert no `$id`/`$ref` in JSON output (per Rule 8). + +#### Files to Verify (NO modification expected) +- `src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/InterfaceFactoryRecordSerializationTests.cs` -- `InterfaceFactory_NonNeatooType_NoRefMetadata` must pass unchanged +- `src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/SharedReferenceExplorationTests.cs` -- Phase 1 exploration tests. Scenarios 1 and 4 will change behavior (identity now preserved / circular refs now handled). These are IN SCOPE for modification since they document "current behavior" which is changing. + +### Out of Scope +- Documentation updates (docs/serialization.md, docs/appendix/record-reference-handling.md, CLAUDE-DESIGN.md) -- Step 9 deliverables +- Neatoo repository changes -- downstream, benefits automatically from shared resolver +- `ToRemoteDelegateRequest` per-parameter serialization -- pre-existing limitation +- Ordinal format changes -- not affected + +### Tests NOT to Modify (Sacred) +- `InterfaceFactory_SimpleRecord_RoundTrip` (line 39-48) -- must pass unchanged +- `InterfaceFactory_RecordWithCollection_RoundTrip` (line 55-71) -- must pass unchanged +- `InterfaceFactory_NestedRecord_RoundTrip` (line 79-88) -- must pass unchanged +- `InterfaceFactory_NullableRecord_ReturnsNull` (line 95-101) -- must pass unchanged +- `InterfaceFactory_NullableRecord_ReturnsValue` (line 104-113) -- must pass unchanged +- `InterfaceFactory_NonNeatooType_NoRefMetadata` (line 120-140) -- must pass unchanged +- All Design.Tests -- must pass unchanged +- All other existing IntegrationTests and UnitTests -- must pass unchanged + +### Phase 1 Exploration Tests (Expected Behavior Changes) +- `Scenario1_SharedDictionary_CurrentBehavior_IdentityLost` -- Currently asserts `ReferenceEquals` is `false`. After the fix, identity IS preserved. This test must be updated to reflect the new behavior (identity preserved). This is an IN SCOPE change. +- `Scenario2_RecordRoundTrip_CurrentBehavior_Succeeds` -- Should continue to pass (records still work). +- `Scenario4_CircularReference_NoHandler_ThrowsJsonException` -- Currently asserts `JsonException`. After the fix, circular references are handled. This test must be updated to reflect the new behavior. This is an IN SCOPE change. + +### Verification Gates + +1. After creating `RecordBypassConverterFactory.cs`: build succeeds +2. After modifying `NeatooJsonSerializer.cs` (wiring both components): build succeeds +3. After wiring: run `InterfaceFactory_NonNeatooType_NoRefMetadata` -- MUST pass unchanged (if it fails, STOP immediately -- the bypass converter is not working as expected) +4. After wiring: run all `InterfaceFactoryRecordSerializationTests` -- all 6 tests must pass +5. After wiring: remove Skip from Phase 2 tests (Scenarios 7, 9, 10, 11) and run them -- all must pass +6. After all changes: run full test suite (`dotnet test src/Neatoo.RemoteFactory.sln`) -- zero failures +7. Verify on both net9.0 and net10.0 + +### Stop Conditions + +- If `InterfaceFactory_NonNeatooType_NoRefMetadata` fails after wiring: STOP. The bypass converter is not claiming the record type. Investigate `CanConvert` logic. +- If `InterfaceFactory_RecordWithCollection_RoundTrip` or `InterfaceFactory_NestedRecord_RoundTrip` fails: STOP. These are out-of-scope sacred tests. The bypass converter is not preventing reference metadata on records with reference-type constructor parameters. +- If any Design.Tests fail: STOP. These are out-of-scope. +- If any other existing test fails: STOP and report. + +### Open Decision for User + +**Converter ordering**: The plan says to add `RecordBypassConverterFactory` BEFORE Neatoo converters. My analysis (Concern C6) suggests adding it AFTER Neatoo converters is slightly safer (Neatoo converters get first priority). Either works today because Neatoo entity types do not have parameterized constructors. I recommend AFTER for defense-in-depth but will follow the plan's ordering if the user prefers. + +--- + +## Implementation Progress -- Phase 1 + +### Files Created + +1. **`src/Tests/RemoteFactory.IntegrationTests/TestTargets/TypeSerialization/SharedReferenceTargets.cs`** + - `SharedDictionaryHolder` -- plain class with two `Dictionary` properties + - `CircularNode` -- plain class with `Name` and `Next` properties for circular reference + - `SharedRecordHolder` -- wrapper with two properties holding the same record instance (needed for Scenario 3 per Concern C1) + - `CrossTypeSharedReferenceHolder` -- plain class with scalar + shared Dictionary properties (Scenario 10) + - `RecordWithSharedDictionaryHolder` -- plain class with record + shared Dictionary properties (Scenario 11) + +2. **`src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/SharedReferenceExplorationTests.cs`** + - 6 test methods covering Scenarios 1-6 + - Temporary `TestNeatooPreserveReferenceHandler` class nested in the test class for Scenario 6 + +3. **`src/RemoteFactory/Internal/NeatooPreserveReferenceHandler.cs`** -- Production-ready custom `ReferenceHandler` subclass. + +4. **`src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/SharedReferenceTests.cs`** -- Phase 2 acceptance tests. Scenarios 7, 9, 10, 11 marked Skip. Scenario 8 passes. + +### Test Results (Phase 1 -- All Pass) + +| Scenario | Test Method | Result | +|----------|------------|--------| +| 1 | `Scenario1_SharedDictionary_CurrentBehavior_IdentityLost` | PASS | +| 2 | `Scenario2_RecordRoundTrip_CurrentBehavior_Succeeds` | PASS | +| 3 | `Scenario3_RecordWithReferenceHandlerPreserve_ThrowsOnDeserialization` | PASS | +| 4 | `Scenario4_CircularReference_NoHandler_ThrowsJsonException` | PASS | +| 5 | `Scenario5_SharedDictionary_ReferenceHandlerPreserve_IdentityPreserved` | PASS | +| 6 | `Scenario6_CustomReferenceHandler_NeatooReferenceResolver_IdentityPreserved` | PASS | + +### Phase 2 Blocker (Now Resolved by Revised Plan) + +Phase 2 originally attempted adding `ReferenceHandler` without a bypass converter. STJ threw `NotSupportedException` for records with reference-type constructor parameters (`Items: IReadOnlyList`). The revised plan introduces `RecordBypassConverterFactory` to prevent reference metadata on parameterized-constructor types. + +--- + +## Completion Evidence -- Phase 1 + +### Test Output Summary + +``` +Test Run Successful (net9.0 and net10.0). +Phase 1 tests: 6 passed +Full suite: 494 passed, 7 skipped (3 pre-existing + 4 Phase 2 skips), 0 failed +``` + +--- + +## Implementation Progress -- Phase 2 (Revised) + +### Files Created + +1. **`src/RemoteFactory/Internal/RecordBypassConverterFactory.cs`** (~120 lines) + - `RecordBypassConverterFactory : JsonConverterFactory` -- claims types with parameterized constructors (no public parameterless ctor AND at least one public ctor with parameters) + - `RecordBypassConverter : JsonConverter` -- delegates Read/Write to `JsonSerializer.Serialize/Deserialize` with inner options that have `ReferenceHandler = null` and no `RecordBypassConverterFactory` + - Inner options created with double-check locking (`lock` + null check), cached for the lifetime of the factory instance + - Handles nulls in Read (returns default) and Write (writes null token) + +### Files Modified + +2. **`src/RemoteFactory/Internal/NeatooJsonSerializer.cs`** -- Constructor changes: + - Added `ReferenceHandler = new NeatooPreserveReferenceHandler()` to `JsonSerializerOptions` initializer + - Removed the 6-line explanatory comment about the Phase 2 revert + - Added `this.Options.Converters.Add(new RecordBypassConverterFactory())` AFTER the existing Neatoo converter loop (per user decision: Neatoo converters get first priority) + - Added comment explaining the ordering rationale + +3. **`src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/SharedReferenceExplorationTests.cs`** -- Phase 1 exploration tests updated for new behavior: + - Scenario 1: renamed `Scenario1_SharedDictionary_CurrentBehavior_IdentityLost` -> `Scenario1_SharedDictionary_IdentityPreserved`, assertion changed from `Assert.False(ReferenceEquals)` to `Assert.True(ReferenceEquals)`, comments updated + - Scenario 4: renamed `Scenario4_CircularReference_NoHandler_ThrowsJsonException` -> `Scenario4_CircularReference_Handled`, changed from `Assert.Throws` to asserting the circular graph is preserved after round-trip + - Scenarios 2, 3, 5, 6: unchanged (their behavior is unaffected by the fix) + +4. **`src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/SharedReferenceTests.cs`** -- Phase 2 acceptance tests: + - Removed `Skip` attribute from Scenario 7 (`Scenario7_SharedDictionary_AfterFix_IdentityPreserved`) + - Removed `Skip` attribute from Scenario 9 (`Scenario9_CircularReference_AfterFix_IdentityPreserved`) + - Removed `Skip` attribute from Scenario 10 (`Scenario10_CrossTypeSharedReference_DictionaryWithMixedProperties`) + - Removed `Skip` attribute from Scenario 11 (`Scenario11_RecordInGraphWithSharedMutableRefs`) + - No other changes to test bodies or assertions + +### Files Verified Unchanged + +- `src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/InterfaceFactoryRecordSerializationTests.cs` -- all 6 tests pass unchanged, including `InterfaceFactory_NonNeatooType_NoRefMetadata` which asserts `DoesNotContain("$id")` and `DoesNotContain("$ref")` +- `src/Tests/RemoteFactory.IntegrationTests/TestTargets/TypeSerialization/SharedReferenceTargets.cs` -- no changes needed +- `src/RemoteFactory/Internal/NeatooPreserveReferenceHandler.cs` -- already built in Phase 1, no changes + +### Design Decision: Converter Ordering + +Per user decision, `RecordBypassConverterFactory` is added AFTER Neatoo converters in the list. The ordering in `NeatooJsonSerializer` constructor is: + +1. `NeatooOrdinalConverterFactory` (if ordinal format) +2. Neatoo converter factories from DI (NeatooInterfaceJsonConverterFactory + downstream converters) +3. `RecordBypassConverterFactory` + +This gives Neatoo converters first priority. The bypass converter only picks up types that no Neatoo converter claimed. This aligns with Concern C6 analysis: defense-in-depth against future Neatoo types that might have parameterized constructors. + +### Verification Gates -- All Passed + +| Gate | Expected | Actual | +|------|----------|--------| +| 1. `RecordBypassConverterFactory.cs` compiles | Build succeeds | PASS -- 0 errors, 0 warnings | +| 2. `NeatooJsonSerializer.cs` wiring compiles | Build succeeds | PASS -- 0 errors, 0 warnings | +| 3. `InterfaceFactory_NonNeatooType_NoRefMetadata` unchanged | Passes | PASS -- both net9.0 and net10.0 | +| 4. All `InterfaceFactoryRecordSerializationTests` | 6 pass | PASS -- 6 pass on both frameworks | +| 5. Phase 2 tests unskipped | Scenarios 7, 9, 10, 11 pass | PASS -- all pass on both frameworks | +| 6. Full test suite | Zero failures | PASS -- see below | +| 7. Both net9.0 and net10.0 | All pass | PASS | + +--- + +## Completion Evidence -- Phase 2 (Revised) + +### Full Test Suite Results + +| Test Project | Framework | Passed | Failed | Skipped | Total | +|---|---|---|---|---|---| +| RemoteFactory.UnitTests | net9.0 | 490 | 0 | 0 | 490 | +| RemoteFactory.UnitTests | net10.0 | 490 | 0 | 0 | 490 | +| RemoteFactory.IntegrationTests | net9.0 | 498 | 0 | 3 | 501 | +| RemoteFactory.IntegrationTests | net10.0 | 498 | 0 | 3 | 501 | +| Design.Tests | net9.0 | 42 | 0 | 0 | 42 | +| Design.Tests | net10.0 | 42 | 0 | 0 | 42 | + +**Zero failures across all test projects and both frameworks.** + +The 3 skipped tests are pre-existing `ShowcasePerformanceTests` (unrelated to this work). + +### Test Scenario Mapping + +| Plan Scenario | Test Method | File | Status | +|---|---|---|---| +| 7: Shared Dictionary after fix | `Scenario7_SharedDictionary_AfterFix_IdentityPreserved` | SharedReferenceTests.cs | PASS | +| 8: Record round-trip after fix | `Scenario8_RecordRoundTrip_AfterFix_Succeeds` | SharedReferenceTests.cs | PASS | +| 9: Circular reference after fix | `Scenario9_CircularReference_AfterFix_IdentityPreserved` | SharedReferenceTests.cs | PASS | +| 10: Cross-type shared reference | `Scenario10_CrossTypeSharedReference_DictionaryWithMixedProperties` | SharedReferenceTests.cs | PASS | +| 11: Record in mixed graph | `Scenario11_RecordInGraphWithSharedMutableRefs` | SharedReferenceTests.cs | PASS | +| 12: Ordinal format preserved | All existing ordinal tests | RemoteFactory.UnitTests + IntegrationTests | PASS (490 + 498 passing) | +| 13: Existing tests no regression | Full suite | All test projects | PASS (zero failures) | + +Also covered by updated Phase 1 exploration tests: +| Plan Scenario | Updated Exploration Test | Status | +|---|---|---| +| 7 (proxy) | `Scenario1_SharedDictionary_IdentityPreserved` | PASS | +| 9 (proxy) | `Scenario4_CircularReference_Handled` | PASS | + +### Sacred Tests Verification + +All sacred tests passed unchanged (no modifications made): + +| Test | Status | +|---|---| +| `InterfaceFactory_SimpleRecord_RoundTrip` | PASS | +| `InterfaceFactory_RecordWithCollection_RoundTrip` | PASS | +| `InterfaceFactory_NestedRecord_RoundTrip` | PASS | +| `InterfaceFactory_NullableRecord_ReturnsNull` | PASS | +| `InterfaceFactory_NullableRecord_ReturnsValue` | PASS | +| `InterfaceFactory_NonNeatooType_NoRefMetadata` | PASS | +| All Design.Tests (42 tests) | PASS | + +### Implementation Contract Status + +| Contract Item | Status | +|---|---| +| Create `RecordBypassConverterFactory.cs` | DONE | +| Wire `NeatooPreserveReferenceHandler` into options | DONE | +| Wire `RecordBypassConverterFactory` into options (AFTER Neatoo converters) | DONE | +| Remove revert comment from NeatooJsonSerializer | DONE | +| Unskip Scenarios 7, 9, 10, 11 | DONE | +| Update Phase 1 exploration tests (Scenarios 1, 4) | DONE | +| Verify `InterfaceFactory_NonNeatooType_NoRefMetadata` unchanged | DONE | +| Full test suite zero failures | DONE | +| Both net9.0 and net10.0 | DONE | + +--- + +## Implementation Progress -- Phase 2 (Revised) + +### Status: Approved -- Ready for Implementation diff --git a/docs/plans/completed/shared-reference-handling-plan.memory/requirements-documenter.md b/docs/plans/completed/shared-reference-handling-plan.memory/requirements-documenter.md new file mode 100644 index 00000000..e0451472 --- /dev/null +++ b/docs/plans/completed/shared-reference-handling-plan.memory/requirements-documenter.md @@ -0,0 +1,64 @@ +# Requirements Documenter Memory - Shared Reference Handling + +**Date:** 2026-03-21 +**Plan:** shared-reference-handling-plan.md +**Todo:** shared-reference-handling-non-custom-types.md + +## Work Completed + +### Step 2: Categorize Changes + +Reviewed all 13 business rules in the plan. Categorization: + +| Rule | Source | Category | Action | +|------|--------|----------|--------| +| 1-6 | Phase 1 exploration (existing behavior documentation) | Existing/baseline | No docs update needed -- these document pre-fix behavior | +| 7 | NEW -- shared Dictionary identity for mutable types | New rule | Updated docs/serialization.md, CLAUDE-DESIGN.md | +| 8 | NEW -- RecordBypassConverterFactory for parameterized-constructor types | New rule | Updated docs/serialization.md, CLAUDE-DESIGN.md, verified appendix | +| 9 | NEW -- circular reference support for mutable types | New rule | Updated docs/serialization.md | +| 10 | NEW -- cross-type shared references (Neatoo + non-Neatoo) | New rule | Updated docs/serialization.md | +| 11 | NEW -- record with nested mutable type (DDD semantics) | New rule | Updated docs/serialization.md, verified appendix | +| 12 | NEW -- ordinal format coexistence | New rule | No specific doc update needed (coexistence is implicit) | +| 13 | Sacred tests -- zero regressions | Existing constraint | No doc update needed | + +### Step 3: Files Directly Updated + +1. **`src/Design/CLAUDE-DESIGN.md`** + - Anti-Pattern 9 "Why it matters": Replaced stale "no ReferenceHandler set / converter-level concern" with two-path strategy (NeatooPreserveReferenceHandler + RecordBypassConverterFactory) + - Quick Decisions Table record row: Changed "serialized without `$id`/`$ref`" to "bypass reference handling (`RecordBypassConverterFactory`)" + - Common Mistakes #10: Changed "cause serialization mismatches" to "bypass reference handling entirely" + +2. **`docs/serialization.md`** + - Replaced entire "Scope: Converter-Level, Not Serializer-Level" subsection with "Reference Handling by Type Category" + - New content: three-row type category table (Neatoo types / mutable reference types / parameterized constructors), shared resolver explanation, nested-mutable-in-record DDD rationale, cross-link to appendix + +3. **`docs/appendix/serialization.md`** + - Updated section 3 ("Shared Object Identity Is Lost"): added paragraph clarifying scope of reference tracking (Neatoo types + mutable reference types yes, records no) with cross-link to record-reference-handling appendix + +4. **`docs/appendix/record-reference-handling.md`** + - Already existed and is comprehensive. Verified it matches implementation. No changes needed. + +5. **`docs/plans/shared-reference-handling-plan.md`** + - Set status to "Requirements Documented" + - Updated Documentation deliverables checklist (marked CLAUDE-DESIGN.md item as done) + - Added "Requirements Documentation (Step 9)" section with files updated and developer deliverables + +### Step 4: Developer Deliverables (NOT directly edited) + +1. **`src/Design/Design.Tests/FactoryTests/SerializationTests.cs:38`** -- Stale comment. Line 38 says "Circular references without proper handling" under the "NO" list. Should be updated because circular references in mutable types now work via NeatooPreserveReferenceHandler. Suggested change: move to "PARTIAL" section with note that circular references work for mutable types but not for types with parameterized constructors (records). + +2. **(Optional) Design.Tests shared-reference test** -- Consider adding a test demonstrating shared mutable object identity preservation on a [Factory] class. Would close Gap 10 from the requirements review. Not blocking since IntegrationTests/SharedReferenceTests.cs covers the scenarios. + +### Stale Docs Identified by Reviewer -- Resolution Status + +| Location | Issue | Resolution | +|----------|-------|------------| +| `docs/serialization.md:120-124` | "Scope" section says "no ReferenceHandler set" | FIXED -- replaced with "Reference Handling by Type Category" | +| `src/Design/CLAUDE-DESIGN.md:419` | Anti-Pattern 9 references converter-level mechanism | FIXED -- updated to two-path strategy | +| `src/Design/Design.Tests/FactoryTests/SerializationTests.cs:38` | "Circular references without proper handling" partially stale | DEVELOPER DELIVERABLE -- listed for developer agent | + +### Design Debt and Completeness + +- No Design Debt items resolved (this feature was not in the Design Debt table) +- No Design Completeness Checklist items affected +- No anti-patterns added or removed -- Anti-Pattern 9 updated in-place (same rule, updated explanation) diff --git a/docs/plans/completed/shared-reference-handling-plan.memory/requirements-reviewer.md b/docs/plans/completed/shared-reference-handling-plan.memory/requirements-reviewer.md new file mode 100644 index 00000000..4011b56a --- /dev/null +++ b/docs/plans/completed/shared-reference-handling-plan.memory/requirements-reviewer.md @@ -0,0 +1,75 @@ +# Requirements Reviewer Memory — Shared Reference Handling Plan + +## Post-Implementation Verification (Step 7B) + +**Date:** 2026-03-21 +**Verdict:** REQUIREMENTS SATISFIED (with documentation gap noted) + +--- + +### Compliance Table + +| # | Requirement | Source | Status | Evidence | +|---|------------|--------|--------|----------| +| 1 | Published docs promise shared-instance identity for all types | `docs/serialization.md:10`, `docs/appendix/serialization.md:53-55` | SATISFIED | `NeatooPreserveReferenceHandler` wired into `NeatooJsonSerializer` options (`NeatooJsonSerializer.cs:71`). STJ built-in converters now emit `$id`/`$ref` for mutable reference types (Dictionary, List, plain classes). Scenario 7 test (`SharedReferenceTests.cs:31-67`) verifies shared Dictionary identity. Scenario 9 test (`SharedReferenceTests.cs:128-151`) verifies circular references. | +| 2 | Client-server architecture promises "single process" abstraction | `docs/client-server-architecture.md:3` | SATISFIED | Mutable reference types now preserve identity across serialization, closing the gap. Records (value objects) are excluded by design -- DDD semantics justify this. | +| 3 | v0.22.0 "converter-level, not serializer-level" principle | `docs/serialization.md:120-124`, `docs/release-notes/v0.22.0.md:16-17,24` | SATISFIED (with architectural evolution) | The implementation does set `options.ReferenceHandler`, partially reversing v0.22.0. However, this is deliberate and justified: Neatoo converters still access `NeatooReferenceResolver.Current` directly (converter-level), while STJ built-in converters use `options.ReferenceHandler` (serializer-level). Both paths share the same resolver instance. `RecordBypassConverterFactory` extends the converter-level principle to records by claiming them via a custom converter that delegates to inner options without `ReferenceHandler`. The three-path architecture (Neatoo converters, bypass converter, STJ built-in) coexists correctly. | +| 4 | Anti-Pattern 9 explanation references converter-level mechanism | `src/Design/CLAUDE-DESIGN.md:378-419` | STALE DOC (not a violation) | Anti-Pattern 9's "Why it matters" text at line 419 says "RemoteFactory's `JsonSerializerOptions` has no `ReferenceHandler` set." This is now factually incorrect -- options DO have `NeatooPreserveReferenceHandler`. However, the user-facing rule (do not mix Neatoo types with records) remains valid because the underlying STJ limitation is unchanged. The plan explicitly marks this as a Step 9 documentation deliverable. See Documentation Gap below. | +| 5 | `InterfaceFactory_NonNeatooType_NoRefMetadata` test guards against record corruption | `InterfaceFactoryRecordSerializationTests.cs:121-140` | SATISFIED | Test passes unchanged per developer evidence (zero failures). The `RecordBypassConverterFactory` claims the record type (`InterfaceRecordWithCollection` has parameterized constructor, no parameterless constructor), delegates to inner options without `ReferenceHandler`, so no `$id`/`$ref` metadata appears. The test's `Assert.DoesNotContain("$id", json)` and `Assert.DoesNotContain("$ref", json)` continue to pass. Test intent (records are not corrupted) is preserved. | +| 6 | STJ parameterized-constructor limitation | Microsoft docs, dotnet/runtime#73302 | SATISFIED | `RecordBypassConverterFactory.CanConvert` (`RecordBypassConverterFactory.cs:36-58`) correctly detects types with parameterized constructors (no public parameterless constructor + at least one public constructor with parameters). These types are serialized via inner options with `ReferenceHandler = null`, preventing STJ's `NotSupportedException`. Phase 1 Scenario 3 test (`SharedReferenceExplorationTests.cs:130-169`) confirms the STJ limitation exists. Phase 2 Scenario 8 test (`SharedReferenceTests.cs:83-113`) confirms records work after the fix. | +| 7 | Design project serialization tests | `src/Design/Design.Tests/FactoryTests/SerializationTests.cs` | SATISFIED | Developer reports 42 design tests pass per framework. The seven serialization tests in `SerializationTests.cs` do not test shared object identity (confirmed by review of the file). They test Create, Fetch, ValueObject, Collection, Nullable, Modified, and SaveMeta round-trips. These are unaffected because: (a) in Ordinal format, the ordinal converter claims `[Factory]` types before either the bypass converter or `ReferenceHandler` intervene; (b) in Named format, the bypass converter claims records without reference metadata; (c) mutable types get `$id`/`$ref` which is additive, not breaking. | +| 8 | Design Debt table has no entry for this feature | `src/Design/CLAUDE-DESIGN.md:732-738` | SATISFIED | Verified the Design Debt table. It contains entries for: private setter support, OR logic for AspAuthorize, automatic Remote detection, collection factory injection, and IEnumerable serialization. None relate to shared reference handling for non-custom types. No design debt boundary was crossed. | + +--- + +### Implementation Details Verified + +**Component 1: NeatooPreserveReferenceHandler** (`src/RemoteFactory/Internal/NeatooPreserveReferenceHandler.cs`) +- Correctly extends `ReferenceHandler` and delegates `CreateResolver()` to `NeatooReferenceResolver.Current` +- Throws `InvalidOperationException` with clear message if `Current` is null +- Internal sealed class -- appropriate visibility + +**Component 2: RecordBypassConverterFactory** (`src/RemoteFactory/Internal/RecordBypassConverterFactory.cs`) +- `CanConvert` detection rule: no public parameterless constructor AND at least one public constructor with parameters -- matches plan's specification and STJ's own heuristic +- Inner options constructed via copy constructor with `ReferenceHandler = null` and self removed from converters list (prevents recursion) +- Double-checked locking for thread-safe inner options caching +- `RecordBypassConverter` handles null correctly in both Read and Write + +**NeatooJsonSerializer wiring** (`src/RemoteFactory/Internal/NeatooJsonSerializer.cs:68-94`) +- `ReferenceHandler = new NeatooPreserveReferenceHandler()` set on options (line 71) +- Converter ordering: ordinal converter first (if ordinal format), then Neatoo converters, then `RecordBypassConverterFactory` LAST +- NOTE: The plan's Design section originally said bypass converter should be "BEFORE other converters" but the implementation places it AFTER, with a comment explaining: "Neatoo converters get first priority -- they claim interfaces, abstract types, and IOrdinalSerializable types that have purpose-built converters." This is correct behavior: Neatoo types must be claimed by their own converters first; the bypass converter only picks up remaining types with parameterized constructors. The code comment at line 89-92 documents the rationale. + +**Resolver lifecycle** -- Unchanged from v0.22.0. Each Serialize/Deserialize method creates a `NeatooReferenceResolver`, sets `Current`, calls STJ, and clears `Current` in a finally block. The `NeatooPreserveReferenceHandler.CreateResolver()` returns this same resolver when STJ's built-in converters need it. + +--- + +### Unintended Side Effects Check + +1. **Generated code patterns in Design projects** -- Not affected. No changes to generator output. The bypass converter operates at runtime serialization, not at code generation. + +2. **Serialization contracts** -- Changed as intended. Mutable reference types now get `$id`/`$ref` metadata in their JSON. This is additive. Records do NOT get `$id`/`$ref`, preserving existing behavior. + +3. **Design project tests** -- All 42 pass per framework (developer evidence). Verified by tracing: the seven `SerializationTests.cs` tests exercise Create/Fetch/ValueObject/Collection/Nullable/Modified/SaveMeta round-trips, which are unaffected by the reference handler addition. + +4. **Published docs accuracy** -- See Documentation Gap below. + +5. **Reflection usage** -- `RecordBypassConverterFactory` uses `GetConstructors`, `GetParameters`, `MakeGenericType`, and `Activator.CreateInstance`. This follows the established pattern used by `NeatooInterfaceJsonConverterFactory` (line 37) and `NeatooOrdinalConverterFactory` (lines 109-110). In the `JsonConverterFactory` context, reflection is unavoidable and consistent with existing code. + +6. **Multi-targeting** -- `JsonSerializerOptions` copy constructor and `ReferenceHandler` API are available in both net9.0 and net10.0. Developer reports zero failures across both frameworks (490 unit + 498 integration tests per framework). + +--- + +### Documentation Gap (Step 9 Deliverable -- NOT a Violation) + +The plan explicitly identifies documentation updates as Step 9 deliverables. The following docs are now stale but are scheduled to be updated: + +1. **`docs/serialization.md:120-124`** -- "Scope: Converter-Level, Not Serializer-Level" section says "RemoteFactory's `JsonSerializerOptions` has no `ReferenceHandler` set." This is now factually incorrect. Options have `NeatooPreserveReferenceHandler`. + +2. **`src/Design/CLAUDE-DESIGN.md:419`** -- Anti-Pattern 9 "Why it matters" says the same. Now stale. + +3. **`docs/appendix/record-reference-handling.md`** -- Does not exist yet. Plan specifies this as a new doc explaining DDD rationale for bypass converter. + +4. **`src/Design/Design.Tests/FactoryTests/SerializationTests.cs:38`** -- Comment says "Circular references without proper handling" are in the "NO" category. This is now potentially misleading since mutable types DO handle circular references via `ReferenceHandler`. However, the qualification "without proper handling" is still technically accurate. + +These are not requirements violations because the plan defers documentation to a dedicated step after implementation verification. diff --git a/docs/release-notes/index.md b/docs/release-notes/index.md index 996ee89a..4dd0ddec 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.23.0](v0.23.0.md) | 2026-03-21 | Shared reference handling for mutable types, record bypass converter | | [v0.22.0](v0.22.0.md) | 2026-03-20 | Serializer responsibility redesign: converter-level reference handling | | [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 | diff --git a/docs/release-notes/v0.22.0.md b/docs/release-notes/v0.22.0.md index 81fc348c..eeb377e0 100644 --- a/docs/release-notes/v0.22.0.md +++ b/docs/release-notes/v0.22.0.md @@ -3,7 +3,7 @@ layout: default title: "v0.22.0" description: "Release notes for Neatoo RemoteFactory v0.22.0" parent: Release Notes -nav_order: 1 +nav_order: 2 --- # v0.22.0 - Serializer Responsibility Redesign diff --git a/docs/release-notes/v0.23.0.md b/docs/release-notes/v0.23.0.md new file mode 100644 index 00000000..8185d98f --- /dev/null +++ b/docs/release-notes/v0.23.0.md @@ -0,0 +1,60 @@ +--- +layout: default +title: "v0.23.0" +description: "Release notes for Neatoo RemoteFactory v0.23.0" +parent: Release Notes +nav_order: 1 +--- + +# v0.23.0 - Shared Reference Handling for Mutable Types + +**Release Date:** 2026-03-21 +**NuGet:** [Neatoo.RemoteFactory 0.23.0](https://nuget.org/packages/Neatoo.RemoteFactory/0.23.0) + +## Overview + +Restores `$id`/`$ref` reference tracking for mutable reference types (Dictionary, List, plain classes) while keeping records safe from STJ's parameterized-constructor limitation. Shared object identity is now preserved across serialization round-trips for types with default constructors, fulfilling RemoteFactory's promise to abstract away the client/server physical layer. + +Records (value objects with parameterized constructors) bypass reference tracking entirely — this is correct DDD behavior, not a limitation. Value objects are defined by their values, not their identity. + +## What's New + +- **Shared reference identity for mutable types.** When two properties reference the same `Dictionary`, `List`, or plain class instance, RemoteFactory now preserves that identity across serialization. After round-trip, both properties point to the same object (`ReferenceEquals` returns `true`). + +- **Circular reference support for mutable types.** Parent-child circular references in classes with default constructors are now handled without infinite loops or exceptions. + +- **`NeatooPreserveReferenceHandler`.** A custom `ReferenceHandler` that bridges STJ's built-in converters to `NeatooReferenceResolver.Current`. Both Neatoo's custom converters and STJ's built-in converters share the same resolver instance, enabling cross-type reference tracking. + +- **`RecordBypassConverterFactory`.** A `JsonConverterFactory` that claims types with parameterized constructors (records, immutable types) and serializes them without reference metadata. Prevents STJ's `ObjectWithParameterizedCtorRefMetadataNotSupported` exception. + +- **New appendix documentation.** [`docs/appendix/record-reference-handling.md`](../appendix/record-reference-handling.md) explains the DDD rationale, the STJ limitation, and the three-path serialization architecture. + +## How It Works + +RemoteFactory's serializer now uses three paths: + +| Type Category | Converter | Reference Tracking | +|--------------|-----------|-------------------| +| Neatoo entities/lists | Neatoo custom converters | `$id`/`$ref` via `NeatooReferenceResolver.Current` directly | +| Mutable reference types (Dictionary, List, classes with default ctors) | STJ built-in converters | `$id`/`$ref` via `NeatooPreserveReferenceHandler` | +| Records / types with parameterized ctors | `RecordBypassConverterFactory` | None — value objects, duplication is correct | + +All three paths share the same `NeatooReferenceResolver` instance per serialization operation, so cross-type reference identity is maintained. + +## Bug Fixes + +- **Fixed shared object identity loss for non-custom types.** Previously, two properties pointing to the same Dictionary became independent copies after serialization round-trip. Now preserved via `$id`/`$ref`. + +- **Fixed circular reference crash for mutable types.** Circular references in plain classes (e.g., parent-child bidirectional) previously threw `JsonException` (max depth exceeded). Now handled correctly. + +## Commits + +- Add `NeatooPreserveReferenceHandler` — custom `ReferenceHandler` bridging STJ to `NeatooReferenceResolver.Current` +- Add `RecordBypassConverterFactory` — bypasses reference handling for parameterized-constructor types +- Wire both components into `NeatooJsonSerializer` options +- Add shared reference exploration tests (Phase 1) and acceptance tests (Phase 2) +- Update serialization docs, CLAUDE-DESIGN.md Anti-Pattern 9, and serialization appendix +- Create new appendix: `docs/appendix/record-reference-handling.md` + +**Builds on:** [v0.22.0](v0.22.0.md) converter-level reference handling redesign +**Related:** [Shared Reference Handling Plan](../plans/completed/shared-reference-handling-plan.md) diff --git a/docs/serialization.md b/docs/serialization.md index a5b9d935..7340e30c 100644 --- a/docs/serialization.md +++ b/docs/serialization.md @@ -117,11 +117,21 @@ 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: Converter-Level, Not Serializer-Level +### Reference Handling by Type Category -Reference preservation (`$id`/`$ref` metadata) is a converter-level concern. RemoteFactory's `JsonSerializerOptions` has no `ReferenceHandler` set -- System.Text.Json serializes types natively unless a custom converter intervenes. Neatoo's custom converters access a per-operation `NeatooReferenceResolver` via a static `AsyncLocal` accessor (`NeatooReferenceResolver.Current`) to add `$id`/`$ref` metadata for Neatoo types. Plain records and DTOs have no custom converter, so they are serialized by System.Text.Json without reference metadata. This means plain records/DTOs do not support circular references, but they do support parameterized constructors (primary constructors) without issue. +RemoteFactory uses a two-path strategy so that reference tracking works for mutable types while records (DDD value objects) serialize cleanly: -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's native STJ serialization does not produce reference metadata, but the embedded Neatoo type's converter expects the resolver to be tracking references. Use either pure Neatoo types or pure records/DTOs. +| Type Category | Examples | Reference Handling | Rationale | +|---------------|----------|--------------------|-----------| +| **Neatoo types** (custom converters) | Entities, lists with `[Factory]` | `$id`/`$ref` via `NeatooReferenceResolver.Current` | Converter-level, unchanged from v0.22.0 | +| **Mutable reference types** (STJ built-in) | `Dictionary`, `List`, plain classes with default constructors | `$id`/`$ref` via `NeatooPreserveReferenceHandler` on `JsonSerializerOptions` | STJ's built-in converters participate in reference tracking through the same resolver | +| **Types with parameterized constructors** | Records, immutable classes | No `$id`/`$ref` -- `RecordBypassConverterFactory` claims them | STJ cannot deserialize reference metadata on parameterized-constructor types; records are value objects where duplication is semantically correct | + +All three paths share the same `NeatooReferenceResolver` instance per serialization operation, so cross-type reference identity is maintained. For example, the same `Dictionary` instance referenced by both a Neatoo entity property and a plain class property will be serialized once (with `$id`) and referenced (with `$ref`) in both locations. + +A mutable type nested inside a record's constructor parameters is serialized as an independent copy. This is correct DDD behavior -- records are value objects, and their internal state is logically independent even if the same instance was shared at runtime. See [Appendix: Record Reference Handling](appendix/record-reference-handling.md) for the full rationale. + +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 bypasses reference handling entirely (including its subtree), but the embedded Neatoo type's converter expects the resolver to be tracking references. Use either pure Neatoo types or pure records/DTOs. ## Debugging diff --git a/docs/todos/completed/shared-reference-handling-non-custom-types.md b/docs/todos/completed/shared-reference-handling-non-custom-types.md new file mode 100644 index 00000000..a64b64e4 --- /dev/null +++ b/docs/todos/completed/shared-reference-handling-non-custom-types.md @@ -0,0 +1,242 @@ +# Shared Reference Handling for Non-Custom Types + +**Status:** Complete +**Priority:** High +**Created:** 2026-03-20 +**Last Updated:** 2026-03-21 + +--- + +## Problem + +RemoteFactory v0.22.0 removed `ReferenceHandler` from `JsonSerializerOptions` as part of the `NeatooReferenceResolver.Current` migration. This means STJ's built-in converters for non-Neatoo types (e.g., `Dictionary`, `List`, plain objects) no longer participate in `$id`/`$ref` reference tracking. + +Neatoo's custom converters (`NeatooBaseJsonTypeConverter`, `NeatooListBaseJsonTypeConverter`) handle `$id`/`$ref` for Neatoo entities and lists directly via `NeatooReferenceResolver.Current`. But when a Neatoo entity has properties of plain .NET types that share the same object instance, STJ serializes them as two separate copies — shared identity is lost on round-trip. + +It breaks things having ReferenceHandler and it breaks things not having it: + +- **With `ReferenceHandler`:** STJ injects `$id`/`$ref` on ALL types, including records with parameterized constructors. STJ can't deserialize its own `$ref` metadata on those records — throws `ObjectWithParameterizedCtorRefMetadataNotSupported`. +- **Without `ReferenceHandler`:** Non-custom types lose shared-reference identity after deserialization. + +## Design Question + +RemoteFactory v0.22.0 provides `NeatooReferenceResolver` as a tool that Neatoo converters use explicitly. The question is whether `NeatooJsonSerializer` should also wire a `ReferenceHandler` into `JsonSerializerOptions` so that STJ's built-in converters participate in reference tracking too. + +**Option A: Restore ReferenceHandler on options** — `NeatooJsonSerializer` sets `options.ReferenceHandler` to a handler that delegates `CreateResolver()` to `NeatooReferenceResolver.Current`. All types (Neatoo and non-Neatoo) participate in `$id`/`$ref`. Risk: could introduce unexpected `$id`/`$ref` tokens in output for types that don't need it, or cause errors with types that don't support reference preservation. + +**Option B: Keep current design (tool, not always-on)** — Only Neatoo's custom converters do `$id`/`$ref`. Shared references for plain .NET types are not preserved. This is simpler and more predictable but loses shared-reference identity for non-Neatoo property types. + +**Option C: Neatoo adds this feature** — Neatoo (the DDD framework) is the one that cares about reference identity. Neatoo could handle shared-reference tracking for non-Neatoo property values within its own converters, rather than pushing this into RemoteFactory. + +## Context + +Discovered during Neatoo's migration to RemoteFactory v0.22.0. The failing Neatoo test: + +- **`FatClientValidate_Deserialize_SharedDictionaryReference`** — Assigns the same `Dictionary` instance to two properties (`Data` and `Data2`) on a Neatoo entity. After serialize/deserialize round-trip, asserts `Assert.AreSame(newTarget.Data, newTarget.Data2)`. This fails because the dictionary is serialized twice without `$id`/`$ref`. + +## Related Work + +- Completed: [Serializer Responsibility Redesign](completed/serializer-responsibility-redesign.md) — v0.22.0 redesign that removed `ReferenceHandler` from options +- Completed (Neatoo): `Neatoo/docs/todos/completed/remotefactory-serializer-migration.md` — Neatoo converter migration to v0.22.0, [Ignore]d the `SharedDictionaryReference` test +- Completed: [Record Deserialization Ref Metadata](completed/record-deserialization-ref-metadata.md) — the original v0.21.3 bug + +## Key Files + +- `src/RemoteFactory/Internal/NeatooReferenceResolver.cs` — the `AsyncLocal` resolver +- `src/RemoteFactory/Internal/NeatooJsonSerializer.cs` — serializer that manages resolver lifecycle +- Neatoo: `src/Neatoo.UnitTest/Integration/Concepts/Serialization/FatClientValidateTests.cs` — `FatClientValidate_Deserialize_SharedDictionaryReference` test (currently [Ignore]d) + +## Tests to Analyze + +These Neatoo tests exercise shared-reference behavior for non-Neatoo types and are affected by this decision: + +| Test | File | Line | What It Tests | +|------|------|------|---------------| +| `FatClientValidate_Deserialize_SharedDictionaryReference` | `Neatoo/src/Neatoo.UnitTest/Integration/Concepts/Serialization/FatClientValidateTests.cs` | 209 | Same `Dictionary` assigned to two properties — asserts `AreSame` after round-trip | + +These Neatoo tests exercise shared-reference behavior for **Neatoo types** and are NOT affected (Neatoo converters handle `$id`/`$ref` directly): + +| Test | File | Line | What It Tests | +|------|------|------|---------------| +| `FatClientEntity_Deserialize_Child_ParentRef` | `FatClientEntityTests.cs` | 86 | Child entity's Parent ref resolves to parent object after round-trip | +| `FatClientValidate_Deserialize_Child_ParentRef` | `FatClientValidateTests.cs` | 157 | ValidateBase child's Parent ref resolves after round-trip | +| `FatClientEntityList_Deserialize_Child_ParentRef` | `FatClientEntityListTests.cs` | 69 | Entity list child's Parent ref resolves after round-trip | + +--- + +## Clarifications + +**Q1 (Architect):** How common is shared non-Neatoo instances in practice? Beyond the test case, do real domain models typically share the same Dictionary/List/plain-object instance across multiple properties? + +**A1:** Edge case, but when it happens it's really hard to spot. RemoteFactory is billing itself as abstracting away the client/server physical layer. We should try as hard as we can to do that. + +**Q2 (Architect):** Cross-entity sharing? Could `EntityA.Data` and `EntityB.Data` (parent-child) both point to the same dictionary? That would require reference tracking across the entire graph, not just within one entity. + +**A2:** Yes, at least start with that scope. + +**Q3 (Architect):** Is Option C (Neatoo owns it) your leaning? If so, should this todo result in a documented design decision with no RemoteFactory code change? + +**A3:** Leaning is RemoteFactory owns it. That's why we're back here. + +**Q4 (Architect):** If Neatoo owns it, what RemoteFactory support is expected? + +**A4:** N/A — RemoteFactory owns it (see A3). + +**Q5 (Architect):** Circular references in non-Neatoo types — shared identity only, or also circular references (infinite serialization loops)? + +**A5:** Both. Can we re-create the issues with records and with dictionaries and duplicate refs and see if we can get all working? First, I want to see if the main difficulty was our own creation of having Neatoo and RemoteFactory sharing responsibilities. + +Architect confirmed **Ready**. + +--- + +## Requirements Review + +**Reviewer:** business-requirements-reviewer +**Reviewed:** 2026-03-20 +**Verdict:** APPROVED (with critical tension to resolve) + +### Relevant Requirements Found + +1. **Published docs explicitly promise shared-instance identity for all types (`docs/serialization.md:10`).** The serialization overview lists three things RemoteFactory handles beyond STJ. The second bullet reads: "Shared instance identity -- When the same object is referenced by two properties (e.g., a parent-child bidirectional reference), System.Text.Json duplicates it. RemoteFactory tracks object identity and serializes shared references as `$ref` pointers, preserving the graph structure." This claim is not qualified as "Neatoo types only." The todo's goal of extending reference handling to non-custom types would bring the implementation in line with this published promise. + +2. **Serialization appendix reinforces the general promise (`docs/appendix/serialization.md:53-55`).** Section "3. Shared Object Identity Is Lost" states: "When two properties reference the same object instance (common in aggregate root / child entity relationships), STJ duplicates it -- creating two independent copies. RemoteFactory preserves identity using `$id` / `$ref` pointers, maintaining the object graph structure across the wire." Again, no qualification that this applies only to Neatoo types. + +3. **Client-server architecture doc establishes the "single process" abstraction goal (`docs/client-server-architecture.md:3`).** "RemoteFactory lets you write your domain model as if it runs in a single process." The user's clarification A1 reinforces this: "RemoteFactory is billing itself as abstracting away the client/server physical layer. We should try as hard as we can to do that." Losing shared-identity for non-custom types violates this abstraction -- in a single process, two properties pointing at the same Dictionary are the same object; after round-trip, they become two copies. + +4. **v0.22.0 explicitly documents "Scope: Converter-Level, Not Serializer-Level" (`docs/serialization.md:120-124`).** This section states: "Plain records and DTOs have no custom converter, so they are serialized by System.Text.Json without reference metadata. This means plain records/DTOs do not support circular references, but they do support parameterized constructors (primary constructors) without issue." This is the documented current-state limitation. The todo proposes changing this behavior. + +5. **Anti-Pattern 9 ("Mixing Neatoo types with records") rationale (`src/Design/CLAUDE-DESIGN.md:378-419`).** The "Why it matters" explanation references the converter-level mechanism: "RemoteFactory's `JsonSerializerOptions` has no `ReferenceHandler` set." If this todo restores `ReferenceHandler` on options (Option A), Anti-Pattern 9's technical explanation would need to change. The user-facing rule (do not mix Neatoo types with records) may or may not change depending on implementation approach. + +6. **Existing test explicitly asserts records have NO `$id`/`$ref` (`src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/InterfaceFactoryRecordSerializationTests.cs:121-140`).** The test `InterfaceFactory_NonNeatooType_NoRefMetadata` serializes a record and asserts `Assert.DoesNotContain("$id", json)` and `Assert.DoesNotContain("$ref", json)`. If the implementation adds `ReferenceHandler` to options, this test will fail. This is a critical tension point -- the test was written to verify the v0.21.3/v0.22.0 fix. The todo's scope would intentionally reverse this behavior. + +7. **The STJ parameterized-constructor limitation remains (`docs/todos/completed/record-deserialization-ref-metadata.md:13-17`).** STJ cannot deserialize types with parameterized constructors when `$ref` metadata appears in the payload (`ObjectWithParameterizedCtorRefMetadataNotSupported`). This was the original v0.21.3 bug. If `ReferenceHandler` is restored globally (Option A), this bug would resurface for records. The architect must find a way to handle both: shared identity for non-custom types AND records with parameterized constructors. + +8. **`NeatooReferenceResolver` already provides the infrastructure (`src/RemoteFactory/Internal/NeatooReferenceResolver.cs`).** The resolver is created per-operation in `NeatooJsonSerializer`, tracks reference identity via `ReferenceEqualityComparer.Instance`, and supports `GetReference`/`AddReference`/`ResolveReference` operations. It is public and accessible via `AsyncLocal`. The mechanism exists; the question is how to wire non-custom types into it. + +9. **`NeatooInterfaceJsonTypeConverter` no longer handles `$id`/`$ref` (`src/RemoteFactory/Internal/NeatooInterfaceJsonTypeConverter.cs`).** The v0.22.0 release notes confirm dead reference handler code was removed from this converter's `Read()` method. The converter only handles `$type`/`$value` wrapping for interface-typed properties. It does not call `NeatooReferenceResolver` at all. If the todo needs interface-typed properties to participate in reference tracking, this converter will need changes. + +10. **SerializationTests in Design project test round-trip but not shared identity (`src/Design/Design.Tests/FactoryTests/SerializationTests.cs`).** The seven serialization tests verify property values survive round-trip (Create, Fetch, ValueObject, Collection, Nullable, Modified, SaveMeta). None test shared object identity (same instance referenced from multiple properties). There is no Design project test that exercises the scenario described in this todo. + +11. **"Circular references without proper handling" is listed as a NO in the round-trip guide (`src/Design/Design.Tests/FactoryTests/SerializationTests.cs:38`).** Line 38 of the test file's header comment lists "Circular references without proper handling" under "NO -- These will NOT serialize correctly." This documents the current limitation but does not prohibit fixing it. + +12. **Design Debt table has no entry for shared-reference handling of non-custom types (`src/Design/CLAUDE-DESIGN.md:728-739`).** This feature was not deliberately deferred. It is not listed in the Design Debt table. No "Reconsider When" condition applies. The todo does not contradict a deliberate non-implementation. + +13. **v0.22.0 release notes confirm no `ReferenceHandler` on options is the current design (`docs/release-notes/v0.22.0.md:16-17, 24`).** "Returns to a single `JsonSerializerOptions` instance with no `ReferenceHandler` set." And as a documented breaking change: "`options.ReferenceHandler` is no longer set on `JsonSerializerOptions`." + +### Gaps + +1. **No documented contract for what "shared instance identity" means for non-Neatoo types.** The published docs promise shared identity (Finding 1, 2) but the implementation only delivers it for Neatoo types with custom converters. There is no documented design decision about whether this gap is intentional or an oversight. The architect must establish: (a) Which non-custom types should participate (all? only reference types? only types without parameterized constructors?), (b) Whether cross-entity sharing is supported (EntityA.Data === EntityB.Data per clarification A2), (c) How this interacts with ordinal format. + +2. **No documented approach for handling the record/parameterized-constructor conflict alongside `ReferenceHandler`.** The todo identifies the core tension (with `ReferenceHandler`: records break; without it: shared identity lost) but does not propose a resolution strategy. This is the central design challenge and needs architectural analysis. Possible approaches include: selective `ReferenceHandler` that skips types with parameterized constructors, a custom `ReferenceHandler` subclass, or per-type converter logic. + +3. **No test infrastructure for shared-reference scenarios in RemoteFactory.** The only test is in the Neatoo repository (`FatClientValidate_Deserialize_SharedDictionaryReference`, currently [Ignore]d). RemoteFactory needs its own reproduction tests. The user explicitly asked for this in clarification A5: "Can we re-create the issues with records and with dictionaries and duplicate refs and see if we can get all working?" + +4. **No documented requirement for circular reference handling in non-custom types.** The user requested both shared identity AND circular references for non-custom types (clarification A5). Circular references in non-custom types are a harder problem than shared identity -- they can cause infinite serialization loops without a mechanism to break the cycle. The completed `record-deserialization-ref-metadata.md` plan (`docs/plans/completed/record-deserialization-ref-metadata.md:144`) explicitly assessed circular reference support for plain records/DTOs as "NOT trivial." + +5. **No analysis of ordinal format interaction.** The ordinal converter (`NeatooOrdinalConverter`) serializes as JSON arrays. Reference handling metadata (`$id`/`$ref`) is typically emitted as object properties. The architect must clarify whether shared-reference tracking is needed for ordinal format, and if so, how it interacts with array serialization. + +### Contradictions + +No hard contradictions with the Design Debt table or documented anti-patterns. However, there is a significant **tension** with the v0.22.0 design decision: + +1. **Tension with the "converter-level, not serializer-level" principle.** The v0.22.0 redesign (completed `serializer-responsibility-redesign.md`) established that reference handling is a converter-level concern. The serializer provides the resolver via `AsyncLocal`, but `JsonSerializerOptions` has no `ReferenceHandler`. Option A in this todo (restoring `ReferenceHandler` on options) would partially reverse this decision. This is not a contradiction per se -- the user has the authority to change direction -- but the architect should explicitly address why the v0.22.0 principle does not fully apply here, or propose an approach that extends the converter-level principle to non-custom types rather than reverting to a serializer-level mechanism. + +2. **Test `InterfaceFactory_NonNeatooType_NoRefMetadata` would break under Option A.** This is an existing passing test that asserts the current behavior. Per the sacred tests rule, this test's intent must be understood before modifying it. The test's intent was to verify the v0.21.3 record fix -- ensuring records are not corrupted by unwanted `$id`/`$ref`. If the implementation adds `ReferenceHandler` globally, this test's assertion is no longer correct. The architect must determine: (a) Is the test's intent still valid under the new design? (b) If so, how is the record problem solved alongside reference tracking? + +### Recommendations for Architect + +1. **Start with reproduction tests as the user requested (clarification A5).** Before designing a solution, create RemoteFactory-internal tests that reproduce: (a) shared Dictionary assigned to two properties -- assert identity after round-trip, (b) record with parameterized constructor -- assert it deserializes without error, (c) circular reference in a non-custom type. This will clarify the exact failure modes and constrain the solution space. + +2. **Evaluate a custom `ReferenceHandler` that is selective about which types get `$id`/`$ref`.** A custom `ReferenceHandler` subclass could create a resolver that skips types with parameterized constructors (records with primary constructors) while tracking reference types like Dictionary and List. This would thread the needle between the two conflicting requirements. + +3. **Consider extending the converter-level approach rather than reverting.** Instead of restoring `ReferenceHandler` on options (which reverses v0.22.0's principle), consider whether RemoteFactory could provide a converter (or set of converters) for common non-custom types (Dictionary, List, plain classes) that use `NeatooReferenceResolver.Current` directly -- similar to how Neatoo's converters do it. This would preserve the "converter-level, not serializer-level" principle while extending reference tracking. Evaluate whether this is feasible or too broad. + +4. **Address the `InterfaceFactory_NonNeatooType_NoRefMetadata` test explicitly.** This test asserts records have no `$id`/`$ref`. Any solution must either: (a) keep this assertion true (records still have no metadata, but other non-custom types do), or (b) update the test with a documented rationale for why the behavior changed. The test was created to guard against the `ObjectWithParameterizedCtorRefMetadataNotSupported` bug -- if the bug is no longer possible under the new design, the test's intent changes. + +5. **Update published docs to match the actual scope.** Regardless of the implementation approach, the published docs (`docs/serialization.md:10`, `docs/appendix/serialization.md:53-55`) currently promise universal shared-instance identity. After implementation, verify the docs accurately describe which types participate in reference tracking and which do not. If any types are excluded (e.g., records with parameterized constructors), document the limitation. + +6. **Multi-targeting.** Verify the solution works on both net9.0 and net10.0. The STJ parameterized-constructor limitation exists in both. + +--- + +## Plans + +- [Shared Reference Handling Plan](../../plans/completed/shared-reference-handling-plan.md) + +--- + +## Tasks + +- [x] Step 2: Architect comprehension check +- [x] Step 3: Business requirements review — APPROVED (with critical tension to resolve) +- [x] Step 4: Architect plan creation & design +- [x] Step 5: Developer review — Approved, implementation contract created +- [x] Step 7: Implementation — Phase 1 + Phase 2 complete, zero failures +- [x] Step 8: Verification — Architect VERIFIED, Requirements SATISFIED +- [x] Step 9: Documentation — requirements docs, appendix, serialization docs updated +- [x] Step 10: Completion + +--- + +## Progress Log + +### 2026-03-20 +- Todo created from Neatoo's RemoteFactory v0.22.0 serializer migration work +- The Neatoo migration [Ignore]d `SharedDictionaryReference` test and flagged this for RemoteFactory architectural decision +- The Neatoo migration is otherwise complete (2111 passed, 1 [Ignore]d) +- Decision pending on whether RemoteFactory or Neatoo owns this feature +- Promoted to full project-todos workflow for architectural analysis +- Architect comprehension check: Ready -- 5 questions answered +- Requirements review: APPROVED -- 13 relevant requirements, 5 gaps, no hard contradictions, critical tension with v0.22.0 converter-level principle +- Architect plan created: two-phase approach (reproduction then fix), 13 business rules, custom ReferenceHandler subclass design +- Developer review: Approved -- all 13 assertions traced, no logic errors, implementation contract created +- Phase 1 implementation: exploration tests created, NeatooPreserveReferenceHandler built +- Phase 2 blocked: STJ parameterized-constructor limitation is permanent and comprehensive -- throws for ANY $id/$ref on reference-type constructor params. Plan sent back for architect revision. + +### 2026-03-21 +- Architect revision complete for Phase 2 after extensive analysis and user discussion +- Decided approach: **Simple Bypass Converter with DDD Justification** + - `NeatooPreserveReferenceHandler` (already built) wired into serializer options for mutable types + - New `RecordBypassConverterFactory` (~50-80 lines) claims ALL types with parameterized constructors, delegates to inner options without ReferenceHandler + - Records serialize without $id/$ref -- correct DDD behavior (value objects have no identity to track) +- Detection rule: all types with parameterized constructors (simplest, safest per user decision) +- DDD justification resolves the nested-reference-type concern: a Dictionary inside a record is part of the value object's state; duplication is semantically correct +- New documentation deliverable: `docs/appendix/record-reference-handling.md` -- appendix explaining the DDD rationale (user request: "document this in its own appendix style/level") +- `InterfaceFactory_NonNeatooType_NoRefMetadata` test passes unchanged -- bypass converter claims records before ReferenceHandler can add metadata +- Plan updated to "Under Review (Developer)" -- ready for developer review (Step 5) +- Developer review of revised Phase 2: Approved +- Phase 2 implementation complete: RecordBypassConverterFactory + NeatooPreserveReferenceHandler wired in. Zero failures. +- Verification: Architect VERIFIED (13/13 scenarios), Requirements SATISFIED (all 8 requirements traced) +- Documentation: CLAUDE-DESIGN.md, serialization.md, appendix/serialization.md updated. New appendix `record-reference-handling.md` created. +- Stale comment in SerializationTests.cs fixed (moved circular references to PARTIAL section) +- **Complete** + +--- + +## Completion Verification + +Before marking this todo as Complete, verify: + +- [x] All builds pass +- [x] All tests pass + +**Verification results:** +- Build: 0 errors, all projects build (net9.0 + net10.0) +- Tests: 490 unit + 498 integration + 42 design per framework, 0 failures + +--- + +## Results / Conclusions + +Shared reference handling for non-custom types implemented with a two-component approach justified by DDD semantics: + +1. **`NeatooPreserveReferenceHandler`** — bridges STJ's built-in converters to `NeatooReferenceResolver.Current`. Mutable reference types (Dictionary, List, plain classes with default constructors) now participate in `$id`/`$ref` reference tracking. + +2. **`RecordBypassConverterFactory`** — claims types with parameterized constructors (records) and delegates to inner options without `ReferenceHandler`. Records serialize without reference metadata. + +**DDD rationale:** Records are value objects — defined by their values, not their identity. Duplicating a record's internal state (including nested collections) on round-trip is semantically correct. Reference identity only matters for entities. This is not a limitation or compromise; it's correct domain model behavior. + +**STJ limitation:** `dotnet/runtime#73302` (closed NOT_PLANNED) — STJ cannot handle `$id`/`$ref` metadata on types with parameterized constructors. The bypass converter is the designed extensibility mechanism, not a workaround. + +New appendix documentation at `docs/appendix/record-reference-handling.md` explains the full rationale to prevent future sessions from re-flagging this as a bug. diff --git a/src/Design/CLAUDE-DESIGN.md b/src/Design/CLAUDE-DESIGN.md index abac0859..36a23b8f 100644 --- a/src/Design/CLAUDE-DESIGN.md +++ b/src/Design/CLAUDE-DESIGN.md @@ -150,7 +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 | +| Can Interface Factory return a record? | Yes, plain records/DTOs without Neatoo types | `AllPatterns.cs` | Records bypass reference handling (`RecordBypassConverterFactory`); do not mix Neatoo types into record properties | --- @@ -416,7 +416,7 @@ public interface IOrderService } ``` -**Why it matters:** Reference handling (`$id`/`$ref` metadata) is a converter-level concern, not a serializer-level concern. RemoteFactory's `JsonSerializerOptions` has no `ReferenceHandler` set -- STJ serializes plain records/DTOs natively without injecting `$id`/`$ref`. Neatoo's custom converters access a per-operation `NeatooReferenceResolver` via a static `AsyncLocal` accessor (`NeatooReferenceResolver.Current`) to add reference metadata for their own types. Mixing Neatoo types into a plain record creates a serialization mismatch: the record is serialized natively by STJ (no reference metadata), but the embedded Neatoo type's converter expects the resolver to be tracking references across the graph. Additionally, STJ's native record deserialization cannot process `$ref` in parameterized constructors. Use either pure Neatoo types (with `[Factory]`) or pure records/DTOs -- not a mix. +**Why it matters:** RemoteFactory uses a two-path serialization strategy for reference handling. Mutable reference types (Dictionary, List, plain classes with default constructors) participate in `$id`/`$ref` reference tracking via `NeatooPreserveReferenceHandler` on `JsonSerializerOptions`. Types with parameterized constructors (records, immutable types) are claimed by `RecordBypassConverterFactory`, which serializes them without any reference metadata -- this is correct DDD behavior because records are value objects whose identity is defined by their values, not by reference. STJ cannot deserialize `$id`/`$ref` metadata on types with parameterized constructors (`ObjectWithParameterizedCtorRefMetadataNotSupported`), so bypassing is also a technical necessity. Mixing Neatoo types into a plain record creates a serialization mismatch: the record bypasses reference handling entirely (including its subtree), but the embedded Neatoo type's converter expects the resolver to be tracking references across the graph. Use either pure Neatoo types (with `[Factory]`) or pure records/DTOs -- not a mix. --- @@ -750,7 +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. +10. **Mixing Neatoo types with records in Interface Factory return types** - Records bypass reference handling entirely (`RecordBypassConverterFactory`), so embedded Neatoo types lose reference tracking. Use pure records/DTOs or pure Neatoo types, not both. --- diff --git a/src/Design/Design.Tests/FactoryTests/SerializationTests.cs b/src/Design/Design.Tests/FactoryTests/SerializationTests.cs index a41865a0..7cffafaa 100644 --- a/src/Design/Design.Tests/FactoryTests/SerializationTests.cs +++ b/src/Design/Design.Tests/FactoryTests/SerializationTests.cs @@ -35,11 +35,14 @@ // - Delegate/Func/Action fields // - Lazy or other deferred types // - Interface-typed collections (IEnumerable, IList) -// - Circular references without proper handling // // PARTIAL - Special handling required: // - Collections with injected factories (work in local mode only) // - Child entity collections (see OrderLineList pattern) +// - Circular references: Handled for mutable types (classes with default +// constructors) via NeatooPreserveReferenceHandler. NOT handled for types +// with parameterized constructors (records) -- those bypass reference +// tracking entirely because value objects have no identity to track. // // ----------------------------------------------------------------------------- // WHY IORDINALSERIALIZABLE MATTERS diff --git a/src/Directory.Build.props b/src/Directory.Build.props index fb16b6c9..e144bb05 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -16,7 +16,7 @@ net9.0;net10.0 True 0.7.0 - 0.22.0 + 0.23.0 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 5194d5a0..78909f83 100644 --- a/src/RemoteFactory/Internal/NeatooJsonSerializer.cs +++ b/src/RemoteFactory/Internal/NeatooJsonSerializer.cs @@ -68,6 +68,7 @@ public NeatooJsonSerializer( this.Options = new JsonSerializerOptions { TypeInfoResolver = neatooDefaultJsonTypeInfoResolver, + ReferenceHandler = new NeatooPreserveReferenceHandler(), WriteIndented = serializationOptions.Format == SerializationFormat.Named, // Only indent for named format (debugging) IncludeFields = true }; @@ -78,10 +79,18 @@ public NeatooJsonSerializer( this.Options.Converters.Add(new NeatooOrdinalConverterFactory(serializationOptions)); } + // Neatoo converters get first priority -- they claim interfaces, abstract types, + // and IOrdinalSerializable types that have purpose-built converters. foreach (var neatooJsonConverterFactory in converterFactoryList) { this.Options.Converters.Add(neatooJsonConverterFactory); } + + // RecordBypassConverterFactory goes AFTER Neatoo converters. It claims types + // with parameterized constructors (records) and delegates to inner options + // without ReferenceHandler, preventing STJ's NotSupportedException for + // reference metadata on parameterized-constructor types. + this.Options.Converters.Add(new RecordBypassConverterFactory()); } diff --git a/src/RemoteFactory/Internal/NeatooPreserveReferenceHandler.cs b/src/RemoteFactory/Internal/NeatooPreserveReferenceHandler.cs new file mode 100644 index 00000000..b0715aec --- /dev/null +++ b/src/RemoteFactory/Internal/NeatooPreserveReferenceHandler.cs @@ -0,0 +1,41 @@ +using System; +using System.Text.Json.Serialization; + +namespace Neatoo.RemoteFactory.Internal; + +/// +/// Bridges STJ's API to the existing +/// AsyncLocal. This enables +/// STJ's built-in converters (for Dictionary, List, plain classes, etc.) +/// to participate in the same reference tracking used by Neatoo's custom +/// converters, preserving shared-instance identity across the full +/// serialization graph. +/// +/// +/// +/// STJ calls once at the start of each +/// top-level serialize/deserialize operation. By returning +/// , both STJ built-in +/// converters and Neatoo custom converters share the same resolver +/// instance and ID counter. +/// +/// +/// The resolver lifecycle is managed by : +/// it creates a new , sets +/// , calls +/// JsonSerializer.Serialize/Deserialize, then clears Current +/// in a finally block. This handler simply returns whatever Current +/// is at the time STJ calls . +/// +/// +internal sealed class NeatooPreserveReferenceHandler : ReferenceHandler +{ + public override ReferenceResolver CreateResolver() + { + return NeatooReferenceResolver.Current + ?? throw new InvalidOperationException( + "NeatooReferenceResolver.Current is null. " + + "A NeatooReferenceResolver must be created and set as Current before serialization. " + + "This handler should only be used with NeatooJsonSerializer, which manages the resolver lifecycle."); + } +} diff --git a/src/RemoteFactory/Internal/RecordBypassConverterFactory.cs b/src/RemoteFactory/Internal/RecordBypassConverterFactory.cs new file mode 100644 index 00000000..d579da9e --- /dev/null +++ b/src/RemoteFactory/Internal/RecordBypassConverterFactory.cs @@ -0,0 +1,134 @@ +using System; +using System.Linq; +using System.Reflection; +using System.Text.Json; +using System.Text.Json.Serialization; + +namespace Neatoo.RemoteFactory.Internal; + +/// +/// Claims types with parameterized constructors (records, classes without a default +/// constructor) and serializes them without reference metadata ($id/$ref). +/// +/// +/// +/// STJ throws NotSupportedException when reference metadata appears on +/// types deserialized through a parameterized constructor. This converter prevents +/// that by delegating to inner with +/// set to null. +/// +/// +/// Records are DDD value objects whose identity is defined by their values, not +/// by reference. Excluding them from reference tracking is semantically correct: +/// duplicating a value object's internal state on round-trip is the right behavior. +/// +/// +/// Detection rule: the type has no public parameterless constructor AND has at +/// least one public constructor with parameters. This matches STJ's own heuristic +/// for parameterized-constructor deserialization. +/// +/// +internal sealed class RecordBypassConverterFactory : JsonConverterFactory +{ + private JsonSerializerOptions? _innerOptions; + private readonly object _lock = new(); + + public override bool CanConvert(Type typeToConvert) + { + var constructors = typeToConvert.GetConstructors(BindingFlags.Public | BindingFlags.Instance); + + bool hasParameterlessCtor = false; + bool hasParameterizedCtor = false; + + for (int i = 0; i < constructors.Length; i++) + { + if (constructors[i].GetParameters().Length == 0) + { + hasParameterlessCtor = true; + } + else + { + hasParameterizedCtor = true; + } + } + + // Claim the type only when STJ would use a parameterized constructor: + // no public parameterless constructor, but at least one public constructor with parameters. + return !hasParameterlessCtor && hasParameterizedCtor; + } + + public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options) + { + var innerOpts = GetOrCreateInnerOptions(options); + + var converterType = typeof(RecordBypassConverter<>).MakeGenericType(typeToConvert); + return (JsonConverter)Activator.CreateInstance(converterType, innerOpts)!; + } + + private JsonSerializerOptions GetOrCreateInnerOptions(JsonSerializerOptions outerOptions) + { + if (_innerOptions is not null) + { + return _innerOptions; + } + + lock (_lock) + { + if (_innerOptions is not null) + { + return _innerOptions; + } + + var inner = new JsonSerializerOptions(outerOptions); + inner.ReferenceHandler = null; + + // Remove this factory from the inner options to prevent infinite recursion. + for (int i = inner.Converters.Count - 1; i >= 0; i--) + { + if (inner.Converters[i] is RecordBypassConverterFactory) + { + inner.Converters.RemoveAt(i); + } + } + + _innerOptions = inner; + return inner; + } + } +} + +/// +/// Converter that delegates serialization to inner options without +/// , preventing $id/$ref metadata +/// on types with parameterized constructors. +/// +internal sealed class RecordBypassConverter : JsonConverter +{ + private readonly JsonSerializerOptions _innerOptions; + + public RecordBypassConverter(JsonSerializerOptions innerOptions) + { + _innerOptions = innerOptions; + } + + public override T? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) + { + if (reader.TokenType == JsonTokenType.Null) + { + return default; + } + + return JsonSerializer.Deserialize(ref reader, _innerOptions); + } + + public override void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions options) + { + if (value is null) + { + writer.WriteNullValue(); + return; + } + + JsonSerializer.Serialize(writer, value, _innerOptions); + } +} diff --git a/src/Tests/RemoteFactory.IntegrationTests/TestTargets/TypeSerialization/SharedReferenceTargets.cs b/src/Tests/RemoteFactory.IntegrationTests/TestTargets/TypeSerialization/SharedReferenceTargets.cs new file mode 100644 index 00000000..be490f3d --- /dev/null +++ b/src/Tests/RemoteFactory.IntegrationTests/TestTargets/TypeSerialization/SharedReferenceTargets.cs @@ -0,0 +1,66 @@ +namespace RemoteFactory.IntegrationTests.TestTargets.TypeSerialization; + +// ============================================================================ +// Plain types (no [Factory] attribute) for testing shared reference behavior +// in NeatooJsonSerializer. These types are serialized directly, not through +// factory round-trips, to isolate serialization behavior from factory plumbing. +// ============================================================================ + +/// +/// Plain class with two Dictionary properties for testing shared-reference identity. +/// When the same Dictionary instance is assigned to both properties, serialization +/// should ideally preserve that identity (ReferenceEquals is true after round-trip). +/// +public class SharedDictionaryHolder +{ + public Dictionary DictionaryA { get; set; } = new(); + public Dictionary DictionaryB { get; set; } = new(); +} + +/// +/// Plain class with Name and Next properties for testing circular reference handling. +/// A circular graph (a.Next = b, b.Next = a) requires reference tracking to serialize +/// without infinite recursion. +/// +public class CircularNode +{ + public string Name { get; set; } = ""; + public CircularNode? Next { get; set; } +} + +/// +/// Wrapper that holds two references to the same record instance. +/// Used to test whether STJ emits $ref for records with parameterized constructors +/// when ReferenceHandler.Preserve is active. The $ref triggers +/// ObjectWithParameterizedCtorRefMetadataNotSupported on deserialization. +/// +public class SharedRecordHolder +{ + public InterfaceRecordWithCollection? RecordA { get; set; } + public InterfaceRecordWithCollection? RecordB { get; set; } +} + +/// +/// Plain class with both scalar properties and two shared Dictionary references. +/// Used for Scenario 10 to verify reference tracking works correctly in a +/// graph that mixes Dictionary and scalar properties. +/// +public class CrossTypeSharedReferenceHolder +{ + public string Name { get; set; } = ""; + public int Count { get; set; } + public Dictionary? DictionaryA { get; set; } + public Dictionary? DictionaryB { get; set; } +} + +/// +/// Plain class containing both a record (immutable, parameterized constructor) +/// and two shared Dictionary references (mutable). Used for Scenario 11 to +/// verify that records and shared mutable references coexist in the same graph. +/// +public class RecordWithSharedDictionaryHolder +{ + public InterfaceRecordItem? Record { get; set; } + public Dictionary? DictionaryA { get; set; } + public Dictionary? DictionaryB { get; set; } +} diff --git a/src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/SharedReferenceExplorationTests.cs b/src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/SharedReferenceExplorationTests.cs new file mode 100644 index 00000000..9cdbfdf2 --- /dev/null +++ b/src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/SharedReferenceExplorationTests.cs @@ -0,0 +1,359 @@ +using System.Text.Json; +using System.Text.Json.Serialization; +using Microsoft.Extensions.DependencyInjection; +using Neatoo.RemoteFactory; +using Neatoo.RemoteFactory.Internal; +using RemoteFactory.IntegrationTests.TestContainers; +using RemoteFactory.IntegrationTests.TestTargets.TypeSerialization; + +namespace RemoteFactory.IntegrationTests.TypeSerialization; + +/// +/// Phase 1 exploration tests for shared reference handling. +/// These tests reproduce the three problem scenarios (shared identity, records, +/// circular references) using ONLY RemoteFactory's serialization infrastructure +/// to determine whether the issues are inherent STJ limitations or caused by +/// the current NeatooJsonSerializer configuration. +/// +/// Each test documents what it proves and whether the result was expected. +/// These tests become the baseline for Phase 2 acceptance testing. +/// +public class SharedReferenceExplorationTests +{ + // ============================================================================ + // Scenario 1: Shared Dictionary -- identity preserved + // + // Rule 7: WHEN a mutable reference type (Dictionary) is assigned to two + // properties and serialized through NeatooJsonSerializer, THEN after + // deserialization the two properties reference the same object instance. + // + // Originally documented the broken behavior (identity lost). After wiring + // NeatooPreserveReferenceHandler + RecordBypassConverterFactory, shared + // identity is now preserved for mutable types. + // ============================================================================ + + [Fact] + public void Scenario1_SharedDictionary_IdentityPreserved() + { + // Arrange -- same Dictionary instance assigned to both properties + var scopes = ClientServerContainers.Scopes(); + var serializer = scopes.server.ServiceProvider.GetRequiredService(); + + var sharedDict = new Dictionary + { + ["key1"] = "value1", + ["key2"] = "value2" + }; + + var holder = new SharedDictionaryHolder + { + DictionaryA = sharedDict, + DictionaryB = sharedDict + }; + + // Verify the setup: same instance before serialization + Assert.True(ReferenceEquals(holder.DictionaryA, holder.DictionaryB), + "Setup: DictionaryA and DictionaryB should be the same instance before serialization"); + + // Act -- serialize and deserialize through NeatooJsonSerializer (with ReferenceHandler) + var json = serializer.Serialize(holder); + var result = serializer.Deserialize(json!); + + // Assert -- shared identity IS preserved via NeatooPreserveReferenceHandler + Assert.NotNull(result); + Assert.True(ReferenceEquals(result.DictionaryA, result.DictionaryB), + "Shared dictionary identity should be preserved after round-trip " + + "because NeatooJsonSerializer now has NeatooPreserveReferenceHandler on options"); + + // Both dictionaries should still have the correct values + Assert.Equal(2, result.DictionaryA!.Count); + Assert.Equal("value1", result.DictionaryA["key1"]); + Assert.Equal("value2", result.DictionaryA["key2"]); + } + + // ============================================================================ + // Scenario 2: Record round-trip -- current behavior (no ReferenceHandler) + // + // Rule 2: WHEN NeatooJsonSerializer serializes a record with a parameterized + // constructor (no ReferenceHandler), THEN deserialization succeeds. + // + // This proves records work correctly in the current configuration. + // ============================================================================ + + [Fact] + public void Scenario2_RecordRoundTrip_CurrentBehavior_Succeeds() + { + // Arrange + var scopes = ClientServerContainers.Scopes(); + var serializer = scopes.server.ServiceProvider.GetRequiredService(); + + var items = new List + { + new(1, "First"), + new(2, "Second"), + new(3, "Third") + }; + var record = new InterfaceRecordWithCollection("Test", items); + + // Act -- serialize and deserialize through NeatooJsonSerializer + var json = serializer.Serialize(record); + var result = serializer.Deserialize(json!); + + // Assert -- record deserialized successfully with all properties intact + Assert.NotNull(result); + Assert.Equal("Test", 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: Record with ReferenceHandler.Preserve -- STJ limitation + // + // Rule 3: WHEN ReferenceHandler.Preserve is set AND the same record instance + // is assigned to two properties (forcing $ref emission), THEN STJ throws + // NotSupportedException on deserialization. + // + // Per developer review concern C1: the record must be referenced TWICE to + // trigger $ref emission. A single-occurrence record only gets $id, which + // does not cause the exception. + // + // Uses bare JsonSerializerOptions (NOT NeatooJsonSerializer) to test STJ + // behavior in isolation. + // ============================================================================ + + [Fact] + public void Scenario3_RecordWithReferenceHandlerPreserve_ThrowsOnDeserialization() + { + // Arrange -- bare STJ options with ReferenceHandler.Preserve + var options = new JsonSerializerOptions + { + ReferenceHandler = ReferenceHandler.Preserve, + WriteIndented = true + }; + + var items = new List + { + new(1, "First"), + new(2, "Second") + }; + var sharedRecord = new InterfaceRecordWithCollection("Shared", items); + + // The same record instance assigned to BOTH properties forces STJ to emit + // $id on the first occurrence and $ref on the second occurrence. + var holder = new SharedRecordHolder + { + RecordA = sharedRecord, + RecordB = sharedRecord + }; + + // Act -- serialize succeeds (STJ writes $id and $ref) + var json = JsonSerializer.Serialize(holder, options); + + // Verify $ref is in the JSON (confirms the record IS being shared) + Assert.Contains("$ref", json); + + // Assert -- deserialization throws because STJ cannot construct the + // immutable record from a $ref pointer. + // The internal STJ error is ObjectWithParameterizedCtorRefMetadataNotSupported, + // but the exception message is the user-facing string. + var ex = Assert.Throws(() => + JsonSerializer.Deserialize(json, options)); + + Assert.Contains("Reference metadata is not supported when deseriali", ex.Message); + } + + // ============================================================================ + // Scenario 4: Circular reference -- handled + // + // Rule 9: WHEN a mutable reference type has a circular reference and is + // serialized through NeatooJsonSerializer, THEN the circular reference is + // preserved after deserialization. + // + // Originally documented the broken behavior (JsonException thrown). After + // wiring NeatooPreserveReferenceHandler, circular references are now + // handled via $id/$ref tracking. + // ============================================================================ + + [Fact] + public void Scenario4_CircularReference_Handled() + { + // Arrange -- create a circular graph: a.Next = b, b.Next = a + var scopes = ClientServerContainers.Scopes(); + var serializer = scopes.server.ServiceProvider.GetRequiredService(); + + var nodeA = new CircularNode { Name = "A" }; + var nodeB = new CircularNode { Name = "B" }; + nodeA.Next = nodeB; + nodeB.Next = nodeA; + + // Act -- serialize and deserialize through NeatooJsonSerializer (with ReferenceHandler) + var json = serializer.Serialize(nodeA); + var result = serializer.Deserialize(json!); + + // Assert -- circular reference is preserved + Assert.NotNull(result); + Assert.Equal("A", result.Name); + Assert.NotNull(result.Next); + Assert.Equal("B", result.Next.Name); + Assert.NotNull(result.Next.Next); + Assert.True(ReferenceEquals(result, result.Next.Next), + "Circular reference should be preserved: a.Next.Next should be the same instance as a"); + } + + // ============================================================================ + // Scenario 5: Shared Dictionary with ReferenceHandler.Preserve + // + // Rule 5: WHEN a Dictionary is assigned to two properties AND + // ReferenceHandler.Preserve is set on bare JsonSerializerOptions, + // THEN ReferenceEquals returns true after deserialization. + // + // Uses bare JsonSerializerOptions (NOT NeatooJsonSerializer) to confirm + // that STJ's ReferenceHandler.Preserve correctly handles shared Dictionary + // identity. This is the "happy path" that Phase 2 aims to integrate. + // ============================================================================ + + [Fact] + public void Scenario5_SharedDictionary_ReferenceHandlerPreserve_IdentityPreserved() + { + // Arrange -- bare STJ options with ReferenceHandler.Preserve + var options = new JsonSerializerOptions + { + ReferenceHandler = ReferenceHandler.Preserve, + WriteIndented = true + }; + + var sharedDict = new Dictionary + { + ["key1"] = "value1", + ["key2"] = "value2" + }; + + var holder = new SharedDictionaryHolder + { + DictionaryA = sharedDict, + DictionaryB = sharedDict + }; + + // Verify the setup + Assert.True(ReferenceEquals(holder.DictionaryA, holder.DictionaryB)); + + // Act -- serialize and deserialize with ReferenceHandler.Preserve + var json = JsonSerializer.Serialize(holder, options); + var result = JsonSerializer.Deserialize(json, options); + + // Assert -- shared identity IS preserved + Assert.NotNull(result); + Assert.True(ReferenceEquals(result.DictionaryA, result.DictionaryB), + "ReferenceHandler.Preserve correctly preserves shared dictionary identity"); + + // Values are intact + Assert.Equal(2, result.DictionaryA.Count); + Assert.Equal("value1", result.DictionaryA["key1"]); + Assert.Equal("value2", result.DictionaryA["key2"]); + } + + // ============================================================================ + // Scenario 6: Custom ReferenceHandler delegating to NeatooReferenceResolver + // + // Rule 6: WHEN options.ReferenceHandler uses a custom ReferenceHandler + // subclass that delegates CreateResolver() to NeatooReferenceResolver.Current, + // THEN STJ's built-in converters emit $id/$ref and shared identity is preserved. + // + // This validates the Phase 2 approach: a custom ReferenceHandler that bridges + // STJ's API to the existing NeatooReferenceResolver infrastructure. + // + // The test manually manages the NeatooReferenceResolver lifecycle (normally + // done by NeatooJsonSerializer) to isolate the custom handler behavior. + // ============================================================================ + + [Fact] + public void Scenario6_CustomReferenceHandler_NeatooReferenceResolver_IdentityPreserved() + { + // Arrange -- custom ReferenceHandler that delegates to NeatooReferenceResolver.Current + var options = new JsonSerializerOptions + { + ReferenceHandler = new TestNeatooPreserveReferenceHandler(), + WriteIndented = true + }; + + var sharedDict = new Dictionary + { + ["key1"] = "value1", + ["key2"] = "value2" + }; + + var holder = new SharedDictionaryHolder + { + DictionaryA = sharedDict, + DictionaryB = sharedDict + }; + + Assert.True(ReferenceEquals(holder.DictionaryA, holder.DictionaryB)); + + // Act -- manually manage the resolver lifecycle (as NeatooJsonSerializer would) + // Serialize + string json; + using (var rr = new NeatooReferenceResolver()) + { + NeatooReferenceResolver.Current = rr; + try + { + json = JsonSerializer.Serialize(holder, options); + } + finally + { + NeatooReferenceResolver.Current = null; + } + } + + // Verify $ref is in the JSON (the second dictionary reference) + Assert.Contains("$ref", json); + + // Deserialize -- requires a fresh resolver + SharedDictionaryHolder? result; + using (var rr = new NeatooReferenceResolver()) + { + NeatooReferenceResolver.Current = rr; + try + { + result = JsonSerializer.Deserialize(json, options); + } + finally + { + NeatooReferenceResolver.Current = null; + } + } + + // Assert -- shared identity IS preserved through the custom handler + Assert.NotNull(result); + Assert.True(ReferenceEquals(result.DictionaryA, result.DictionaryB), + "Custom ReferenceHandler delegating to NeatooReferenceResolver preserves shared identity"); + + // Values are intact + Assert.Equal(2, result.DictionaryA.Count); + Assert.Equal("value1", result.DictionaryA["key1"]); + Assert.Equal("value2", result.DictionaryA["key2"]); + } + + // ============================================================================ + // Temporary custom ReferenceHandler for Phase 1 exploration (Scenario 6). + // In Phase 2, this becomes NeatooPreserveReferenceHandler in the production code. + // ============================================================================ + + private sealed class TestNeatooPreserveReferenceHandler : ReferenceHandler + { + public override ReferenceResolver CreateResolver() + { + return NeatooReferenceResolver.Current + ?? throw new InvalidOperationException( + "NeatooReferenceResolver.Current is null. " + + "A NeatooReferenceResolver must be created and set as Current before serialization."); + } + } +} diff --git a/src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/SharedReferenceTests.cs b/src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/SharedReferenceTests.cs new file mode 100644 index 00000000..4541b79b --- /dev/null +++ b/src/Tests/RemoteFactory.IntegrationTests/TypeSerialization/SharedReferenceTests.cs @@ -0,0 +1,262 @@ +using Microsoft.Extensions.DependencyInjection; +using Neatoo.RemoteFactory; +using Neatoo.RemoteFactory.Internal; +using RemoteFactory.IntegrationTests.TestContainers; +using RemoteFactory.IntegrationTests.TestTargets.TypeSerialization; + +namespace RemoteFactory.IntegrationTests.TypeSerialization; + +/// +/// Phase 2 acceptance tests for shared reference handling after the fix. +/// These tests validate that NeatooJsonSerializer (with NeatooPreserveReferenceHandler) +/// preserves shared-instance identity for non-custom types, handles circular +/// references, and does not break record deserialization. +/// +/// Each test corresponds to a scenario in the plan's Test Scenarios table. +/// +public class SharedReferenceTests +{ + // ============================================================================ + // Scenario 7: Shared Dictionary -- after fix + // + // Rule 7: WHEN a mutable reference type (Dictionary) is assigned to two + // properties and serialized/deserialized through NeatooJsonSerializer, + // THEN after deserialization the two properties reference the same object + // instance (ReferenceEquals returns true). + // + // This is the primary acceptance test: the fix restores shared identity. + // ============================================================================ + + [Fact] + public void Scenario7_SharedDictionary_AfterFix_IdentityPreserved() + { + // Arrange -- same Dictionary instance assigned to both properties + var scopes = ClientServerContainers.Scopes(); + var serializer = scopes.server.ServiceProvider.GetRequiredService(); + + var sharedDict = new Dictionary + { + ["key1"] = "value1", + ["key2"] = "value2" + }; + + var holder = new SharedDictionaryHolder + { + DictionaryA = sharedDict, + DictionaryB = sharedDict + }; + + // Verify the setup: same instance before serialization + Assert.True(ReferenceEquals(holder.DictionaryA, holder.DictionaryB), + "Setup: DictionaryA and DictionaryB should be the same instance before serialization"); + + // Act -- serialize and deserialize through NeatooJsonSerializer (now with ReferenceHandler) + var json = serializer.Serialize(holder); + var result = serializer.Deserialize(json!); + + // Assert -- shared identity IS preserved after the fix + Assert.NotNull(result); + Assert.True(ReferenceEquals(result.DictionaryA, result.DictionaryB), + "After fix: shared dictionary identity should be preserved after round-trip " + + "because NeatooJsonSerializer now has NeatooPreserveReferenceHandler on options"); + + // Values are intact + Assert.Equal(2, result.DictionaryA!.Count); + Assert.Equal("value1", result.DictionaryA["key1"]); + Assert.Equal("value2", result.DictionaryA["key2"]); + } + + // ============================================================================ + // Scenario 8: Record round-trip -- after fix + // + // Rule 8: WHEN a record type with a parameterized constructor is + // serialized/deserialized through NeatooJsonSerializer, THEN deserialization + // succeeds without error and all property values are preserved. + // + // Per Phase 1 Finding F3: STJ adds $id to records (does NOT skip them). + // A single-occurrence record with $id deserializes correctly. Only shared + // records (same instance in two properties, triggering $ref) would fail. + // This test verifies the common case: a single-occurrence record. + // ============================================================================ + + [Fact] + public void Scenario8_RecordRoundTrip_AfterFix_Succeeds() + { + // Arrange + var scopes = ClientServerContainers.Scopes(); + var serializer = scopes.server.ServiceProvider.GetRequiredService(); + + var items = new List + { + new(1, "First"), + new(2, "Second"), + new(3, "Third") + }; + var record = new InterfaceRecordWithCollection("Test", items); + + // Act -- serialize and deserialize through NeatooJsonSerializer (now with ReferenceHandler) + var json = serializer.Serialize(record); + var result = serializer.Deserialize(json!); + + // Assert -- record deserialized successfully with all properties intact + // even though STJ now adds $id metadata to the record + Assert.NotNull(result); + Assert.Equal("Test", 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 9: Circular reference -- after fix + // + // Rule 9: WHEN a mutable reference type has a circular reference + // (a.Next = b, b.Next = a) AND is serialized/deserialized through + // NeatooJsonSerializer, THEN the circular reference is preserved after + // deserialization (a.Next.Next is the same instance as a). + // + // Before the fix, this threw JsonException (max depth exceeded). + // Now with ReferenceHandler, STJ detects cycles via the resolver. + // ============================================================================ + + [Fact] + public void Scenario9_CircularReference_AfterFix_IdentityPreserved() + { + // Arrange -- create a circular graph: a.Next = b, b.Next = a + var scopes = ClientServerContainers.Scopes(); + var serializer = scopes.server.ServiceProvider.GetRequiredService(); + + var nodeA = new CircularNode { Name = "A" }; + var nodeB = new CircularNode { Name = "B" }; + nodeA.Next = nodeB; + nodeB.Next = nodeA; + + // Act -- serialize and deserialize through NeatooJsonSerializer + var json = serializer.Serialize(nodeA); + var result = serializer.Deserialize(json!); + + // Assert -- circular reference is preserved + Assert.NotNull(result); + Assert.Equal("A", result.Name); + Assert.NotNull(result.Next); + Assert.Equal("B", result.Next.Name); + Assert.NotNull(result.Next.Next); + Assert.True(ReferenceEquals(result, result.Next.Next), + "Circular reference should be preserved: a.Next.Next should be the same instance as a"); + } + + // ============================================================================ + // Scenario 10: Cross-type shared reference + // + // Rule 10: WHEN a Neatoo type with a custom converter is serialized alongside + // non-custom types in the same object graph, THEN both use the SAME + // NeatooReferenceResolver instance. + // + // Neatoo custom converters (NeatooBaseJsonTypeConverter, NeatooListBaseJsonTypeConverter) + // are in the Neatoo repository, not RemoteFactory. We cannot test a true + // cross-converter-type scenario here. Instead, we verify that the serializer + // preserves shared Dictionary identity in a graph that also contains other + // property types (strings, ints), confirming the resolver handles mixed-type + // graphs correctly. The full cross-converter test requires the downstream + // Neatoo repository. + // + // This test uses a holder that has both a shared Dictionary AND additional + // scalar properties to verify the reference tracking does not interfere + // with normal property serialization. + // ============================================================================ + + [Fact] + public void Scenario10_CrossTypeSharedReference_DictionaryWithMixedProperties() + { + // Arrange -- a graph with both a shared Dictionary and scalar properties + var scopes = ClientServerContainers.Scopes(); + var serializer = scopes.server.ServiceProvider.GetRequiredService(); + + var sharedDict = new Dictionary + { + ["key1"] = "value1", + ["key2"] = "value2" + }; + + var holder = new CrossTypeSharedReferenceHolder + { + Name = "TestHolder", + Count = 42, + DictionaryA = sharedDict, + DictionaryB = sharedDict + }; + + Assert.True(ReferenceEquals(holder.DictionaryA, holder.DictionaryB)); + + // Act -- serialize and deserialize + var json = serializer.Serialize(holder); + var result = serializer.Deserialize(json!); + + // Assert -- shared Dictionary identity preserved AND scalar properties intact + Assert.NotNull(result); + Assert.Equal("TestHolder", result.Name); + Assert.Equal(42, result.Count); + Assert.True(ReferenceEquals(result.DictionaryA, result.DictionaryB), + "Shared dictionary identity should be preserved in a mixed-property graph"); + Assert.Equal(2, result.DictionaryA!.Count); + Assert.Equal("value1", result.DictionaryA["key1"]); + } + + // ============================================================================ + // Scenario 11: Record in graph with shared mutable refs + // + // Rule 11: WHEN a record type with a parameterized constructor appears in a + // graph with shared mutable references, THEN the record itself deserializes + // correctly AND the mutable reference identity is preserved. + // + // Per Phase 1 Finding F3: STJ adds $id to records but does NOT skip them. + // This test verifies that a single-occurrence record coexists safely with + // shared mutable references in the same serialization graph. + // ============================================================================ + + [Fact] + public void Scenario11_RecordInGraphWithSharedMutableRefs() + { + // Arrange -- a graph containing a record AND a shared Dictionary + var scopes = ClientServerContainers.Scopes(); + var serializer = scopes.server.ServiceProvider.GetRequiredService(); + + var sharedDict = new Dictionary + { + ["key1"] = "value1" + }; + + var record = new InterfaceRecordItem(1, "TestItem"); + + var holder = new RecordWithSharedDictionaryHolder + { + Record = record, + DictionaryA = sharedDict, + DictionaryB = sharedDict + }; + + Assert.True(ReferenceEquals(holder.DictionaryA, holder.DictionaryB)); + + // Act -- serialize and deserialize + var json = serializer.Serialize(holder); + var result = serializer.Deserialize(json!); + + // Assert -- record deserializes correctly + Assert.NotNull(result); + Assert.NotNull(result.Record); + Assert.Equal(1, result.Record.Id); + Assert.Equal("TestItem", result.Record.Description); + + // Assert -- shared Dictionary identity is preserved + Assert.True(ReferenceEquals(result.DictionaryA, result.DictionaryB), + "Shared dictionary identity should be preserved even when a record is in the same graph"); + Assert.NotNull(result.DictionaryA); + Assert.Single(result.DictionaryA); + Assert.Equal("value1", result.DictionaryA["key1"]); + } +}