Search places by address and any other OSM tag#99
Conversation
`rustfmt` on current stable reformats `xpub_balance`. Unrelated to any feature work, but `cargo fmt -- --check` in CI fails without it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rebuilds the undocumented GET /v4/search so that searching a city name finds the places addressed in that city, not just the places named after it. Places now match against every OSM tag *value* via json_each -- never a tag key -- requiring all query words to match, each possibly in a different tag. Localized name:* tags are searched for free. This ports the client-side search btcmap.org ran until 2025-07-31, which matched Object.values(tags). Areas match name and alias only: their tags hold the full geo_json polygon, so matching tag values there would substring-match coordinate digits. Ranking, LIMIT and OFFSET move into SQL. The handler previously loaded every match into memory before paginating, which is untenable once a query like "yes" matches every payment:*=yes place. Each side returns its own top offset+limit rows and the handler merges them, so pagination stays globally correct. Areas precede places at equal rank; optional lat/lon break remaining ties by proximity. Fixed in passing: - LIKE wildcards were never escaped, so q=% matched every row. - SearchedPlace::from panics on an element with NULL coordinates. - Area search deserialised every matching geo_json polygon. The existing select_by_search_query functions are untouched. JSON-RPC search shares them, and btcmap-admin's area picker depends on that. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GET /v4/search was undocumented. It is now a supported endpoint. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Worth using this rather than rolling our own? |
(Claude here, drafted for @escapedcat — he asked me to dig into this and reply.) Good instinct, and you're right that there's a performance problem — but measuring it at production scale (28,802 places, benchmarked with 30k) turned up a twist worth knowing before we reach for FTS5. The slowness isn't the text matching — it's the JSON. We re-parse every merchant's tag blob on every query via
So this PR is ~3× slower than master, and FTS5's default tokenizer would also be a regression. It indexes whole words, so it can't match inside one: Matching an infix is exactly what the old client-side search did ( Where FTS5 genuinely wins: it's the only way to make Proposal: land the Numbers are in-memory SQLite; prod is on-disk WAL so absolutes will shift, but the ratios should hold. Happy to add the column to this PR or as a follow-up — @escapedcat's call. |
Both search queries ordered by (rank, [distance], name_len, name) with no unique final key, and the in-memory merge did the same. Rows tied on every key came back in arbitrary order, and since consecutive pages are independent SQL runs, a tied row could land on two pages or none. Append id as the last ORDER BY term in both queries and the last comparator in the merge, mirroring each other so the merge-and-reslice stays correct. The new test pages one row at a time and asserts every id appears exactly once — the previous test only checked page sizes, so it missed this. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
offset is clamped to MAX_OFFSET (10000), but has_more was computed from the true total_count. For a broad query matching more than the cap, a client paging until has_more is false reached offset=10000, was told has_more=true, requested the next page, got clamped back to 10000, and looped forever — with the rows past the cap unreachable. Compute has_more via has_next_page, which is false once the next page would fall past the cap. Extracted as a pure function so the boundary is unit-tested without inserting 10000 rows. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The response shows name(Some("en")) — name:en when present, else the raw name —
but ranking and ordering used the raw $.tags.name. Two bugs: a place whose
name:en exactly matches the query was demoted to an "other tag" hit (rank 3)
while shown under its exact-matching English name; and because the merge-and-
reslice needs each side's DB order to equal the in-memory order, a place whose
localized name places it inside the global page but whose raw name sorts it
outside the fetched window could be dropped at a page boundary.
Rank and order on COALESCE(NULLIF(name:en,''), name) — the same string the
response displays. The predicate's IS NOT NULL gate stays on the raw name.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Swap the German Hamburg/Kaffeeklatsch sample for Prague/Bitcoin Coffee. Also fix the address to house-number-first (43 Lange Reihe -> 5 Wenceslas Square Prague), matching the actual address() output and the places.md examples; the old sample rendered it street-first. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR rebuilds GET /v4/search into a single “omnisearch” endpoint that searches both areas and places, expanding place matching from tags.name to any OSM tag value and adding deterministic, SQL-driven ranking/pagination (with optional proximity tie-breaking). It also documents the endpoint in the v4 REST docs.
Changes:
- Added shared search utilities (
escape_like,split_words) with tests to safely buildLIKEpatterns and cap query word count. - Implemented new DB search/count queries for areas and elements (tag-value search) with ranking and row limiting done in SQL.
- Reworked the
/v4/searchREST handler to merge ranked area+place results, enforce input constraints, and publish docs.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/service/wallet.rs | rustfmt-only change to keep formatting consistent with current stable toolchain. |
| src/service/search.rs | Introduces LIKE escaping + query word splitting helpers (with tests) for new search SQL. |
| src/service/overpass.rs | Adds a test-only helper to create mock elements with multiple tags. |
| src/service/mod.rs | Exposes the new service::search module. |
| src/rest/v4/search.rs | Replaces prior search behavior with merged ranked area+place omnisearch, SQL-backed pagination, and new response shape. |
| src/db/main/element/queries.rs | Adds async wrappers for new tag-value search and count queries + re-exports RankedElement. |
| src/db/main/element/blocking_queries.rs | Implements ranked tag-value search/count SQL with wildcard escaping and tie-breaking. |
| src/db/main/area/queries.rs | Adds async wrappers for new area search/count queries + re-exports RankedArea. |
| src/db/main/area/blocking_queries.rs | Implements ranked area search/count SQL (name/alias only) and bbox reporting behavior. |
| docs/rest/v4/search.md | Documents the new /v4/search omnisearch behavior, parameters, ordering, and response types. |
| docs/rest/v4/README.md | Adds the Search endpoint to the v4 documentation index. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| let response = SearchResponse { | ||
| results: paginated_results, | ||
| let total = total.max(0) as u32; |
There was a problem hiding this comment.
Addressed in 2d5d38d — clamped the conversion so total_count/pagination.total saturate instead of wrapping past u32::MAX (unreachable in practice, since total is bounded by the table size, but cheap to make obviously correct). — drafted with Claude.
total sums COUNT(*) results as i64 and was cast with `as u32`, which truncates past u32::MAX. Unreachable in practice — total is bounded by the table size — but clamp so total_count/pagination.total can never return a wrapped value. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(search): match address and other OSM tags via /v4/search Points the worldwide search proxy at the new omnisearch endpoint, so typing a city name finds the places addressed in that city. Localized name:* tags are searched for free. The proxy still returns a bare Place[], so the map route and merchantListStore are untouched. The `fields` param is dropped: /v4/places/search never read it (SearchArgs has no `fields` field), so nothing is lost. Depends on teambtcmap/btcmap-api#99. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(search): rank search results by distance from map centre /v4/search breaks relevance ties by proximity when given lat/lon. Without it, a query like "hamburg" leaves every match at the lowest rank, and the 100-row cap selects among them alphabetically rather than by distance. Restores the ranking the client-side search had before it was removed in #276, which sorted purely by mapCenter.distanceTo(element). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(search): surface truncation, preserve relevance order, wrap longitude Three fixes from review of the /v4/search switch. Relevance order never reached the user. openWithSearchResults re-sorted the server's results with sortMerchants — boosted first, then distance from the user's geolocation, falling back to alphabetical when geolocation is absent — which discarded the server's ranking entirely. Searching "coinmat" listed the ~75 "Bitcoinmat" places before the ones actually named "Coinmat". Now only boosted places are promoted; the server's order (exact name > prefix > substring > other tag, then proximity) is preserved beneath them via a stable sort. Truncation was silent, and this was a regression. The old /v4/places/search was unbounded — "str" returned all 453 name matches. /v4/search caps limit at 100, and matching every tag value widens broad queries enormously ("str" now hits addr:street, so 6171 matches). The proxy returned a bare Place[] and dropped total_count, so the panel showed 100 rows and labelled them "100 results". The proxy now returns { places, total, hasMore } and the panel reads "100 of 6,171 results". map.getCenter().lng is unwrapped, so panning across the antimeridian yields a longitude like 190, which the API would take verbatim as the distance origin. wrap() normalises it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(e2e): match search requests by param, not raw URL shape The search-race specs broke on three assumptions this branch invalidated: - The mocks returned a bare array, but the proxy now returns { places, total, hasMore }. Destructuring left `places` undefined, so openWithSearchResults threw "Cannot read properties of undefined". - The matchers used url.endsWith('...?name=kiosk'), but the request now also carries lat/lon, so the URL no longer ends at the name. - The full-phrase waiter looked for "kiosk%20hamburg", but the request is now built with URLSearchParams, which encodes the space as "+". Match on the decoded `name` param instead, which is immune to param order and encoding while still telling "kiosk" and "kiosk hamburg" apart exactly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * i18n(search): localize the result-count strings The three result-count strings in the panel were hardcoded English in an otherwise fully localized component — and search.resultsCount already existed, translated in all 9 locales, but was never wired up. Wire up resultsCount for the plain count and add resultsCountOf ("{shown} of {total} result(s)") for the two "X of Y" forms — the category-filtered count and the truncated-total count. Each locale's new string reuses that file's existing result(s) noun, so diacritic/entity style matches it exactly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(store): use a type alias for MerchantListState CLAUDE.md asks that existing interfaces migrate to type aliases when the file is touched. This PR adds searchTotal to MerchantListState, so convert it. No declaration merging or implements relies on it — it is only used as an annotation, a generic constraint and a writable<T> parameter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(map): refresh the nearby list after search moves the map Clicking a search result flies the map, but updateMerchantList early-returns in search mode, and close() deliberately keeps the old merchant data so the count survives on the collapsed facade. So on returning to nearby, the list and its "N nearby" count still described wherever the user was before searching: search Hamburg from Helsinki, click the result, and it reported Helsinki's merchants over a Hamburg map until the map was next panned. Watch for the search -> nearby transition and refresh once. This covers every exit (clearing the input, closing the panel, exitSearchMode), so the explicit refresh in handlePanelSearch is now redundant and removed. Not fixed by dropping the early-return: setMerchants also rewrites categoryCounts/selectedCategory, which the search panel's category chips own. Pre-existing since #1003, but the new address-matching search makes it easy to hit — searching a distant city and flying there is now a normal thing to do. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rebuilds the undocumented
GET /v4/searchinto an omnisearch over areas and places.Searching
hamburgnow returns the Hamburg area and the places whoseaddr:cityisHamburg. Localized
name:*tags are searched for free.This is the server-side version of the client-side search
btcmap.orgran until 2025-07-31,which matched
Object.values(tags)— every OSM tag value, never a key. It was removed in01127138during the v4 Places migration and never replaced; the API has only ever matched$.tags.name.Per @bubelov's steer in chat:
/v4/places/searchstays scoped to places and is nottouched; the omnisearch lives on the global endpoint.
What changed
json_each— never a key, soq=addrmatchesnothing. All query words must match, each possibly in a different tag, so
q=hamburg cafemeans a place with
addr:city=Hamburgandcuisine=cafe. (The old client-side versionwas an any-word OR.)
equal rank. Optional
lat/lonbreak remaining ties by proximity — this matters, becausea query like
hamburgleaves thousands of places tied at the lowest rank.LIMITandOFFSETmoved into SQL. The handler previously loaded every matchinto a
Vec<Element>before slicing 20 rows — untenable onceq=yesmatches everypayment:*=yesplace. Each side fetches its own topoffset+limit; the handler merges.tagshold the fullgeo_jsonpolygon, so matchingtag values there would substring-match coordinate digits.
bbox, so a client can fly the map to a city. The world-rectangledefault is reported as absent rather than inviting a zoom-to-planet.
docs/rest/v4/search.md. It was previously undocumented.Compatibility
GET /v4/places/searchis unchanged.searchis unchanged. The new matching lives in new DB functions, sobtcmap-admin's area picker is unaffected.
git difftouches zero lines insrc/rpc/search.rsand adds/removes zero lines mentioning
select_by_search_query.typediscriminator for a place changes from"element"to"place", matching v4naming. Safe: no client calls this endpoint.
Defects fixed in passing
LIKEwildcards were never escaped, soq=%matched every row.LOWER(x) LIKE UPPER(?)only ever worked because SQLite'sLIKEis alreadyASCII-case-insensitive; the two cancel each other's intent.
SearchedPlace::fromdoesit.lat.unwrap()and panics on an element with NULLcoordinates. The new query excludes them. (The same latent panic remains on
/v4/places/search— out of scope here.)geo_jsonpolygon into aserde_json::Map.The first commit reformats
src/service/wallet.rs. It is unrelated, butrustfmton currentstable reformats
xpub_balanceandcargo fmt -- --checkfails on master without it.Known limitations
q=cafedoes not matchCafé. SQLite'sLIKEfolds ASCII case only and there is no ICUcollation; the old client-side
toLowerCase()had the identical limitation. Fixing it meansFTS5 with
unicode61 remove_diacritics=2.per-row cost by the tag count. FTS5 over a precomputed search document is the fix if it bites.
does mean
q=yesmatches everypayment:*=yesplace andq=2025everycheck_date.Verification
cargo fmt --check,cargo clippy -- -D warningsandcargo testall pass (537 tests).31 new tests. Also exercised against a running server with seeded data:
q=hamburgreturnsthe area then the places;
q=hamburg cafenarrows to one;q=addrandq=%%%returnnothing;
q=ハンブルクmatches aname:jatag.🤖 Generated with Claude Code