NEO-125: address code review findings for bundle validation
Rename freeze-first loader tests, add PrototypeE7M2QuestCatalogRulesTests for cross-ref paths, align missing skill-def error with Python, and document bundle schema path resolution in server README.pull/164/head
parent
ad80ed8760
commit
766837d7e8
|
|
@ -53,7 +53,7 @@
|
|||
- **DTOs:** `QuestSkillXpGrantRow`, `QuestRewardBundleRow`; optional **`CompletionRewardBundle`** on **`QuestDefRow`**.
|
||||
- **Rules:** `PrototypeE7M2QuestCatalogRules` — presence, freeze-table, cross-ref + **`mission_reward`** allowlist gates synced to `validate_content.py`.
|
||||
- **Loader:** registers bundle-related JSON Schemas; parses **`completionRewardBundle`**; accepts **`SkillDefinitionCatalog.ById`** from DI.
|
||||
- **Tests:** 25 AAA tests in **`QuestDefinitionCatalogLoaderTests`** (5 new E7M2 cases + updated prototype fixture with bundles).
|
||||
- **Tests:** 25 AAA tests in **`QuestDefinitionCatalogLoaderTests`** (5 E7M2 loader cases + updated prototype fixture with bundles); **`PrototypeE7M2QuestCatalogRulesTests`** (4 direct cross-ref / allowlist cases).
|
||||
- **Docs:** `server/README.md` quest catalog E7M2 paragraph; E7M2-02 backlog AC checked.
|
||||
|
||||
## Technical approach
|
||||
|
|
@ -148,8 +148,9 @@ Extend **`EnsureQuestSchemasLoaded`** signature to accept bundle schema paths; v
|
|||
|
||||
| Target | Coverage |
|
||||
|--------|----------|
|
||||
| `server/NeonSprawl.Server.Tests/Game/Quests/QuestDefinitionCatalogLoaderTests.cs` | Happy path (repo + temp fixture with bundles); E7M2 presence, freeze, unknown item/skill, missing **`mission_reward`** allowlist; existing E7M1 tests unchanged; host DI resolve. |
|
||||
| **No new test files** | Extend existing loader test class per NEO-113 precedent. |
|
||||
| `server/NeonSprawl.Server.Tests/Game/Quests/PrototypeE7M2QuestCatalogRulesTests.cs` | Direct unit tests for **`TryGetCompletionBundleCrossRefError`** (item/skill catalog + allowlist paths). |
|
||||
| `server/NeonSprawl.Server.Tests/Game/Quests/QuestDefinitionCatalogLoaderTests.cs` | Happy path (repo + temp fixture with bundles); E7M2 presence, freeze, allowlist deny; host DI resolve. |
|
||||
| **No other new test files** | Loader + rules classes per NEO-113 precedent. |
|
||||
| **Manual verification** | `dotnet test server/NeonSprawl.Server.Tests --filter QuestDefinitionCatalogLoaderTests`; boot dev server and confirm quest catalog Information log after NEO-124 content. |
|
||||
|
||||
## Open questions / risks
|
||||
|
|
|
|||
|
|
@ -32,14 +32,14 @@ None.
|
|||
|
||||
## Suggestions
|
||||
|
||||
1. **Rename misleading bundle cross-ref tests** — `Load_ShouldThrow_WhenUnknownItemIdInCompletionRewardBundle` and `Load_ShouldThrow_WhenUnknownSkillIdInCompletionRewardBundle` mutate bundle ids so the **freeze** gate fails first (assertions correctly expect `"completionRewardBundle must match E7M2 freeze table"`). Consider renaming to `…WhenCompletionRewardBundleItemIdDoesNotMatchFreezeTable` / `…SkillIdDoesNotMatchFreezeTable`, or add a focused unit test on **`PrototypeE7M2QuestCatalogRules.TryGetCompletionBundleCrossRefError`** with a matching-freeze bundle to assert the cross-ref message paths (`is not in the frozen prototype item catalog` / skill catalog).
|
||||
1. ~~**Rename misleading bundle cross-ref tests** — `Load_ShouldThrow_WhenUnknownItemIdInCompletionRewardBundle` and `Load_ShouldThrow_WhenUnknownSkillIdInCompletionRewardBundle` mutate bundle ids so the **freeze** gate fails first (assertions correctly expect `"completionRewardBundle must match E7M2 freeze table"`). Consider renaming to `…WhenCompletionRewardBundleItemIdDoesNotMatchFreezeTable` / `…SkillIdDoesNotMatchFreezeTable`, or add a focused unit test on **`PrototypeE7M2QuestCatalogRules.TryGetCompletionBundleCrossRefError`** with a matching-freeze bundle to assert the cross-ref message paths (`is not in the frozen prototype item catalog` / skill catalog).~~ **Done.** Renamed loader tests; added **`PrototypeE7M2QuestCatalogRulesTests`** with direct cross-ref coverage.
|
||||
|
||||
2. **Manual boot smoke (plan §Tests)** — Plan lists booting the dev server and confirming the quest catalog Information log; worth a quick run before merge since NEO-124 content now depends on bundle schema registration at host startup.
|
||||
2. ~~**Manual boot smoke (plan §Tests)** — Plan lists booting the dev server and confirming the quest catalog Information log; worth a quick run before merge since NEO-124 content now depends on bundle schema registration at host startup.~~ **Done.** Dev server boot logged `Loaded quest catalog from …/content/quests: 4 quest(s) across 1 JSON catalog file(s).` with no startup exception.
|
||||
|
||||
## Nits
|
||||
|
||||
- Nit: Bundle schema paths use sibling `content/schemas/` resolution only; quest def/step/objective schemas support **`Content:*`** overrides. Plan marked overrides as optional — fine for prototype, but document if custom content layouts need the same override pattern later.
|
||||
- Nit: In **`TryGetCompletionBundleCrossRefError`**, a skill id present in **`PrototypeSlice1SkillCatalogRules.ExpectedSkillIds`** but missing from **`skillDefsById`** returns the “not in the frozen prototype skill catalog” message; Python’s cross-ref gate would report a **`mission_reward`** allowlist failure instead. Unreachable in production DI (skills load before quests) but slightly divergent for isolated unit tests.
|
||||
- Nit: Bundle schema paths use sibling `content/schemas/` resolution only; quest def/step/objective schemas support **`Content:*`** overrides. Plan marked overrides as optional — fine for prototype; **`server/README.md`** now documents that bundle schemas have no separate override keys yet.
|
||||
- ~~Nit: In **`TryGetCompletionBundleCrossRefError`**, a skill id present in **`PrototypeSlice1SkillCatalogRules.ExpectedSkillIds`** but missing from **`skillDefsById`** returns the “not in the frozen prototype skill catalog” message; Python’s cross-ref gate would report a **`mission_reward`** allowlist failure instead. Unreachable in production DI (skills load before quests) but slightly divergent for isolated unit tests.~~ **Done.** Missing loaded skill defs now mirror Python — empty **`allowedXpSourceKinds`** → **`mission_reward`** allowlist error.
|
||||
|
||||
## Verification
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,95 @@
|
|||
using NeonSprawl.Server.Game.Encounters;
|
||||
using NeonSprawl.Server.Game.Quests;
|
||||
using NeonSprawl.Server.Game.Skills;
|
||||
using Xunit;
|
||||
|
||||
namespace NeonSprawl.Server.Tests.Game.Quests;
|
||||
|
||||
public class PrototypeE7M2QuestCatalogRulesTests
|
||||
{
|
||||
private static readonly IReadOnlyList<QuestStepDefRow> EmptySteps =
|
||||
[new QuestStepDefRow("step", "Step", [])];
|
||||
|
||||
private static QuestDefRow QuestWithBundle(string questId, QuestRewardBundleRow bundle) =>
|
||||
new(questId, "Test", [], EmptySteps, bundle);
|
||||
|
||||
private static readonly IReadOnlyDictionary<string, SkillDefRow> ValidSkillDefs =
|
||||
new Dictionary<string, SkillDefRow>(StringComparer.Ordinal)
|
||||
{
|
||||
["salvage"] = new("salvage", "gather", "Salvage", ["activity", "mission_reward"]),
|
||||
["refine"] = new("refine", "process", "Refine", ["activity", "mission_reward", "trainer"]),
|
||||
};
|
||||
|
||||
[Fact]
|
||||
public void TryGetCompletionBundleCrossRefError_ShouldReturnError_WhenItemIdNotInFrozenCatalog()
|
||||
{
|
||||
// Arrange
|
||||
var rows = new Dictionary<string, QuestDefRow>(StringComparer.Ordinal)
|
||||
{
|
||||
[PrototypeE7M1QuestCatalogRules.ChainQuestId] = QuestWithBundle(
|
||||
PrototypeE7M1QuestCatalogRules.ChainQuestId,
|
||||
new QuestRewardBundleRow(
|
||||
[new RewardGrantRow("unknown_item_id", 1)],
|
||||
[new QuestSkillXpGrantRow("salvage", 50)])),
|
||||
};
|
||||
// Act
|
||||
var error = PrototypeE7M2QuestCatalogRules.TryGetCompletionBundleCrossRefError(rows, ValidSkillDefs);
|
||||
// Assert
|
||||
Assert.NotNull(error);
|
||||
Assert.Contains("completionRewardBundle.itemGrants[0]: itemId 'unknown_item_id'", error, StringComparison.Ordinal);
|
||||
Assert.Contains("is not in the frozen prototype item catalog", error, StringComparison.Ordinal);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void TryGetCompletionBundleCrossRefError_ShouldReturnError_WhenSkillIdNotInFrozenCatalog()
|
||||
{
|
||||
// Arrange
|
||||
var rows = new Dictionary<string, QuestDefRow>(StringComparer.Ordinal)
|
||||
{
|
||||
[PrototypeE7M1QuestCatalogRules.GatherIntroQuestId] = QuestWithBundle(
|
||||
PrototypeE7M1QuestCatalogRules.GatherIntroQuestId,
|
||||
new QuestRewardBundleRow([], [new QuestSkillXpGrantRow("unknown_skill_id", 25)])),
|
||||
};
|
||||
// Act
|
||||
var error = PrototypeE7M2QuestCatalogRules.TryGetCompletionBundleCrossRefError(rows, ValidSkillDefs);
|
||||
// Assert
|
||||
Assert.NotNull(error);
|
||||
Assert.Contains("completionRewardBundle.skillXpGrants[0]: skillId 'unknown_skill_id'", error, StringComparison.Ordinal);
|
||||
Assert.Contains("is not in the frozen prototype skill catalog", error, StringComparison.Ordinal);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void TryGetCompletionBundleCrossRefError_ShouldReturnError_WhenSkillMissingFromLoadedDefs()
|
||||
{
|
||||
// Arrange
|
||||
var rows = new Dictionary<string, QuestDefRow>(StringComparer.Ordinal)
|
||||
{
|
||||
[PrototypeE7M1QuestCatalogRules.GatherIntroQuestId] = QuestWithBundle(
|
||||
PrototypeE7M1QuestCatalogRules.GatherIntroQuestId,
|
||||
new QuestRewardBundleRow([], [new QuestSkillXpGrantRow("salvage", 25)])),
|
||||
};
|
||||
// Act
|
||||
var error = PrototypeE7M2QuestCatalogRules.TryGetCompletionBundleCrossRefError(
|
||||
rows,
|
||||
new Dictionary<string, SkillDefRow>(StringComparer.Ordinal));
|
||||
// Assert
|
||||
Assert.NotNull(error);
|
||||
Assert.Contains("must allow sourceKind 'mission_reward' in allowedXpSourceKinds (got [])", error, StringComparison.Ordinal);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void TryGetCompletionBundleCrossRefError_ShouldReturnNull_WhenBundlesMatchFreezeAndAllowlists()
|
||||
{
|
||||
// Arrange
|
||||
var rows = new Dictionary<string, QuestDefRow>(StringComparer.Ordinal);
|
||||
foreach (var (qid, bundle) in PrototypeE7M2QuestCatalogRules.ExpectedCompletionBundles)
|
||||
{
|
||||
rows[qid] = QuestWithBundle(qid, bundle);
|
||||
}
|
||||
|
||||
// Act
|
||||
var error = PrototypeE7M2QuestCatalogRules.TryGetCompletionBundleCrossRefError(rows, ValidSkillDefs);
|
||||
// Assert
|
||||
Assert.Null(error);
|
||||
}
|
||||
}
|
||||
|
|
@ -645,7 +645,7 @@ public class QuestDefinitionCatalogLoaderTests
|
|||
}
|
||||
|
||||
[Fact]
|
||||
public void Load_ShouldThrow_WhenUnknownItemIdInCompletionRewardBundle()
|
||||
public void Load_ShouldThrow_WhenCompletionRewardBundleItemIdDoesNotMatchFreezeTable()
|
||||
{
|
||||
// Arrange
|
||||
var (_, questsDir, defSchemaPath, stepSchemaPath, objectiveSchemaPath) = CreateTempContentLayout();
|
||||
|
|
@ -667,7 +667,7 @@ public class QuestDefinitionCatalogLoaderTests
|
|||
}
|
||||
|
||||
[Fact]
|
||||
public void Load_ShouldThrow_WhenUnknownSkillIdInCompletionRewardBundle()
|
||||
public void Load_ShouldThrow_WhenCompletionRewardBundleSkillIdDoesNotMatchFreezeTable()
|
||||
{
|
||||
// Arrange
|
||||
var (_, questsDir, defSchemaPath, stepSchemaPath, objectiveSchemaPath) = CreateTempContentLayout();
|
||||
|
|
|
|||
|
|
@ -103,17 +103,13 @@ public static class PrototypeE7M2QuestCatalogRules
|
|||
$"'{grant.SkillId}' is not in the frozen prototype skill catalog";
|
||||
}
|
||||
|
||||
if (!skillDefsById.TryGetValue(grant.SkillId, out var skill))
|
||||
{
|
||||
return
|
||||
$"error: quest '{qid}' completionRewardBundle.skillXpGrants[{gi}]: skillId " +
|
||||
$"'{grant.SkillId}' is not in the frozen prototype skill catalog";
|
||||
}
|
||||
|
||||
var allowedKinds = skill.AllowedXpSourceKinds;
|
||||
skillDefsById.TryGetValue(grant.SkillId, out var skill);
|
||||
var allowedKinds = skill?.AllowedXpSourceKinds ?? [];
|
||||
if (!allowedKinds.Contains(MissionRewardSourceKind, StringComparer.Ordinal))
|
||||
{
|
||||
var kindsDisplay = $"[{string.Join(", ", allowedKinds.Select(k => $"'{k}'"))}]";
|
||||
var kindsDisplay = allowedKinds.Count == 0
|
||||
? "[]"
|
||||
: $"[{string.Join(", ", allowedKinds.Select(k => $"'{k}'"))}]";
|
||||
return
|
||||
$"error: quest '{qid}' completionRewardBundle.skillXpGrants[{gi}]: skillId " +
|
||||
$"'{grant.SkillId}' must allow sourceKind '{MissionRewardSourceKind}' in allowedXpSourceKinds " +
|
||||
|
|
|
|||
|
|
@ -144,6 +144,8 @@ On startup the host loads every **`*_quests.json`** under the quests directory *
|
|||
| **`Content:QuestStepDefSchemaPath`** | Optional override for **`quest-step-def.schema.json`**. When unset, **`{parent of quests directory}/schemas/quest-step-def.schema.json`**. |
|
||||
| **`Content:QuestObjectiveDefSchemaPath`** | Optional override for **`quest-objective-def.schema.json`**. When unset, **`{parent of quests directory}/schemas/quest-objective-def.schema.json`**. |
|
||||
|
||||
Bundle-related schemas (**`quest-reward-bundle.schema.json`**, **`quest-skill-xp-grant.schema.json`**, **`reward-grant-row.schema.json`**) resolve as siblings under **`{parent of quests directory}/schemas/`** only — there are no separate **`Content:*`** override keys yet; custom layouts must keep those three files next to the quest schemas (NEO-125).
|
||||
|
||||
**Docker / CI:** include **`content/quests`**, upstream catalogs (**`content/items`**, **`content/recipes`**, **`content/encounters`**, **`content/skills`**), and quest/bundle schemas in the mounted **`content/`** tree; set **`Content__QuestsDirectory`** when layout differs.
|
||||
|
||||
On success, **Information** logs include the resolved quests directory path, distinct quest count, and catalog file count. Game code should use **`IQuestDefinitionRegistry`** for lookups (`TryGetDefinition`, `TryNormalizeKnown`, `GetDefinitionsInIdOrder`; NEO-114). The catalog singleton remains for fail-fast startup only; do not inject **`QuestDefinitionCatalog`** in new game code. **`TryGetDefinition`** is case-sensitive on catalog keys; HTTP and game callers validating wire ids should use **`TryNormalizeKnown`** (trim + lowercase + fail-closed lookup), same as encounter/ability routes.
|
||||
|
|
|
|||
Loading…
Reference in New Issue