mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-23 23:46:35 +00:00
feat(places): send a search to Google, once or always
Since 4.3.0 the TREK index and OpenStreetMap answer every search first and Google is only asked when they find nothing, so a list with the wrong place on it never reached Google, whatever key the instance had, and the provider choice only decided who fills the empty case. Two ways to change that, both only where Google holds the key slot. Under a result list that did not come from Google, one quiet line sends the same query to Google Places alone (`provider: 'google'` on the search contract, the MCP search tool takes it too). And beside the key, an admin switch "Search with Google only" hands every search and every suggestion to Google and asks nothing of the index or OpenStreetMap; off by default, read by the search after the keyed provider so an install without a key, or one on Amap or OpenStreetMap, is untouched. The panel says so beside the switch. Both shells share the one rule for when the line is offered (placeSource), and the wiki names both switches.
This commit is contained in:
@@ -198,6 +198,15 @@ export class AddonsService {
|
||||
getPlaceShadow() { return this.readFlag('place_shadow_enabled'); }
|
||||
updatePlaceShadow(enabled: boolean) { return this.writeFlag('place_shadow_enabled', enabled); }
|
||||
|
||||
/**
|
||||
* Search and suggestions from Google alone, skipping the index and
|
||||
* OpenStreetMap. Fail-closed for the same reason as the shadow log: nobody had
|
||||
* this before it existed, so an absent row correctly means off. MapsService
|
||||
* reads the same key the same way, and only once Google holds the key slot.
|
||||
*/
|
||||
getPlacesGoogleOnly() { return this.readFlag('places_google_only'); }
|
||||
updatePlacesGoogleOnly(enabled: boolean) { return this.writeFlag('places_google_only', enabled); }
|
||||
|
||||
/**
|
||||
* Enrichment reads fail-OPEN, unlike the three switches above.
|
||||
*
|
||||
|
||||
@@ -243,6 +243,16 @@ export class AdminController {
|
||||
return result;
|
||||
}
|
||||
|
||||
@Get('places-google-only')
|
||||
getPlacesGoogleOnly() { return this.addons.getPlacesGoogleOnly(); }
|
||||
|
||||
@Put('places-google-only')
|
||||
updatePlacesGoogleOnly(@CurrentUser() user: User, @Body() body: AdminFeatureToggleDto, @Req() req: Request) {
|
||||
const result = this.addons.updatePlacesGoogleOnly(body.enabled);
|
||||
this.audit.writeAudit({ userId: user.id, action: 'admin.places_google_only', ip: getClientIp(req), details: { enabled: result.enabled } });
|
||||
return result;
|
||||
}
|
||||
|
||||
@Get('places-enrich')
|
||||
getPlacesEnrich() { return this.addons.getPlacesEnrich(); }
|
||||
|
||||
|
||||
@@ -76,7 +76,7 @@ export class MapsController {
|
||||
@Query('lang') lang?: string,
|
||||
): Promise<MapsSearchResult> {
|
||||
try {
|
||||
return await this.maps.search(user.id, body.query, lang, body.locationBias);
|
||||
return await this.maps.search(user.id, body.query, lang, body.locationBias, body.provider);
|
||||
} catch (err: unknown) {
|
||||
console.error('Maps search error:', err);
|
||||
throw toHttpException(err, 'Search error', 500);
|
||||
|
||||
@@ -664,6 +664,12 @@ type LocationBias = { low: { lat: number; lng: number }; high: { lat: number; ln
|
||||
* predates Amap, means `auto`, which keeps Google.
|
||||
*/
|
||||
export const PLACES_PROVIDER_SETTING = 'places_provider';
|
||||
/**
|
||||
* The admin switch that hands search and suggestions to Google alone. Off, the
|
||||
* index and OpenStreetMap answer first and Google is only asked when they find
|
||||
* nothing, which is what every install has had since 4.3.0.
|
||||
*/
|
||||
export const PLACES_GOOGLE_ONLY_SETTING = 'places_google_only';
|
||||
|
||||
/**
|
||||
* Whoever holds the keyed slot beside the index for one request: Google's
|
||||
@@ -761,8 +767,14 @@ export class MapsService {
|
||||
|
||||
// ── Controller-facing surface (unchanged signatures) ───────────────────────
|
||||
|
||||
search(userId: number, query: string, lang?: string, locationBias?: { lat: number; lng: number; radius?: number }): Promise<MapsSearchResult> {
|
||||
return this.searchPlaces(userId, query, lang, locationBias) as Promise<MapsSearchResult>;
|
||||
search(
|
||||
userId: number,
|
||||
query: string,
|
||||
lang?: string,
|
||||
locationBias?: { lat: number; lng: number; radius?: number },
|
||||
provider?: 'google',
|
||||
): Promise<MapsSearchResult> {
|
||||
return this.searchPlaces(userId, query, lang, locationBias, { googleOnly: provider === 'google' }) as Promise<MapsSearchResult>;
|
||||
}
|
||||
|
||||
autocomplete(userId: number, input: string, lang?: string, locationBias?: LocationBias, sessionToken?: string): Promise<MapsAutocompleteResult> {
|
||||
@@ -1065,6 +1077,33 @@ export class MapsService {
|
||||
: null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether this search goes to Google and nowhere else.
|
||||
*
|
||||
* Two ways to ask for that, both born of the same moment: the index answered
|
||||
* a query with something that is not the place the traveller meant, and with
|
||||
* the index and OpenStreetMap answering first, Google was never consulted as
|
||||
* long as they found anything at all. The caller can send one search to
|
||||
* Google (`requested`, the "search Google instead" link under the results),
|
||||
* and the admin can make that the rule for every search and suggestion (the
|
||||
* switch beside the key). Either way it only holds when Google holds the key
|
||||
* slot: on an install without a Google key, or one that picked Amap or
|
||||
* OpenStreetMap, both change nothing, and the admin panel says so.
|
||||
*
|
||||
* Read after the keyed provider on purpose: that lookup already walked the
|
||||
* key chain, and a setting read ahead of it would shift the order of the
|
||||
* app_settings reads every test of the chain stubs by position.
|
||||
*/
|
||||
private googleOnly(keyed: KeyedProvider | null, requested = false): boolean {
|
||||
if (keyed?.id !== 'google') return false;
|
||||
if (requested) return true;
|
||||
const row = this.database.get<{ value: string }>(
|
||||
'SELECT value FROM app_settings WHERE key = ?',
|
||||
PLACES_GOOGLE_ONLY_SETTING,
|
||||
);
|
||||
return row?.value === 'true';
|
||||
}
|
||||
|
||||
/** The Amap provider, when Amap holds the keyed slot; null otherwise. */
|
||||
resolvePlacesProvider(userId: number): AmapPlacesProvider | null {
|
||||
const keyed = this.keyedProvider(userId);
|
||||
@@ -2021,7 +2060,7 @@ export class MapsService {
|
||||
query: string,
|
||||
lang?: string,
|
||||
locationBias?: { lat: number; lng: number; radius?: number },
|
||||
opts: { googleIdentityOnly?: boolean } = {},
|
||||
opts: { googleIdentityOnly?: boolean; googleOnly?: boolean } = {},
|
||||
): Promise<{ places: Record<string, unknown>[]; source: string }> {
|
||||
const keyed = this.keyedProvider(userId);
|
||||
const { key: apiKey, source: keySource } = keyed?.id === 'google' ? keyed : { key: null, source: null };
|
||||
@@ -2048,7 +2087,10 @@ export class MapsService {
|
||||
// the same question twice. `null` means it never ran.
|
||||
let osmAnswer: Record<string, unknown>[] | null = null;
|
||||
|
||||
if (this.trekPlacesEnabled() && !(opts.googleIdentityOnly && apiKey)) {
|
||||
// A search sent to Google on purpose, or the admin's "Google only" switch,
|
||||
// skips the pair the same way: the search then reads exactly as it did
|
||||
// before 4.3.0 on an install with a key.
|
||||
if (this.trekPlacesEnabled() && !(opts.googleIdentityOnly && apiKey) && !this.googleOnly(keyed, opts.googleOnly)) {
|
||||
// Both at once. The index is a dataset of businesses and is very good
|
||||
// at those; OpenStreetMap is where the temples, bridges, riverside
|
||||
// walks and viewpoints are, and a travel search asks for those
|
||||
@@ -2188,8 +2230,9 @@ export class MapsService {
|
||||
// could get every instance blocked at once. The index removes that.
|
||||
//
|
||||
// Same contract as search: never throws upward, falls through to what this
|
||||
// method did before.
|
||||
if (this.trekPlacesEnabled()) {
|
||||
// method did before. The admin's "Google only" switch skips the index here
|
||||
// too, so the suggestions and the search agree on where they come from.
|
||||
if (this.trekPlacesEnabled() && !this.googleOnly(keyed)) {
|
||||
try {
|
||||
const centre = locationBias
|
||||
? {
|
||||
|
||||
@@ -306,18 +306,19 @@ export class PlacesMcp {
|
||||
query: z.string().min(1).max(500).describe('Place name or address to search for'),
|
||||
locationBias: mapsSearchRequestSchema.shape.locationBias.describe('Centre the search on a coordinate: { lat, lng, radius? } with radius in metres (default 50000). Only the Google provider honours it; the OpenStreetMap fallback ignores it'),
|
||||
lang: z.string().max(35).optional().describe('BCP 47 language for the result names, e.g. "de" or "ja". Defaults to English'),
|
||||
provider: mapsSearchRequestSchema.shape.provider.describe('"google" sends this one search to Google Places alone instead of the TREK index and OpenStreetMap, the same as the "search Google instead" link under the results in the app. Ignored when the instance has no Google key'),
|
||||
},
|
||||
annotations: TOOL_ANNOTATIONS_READONLY,
|
||||
access: { group: 'places', mode: 'read' },
|
||||
})
|
||||
async searchPlace(
|
||||
{ query, locationBias, lang }: {
|
||||
query: string; locationBias?: { lat: number; lng: number; radius?: number }; lang?: string;
|
||||
{ query, locationBias, lang, provider }: {
|
||||
query: string; locationBias?: { lat: number; lng: number; radius?: number }; lang?: string; provider?: 'google';
|
||||
},
|
||||
ctx: McpContext,
|
||||
) {
|
||||
try {
|
||||
const result = await this.maps.searchPlaces(ctx.userId, query, lang, locationBias);
|
||||
const result = await this.maps.searchPlaces(ctx.userId, query, lang, locationBias, { googleOnly: provider === 'google' });
|
||||
return ok(result);
|
||||
} catch {
|
||||
return errorResult('Place search failed.');
|
||||
|
||||
@@ -495,6 +495,36 @@ describe('AddonsService transit provider', () => {
|
||||
* PlaceShadowService.enabled(), which reads the same key itself — if the two
|
||||
* diverge the admin panel shows "off" while the log keeps collecting picks.
|
||||
*/
|
||||
/**
|
||||
* The Google-only switch reads like the shadow log: nobody had it before it
|
||||
* existed, so an absent row is off, and MapsService.googleOnly() compares the
|
||||
* same key against the same literal.
|
||||
*/
|
||||
describe('AddonsService places Google-only flag', () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
it('ADDONS-SVC-095 an unset flag reads as OFF, only the literal "true" switches it on', () => {
|
||||
dbMock._stmt.get.mockReturnValueOnce(undefined);
|
||||
expect(svc().getPlacesGoogleOnly()).toEqual({ enabled: false });
|
||||
expect(dbMock._stmt.get).toHaveBeenLastCalledWith('places_google_only');
|
||||
for (const value of ['false', 'TRUE', '1', '']) {
|
||||
dbMock._stmt.get.mockReturnValueOnce({ value });
|
||||
expect(svc().getPlacesGoogleOnly()).toEqual({ enabled: false });
|
||||
}
|
||||
dbMock._stmt.get.mockReturnValueOnce({ value: 'true' });
|
||||
expect(svc().getPlacesGoogleOnly()).toEqual({ enabled: true });
|
||||
});
|
||||
|
||||
it('ADDONS-SVC-096 the setter persists the literal string under its own key', () => {
|
||||
expect(svc().updatePlacesGoogleOnly(true)).toEqual({ enabled: true });
|
||||
expect(dbMock._stmt.run).toHaveBeenLastCalledWith('places_google_only', 'true');
|
||||
expect(svc().updatePlacesGoogleOnly(false)).toEqual({ enabled: false });
|
||||
expect(dbMock._stmt.run).toHaveBeenLastCalledWith('places_google_only', 'false');
|
||||
});
|
||||
});
|
||||
|
||||
describe('AddonsService place shadow flag', () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
|
||||
@@ -48,6 +48,9 @@ const addonsStub = () => ({
|
||||
// Fail-open, unlike its three neighbours — see AddonsService.getPlacesEnrich.
|
||||
getPlacesEnrich: vi.fn(() => ({ enabled: true })),
|
||||
updatePlacesEnrich: vi.fn((enabled: boolean) => ({ enabled })),
|
||||
// Fail-closed with nothing to backfill, like the shadow log.
|
||||
getPlacesGoogleOnly: vi.fn(() => ({ enabled: false })),
|
||||
updatePlacesGoogleOnly: vi.fn((enabled: boolean) => ({ enabled })),
|
||||
// Fail-open too, and the only switch here about egress rather than spending.
|
||||
getCollabFeatures: vi.fn(() => ({ chat: false })),
|
||||
updateCollabFeatures: vi.fn(() => ({ features: { chat: true }, changed: true })),
|
||||
@@ -277,9 +280,16 @@ describe('AdminController feature toggles', () => {
|
||||
expect(c.getPlacesDetails()).toEqual({ enabled: false });
|
||||
expect(c.getPlaceShadow()).toEqual({ enabled: false });
|
||||
expect(c.getPlacesEnrich()).toEqual({ enabled: true });
|
||||
expect(c.getPlacesGoogleOnly()).toEqual({ enabled: false });
|
||||
expect(c.getCollabFeatures()).toEqual({ chat: false });
|
||||
});
|
||||
|
||||
it('ADMIN-TOGGLE-002d places-google-only updates through the addons domain and is audited', () => {
|
||||
const c = adminCtl(svc());
|
||||
expect(c.updatePlacesGoogleOnly(user, { enabled: true }, req)).toEqual({ enabled: true });
|
||||
expect(writeAudit).toHaveBeenCalledWith(expect.objectContaining({ action: 'admin.places_google_only', details: { enabled: true } }));
|
||||
});
|
||||
|
||||
it('ADMIN-TOGGLE-002b places-enrich updates through the addons domain and is audited', () => {
|
||||
const c = adminCtl(svc());
|
||||
expect(c.updatePlacesEnrich(user, { enabled: false }, req)).toEqual({ enabled: false });
|
||||
|
||||
@@ -49,14 +49,20 @@ describe('MapsController (parity with the legacy /api/maps route)', () => {
|
||||
const search = vi.fn().mockResolvedValue({ places: [], source: 'osm' });
|
||||
const res = await makeController({ search }).search(user, { query: 'berlin' }, 'de');
|
||||
expect(res).toEqual({ places: [], source: 'osm' });
|
||||
expect(search).toHaveBeenCalledWith(3, 'berlin', 'de', undefined);
|
||||
expect(search).toHaveBeenCalledWith(3, 'berlin', 'de', undefined, undefined);
|
||||
});
|
||||
|
||||
it('forwards the provider a search is sent to alone', async () => {
|
||||
const search = vi.fn().mockResolvedValue({ places: [], source: 'google' });
|
||||
await makeController({ search }).search(user, { query: 'x', provider: 'google' }, 'de');
|
||||
expect(search).toHaveBeenCalledWith(3, 'x', 'de', undefined, 'google');
|
||||
});
|
||||
|
||||
it('forwards a valid locationBias to the service', async () => {
|
||||
const search = vi.fn().mockResolvedValue({ places: [], source: 'osm' });
|
||||
const bias = { lat: 1, lng: 2, radius: 5000 };
|
||||
await makeController({ search }).search(user, { query: 'x', locationBias: bias }, 'de');
|
||||
expect(search).toHaveBeenCalledWith(3, 'x', 'de', bias);
|
||||
expect(search).toHaveBeenCalledWith(3, 'x', 'de', bias, undefined);
|
||||
});
|
||||
|
||||
it('maps a service error to its status + message', async () => {
|
||||
|
||||
@@ -76,13 +76,27 @@ const osmAnswer = (rows: unknown[]) => ({ ok: true, json: async () => rows });
|
||||
/**
|
||||
* Keyed on the statement so the kill switch can be driven without also handing
|
||||
* `'false'` to the API-key resolver, which would send the fallback at Google.
|
||||
*
|
||||
* `rows` are the app_settings the instance holds, by key: a Google key, the
|
||||
* provider choice, the Google-only switch. Anything else reads as absent.
|
||||
*/
|
||||
function make(enabled = true) {
|
||||
function make(enabled = true, rows: Record<string, string> = {}) {
|
||||
trekPlaces.on = enabled;
|
||||
const database = { get: vi.fn(() => undefined) } as unknown as DatabaseService;
|
||||
const database = {
|
||||
get: vi.fn((sql: string, key?: unknown) =>
|
||||
typeof key === 'string' && sql.includes('app_settings') && rows[key] !== undefined ? { value: rows[key] } : undefined,
|
||||
),
|
||||
} as unknown as DatabaseService;
|
||||
return new MapsService(database, {} as PlacePhotoCacheService);
|
||||
}
|
||||
|
||||
const googleAnswer = (name: string) => ({
|
||||
ok: true,
|
||||
json: async () => ({
|
||||
places: [{ id: 'g1', displayName: { text: name }, formattedAddress: 'Tokyo', location: { latitude: 35.68, longitude: 139.77 } }],
|
||||
}),
|
||||
});
|
||||
|
||||
beforeEach(() => {
|
||||
mockSearch.mockReset();
|
||||
mockSearch.mockResolvedValue([]);
|
||||
@@ -195,6 +209,64 @@ describe('MapsService.searchPlaces — the merged path', () => {
|
||||
expect(out.places[2].name).toBe('Index 1');
|
||||
});
|
||||
|
||||
it('MAPS-SEARCH-011: the Google-only switch sends the search to Google and asks neither the index nor OpenStreetMap', async () => {
|
||||
mockSearch.mockResolvedValue([indexHit('Weigh station, Ritzville')]);
|
||||
mockNominatim.mockResolvedValue(osmAnswer([osmRow('Some station')]));
|
||||
const fetchMock = vi.fn().mockResolvedValue(googleAnswer('Tokyo Station'));
|
||||
vi.stubGlobal('fetch', fetchMock);
|
||||
|
||||
const out = await make(true, { maps_api_key: 'key', places_google_only: 'true' }).searchPlaces(1, 'Tokyo Station');
|
||||
|
||||
expect(mockSearch).not.toHaveBeenCalled();
|
||||
expect(mockNominatim).not.toHaveBeenCalled();
|
||||
expect(out.source).toBe('google');
|
||||
expect(out.places[0].name).toBe('Tokyo Station');
|
||||
expect(String(fetchMock.mock.calls[0][0])).toContain('places:searchText');
|
||||
vi.unstubAllGlobals();
|
||||
});
|
||||
|
||||
it('MAPS-SEARCH-012: the switch changes nothing without a Google key, or when Google does not hold the slot', async () => {
|
||||
mockSearch.mockResolvedValue([indexHit("L'Osteria")]);
|
||||
mockNominatim.mockResolvedValue(osmAnswer([osmRow('Steinstrasse')]));
|
||||
|
||||
// No key at all: the index and OpenStreetMap answer, as before.
|
||||
const keyless = await make(true, { places_google_only: 'true' }).searchPlaces(1, "L'Osteria");
|
||||
expect(keyless.source).toBe('trek-places+openstreetmap');
|
||||
|
||||
// A key, but the admin picked OpenStreetMap as the provider: Google holds
|
||||
// no slot, so there is nothing for the switch to hand the search to.
|
||||
mockSearch.mockClear();
|
||||
const osmOnly = await make(true, { maps_api_key: 'key', places_google_only: 'true', places_provider: 'openstreetmap' })
|
||||
.searchPlaces(1, "L'Osteria");
|
||||
expect(mockSearch).toHaveBeenCalled();
|
||||
expect(osmOnly.source).toBe('trek-places+openstreetmap');
|
||||
|
||||
// A key and the switch off: the index still answers first.
|
||||
mockSearch.mockClear();
|
||||
const off = await make(true, { maps_api_key: 'key', places_google_only: 'false' }).searchPlaces(1, "L'Osteria");
|
||||
expect(mockSearch).toHaveBeenCalled();
|
||||
expect(off.source).toBe('trek-places+openstreetmap');
|
||||
});
|
||||
|
||||
it('MAPS-SEARCH-013: one search sent to Google on purpose goes there alone, and only where Google holds the slot', async () => {
|
||||
mockSearch.mockResolvedValue([indexHit('Weigh station, Ritzville')]);
|
||||
mockNominatim.mockResolvedValue(osmAnswer([osmRow('Some station')]));
|
||||
vi.stubGlobal('fetch', vi.fn().mockResolvedValue(googleAnswer('Tokyo Station')));
|
||||
|
||||
const asked = await make(true, { maps_api_key: 'key' }).searchPlaces(1, 'Tokyo Station', undefined, undefined, { googleOnly: true });
|
||||
expect(mockSearch).not.toHaveBeenCalled();
|
||||
expect(asked.source).toBe('google');
|
||||
|
||||
// The same request on an install without a key is an ordinary search: the
|
||||
// link that sends it is never shown there, and a client that sends it
|
||||
// anyway gets what everybody else gets.
|
||||
mockSearch.mockClear();
|
||||
const keyless = await make(true, {}).searchPlaces(1, 'Tokyo Station', undefined, undefined, { googleOnly: true });
|
||||
expect(mockSearch).toHaveBeenCalled();
|
||||
expect(keyless.source).toBe('trek-places+openstreetmap');
|
||||
vi.unstubAllGlobals();
|
||||
});
|
||||
|
||||
it('MAPS-SEARCH-007: with the index switched off the query never leaves for it', async () => {
|
||||
mockSearch.mockResolvedValue([indexHit("L'Osteria")]);
|
||||
mockNominatim.mockResolvedValue(osmAnswer([osmRow('Steinstrasse')]));
|
||||
|
||||
@@ -2721,7 +2721,9 @@ describe('controller-facing wrappers delegate to the folded methods', () => {
|
||||
try {
|
||||
const circleBias = { lat: 1, lng: 2, radius: 5 };
|
||||
await svc.search(3, 'berlin', 'de', circleBias);
|
||||
expect(spies.searchPlaces).toHaveBeenCalledWith(3, 'berlin', 'de', circleBias);
|
||||
expect(spies.searchPlaces).toHaveBeenCalledWith(3, 'berlin', 'de', circleBias, { googleOnly: false });
|
||||
await svc.search(3, 'berlin', 'de', circleBias, 'google');
|
||||
expect(spies.searchPlaces).toHaveBeenLastCalledWith(3, 'berlin', 'de', circleBias, { googleOnly: true });
|
||||
|
||||
const rectBias = { low: { lat: 1, lng: 2 }, high: { lat: 3, lng: 4 } };
|
||||
await svc.autocomplete(3, 'be', 'en', rectBias);
|
||||
|
||||
Reference in New Issue
Block a user