From 09f7cc481171f558d3f68f631751d9cd4ecda48f Mon Sep 17 00:00:00 2001 From: jubnl Date: Wed, 29 Jul 2026 20:27:30 +0200 Subject: [PATCH] fix(maps): repair the quirks preserved by the DI fold --- server/src/nest/maps/maps.mcp.ts | 5 +- server/src/nest/maps/maps.service.ts | 91 +++++++++++-------- .../unit/mcp/tools-tags-maps-weather.test.ts | 21 ++--- server/tests/unit/nest/maps.service.test.ts | 67 +++++++++++++- 4 files changed, 126 insertions(+), 58 deletions(-) diff --git a/server/src/nest/maps/maps.mcp.ts b/server/src/nest/maps/maps.mcp.ts index 34fdf5d95..3afd752c7 100644 --- a/server/src/nest/maps/maps.mcp.ts +++ b/server/src/nest/maps/maps.mcp.ts @@ -1,4 +1,4 @@ -import { McpController, Tool, TOOL_ANNOTATIONS_READONLY, errorResult, ok, type McpContext } from '@trek/nest-mcp'; +import { McpController, Tool, TOOL_ANNOTATIONS_READONLY, ok, type McpContext } from '@trek/nest-mcp'; import { z } from 'zod'; import { MapsService } from './maps.service'; @@ -26,7 +26,6 @@ export class MapsMcp { }) async getPlaceDetails({ placeId, lang }: { placeId: string; lang?: string }, ctx: McpContext) { const details = await this.maps.getPlaceDetails(ctx.userId, placeId, lang ?? 'en'); - if (!details) return errorResult('Place not found or maps service not configured.'); return ok({ details }); } @@ -43,7 +42,6 @@ export class MapsMcp { }) async reverseGeocode({ lat, lng, lang }: { lat: number; lng: number; lang?: string }, _ctx: McpContext) { const result = await this.maps.reverseGeocode(String(lat), String(lng), lang ?? 'en'); - if (!result) return errorResult('Reverse geocode failed or maps service not configured.'); return ok(result); } @@ -58,7 +56,6 @@ export class MapsMcp { }) async resolveMapsUrl({ url }: { url: string }, _ctx: McpContext) { const result = await this.maps.resolveGoogleMapsUrl(url); - if (!result) return errorResult('Could not resolve URL or maps service not configured.'); return ok(result); } } diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index e7cf6e9f5..353137f7c 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -334,19 +334,25 @@ export class MapsService { ); } const data = (await response.json()) as NominatimResult[]; - return data.map((item) => ({ - google_place_id: null, - google_ftid: null, - osm_id: `${item.osm_type}:${item.osm_id}`, - name: item.name || item.display_name?.split(',')[0] || '', - address: item.display_name || '', - lat: parseFloat(item.lat) || null, - lng: parseFloat(item.lon) || null, - rating: null, - website: null, - phone: null, - source: 'openstreetmap', - })); + return data.map((item) => { + // Number.isFinite, not `|| null`: a place on the equator or prime + // meridian has a legitimate 0 coordinate. + const lat = parseFloat(item.lat); + const lng = parseFloat(item.lon); + return { + google_place_id: null, + google_ftid: null, + osm_id: `${item.osm_type}:${item.osm_id}`, + name: item.name || item.display_name?.split(',')[0] || '', + address: item.display_name || '', + lat: Number.isFinite(lat) ? lat : null, + lng: Number.isFinite(lng) ? lng : null, + rating: null, + website: null, + phone: null, + source: 'openstreetmap', + }; + }); } // ── Nominatim lookup (by OSM ID) ─────────────────────────────────────────── @@ -375,11 +381,13 @@ export class MapsService { const data = (await res.json()) as NominatimResult[]; const item = data[0]; if (!item) return null; + const lat = parseFloat(item.lat); + const lng = parseFloat(item.lon); return { name: item.name || item.display_name?.split(',')[0] || '', address: item.display_name || '', - lat: parseFloat(item.lat) || null, - lng: parseFloat(item.lon) || null, + lat: Number.isFinite(lat) ? lat : null, + lng: Number.isFinite(lng) ? lng : null, }; } catch { return null; @@ -631,8 +639,9 @@ export class MapsService { google_ftid: googleFtidFromMapsUrl(p.googleMapsUri), name: p.displayName?.text || '', address: p.formattedAddress || '', - lat: p.location?.latitude || null, - lng: p.location?.longitude || null, + // `?? null`, not `|| null`: 0 is a real coordinate (equator / prime meridian). + lat: p.location?.latitude ?? null, + lng: p.location?.longitude ?? null, rating: p.rating || null, website: p.websiteUri || null, phone: p.nationalPhoneNumber || null, @@ -739,25 +748,27 @@ export class MapsService { const element = await this.fetchOverpassDetails(osmType, osmId); const details = buildOsmDetails(element?.tags || {}, osmType, osmId); - // Fetch Nominatim only when Overpass lacks coordinates or address - const d = details as Record; - const needsNominatim = !d.lat || !d.lng || !d.address; - const nominatim = needsNominatim ? await this.lookupNominatim(osmType, osmId, lang) : null; + // buildOsmDetails never yields name/address/coordinates — Nominatim is + // always the source for those (Overpass contributes the tag-derived rest). + const nominatim = await this.lookupNominatim(osmType, osmId, lang); return { place: { ...details, - name: (d.name as string) || nominatim?.name || element?.tags?.name || '', - address: (d.address as string) || nominatim?.address || '', - lat: d.lat ?? nominatim?.lat ?? null, - lng: d.lng ?? nominatim?.lng ?? null, + name: nominatim?.name || element?.tags?.name || '', + address: nominatim?.address || '', + lat: nominatim?.lat ?? null, + lng: nominatim?.lng ?? null, osm_id: placeId, }, }; } // Google details - const langKey = toApiLang(lang, 'de'); + // 'en' default, aligned with search/autocomplete and the MCP tools' ?? 'en' + // (the 'de' the legacy service defaulted to was a development leftover; + // cache rows keyed 'de' for lang-less callers go cold once — 7-day TTL). + const langKey = toApiLang(lang); const apiKey = this.getMapsKey(userId); if (!apiKey) { throw Object.assign(new Error('Google Maps API key not configured'), { status: 400 }); @@ -798,8 +809,9 @@ export class MapsService { google_ftid: googleFtidFromMapsUrl(data.googleMapsUri), name: data.displayName?.text || '', address: data.formattedAddress || '', - lat: data.location?.latitude || null, - lng: data.location?.longitude || null, + // `?? null`, not `|| null`: 0 is a real coordinate (equator / prime meridian). + lat: data.location?.latitude ?? null, + lng: data.location?.longitude ?? null, rating: data.rating || null, rating_count: data.userRatingCount || null, website: data.websiteUri || null, @@ -848,7 +860,7 @@ export class MapsService { return OSM_PLACE_ID.test(placeId) ? this.getPlaceDetails(userId, placeId, lang) : { place: null }; } - const langKey = toApiLang(lang, 'de'); + const langKey = toApiLang(lang); // 'en' default — see getPlaceDetails const apiKey = this.getMapsKey(userId); if (!apiKey) throw Object.assign(new Error('Google Maps API key not configured'), { status: 400 }); @@ -888,8 +900,9 @@ export class MapsService { google_ftid: googleFtidFromMapsUrl(data.googleMapsUri), name: data.displayName?.text || '', address: data.formattedAddress || '', - lat: data.location?.latitude || null, - lng: data.location?.longitude || null, + // `?? null`, not `|| null`: 0 is a real coordinate (equator / prime meridian). + lat: data.location?.latitude ?? null, + lng: data.location?.longitude ?? null, rating: data.rating || null, rating_count: data.userRatingCount || null, website: data.websiteUri || null, @@ -1167,7 +1180,7 @@ export class MapsService { if (!coords) { try { const pageRes = await followRedirects(resolvedUrl, { - headers: { 'User-Agent': 'TREK-Travel-Planner/1.0' }, + headers: { 'User-Agent': UA }, }); coords = extractCoords(await pageRes.text()); } catch (err) { @@ -1188,16 +1201,16 @@ export class MapsService { } const { lat, lng } = coords; - // Reverse geocode to get address + // Reverse geocode to get address. A non-ok answer (Nominatim 5xx/429) must + // not fail the whole resolution — the coordinates are already extracted, so + // fall back to the URL-derived name and a null address. const nominatimRes = await fetch( `https://nominatim.openstreetmap.org/reverse?lat=${lat}&lon=${lng}&format=json&addressdetails=1`, - { headers: { 'User-Agent': 'TREK-Travel-Planner/1.0' }, signal: AbortSignal.timeout(8000) }, + { headers: { 'User-Agent': UA }, signal: AbortSignal.timeout(8000) }, ); - const nominatim = (await nominatimRes.json()) as { - display_name?: string; - name?: string; - address?: Record; - }; + const nominatim: { display_name?: string; name?: string; address?: Record } = nominatimRes.ok + ? await nominatimRes.json() + : {}; const name = placeName || nominatim.name || nominatim.address?.tourism || nominatim.address?.building || null; const address = nominatim.display_name || null; 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 f840d9e62..d92aa9de8 100644 --- a/server/tests/unit/mcp/tools-tags-maps-weather.test.ts +++ b/server/tests/unit/mcp/tools-tags-maps-weather.test.ts @@ -51,9 +51,10 @@ import { MapsService } from '../../../src/nest/maps/maps.service'; // test registry builds a real MapsService over the mocked db proxy, so stub the // provider methods on the prototype (no auto-restore in the vitest config — // these survive across tests, exactly like the old module mock did). -const getPlaceDetailsSpy = vi - .spyOn(MapsService.prototype, 'getPlaceDetails') - .mockResolvedValue({ name: 'Eiffel Tower', address: 'Paris' } as never); +vi.spyOn(MapsService.prototype, 'getPlaceDetails').mockResolvedValue({ + name: 'Eiffel Tower', + address: 'Paris', +} as never); vi.spyOn(MapsService.prototype, 'reverseGeocode').mockResolvedValue({ name: 'Paris', address: 'France' }); vi.spyOn(MapsService.prototype, 'resolveGoogleMapsUrl').mockResolvedValue({ lat: 48.8566, @@ -311,17 +312,9 @@ describe('Tool: get_place_details', () => { }); }); - it('returns isError when service returns null', async () => { - getPlaceDetailsSpy.mockResolvedValueOnce(null as never); - const { user } = createUser(testDb); - await withHarness(user.id, async (h) => { - const result = await h.client.callTool({ - name: 'get_place_details', - arguments: { placeId: 'nonexistent-place-id' }, - }); - expect(result.isError).toBe(true); - }); - }); + // The former "isError when service returns null" case pinned a dead branch — + // MapsService.getPlaceDetails throws or returns an object, never null — and + // died with the guard in the fix(maps) quirk pass. }); // --------------------------------------------------------------------------- diff --git a/server/tests/unit/nest/maps.service.test.ts b/server/tests/unit/nest/maps.service.test.ts index ae4a6fa1f..77b8300fb 100644 --- a/server/tests/unit/nest/maps.service.test.ts +++ b/server/tests/unit/nest/maps.service.test.ts @@ -1,5 +1,5 @@ /** - * Unit tests for the DI-native MapsService — MAPS-001 through MAPS-106. + * Unit tests for the DI-native MapsService — MAPS-001 through MAPS-110. * Moved 1:1 from the legacy tests/unit/services/mapsService.test.ts when the * maps domain folded into src/nest/maps/ (case IDs preserved; the wrapper * suite's delegation cases died with the delegation itself — the kill-switch, @@ -572,6 +572,19 @@ describe('resolveGoogleMapsUrl coordinate extraction (ReDoS guards)', () => { const result = await svc.resolveGoogleMapsUrl('https://www.google.com/maps/@48.8606,2.3376,15z'); expect(result.name).toBe('Louvre Museum'); }); + + it('MAPS-110: a non-ok reverse-geocode answer degrades to URL-derived values instead of failing', async () => { + // The coordinates are already extracted from the URL — a Nominatim 5xx must + // not turn the resolution into a 400. + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: false, status: 503 })); + const result = await svc.resolveGoogleMapsUrl( + 'https://www.google.com/maps/place/Eiffel+Tower/@48.8584,2.2945,15z', + ); + expect(result.lat).toBeCloseTo(48.8584, 3); + expect(result.lng).toBeCloseTo(2.2945, 3); + expect(result.name).toBe('Eiffel Tower'); + expect(result.address).toBeNull(); + }); }); // ── searchNominatim (fetch-dependent) ──────────────────────────────────────── @@ -622,6 +635,24 @@ describe('searchNominatim (fetch stubbed)', () => { const results = await svc.searchNominatim('London'); expect((results[0] as any).name).toBe('London'); }); + + it('MAPS-108: keeps "0" coordinates and nulls only unparseable ones', async () => { + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue({ + ok: true, + json: async () => [ + { osm_type: 'node', osm_id: '3', lat: '0', lon: '0', name: 'Null Island' }, + { osm_type: 'node', osm_id: '4', lat: 'not-a-number', lon: '', name: 'Broken' }, + ], + }), + ); + const results = await svc.searchNominatim('null island'); + expect((results[0] as any).lat).toBe(0); + expect((results[0] as any).lng).toBe(0); + expect((results[1] as any).lat).toBeNull(); + expect((results[1] as any).lng).toBeNull(); + }); }); // ── fetchOverpassDetails (fetch stubbed) ───────────────────────────────────── @@ -1068,6 +1099,23 @@ describe('searchPlaces (fetch stubbed)', () => { expect(place.website).toBeNull(); expect(place.phone).toBeNull(); }); + + it('MAPS-107: keeps 0 coordinates (equator / prime meridian) instead of nulling them', async () => { + mockDbGet.mockReturnValueOnce({ maps_api_key: 'some-key' }); + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue({ + ok: true, + json: async () => ({ + places: [{ id: 'gid-null-island', displayName: { text: 'Null Island' }, location: { latitude: 0, longitude: 0 } }], + }), + }), + ); + const result = await svc.searchPlaces(1, 'null island'); + const place = result.places[0] as any; + expect(place.lat).toBe(0); + expect(place.lng).toBe(0); + }); }); // ── autocompletePlaces (fetch stubbed) ────────────────────────────────────── @@ -1410,6 +1458,23 @@ describe('getPlaceDetails (fetch stubbed)', () => { expect(String(fetchMock.mock.calls[2][0])).toContain('languageCode=de'); }); + it('MAPS-109: defaults to English when no language is given (the legacy de default was a leftover)', async () => { + const fetchMock = vi.fn().mockResolvedValue({ + ok: true, + json: async () => ({ id: 'ChIJ1', displayName: { text: 'X' } }), + }); + mockDbGet.mockReturnValue({ maps_api_key: 'gkey' }); + vi.stubGlobal('fetch', fetchMock); + + await svc.getPlaceDetails(1, 'ChIJ-nolang'); + expect(String(fetchMock.mock.calls[0][0])).toContain('languageCode=en'); + + // refresh=true skips the expanded-cache read (the blanket mockDbGet return + // would otherwise be mistaken for a cached row). + await svc.getPlaceDetailsExpanded(1, 'ChIJ-nolang-exp', undefined, true); + expect(String(fetchMock.mock.calls[1][0])).toContain('languageCode=en'); + }); + it('MAPS-041c: throws with status when Google API returns non-ok response', async () => { mockDbGet.mockReturnValueOnce({ maps_api_key: 'gkey' }); vi.stubGlobal(