Skip to content

Commit b9de199

Browse files
MedformatikMedformatik
committed
refactor(integrations): make orchestrators stateless
Removes the `let _ctx` singleton-with-throw pattern from the photos, reviews, and transit/place-transit orchestrators. Instead of capturing ctx in module-level state and throwing "<X> orchestrator not initialized" if a caller missed the init step, each orchestrator now exposes pure functions that take the provider list (or, in place-transit's case, ctx + the transit orchestrator) as arguments. Companion to e250292 (alias-external + esbuild guardrail), which prevented the dual-instance import that triggered the original 500. This commit removes the fragile pattern itself so a future regression can't reintroduce the same bug class. See docs/plans/stateless-orchestrators.md for the full rationale, design tradeoffs, and migration plan. Public surface change for community integrations: * `searchPhotos`, `searchHeroPhotos`, `fetchReviews`, `fetchAggregate`, `submitReview`, `uploadReviewImage` now take a `providers` arg. * `initPhotosOrchestrator`, `initReviewsOrchestrator`, `initTransitOrchestrator` are removed. * `getPhotoProviders(integrations)`, `getReviewProviders(integrations)`, `createPlaceTransit(ctx, orchestrator)` are added. Migration in an integration `setup(ctx)`: -import { searchHeroPhotos } from "@integrations/photos/orchestrator"; +import { getPhotoProviders, searchHeroPhotos } from + "@integrations/photos/orchestrator"; -const photos = await searchHeroPhotos(osmTags); +const providers = getPhotoProviders(ctx.getIntegrationsByDomain("photos")); +const photos = await searchHeroPhotos(osmTags, providers); Adds first unit tests for the photos and reviews orchestrators, covering the enable/domain filter and (for photos) the street-level imagery priority sort. Out of scope: provider-level URL/key module state and geocoding/place-lookup's NOMINATIM_URL — both work reliably under the current alias+guardrail and don't share the throw-on-uninitialised hazard. See the plan for the followup.
1 parent e250292 commit b9de199

10 files changed

Lines changed: 674 additions & 463 deletions

File tree

‎apps/api/src/routes/__tests__/places.test.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,17 +67,23 @@ vi.mock("../../services/knowledge/index.js", () => ({
6767

6868
// Mock photo service
6969

70-
vi.mock("../../../../../integrations/photos/orchestrator.js", () => ({
70+
vi.mock("@integrations/photos/orchestrator", () => ({
71+
getPhotoProviders: vi.fn().mockReturnValue([]),
7172
searchHeroPhotos: vi.fn().mockResolvedValue([]),
7273
deduplicatePhotos: vi.fn((photos: unknown[]) => photos),
7374
}));
7475

7576
// Mock reviews orchestrator — `fetchAggregate` would otherwise hit the
7677
// real Mangrove service via safeAggregate on every `/places/:id` call.
77-
vi.mock("../../../../../integrations/reviews/orchestrator.js", () => ({
78+
vi.mock("@integrations/reviews/orchestrator", () => ({
79+
getReviewProviders: vi.fn().mockReturnValue([]),
7880
fetchAggregate: vi.fn().mockResolvedValue(null),
7981
}));
8082

83+
vi.mock("../../integration-host.js", () => ({
84+
getAllIntegrations: vi.fn().mockReturnValue([]),
85+
}));
86+
8187
// Mock DB RIS service
8288

8389
const mockLookupDbStation = vi.fn();

‎apps/api/src/routes/places.ts‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,11 @@
11
import { lookupByCoords, lookupByNameAndCoords } from "@integrations/geocoding/place-lookup";
2-
import { deduplicatePhotos, searchHeroPhotos } from "@integrations/photos/orchestrator";
3-
import { fetchAggregate } from "@integrations/reviews/orchestrator";
4-
import type { Place } from "@openmapx/core";
2+
import {
3+
deduplicatePhotos,
4+
getPhotoProviders,
5+
searchHeroPhotos,
6+
} from "@integrations/photos/orchestrator";
7+
import { fetchAggregate, getReviewProviders } from "@integrations/reviews/orchestrator";
8+
import type { Place, ReviewProvider } from "@openmapx/core";
59
import {
610
CATEGORY_FILTERS,
711
categoryPlaceToPlace,
@@ -14,6 +18,7 @@ import {
1418
searchByCategory,
1519
} from "@openmapx/core";
1620
import type { FastifyPluginAsync } from "fastify";
21+
import { getAllIntegrations } from "../integration-host.js";
1722
import { getPlaceKnowledge } from "../services/knowledge/index";
1823
import { buildReviewLinks } from "../services/review-links";
1924
import { TTL, withCache } from "../utils/cache.js";
@@ -51,11 +56,12 @@ async function safeAggregate(
5156
lat: number,
5257
lng: number,
5358
name: string,
59+
providers: ReviewProvider[],
5460
): Promise<{ stars: number; count: number } | null> {
5561
if (!name) return null;
5662
try {
5763
const result = await Promise.race([
58-
fetchAggregate({ lat, lng, name }),
64+
fetchAggregate({ lat, lng, name }, providers),
5965
new Promise<null>((resolve) => setTimeout(() => resolve(null), 1500)),
6066
]);
6167
if (!result || result.count < 3 || result.stars <= 0) return null;
@@ -78,10 +84,15 @@ async function enrichPlace(place: Place, lang: string | undefined): Promise<Plac
7884
...knowledge
7985
} = await getPlaceKnowledge(place, lang);
8086
const enriched = foldExternalIdsIntoPlace(place, externalIds);
81-
const heroPhotos = enriched.osmTags ? await searchHeroPhotos(enriched.osmTags) : [];
87+
const allIntegrations = getAllIntegrations();
88+
const photoProviders = getPhotoProviders(allIntegrations);
89+
const reviewProviders = getReviewProviders(allIntegrations);
90+
const heroPhotos = enriched.osmTags
91+
? await searchHeroPhotos(enriched.osmTags, photoProviders)
92+
: [];
8293
const photos = deduplicatePhotos([...heroPhotos, ...(knowledgePhotos ?? [])]);
8394
const [plng, plat] = enriched.coordinates;
84-
const reviewStats = await safeAggregate(plat, plng, enriched.name);
95+
const reviewStats = await safeAggregate(plat, plng, enriched.name, reviewProviders);
8596
return {
8697
...enriched,
8798
...knowledge,

‎integrations/photos/__tests__/orchestrator.test.ts‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
import type { LoadedIntegration, PlacePhoto } from "@openmapx/core";
2+
import { describe, expect, it, vi } from "vitest";
3+
import { getPhotoProviders, searchHeroPhotos } from "../orchestrator.js";
4+
import type { PhotoProvider } from "../types.js";
5+
6+
function photoProvider(id: string): PhotoProvider {
7+
return {
8+
id,
9+
name: id,
10+
search: vi.fn<PhotoProvider["search"]>().mockResolvedValue([]),
11+
};
12+
}
13+
14+
function loadedIntegration(
15+
id: string,
16+
domains: string[],
17+
enabled: boolean,
18+
providers: PhotoProvider[],
19+
): LoadedIntegration {
20+
return {
21+
id,
22+
manifest: { id, domains },
23+
config: {},
24+
directory: `/integrations/${id}`,
25+
isBuiltIn: true,
26+
enabled,
27+
providers: new Map([["photos", providers]]),
28+
strings: {},
29+
shutdownHandlers: [],
30+
};
31+
}
32+
33+
describe("getPhotoProviders", () => {
34+
it("ignores disabled integrations and integrations outside the photos domain", () => {
35+
const enabledPhoto = photoProvider("enabled-photo");
36+
const disabledPhoto = photoProvider("disabled-photo");
37+
const wrongDomain = photoProvider("wrong-domain");
38+
39+
const providers = getPhotoProviders([
40+
loadedIntegration("photos-enabled", ["photos"], true, [enabledPhoto]),
41+
loadedIntegration("photos-disabled", ["photos"], false, [disabledPhoto]),
42+
loadedIntegration("reviews-only", ["reviews"], true, [wrongDomain]),
43+
]);
44+
45+
expect(providers.map((p) => p.id)).toEqual(["enabled-photo"]);
46+
});
47+
48+
it("sorts street-level imagery providers after editorial providers", () => {
49+
const mapillary = photoProvider("mapillary");
50+
const flickr = photoProvider("flickr");
51+
const panoramax = photoProvider("panoramax");
52+
const wikimedia = photoProvider("wikimedia-commons");
53+
54+
const providers = getPhotoProviders([
55+
loadedIntegration("a", ["photos"], true, [mapillary, flickr]),
56+
loadedIntegration("b", ["photos"], true, [panoramax, wikimedia]),
57+
]);
58+
59+
expect(providers.map((p) => p.id)).toEqual([
60+
"flickr",
61+
"wikimedia-commons",
62+
"mapillary",
63+
"panoramax",
64+
]);
65+
});
66+
});
67+
68+
describe("searchHeroPhotos", () => {
69+
it("returns OSM image tag photos without registered providers", async () => {
70+
const photos = await searchHeroPhotos(
71+
{
72+
image: "https://example.com/place.jpg",
73+
"image:0": "File:Example.jpg",
74+
},
75+
[],
76+
);
77+
78+
expect(photos).toEqual<PlacePhoto[]>([
79+
{
80+
url: "https://example.com/place.jpg",
81+
attribution: "OpenStreetMap",
82+
source: "osm",
83+
pageUrl: undefined,
84+
},
85+
{
86+
url: "https://commons.wikimedia.org/wiki/Special:FilePath/Example.jpg?width=1200",
87+
attribution: "OpenStreetMap",
88+
source: "osm",
89+
pageUrl: undefined,
90+
},
91+
]);
92+
});
93+
});

‎integrations/photos/index.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,7 @@
11
import type { IntegrationContext } from "@openmapx/core";
2-
import { initPhotosOrchestrator, resolveOsmTags, searchPhotos } from "./orchestrator.js";
2+
import { getPhotoProviders, resolveOsmTags, searchPhotos } from "./orchestrator.js";
33

44
export function setup(ctx: IntegrationContext): void {
5-
initPhotosOrchestrator(ctx);
65
ctx.registerRoute("GET", "/search", async (req, reply) => {
76
const lat = Number.parseFloat(req.query.lat);
87
const lng = Number.parseFloat(req.query.lng);
@@ -31,7 +30,8 @@ export function setup(ctx: IntegrationContext): void {
3130
if (placeId) {
3231
osmTags = await resolveOsmTags(placeId, name, lat, lng);
3332
}
34-
return searchPhotos({ lat, lng, name, limit, osmTags });
33+
const providers = getPhotoProviders(ctx.getIntegrationsByDomain("photos"));
34+
return searchPhotos({ lat, lng, name, limit, osmTags }, providers);
3535
});
3636

3737
reply.header("Cache-Control", "public, max-age=3600");

‎integrations/photos/orchestrator.ts‎

Lines changed: 13 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,19 @@
1-
import type { IntegrationContext, PlacePhoto } from "@openmapx/core";
1+
import type { LoadedIntegration, PlacePhoto } from "@openmapx/core";
22
import { parseId } from "@openmapx/core";
33
import { lookupByNameAndCoords, lookupByOsmRef } from "../geocoding/place-lookup.js";
44
import type { PhotoProvider, PhotoQuery } from "./types.js";
55

6-
let _ctx: IntegrationContext | null = null;
7-
8-
/** Must be called during setup() to provide the integration context. */
9-
export function initPhotosOrchestrator(ctx: IntegrationContext): void {
10-
_ctx = ctx;
11-
}
12-
136
// Providers in this list are ordered last (street-level imagery — shown after editorial photos)
147
const DEPRIORITIZED_PROVIDER_IDS = new Set(["mapillary", "panoramax"]);
158

169
/**
17-
* Collect photo providers from all integrations registered under the "photos" domain.
10+
* Collect photo providers from integrations registered under the "photos" domain.
1811
* Street-level imagery providers (mapillary, panoramax) are sorted to the end.
1912
*/
20-
function getPhotoProviders(): PhotoProvider[] {
21-
if (!_ctx)
22-
throw new Error("Photos orchestrator not initialized — call initPhotosOrchestrator first");
13+
export function getPhotoProviders(integrations: LoadedIntegration[]): PhotoProvider[] {
2314
const providers: PhotoProvider[] = [];
24-
for (const integration of _ctx.getIntegrationsByDomain("photos")) {
15+
for (const integration of integrations) {
16+
if (!integration.enabled || !integration.manifest.domains.includes("photos")) continue;
2517
for (const p of (integration.providers.get("photos") ?? []) as PhotoProvider[]) {
2618
providers.push(p);
2719
}
@@ -42,9 +34,10 @@ function getPhotoProviders(): PhotoProvider[] {
4234
*
4335
* Never throws — individual provider failures are silently dropped.
4436
*/
45-
export async function searchPhotos(query: PhotoQuery): Promise<PlacePhoto[]> {
46-
const providers = getPhotoProviders();
47-
37+
export async function searchPhotos(
38+
query: PhotoQuery,
39+
providers: PhotoProvider[],
40+
): Promise<PlacePhoto[]> {
4841
const totalLimit = query.limit ?? 20;
4942
const perProvider = Math.max(6, Math.ceil(totalLimit / Math.max(providers.length, 1)));
5043

@@ -67,9 +60,10 @@ export async function searchPhotos(query: PhotoQuery): Promise<PlacePhoto[]> {
6760
* Only calls providers that support searchByTags — no geo-search, no coordinate queries.
6861
* Never throws — individual provider failures are silently dropped.
6962
*/
70-
export async function searchHeroPhotos(osmTags: Record<string, string>): Promise<PlacePhoto[]> {
71-
const providers = getPhotoProviders();
72-
63+
export async function searchHeroPhotos(
64+
osmTags: Record<string, string>,
65+
providers: PhotoProvider[],
66+
): Promise<PlacePhoto[]> {
7367
// OSM image tags first
7468
const imageTagPhotos = await extractImageTagPhotos(osmTags);
7569

‎integrations/reviews/__tests__/orchestrator.test.ts‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
import type { LoadedIntegration } from "@openmapx/core";
2+
import { describe, expect, it, vi } from "vitest";
3+
import { getReviewProviders } from "../orchestrator.js";
4+
import type { ReviewProvider } from "../types.js";
5+
6+
function reviewProvider(id: string): ReviewProvider {
7+
return {
8+
id,
9+
name: id,
10+
getReviews: vi.fn<ReviewProvider["getReviews"]>().mockResolvedValue([]),
11+
getAggregate: vi.fn<ReviewProvider["getAggregate"]>().mockResolvedValue({
12+
count: 0,
13+
opinionCount: 0,
14+
positiveCount: 0,
15+
confirmedCount: 0,
16+
quality: 0,
17+
stars: 0,
18+
}),
19+
};
20+
}
21+
22+
function loadedIntegration(
23+
id: string,
24+
domains: string[],
25+
enabled: boolean,
26+
providers: ReviewProvider[],
27+
): LoadedIntegration {
28+
return {
29+
id,
30+
manifest: { id, domains },
31+
config: {},
32+
directory: `/integrations/${id}`,
33+
isBuiltIn: true,
34+
enabled,
35+
providers: new Map([["reviews", providers]]),
36+
strings: {},
37+
shutdownHandlers: [],
38+
};
39+
}
40+
41+
describe("getReviewProviders", () => {
42+
it("ignores disabled integrations and integrations outside the reviews domain", () => {
43+
const enabledReview = reviewProvider("enabled-review");
44+
const disabledReview = reviewProvider("disabled-review");
45+
const wrongDomain = reviewProvider("wrong-domain");
46+
47+
const providers = getReviewProviders([
48+
loadedIntegration("reviews-enabled", ["reviews"], true, [enabledReview]),
49+
loadedIntegration("reviews-disabled", ["reviews"], false, [disabledReview]),
50+
loadedIntegration("photos-only", ["photos"], true, [wrongDomain]),
51+
]);
52+
53+
expect(providers.map((p) => p.id)).toEqual(["enabled-review"]);
54+
});
55+
56+
it("preserves registration order for primary-provider selection", () => {
57+
const first = reviewProvider("first");
58+
const second = reviewProvider("second");
59+
const third = reviewProvider("third");
60+
61+
const providers = getReviewProviders([
62+
loadedIntegration("a", ["reviews"], true, [first, second]),
63+
loadedIntegration("b", ["reviews"], true, [third]),
64+
]);
65+
66+
expect(providers.map((p) => p.id)).toEqual(["first", "second", "third"]);
67+
});
68+
});

‎integrations/reviews/index.ts‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import {
33
cacheKeyForSubject,
44
fetchAggregate,
55
fetchReviews,
6-
initReviewsOrchestrator,
6+
getReviewProviders,
77
submitReview,
88
uploadReviewImage,
99
} from "./orchestrator.js";
@@ -31,8 +31,6 @@ function parseSubject(query: Record<string, string>): ReviewSubject | null {
3131
}
3232

3333
export function setup(ctx: IntegrationContext): void {
34-
initReviewsOrchestrator(ctx);
35-
3634
/** GET /reviews — list reviews for a subject (Redis-cached). */
3735
ctx.registerRoute("GET", "/reviews", async (req, reply) => {
3836
const subject = parseSubject(req.query);
@@ -42,7 +40,10 @@ export function setup(ctx: IntegrationContext): void {
4240
}
4341
const key = `cache:reviews:list:${cacheKeyForSubject(subject)}`;
4442
try {
45-
const reviews = await ctx.cache.withCache(key, READ_TTL_SECONDS, () => fetchReviews(subject));
43+
const reviews = await ctx.cache.withCache(key, READ_TTL_SECONDS, () => {
44+
const providers = getReviewProviders(ctx.getIntegrationsByDomain("reviews"));
45+
return fetchReviews(subject, providers);
46+
});
4647
// Redis handles server-side caching; tell the browser to always revalidate
4748
// so post-write invalidations (edits, deletes) are visible immediately.
4849
reply.header("Cache-Control", "no-store");
@@ -62,9 +63,10 @@ export function setup(ctx: IntegrationContext): void {
6263
}
6364
const key = `cache:reviews:agg:${cacheKeyForSubject(subject)}`;
6465
try {
65-
const aggregate = await ctx.cache.withCache(key, READ_TTL_SECONDS, () =>
66-
fetchAggregate(subject),
67-
);
66+
const aggregate = await ctx.cache.withCache(key, READ_TTL_SECONDS, () => {
67+
const providers = getReviewProviders(ctx.getIntegrationsByDomain("reviews"));
68+
return fetchAggregate(subject, providers);
69+
});
6870
reply.header("Cache-Control", "no-store");
6971
reply.send({ aggregate });
7072
} catch (err) {
@@ -93,7 +95,8 @@ export function setup(ctx: IntegrationContext): void {
9395
return;
9496
}
9597
try {
96-
const result = await submitReview(jwt);
98+
const providers = getReviewProviders(ctx.getIntegrationsByDomain("reviews"));
99+
const result = await submitReview(jwt, providers);
97100
// Best-effort cache invalidation for this subject's reads.
98101
if (
99102
body?.invalidate?.lat !== undefined &&
@@ -162,7 +165,8 @@ export function setup(ctx: IntegrationContext): void {
162165
(typeof body.filename === "string" && body.filename.trim()) || `upload.${ext}`;
163166
const blob = new Blob([buffer], { type: normalizedMime });
164167
try {
165-
const result = await uploadReviewImage(blob, filename);
168+
const providers = getReviewProviders(ctx.getIntegrationsByDomain("reviews"));
169+
const result = await uploadReviewImage(blob, filename, providers);
166170
reply.send(result);
167171
} catch (err) {
168172
ctx.log.error("reviews:image upload failed", err);

0 commit comments

Comments
 (0)