mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-23 23:46:35 +00:00
chore: move place backend to proper DI
Fold the 1029-line services/placeService.ts into the DI-native PlacesService: the CRUD + ratings SQL, the GPX/KML/KMZ importers and the Google/Naver list importers become methods over the injected DatabaseService, byte-identical statements, COALESCE semantics and the If-Match conflict protocol (#1135). The pure half — frozen XML parsers, the KMZ unpacker, the dedup predicates, the Google hex-id parsers, reclaimPhotoCache — moves to places.helpers.ts (maps.helpers precedent). The 10-tool mcp/tools/places.ts registrar plus the trek://trips/{tripId}/places resource move onto the decorator registry as places.mcp.ts. search_place travels with them because its gate is places:read, not the read-only geo group, and now injects MapsService. TripsService, DaysMcp, BookingImportService and the plugin RPC host (PluginHostDepsFactory's 21st constructor dep) all inject PlacesService, so the domain needs no bridge at all. The two assignments.bridge imports that places.mcp.ts keeps are a deliberate cycle break: AssignmentsModule imports DaysModule and DaysModule now imports PlacesModule for days.mcp.ts's place creation, so injecting AssignmentsService there would close DaysModule -> PlacesModule -> AssignmentsModule -> DaysModule. Same seam reservations.mcp.ts uses, same trade trips.bridge.ts documents. Also folds services/placeEnrichment.ts in — its DB/websocket/Maps half becomes PlacesService methods over the injected DatabaseService/RealtimeService/ MapsService, its pure match selector joins places.helpers.ts — which retires nest/maps/maps.bridge.ts with its last consumer. Adopts places.dto.ts for the seven grandfathered body contracts and clears every PlacesController entry from body-contract-allow-list.ts. Two consequences worth knowing: the bespoke 400s 'Place name is required', 'ids must be an array of numbers' and 'URL is required' are now the ZodValidationPipe envelope, and because a pipe runs before the handler a malformed body 400s ahead of the trip-access 404 it used to follow — the same trade the todo and trips migrations took. placeBulkUpdateRequestSchema.ids drops .min(1) so the endpoint's empty-list short-circuit stays reachable. Fixes four defects the relocation had faithfully carried, each with a failing-first regression test: - create dropped lat/lng of exactly 0, so a place on the equator or the prime meridian lost its coordinates (`x || null` cannot tell absent from zero). - duration_minutes: 0 was unsettable — replaced by the 60-minute default on create, and read as "absent" by COALESCE on update. - the journey delete hook fired on unscoped ids: DELETE /:id, POST /bulk-delete and the delete_place MCP tool could detach another trip's journey entries for an id they then refused. All three scope first via the new PlacesService.scopedIds, mirroring the guard PluginHostDepsFactory already had. bulk_delete_places also moves its hook ahead of the DELETE — journey_entries.source_place_id is ON DELETE SET NULL, so running it afterwards left the entries as orphans. - the place search treated % and _ as LIKE wildcards; a bare % returned the whole trip. The three clauses now carry ESCAPE with the term escaped. Plus hardening on the Google list fetch: the response is capped at 8 MB (declared content-length and post-read length, the transit.service precedent) and its JSON.parse is guarded, so a malformed provider payload produces the existing 'Invalid list data received from Google Maps' 400 instead of throwing. Quirks deliberately preserved: the non-COALESCE route_color (#776), currency/transport_mode staying unclearable, importGpx returning null for a file that parses but yields no usable geometry, update skipping the If-Match check when the stored updated_at is null, and every string-valued `x || null`. Tests move with their case IDs: placeService.test.ts and the wrapper suite merge into unit/nest/places.service.test.ts, kmzUnpack into places.helpers.test.ts, placeEnrichment into places.enrichment.test.ts. The places e2e converts to the DI-native pattern (real SQL against temp-db DDL), tools-places.test.ts is kept and extended so it exercises the decorator path through the test registry, and the plugin-host and booking-import path mocks become constructor stubs.
This commit is contained in:
jubnl
@@ -1,168 +0,0 @@
|
||||
import { db, getPlaceWithTags } from '../db/database';
|
||||
import { broadcast } from '../websocket';
|
||||
import { getMapsKey, searchPlaces, getPlacePhoto } from '../nest/maps/maps.bridge';
|
||||
|
||||
/**
|
||||
* Background enrichment for list-imported places (#886).
|
||||
*
|
||||
* Google/Naver list imports only carry name + coordinates, so the imported
|
||||
* places open as bare pins (the Maps tab jumps to coordinates, no photo, no
|
||||
* open/closed). When the importer opts in and a Google Maps key is configured,
|
||||
* we re-resolve each place by name — biased to and validated against the
|
||||
* imported coordinates — to a real Google place, then fill in the empty fields
|
||||
* and persist the resolved `google_place_id` plus `google_ftid` (which power
|
||||
* on-demand opening hours and proper Maps links going forward).
|
||||
*
|
||||
* This runs detached from the import request (fire-and-forget) so a long list
|
||||
* never blocks the response, and pushes each enriched row over the websocket so
|
||||
* the sidebar fills in progressively. It only ever fills EMPTY columns, so it
|
||||
* can never clobber data the import already captured (e.g. a Naver address).
|
||||
*/
|
||||
|
||||
/** A place the import produced — only the fields enrichment reads/writes. */
|
||||
export interface EnrichablePlace {
|
||||
id: number;
|
||||
name: string;
|
||||
lat: number;
|
||||
lng: number;
|
||||
google_place_id?: string | null;
|
||||
google_ftid?: string | null;
|
||||
address?: string | null;
|
||||
website?: string | null;
|
||||
phone?: string | null;
|
||||
image_url?: string | null;
|
||||
}
|
||||
|
||||
/** How close a search hit must be to the imported coordinates to be trusted. */
|
||||
const MATCH_RADIUS_METERS = 250;
|
||||
/** Bias the text search to roughly the imported area. */
|
||||
const SEARCH_BIAS_RADIUS_METERS = 2000;
|
||||
/** Concurrent enrichment lookups — small, to stay friendly to the Maps quota. */
|
||||
const ENRICH_CONCURRENCY = 3;
|
||||
|
||||
function haversineMeters(a: { lat: number; lng: number }, b: { lat: number; lng: number }): number {
|
||||
const R = 6371000;
|
||||
const toRad = (d: number) => (d * Math.PI) / 180;
|
||||
const dLat = toRad(b.lat - a.lat);
|
||||
const dLng = toRad(b.lng - a.lng);
|
||||
const lat1 = toRad(a.lat);
|
||||
const lat2 = toRad(b.lat);
|
||||
const h = Math.sin(dLat / 2) ** 2 + Math.cos(lat1) * Math.cos(lat2) * Math.sin(dLng / 2) ** 2;
|
||||
return 2 * R * Math.asin(Math.sqrt(h));
|
||||
}
|
||||
|
||||
/**
|
||||
* Pick the search result that is the same place as the import: it must be a
|
||||
* Google result (have a google_place_id) with coordinates within
|
||||
* MATCH_RADIUS_METERS of the imported point. Returns the closest such hit, or
|
||||
* null when nothing is close enough — in which case the place is left as
|
||||
* imported rather than risking a wrong-place overwrite (common-name / romanized
|
||||
* lists). Exported for unit testing.
|
||||
*/
|
||||
export function pickEnrichmentMatch(
|
||||
candidates: Record<string, unknown>[],
|
||||
target: { lat: number; lng: number },
|
||||
maxMeters: number = MATCH_RADIUS_METERS,
|
||||
): Record<string, unknown> | null {
|
||||
let best: { c: Record<string, unknown>; dist: number } | null = null;
|
||||
for (const c of candidates || []) {
|
||||
const gpid = c.google_place_id;
|
||||
const lat = c.lat;
|
||||
const lng = c.lng;
|
||||
if (typeof gpid !== 'string' || !gpid) continue;
|
||||
if (typeof lat !== 'number' || typeof lng !== 'number') continue;
|
||||
const dist = haversineMeters(target, { lat, lng });
|
||||
if (dist > maxMeters) continue;
|
||||
if (!best || dist < best.dist) best = { c, dist };
|
||||
}
|
||||
return best?.c ?? null;
|
||||
}
|
||||
|
||||
async function mapWithConcurrency<T>(items: T[], limit: number, fn: (item: T) => Promise<void>): Promise<void> {
|
||||
let cursor = 0;
|
||||
const workers = Array.from({ length: Math.min(limit, items.length) }, async () => {
|
||||
while (cursor < items.length) {
|
||||
const item = items[cursor++];
|
||||
await fn(item);
|
||||
}
|
||||
});
|
||||
await Promise.all(workers);
|
||||
}
|
||||
|
||||
const str = (v: unknown): string | null => (typeof v === 'string' && v.trim() ? v.trim() : null);
|
||||
|
||||
async function enrichOne(tripId: string, userId: number, place: EnrichablePlace, lang?: string): Promise<void> {
|
||||
// Already linked (shouldn't happen for list imports) — nothing to resolve.
|
||||
if (place.google_place_id) return;
|
||||
if (typeof place.lat !== 'number' || typeof place.lng !== 'number') return;
|
||||
|
||||
const { places: results } = await searchPlaces(userId, place.name, lang, {
|
||||
lat: place.lat,
|
||||
lng: place.lng,
|
||||
radius: SEARCH_BIAS_RADIUS_METERS,
|
||||
});
|
||||
const match = pickEnrichmentMatch(results, { lat: place.lat, lng: place.lng });
|
||||
if (!match) return;
|
||||
|
||||
const gpid = str(match.google_place_id);
|
||||
if (!gpid) return;
|
||||
const gftid = str(match.google_ftid);
|
||||
|
||||
// COALESCE so enrichment only fills empty columns — never overwrites data the
|
||||
// import already captured (e.g. Naver's address) or anything the user edited.
|
||||
db.prepare(
|
||||
`UPDATE places
|
||||
SET google_place_id = COALESCE(google_place_id, ?),
|
||||
google_ftid = COALESCE(google_ftid, ?),
|
||||
address = COALESCE(address, ?),
|
||||
website = COALESCE(website, ?),
|
||||
phone = COALESCE(phone, ?),
|
||||
updated_at = CURRENT_TIMESTAMP
|
||||
WHERE id = ? AND trip_id = ?`,
|
||||
).run(gpid, gftid, str(match.address), str(match.website), str(match.phone), place.id, tripId);
|
||||
|
||||
// Photo is best-effort: Google often has none, in which case getPlacePhoto
|
||||
// resolves with photoUrl: null. A missing photo (or a provider outage, which
|
||||
// still throws) must never abort the rest of the enrichment.
|
||||
try {
|
||||
const photo = await getPlacePhoto(userId, gpid, place.lat, place.lng, place.name);
|
||||
if (photo?.photoUrl) {
|
||||
db.prepare(
|
||||
'UPDATE places SET image_url = COALESCE(image_url, ?), updated_at = CURRENT_TIMESTAMP WHERE id = ? AND trip_id = ?',
|
||||
).run(photo.photoUrl, place.id, tripId);
|
||||
}
|
||||
} catch {
|
||||
/* no photo — leave image_url as-is */
|
||||
}
|
||||
|
||||
// Push the enriched row to every connected client (no socket exclusion: the
|
||||
// importer's own client should also receive the late update).
|
||||
const updated = getPlaceWithTags(place.id);
|
||||
if (updated) broadcast(tripId, 'place:updated', { place: updated }, undefined);
|
||||
}
|
||||
|
||||
/**
|
||||
* Enrich a batch of just-imported places in the background. Never throws —
|
||||
* any per-place failure is swallowed so one bad lookup can't take down the
|
||||
* detached task or the process. No-ops when no Google Maps key is configured.
|
||||
*/
|
||||
export async function enrichImportedPlaces(
|
||||
tripId: string,
|
||||
userId: number,
|
||||
places: EnrichablePlace[],
|
||||
lang?: string,
|
||||
): Promise<void> {
|
||||
try {
|
||||
if (!places.length) return;
|
||||
if (!getMapsKey(userId)) return;
|
||||
await mapWithConcurrency(places, ENRICH_CONCURRENCY, async (place) => {
|
||||
try {
|
||||
await enrichOne(tripId, userId, place, lang);
|
||||
} catch (err) {
|
||||
console.error(`[Places] enrichment failed for place ${place.id}:`, err instanceof Error ? err.message : err);
|
||||
}
|
||||
});
|
||||
} catch (err) {
|
||||
console.error('[Places] import enrichment pass failed:', err instanceof Error ? err.message : err);
|
||||
}
|
||||
}
|
||||
File diff suppressed because it is too large
Load Diff
Reference in New Issue
Block a user