Repository navigation
feat(bazaar): add GET /discovery/search with relevance ranking and quality metrics (#129) - #527
Conversation
Miracle656
left a comment
There was a problem hiding this comment.
#129 is unassigned so this is yours, and the scoring design is reasonable — tiered weights by field, quality separated from relevance, and matchReasons so a caller can see why something ranked. That last one is a genuinely good idea and most search endpoints don't bother.
But the retrieval underneath it has a problem that the ranking can't fix.
1. It ranks an arbitrary 200 rows, not the catalog
resources = await prisma.bazaarResource.findMany({
where: { network: networkFilter, ...(typeFilter ? { type: typeFilter } : {}) },
take: 200, // Search sample space
})Two things follow from this, and both get worse as the catalog fills:
- It's a slice, not a search. Once there are more than 200 resources on a network, every query can only ever see 200 of them. A resource whose
serviceNameis an exact match for the query is simply absent if it fell outside that window — and the response gives no hint that happened. For an endpoint whose purpose is "find me the thing", silently not looking at most of the things is the one failure mode that matters. - The 200 isn't even stable. There's no
orderBy, so Postgres returns rows in whatever order it likes, and that can change between calls for reasons entirely unrelated to the query — a vacuum, a plan change, an insert. So the same search can return different results on two consecutive requests, with no way for a caller to tell why. That's worse than a wrong-but-consistent answer, because it isn't reproducible and therefore isn't debuggable.
The filtering has to happen in the database so the ranking applies to everything. queryDiscoveryResources in src/bazaar/catalog.ts already establishes the pattern for this route family — filters pushed into SQL, not applied in JS after the fact.
Simplest version that works: build the term predicates into the where (ILIKE across serviceName, description, url, and an array containment check for tags), so only candidate rows come back, and keep your scoring to order what the database already narrowed. If you want to go further, Postgres full-text search (to_tsvector / plainto_tsquery with ts_rank) does the matching and a first-pass rank in the query itself — but the ILIKE version is enough to close this and much easier to review.
Either way, add an explicit orderBy so the result is deterministic, and apply limit after ranking rather than before matching.
2. A database outage returns 200 with zero results
} catch {
// Return empty in mock/in-memory environments without Postgres
resources = []
}During a Postgres outage this answers "nothing matches your query" with a success status. A caller cannot distinguish an empty catalog from a broken one, so a client sees the catalog as genuinely empty and stops asking.
The reason given is the real issue though: this is production error handling written to accommodate the test harness. If the tests need a database that isn't there, the fix belongs in the test setup — mock prisma.bazaarResource.findMany, which is what src/__tests__/facilitatorSettle.test.ts does for its queries — not in the route. Let the error propagate, or catch it, log it and answer 503.
3. any where Prisma has types
calculateQualityScore(res: any), calculateRelevanceScore(query: string, res: any), let resources: any[]. Prisma generates a BazaarResource type for exactly this. With any, a renamed column — iconUrl to icon_url, say — compiles fine and silently scores every resource 0.1 lower forever, with no test failing. The scoring functions are the place you least want that, because a wrong score looks like a plausible score.
Smaller
networkFilter = req.query.network ?? activeNetworkis unvalidated, so an unknown network string returns an empty result rather than a400. The other routes resolve network throughsrc/middleware/network; worth using it here.- Relevance is summed per term then clamped with
Math.min(1.0, score), so any query matching three or more fields saturates at 1.0 and stops discriminating. Dividing by term count before clamping keeps multi-term queries comparable to single-term ones. // ... search with ranking & quality evaluation (#129)— house style here is comments that explain why rather than issue references; git history carries the number.
What's right
matchReasons is a good instinct — making ranking explainable rather than magic. Separating quality (metadata completeness) from relevance (query match) is the correct decomposition, and the quality baseline of 0.2 for merely existing is a sensible floor. Capping with Math.min(1.0, ...) keeps the scores interpretable as fractions. And limit is clamped to a sane range at both ends, which is more than most paginated endpoints do.
#129 stays yours. Move the matching into the query, let database failures be visible, and type res. Re-request review and I'll come back to it quickly.
Closes #129
Summary of Changes
GET /discovery/searchendpoint for Bazaar discoverable resources.serviceName,tags,description, andurl.openapi.yamland verified with comprehensive unit tests insrc/__tests__/discoverySearch.test.ts.