mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
fix(maps): name the index as the source, and stop it being asked twice
Attribution first, because this branch argues the point itself: the card names the sources with the comment that naming them is a licence obligation rather than decoration, and the copy beside it promises that every field says where it came from. The POI path then stamped Overture rows as OpenStreetMap. Overture is not OpenStreetMap — it carries OSM among other sources under other licences — so that is a wrong attribution rather than a rounded one. The wire contract keeps `source` an open string and nothing on the client switches on it, so saying `trek-places` costs nothing. The MCP tool description no longer calls itself an OpenStreetMap tool either, and says which source answered. Adding one place cost two round trips to the index. The details lookup fetches the place and its answer carries the description; toPlaceRecord dropped that field, so the enrichment fetched the same place again a moment later to read it. It is carried through now and read from what the caller already has, with the second lookup left as the fallback for when there is nothing to read. That description's source URL becomes an href on the client and was taken on `typeof === 'string'` alone. It now goes through the same allow-list a place's website goes through — the file already does this for menu_url, with the comment explaining why — so anything but http(s) loses the link and keeps the text. The instance token was hashed from raw APP_URL, which the shipped compose file leaves commented out. Every default install therefore sent the same token, and a token every caller shares is not a caller: one instance running a large import would rate-limit all the others, who would fall back to Nominatim with nothing to point at. getAppUrl() falls through to ALLOWED_ORIGINS and then to the port, so unconfigured instances still differ. Two client-side leaks. A pick made out of the offline cache was recorded into the shadow corpus, where it moved the two figures the corpus exists to measure — and offline is a setting, so the network is usually reachable and the row landed. And the prefetch never re-checked the session after its request came back: a logout in that window repoints the handle at the anonymous database, so one person's trip area was written where the next person's offline search reads. The cache path also ignored the abort signal, so an answer for a keystroke two keystrokes old could overwrite a newer suggestion list. It rejects the way both surfaces already know to swallow. Left alone, and said in a comment rather than hidden: the index cannot localise. The Overpass path picks name:<lang> and falls back to int_name; the service has no language parameter at all, so a German reader exploring Tokyo gets the Japanese primary names on this path.
This commit is contained in:
@@ -483,7 +483,11 @@ describe('Tool: search_pois', () => {
|
||||
// The index carries opening hours; dropping them would leave the tool
|
||||
// description promising something the answer never has.
|
||||
expect(data.pois[0].opening_hours).toBe('Mo-Fr 09:00-17:00');
|
||||
expect(data.source).toBe('openstreetmap');
|
||||
// The answer names the index that produced it, over MCP as over REST. An
|
||||
// assistant reading `source` to attribute the data would otherwise credit
|
||||
// OpenStreetMap for rows Overture carries under other licences.
|
||||
expect(data.source).toBe('trek-places');
|
||||
expect(data.pois[0].source).toBe('trek-places');
|
||||
expect(data.clamped).toBe(false);
|
||||
expect(MapsService.prototype.searchOverpassPois).not.toHaveBeenCalled();
|
||||
|
||||
|
||||
@@ -142,12 +142,15 @@ describe('MapsService.pois answered from the index', () => {
|
||||
phone: '+43 1 5333763',
|
||||
opening_hours: 'Mo-Sa 07:30-22:00',
|
||||
cuisine: null,
|
||||
source: 'openstreetmap',
|
||||
// Named as what it is. Overture is not OpenStreetMap — it carries OSM
|
||||
// among other sources under other licences — and this branch argues
|
||||
// elsewhere that naming a source is a licence obligation rather than a
|
||||
// courtesy. Nothing on the client switches on the field, and the wire
|
||||
// contract keeps it an open string, so saying so costs nothing.
|
||||
source: 'trek-places',
|
||||
},
|
||||
]);
|
||||
// 'trek-places' would be a third value for a field the client treats as a
|
||||
// closed set, and these are the same places either way.
|
||||
expect(out.source).toBe('openstreetmap');
|
||||
expect(out.source).toBe('trek-places');
|
||||
});
|
||||
|
||||
it('MAPS-POIS-002: a row without address, contact or hours becomes nulls, typed as the pill', async () => {
|
||||
@@ -169,7 +172,7 @@ describe('MapsService.pois answered from the index', () => {
|
||||
phone: null,
|
||||
opening_hours: null,
|
||||
cuisine: null,
|
||||
source: 'openstreetmap',
|
||||
source: 'trek-places',
|
||||
});
|
||||
});
|
||||
|
||||
@@ -203,7 +206,7 @@ describe('MapsService.pois answered from the index', () => {
|
||||
phone: null,
|
||||
opening_hours: 'Mo-Sa 07:30-22:00',
|
||||
cuisine: null,
|
||||
source: 'openstreetmap',
|
||||
source: 'trek-places',
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -362,6 +362,56 @@ describe("the description on the place's own website", () => {
|
||||
lang: 'de',
|
||||
};
|
||||
|
||||
it('ENRICH-100: reads the description the details lookup already fetched', async () => {
|
||||
// The details call fetched this place a moment ago and its answer carries
|
||||
// the description. Asking again made adding one place cost two full round
|
||||
// trips to the index, each with its own timeout, for a field already in
|
||||
// hand.
|
||||
mockTrekPlacesById.mockClear();
|
||||
|
||||
const out = await make(mapsStub()).enrich(1, {
|
||||
...GERS_REQ,
|
||||
details: { description: { text: 'Pizza in Rostock, seit 2015.', sourceUrl: 'https://losteria.net/rostock' } },
|
||||
});
|
||||
|
||||
expect(out.description).toMatchObject({ text: 'Pizza in Rostock, seit 2015.', source: 'website' });
|
||||
expect(mockTrekPlacesById).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('ENRICH-101: still asks when the caller passed no details', async () => {
|
||||
mockTrekPlacesById.mockClear();
|
||||
mockTrekPlacesById.mockResolvedValue({
|
||||
description: { text: 'Pizza in Rostock, seit 2015.', sourceUrl: 'https://losteria.net/rostock' },
|
||||
});
|
||||
|
||||
const out = await make(mapsStub()).enrich(1, GERS_REQ);
|
||||
|
||||
expect(out.description).toMatchObject({ source: 'website' });
|
||||
expect(mockTrekPlacesById).toHaveBeenCalledWith('abc-123');
|
||||
});
|
||||
|
||||
it('ENRICH-102: a source URL that is not http(s) loses the link, not the text', async () => {
|
||||
// It becomes an href on the client, and the value comes from whatever index
|
||||
// the instance is pointed at. The same allow-list a place's website goes
|
||||
// through, for the same reason.
|
||||
mockTrekPlacesById.mockClear();
|
||||
|
||||
for (const bad of ['javascript:alert(1)', 'data:text/html,<script>', 'file:///etc/passwd', 'not a url']) {
|
||||
const out = await make(mapsStub()).enrich(1, {
|
||||
...GERS_REQ,
|
||||
details: { description: { text: 'Pizza in Rostock.', sourceUrl: bad } },
|
||||
});
|
||||
expect(out.description, bad).toMatchObject({ text: 'Pizza in Rostock.', sourceUrl: null });
|
||||
}
|
||||
|
||||
// The ordinary case still keeps its link.
|
||||
const ok = await make(mapsStub()).enrich(1, {
|
||||
...GERS_REQ,
|
||||
details: { description: { text: 'Pizza in Rostock.', sourceUrl: 'https://losteria.net/rostock' } },
|
||||
});
|
||||
expect(ok.description).toMatchObject({ sourceUrl: 'https://losteria.net/rostock' });
|
||||
});
|
||||
|
||||
it('ENRICH-090: quotes the site and credits it by URL', async () => {
|
||||
mockTrekPlacesById.mockResolvedValue({
|
||||
description: { text: 'Pizza in Rostock, seit 2015.', sourceUrl: 'https://losteria.net/rostock' },
|
||||
|
||||
@@ -9,10 +9,31 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
|
||||
|
||||
// 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' } },
|
||||
env: {
|
||||
maps: { trekPlacesUrl: '' },
|
||||
app: { appUrl: 'https://trip.example.org', port: 3001 },
|
||||
http: { allowedOriginsRaw: '' },
|
||||
},
|
||||
}));
|
||||
|
||||
vi.mock('../../../src/app-config', () => ({ readEnv: () => env }));
|
||||
// getAppUrl is the real resolution chain, reproduced here rather than stubbed
|
||||
// to a constant: the instance token depends on the whole chain, and the case
|
||||
// that matters is the one where APP_URL is unset — the shipped compose file
|
||||
// leaves it commented out, so that is the default install, not an edge case.
|
||||
vi.mock('../../../src/app-config', () => ({
|
||||
readEnv: () => env,
|
||||
getAppUrl: () => {
|
||||
const strip = (v: string) => v.replace(/\/+$/, '');
|
||||
if (env.app.appUrl) {
|
||||
try { new URL(env.app.appUrl); return strip(env.app.appUrl); } catch { /* fall through */ }
|
||||
}
|
||||
const first = env.http.allowedOriginsRaw.split(',')[0]?.trim();
|
||||
if (first) {
|
||||
try { new URL(first); return strip(first); } catch { /* fall through */ }
|
||||
}
|
||||
return `http://localhost:${env.app.port}`;
|
||||
},
|
||||
}));
|
||||
|
||||
import {
|
||||
DEFAULT_TREK_PLACES_URL,
|
||||
@@ -92,6 +113,8 @@ beforeEach(() => {
|
||||
resetTrekPlacesBreaker();
|
||||
env.maps.trekPlacesUrl = '';
|
||||
env.app.appUrl = 'https://trip.example.org';
|
||||
env.app.port = 3001;
|
||||
env.http.allowedOriginsRaw = '';
|
||||
// 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'); }));
|
||||
@@ -168,6 +191,33 @@ describe('trekPlacesSearch', () => {
|
||||
expect(unconfigured).not.toBe(configured);
|
||||
});
|
||||
|
||||
it('tells two unconfigured instances apart, which is the default install', async () => {
|
||||
// APP_URL is commented out in the shipped compose file, so "unset" is the
|
||||
// ordinary case rather than the odd one. Reading it raw gave every such
|
||||
// install the same token, and a token every caller shares is not a caller:
|
||||
// one instance running a large import would rate-limit all the others,
|
||||
// who would fall back to Nominatim with nothing to point at.
|
||||
const token = async () => {
|
||||
stubFetch({ query: 'x', results: [] });
|
||||
await trekPlacesSearch('x');
|
||||
return (calls[0].init?.headers as Record<string, string>)['X-TREK-Instance'];
|
||||
};
|
||||
|
||||
env.app.appUrl = '';
|
||||
env.http.allowedOriginsRaw = 'https://a.example.net';
|
||||
const viaOrigins = await token();
|
||||
|
||||
env.http.allowedOriginsRaw = 'https://b.example.net';
|
||||
expect(await token()).not.toBe(viaOrigins);
|
||||
|
||||
// And with nothing at all, the port still separates two instances on one host.
|
||||
env.http.allowedOriginsRaw = '';
|
||||
env.app.port = 3001;
|
||||
const onDefaultPort = await token();
|
||||
env.app.port = 8080;
|
||||
expect(await token()).not.toBe(onDefaultPort);
|
||||
});
|
||||
|
||||
it('omits parameters that were not given rather than sending empties', async () => {
|
||||
stubFetch({ query: 'x', results: [] });
|
||||
await trekPlacesSearch('x');
|
||||
|
||||
Reference in New Issue
Block a user