From 261de9dc816b350d3eb3e4819ded174bf59fa389 Mon Sep 17 00:00:00 2001 From: Sebastian Schloesser Date: Thu, 20 Aug 2026 15:53:35 -0600 Subject: [PATCH 1/2] Curate two Maltese localities misfiled under Mali MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Who's On First ships duplicates of Mellieħa (101912869) and Naxxar (101846471) inside whosonfirst-data-admin-ml. Each carries Malta's coordinates, name and population - Mellieħa's population is 5976, exactly the Maltese record's - but wof:country ML, wof:parent_id -1 and a hierarchy containing nothing but Mali's country id. Between them they own three cells over the real towns, so those coordinates reverse geocode into Mali while forward search returns the Maltese ids, and the two can no longer be grouped. Curation is the last resort here, so the rationale records the four generic rules that were tried against a world build and the measured reason each was rejected: distance from the nearest same-country neighbour (useless - the misfiled records sit beside each other, 1 km apart - and unsafe, since 973 legitimate places are more than 100 km from one); a missing admin1 parent (only 20 localities world-wide, but they include Lilongwe, Banjul, Honiara and Tarawa); validating the country tag against the country polygon holding the centroid (impossible - the admin repositories ship no country records at all, 0 in Mali's 15,060 files and 0 in Malta's 395); and cross-country name-and-population duplicate matching (narrow enough at 14 pairs world-wide, but it misses this record because "ħ" has no Unicode decomposition to "h"). The entry needed two small, guarded additions to the overlay format: - "absorbForeignTagged" lets an absorb id carry another country's tag, which is the whole defect. It is opt-in per entry so it can never be the result of a typo'd id, and the target is never exempt - a file can still only produce labels for the country it is named for. - "probes[].country" lets a guard probe name the country its expected label belongs to, so the entry can assert that Mali itself is untouched. Verification resolves the country from the database, so a same-named place across the border cannot satisfy the guard. Both are covered red-green. Two Mali-tagged records inside Malta's bounding box are deliberately left out, and the entries say so: San Giljan owns no cells, and Xghajra's population of 837 does not clear the build's floor. Verified strictly against a Mali+Malta build with --verify and no --skip-unresolvable: 6 probes passed, 0 skipped, 0 failed, 3 cells relabeled. --- curation/README.md | 31 +++++++++++++ curation/mt.json | 31 +++++++++++++ scripts/apply_curation.js | 75 ++++++++++++++++++++++++------ spec/curation_spec.js | 98 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 221 insertions(+), 14 deletions(-) create mode 100644 curation/mt.json diff --git a/curation/README.md b/curation/README.md index b7d8f80..169aff3 100644 --- a/curation/README.md +++ b/curation/README.md @@ -87,11 +87,13 @@ named for, and a country's whole overlay always travels in one file. | `entries[].op` | string | Operation. Currently only `"merge"`. | | `entries[].into` | integer | Place id (Who's On First id) whose label wins. Must exist in `compact_places`. | | `entries[].absorb` | array of integers | Place ids whose reverse-lookup cells are relabeled to `into`. Must exist in `compact_places`; must not include `into`. | +| `entries[].absorbForeignTagged` | boolean | Optional, default `false`. Allows `absorb` ids whose stored `country_id` is *not* the file's country — for a record misfiled under the wrong country while carrying this country's coordinates. `into` is never exempt, so a file can still only ever produce labels for the country it is named for. | | `entries[].minPrecision` | integer (1–12) | Only geohash cells of at least this length are relabeled. Coarser cells keep their original owner. | | `entries[].rationale` | string | Required. The reviewable justification for the entry. | | `entries[].probes` | array | Required. Coordinates with expected reverse-lookup labels, checked by `--verify`. Must include at least one positive probe (`expect` equals the merge target's name, verified against the target's place id) and at least one guard probe (`expect` differs). | | `probes[].lat`, `probes[].lon` | number | Probe coordinate. | | `probes[].expect` | string | Expected `result.name` from a reverse lookup at that coordinate. Must name a place that exists in the entry's country (typo protection). | +| `probes[].country` | string | Optional two-letter uppercase ISO code, default the entry's country. Names the country the expected label belongs to, so a guard can assert that a coordinate still resolves *outside* the curated country. Verification checks the resolved place's country, so a same-named place across the border cannot satisfy it. | | `probes[].note` | string | Optional human context (what the coordinate is, or which guard it enforces). | Unknown fields are rejected so that typos (`"absorbs"`, `"minPrecison"`) fail @@ -265,6 +267,24 @@ the source owning that cell has no relabelable cells at all, the same situation *is* a data gap — the build simply has not reached this precision here — and stays deferrable. +### Misfiled records: when the country tag itself is the defect + +Occasionally a source record carries one country's coordinates, name and +population while being filed under another country entirely — Who's On First +ships a duplicate of Mellieħa, Malta inside the Mali repository, tagged `ML` +with `wof:parent_id: -1`. Such a record can win the cells over the real town, +so reverse geocoding answers with a foreign id while forward search answers +with the correct one, and the two can no longer be grouped. + +This is data repair, not a labeling opinion, so prefer a build rule if one can +be made to work — `mt.json` records the four generic rules that were tried +against a world build and the measured reason each was rejected, which is the +sort of evidence a reviewer should expect before accepting an entry of this +kind. When no rule can reach it, absorb the misfiled record into the correct +place with `"absorbForeignTagged": true`, and pin the blast radius with a guard +probe in the country the record was wrongly filed under, using +`probes[].country`. + ### A note on the Guatemala probes Every coordinate in `gt.json` was chosen **empirically against a world build @@ -341,3 +361,14 @@ changing them are strict by design: intentionally keep their own identities, and Villa Canales — which owns the corridor cells nearer the city — keeps its own name too. Verified strictly against a world build; see the note on the Guatemala probes above. +- **`mt.json` — Malta.** Who's On First files duplicates of two Maltese + localities inside the Mali repository — Mellieħa (101912869) and Naxxar + (101846471) — each carrying Malta's coordinates, name and population but a + Mali country tag, no parent and a hierarchy containing only Mali. Between + them they own three cells over the real towns, so those coordinates reverse + geocode into Mali while forward search returns the Maltese ids. Each is + absorbed into its correct Maltese locality with `absorbForeignTagged`. Two + further Mali-tagged records inside Malta's bounding box are deliberately left + out and the entries say why: San Giljan owns no cells, and Xghajra does not + clear the build's population floor. The rationale records the four generic + rules that were measured and rejected before curating. Verified strictly. diff --git a/curation/mt.json b/curation/mt.json new file mode 100644 index 0000000..eab3829 --- /dev/null +++ b/curation/mt.json @@ -0,0 +1,31 @@ +{ + "country": "MT", + "entries": [ + { + "op": "merge", + "into": 101752447, + "absorb": [101912869], + "absorbForeignTagged": true, + "minPrecision": 5, + "rationale": "Who's On First carries a duplicate of Mellieħa (101912869) inside whosonfirst-data-admin-ml, the Mali repository: the record has Malta's coordinates (35.9707, 14.3656), Malta's name and Malta's population (5976, identical to the Maltese record 101752447), but wof:country ML, iso:country ML, wof:parent_id -1 and a hierarchy containing nothing but Mali's country id. It is a misfiled record, not a place in Mali. Because it is the only owner of the cells over northern Mellieħa, reverse geocoding there answers with a Malian id while forward search answers with the Maltese one, so a resident and someone picking the town out of search receive different city keys and cannot be grouped together. This needs curation rather than a build rule because every generic rule that could catch it was tried against the world build and measured: (1) flagging places far from any same-country neighbour is useless here, since the four misfiled Maltese records sit beside each other and their nearest ML neighbour is 1 km away, and it is unsafe anyway - 973 places world-wide are more than 100 km from a same-country neighbour and are entirely legitimate (Svalbard, Bonaire, French Polynesia); (2) flagging places with no admin1 parent touches only 20 localities world-wide, but they include Lilongwe, Banjul, Honiara and Tarawa, so it would drop capitals; (3) validating the country tag against the country polygon holding the centroid is impossible from this dataset, because whosonfirst-data-admin-* repositories ship no country records at all - 0 in Mali's 15,060 files, 0 in Malta's 395; (4) matching cross-country duplicates on name and population is narrow enough (14 pairs world-wide, 4 with identical population) but misses this very record, because 'ħ' has no Unicode decomposition to 'h' and the two names never normalise equal. The defect is in one upstream record and only a statement about that record can fix it.", + "probes": [ + { "lat": 35.958386, "lon": 14.367172, "expect": "Mellieha", "note": "Mellieħa's own centre; must be the Maltese record, not the Mali-tagged clone" }, + { "lat": 35.939221, "lon": 14.400375, "expect": "San Pawl Il-Bahar", "note": "guard: the neighbouring Maltese locality keeps its own identity" }, + { "lat": 12.650000, "lon": -8.000000, "expect": "Bamako", "country": "ML", "note": "guard: Mali itself is untouched - absorbing a misfiled record must not reach real Malian places" } + ] + }, + { + "op": "merge", + "into": 101752441, + "absorb": [101846471], + "absorbForeignTagged": true, + "minPrecision": 5, + "rationale": "The same defect, same repository: Naxxar (101846471) is filed under Mali with Malta's coordinates (35.9418, 14.4590) and Malta's population (10378, identical to the Maltese locality 101752441), and it owns the cell over north-eastern Naxxar outright, so every point in that cell reverse-geocodes into Mali. The target is 101752441 rather than the other Maltese record of the same name (101846467), because 101752441 is parented to the Naxxar region 85686937 while 101846467 is parented to Gharghur, which does not describe it. Two further Mali-tagged records inside Malta's bounding box are deliberately left out. San Giljan 101752435 owns no cells in the world build, so it can produce no wrong answer and absorbing it would be a no-op that only adds a claim to review; Xghajra 101846463 has a population of 837 and does not survive the build's population floor at all. Both are recorded here so a future contributor can see they were considered rather than missed - if a later build gives either of them cells, they belong in this file.", + "probes": [ + { "lat": 35.969238, "lon": 14.436035, "expect": "Naxxar", "note": "centre of the cell the Mali-tagged record owned; must be the Maltese Naxxar" }, + { "lat": 35.939221, "lon": 14.400375, "expect": "San Pawl Il-Bahar", "note": "guard: the merge must not reach west into San Pawl Il-Bahar" }, + { "lat": 16.270000, "lon": -0.050000, "expect": "Gao", "country": "ML", "note": "guard: real Malian places keep their own labels" } + ] + } + ] +} diff --git a/scripts/apply_curation.js b/scripts/apply_curation.js index bb061db..955d105 100644 --- a/scripts/apply_curation.js +++ b/scripts/apply_curation.js @@ -10,8 +10,8 @@ const SUPPORTED_OPS = ['merge'] const MIN_PRECISION_FLOOR = 1 const MIN_PRECISION_CEILING = 12 -const ENTRY_KEYS = ['op', 'into', 'absorb', 'minPrecision', 'rationale', 'probes'] -const PROBE_KEYS = ['lat', 'lon', 'expect', 'note'] +const ENTRY_KEYS = ['op', 'into', 'absorb', 'absorbForeignTagged', 'minPrecision', 'rationale', 'probes'] +const PROBE_KEYS = ['lat', 'lon', 'expect', 'country', 'note'] const DOCUMENT_KEYS = ['country', 'entries'] function parseArgs(argv) { @@ -146,10 +146,18 @@ function validateProbe(probe, label) { throw new Error(label + ': "note" must be a non-empty string when present') } + // A probe may name the country its expected label belongs to, so a guard can + // assert that a coordinate still resolves into a *different* country than + // the file curates. Defaults to the file's own country. + if (probe.country !== undefined && (typeof probe.country !== 'string' || !/^[A-Z]{2}$/.test(probe.country))) { + throw new Error(label + ': "country" must be a two-letter uppercase ISO code when present') + } + return { lat: lat, lon: lon, expect: probe.expect.trim(), + country: probe.country || null, note: probe.note ? probe.note.trim() : null } } @@ -192,6 +200,10 @@ function validateEntry(entry, label) { MIN_PRECISION_FLOOR + ' and ' + MIN_PRECISION_CEILING) } + if (entry.absorbForeignTagged !== undefined && typeof entry.absorbForeignTagged !== 'boolean') { + throw new Error(label + ': "absorbForeignTagged" must be a boolean when present') + } + if (typeof entry.rationale !== 'string' || !entry.rationale.trim()) { throw new Error(label + ': "rationale" must be a non-empty string explaining the judgment call') } @@ -209,6 +221,7 @@ function validateEntry(entry, label) { op: entry.op, into: into, absorb: absorb, + absorbForeignTagged: Boolean(entry.absorbForeignTagged), minPrecision: entry.minPrecision, rationale: entry.rationale.trim(), probes: probes @@ -415,14 +428,25 @@ async function assertPlaceIdsExist(db, entries) { // A referenced id must exist AND belong to the file's declared country: a // typo'd id that happens to identify a real place in another country must // not silently relabel that foreign place's cells. + // + // The one exception is a record whose own country tag is the defect - a + // place filed under the wrong country while carrying this country's + // coordinates. Absorbing it needs "absorbForeignTagged": true, so it is + // always a visible, reviewed decision rather than the result of a typo. The + // target is never exempt: a file can still only ever produce labels for the + // country it is named for. function checkId(id, role, entry, problems) { var label = entry.source + ' entry ' + entry.index if (countryById[id] === undefined) { problems.push('place id ' + id + ' ("' + role + '", ' + label + ') not found in compact_places') - } else if (countryById[id] !== entry.country) { - problems.push('place id ' + id + ' ("' + role + '", ' + label + ') belongs to country ' + - (countryById[id] || '') + ', not ' + entry.country) + return } + if (countryById[id] === entry.country) return + if (role === 'absorb' && entry.absorbForeignTagged) return + + problems.push('place id ' + id + ' ("' + role + '", ' + label + ') belongs to country ' + + (countryById[id] || '') + ', not ' + entry.country + + (role === 'absorb' ? '; set "absorbForeignTagged": true if the record is misfiled under that country' : '')) } var problems = [] @@ -465,25 +489,26 @@ async function assertGuardProbes(db, entries) { for (var j = 0; j < entry.probes.length; j++) { var probe = entry.probes[j] - if (probe.expect === intoName) { + var probeCountry = probe.country || entry.country + if (probe.expect === intoName && probeCountry === entry.country) { hasPositive = true } else { hasGuard = true } - var nameKey = entry.country + '|' + probe.expect + var nameKey = probeCountry + '|' + probe.expect if (nameExistsCache[nameKey] === undefined) { var rows = await dbAll(db, ` SELECT COUNT(*) AS count FROM compact_places WHERE name = ? AND UPPER(country_id) = ? - `, [probe.expect, entry.country]) + `, [probe.expect, probeCountry]) nameExistsCache[nameKey] = Boolean(rows.length && rows[0].count > 0) } if (!nameExistsCache[nameKey]) { problems.push(entry.source + ' entry ' + entry.index + ': probes[' + j + '] expects "' + - probe.expect + '", which does not name any place in country ' + entry.country) + probe.expect + '", which does not name any place in country ' + probeCountry) } } @@ -1008,6 +1033,16 @@ async function entryHasDrainedCells(db, entry) { // Completeness: an entry that relabeled cells must have at least one passing // positive probe resolving from those cells, or its own effect was never // tested (a positive probe can otherwise pass on target-native territory). +var placeCountryCache = Object.create(null) + +async function placeCountry(db, id) { + if (placeCountryCache[id] === undefined) { + var rows = await dbAll(db, 'SELECT country_id FROM compact_places WHERE id = ?', [id]) + placeCountryCache[id] = rows.length ? String(rows[0].country_id || '').toUpperCase() : null + } + return placeCountryCache[id] +} + async function verifyEntries(db, entries, skipUnresolvable, boundary, resolvability) { var createGeocoder = require('../src/index.js') var geocoder = createGeocoder({ @@ -1045,14 +1080,23 @@ async function verifyEntries(db, entries, skipUnresolvable, boundary, resolvabil // They must also resolve FROM A LOOKUP CELL: the nearest-centroid // fallback can return the target itself for a coordinate no cell // covers, which would pass without exercising any relabeled cell. - var isPositive = probe.expect === intoName + var probeCountry = probe.country || entry.country + var isPositive = probe.expect === intoName && probeCountry === entry.country + // A country-qualified probe must resolve into that country, so a guard + // cannot be satisfied by a same-named place on the other side of it. The + // country comes from the database rather than the reverse result, whose + // shape differs between reader modes. + var resolvedCountry = result && result.id !== undefined + ? await placeCountry(db, result.id) + : null + var countryMatches = !probe.country || resolvedCountry === probeCountry var idMatches = !isPositive || Boolean(result && Number(result.id) === entry.into) var pathMatches = !isPositive || resolvedVia === 'geohash_lookup' var territory = isPositive ? await probeTerritory(db, entry, probe, boundary) : { onDrainedCell: false, throughDrainedCell: false } - if (actual === probe.expect && idMatches && pathMatches) { + if (actual === probe.expect && idMatches && pathMatches && countryMatches) { console.log('PASS ' + context + ' -> "' + actual + '"') passed += 1 if (territory.throughDrainedCell) { @@ -1061,9 +1105,9 @@ async function verifyEntries(db, entries, skipUnresolvable, boundary, resolvabil continue } - var expectKey = entry.country + '|' + probe.expect + var expectKey = probeCountry + '|' + probe.expect if (expectOwnsCache[expectKey] === undefined) { - expectOwnsCache[expectKey] = await expectedPlaceOwnsCells(db, probe.expect, entry.country) + expectOwnsCache[expectKey] = await expectedPlaceOwnsCells(db, probe.expect, probeCountry) } // A positive probe can end up on a cell still owned by an absorbed @@ -1086,7 +1130,7 @@ async function verifyEntries(db, entries, skipUnresolvable, boundary, resolvabil var reasons = [] if (!misplacedPositive) { if (!expectOwnsCache[expectKey]) { - reasons.push('expected place "' + probe.expect + '" (' + entry.country + ') owns no cells in this database yet') + reasons.push('expected place "' + probe.expect + '" (' + probeCountry + ') owns no cells in this database yet') } if (missingSources.length && isPositive && !territory.onDrainedCell) { reasons.push('merge source(s) ' + missingSources.join(', ') + ' own no cells at precision >= ' + entry.minPrecision + ' in this database yet') @@ -1109,6 +1153,9 @@ async function verifyEntries(db, entries, skipUnresolvable, boundary, resolvabil territory.matched.place_id + ' because it is below this entry\'s minPrecision ' + entry.minPrecision + '); this probe stands on ground the merge deliberately leaves alone, so it can never pass' + ' — move it onto a cell the entry relabels, or make it a guard probe expecting "' + actual + '"' + } else if (actual === probe.expect && !countryMatches) { + hint = ' — resolved to a place named "' + actual + '" in ' + + (resolvedCountry || '?') + ', not ' + probeCountry } else if (actual === probe.expect && !idMatches) { got = '"' + actual + '" (same-named place ' + (result && result.id) + ', not the merge target ' + entry.into + ')' } else if (actual === probe.expect && !pathMatches) { diff --git a/spec/curation_spec.js b/spec/curation_spec.js index ff13ad9..5c4638f 100644 --- a/spec/curation_spec.js +++ b/spec/curation_spec.js @@ -531,6 +531,104 @@ describe('curation overlay (scripts/apply_curation.js)', () => { } }); + it('absorbs a foreign-tagged record only behind the explicit opt-in', async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'offline-geocoder-curation-')); + try { + const dbPath = path.join(dir, 'compact.sqlite'); + await seedCompactDb(dbPath); + const before = await lookupSnapshot(dbPath); + + // HOMONYM carries a CA tag while sitting on this file's ground - the + // misfiled-record case. Without the opt-in it is indistinguishable from + // a typo'd id, and must still be refused. + const withoutOptIn = baseDoc(); + withoutOptIn.entries[0].absorb = [EAST, HOMONYM]; + const refused = runCurate([ + '--database', dbPath, + '--curation', writeDoc(dir, 'us.json', withoutOptIn) + ]); + expect(refused.status).toEqual(1); + expect(refused.stderr).toContain('belongs to country CA'); + expect(refused.stderr).toContain('absorbForeignTagged'); + expect(await lookupSnapshot(dbPath)).toEqual(before); + + // With it, the foreign-tagged source is absorbed - but the target is + // never exempt, so the file can still only produce its own country's + // labels. + const withOptIn = baseDoc(); + withOptIn.entries[0].absorb = [EAST, HOMONYM]; + withOptIn.entries[0].absorbForeignTagged = true; + const accepted = runCurate([ + '--database', dbPath, + '--curation', writeDoc(dir, 'us.json', withOptIn) + ]); + expect(accepted.status).toEqual(0); + const after = await lookupSnapshot(dbPath); + const homonymRow = after.find((row) => row.geohash === cells.homonymFine); + expect(homonymRow.place_id).toEqual(CITY); + + const foreignTarget = baseDoc(); + foreignTarget.entries[0].into = HOMONYM; + foreignTarget.entries[0].absorbForeignTagged = true; + const rejectedTarget = runCurate([ + '--database', dbPath, + '--curation', writeDoc(dir, 'us.json', foreignTarget) + ]); + expect(rejectedTarget.status).toEqual(1); + expect(rejectedTarget.stderr).toContain('belongs to country CA'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('holds a country-qualified guard probe to that country', async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'offline-geocoder-curation-')); + try { + const dbPath = path.join(dir, 'compact.sqlite'); + await seedCompactDb(dbPath); + + // 'Ghost Town' names a place in both US and CA. A guard probe that names + // the country must be satisfied by the CA one only - without the country + // check the same-named US place would silently pass it. + const doc = baseDoc(); + doc.entries[0].probes.push({ + lat: points.homonym.lat, + lon: points.homonym.lon, + expect: 'Ghost Town', + country: 'CA', + note: 'guard: the Canadian homonym keeps its own label' + }); + const passing = runCurate([ + '--database', dbPath, + '--curation', writeDoc(dir, 'us.json', doc), + '--verify' + ]); + expect(passing.status).toEqual(0); + expect(passing.stdout).toContain('the Canadian homonym keeps its own label'); + + // Pointing the same expectation at the US homonym's ground fails on the + // country even though the name matches. + await seedCompactDb(path.join(dir, 'second.sqlite')); + const wrongCountry = baseDoc(); + wrongCountry.entries[0].probes.push({ + lat: points.ghost.lat, + lon: points.ghost.lon, + expect: 'Ghost Town', + country: 'CA', + note: 'guard: pointed at the US homonym' + }); + const failing = runCurate([ + '--database', path.join(dir, 'second.sqlite'), + '--curation', writeDoc(dir, 'us.json', wrongCountry), + '--verify' + ]); + expect(failing.status).toEqual(1); + expect(failing.stdout + failing.stderr).toContain('not CA'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + it('does not defer guard failures unrelated to a missing merge source', async () => { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'offline-geocoder-curation-')); try { From 42ad8afd9829257a7a06ce8e3f50cf3ffea97059 Mon Sep 17 00:00:00 2001 From: Sebastian Schloesser Date: Thu, 20 Aug 2026 16:59:23 -0600 Subject: [PATCH 2/2] Hold every probe to its country, not just qualified ones The country check short-circuited on `!probe.country`, so it only ever ran for probes that spelled a country out. An omitted "country" is documented to mean the entry's own country, and that default was never enforced: every probe written without one - including all six in gt.json - could be satisfied by a same-named place across a border, which is the exact confusion the field was added to prevent. The comparison now runs unconditionally. Case: no mismatch to reconcile. Entry and probe countries are validated as /^[A-Z]{2}$/ and the files declare "GT"/"MT"; placeCountry() reads country_id, which the builds store uppercase, and uppercases it anyway. Both sides are normalised explicitly so the comparison stays correct if either validator is ever relaxed. Null: fails closed. compact_places.country_id is NOT NULL and no built database has an empty one, so this can only mean the reverse result carried no id - and a verification tool must never pass a probe it was unable to check. Pinned by its own spec, because nothing else was holding that decision in place. The failure hint names the resolved country and, for an unqualified probe, says probes default to the entry's country, so an author who meant to cross a border knows to add the field rather than guessing. Verified strictly over both curation files against a gt+ml+mt build: 12 probes passed, 0 skipped, 0 failed, 7 cells relabeled. Forcing every resolved country to a wrong value turns all of them red - including the six unqualified gt.json probes - which is what proves they were checked rather than skipped. --- scripts/apply_curation.js | 31 +++++++++++---- spec/curation_spec.js | 80 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 8 deletions(-) diff --git a/scripts/apply_curation.js b/scripts/apply_curation.js index 955d105..0213393 100644 --- a/scripts/apply_curation.js +++ b/scripts/apply_curation.js @@ -1080,16 +1080,30 @@ async function verifyEntries(db, entries, skipUnresolvable, boundary, resolvabil // They must also resolve FROM A LOOKUP CELL: the nearest-centroid // fallback can return the target itself for a coordinate no cell // covers, which would pass without exercising any relabeled cell. - var probeCountry = probe.country || entry.country - var isPositive = probe.expect === intoName && probeCountry === entry.country - // A country-qualified probe must resolve into that country, so a guard - // cannot be satisfied by a same-named place on the other side of it. The - // country comes from the database rather than the reverse result, whose - // shape differs between reader modes. + var entryCountry = String(entry.country).toUpperCase() + var probeCountry = String(probe.country || entry.country).toUpperCase() + var isPositive = probe.expect === intoName && probeCountry === entryCountry + + // The expected label must live in the expected country, whether or not + // the probe spelled that country out. An omitted "country" means the + // entry's own country, so a same-named place across the border must not + // satisfy an unqualified guard either - which is the whole reason the + // field exists. + // + // Both sides are already uppercase: entry and probe countries are + // validated as /^[A-Z]{2}$/, and placeCountry() uppercases the stored + // country_id. The explicit normalisation keeps the comparison correct if + // either validator is ever relaxed. + // + // The country comes from the database rather than the reverse result, + // whose shape differs between reader modes. A country that cannot be + // resolved fails closed: compact_places.country_id is NOT NULL, so this + // means the result carried no id at all, and a verification tool must + // never pass a probe it was unable to check. var resolvedCountry = result && result.id !== undefined ? await placeCountry(db, result.id) : null - var countryMatches = !probe.country || resolvedCountry === probeCountry + var countryMatches = Boolean(resolvedCountry) && resolvedCountry === probeCountry var idMatches = !isPositive || Boolean(result && Number(result.id) === entry.into) var pathMatches = !isPositive || resolvedVia === 'geohash_lookup' var territory = isPositive @@ -1155,7 +1169,8 @@ async function verifyEntries(db, entries, skipUnresolvable, boundary, resolvabil ' — move it onto a cell the entry relabels, or make it a guard probe expecting "' + actual + '"' } else if (actual === probe.expect && !countryMatches) { hint = ' — resolved to a place named "' + actual + '" in ' + - (resolvedCountry || '?') + ', not ' + probeCountry + (resolvedCountry || '') + ', not ' + probeCountry + + (probe.country ? '' : ' (probes default to the entry\'s country; set "country" if another is intended)') } else if (actual === probe.expect && !idMatches) { got = '"' + actual + '" (same-named place ' + (result && result.id) + ', not the merge target ' + entry.into + ')' } else if (actual === probe.expect && !pathMatches) { diff --git a/spec/curation_spec.js b/spec/curation_spec.js index 5c4638f..ab50cf6 100644 --- a/spec/curation_spec.js +++ b/spec/curation_spec.js @@ -581,6 +581,86 @@ describe('curation overlay (scripts/apply_curation.js)', () => { } }); + it('holds an unqualified guard probe to the entry country', async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'offline-geocoder-curation-')); + try { + const dbPath = path.join(dir, 'compact.sqlite'); + await seedCompactDb(dbPath); + + // 'Ghost Town' exists in both US and CA. This guard omits "country", so + // it means the entry's own country - US - but the coordinate resolves to + // the Canadian homonym. Matching on name alone would pass it, which is + // exactly the cross-border confusion the country check exists to stop, + // and it would apply to every probe ever written without a "country". + const doc = baseDoc(); + doc.entries[0].probes.push({ + lat: points.homonym.lat, + lon: points.homonym.lon, + expect: 'Ghost Town', + note: 'guard: unqualified, so it means the entry country' + }); + + const result = runCurate([ + '--database', dbPath, + '--curation', writeDoc(dir, 'us.json', doc), + '--verify' + ]); + + expect(result.status).toEqual(1); + const output = result.stdout + result.stderr; + expect(output).toContain('not US'); + // And it says why, so the author can qualify the probe if that was the + // intent rather than guessing at the failure. + expect(output).toContain('probes default to the entry'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('fails a probe whose resolved place carries no country', async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'offline-geocoder-curation-')); + try { + const dbPath = path.join(dir, 'compact.sqlite'); + await seedCompactDb(dbPath); + + // A place with no country at all owns the cell the probe lands on. The + // name matches, so only the country check can catch it - and it has + // nothing to compare against. Verification fails closed rather than + // passing a probe it could not actually check. + const orphanPoint = { lat: 60.2, lon: 60.2 }; + const orphanCell = geohash.encode(orphanPoint.lat, orphanPoint.lon, 5); + const db = new sqlite3.Database(dbPath); + try { + await exec(db, ` + INSERT INTO compact_places(id, name, country_id, admin1_id, placetype_code, latitude, longitude) + VALUES (9999001, 'Bystander County', '', 5, 3, ${orphanPoint.lat}, ${orphanPoint.lon}); + INSERT INTO compact_geohash_lookup(geohash, place_id) VALUES ('${orphanCell}', 9999001); + `); + } finally { + await close(db); + } + + const doc = baseDoc(); + doc.entries[0].probes.push({ + lat: orphanPoint.lat, + lon: orphanPoint.lon, + expect: 'Bystander County', + note: 'guard: resolved place has no country' + }); + + const result = runCurate([ + '--database', dbPath, + '--curation', writeDoc(dir, 'us.json', doc), + '--verify' + ]); + + expect(result.status).toEqual(1); + expect(result.stdout + result.stderr).toContain(''); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + it('holds a country-qualified guard probe to that country', async () => { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'offline-geocoder-curation-')); try {