From d613c91e16830f49b4271b16492b52387f87f696 Mon Sep 17 00:00:00 2001 From: Maurice Date: Sun, 6 Sep 2026 23:35:26 +0200 Subject: [PATCH] fix(places): keep the chain out of the field that means "this place" toPlaceRecord wrote the brand's Wikidata item into `wikidata`, and getPlaceDetails spreads the record over the OSM details, so for a chain branch picked from the index that value replaced the building's own tag - and nulled a real one when the place had no brand. resolveIdentity reads that field as the place's own identity, so the photo ladder returned the chain's Commons files as pictures of this branch and the description came back as the company's article without the aboutBrand flag. That is precisely the failure WIKI_IDENTITY_TAGS and readBrandIdentity were written to prevent, in their own words: "a chain's logo passed off as a photo of the restaurant would be a plain lie". The item now travels under brand:wikidata, the key OSM uses and readBrandIdentity already reads, and resolveIdentity picks it up from the payload for a place OSM does not know - so a branch still gets the chain's description, flagged as such, and never its logo. The client test also covers trekPlacesArea, the streaming half of readCapped including the size cap, the configured base URL and the nearby defaults, none of which any test reached. --- server/src/nest/maps/trek-places.client.ts | 7 +- .../place-enrichment.service.ts | 9 +- .../unit/nest/trek-places.client.test.ts | 213 +++++++++++++++++- 3 files changed, 223 insertions(+), 6 deletions(-) diff --git a/server/src/nest/maps/trek-places.client.ts b/server/src/nest/maps/trek-places.client.ts index 3c10d3460..88b9a6fbf 100644 --- a/server/src/nest/maps/trek-places.client.ts +++ b/server/src/nest/maps/trek-places.client.ts @@ -258,7 +258,12 @@ export function toPlaceRecord(p: TrekPlace): Record { email: p.contact?.email ?? null, category: p.category ?? null, brand: p.brand?.name ?? null, - wikidata: p.brand?.wikidata ?? null, + // The chain's Wikidata item, under the key that says so. It must NOT go + // into `wikidata`: that field means "the article about this place", and the + // enrichment ladder reads it as such — a branch would then be illustrated + // with the chain's logo and described as the company, which is the failure + // readBrandIdentity and WIKI_IDENTITY_TAGS exist to prevent. + 'brand:wikidata': p.brand?.wikidata ?? null, source: 'trek-places', }; } diff --git a/server/src/nest/place-enrichment/place-enrichment.service.ts b/server/src/nest/place-enrichment/place-enrichment.service.ts index 0989858ea..21cc9f9fc 100644 --- a/server/src/nest/place-enrichment/place-enrichment.service.ts +++ b/server/src/nest/place-enrichment/place-enrichment.service.ts @@ -391,16 +391,21 @@ export class PlaceEnrichmentService { wikidata: fromPayload('wikidata'), wikimedia_commons: fromPayload('wikimedia_commons'), osmTags: null, - brand: { wikidata: null, wikipedia: null }, + // The index knows the chain a branch belongs to and hands it over under + // the same key OSM uses. It stays out of the three fields above: those + // say "this is the article about this place", and a chain's is not. + brand: { wikidata: fromPayload('brand:wikidata'), wikipedia: fromPayload('brand:wikipedia') }, }; if (carried.wikipedia || carried.wikidata || carried.wikimedia_commons) return carried; const resolved = await this.maps.resolveOsmIdentity(req.name, req.lat, req.lng, { lang: req.lang }); if (!resolved) return carried; + const brand = readBrandIdentity(resolved.tags); return { ...readWikiIdentity(resolved.tags), osmTags: resolved.tags, - brand: readBrandIdentity(resolved.tags), + // OSM first, the carried one when OSM has no brand tag for this object. + brand: brand.wikidata || brand.wikipedia ? brand : carried.brand, }; } diff --git a/server/tests/unit/nest/trek-places.client.test.ts b/server/tests/unit/nest/trek-places.client.test.ts index dae9d6b31..dc466b8cc 100644 --- a/server/tests/unit/nest/trek-places.client.test.ts +++ b/server/tests/unit/nest/trek-places.client.test.ts @@ -7,14 +7,18 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -vi.mock('../../../src/app-config', () => ({ - readEnv: () => ({ maps: { trekPlacesUrl: '' }, app: { appUrl: 'https://trip.example.org' } }), +// Mutable, so the instance URL and the operator's own index can be set per test. +const { env } = vi.hoisted(() => ({ + env: { maps: { trekPlacesUrl: '' }, app: { appUrl: 'https://trip.example.org' } }, })); +vi.mock('../../../src/app-config', () => ({ readEnv: () => env })); + import { DEFAULT_TREK_PLACES_URL, POI_CATEGORY_TO_TREK, toPlaceRecord, + trekPlacesArea, trekPlacesBaseUrl, trekPlacesById, trekPlacesNearby, @@ -37,6 +41,8 @@ const PLACE: TrekPlace = { hours: null, }; +const BOX = { minLat: 54, minLng: 12, maxLat: 54.2, maxLng: 12.3 }; + let calls: { url: string; init?: RequestInit }[] = []; function stubFetch(body: unknown, status = 200) { @@ -52,13 +58,71 @@ function stubFetch(body: unknown, status = 200) { })); } -beforeEach(() => { calls = []; }); +const encoder = new TextEncoder(); +let streamCancelled = false; + +// The same stub with a real body stream, so the capped reader runs instead of +// res.text(). text() throws here to prove which arm was taken. +function stubStreamingFetch(chunks: (string | Uint8Array)[], status = 200) { + calls = []; + streamCancelled = false; + const queue = chunks.map((c) => (typeof c === 'string' ? encoder.encode(c) : c)); + vi.stubGlobal('fetch', vi.fn(async (url: URL | string, init?: RequestInit) => { + calls.push({ url: String(url), init }); + return { + ok: status >= 200 && status < 300, + status, + body: new ReadableStream({ + pull(controller) { + const next = queue.shift(); + if (next) controller.enqueue(next); + else controller.close(); + }, + cancel() { streamCancelled = true; }, + }), + text: async () => { throw new Error('read the stream, not text()'); }, + } as unknown as Response; + })); +} + +beforeEach(() => { + calls = []; + env.maps.trekPlacesUrl = ''; + env.app.appUrl = 'https://trip.example.org'; + // Nothing in this file may reach the real service: a test that forgets to + // stub gets a thrown request rather than a request. + vi.stubGlobal('fetch', vi.fn(async () => { throw new Error('unstubbed fetch'); })); +}); afterEach(() => vi.unstubAllGlobals()); describe('trekPlacesBaseUrl', () => { it('falls back to the public instance when nothing is configured', () => { expect(trekPlacesBaseUrl()).toBe(DEFAULT_TREK_PLACES_URL); }); + + it('uses the operator own copy when one is configured', () => { + env.maps.trekPlacesUrl = 'https://places.example.net'; + expect(trekPlacesBaseUrl()).toBe('https://places.example.net'); + }); + + it('strips trailing slashes rather than doubling them into the path', () => { + env.maps.trekPlacesUrl = 'https://places.example.net//'; + expect(trekPlacesBaseUrl()).toBe('https://places.example.net'); + }); + + it('treats a blank setting as unconfigured', () => { + env.maps.trekPlacesUrl = ' '; + expect(trekPlacesBaseUrl()).toBe(DEFAULT_TREK_PLACES_URL); + }); + + it('sends the request to the configured host, not the public one', async () => { + env.maps.trekPlacesUrl = 'https://places.example.net/'; + stubFetch({ query: 'x', results: [] }); + await trekPlacesSearch('x'); + const url = new URL(calls[0].url); + expect(url.origin).toBe('https://places.example.net'); + expect(url.pathname).toBe('/v1/search'); + }); }); describe('trekPlacesSearch', () => { @@ -84,6 +148,23 @@ describe('trekPlacesSearch', () => { expect(token).not.toContain('example.org'); }); + it('names one instance the same way twice, and another instance differently', async () => { + const token = async () => { + stubFetch({ query: 'x', results: [] }); + await trekPlacesSearch('x'); + return (calls[0].init?.headers as Record)['X-TREK-Instance']; + }; + const configured = await token(); + // Derived from the URL, not generated: a token that changed per call would + // hand the same instance a fresh rate-limit budget on every request. + expect(await token()).toBe(configured); + env.app.appUrl = ''; + const unconfigured = await token(); + expect(unconfigured).toMatch(/^trek-[a-z0-9]+$/); + // Two instances the far side must be able to tell apart for its rate limit. + expect(unconfigured).not.toBe(configured); + }); + it('omits parameters that were not given rather than sending empties', async () => { stubFetch({ query: 'x', results: [] }); await trekPlacesSearch('x'); @@ -103,6 +184,37 @@ describe('trekPlacesSearch', () => { }); }); +describe('reading the body', () => { + // One buffer, handed out more than once: two of them are already over the cap. + const BIG = new Uint8Array(600_000); + + it('assembles an answer that arrives in pieces', async () => { + const payload = JSON.stringify({ query: 'x', results: [{ ...PLACE, name: 'Café Kröpeliner' }] }); + const bytes = encoder.encode(payload); + // Cut between the two bytes of the é: decoding chunk by chunk would leave a + // replacement character where the name is. + const cut = bytes.indexOf(0xc3) + 1; + stubStreamingFetch([bytes.slice(0, cut), bytes.slice(cut)]); + const found = await trekPlacesSearch('cafe'); + expect(found[0].name).toBe('Café Kröpeliner'); + }); + + it('refuses a body past the megabyte cap instead of buffering it', async () => { + // The first chunk is under the cap, the second takes it over. + stubStreamingFetch([BIG, BIG]); + await expect(trekPlacesSearch('x')).rejects.toThrow('TREK Places API response too large'); + }); + + it('drops the connection instead of draining the rest of an oversized body', async () => { + // A third chunk nobody reads: the cap trips while the body is still running, + // which is the case that has a connection left to drop. Cancelling a stream + // that already ended is a no-op and would assert nothing. + stubStreamingFetch([BIG, BIG, BIG]); + await expect(trekPlacesSearch('x')).rejects.toThrow('TREK Places API response too large'); + expect(streamCancelled).toBe(true); + }); +}); + describe('trekPlacesById', () => { it('returns null for a place that does not exist instead of throwing', async () => { stubFetch({}, 404); @@ -129,6 +241,90 @@ describe('trekPlacesNearby', () => { expect(url.searchParams.get('radius')).toBe('813'); expect(url.searchParams.get('category')).toBe('cafe,coffee_shop'); }); + + it('asks 1500m and 50 rows when the caller says nothing', async () => { + stubFetch({ results: [] }); + await trekPlacesNearby(54.1, 12.1); + const url = new URL(calls[0].url); + expect(url.pathname).toBe('/v1/nearby'); + expect(url.searchParams.get('radius')).toBe('1500'); + expect(url.searchParams.get('limit')).toBe('50'); + expect(url.searchParams.has('category')).toBe(false); + }); + + it('lets the caller ask for a smaller circle and fewer rows', async () => { + stubFetch({ results: [] }); + await trekPlacesNearby(54.1, 12.1, { radius: 300, limit: 5 }); + const url = new URL(calls[0].url); + expect(url.searchParams.get('radius')).toBe('300'); + expect(url.searchParams.get('limit')).toBe('5'); + }); + + it('passes the distance the index measured through', async () => { + stubFetch({ results: [{ ...PLACE, distanceMetres: 12 }] }); + const out = await trekPlacesNearby(54.1, 12.1); + expect(out).toHaveLength(1); + expect(out[0].distanceMetres).toBe(12); + }); + + it('survives a malformed results field', async () => { + stubFetch({ results: { 0: PLACE } }); + await expect(trekPlacesNearby(54.1, 12.1)).resolves.toEqual([]); + }); +}); + +describe('trekPlacesArea', () => { + it('sends the whole box and caps the rows at 2000', async () => { + stubFetch({ count: 1, truncated: false, results: [PLACE] }); + await trekPlacesArea(BOX); + const url = new URL(calls[0].url); + expect(url.pathname).toBe('/v1/bbox'); + expect(url.searchParams.get('minLat')).toBe('54'); + expect(url.searchParams.get('minLng')).toBe('12'); + expect(url.searchParams.get('maxLat')).toBe('54.2'); + expect(url.searchParams.get('maxLng')).toBe('12.3'); + expect(url.searchParams.get('limit')).toBe('2000'); + }); + + it('sends the caller own cap when one is given', async () => { + stubFetch({ count: 0, truncated: false, results: [] }); + await trekPlacesArea(BOX, 500); + expect(new URL(calls[0].url).searchParams.get('limit')).toBe('500'); + }); + + it('keeps an edge on the equator instead of dropping it', async () => { + stubFetch({ count: 0, truncated: false, results: [] }); + await trekPlacesArea({ ...BOX, minLat: 0 }); + // 0 is a coordinate, not a missing value; dropping it would widen the box. + expect(new URL(calls[0].url).searchParams.get('minLat')).toBe('0'); + }); + + it('passes truncation through rather than smoothing it over', async () => { + stubFetch({ count: 20000, truncated: true, results: [PLACE] }); + const area = await trekPlacesArea(BOX); + // A client that believed it had the whole area would search offline and + // quietly miss places. + expect(area.truncated).toBe(true); + expect(area.count).toBe(20000); + expect(area.results).toHaveLength(1); + }); + + it('reads a count that arrived as a string, and a truthy truncated flag', async () => { + stubFetch({ count: '17', truncated: 1, results: [] }); + const area = await trekPlacesArea(BOX); + expect(area.count).toBe(17); + expect(area.truncated).toBe(true); + }); + + it('turns a malformed answer into an empty one rather than passing it on', async () => { + stubFetch({ count: 'lots', truncated: undefined, results: null }); + await expect(trekPlacesArea(BOX)).resolves.toEqual({ count: 0, truncated: false, results: [] }); + }); + + it('throws on an error status so the sync can leave the area alone', async () => { + stubFetch({}, 502); + await expect(trekPlacesArea(BOX)).rejects.toThrow(/502/); + }); }); describe('toPlaceRecord', () => { @@ -146,6 +342,17 @@ describe('toPlaceRecord', () => { }); }); + it('files the chain under brand:wikidata, never under the place own identity', () => { + // `wikidata` means "the article about this place". Putting the chain's item + // there is how a branch ends up illustrated with the company logo and + // described as the franchise operator, which is the whole reason + // readWikiIdentity refuses to follow brand:* tags. + const r = toPlaceRecord(PLACE); + expect(r['brand:wikidata']).toBe('Q123'); + expect(r.wikidata).toBeUndefined(); + expect(r.brand).toBe("L'Osteria"); + }); + it('reports no rating rather than a made-up one', () => { // Ratings exist in no open dataset. Null says so; 0 would be a claim. expect(toPlaceRecord(PLACE).rating).toBeNull();