k1nq/TREK
1
0
Fork 0
mirror of https://github.com/mauriceboe/TREK.git synced 2026-10-18 21:16:47 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(maps): repair the quirks preserved by the DI fold

Browse Source
This commit is contained in:
jubnl
2026-07-29 20:27:30 +02:00
parent 74e1e444c5
commit 09f7cc4811
4 changed files with 126 additions and 58 deletions
Download Patch File Download Diff File Expand all files Collapse all files
server/src/nest/maps/maps.mcp.ts
+1 -4
View File
@@ -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);
}
}
server/src/nest/maps/maps.service.ts
+52 -39
View File
@@ -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<string, unknown>;
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<string, string>;
};
const nominatim: { display_name?: string; name?: string; address?: Record<string, string> } = nominatimRes.ok
? await nominatimRes.json()
: {};
const name = placeName || nominatim.name || nominatim.address?.tourism || nominatim.address?.building || null;
const address = nominatim.display_name || null;
server/tests/unit/mcp/tools-tags-maps-weather.test.ts
+7 -14
View File
@@ -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.
});
// ---------------------------------------------------------------------------
server/tests/unit/nest/maps.service.test.ts
+66 -1
View File
@@ -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(