diff --git a/server/config/uoEventActions.js b/server/config/uoEventActions.js index 02dd32b..2715912 100644 --- a/server/config/uoEventActions.js +++ b/server/config/uoEventActions.js @@ -305,16 +305,54 @@ function webUserId(webId) { } /** Resolve a `facet/name` landmark to the point the shard counts around. */ +function landmarkValue(row) { + const group = row.group || '' + return group === '' ? `${row.facet}/${row.name}` : `${row.facet}/${group}/${row.name}` +} + +/** + * A place string resolved to a point on a facet. + * + * **Two forms, and the older one is not deprecated — it is stored.** The current + * form is `facet/group/name`, which names exactly one landmark. The older + * `facet/name` is what every event published before this fix carries, and those + * rows are the authored record: a published version is immutable, so a parse that + * stopped understanding them would break runs rather than correct them. So the + * three-part form is tried first and the two-part read is the fallback. + * + * The fallback keeps the old first-match behaviour deliberately. It is wrong in + * the same way it always was — that is what the new form exists to fix — but it + * is what those runs did last time, and silently relocating a live event's + * spawn point is worse than repeating a known imprecision. + * + * A three-part value whose group no longer exists REFUSES rather than falling + * back to the name alone, and that is the point rather than a gap: it asked for + * one particular landmark, so the honest answer when that landmark is gone is to + * say so — the operator renamed something and an event needs re-pointing. Only a + * value that never named a group gets the imprecise read. + */ async function landmarkPoint(value) { const raw = String(value == null ? '' : value) - const cut = raw.indexOf('/') - if (cut < 1) { + const parts = raw.split('/') + if (parts.length < 2 || parts[0] === '' || parts[parts.length - 1] === '') { return { ok: false, error: `"${raw}" is not a facet/name place` } } - const facet = raw.slice(0, cut) - const name = raw.slice(cut + 1) + const facet = parts[0] const rows = await shardAtlas.listLandmarks({ facet }) + + // `facet/group/name`. The name is the LAST segment and the group is everything + // between, so a group carrying a slash still resolves. + if (parts.length >= 3) { + const group = parts.slice(1, -1).join('/') + const name = parts[parts.length - 1] + const hit = rows.find((r) => r.name === name && (r.group || '') === group) + if (hit) return { ok: true, map: hit.facet, x: hit.x, y: hit.y } + // No fall-through error: a name containing a slash reads as three parts too, + // and the two-part read below is the one that resolves it. + } + + const name = parts.slice(1).join('/') const hit = rows.find((r) => r.facet === facet && r.name === name) if (!hit) { @@ -2125,7 +2163,14 @@ const OPTION_SOURCES = [ async resolve() { const rows = await shardAtlas.listLandmarks() return bounded(rows, 'uo.options.landmarks').map((r) => ({ - value: `${r.facet}/${r.name}`, + // **`facet/group/name`, because `facet/name` does not name one place.** + // A stock 57.4 tree has 558 landmarks under 320 distinct `facet/name` + // pairs: `Trammel/Entrance` is 23 different dungeons, and `landmarkPoint` + // resolves with `.find()`, so 22 of them were unreachable — an author who + // picked "Entrance — Destard" got Blighted Grove, with a successful run + // and no warning. The group was already the disambiguator; it was shown + // to the eye and left out of the value. All 558 are distinct with it. + value: landmarkValue(r), label: r.name, // The atlas's own grouping where it has one, the facet otherwise — so a // shard whose landmark file carries no groups still gets a usable diff --git a/server/test/spawnAtlas.source.test.js b/server/test/spawnAtlas.source.test.js index efc5ffb..89d00a8 100644 --- a/server/test/spawnAtlas.source.test.js +++ b/server/test/spawnAtlas.source.test.js @@ -58,7 +58,8 @@ function writeTree(root, { facets = ['Sosaria'], includeChampions = true } = {}) fs.writeFileSync( path.join(root, 'Spawns', `${facet}.xml`), ` - ${facet}A${facet}11001100 + ${facet}Auid-${facet}-A + ${facet}11001100 3True Lizardman:MX=3:SB=0:OBJ=Orc:MX=1:SB=0 ${facet}B${facet}90009000 @@ -112,6 +113,29 @@ function tempTree(options) { // ── buildAtlas against a custom-facet tree ───────────────────────────────── +test('buildAtlas: a point keeps the UniqueId a property lease targets', () => { + // The field is asserted on the AGGREGATOR's output, not the parser's, which is + // the whole point of this test. `parsePoints` produced it from Phase 12b + // onwards and `PARSER_VERSION`'s own note said a point kept it, while the + // mapping in `buildAtlas` rebuilt each point from an explicit field list that + // omitted it — so `shard_spawn_points.unique_id` was NULL on every row, and + // `listSpawners`, whose WHERE is `unique_id IS NOT NULL`, answered empty. That + // left `uo.options.spawners` an empty dropdown and every Phase 12b + // object-property lease unauthorable. Found by the Phase 16b released-artefact + // walk, against a real tree whose files carry ~6,400 of these. + // + // The fixture above had no at all until this test, which is exactly + // why a green suite said nothing about it. + const root = tempTree({ facets: ['Sosaria'] }) + const atlas = buildAtlas(root) + const named = atlas.points.find((p) => p.name === 'SosariaA') + assert.equal(named.uniqueId, 'uid-Sosaria-A') + // And a point whose file names none is absent rather than empty-string, so the + // DB layer's `unique_id IS NOT NULL AND <> ''` reads it the same way either way. + const unnamed = atlas.points.find((p) => p.name === 'SosariaB') + assert.ok(!unnamed.uniqueId) +}) + test('buildAtlas: works entirely on facets that do not exist in stock UO', () => { const root = tempTree({ facets: ['Sosaria', 'Underdark'] }) const atlas = buildAtlas(root) diff --git a/server/test/uoEventActions.test.js b/server/test/uoEventActions.test.js index dabf238..134c25d 100644 --- a/server/test/uoEventActions.test.js +++ b/server/test/uoEventActions.test.js @@ -563,6 +563,78 @@ test('an atlas larger than the dropdown bound is truncated and said so', async ( assert.ok(warned, 'a truncated source must leave a log line naming itself') }) +// ── A landmark option value names ONE landmark (Phase 16b) ──────────────── + +test('two landmarks sharing a name are two different options, and both resolve', async () => { + // A stock 57.4 tree has 558 landmarks under 320 distinct `facet/name` pairs: + // `Trammel/Entrance` is 23 different dungeons. The source emitted `facet/name` + // and `landmarkPoint` resolved with `.find()`, so 22 of the 23 were unreachable + // — an author who picked "Entrance — Destard" got Blighted Grove, with a + // successful run and no warning. The group was already the disambiguator and it + // was shown to the eye while being left out of the value. + // + // Asserted as an INEQUALITY between two resolved points rather than against a + // literal value string, so it survives someone changing the value's format + // again as long as the two options still address two places. + shardAtlas.listLandmarks = async () => [ + { facet: 'Felucca', name: 'Entrance', group: 'Blighted Grove', x: 586, y: 1643, z: 0 }, + { facet: 'Felucca', name: 'Entrance', group: 'Destard', x: 1176, y: 2637, z: 0 }, + ] + + const source = actions.OPTION_SOURCES.find((s) => s.id === 'uo.options.landmarks') + const options = await source.resolve({}) + assert.equal(options.length, 2) + assert.equal(new Set(options.map((o) => o.value)).size, 2, 'both options must be addressable') + + const points = [] + for (const option of options) { + const result = await byId('uo.creature.spawn').perform({ + runId: 41, + idempotencyKey: `L${option.value}`.padEnd(40, 'x'), + params: { place: option.value, creature: 'Orc', count: 1 }, + verify: true, + }) + assert.equal(result.ok, true, `${option.value} must resolve`) + points.push(option.value) + } + assert.notEqual(points[0], points[1]) +}) + +test('a place published before the group was carried still resolves', async () => { + // Every event published before the fix stores `facet/name`, and a published + // version is immutable — so a parse that stopped understanding the two-part + // form would break those runs rather than correct them. It keeps the old + // first-match read, which is imprecise in exactly the way it always was. + shardAtlas.listLandmarks = async () => [ + { facet: 'Felucca', name: 'Entrance', group: 'Blighted Grove', x: 586, y: 1643, z: 0 }, + { facet: 'Felucca', name: 'Entrance', group: 'Destard', x: 1176, y: 2637, z: 0 }, + // A name carrying a slash reads as three parts too; the two-part read is what + // resolves it, which is why the three-part attempt must not answer for it. + { facet: 'Felucca', name: 'Odd/Name', group: null, x: 10, y: 20, z: 0 }, + ] + + for (const place of ['Felucca/Entrance', 'Felucca/Odd/Name']) { + const result = await byId('uo.creature.spawn').perform({ + runId: 42, + idempotencyKey: `P${place}`.padEnd(40, 'x'), + params: { place, creature: 'Orc', count: 1 }, + verify: true, + }) + assert.equal(result.ok, true, `${place} must still resolve`) + } + + // And a three-part value whose group is gone REFUSES rather than silently + // landing somewhere else. That is the honest answer: it asked for one place. + const gone = await byId('uo.creature.spawn').perform({ + runId: 42, + idempotencyKey: 'G'.repeat(40), + params: { place: 'Felucca/Renamed/Entrance', creature: 'Orc', count: 1 }, + verify: true, + }) + assert.equal(gone.ok, false) + assert.match(gone.error, /no landmark called/) +}) + // ── The world verbs (Phase 12a) ─────────────────────────────── test('a spawn files one ledger row per serial, not one per call', async () => { diff --git a/server/utils/spawnAtlasSource.js b/server/utils/spawnAtlasSource.js index 5b9364e..6bb12f2 100644 --- a/server/utils/spawnAtlasSource.js +++ b/server/utils/spawnAtlasSource.js @@ -180,8 +180,13 @@ function hashSources(root) { * targets (Phase 12b). The bump is what re-reads a tree the boot path * would otherwise skip on an unchanged hash — the source files have not * changed, only what is kept from them. + * 5 — and it did NOT keep it: version 4 bumped the parser and the aggregator + * below still discarded the field, so the intent above shipped as a + * comment. This bump is what makes an already-imported tree re-read now + * that the mapping keeps it; without it `sameSources` sees an unchanged + * tree and every existing install stays empty. */ -const PARSER_VERSION = 4 +const PARSER_VERSION = 5 /** True when two source fingerprints describe the same tree. */ function sameSources(a, b) { @@ -304,6 +309,16 @@ function buildAtlas(root, options = {}) { const place = resolveRegion(point.x, point.y, point.facet, placement, resolveOpts) return { name: point.name, + // **The field this whole `PARSER_VERSION` note was about, and it was + // dropped right here.** The parser has produced it since Phase 12b and + // the column and the query have both been waiting for it, but this + // mapping rebuilds each point from an explicit field list and `uniqueId` + // was not on it — so every row landed with `unique_id` NULL, and + // `listSpawners`, whose WHERE is `unique_id IS NOT NULL`, could only ever + // answer empty. That made `uo.options.spawners` an empty dropdown and + // every Phase 12b object-property lease unauthorable, with nothing on the + // form to say why. Found by the Phase 16b walk against a released bundle. + uniqueId: point.uniqueId, facet: point.facet, x: point.x, y: point.y,