NEO-61: address code review follow-ups.
Fix AAA layout in depletion tests, add ResourceNodeInstanceIds.Normalize, throw on failed lazy-init after catalog hit, and update dependency register footnote plus review doc strikethroughs.pull/96/head
parent
0dfbe769e7
commit
0cc5bb8925
|
|
@ -48,7 +48,7 @@ Fleshed-out scope, contracts, and integration notes live in **per-module documen
|
|||
| E3.M4 | SinkAndDurabilityLifecycle | E3.M3, E8.M3 | DurabilityState, ItemSinkEvent, RepairCostRule | Pre-production | Planned |
|
||||
| E3.M5 | EconomyBalancePolicy | E3.M4, E9.M2 | EconomyPolicy, PriceBandRule, FaucetSinkRatio | Pre-production | Planned |
|
||||
|
||||
**E3.M1 note:** Epic 3 **Slice 2** backlog in Linear ([Epic 3 — Crafting, Gathering, and Itemization Economy](https://linear.app/neon-sprawl/project/epic-3-crafting-gathering-and-itemization-economy-65785ed05bc2)): [NEO-57](https://linear.app/neon-sprawl/issue/NEO-57) → [NEO-64](https://linear.app/neon-sprawl/issue/NEO-64); label **`E3.M1`**. See [E3M1-prototype-backlog.md](../../plans/E3M1-prototype-backlog.md), [E3_M1_ResourceNodeAndGatherLoop.md](E3_M1_ResourceNodeAndGatherLoop.md). **NEO-57** (content + CI), **NEO-58** (server fail-fast load), **NEO-59** (`IResourceNodeDefinitionRegistry` + DI), **NEO-60** (`GET /game/world/resource-node-definitions`) — E3M1-01–**E3M1-04** complete; register row **In Progress** until depletion / gather engine slices land. **NEO-41** (prototype gather XP on `resource_node` interact) remains until **E3M1-07** migrates into the gather engine. Remaining Slice 2 work adds **`GatherResult`**, depletion store, inventory grants, and telemetry hooks **`resource_gathered`** / **`gather_node_depleted`**.
|
||||
**E3.M1 note:** Epic 3 **Slice 2** backlog in Linear ([Epic 3 — Crafting, Gathering, and Itemization Economy](https://linear.app/neon-sprawl/project/epic-3-crafting-gathering-and-itemization-economy-65785ed05bc2)): [NEO-57](https://linear.app/neon-sprawl/issue/NEO-57) → [NEO-64](https://linear.app/neon-sprawl/issue/NEO-64); label **`E3.M1`**. See [E3M1-prototype-backlog.md](../../plans/E3M1-prototype-backlog.md), [E3_M1_ResourceNodeAndGatherLoop.md](E3_M1_ResourceNodeAndGatherLoop.md). **NEO-57** (content + CI), **NEO-58** (server fail-fast load), **NEO-59** (`IResourceNodeDefinitionRegistry` + DI), **NEO-60** (`GET /game/world/resource-node-definitions`), **NEO-61** (`IResourceNodeInstanceStore` + depletion operations, `V006`) — E3M1-01–**E3M1-05** complete; register row **In Progress** until gather engine / interact wiring slices land. **NEO-41** (prototype gather XP on `resource_node` interact) remains until **E3M1-07** migrates into the gather engine. Remaining Slice 2 work adds **`GatherResult`**, inventory grants, and telemetry hooks **`resource_gathered`** / **`gather_node_depleted`**.
|
||||
|
||||
**E3.M3 note:** Epic 3 **Slice 1** backlog in Linear ([Epic 3 — Crafting, Gathering, and Itemization Economy](https://linear.app/neon-sprawl/project/epic-3-crafting-gathering-and-itemization-economy-65785ed05bc2)): [NEO-50](https://linear.app/neon-sprawl/issue/NEO-50) → [NEO-56](https://linear.app/neon-sprawl/issue/NEO-56); label **`E3.M3`**. See [E3M3-prototype-backlog.md](../../plans/E3M3-prototype-backlog.md), [E3_M3_ItemizationAndInventorySchema.md](E3_M3_ItemizationAndInventorySchema.md). **NEO-50** (content + CI), **NEO-51** (server fail-fast load), **NEO-52** (`IItemDefinitionRegistry` + DI), **NEO-53** (`GET /game/world/item-definitions`), **NEO-54** (inventory store + stack/slot rules engine), **NEO-55** (`GET`/`POST /game/players/{id}/inventory`), and **NEO-56** (comment-only `item_created` / `inventory_transfer_denied` telemetry hook sites in `PlayerInventoryOperations`) — Epic 3 Slice 1 backlog **E3M3-01**–**E3M3-07** complete; register row **In Progress** until Slice 2+; later slices update the alignment table as they land.
|
||||
|
||||
|
|
|
|||
|
|
@ -20,11 +20,11 @@ NEO-61 adds server-authoritative **remaining gathers** per world node instance:
|
|||
| [`docs/plans/E3M1-prototype-backlog.md`](../plans/E3M1-prototype-backlog.md) | **Matches** — E3M1-05 acceptance items flipped with NEO-61 landed note. |
|
||||
| [`docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md`](../decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md) | **Matches** — NEO-61 depletion-store bullet under **Related implementation slices**; per-instance capacity, world-wide keying, **`node_depleted`** align with **Purpose** / **Responsibilities**. |
|
||||
| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M1 row notes NEO-61 landed with README link; **Still planned** correctly lists gather engine / interact wiring. |
|
||||
| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — E3.M1 row **In Progress** is correct, but **E3.M1 note** footnote still lists NEO-57–NEO-60 only and says “until depletion … slices land” without NEO-61 (alignment table already updated). |
|
||||
| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Matches** — E3.M1 row **In Progress** is correct; **E3.M1 note** footnote updated for NEO-61 (E3M1-05). |
|
||||
| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — server-authoritative capacity; no client-trusted depletion state. |
|
||||
| [`server/README.md`](../../server/README.md) | **Matches** — Resource node instance depletion subsection: lazy init, reason-code table, persistence split, NEO-63 deferral. |
|
||||
|
||||
Register/tracking: alignment table update is done; **E3.M1 note** footnote should mention NEO-61 when touched (same pattern as NEO-60 register suggestion).
|
||||
Register/tracking: alignment table and **E3.M1 note** footnote updated for NEO-61.
|
||||
|
||||
## Blocking issues
|
||||
|
||||
|
|
@ -32,20 +32,20 @@ None.
|
|||
|
||||
## Suggestions
|
||||
|
||||
1. **AAA layout in stateful tests** — Two new test methods mix **Assert** into **Act**:
|
||||
- `ResourceNodeInstanceOperationsTests.TryCommitSuccessfulGatherDecrement_RepeatedUntilCapacity_ShouldReturnNodeDepletedWithoutGoingNegative` — assertions inside the decrement loop.
|
||||
- `ResourceNodeInstancePersistenceIntegrationTests.TryCommitSuccessfulGatherDecrement_WhenDepleted_ShouldPersistZeroRow` — assertions inside the final `using` Act block.
|
||||
Move loop invariants / final expectations to a dedicated **Assert** section (collect last applied outcome during Act, assert after) per [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert).
|
||||
1. ~~**AAA layout in stateful tests** — Two new test methods mix **Assert** into **Act**:~~ Done.
|
||||
- ~~`ResourceNodeInstanceOperationsTests.TryCommitSuccessfulGatherDecrement_RepeatedUntilCapacity_ShouldReturnNodeDepletedWithoutGoingNegative` — assertions inside the decrement loop.~~
|
||||
- ~~`ResourceNodeInstancePersistenceIntegrationTests.TryCommitSuccessfulGatherDecrement_WhenDepleted_ShouldPersistZeroRow` — assertions inside the final `using` Act block.~~
|
||||
~~Move loop invariants / final expectations to a dedicated **Assert** section (collect last applied outcome during Act, assert after) per [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert).~~
|
||||
|
||||
2. **Dependency register footnote (optional)** — Extend **E3.M1 note** in [`module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) to include **NEO-61** (E3M1-05 depletion store) so the register matches the alignment table — same follow-up as NEO-60 for NEO-57–NEO-60.
|
||||
2. ~~**Dependency register footnote (optional)** — Extend **E3.M1 note** in [`module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) to include **NEO-61** (E3M1-05 depletion store) so the register matches the alignment table — same follow-up as NEO-60 for NEO-57–NEO-60.~~ Done.
|
||||
|
||||
3. **Concurrency test gap (optional)** — Plan §8 listed in-memory get/ensure/decrement **concurrency hygiene** tests; implementation uses per-id locks but has no parallel test. NEO-54 inventory store also omits concurrent tests, so non-blocking; add a lightweight `Parallel.For` smoke test if you want plan parity.
|
||||
|
||||
## Nits
|
||||
|
||||
- Nit: **`NormalizeInteractableId`** lives as `internal static` on **`InMemoryResourceNodeInstanceStore`** but is called from **`PostgresResourceNodeInstanceStore`** and **`ResourceNodeInstanceOperations`** — works, but a small shared helper (e.g. `ResourceNodeInstanceIds.Normalize`) would reduce cross-type coupling if this grows.
|
||||
- ~~Nit: **`NormalizeInteractableId`** lives as `internal static` on **`InMemoryResourceNodeInstanceStore`** but is called from **`PostgresResourceNodeInstanceStore`** and **`ResourceNodeInstanceOperations`** — works, but a small shared helper (e.g. `ResourceNodeInstanceIds.Normalize`) would reduce cross-type coupling if this grows.~~ Done.
|
||||
|
||||
- Nit: **`TryEnsureInitialized` failure → `unknown_node`** in `ResourceNodeInstanceOperations` (lines 26–31) is unreachable after a registry hit with non-negative **`maxGathers`**; a distinct internal reason or `Debug.Assert` would clarify intent vs catalog miss.
|
||||
- ~~Nit: **`TryEnsureInitialized` failure → `unknown_node`** in `ResourceNodeInstanceOperations` (lines 26–31) is unreachable after a registry hit with non-negative **`maxGathers`**; a distinct internal reason or `Debug.Assert` would clarify intent vs catalog miss.~~ Done — throws **`InvalidOperationException`** (programmer error after validated catalog **`maxGathers`**).
|
||||
|
||||
- Nit: Postgres **`TryDecrementRemainingGathers`** opens a second connection via **`TryGetRemainingGathers`** on decrement miss to distinguish depleted-at-zero — correct behavior, slightly extra round-trip on deny path only.
|
||||
|
||||
|
|
|
|||
|
|
@ -37,24 +37,28 @@ public sealed class ResourceNodeInstanceOperationsTests
|
|||
var registry = factory.Services.GetRequiredService<IResourceNodeDefinitionRegistry>();
|
||||
var store = factory.Services.GetRequiredService<IResourceNodeInstanceStore>();
|
||||
|
||||
// Act — ten successful gathers (10 → 0)
|
||||
ResourceNodeInstanceMutationOutcome lastApplied = default;
|
||||
for (var i = 0; i < 10; i++)
|
||||
// Act — ten successful gathers (10 → 0), then one depletion attempt
|
||||
var appliedOutcomes = new ResourceNodeInstanceMutationOutcome[10];
|
||||
for (var i = 0; i < appliedOutcomes.Length; i++)
|
||||
{
|
||||
lastApplied = ResourceNodeInstanceOperations.TryCommitSuccessfulGatherDecrement(AlphaId, registry, store);
|
||||
Assert.Equal(ResourceNodeInstanceMutationKind.Applied, lastApplied.Kind);
|
||||
Assert.True(lastApplied.Snapshot!.Value.RemainingGathers >= 0);
|
||||
appliedOutcomes[i] = ResourceNodeInstanceOperations.TryCommitSuccessfulGatherDecrement(AlphaId, registry, store);
|
||||
}
|
||||
|
||||
var depleted = ResourceNodeInstanceOperations.TryCommitSuccessfulGatherDecrement(AlphaId, registry, store);
|
||||
store.TryGetRemainingGathers(AlphaId, out var persisted);
|
||||
|
||||
// Assert
|
||||
Assert.All(appliedOutcomes, static o =>
|
||||
{
|
||||
Assert.Equal(ResourceNodeInstanceMutationKind.Applied, o.Kind);
|
||||
Assert.True(o.Snapshot!.Value.RemainingGathers >= 0);
|
||||
});
|
||||
var lastApplied = appliedOutcomes[^1];
|
||||
Assert.Equal(0, lastApplied.Snapshot!.Value.RemainingGathers);
|
||||
Assert.Equal(ResourceNodeInstanceMutationKind.Denied, depleted.Kind);
|
||||
Assert.Equal(ResourceNodeInstanceReasonCodes.NodeDepleted, depleted.ReasonCode);
|
||||
Assert.NotNull(depleted.Snapshot);
|
||||
Assert.Equal(0, depleted.Snapshot!.Value.RemainingGathers);
|
||||
store.TryGetRemainingGathers(AlphaId, out var persisted);
|
||||
Assert.Equal(0, persisted.RemainingGathers);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -58,19 +58,20 @@ public sealed class ResourceNodeInstancePersistenceIntegrationTests(PostgresInte
|
|||
|
||||
// Act
|
||||
ResourceNodeInstanceMutationOutcome denyOutcome;
|
||||
ResourceNodeInstanceSnapshot persisted;
|
||||
await using var secondFactory = new PostgresWebApplicationFactory();
|
||||
using (var scope = secondFactory.Services.CreateScope())
|
||||
{
|
||||
var registry = scope.ServiceProvider.GetRequiredService<IResourceNodeDefinitionRegistry>();
|
||||
var store = scope.ServiceProvider.GetRequiredService<IResourceNodeInstanceStore>();
|
||||
denyOutcome = ResourceNodeInstanceOperations.TryCommitSuccessfulGatherDecrement(AlphaId, registry, store);
|
||||
store.TryGetRemainingGathers(AlphaId, out var snapshot);
|
||||
store.TryGetRemainingGathers(AlphaId, out persisted);
|
||||
}
|
||||
|
||||
// Assert
|
||||
Assert.Equal(ResourceNodeInstanceMutationKind.Denied, denyOutcome.Kind);
|
||||
Assert.Equal(ResourceNodeInstanceReasonCodes.NodeDepleted, denyOutcome.ReasonCode);
|
||||
Assert.Equal(0, snapshot.RemainingGathers);
|
||||
}
|
||||
Assert.Equal(0, persisted.RemainingGathers);
|
||||
}
|
||||
|
||||
private async Task ResetInstanceTableAsync()
|
||||
|
|
|
|||
|
|
@ -12,7 +12,7 @@ public sealed class InMemoryResourceNodeInstanceStore : IResourceNodeInstanceSto
|
|||
/// <inheritdoc />
|
||||
public bool TryGetRemainingGathers(string interactableId, out ResourceNodeInstanceSnapshot snapshot)
|
||||
{
|
||||
var key = NormalizeInteractableId(interactableId);
|
||||
var key = ResourceNodeInstanceIds.Normalize(interactableId);
|
||||
if (key.Length == 0)
|
||||
{
|
||||
snapshot = default;
|
||||
|
|
@ -35,7 +35,7 @@ public sealed class InMemoryResourceNodeInstanceStore : IResourceNodeInstanceSto
|
|||
/// <inheritdoc />
|
||||
public bool TryEnsureInitialized(string interactableId, int initialRemainingGathers)
|
||||
{
|
||||
var key = NormalizeInteractableId(interactableId);
|
||||
var key = ResourceNodeInstanceIds.Normalize(interactableId);
|
||||
if (key.Length == 0 || initialRemainingGathers < 0)
|
||||
{
|
||||
return false;
|
||||
|
|
@ -61,7 +61,7 @@ public sealed class InMemoryResourceNodeInstanceStore : IResourceNodeInstanceSto
|
|||
{
|
||||
previousRemaining = 0;
|
||||
remainingGathers = 0;
|
||||
var key = NormalizeInteractableId(interactableId);
|
||||
var key = ResourceNodeInstanceIds.Normalize(interactableId);
|
||||
if (key.Length == 0)
|
||||
{
|
||||
return false;
|
||||
|
|
@ -85,15 +85,4 @@ public sealed class InMemoryResourceNodeInstanceStore : IResourceNodeInstanceSto
|
|||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
internal static string NormalizeInteractableId(string? interactableId)
|
||||
{
|
||||
var trimmed = interactableId?.Trim();
|
||||
if (string.IsNullOrEmpty(trimmed))
|
||||
{
|
||||
return string.Empty;
|
||||
}
|
||||
|
||||
return trimmed.ToLowerInvariant();
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -6,7 +6,7 @@ public sealed class PostgresResourceNodeInstanceStore(Npgsql.NpgsqlDataSource da
|
|||
/// <inheritdoc />
|
||||
public bool TryGetRemainingGathers(string interactableId, out ResourceNodeInstanceSnapshot snapshot)
|
||||
{
|
||||
var key = InMemoryResourceNodeInstanceStore.NormalizeInteractableId(interactableId);
|
||||
var key = ResourceNodeInstanceIds.Normalize(interactableId);
|
||||
if (key.Length == 0)
|
||||
{
|
||||
snapshot = default;
|
||||
|
|
@ -34,7 +34,7 @@ public sealed class PostgresResourceNodeInstanceStore(Npgsql.NpgsqlDataSource da
|
|||
/// <inheritdoc />
|
||||
public bool TryEnsureInitialized(string interactableId, int initialRemainingGathers)
|
||||
{
|
||||
var key = InMemoryResourceNodeInstanceStore.NormalizeInteractableId(interactableId);
|
||||
var key = ResourceNodeInstanceIds.Normalize(interactableId);
|
||||
if (key.Length == 0 || initialRemainingGathers < 0)
|
||||
{
|
||||
return false;
|
||||
|
|
@ -63,7 +63,7 @@ public sealed class PostgresResourceNodeInstanceStore(Npgsql.NpgsqlDataSource da
|
|||
{
|
||||
previousRemaining = 0;
|
||||
remainingGathers = 0;
|
||||
var key = InMemoryResourceNodeInstanceStore.NormalizeInteractableId(interactableId);
|
||||
var key = ResourceNodeInstanceIds.Normalize(interactableId);
|
||||
if (key.Length == 0)
|
||||
{
|
||||
return false;
|
||||
|
|
|
|||
|
|
@ -0,0 +1,17 @@
|
|||
namespace NeonSprawl.Server.Game.Gathering;
|
||||
|
||||
/// <summary>Canonical interactable id normalization for resource-node instance stores (NEO-61).</summary>
|
||||
public static class ResourceNodeInstanceIds
|
||||
{
|
||||
/// <summary>Trim + lowercase; empty when input is null/whitespace.</summary>
|
||||
public static string Normalize(string? interactableId)
|
||||
{
|
||||
var trimmed = interactableId?.Trim();
|
||||
if (string.IsNullOrEmpty(trimmed))
|
||||
{
|
||||
return string.Empty;
|
||||
}
|
||||
|
||||
return trimmed.ToLowerInvariant();
|
||||
}
|
||||
}
|
||||
|
|
@ -12,7 +12,7 @@ public static class ResourceNodeInstanceOperations
|
|||
IResourceNodeDefinitionRegistry registry,
|
||||
IResourceNodeInstanceStore store)
|
||||
{
|
||||
var key = InMemoryResourceNodeInstanceStore.NormalizeInteractableId(interactableId);
|
||||
var key = ResourceNodeInstanceIds.Normalize(interactableId);
|
||||
if (key.Length == 0 || !registry.TryGetDefinition(key, out var definition))
|
||||
{
|
||||
return new ResourceNodeInstanceMutationOutcome(
|
||||
|
|
@ -25,10 +25,8 @@ public static class ResourceNodeInstanceOperations
|
|||
{
|
||||
if (!store.TryEnsureInitialized(key, definition.MaxGathers))
|
||||
{
|
||||
return new ResourceNodeInstanceMutationOutcome(
|
||||
ResourceNodeInstanceMutationKind.Denied,
|
||||
ResourceNodeInstanceReasonCodes.UnknownNode,
|
||||
null);
|
||||
throw new InvalidOperationException(
|
||||
$"Failed to initialize resource node instance '{key}' with maxGathers {definition.MaxGathers}.");
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue