diff --git a/server/src/db/migrations.ts b/server/src/db/migrations.ts index 47235d413..713cb737f 100644 --- a/server/src/db/migrations.ts +++ b/server/src/db/migrations.ts @@ -5257,6 +5257,34 @@ function runMigrations(db: Database.Database): void { `); } }, + + /* + * A place that moves takes its Atlas country with it (#2527). + * + * place_regions caches the country and region Atlas resolved from a + * place's coordinates and address, and nothing re-derives a row that is + * already there. Correcting a place's location left the old row in charge, + * so Atlas, the dashboard stats and the journey stats kept counting the + * country the place had just left. + * + * A trigger rather than a delete in the places code, because the row is + * derived from the place and every writer of lat, lng or address has to + * let go of it: the place editor, update_place, the plugin RPC, the import + * enrichment and its address backfill, and whatever comes next. The WHEN + * clause matters because every place edit writes lat, lng and address + * back whether they changed or not, and renaming a place must not throw + * away a good row. The next Atlas load resolves the place where it is now. + */ + () => { + db.exec(` + CREATE TRIGGER IF NOT EXISTS trg_place_regions_follow_place + AFTER UPDATE OF lat, lng, address ON places + WHEN OLD.lat IS NOT NEW.lat OR OLD.lng IS NOT NEW.lng OR OLD.address IS NOT NEW.address + BEGIN + DELETE FROM place_regions WHERE place_id = NEW.id; + END + `); + }, ]; if (currentVersion < migrations.length) { diff --git a/server/src/nest/atlas/atlas-geo.ts b/server/src/nest/atlas/atlas-geo.ts index aae5efbb1..a81a7794a 100644 --- a/server/src/nest/atlas/atlas-geo.ts +++ b/server/src/nest/atlas/atlas-geo.ts @@ -781,7 +781,7 @@ async function resolveCountryCode(place: Place): Promise { return null; } -export function resolveCountryCodeSync(place: Place): string | null { +export function resolveCountryCodeSync(place: Pick): string | null { const hasCoords = !!(place.lat && place.lng); if (hasCoords) { const fromCoords = getCountryFromCoords(place.lat!, place.lng!); @@ -937,10 +937,13 @@ function buildRegionInfo(address: Record, preferFinest: boolean) }; } -export async function reverseGeocodeRegion(lat: number, lng: number, placeAddress?: string | null): Promise { - const key = cacheKeyFor(lat, lng, 'region'); - if (regionCache.has(key)) return regionCache.get(key)!; - +/** + * The offline half of reverseGeocodeRegion: the bundled admin1 polygons, then the + * box-gated address fallback. Null when the bundle has no answer for the point, which + * is where reverseGeocodeRegion goes on to ask Nominatim. Deterministic and uncached, + * so it is also what the one time place_regions repair re-derives rows with (#2527). + */ +export async function resolveRegionFromBundle(lat: number, lng: number, placeAddress?: string | null): Promise { // Prefer resolving directly against the bundled polygons: offline, deterministic, and — // unlike Nominatim's address levels — guaranteed to match a feature the client can // actually highlight. Falls through to reverse geocoding when the country has no admin1 @@ -948,10 +951,7 @@ export async function reverseGeocodeRegion(lat: number, lng: number, placeAddres const coordCountry = getCountryFromCoords(lat, lng); if (coordCountry) { const fromBundle = await getRegionFromCoords(coordCountry, lat, lng); - if (fromBundle) { - regionCache.set(key, fromBundle); - return fromBundle; - } + if (fromBundle) return fromBundle; } // The coordinate-only lookup found no matching region — either no country polygon contains // the point, or a simplified admin0 border put it in the WRONG country (a place on the @@ -966,11 +966,22 @@ export async function reverseGeocodeRegion(lat: number, lng: number, placeAddres // trusting a region match in it. const addressCountry = getCountryFromAddress(placeAddress ?? null); if (addressCountry && addressCountry !== coordCountry && isPointInCountryBox(addressCountry, lat, lng)) { - const fromAddress = await getRegionFromCoords(addressCountry, lat, lng); - if (fromAddress) { - regionCache.set(key, fromAddress); - return fromAddress; - } + return getRegionFromCoords(addressCountry, lat, lng); + } + return null; +} + +export async function reverseGeocodeRegion(lat: number, lng: number, placeAddress?: string | null): Promise { + // The address takes part in the answer only through the country it names, so that + // country is part of the key. Keyed by coordinates alone, a place whose address was + // corrected on a border point got the old answer back from memory (#2527). + const key = `${cacheKeyFor(lat, lng, 'region')}|${getCountryFromAddress(placeAddress ?? null) ?? ''}`; + if (regionCache.has(key)) return regionCache.get(key)!; + + const fromBundle = await resolveRegionFromBundle(lat, lng, placeAddress); + if (fromBundle) { + regionCache.set(key, fromBundle); + return fromBundle; } // Only reached when the bundle's own polygons for this country don't cover the point at diff --git a/server/src/nest/atlas/atlas.module.ts b/server/src/nest/atlas/atlas.module.ts index c2b9d4617..2367554aa 100644 --- a/server/src/nest/atlas/atlas.module.ts +++ b/server/src/nest/atlas/atlas.module.ts @@ -11,6 +11,8 @@ import { PublicStatsController } from './public-stats.controller'; import { ApiTokenGuard } from '../public-api/api-token.guard'; import { TokensModule } from '../tokens/tokens.module'; import { RateLimitModule } from '../common/rate-limit.module'; +import { SchedulingModule } from '../scheduling/scheduling.module'; +import { PlaceRegionsRepairJob } from './place-regions-repair.job'; /** * Atlas addon domain (L7 leaf module). Registered in AppModule. Exports @@ -29,11 +31,13 @@ import { RateLimitModule } from '../common/rate-limit.module'; * than pulled in with PublicApiModule: @UseGuards instantiates a guard in the * declaring controller's module, so exporting it from over there would still leave * its TokenService unresolvable here. TokensModule is a leaf, so the edge is free. + * + * PlaceRegionsRepairJob puts right, once, the place_regions rows cached before #2527. */ @Module({ - imports: [AuthModule, PluginGuardsModule, AddonsModule, TokensModule, RateLimitModule], + imports: [AuthModule, PluginGuardsModule, AddonsModule, TokensModule, RateLimitModule, SchedulingModule], controllers: [AtlasController, TravelStatsController, PublicStatsController], - providers: [AtlasService, AtlasMcp, AtlasRpc, ApiTokenGuard], + providers: [AtlasService, AtlasMcp, AtlasRpc, ApiTokenGuard, PlaceRegionsRepairJob], exports: [AtlasService], }) export class AtlasModule {} diff --git a/server/src/nest/atlas/atlas.service.ts b/server/src/nest/atlas/atlas.service.ts index 736aea4a0..77ebae710 100644 --- a/server/src/nest/atlas/atlas.service.ts +++ b/server/src/nest/atlas/atlas.service.ts @@ -4,14 +4,17 @@ import type { AtlasLocateResponse } from '@trek/shared'; import { Trip, Place } from '../../types'; import { DatabaseService } from '../database/database.service'; import { + getCountryFromAddress, getCountryFromCoords, getCountryGeoGz, getRegionFromCoords, getRegionGeo, geocodingInFlight, resolveCountryCodeSync, + resolveRegionFromBundle, reverseGeocodeRegion, } from './atlas-geo'; +import type { RegionInfo } from './atlas-geo'; import { KNOWN_COUNTRIES } from './known-countries'; import { cityFromAddress } from './city-from-address'; import { transferEndpointIds } from './transfer-endpoints'; @@ -19,6 +22,15 @@ import type { FlightEndpointRow } from './transfer-endpoints'; import { countryVisitDates } from './visit-dates'; import { haversineKm } from '../common/geo'; +/** The part of a place that its cached region is derived from. */ +type LocatedPlace = Pick; + +/** A place_regions row next to the location of its place. */ +type CachedRegionRow = LocatedPlace & { country_code: string; region_code: string }; + +/** How many cached rows the #2527 repair checks before it lets other work run. */ +const REPAIR_YIELD_EVERY = 200; + /** * A reservation endpoint plus the two booking columns that decide whether it may * take part in layover pairing at all (see transfer-endpoints.ts). @@ -175,32 +187,131 @@ export class AtlasService { } } - if (uncachedForGeocode.length > 0) { - const insertStmt = this.db.prepare( - 'INSERT OR REPLACE INTO place_regions (place_id, country_code, region_code, region_name) VALUES (?, ?, ?, ?)', - ); - for (const p of uncachedForGeocode) geocodingInFlight.add(p.id); - void (async () => { - try { - for (const place of uncachedForGeocode) { - try { - const info = await reverseGeocodeRegion(place.lat!, place.lng!, place.address); - if (info) insertStmt.run(place.id, info.country_code, info.region_code, info.region_name); - } catch { - /* continue */ - } finally { - geocodingInFlight.delete(place.id); - } - } - } catch { - for (const p of uncachedForGeocode) geocodingInFlight.delete(p.id); - } - })(); - } + this.cacheRegionsInBackground(uncachedForGeocode); return out; } + /** + * Resolve each place's region in the background and cache it in place_regions. + */ + private cacheRegionsInBackground(places: Place[]): void { + if (places.length === 0) return; + for (const p of places) geocodingInFlight.add(p.id); + void (async () => { + try { + for (const place of places) { + try { + const info = await reverseGeocodeRegion(place.lat!, place.lng!, place.address); + if (info) this.cacheRegionWhileUnmoved(place, info); + } catch { + // individual failure, continue with the remaining places + } finally { + geocodingInFlight.delete(place.id); + } + } + } catch { + for (const p of places) geocodingInFlight.delete(p.id); + } + })(); + } + + /** + * Cache a resolved region, but only while the place still sits where it was resolved + * from. A place moved while its lookup was running has already had its row dropped by + * the place_regions trigger (#2527), and writing the old answer back would pin it to + * the country it just left. A place deleted in the meantime is skipped the same way. + */ + private cacheRegionWhileUnmoved(place: LocatedPlace, info: RegionInfo): boolean { + const written = this.db.run( + `INSERT OR REPLACE INTO place_regions (place_id, country_code, region_code, region_name) + SELECT id, ?, ?, ? FROM places WHERE id = ? AND lat = ? AND lng = ? AND address IS ?`, + info.country_code, + info.region_code, + info.region_name, + place.id, + place.lat, + place.lng, + place.address ?? null, + ); + return written.changes > 0; + } + + // ── One time repair of the rows cached before #2527 ─────────────────────── + + /** + * Before #2527 nothing dropped a place_regions row when its place moved, so an + * install that upgrades still holds the country every corrected place left. This + * re-derives each row with the bundled resolver alone, never Nominatim, and puts + * right the ones that no longer match where their place is now. + * + * Where the bundle answers for the place's current location with another country + * or region, the row gets that answer, which is what a fresh lookup would cache. A + * good row matches it, the address fallback case included. + * + * Where the bundle has no answer (a coastal point outside the simplified polygons, + * a country the bundle has no regions for), the row came from Nominatim and only + * Nominatim could judge it, so it stays, unless the coordinates put the place in + * another country and the address does not name the cached one either. Then it is + * dropped and the next Atlas load looks the place up again. + * + * A place without coordinates keeps no row, the same as the trigger does. + * + * Every write only lands while the place still holds the location it was read with, + * so a place edited while this runs keeps what the trigger and the next lookup give it. + */ + async repairStaleRegionCache(): Promise<{ replaced: number; dropped: number }> { + const rows = this.db.all(` + SELECT pr.place_id AS id, pr.country_code, pr.region_code, p.lat, p.lng, p.address + FROM place_regions pr + JOIN places p ON p.id = pr.place_id + ORDER BY pr.place_id + `); + let replaced = 0; + let dropped = 0; + for (const [i, row] of rows.entries()) { + // A big install holds many rows, so requests get a turn every so often. + if (i > 0 && i % REPAIR_YIELD_EVERY === 0) await new Promise((resolve) => setImmediate(resolve)); + const fix = await this.staleRegionFix(row); + if (fix === 'drop') { + if (this.dropRegionWhileUnmoved(row)) dropped++; + } else if (fix && this.cacheRegionWhileUnmoved(row, fix)) { + replaced++; + } + } + return { replaced, dropped }; + } + + /** What a cached row needs: a new region, 'drop', or null when it is right. */ + private async staleRegionFix(row: CachedRegionRow): Promise { + if (row.lat == null || row.lng == null) return 'drop'; + const cachedCountry = row.country_code.toUpperCase(); + const fresh = await resolveRegionFromBundle(row.lat, row.lng, row.address); + if (fresh) { + const same = fresh.country_code.toUpperCase() === cachedCountry && fresh.region_code === row.region_code; + return same ? null : fresh; + } + const countryNow = resolveCountryCodeSync(row); + const contradicted = !!countryNow && countryNow !== cachedCountry && getCountryFromAddress(row.address) !== cachedCountry; + return contradicted ? 'drop' : null; + } + + private dropRegionWhileUnmoved(row: CachedRegionRow): boolean { + const removed = this.db.run( + `DELETE FROM place_regions + WHERE place_id = ? AND country_code = ? AND region_code = ? + AND EXISTS (SELECT 1 FROM places WHERE id = ? AND lat IS ? AND lng IS ? AND address IS ?)`, + row.id, + row.country_code, + row.region_code, + row.id, + row.lat ?? null, + row.lng ?? null, + row.address ?? null, + ); + return removed.changes > 0; + } + // ── Stats ───────────────────────────────────────────────────────────────── async stats(userId: number) { @@ -731,29 +842,9 @@ export class AtlasService { const cachedMap = new Map(cached.map((c) => [c.place_id, c])); // Kick off background geocoding for uncached places; return cached data immediately. - const uncached = places.filter((p) => p.lat && p.lng && !cachedMap.has(p.id) && !geocodingInFlight.has(p.id)); - if (uncached.length > 0) { - const insertStmt = this.db.prepare( - 'INSERT OR REPLACE INTO place_regions (place_id, country_code, region_code, region_name) VALUES (?, ?, ?, ?)', - ); - for (const p of uncached) geocodingInFlight.add(p.id); - void (async () => { - try { - for (const place of uncached) { - try { - const info = await reverseGeocodeRegion(place.lat!, place.lng!, place.address); - if (info) insertStmt.run(place.id, info.country_code, info.region_code, info.region_name); - } catch { - // individual failure — continue with remaining places - } finally { - geocodingInFlight.delete(place.id); - } - } - } catch { - for (const p of uncached) geocodingInFlight.delete(p.id); - } - })(); - } + this.cacheRegionsInBackground( + places.filter((p) => p.lat && p.lng && !cachedMap.has(p.id) && !geocodingInFlight.has(p.id)), + ); // Group by country → regions with place counts const regionMap: Record< diff --git a/server/src/nest/atlas/place-regions-repair.job.ts b/server/src/nest/atlas/place-regions-repair.job.ts new file mode 100644 index 000000000..97a3bcc49 --- /dev/null +++ b/server/src/nest/atlas/place-regions-repair.job.ts @@ -0,0 +1,53 @@ +import { logInfo, logError } from '../audit/audit-log.logger'; +import { DatabaseService } from '../database/database.service'; +import { CronRegistrarService } from '../scheduling/cron-registrar.service'; +import { AtlasService } from './atlas.service'; +import { Injectable, type OnApplicationBootstrap } from '@nestjs/common'; + +/** The app_settings row that says the pass below has been through every cached region. */ +export const PLACE_REGIONS_REPAIR_DONE_KEY = 'place_regions_repair_2527'; + +/** + * One pass over place_regions on the first start after #2527 (see + * AtlasService.repairStaleRegionCache). + * + * Not a cron: from this version on the trigger on places drops a row as soon as its + * place moves, so once the rows cached before it are put right there is nothing left + * to repair. app_settings remembers that the pass ran. A pass cut short by a restart + * or an error simply runs again on the next start; a second run changes nothing the + * first one wrote. + * + * In the background, like the other boot sweeps: the server does not wait for it. + */ +@Injectable() +export class PlaceRegionsRepairJob implements OnApplicationBootstrap { + constructor( + private readonly atlas: AtlasService, + private readonly registrar: CronRegistrarService, + private readonly db: DatabaseService, + ) {} + + onApplicationBootstrap(): void { + if (!this.registrar.isEnabled()) return; + void this.runOnce(); + } + + async runOnce(): Promise { + try { + if (this.db.get('SELECT 1 FROM app_settings WHERE key = ?', PLACE_REGIONS_REPAIR_DONE_KEY)) return; + const { replaced, dropped } = await this.atlas.repairStaleRegionCache(); + this.db.run( + 'INSERT OR REPLACE INTO app_settings (key, value) VALUES (?, ?)', + PLACE_REGIONS_REPAIR_DONE_KEY, + new Date().toISOString(), + ); + if (replaced + dropped > 0) { + logInfo( + `Atlas region repair: ${replaced} place region(s) re-derived where the place is now, ${dropped} left for the next Atlas load`, + ); + } + } catch (err: unknown) { + logError(`Atlas region repair: ${err instanceof Error ? err.message : err}`); + } + } +} diff --git a/server/tests/integration/atlas.test.ts b/server/tests/integration/atlas.test.ts index 6eafbb90b..1176babe2 100644 --- a/server/tests/integration/atlas.test.ts +++ b/server/tests/integration/atlas.test.ts @@ -536,3 +536,73 @@ describe('Regions geo', () => { expect(res.body).toHaveProperty('type', 'FeatureCollection'); }); }); + +describe('A place that moves takes its Atlas country with it (#2527)', () => { + // The region cache is filled by a background task that GET /regions only starts, + // so wait for the row the way the next Atlas load would find it. + async function regionRowOf(placeId: number): Promise<{ country_code: string; region_code: string } | undefined> { + for (let i = 0; i < 100; i++) { + const row = testDb.prepare('SELECT country_code, region_code FROM place_regions WHERE place_id = ?').get(placeId) as + | { country_code: string; region_code: string } + | undefined; + if (row) return row; + await new Promise((resolve) => setTimeout(resolve, 20)); + } + return undefined; + } + + async function atlasCountries(userId: number): Promise { + const res = await request(app).get('/api/addons/atlas/stats').set('Cookie', authCookie(userId)); + expect(res.status).toBe(200); + return (res.body.countries as { code: string }[]).map((c) => c.code); + } + + it('ATLAS-015: correcting a place from France to Germany moves it on the Atlas', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Wrongly imported', start_date: '2025-05-01', end_date: '2025-05-05' }); + const place = testDb + .prepare('INSERT INTO places (trip_id, name, lat, lng, address) VALUES (?, ?, ?, ?, ?) RETURNING id') + .get(trip.id, 'Hotel', 48.8566, 2.3522, 'Rue de Rivoli, Paris, France') as { id: number }; + + // The place is seen on the Atlas once, which caches France for it. + await request(app).get('/api/addons/atlas/regions').set('Cookie', authCookie(user.id)); + expect((await regionRowOf(place.id))?.country_code).toBe('FR'); + expect(await atlasCountries(user.id)).toEqual(['FR']); + + // The user fixes the location in the trip. + const put = await request(app) + .put(`/api/trips/${trip.id}/places/${place.id}`) + .set('Cookie', authCookie(user.id)) + .send({ lat: 52.5163, lng: 13.3777, address: 'Pariser Platz, Berlin, Germany' }); + expect(put.status).toBe(200); + + expect(await atlasCountries(user.id)).toEqual(['DE']); + + // And the regions follow on the next load, with nothing left in France. + await request(app).get('/api/addons/atlas/regions').set('Cookie', authCookie(user.id)); + expect((await regionRowOf(place.id))?.country_code).toBe('DE'); + const regions = await request(app).get('/api/addons/atlas/regions').set('Cookie', authCookie(user.id)); + expect(Object.keys(regions.body.regions)).toEqual(['DE']); + expect(await atlasCountries(user.id)).toEqual(['DE']); + }); + + it('ATLAS-015: an edit that leaves the location alone keeps the cached country', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Paris', start_date: '2025-05-01', end_date: '2025-05-05' }); + const place = testDb + .prepare('INSERT INTO places (trip_id, name, lat, lng, address) VALUES (?, ?, ?, ?, ?) RETURNING id') + .get(trip.id, 'Louvre', 48.8606, 2.3376, 'Rue de Rivoli, Paris, France') as { id: number }; + await request(app).get('/api/addons/atlas/regions').set('Cookie', authCookie(user.id)); + const cached = await regionRowOf(place.id); + expect(cached?.country_code).toBe('FR'); + + // The place editor sends the whole form back, location included, for a notes edit. + const put = await request(app) + .put(`/api/trips/${trip.id}/places/${place.id}`) + .set('Cookie', authCookie(user.id)) + .send({ notes: 'Closed on Tuesdays', lat: 48.8606, lng: 2.3376, address: 'Rue de Rivoli, Paris, France' }); + expect(put.status).toBe(200); + + expect(testDb.prepare('SELECT country_code, region_code FROM place_regions WHERE place_id = ?').get(place.id)).toEqual(cached); + }); +}); diff --git a/server/tests/unit/db/migration-hygiene.test.ts b/server/tests/unit/db/migration-hygiene.test.ts index 1e117cebc..9031db506 100644 --- a/server/tests/unit/db/migration-hygiene.test.ts +++ b/server/tests/unit/db/migration-hygiene.test.ts @@ -134,7 +134,7 @@ const ALLOWED_DESTRUCTIVE: Record = { 'DELETE FROM journey_entries': "Migration 121: DELETE ... WHERE title IN ('Gallery','[Trip Photos]') — remove synthetic wrapper entries replaced by the gallery model.", 'DELETE FROM place_regions': - 'Atlas enclave fix: DELETE ... WHERE place_id IN (places inside specific enclave boxes) — invalidate stale region cache; re-resolved on next request.', + 'Atlas enclave fix: DELETE ... WHERE place_id IN (places inside specific enclave boxes) to invalidate a stale region cache, re-resolved on next request. Also the body of trg_place_regions_follow_place (#2527): DELETE ... WHERE place_id = NEW.id when that one place\'s lat, lng or address changed. place_regions is a re-derivable cache either way.', 'DELETE FROM visited_regions': 'Atlas geoBoundaries swap (#1119): DELETE ... WHERE id = ? — after UPDATE OR IGNORE re-codes a manually-marked region to its current code, drop only the single leftover row whose UNIQUE(user_id, region_code) collision caused the update to be skipped (a duplicate of a region the user already has).', 'DELETE FROM hidden_regions': diff --git a/server/tests/unit/db/place-regions-follow-place.test.ts b/server/tests/unit/db/place-regions-follow-place.test.ts new file mode 100644 index 000000000..f2edaec02 --- /dev/null +++ b/server/tests/unit/db/place-regions-follow-place.test.ts @@ -0,0 +1,112 @@ +import { runMigrations } from '../../../src/db/migrations'; +import { createTestDb } from '../../helpers/test-db'; + +import Database from 'better-sqlite3'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; + +// place_regions is Atlas's cache of the country and region a place resolved to. +// It is derived from the place's lat, lng and address, so a change to any of them +// has to take the row with it, whichever code path wrote the change (#2527). + +// The trigger ships at schema version 244. Later migrations are appended after it, +// so the replay case rewinds to just before it; the steps after it are idempotent. +const PLACE_REGIONS_TRIGGER_VERSION = 244; + +describe('place_regions follows the place it was resolved from (#2527)', () => { + let db: Database.Database; + let placeId: number; + let otherPlaceId: number; + + const cached = (id: number) => + db.prepare('SELECT country_code, region_code FROM place_regions WHERE place_id = ?').get(id) as + | { country_code: string; region_code: string } + | undefined; + + beforeEach(() => { + db = createTestDb(); + db.exec("INSERT INTO users (username, email, password_hash) VALUES ('traveller', 'traveller@example.test', 'x')"); + const userId = (db.prepare('SELECT id FROM users').get() as { id: number }).id; + const tripId = db.prepare("INSERT INTO trips (user_id, title) VALUES (?, 'Trip')").run(userId) + .lastInsertRowid as number; + const insertPlace = db.prepare('INSERT INTO places (trip_id, name, lat, lng, address) VALUES (?, ?, ?, ?, ?)'); + placeId = insertPlace.run(tripId, 'Hotel', 48.8566, 2.3522, 'Rue de Rivoli, Paris, France') + .lastInsertRowid as number; + otherPlaceId = insertPlace.run(tripId, 'Museum', 48.8606, 2.3376, 'Rue de Rivoli, Paris, France') + .lastInsertRowid as number; + const insertRegion = db.prepare( + "INSERT INTO place_regions (place_id, country_code, region_code, region_name) VALUES (?, 'FR', 'FR-IDF', 'Ile-de-France')", + ); + insertRegion.run(placeId); + insertRegion.run(otherPlaceId); + }); + + afterEach(() => db?.close()); + + it('drops the cached region when the coordinates move', () => { + db.prepare('UPDATE places SET lat = ?, lng = ? WHERE id = ?').run(52.5163, 13.3777, placeId); + + expect(cached(placeId)).toBeUndefined(); + expect(cached(otherPlaceId)).toEqual({ country_code: 'FR', region_code: 'FR-IDF' }); + }); + + it('drops it when only the latitude or only the longitude changes', () => { + db.prepare('UPDATE places SET lat = ? WHERE id = ?').run(48.9, placeId); + db.prepare('UPDATE places SET lng = ? WHERE id = ?').run(2.4, otherPlaceId); + + expect(cached(placeId)).toBeUndefined(); + expect(cached(otherPlaceId)).toBeUndefined(); + }); + + it('drops it when the address changes, including one filled in where there was none', () => { + db.prepare('UPDATE places SET address = ? WHERE id = ?').run('Pariser Platz, Berlin, Germany', placeId); + db.prepare('UPDATE places SET address = NULL WHERE id = ?').run(otherPlaceId); + db.prepare( + "INSERT INTO place_regions (place_id, country_code, region_code, region_name) VALUES (?, 'FR', 'FR-IDF', 'Ile-de-France')", + ).run(otherPlaceId); + // The import backfill only ever fills an empty address. + db.prepare('UPDATE places SET address = COALESCE(address, ?) WHERE id = ?').run('Paris, France', otherPlaceId); + + expect(cached(placeId)).toBeUndefined(); + expect(cached(otherPlaceId)).toBeUndefined(); + }); + + it('drops it when the location is cleared', () => { + db.prepare('UPDATE places SET lat = NULL, lng = NULL WHERE id = ?').run(placeId); + + expect(cached(placeId)).toBeUndefined(); + }); + + it('keeps it when an edit writes the same location back', () => { + // The place editor and update_place write every column, location included. + db.prepare( + "UPDATE places SET name = 'Hotel du Louvre', notes = 'late check-in', lat = ?, lng = ?, address = ? WHERE id = ?", + ).run(48.8566, 2.3522, 'Rue de Rivoli, Paris, France', placeId); + db.prepare("UPDATE places SET place_time = '09:00', updated_at = CURRENT_TIMESTAMP WHERE id = ?").run(otherPlaceId); + + expect(cached(placeId)).toEqual({ country_code: 'FR', region_code: 'FR-IDF' }); + expect(cached(otherPlaceId)).toEqual({ country_code: 'FR', region_code: 'FR-IDF' }); + }); + + it('ships at its own schema version and replays without a second trigger', () => { + const triggers = () => + db + .prepare( + "SELECT name FROM sqlite_master WHERE type = 'trigger' AND tbl_name = 'places' AND name = 'trg_place_regions_follow_place'", + ) + .all(); + expect(triggers()).toHaveLength(1); + + db.exec('DROP TRIGGER trg_place_regions_follow_place'); + db.prepare('UPDATE schema_version SET version = ?').run(PLACE_REGIONS_TRIGGER_VERSION); + runMigrations(db); + expect(triggers()).toHaveLength(0); + + db.prepare('UPDATE schema_version SET version = ?').run(PLACE_REGIONS_TRIGGER_VERSION - 1); + runMigrations(db); + runMigrations(db); + expect(triggers()).toHaveLength(1); + + db.prepare('UPDATE places SET lat = ? WHERE id = ?').run(52.5163, placeId); + expect(cached(placeId)).toBeUndefined(); + }); +}); diff --git a/server/tests/unit/mcp/tools-places.test.ts b/server/tests/unit/mcp/tools-places.test.ts index 763a9d818..0147c7766 100644 --- a/server/tests/unit/mcp/tools-places.test.ts +++ b/server/tests/unit/mcp/tools-places.test.ts @@ -237,6 +237,26 @@ describe('Tool: update_place', () => { }); }); + it('moving a place drops the country Atlas cached for it, renaming it does not (#2527)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Hotel', lat: 48.8566, lng: 2.3522 }); + testDb.prepare("INSERT INTO place_regions (place_id, country_code, region_code, region_name) VALUES (?, 'FR', 'FR-IDF', 'Ile-de-France')").run(place.id); + const cachedRegion = () => testDb.prepare('SELECT country_code FROM place_regions WHERE place_id = ?').get(place.id); + + await withHarness(user.id, async (h) => { + await h.client.callTool({ name: 'update_place', arguments: { tripId: trip.id, placeId: place.id, name: 'Hotel Adlon' } }); + expect(cachedRegion()).toEqual({ country_code: 'FR' }); + + const moved = await h.client.callTool({ + name: 'update_place', + arguments: { tripId: trip.id, placeId: place.id, lat: 52.5163, lng: 13.3777, address: 'Unter den Linden 77, Berlin, Germany' }, + }); + expect(moved.isError).toBeFalsy(); + expect(cachedRegion()).toBeUndefined(); + }); + }); + it('broadcasts place:updated event', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); diff --git a/server/tests/unit/nest/atlas.service.test.ts b/server/tests/unit/nest/atlas.service.test.ts index 6ccc44b79..784635446 100644 --- a/server/tests/unit/nest/atlas.service.test.ts +++ b/server/tests/unit/nest/atlas.service.test.ts @@ -1155,6 +1155,60 @@ describe('getVisitedRegions', () => { }); }); +// ── A place that moves (#2527) ─────────────────────────────────────────────── + +describe('a place that moves keeps no stale region', () => { + function cachedCountryOf(placeId: number): string | undefined { + const row = testDb.prepare('SELECT country_code FROM place_regions WHERE place_id = ?').get(placeId) as + | { country_code: string } + | undefined; + return row?.country_code; + } + + // The lookup runs in the background; the first one in a worker also loads the admin-1 bundle. + async function settledCountryOf(placeId: number): Promise { + for (let i = 0; i < 150; i++) { + const code = cachedCountryOf(placeId); + if (code) return code; + await new Promise((resolve) => setTimeout(resolve, 20)); + } + return undefined; + } + + it('ATLAS-SVC-2527a: a lookup still running when the place moves does not write the old country back', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Lyon, then Munich' }); + const place = insertPlaceWithCoords(testDb, trip.id, 'Hotel', 45.764, 4.8357, 'Lyon, France'); + + // The Atlas load starts resolving Lyon, and the place is corrected before it answers. + const pending = atlas.visitedRegions(user.id); + testDb + .prepare('UPDATE places SET lat = ?, lng = ?, address = ? WHERE id = ?') + .run(48.1374, 11.5755, 'Marienplatz, Munich, Germany', place.id); + await pending; + await new Promise((resolve) => setTimeout(resolve, 300)); + expect(cachedCountryOf(place.id)).toBeUndefined(); + + // The next load resolves where the place is now. + expect((await atlas.stats(user.id)).countries.map((c) => c.code)).toEqual(['DE']); + await atlas.visitedRegions(user.id); + expect(await settledCountryOf(place.id)).toBe('DE'); + }); + + it('ATLAS-SVC-2527b: a place deleted while its lookup runs leaves nothing behind', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Cancelled' }); + const place = insertPlaceWithCoords(testDb, trip.id, 'Hotel', 43.2965, 5.3698, 'Marseille, France'); + + const pending = atlas.visitedRegions(user.id); + testDb.prepare('DELETE FROM places WHERE id = ?').run(place.id); + await pending; + await new Promise((resolve) => setTimeout(resolve, 300)); + + expect(testDb.prepare('SELECT COUNT(*) AS n FROM place_regions').get()).toEqual({ n: 0 }); + }); +}); + // ── unmarkRegionVisited — tombstones + country cascade ────────────────────── // Places are region-resolved by a fire-and-forget background task (see reverseGeocodeRegion diff --git a/server/tests/unit/nest/place-regions-repair.test.ts b/server/tests/unit/nest/place-regions-repair.test.ts new file mode 100644 index 000000000..e50298330 --- /dev/null +++ b/server/tests/unit/nest/place-regions-repair.test.ts @@ -0,0 +1,216 @@ +import { reverseGeocodeRegion } from '../../../src/nest/atlas/atlas-geo'; +import { AtlasService } from '../../../src/nest/atlas/atlas.service'; +import { PLACE_REGIONS_REPAIR_DONE_KEY, PlaceRegionsRepairJob } from '../../../src/nest/atlas/place-regions-repair.job'; +import { DatabaseService } from '../../../src/nest/database/database.service'; +import type { CronRegistrarService } from '../../../src/nest/scheduling/cron-registrar.service'; +import { createTrip, createUser } from '../../helpers/factories'; +import { createTestDb } from '../../helpers/test-db'; + +import type Database from 'better-sqlite3'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +// An install that upgrades to the #2527 fix still holds the place_regions rows its +// corrected places were cached with before it. The trigger only stops new ones from +// going stale, so a one time pass re-derives the old rows from the bundled borders. + +const BERLIN = { lat: 52.5163, lng: 13.3777, address: 'Unter den Linden 77, 10117 Berlin, Germany' }; +// Kehl sits inside France's bounding box, so a box check alone would call its FR row fine. +const KEHL = { lat: 48.5761, lng: 7.8152, address: 'Hauptstrasse 1, 77694 Kehl, Germany' }; +const PARIS = { lat: 48.8566, lng: 2.3522, address: 'Rue de Rivoli, Paris, France' }; +const LYON = { lat: 45.764, lng: 4.8357, address: 'Place Bellecour, Lyon, France' }; +const MUNICH = { lat: 48.1374, lng: 11.5755, address: 'Marienplatz, Munich, Germany' }; +// The coordinates fall in Germany's simplified polygon, the address puts it in Luxembourg. +const BOLLENDORF_PONT = { + lat: 49.8502458, + lng: 6.3576404, + address: '4 Gruusswiss, Bollendorf-Pont, Distrikt Gréiwemaacher 6555, Luxembourg', +}; +// Points the bundle has no region for: only Nominatim can answer them. +const NICE_HARBOUR = { lat: 43.695, lng: 7.285, address: 'Quai des Etats-Unis, Nice, France' }; +const SYLT_BEACH = { lat: 54.9, lng: 8.29, address: 'Westerland, Sylt, Germany' }; + +type Location = { lat: number | null; lng: number | null; address: string | null }; + +describe('the one time repair of place_regions rows cached before #2527', () => { + let db: Database.Database; + let atlas: AtlasService; + let job: PlaceRegionsRepairJob; + let userId: number; + let tripId: number; + const fetchMock = vi.fn(); + + const addPlace = (name: string, at: Location, cached?: [string, string, string]) => { + const id = db + .prepare('INSERT INTO places (trip_id, name, lat, lng, address) VALUES (?, ?, ?, ?, ?)') + .run(tripId, name, at.lat, at.lng, at.address).lastInsertRowid as number; + if (cached) { + db.prepare( + 'INSERT INTO place_regions (place_id, country_code, region_code, region_name) VALUES (?, ?, ?, ?)', + ).run(id, ...cached); + } + return id; + }; + const cachedRow = (id: number) => + db.prepare('SELECT country_code, region_code FROM place_regions WHERE place_id = ?').get(id) as + | { country_code: string; region_code: string } + | undefined; + const markedDone = () => db.prepare('SELECT 1 FROM app_settings WHERE key = ?').get(PLACE_REGIONS_REPAIR_DONE_KEY); + const registrar = (enabled: boolean) => ({ isEnabled: () => enabled }) as unknown as CronRegistrarService; + + beforeEach(() => { + db = createTestDb(); + const dbService = new DatabaseService(db); + atlas = new AtlasService(dbService); + job = new PlaceRegionsRepairJob(atlas, registrar(true), dbService); + userId = createUser(db).user.id; + tripId = createTrip(db, userId, { title: 'Rhine', start_date: '2025-05-01', end_date: '2025-05-05' }).id; + fetchMock.mockReset(); + vi.stubGlobal('fetch', fetchMock); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + db.close(); + }); + + it('shows an upgraded install the country its corrected places are in now, without editing them again', async () => { + // The shape from the report: moved to Germany on 4.3.2, still cached in France. + const hotel = addPlace('Hotel Adlon', BERLIN, ['FR', 'FR-IDF', 'Ile-de-France']); + const rhine = addPlace('Rhine Hotel', KEHL, ['FR', 'FR-GES', 'Grand Est']); + expect((await atlas.stats(userId)).countries.map((c) => c.code)).toEqual(['FR']); + + await job.runOnce(); + + expect(cachedRow(hotel)).toEqual({ country_code: 'DE', region_code: 'DE-BE' }); + expect(cachedRow(rhine)).toEqual({ country_code: 'DE', region_code: 'DE-BW' }); + expect((await atlas.stats(userId)).countries.map((c) => c.code)).toEqual(['DE']); + expect(Object.keys((await atlas.visitedRegions(userId)).regions)).toEqual(['DE']); + expect(atlas.getTravelStats(userId).countries).toEqual(['DE']); + expect(atlas.lastTrip(userId)?.countries).toEqual(['DE']); + expect(markedDone()).toBeTruthy(); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it('re-derives a region that moved inside the same country', async () => { + const museum = addPlace('Musee des Confluences', LYON, ['FR', 'FR-IDF', 'Ile-de-France']); + + expect(await atlas.repairStaleRegionCache()).toEqual({ replaced: 1, dropped: 0 }); + expect(cachedRow(museum)).toEqual({ country_code: 'FR', region_code: 'FR-ARA' }); + }); + + it('leaves good rows alone, the address fallback and rows only Nominatim could judge included', async () => { + const paris = addPlace('Louvre', PARIS, ['FR', 'FR-IDF', 'Ile-de-France']); + const airbnb = addPlace('Airbnb', BOLLENDORF_PONT, ['LU', 'LU-EC', 'Canton Echternach']); + // The bundle calls this point Italian and has no region for it; the address backs the row. + const harbour = addPlace('Port Lympia', NICE_HARBOUR, ['FR', 'FR-PAC', "Provence-Alpes-Cote d'Azur"]); + const ship = addPlace('Cruise ship', { lat: 0.5, lng: -30, address: null }, ['BR', 'BR-PE', 'Pernambuco']); + + expect(await atlas.repairStaleRegionCache()).toEqual({ replaced: 0, dropped: 0 }); + expect(cachedRow(paris)).toEqual({ country_code: 'FR', region_code: 'FR-IDF' }); + expect(cachedRow(airbnb)).toEqual({ country_code: 'LU', region_code: 'LU-EC' }); + expect(cachedRow(harbour)).toEqual({ country_code: 'FR', region_code: 'FR-PAC' }); + expect(cachedRow(ship)).toEqual({ country_code: 'BR', region_code: 'BR-PE' }); + }); + + it('drops a row the bundle cannot re-derive when coordinates and address both name another country', async () => { + const beach = addPlace('Beach hut', SYLT_BEACH, ['FR', 'FR-BRE', 'Bretagne']); + const cleared = addPlace('Somewhere', { lat: null, lng: null, address: null }, ['FR', 'FR-IDF', 'Ile-de-France']); + + expect(await atlas.repairStaleRegionCache()).toEqual({ replaced: 0, dropped: 2 }); + expect(cachedRow(beach)).toBeUndefined(); + expect(cachedRow(cleared)).toBeUndefined(); + // The dashboard, which reads only the cache, no longer counts France. + expect(atlas.getTravelStats(userId).countries).toEqual([]); + }); + + it('does not write over a place that is edited while the pass runs', async () => { + const hotel = addPlace('Hotel Adlon', BERLIN, ['FR', 'FR-IDF', 'Ile-de-France']); + const beach = addPlace('Beach hut', SYLT_BEACH, ['FR', 'FR-BRE', 'Bretagne']); + + // The pass has read both rows before it gives way for the first time. + const pass = atlas.repairStaleRegionCache(); + db.prepare('UPDATE places SET lat = ?, lng = ?, address = ? WHERE id = ?').run( + MUNICH.lat, + MUNICH.lng, + MUNICH.address, + hotel, + ); + db.prepare("UPDATE places SET address = 'Westerland, Germany' WHERE id = ?").run(beach); + db.prepare( + "INSERT INTO place_regions (place_id, country_code, region_code, region_name) VALUES (?, 'DE', 'DE-SH', 'Schleswig-Holstein')", + ).run(beach); + + expect(await pass).toEqual({ replaced: 0, dropped: 0 }); + // The trigger dropped the moved place's row and nothing wrote Berlin back. + expect(cachedRow(hotel)).toBeUndefined(); + // The row a fresh lookup wrote for the edited place stays. + expect(cachedRow(beach)).toEqual({ country_code: 'DE', region_code: 'DE-SH' }); + }); + + it('runs once, and a second pass over the same rows changes nothing', async () => { + // Enough rows for the pass to give way to other work along the way. + const moved = Array.from({ length: 250 }, (_, i) => + addPlace(`Stop ${i}`, BERLIN, ['FR', 'FR-IDF', 'Ile-de-France']), + ); + + await job.runOnce(); + expect(moved.every((id) => cachedRow(id)?.region_code === 'DE-BE')).toBe(true); + expect(await atlas.repairStaleRegionCache()).toEqual({ replaced: 0, dropped: 0 }); + + // Marked as done, the next start does not read the cache again. + const later = addPlace('Later', BERLIN, ['FR', 'FR-IDF', 'Ile-de-France']); + await job.runOnce(); + expect(cachedRow(later)).toEqual({ country_code: 'FR', region_code: 'FR-IDF' }); + }); + + it('is not marked as done when the pass fails, so the next start tries again', async () => { + const hotel = addPlace('Hotel Adlon', BERLIN, ['FR', 'FR-IDF', 'Ile-de-France']); + vi.spyOn(atlas, 'repairStaleRegionCache').mockRejectedValueOnce(new Error('disk I/O error')); + vi.spyOn(console, 'error').mockImplementation(() => {}); + + await job.runOnce(); + expect(markedDone()).toBeUndefined(); + expect(cachedRow(hotel)?.country_code).toBe('FR'); + + await job.runOnce(); + expect(markedDone()).toBeTruthy(); + expect(cachedRow(hotel)?.country_code).toBe('DE'); + }); + + it('starts from the boot hook, but not where the scheduler is off', () => { + const dbService = new DatabaseService(db); + const offJob = new PlaceRegionsRepairJob(atlas, registrar(false), dbService); + const onJob = new PlaceRegionsRepairJob(atlas, registrar(true), dbService); + const offRun = vi.spyOn(offJob, 'runOnce').mockResolvedValue(); + const onRun = vi.spyOn(onJob, 'runOnce').mockResolvedValue(); + + offJob.onApplicationBootstrap(); + onJob.onApplicationBootstrap(); + + expect(offRun).not.toHaveBeenCalled(); + expect(onRun).toHaveBeenCalledTimes(1); + }); +}); + +describe('the in-memory region cache (#2527)', () => { + afterEach(() => vi.unstubAllGlobals()); + + it('answers again when only the address of a border point changes', async () => { + // Without an address this point falls to Nominatim, which is asked once. + const fetchMock = vi.fn().mockResolvedValue({ + ok: true, + json: async () => ({ address: { country_code: 'de', state: 'Rheinland-Pfalz', 'ISO3166-2-lvl4': 'DE-RP' } }), + }); + vi.stubGlobal('fetch', fetchMock); + + const before = await reverseGeocodeRegion(BOLLENDORF_PONT.lat, BOLLENDORF_PONT.lng, null); + const after = await reverseGeocodeRegion(BOLLENDORF_PONT.lat, BOLLENDORF_PONT.lng, BOLLENDORF_PONT.address); + const again = await reverseGeocodeRegion(BOLLENDORF_PONT.lat, BOLLENDORF_PONT.lng, null); + + expect(before?.country_code).toBe('DE'); + expect(after).toEqual({ country_code: 'LU', region_code: 'LU-EC', region_name: 'Canton Echternach' }); + expect(again?.country_code).toBe('DE'); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); +});