refactor: reflection-based search mapping + location geopoint - #3345
Conversation
Discussion carried over from #2659Reposting the non-bot discussion from the original PR so it isn't lost with the move to this upstream branch. Quoted verbatim with attribution; the resolved bot review nits (codacy / copilot: embedded-pointer unwrap, Why reflection? (design rationale)
@dschmidt answered (click to expand)
@dschmidt on the diff size:
Related: #2715 Review feedback
Follow-up documentation issue opened by @fschade: #3092 @aduffeck (workflow):
Open item: breaking index change / re-indexing (unresolved)
@aduffeck (on
Resolved: ginkgo test conversion@aduffeck (on
@dschmidt: Converted geo_verify + mtime and the new mapping package tests to ginkgo/gomega. (This carried into the rebase: the opensearch backend suite is now ginkgo too.) Open item: Mtime typed as a date (unresolved)@dschmidt (on
An earlier symptom of the same typing (@aduffeck spotted the fixture choking the unit tests with a |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 17 |
| Duplication | 15 |
🟢 Coverage 74.90% diff coverage · +0.54% coverage variation
Metric Results Coverage variation ✅ +0.54% coverage variation (-1.00%) Diff coverage ✅ 74.90% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (0becbb8) 84824 19295 22.75% Head commit (1bf8130) 85264 (+440) 19853 (+558) 23.28% (+0.54%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#3345) 1203 901 74.90% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
a7ed42f to
e5c12a9
Compare
aduffeck
left a comment
There was a problem hiding this comment.
Needs a rebase but the code looks good to me. I also tested with both opensearch and bleve and didn't find any issue (except for the favorite flag issue which was fixed in #3252 and thus should be fixed here after a rebase as well.
b49d0ab to
240a80f
Compare
e4724ac to
9503884
Compare
Build the bleve and OpenSearch index mappings from the Go struct via reflection (json tags + per-field overrides) instead of hand-rolled mappings and hit deserializers. New mapping package: BleveBuildMapping, OpenSearchBuildMapping, Deserialize[T], PrepareForIndex; field decoding is fail-soft. Mtime is typed as a date so mtime ranges are chronological on both backends. Route CS3 facet parsing through mapping.DeserializeStringMap. The any-valued (bleve hit) and string-valued (CS3 metadata) deserializers share one generic fillStruct walker with a per-value setLeaf callback.
Add a TypeGeopoint field type. The libregraph Location facet is kept as an
object (retrieval / numeric queries) and a sibling <name>_geopoint field
carries the {lat,lon} form for geo-distance / bbox / polygon queries,
uniform across bleve and OpenSearch via the shared mapping. PrepareForIndex
splices the sibling in at write time.
Mtime is now a date field; the fixture's Go-format string fails OpenSearch date parsing.
The package's engine suite is ginkgo; these new tests were plain.
New package, so use the repo's standard test framework.
The Mtime field is mapped as an OpenSearch `date`, which rejects an empty value with `mapper_parsing_exception: cannot parse empty date`. The folder and root fixtures had no Mtime, so serializing them to `"Mtime": ""` made TestEngine_Purge/purge_resource_trees fail when the document was indexed. Give both a valid RFC3339 Mtime, matching the file fixture.
Both backends carry a shared search.SchemaVersion in the index name (OpenSearch <base>-vN) and data path (bleve-vN). A breaking schema change bumps the version so the service builds a fresh index instead of colliding with the incompatible previous one; the old index is left in place.
A single word finds the names and titles that contain it, `report` finds Report.txt, on bleve as well as on OpenSearch, which so far only did it by accident of its dynamic mapping. Modelled like SharePoint's NoWordBreaker: a keyword field is one whole value unless the override switches that off, which adds a search-only _words sibling next to _lowercase, analyzed into lowercased words (a dot is a word boundary, no stemming). The base stays the whole value for returning and aggregating, wildcards and whole values keep using _lowercase. Quotes do not change the meaning, a phrase is a phrase either way, and there is no exact-match operator yet.
KQL searches case-insensitively, so every keyword field gets its lowercased search sibling unless it opts out: ids are opaque, paths are POSIX, the mime type is normalized already. That takes the facets along, artist or camera model match regardless of case, while the case-preserved base still answers and aggregates. Which siblings a field carries is decided once, from the struct and the overrides, and the renderers, the document writer and the query lowering all read it from there.
SharePoint's default for a text property is word breaking, so ours is too: every keyword field gets the _words sibling unless it opts out with NoWordBreaker, which now carries SharePoint's polarity as well. Artist, album, camera model and the other facets match by word like name and title; tags and favorites stay one label, ids, paths and the mime type one value.
The hand-written v3 of the interim mapping is taken; both engines derive index name and directory from search.SchemaVersion.
Adopts the parity-pinned semantics from #3408: 'report' does not match 'reports'; the porter fulltext analyzer is gone.
…routing From #3408: hidden takes bool words only, type categories map to the stored value in the shared pass, ? counts as a wildcard, a non-suffix wildcard on a word-broken field forgives the extension, = matches the whole value on the lowercased sibling, paths lose their trailing slash. Dead compiler helpers removed.
Content:"reports monthly" must not match 'monthly reports'; the phrase runs on the analyzed field like on OpenSearch.
Typed Mtime fixtures, VersionedIndexName, and audio.artist matches case-insensitively now (facets search case-insensitively by default).
The engine suites keep what is engine-specific (index setup, health, purge-space batching); every behavior answer lives in the parity suite once. The shared test client learns IndicesCount, and FIELDS-15 pins the Size/Type gate for number queries.
Also make the reindex copy safe to run after the service already indexed (op_type create, conflicts proceed).
The file expansion is grouped again so its NOT stays atomic next to other terms (OpenSearch turned 'mediatype:file OR x' into '(NOT dir) AND x'), and the bleve compiler no longer re-keys resolved groups (a negated mediatype group targeted the raw 'mediatype' field and matched everything). Pinned as MEDIATYPE-07..10.
The OpenSearch _lowercase siblings are search-only like bleve's (no doc_values, own map instance instead of aliasing the base), and the dead Path_words field is gone: _words exists for keyword fields only, as SearchSiblings declares.
The delete-by-query matched the bare Path, so a purge could take same-path documents in other spaces with it; bleve was already RootID-scoped. Pinned as rootscope-04.
Same rule as the other ids; removing the sibling later would take another schema generation.
Copying the old index over misses the search sibling fields, copied documents would be unfindable.
Summary
Merges the bleve and OpenSearch index mappings into one reflection-based package (
services/search/pkg/mapping) driven by thesearch.Resourcestruct + a small overrides map. Pullsservices/graph's duplicated reflection walker onto the same helpers and, as a showcase of what the refactor enables, turns onlocation_geopointindexing for spatial queries on both backends.Adding a new facet (motionPhoto, ...) is now roughly: one struct field on
content.Document, one line inservice.go, one line in graph, one line per backend hit converter, plus whatever tika extraction logic the facet actually needs. Everything else falls out of reflection.Behavior changes (deliberate)
A new schema generation works on a fresh index (
opencloud-resource-v4,bleve-v4) and leaves the old one untouched;services/search/MIGRATION.mddescribes how to fill it.Favoritesmatches case-sensitively, like the other ids: the values are opaque user ids, so they carry no_lowercasesibling.An OpenSearch
Purgeis scoped to the resource's space; it matched the bare path before and could take same-path documents in other spaces with it (bleve was already scoped). Pinned as parityrootscope-04.OpenSearch
Tags/Favorites: dynamickeyword→ explicitkeyword(unified with bleve), searched case-insensitively via the_lowercasesiblings described below. No analyzer.OpenSearch facet sub-strings (
audio.*,photo.*,image.*): dynamictext + keywordmulti-field → explicitkeywordplus the_lowercasesibling every keyword field gets by default (see below), soaudio.artist:"iron maiden"finds "Iron Maiden" on both backends while aggregations keep case-preserving buckets. The tokenized path was never reachable from KQL anyway (no dot-syntax + pre-fix(search): preserve value case for non-lowercased bleve fields #2633 lowercasing), so no working query regresses.location: the libregraph{longitude, latitude, altitude}object is preserved at thelocationkey on both backends (numeric sub-field queries likelocation.latitude:>49keep working). A siblinglocation_geopointis added for geo-distance / bounding-box / polygon queries.path:queries are case-sensitive on both backends; Path gets no_lowercasesibling. Paths act as references (location scoping, deep links) where/Fooand/fooare distinct siblings, and case-insensitive folder discovery is served byname:. This matches bleve on main, which always matched paths case-sensitively; only OpenSearch loses its case-insensitive path matching. Dropping the sibling also removes its costliest maintenance: Path is the one mutable sibling field, so the OpenSearch move script no longer has to rebuild a lowercased copy for a whole subtree. With paths case-sensitive, the ref path scope is applied at query level on both backends (term/prefix on bleve's keywordPath, term filter on OpenSearch'spath_hierarchytokens), replacing the old post-filter: totals and paging now respect the scope instead of being computed over the whole space, and a wrong-cased scope matches nothing.graph facet parsing: fail-soft per field. A malformed value drops only that field; the rest of the facet still populates.
Audio facet is now shown whenever
libre.graph.audio.*metadata is present: the read-sideaudio/guard is dropped from all three readers (bleve, OpenSearch, graph), so the facet follows the metadata rather than re-checking the MIME type. Extraction still only produces audio metadata foraudio/*files.Keyword fields are searchable by word on both backends, the way KQL matches text properties: a single word finds the values that contain it (
reportandname:reportfindReport.txt,Title:quarterlyfinds "quarterly report",audio.artist:ironfinds "Iron Maiden"), which OpenSearch on main happened to do through its dynamic mapping and bleve never could. Modelled on SharePoint'sNoWordBreakerwith its default: every keyword field gets a search-only_wordssibling (lowercased words, a dot is a word boundary, no stemming) next to_lowercase, unless it opts out because its values are labels or identifiers:Tags,Favorites(a tag is one label,tag:foodoes not findfoo-bar),ID/RootID/ParentID,MimeType. The base stays the whole value for returning and aggregating, wildcards and whole values keep using_lowercase, and quotes do not change the meaning (a phrase is a phrase either way). KQL's=(grammar from fix(search): make openSearch and bleve behave the same #3408) matches the whole value: a case-insensitive term on_lowercase. Web is unaffected either way, it always searches wildcarded (name:"*term*", see webuseSearch.ts).Mtime is typed as a date on both backends, so
mtime:>...ranges are chronological (was a keyword field / lexicographic compare on OpenSearch). Note: bleve has no sub-second date field, so a returned/re-indexedMtimeis second-precision (range queries stay exact). Previously the RFC3339Nano keyword round-tripped exactly.Resource.Hiddennow survives Move/Delete/Restore on bleve. The old hand-rolled deserializer never readHidden, so those ops silently reset it tofalse; the reflection deserializer reads every field, preserving it. Latent-bug fix.Case-insensitive search via per-field
_lowercasesiblings, on by default. KQL searches case-insensitively, so every keyword field indexes its case-preserved base (returned to clients, aggregated on, used for exact ops like the move/delete cascade) plus a lowercased sibling used only for matching, unless it opts out: ids are opaque (ID,RootID,ParentID), paths are POSIX (Path), the mime type is normalized already (MimeType). Which siblings a field carries is decided once, inmapping.SearchSiblings, and the mapping renderers, the document writer and the query lowering all read it from there. Queries route to the sibling and lowercase the value with the same Gostrings.ToLowerused at index time, so index and query stay consistent without an analyzer. The sibling is never read back, so it need not be stored: in bleve it is not stored, out of_all, no doc values; in OpenSearch it deliberately stays in_source, because excluding it would force every update-by-query script (move/delete/restore) to rebuild all siblings from the document via painlesstoLowerCase, which lowercases differently than Go and would drift from the query side. A lowercased copy of a name/path is negligible disk in a cluster. The OpenSearch Move script keeps base and sibling in sync via Go-lowercased params, so case-insensitive search still finds a file after it moves (the sibling used to go stale); bleve re-indexes whole documents on move/delete/restore and stays fresh for free. Also fixes a latentmainbug: the Move/Delete cascade matchedPathexactly against the lowercased index, so re-homing a mixed-case folder (e.g./Photos) skipped its descendants;Pathis now case-preserved. bleve path queries additionally match a folder and its descendants like OpenSearch'spath_hierarchy.OpenSearch full-text (
content:) search now analyzes the query. Single-term queries used an unanalyzed term query, so once this refactor dropped the old blanket query-value lowercasing,content:Foo(any uppercase) missed on OpenSearch (a regression introduced here; bleve was unaffected because its query analyzes). Fielded full-text queries now use an analyzed match query.Contentis analyzed by the same words analyzer on both backends, without stemming, adopting the parity-pinned semantics from fix(search): make openSearch and bleve behave the same #3408 (bleve loses its porter stemmer):content:runningandcontent:RUNNINGmatch,content:rundoes not.To decide - content wildcards (
content:foo*) are unanalyzed on both backends, so they match the stemmed, lowercased term dictionary literally:content:run*finds a document containingRunning(indexed termrun), butcontent:running*(past the stem) andcontent:Run*(uppercase) do not. This is inherent to a wildcard over an analyzed field and is now consistent across bleve and OpenSearch (previously OpenSearch degradedcontent:foo*to an exact match). Whether content wildcards should additionally be case-folded is left open.mediatype search is now case-insensitive on both backends: categories (
mediatype:Folder,mediatype:IMAGE, ...) and literal MIME types are lowercased in the KQL lowering. Related behavior change: the rawMimeTypefield no longer expands category words. On mainMimeType:folder/MimeType:fileexpanded to the folder / non-folder MIME set (a quirk of field-based expansion); nowmediatype:is the way to query categories andMimeType:only matches a literal MIME type.Upgrade note: the index name carries
search.SchemaVersion, bumped to 3 here (OpenSearch<name>-v3, blevebleve-v3). An upgraded instance starts on a fresh, empty index and leaves the old one in place;opencloud search index --all-spacesfills it, then the old index can be removed. #3197 adds the startup schema check on top.Follow-ups (not in this PR)
NOT a OR bon OpenSearch: per [MS-KQL] (2.1.13, NOT has highest precedence) this is(NOT a) OR b, but a flat OpenSearch bool query cannot OR-combine amust_notclause, so it collapses toNOT a AND b. bleve represents it correctly (amust_notsub-query can be a disjunct). Rare (a NOT directly followed by OR) and a flat-bool limitation, not the NOT binding, which is now spec-correct on both backends.Media-type category + operator precedence:
X OR mediatype:<multi-type-category> AND Y(document/spreadsheet/presentation/archive) constrains only the last MIME type of the category, because the bleve compiler'smapBinaryredistributes a left disjunction as an OR-chain. Pre-existing (main had the same), a pure query-compiler issue.[]structround-trip: a json-tagged slice-of-struct field is mapped as a nested object but not read back (fillStruct/casing/geodo not descend into slices), and its_lowercase/_geopointsiblings would never be written. Latent: no current facet is a[]struct(facets are*struct, tags are[]string); harden before adding one.bleve
_all: unused (every query is fielded: the resolver always resolves a field, the compiler always emitsfield:value, and a bare term resolves toName→Name_lowercase). Disabling it shrinks the bleve index and turns the per-fieldIncludeInAllhandling into dead code to remove.KQL typed-value parity (booleans/numerics):
Hidden:T/Size:>1000are bleve query-string leakage: they parse as plain string restrictions and only work because the bleve compiler concatenates values into bleve's own query-string syntax; on OpenSearch the same literals hit typed fields and 400. The KQL-canonicalhidden:truecompiles to a query-stringHidden:trueon bleve and silently never matches (bleve indexes booleans as the term "T"); it should compile to a typed bool-field query. Canonical numeric ranges (Size>1000) don't parse at all, the grammar has range operators only for datetime values. Consolidate in the query layer: typed BooleanNode compile on bleve, grammar-level numeric ranges, then decide whether the query-string extras stay.?single-char wildcard parity: bleve treats?as a wildcard, OpenSearch does not (the wildcard check only looks for*), soname:Fo?diverges. Pre-existing, not introduced here.Field-name notation: KQL vs graph
sortProperties: thequeryStringexposes internal index field names (Name:foo*,Size:>1000,Tags:bar,Mtime:...), while the graph search endpoint'ssortPropertiesdeliberately accepts the property names a client sees on the hit resource (name,size,lastModifiedDateTime,mimeType,photo.takenDateTime, ...) and translates them to index fields via an alias table inpkg/search. Align the two surfaces: accept graph-notation field names in KQL (on top of that alias table) and decide whether the internal index names remain part of the public query surface.