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.
This commit is contained in:
Maurice
2026-09-06 23:35:26 +02:00
committed by Maurice
parent 0991fa538b
commit d613c91e16
3 changed files with 223 additions and 6 deletions
+6 -1
View File
@@ -258,7 +258,12 @@ export function toPlaceRecord(p: TrekPlace): Record<string, unknown> {
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',
};
}
@@ -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,
};
}
@@ -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<Uint8Array>({
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<string, string>)['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();