diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index 2ffb9818d..216f08ccf 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -651,18 +651,13 @@ export class MapsService { const lng = (bbox.west + bbox.east) / 2; // Half the diagonal, so the circle covers the viewport corners rather // than leaving the edges of the map empty. - const radius = Math.min( - 20000, - Math.max( - 300, - Math.round( - (Math.hypot( - (bbox.north - bbox.south) * 111_320, - (bbox.east - bbox.west) * 111_320 * Math.cos((lat * Math.PI) / 180), - ) / 2), - ), - ), + const wanted = Math.round( + Math.hypot( + (bbox.north - bbox.south) * 111_320, + (bbox.east - bbox.west) * 111_320 * Math.cos((lat * Math.PI) / 180), + ) / 2, ); + const radius = Math.min(20000, Math.max(300, wanted)); const found = await trekPlacesNearby(lat, lng, { radius, limit: 50, @@ -685,13 +680,17 @@ export class MapsService { address: p.address?.freeform ?? null, website: p.contact?.website ?? null, phone: p.contact?.phone ?? null, - opening_hours: null, + opening_hours: p.hours?.osm ?? null, + // The index has no cuisine field, so this null is the truth + // rather than a field being dropped on the way through. cuisine: null, source: 'openstreetmap' as const, })), source: 'openstreetmap' as const, truncated: found.length >= 50, - clamped: false, + // A wide viewport is narrowed here too, and the caller is told so + // for the same reason the Overpass path tells it. + clamped: radius < wanted, }; } } catch (err: unknown) { diff --git a/server/tests/unit/mcp/tools-tags-maps-weather.test.ts b/server/tests/unit/mcp/tools-tags-maps-weather.test.ts index de08dfa8b..280cf0eba 100644 --- a/server/tests/unit/mcp/tools-tags-maps-weather.test.ts +++ b/server/tests/unit/mcp/tools-tags-maps-weather.test.ts @@ -40,6 +40,22 @@ vi.mock('../../../src/nest/weather/weather.impl', () => ({ getDetailedWeather: vi.fn().mockResolvedValue({ hourly: [] }), })); +// MapsService.pois() asks the TREK Places index before Overpass, and the real +// client would send that request from a unit test. The gate above it is left +// alone on purpose: it fails open, so these cases run on the shipping default +// with the index answering nothing, which is the drop-through they are about. +const { trekNearbyMock } = vi.hoisted(() => ({ + trekNearbyMock: vi.fn(async ( + _lat: number, + _lng: number, + _opts?: { radius?: number; limit?: number; category?: string }, + ): Promise => []), +})); +vi.mock('../../../src/nest/maps/trek-places.client', async (importOriginal) => ({ + ...(await importOriginal()), + trekPlacesNearby: trekNearbyMock, +})); + import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; @@ -413,9 +429,13 @@ describe('Tool: get_place_details', () => { describe('Tool: search_pois', () => { const BBOX = { south: 48.85, west: 2.33, north: 48.87, east: 2.36 }; + // The index is asked first and answers nothing here, so this case also pins + // the drop-through: an empty index page must reach Overpass rather than come + // back as an empty POI list. it('returns the POIs of a category inside the bbox', async () => { const { user } = createUser(testDb); vi.mocked(MapsService.prototype.searchOverpassPois).mockClear(); + trekNearbyMock.mockClear(); await withHarness(user.id, async (h) => { const result = await h.client.callTool({ @@ -426,13 +446,85 @@ describe('Tool: search_pois', () => { expect(data.pois).toHaveLength(1); expect(data.pois[0].name).toBe('Chez Nous'); expect(data.source).toBe('openstreetmap'); + expect(trekNearbyMock).toHaveBeenCalled(); expect(MapsService.prototype.searchOverpassPois).toHaveBeenCalledWith('restaurant', BBOX, 'fr'); }); }); + // Overpass is the fallback now, not the first stop. `source` stays + // 'openstreetmap' because the client switches on it and these are the same + // places; where they were read is this method's business, not the pill's. + it('answers from the TREK Places index and leaves Overpass alone', async () => { + const { user } = createUser(testDb); + vi.mocked(MapsService.prototype.searchOverpassPois).mockClear(); + trekNearbyMock.mockClear(); + trekNearbyMock.mockResolvedValueOnce([ + { + gers: 'abc-123', + name: 'Le Comptoir', + lat: 48.861, + lng: 2.341, + category: 'restaurant', + address: { freeform: '9 Rue de Rivoli' }, + contact: { website: 'https://example.test/', phone: null }, + hours: { osm: 'Mo-Fr 09:00-17:00', source: null, sourceUrl: null }, + }, + ]); + + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ + name: 'search_pois', + arguments: { category: 'restaurant', bbox: BBOX }, + }); + const data = parseToolResult(result) as any; + expect(data.pois).toHaveLength(1); + expect(data.pois[0].osm_id).toBe('gers:abc-123'); + expect(data.pois[0].name).toBe('Le Comptoir'); + // The index carries opening hours; dropping them would leave the tool + // description promising something the answer never has. + expect(data.pois[0].opening_hours).toBe('Mo-Fr 09:00-17:00'); + expect(data.source).toBe('openstreetmap'); + expect(data.clamped).toBe(false); + expect(MapsService.prototype.searchOverpassPois).not.toHaveBeenCalled(); + + // The bbox becomes a centre plus half its diagonal, and the category + // becomes the Overture terms it maps to. + const [lat, lng, opts] = trekNearbyMock.mock.calls[0]; + expect(lat).toBeCloseTo(48.86, 5); + expect(lng).toBeCloseTo(2.345, 5); + expect(opts?.category).toBe('restaurant,casual_eatery,fast_food'); + expect(opts?.limit).toBe(50); + expect(opts?.radius).toBeGreaterThan(300); + expect(opts?.radius).toBeLessThanOrEqual(20000); + }); + }); + + // The index searches a disc, so a viewport wider than 40km is narrowed there + // as well — and says so, the same way the Overpass path does. + it('reports a viewport the index had to narrow as clamped', async () => { + const { user } = createUser(testDb); + trekNearbyMock.mockClear(); + trekNearbyMock.mockResolvedValueOnce([ + { gers: 'wide-1', name: 'Far Away', lat: 48.9, lng: 2.4, category: null, address: null, contact: null, hours: null }, + ]); + + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ + name: 'search_pois', + arguments: { category: 'restaurant', bbox: { south: 48.4, west: 2.0, north: 48.9, east: 2.5 } }, + }); + const data = parseToolResult(result) as any; + expect(data.clamped).toBe(true); + expect(data.pois[0].poi_type).toBe('restaurant'); + expect(data.pois[0].address).toBeNull(); + expect(trekNearbyMock.mock.calls[0][2]?.radius).toBe(20000); + }); + }); + it('passes no language through when none is given', async () => { const { user } = createUser(testDb); vi.mocked(MapsService.prototype.searchOverpassPois).mockClear(); + trekNearbyMock.mockClear(); await withHarness(user.id, async (h) => { await h.client.callTool({ name: 'search_pois', arguments: { category: 'museum', bbox: BBOX } }); @@ -480,6 +572,23 @@ describe('Tool: search_pois', () => { expect((result as { content: { text: string }[] }).content[0].text).toBe('POI search failed.'); }); }); + + // Dropping through must not turn an outage into an empty answer: the index + // failure is logged and Overpass failing under it is still what reaches the + // caller. + it('answers isError when the index fails and every Overpass mirror is unreachable', async () => { + const { user } = createUser(testDb); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + trekNearbyMock.mockRejectedValueOnce(new Error('index down')); + vi.mocked(MapsService.prototype.searchOverpassPois).mockRejectedValueOnce(new Error('all mirrors failed')); + + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ name: 'search_pois', arguments: { category: 'bar', bbox: BBOX } }); + expect(result.isError).toBe(true); + expect((result as { content: { text: string }[] }).content[0].text).toBe('POI search failed.'); + }); + warn.mockRestore(); + }); }); // --------------------------------------------------------------------------- diff --git a/server/tests/unit/nest/maps.pois.test.ts b/server/tests/unit/nest/maps.pois.test.ts new file mode 100644 index 000000000..7456b5f5d --- /dev/null +++ b/server/tests/unit/nest/maps.pois.test.ts @@ -0,0 +1,322 @@ +/** + * MAPS-POIS-001..012 — the explore pill, answered from the index. + * + * The pill used to be Overpass and nothing else, and the public mirrors it asks + * are regularly overloaded. The index answers first now, so the two things that + * matter here are that a hit is mapped onto the exact record the map renderer + * already reads, and that everything which is not a hit still lands on Overpass + * unchanged. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const { mockNearby } = vi.hoisted(() => ({ + mockNearby: vi.fn( + async ( + _lat: number, + _lng: number, + _opts?: { radius?: number; limit?: number; category?: string }, + ): Promise => [], + ), +})); +vi.mock('../../../src/nest/maps/trek-places.client', async (importOriginal) => ({ + ...(await importOriginal()), + trekPlacesNearby: mockNearby, +})); + +vi.mock('../../../src/config', () => ({ JWT_SECRET: 'test-secret', ENCRYPTION_KEY: '0'.repeat(64) })); + +import { MapsService } from '../../../src/nest/maps/maps.service'; +import type { DatabaseService } from '../../../src/nest/database/database.service'; +import type { PlacePhotoCacheService } from '../../../src/nest/place-photos/place-photo-cache.service'; + +// 0.1 degrees a side, centred on the equator: cos(lat) is 1 there, so the radius +// the service derives is exactly half the box's diagonal and the numbers below +// are readable rather than incidental. +const BOX = { south: -0.05, west: -0.05, north: 0.05, east: 0.05 }; + +// A row carrying everything the index can hold. +const FULL = { + gers: 'a-1', + name: 'Cafe Central', + lat: 48.2101, + lng: 16.3657, + category: 'coffee_shop', + categoryPath: 'eat_and_drink>cafe>coffee_shop', + confidence: 0.94, + address: { freeform: 'Herrengasse 14', locality: 'Wien', postcode: '1010', region: null, country: 'AT' }, + contact: { website: 'https://cafecentral.wien/', phone: '+43 1 5333763', email: null, socials: null }, + brand: null, + source: 'overture', + hours: { osm: 'Mo-Sa 07:30-22:00', source: 'schema.org', sourceUrl: 'https://cafecentral.wien/' }, + distanceMetres: 120, +}; + +// A row carrying none of it — the index holds plenty of these. +const BARE = { + gers: 'b-2', + name: 'Kiosk am Ring', + lat: 48.2, + lng: 16.37, + category: null, + categoryPath: null, + confidence: null, + address: null, + contact: null, + brand: null, + source: 'overture', + hours: null, + distanceMetres: 480, +}; + +// What the Overpass half answers when the index does not. Distinct enough that +// "dropped through" is visible in the returned value, not only in the spy. +const OVERPASS_ANSWER = { + pois: [ + { + osm_id: 'node:1', + name: 'From Overpass', + lat: 1, + lng: 2, + category: 'cafe', + poi_type: 'amenity=cafe', + address: null, + website: null, + phone: null, + opening_hours: null, + cuisine: null, + source: 'openstreetmap' as const, + }, + ], + source: 'openstreetmap' as const, + truncated: false, + clamped: false, +}; + +function make(enabled = true) { + const database = { + get: vi.fn(() => (enabled ? undefined : { value: 'false' })), + } as unknown as DatabaseService; + return new MapsService(database, {} as PlacePhotoCacheService); +} + +// Overpass is the one network call this file must never make; stubbing it is +// also what turns "dropped through" into something a case can assert. +function stubOverpass(svc: MapsService) { + return vi.spyOn(svc, 'searchOverpassPois').mockResolvedValue(OVERPASS_ANSWER); +} + +function rows(n: number) { + return Array.from({ length: n }, (_, i) => ({ ...BARE, gers: `c-${i}`, name: `Kiosk ${i}` })); +} + +// Module-level rather than per-case: MAPS-POIS-012 asserts what gets logged. +const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + +beforeEach(() => { + mockNearby.mockReset(); + mockNearby.mockResolvedValue([]); + warn.mockClear(); +}); + +describe('MapsService.pois answered from the index', () => { + it('MAPS-POIS-001: maps a full index row onto the record the map already reads', async () => { + mockNearby.mockResolvedValue([FULL]); + const svc = make(); + const overpass = stubOverpass(svc); + + const out = await svc.pois('cafe', BOX); + + expect(overpass).not.toHaveBeenCalled(); + expect(out.pois).toEqual([ + { + osm_id: 'gers:a-1', + name: 'Cafe Central', + lat: 48.2101, + lng: 16.3657, + // The pill the caller asked for, not the row's own category: the client + // switches on this field to colour and group the markers. + category: 'cafe', + poi_type: 'coffee_shop', + address: 'Herrengasse 14', + website: 'https://cafecentral.wien/', + phone: '+43 1 5333763', + opening_hours: 'Mo-Sa 07:30-22:00', + cuisine: null, + source: 'openstreetmap', + }, + ]); + // 'trek-places' would be a third value for a field the client treats as a + // closed set, and these are the same places either way. + expect(out.source).toBe('openstreetmap'); + }); + + it('MAPS-POIS-002: a row without address, contact or hours becomes nulls, typed as the pill', async () => { + mockNearby.mockResolvedValue([BARE]); + const svc = make(); + stubOverpass(svc); + + const out = await svc.pois('cafe', BOX); + + expect(out.pois[0]).toEqual({ + osm_id: 'gers:b-2', + name: 'Kiosk am Ring', + lat: 48.2, + lng: 16.37, + category: 'cafe', + poi_type: 'cafe', + address: null, + website: null, + phone: null, + opening_hours: null, + cuisine: null, + source: 'openstreetmap', + }); + }); + + it('MAPS-POIS-003: an address and contact object of nulls still produces null, never undefined', async () => { + // JSON.stringify drops an undefined value, so the client would see the key + // missing rather than empty — different from every other row it gets. + mockNearby.mockResolvedValue([ + { + ...FULL, + address: { freeform: null, locality: 'Wien', postcode: null, region: null, country: 'AT' }, + contact: { website: null, phone: null, email: null, socials: null }, + }, + ]); + const svc = make(); + const overpass = stubOverpass(svc); + + const out = await svc.pois('cafe', BOX); + + // The whole record rather than the three nulls: the Overpass answer carries + // those same three, so a subset would still pass with the row dropped. + expect(overpass).not.toHaveBeenCalled(); + expect(out.pois[0]).toEqual({ + osm_id: 'gers:a-1', + name: 'Cafe Central', + lat: 48.2101, + lng: 16.3657, + category: 'cafe', + poi_type: 'coffee_shop', + address: null, + website: null, + phone: null, + opening_hours: 'Mo-Sa 07:30-22:00', + cuisine: null, + source: 'openstreetmap', + }); + }); + + it('MAPS-POIS-004: asks the index for the categories behind the pill, around the viewport centre', async () => { + mockNearby.mockResolvedValue([FULL]); + const svc = make(); + const overpass = stubOverpass(svc); + + const out = await svc.pois('cafe', BOX); + + expect(overpass).not.toHaveBeenCalled(); + // 7872 m is half the diagonal of a 0.1 x 0.1 degree box on the equator, so + // a place in a corner of the viewport is still inside the circle. + expect(mockNearby).toHaveBeenCalledWith(0, 0, { + radius: 7872, + limit: 50, + category: 'cafe,coffee_shop', + }); + expect(out.clamped).toBe(false); + }); + + it('MAPS-POIS-005: a full page of 50 is reported as truncated', async () => { + mockNearby.mockResolvedValue(rows(50)); + const svc = make(); + stubOverpass(svc); + + const out = await svc.pois('cafe', BOX); + + expect(out.pois).toHaveLength(50); + // The page asked for is the page compared against — a caller told nothing + // was cut off will not offer to zoom in for the rest. + expect(out.truncated).toBe(true); + }); + + it('MAPS-POIS-006: a page short of the limit is the whole answer', async () => { + mockNearby.mockResolvedValue(rows(49)); + const svc = make(); + const overpass = stubOverpass(svc); + + const out = await svc.pois('cafe', BOX); + + // The Overpass answer is untruncated as well, so the count and the untouched + // spy are what tell an index answer from a fallback. + expect(overpass).not.toHaveBeenCalled(); + expect(out.pois).toHaveLength(49); + expect(out.truncated).toBe(false); + }); + + it('MAPS-POIS-007: a viewport wider than the 20 km cap is narrowed, and says so', async () => { + mockNearby.mockResolvedValue([FULL]); + const svc = make(); + stubOverpass(svc); + + const out = await svc.pois('cafe', { south: -5, west: -5, north: 5, east: 5 }); + + expect(mockNearby).toHaveBeenCalledWith(0, 0, expect.objectContaining({ radius: 20000 })); + // Half that diagonal is ~787 km: the answer covers the middle of the view + // only, and a caller told otherwise renders the empty rim as "no places". + expect(out.clamped).toBe(true); + }); + + it('MAPS-POIS-008: a viewport under the 300 m floor is widened, which is not clamping', async () => { + mockNearby.mockResolvedValue([FULL]); + const svc = make(); + const overpass = stubOverpass(svc); + + const out = await svc.pois('cafe', { south: -0.0005, west: -0.0005, north: 0.0005, east: 0.0005 }); + + expect(overpass).not.toHaveBeenCalled(); + expect(mockNearby).toHaveBeenCalledWith(0, 0, expect.objectContaining({ radius: 300 })); + // Widened, so nothing in view is missing — the opposite of clamped. + expect(out.clamped).toBe(false); + }); +}); + +describe('MapsService.pois falls back to Overpass', () => { + it('MAPS-POIS-009: a pill the index has no categories for never reaches the index', async () => { + const svc = make(); + + // Deliberately unstubbed: an unknown pill has to keep failing in the + // Overpass path exactly as it did before the index was put in front of it. + await expect(svc.pois('pharmacy', BOX)).rejects.toMatchObject({ + message: 'Unknown POI category', + status: 400, + }); + expect(mockNearby).not.toHaveBeenCalled(); + }); + + it('MAPS-POIS-010: the admin switching the index off skips it entirely', async () => { + const svc = make(false); + const overpass = stubOverpass(svc); + + expect(await svc.pois('cafe', BOX, 'de')).toEqual(OVERPASS_ANSWER); + expect(mockNearby).not.toHaveBeenCalled(); + expect(overpass).toHaveBeenCalledWith('cafe', BOX, 'de'); + }); + + it('MAPS-POIS-011: an empty index page is a miss, not an empty answer', async () => { + mockNearby.mockResolvedValue([]); + const svc = make(); + const overpass = stubOverpass(svc); + + expect(await svc.pois('cafe', BOX, 'de')).toEqual(OVERPASS_ANSWER); + expect(overpass).toHaveBeenCalledWith('cafe', BOX, 'de'); + }); + + it('MAPS-POIS-012: an index failure is logged and dropped through, never surfaced', async () => { + mockNearby.mockRejectedValue(new Error('places api down')); + const svc = make(); + const overpass = stubOverpass(svc); + + expect(await svc.pois('cafe', BOX, 'de')).toEqual(OVERPASS_ANSWER); + expect(overpass).toHaveBeenCalledWith('cafe', BOX, 'de'); + expect(warn).toHaveBeenCalledWith('TREK Places nearby failed, falling back:', 'places api down'); + }); +}); diff --git a/server/tests/unit/nest/maps.service.test.ts b/server/tests/unit/nest/maps.service.test.ts index 209905b82..394af1af4 100644 --- a/server/tests/unit/nest/maps.service.test.ts +++ b/server/tests/unit/nest/maps.service.test.ts @@ -120,6 +120,18 @@ vi.mock('../../../src/config', () => ({ ENCRYPTION_KEY: '0'.repeat(64), })); +// pois() asks the index before Overpass and trekPlacesEnabled fails open, so a +// case that reaches it with no fetch stub in place leaves the runner for +// places.liketrek.com. Only the one export pois() calls is replaced; the rest of +// the client stays real. The index path itself is covered in maps.pois.test.ts. +const { mockNearby } = vi.hoisted(() => ({ + mockNearby: vi.fn(async (): Promise => []), +})); +vi.mock('../../../src/nest/maps/trek-places.client', async (importOriginal) => ({ + ...(await importOriginal()), + trekPlacesNearby: mockNearby, +})); + // Injected stub since the photo-cache fold (was a path mock of the module). // Same seven seams, same mock functions behind them. const photoCacheStub = { @@ -2583,6 +2595,8 @@ describe('controller-facing wrappers delegate to the folded methods', () => { expect(spies.resolveGoogleMapsUrl).toHaveBeenCalledWith('https://maps.app.goo.gl/x'); const bbox = { south: 1, west: 2, north: 3, east: 4 }; + // The index answers with an empty page (see the mock above), so what this + // measures is the wrapper's own delegation. await svc.pois('cafe', bbox, 'de'); expect(spies.searchOverpassPois).toHaveBeenCalledWith('cafe', bbox, 'de'); } finally {