Spotlight cnid search: honor every "word" of a multi-word Finder search query, quoted phrases, folder scope, and the results limit - #3271
Conversation
andylemin
changed the title
8d3afcc to
e2c741b
Compare
andylemin
changed the title
|
Working well so far. Found some minor edge cases which need fixing first |
45f4bc4 to
98df07f
Compare
|
Fixed some security issues where users could do |
2a42334 to
37d4db6
Compare
|
@rdmark This is a joy to use! It is fast, works intuitively, and works by default without user effort. DBD still has some limitation (is missing the new features), and we will not fix.. SQLite and MySQL work beautifully. Also updated the docs to better clarify; macOS searches network volumes through Finder search only, not the Spotlight menu-bar widget, which is reserved for local volumes — macOS client behaviour, not a Netatalk limitation. Ready for review |
rdmark
left a comment
rdmark
left a comment
There was a problem hiding this comment.
massive quality of life improvement!
85389d7 to
c97e555
Compare
…imit The cnid Spotlight backend served only a fraction of what Finder asks for: multi-word searches searched only the first word (Finder sends one predicate pair per word joined with ||, and extraction returned after the first supported predicate); quoted exact-phrase searches returned nothing (the closing-quote scan stopped at Finder's escaped \" phrase delimiters, extracting a lone backslash that fell under the minimum term length); folder-scoped searches returned matches from the entire volume (the kMDScopeArray scope the RPC layer already parsed was never consulted); and the result limit was ignored, while the option it came from, 'sparql results limit', meant something different in each of the other backends. Extraction now honors every filename predicate, requiring the '=' to directly follow the attribute key so a key occurring inside another predicate's quoted value cannot bind elsewhere, stopping each scan at the first unescaped quote and dropping phrase delimiters; one CNID search runs per term and the results are united, dropping CNIDs matched by more than one term. Phrase adjacency falls out of the existing substring search. The folder scope is enforced inside the CNID database via the new cnid_find_scoped(), taking the CNID of the scope directory (resolved once per query with cnid_for_path()): the sqlite and mysql schemes use a recursive CTE seeded with the scope DID, driving both the subtree enumeration and the name match off the (Did, Name) index; the dbd daemon checks each name-index match's ancestor chain against the scope with a per-request verdict cache, caching only walks that reached the scope or the root. The scope rides in the previously unused rqst.did field of the SEARCH op, so the dbd wire format is unchanged and version skew degrades to the unscoped behavior; a mysql server without CTE support, or one refusing the recursion depth at execute time, falls back to the unscoped query. Scope paths are canonicalised at parse time: trailing slashes are stripped, and the components below the volume root are converted from the client's UTF8-MAC form per component, leaving the volume path prefix untouched because it already holds volume-charset bytes that this volume's casefold and precomposition rules must not rewrite. A scope that is not inside the volume, or that escapes it via '..', falls back to the volume root rather than reaching cnid_for_path(), whose contract requires a path inside the volume, and an over-long scope is an error rather than a silent truncation to an ancestor. A path-prefix guard on resolved results covers what the database-side scope cannot: daemon version skew, the mysql fallback, and renames racing the query. Directory path resolution is memoized across results. The result limit option 'sparql results limit' is renamed 'spotlight results limit': it was never SPARQL-specific, and it now caps every search backend identically. The old name 'sparql results limit' remains accepted as a deprecated alias that logs a warning, and the webmin module registers the rename so an existing key is still shown and is cleaned up on save. The limit defaults to 10000 and 0 removes it for all backends; previously 0 meant unlimited for localsearch, 10000 for xapian, and nothing at all for the cnid backend, which could not return more than 10000 results by construction. A nonzero value is clamped to [100, 16000000] and a negative or unparsable one falls back to the default. The end-of-life dbd CNID scheme is excluded from the option and keeps a fixed 10000-result cap. Zero-result fetch replies pack CNIDs and filemeta as header-only containers, which the unmarshaller rejected as malformed, so a query with no matches errored instead of returning an empty set; accept the header-only encoding, add the empty CNID element to its container like the empty filemeta beside it, and guard the three RPC handlers that read the first CNID of a client-supplied array without checking that it holds one. cnid_find() keeps its public signature; the backend function-pointer signature gains the scope parameter. Adds spectest coverage for the query shapes, scoping (trailing-slash, non-ASCII, volume-escaping and foreign-volume scopes), short-term scanning and zero-result draining, a scoped-query testsuite helper, and updates NEWS, the manual's Search page and the afp.conf man page.
c97e555 to
2d025d5
Compare
|
📊 Performance DashboardCommit: 🔥 Spectest (AFP 3.4) - FlameGraphNetatalk Code-time: 2.9% · Runtime: 61s · Stacks: 1057 🔥 Click the preview to open the interactive flamegraph (zoom + search). 🔝 Top 10 leaf functions
📈 Speedtest (AFP 3.4) - PerfGraphPeak Read: 8624 MB/s (+20.3% vs hist avg 7167.8 MB/s; min 6309 / max 9526 over 30 PRs) 🔝 Throughputs per operation (vs. historical average)
⏱️ Lantest (AFP 3.4) - LatencyGraphAvg total runtime: 4492 ms (+7.3% vs hist avg 4186.3 ms; min 2122 / max 5208 over 30 PRs) 🐢 All operations (avg runtime, in test order, vs. historical average)
Run baseline: median op-test delta +14.0%, MAD 3.6%. Adj Δ% shifts each delta by the median; standouts ≥5% in bold. A large MAD means the run did not move uniformly — read the adjusted column with caution. |
It is especially good if you are using modern file managers like "Forklift file manager". Finder is quite clunky, as each time you use Finder search, you have to click on the folder you are in (it always defaults to searching the local Mac). File managers like Forklift and Pathfinder allow you to set Spotlight and "current folder" as the default. So you just work as normal, and when you search it automatically uses your current path context and Netatalk CNID, without extra clicks. It feels like a drop-in upgrade now. |
2 participants
The cnid Spotlight (Finder search) backend served only a fraction of what Finder asks for. Four defects:
two wordsinto one predicate pair per word joined with||; extraction returned after the first supported predicate, so the OR the client asked for was dropped."two words"arrives as*=="\"two words\"*"cdw; the closing-quote scan used a plainstrchr(…, '"'), stopped at the\"delimiter, and extracted a lone\— belowSL_CNID_MIN_TERMLEN, so zero results.kMDScopeArrayand the RPC layer parsed it, but this backend never consulted it.sparql results limit) meant something different in each of the other two.Fix
Extraction.
sl_cnid_extract_terms()collects up to 8 terms from every supported filename predicate (kMDItemFSName,kMDItemDisplayName,_kMDItemFileName, then*==only if no named attribute matched), requiring the=to directly follow the key so a key appearing inside another predicate's value cannot bind elsewhere.sl_cnid_quoted_value()scans to the first unescaped quote, strips wildcards, drops phrase delimiters and unescapes. One search runs per term and the results are united, dropping CNIDs matched by more than one term. Phrase adjacency needs no new machinery: the CNID search is a filename substring match, sotwo wordsonly matches adjacent occurrences.Scope, enforced in the database (for the supported sqlite and mysql schemes; see the dbd note below). Post-filtering an unscoped search cannot be correct: the candidate buffer caps at 10000 volume-wide matches, so on a large volume it fills with out-of-scope entries before an in-scope one is seen. The schema stores
(Id, Name, Did)— parent links, no paths — so the scope path is resolved once per query to its directory CNID withcnid_for_path(), and the newcnid_find_scoped()restricts matching to that subtree:CROSS JOIN/STRAIGHT_JOIN) so both the subtree walk and the name match drive off the(Did, Name)index: cost follows the subtree, not the volume. A server without recursive CTEs (MySQL < 8.0 / MariaDB < 10.2), or one refusing the recursion depth at execute time, falls back to the unscoped query.rqst.didfield of the SEARCH op so the wire format is unchanged and version skew degrades to unscoped behaviour in both directions. It gets none of the optimisation the SQL schemes do — BDB has no subtree index, so the scan is unchanged — and its error handling is deliberately left incomplete (see below). The supported schemes are sqlite and mysql.Scope paths are canonicalised at parse time: trailing slashes stripped, and the components below the volume root converted from the client's UTF8-MAC form per component — the volume path prefix is left untouched, since it already holds volume-charset bytes that this volume's casefold and precomposition rules must not rewrite. A scope that is not inside the volume, or that escapes it via
.., falls back to the volume root rather than reachingcnid_for_path(), whose contract requires a path inside the volume; an over-long scope is an error, not a silent truncation to an ancestor. A path-prefix check on resolved results covers what the database-side scope cannot: daemon version skew, the mysql fallback, and renames racing the query.The result limit — option renamed
sparql results limit→spotlight results limit. (It was never SPARQL-specific; the old name remains a deprecated alias that logs a warning, and the webmin module registers the rename so an existing key is shown and cleaned up). It now defaults to 10000 and means the same thing everywhere —0removes the limit for every backend, where previously0meant unlimited for localsearch, 10000 for xapian, and nothing at all for cnid, which could not exceed 10000 results by construction. A nonzero value is clamped to[100, 16000000]; negative or unparsable values fall back to the default. The end-of-life dbd scheme is excluded and keeps a fixed 10000 cap. The cnid candidate buffer is now sized from the limit, and for an unlimited search it starts at 10000 and grows until the database reports no further matches. Directory path resolution is memoized across results, cuttingcnid_resolve()round-trips for every multi-result search. Zero-result replies pack CNIDs and filemeta as header-only containers, which the unmarshaller rejected as malformed — accepted now, with the empty CNID element added to its container like the empty filemeta beside it, plus guards on the three RPC handlers that read the first CNID of a client-supplied array without checking it holds one, and an empty array is treated as no filter rather than one that matches nothing.cnid_find()keeps its public signature and behaviour; thecnid_dbbackend function-pointer for find gains the scope parameter (installed header — noted BREAKING in NEWS).Coverage
*=="\"two words\"*"cdwtwo words‖-joined per-word predicateskMDScopeArray)..Content queries (
kMDItemTextContentalone) still return zero results gracefully — the CNID database indexes names only, by design.Behaviour changes
sparql results limit→spotlight results limit. The old name still works and logs a deprecation warning, so existing configurations keep running; update them at your convenience.spotlight results limitnow defaults to 10000 and caps every backend identically;0is unlimited. Deployments that relied on the old unlimited-by-default behaviour should set0explicitly.afp.conf(5)and the developer indexing notes, referring to the feature as Spotlight (Finder search) so users know macOS searches network volumes through Finder search and not the menu-bar widget.Testing
test632 — seven Finder search query shapes against a three-file fixture: plain-space term, quoted phrase, per-word OR (a fixture file matching both terms proves dedup), the universal Finder form with
kMDItemTextContentpredicates that must be ignored, a key embedded in another predicate's value, and short-term-alone / short-term-then-valid.test633 — six Finder search scope phases: unscoped baseline, scoped to a subdirectory, trailing-slash scope, a scope named in the client's decomposed form against a precomposed on-disk name, a
..-escaping scope, and a scope naming another volume (plus one merely sharing a path prefix). The scope is sent through a newFPSpotlightOpenQueryScoped()helper that addskMDScopeArrayexactly as Finder does, deriving the server-side path fromFPSpotlightOpen's reply.Both were written first and recorded failing against the unmodified backend, then green after. Verified locally: full spectest 290/290;
FPSpotlightRPC8/8 on all three CNID schemes (dbd, sqlite, mysql with embedded MariaDB); 8/8 with a decomposed non-ASCII volume path; ASAN clean over the extractor across the escape, phrase, short-term and foreign-key inputs; clang-Werror, doxygen-strict and markdownlint gates clean.Unit tests cover the limit option end to end: unset, an explicit value, an explicit
0, a value below the minimum and one above the maximum, a negative and an unparsable value, an empty canonical key falling through to the alias, and canonical-beats-alias precedence, plus the deprecation warning.Standalone harnesses additionally cover extraction and union/dedupe, the per-term buffer reservation (a broad first term must not starve later words — verified failing before the fix and passing after), the scope boundary check (
/srv/afp2vs/srv/afp), the limit clamp, and the path memo. The scoped SQL was validated against the real cnid sqlite schema withEXPLAIN QUERY PLANconfirming both the recursion and the outer join use the(Did, Name)covering index.The limit was also verified behaviourally rather than by unit test alone: a limit of 20 is raised to the 100 minimum and truncates a 250-file corpus to exactly 100, an unlimited search grows the candidate buffer (
10000 → 80000 → …) until the database stops reporting matches, and the dbd cap fires only on dbd volumes.Not in this PR
InRange(…)) as a post-hoc filter from the per-resultstat().cdinsensitivity currently follows each CNID scheme's matching (SQLiteLIKEis ASCII-only; MySQL follows the table collation).afp_spotlightCLI over the same testsuite client library, usable as a live smoke test.cnid_dbdscheme: a BDB engine error during the ancestor walk is still treated as out-of-scope rather than failing the search, and its scoped search cannot reach subtree cost. dbd is not a target of this work.sl_rpc_fetchAttributeNamesForOIDArray(): theEC_CLEANUPpath dereferences the reply array whose allocation failure triggered the jump (out-of-memory only).%%{init: {'flowchart': {'curve': 'basis'}}}%% flowchart LR classDef wire fill:#1e3a8a,stroke:#1e40af,color:#eff6ff,stroke-width:1px; classDef extract fill:#5b21b6,stroke:#6d28d9,color:#f5f3ff,stroke-width:1px; classDef db fill:#0f766e,stroke:#0d9488,color:#f0fdfa,stroke-width:1px; classDef post fill:#b45309,stroke:#d97706,color:#fffbeb,stroke-width:1px; Q("Finder query + kMDScopeArray<br/><i>one predicate pair per word, ‖-joined;<br/>phrases in escaped quotes</i>"):::wire X("extract terms<br/>named attrs → *== fallback<br/>'=' must follow the key<br/>dedupe, max 8"):::extract S("scope → CNID<br/>canonicalise, convert below<br/>the volume root, clamp escapes<br/>cnid_for_path(), once"):::extract F("cnid_find_scoped() per term<br/>sqlite/mysql: recursive CTE on (Did, Name)<br/>dbd: ancestor check in the daemon"):::db U("unite + dedupe<br/><i>a name matching several<br/>words counts once</i>"):::post L("'spotlight results limit' cap<br/><i>0 = unlimited</i>"):::post R("CNID → path<br/>memoized ancestor walk<br/>scope guard + stat + access(R_OK)<br/>20 results per reply page"):::post Q --> X --> F Q --> S --> F F --> U --> L --> R