refactor: reflection-based search mapping + location geopoint - #2659
refactor: reflection-based search mapping + location geopoint#2659dschmidt wants to merge 22 commits into
Conversation
ae6414c to
531fd21
Compare
dschmidt
changed the title
Propose a single central Go-struct + overrides map as the source of truth for the search index layout across bleve and OpenSearch — the same definition drives the per-backend index mapping, the write-time adapter, the hit-decoding path, and the KQL compiler's case-folding rules, so the two backends cannot drift silently again. Also records the end-to-end case-handling principle for facet data (indexed as case-preserving keywords so aggregation buckets return correct display values), the sibling-field pattern for geopoint on Location, and the rationale for replacing the two backends' implicit defaults with an explicit, backend-agnostic contract. PR opencloud-eu#2659 is a proof-of-concept implementation the proposal emerged from; scope and APIs there will be revisited once this ADR lands.
Propose a single central Go-struct + overrides map as the source of truth for the search index layout across bleve and OpenSearch — the same definition drives the per-backend index mapping, the write-time adapter, the hit-decoding path, and the KQL compiler's case-folding rules, so the two backends cannot drift silently again. Also records the end-to-end case-handling principle for facet data (indexed as case-preserving keywords so aggregation buckets return correct display values), the sibling-field pattern for geopoint on Location, and the rationale for replacing the two backends' implicit defaults with an explicit, backend-agnostic contract. PR opencloud-eu#2659 is a proof-of-concept implementation the proposal emerged from; scope and APIs there will be revisited once this ADR lands.
Propose a single central Go-struct + overrides map as the source of truth for the search index layout across bleve and OpenSearch — the same definition drives the per-backend index mapping, the write-time adapter, the hit-decoding path, and the KQL compiler's case-folding rules, so the two backends cannot drift silently again. Also records the end-to-end case-handling principle for facet data (indexed as case-preserving keywords so aggregation buckets return correct display values), the sibling-field pattern for geopoint on Location, and the rationale for replacing the two backends' implicit defaults with an explicit, backend-agnostic contract. PR opencloud-eu#2659 is a proof-of-concept implementation the proposal emerged from; scope and APIs there will be revisited once this ADR lands.
Propose a single central Go-struct + overrides map as the source of truth for the search index layout across bleve and OpenSearch — the same definition drives the per-backend index mapping, the write-time adapter, the hit-decoding path, and the KQL compiler's case-folding rules, so the two backends cannot drift silently again. Also records the end-to-end case-handling principle for facet data (indexed as case-preserving keywords so aggregation buckets return correct display values), the sibling-field pattern for geopoint on Location, and the rationale for replacing the two backends' implicit defaults with an explicit, backend-agnostic contract. PR opencloud-eu#2659 is a proof-of-concept implementation the proposal emerged from; scope and APIs there will be revisited once this ADR lands.
Propose a single central Go-struct + overrides map as the source of truth for the search index layout across bleve and OpenSearch — the same definition drives the per-backend index mapping, the write-time adapter, the hit-decoding path, and the KQL compiler's case-folding rules, so the two backends cannot drift silently again. Also records the end-to-end case-handling principle for facet data (indexed as case-preserving keywords so aggregation buckets return correct display values), the sibling-field pattern for geopoint on Location, and the rationale for replacing the two backends' implicit defaults with an explicit, backend-agnostic contract. PR opencloud-eu#2659 is a proof-of-concept implementation the proposal emerged from; scope and APIs there will be revisited once this ADR lands.
|
Why the extensive usage of reflections? |
|
Short version: Keeping all the mappings in sync for the existing facets and having to do a lot of copy paste for adding new facets, is tedious and error prone. Long version
The alternative would be either codegen (more machinery for the same outcome) or keeping the three parallel hand-written schemas (which is what already doesnt work right now and what motivated the refactor). |
|
By the way looking at the line counts is a bit misleading. It's not 1.8k lines plus for a simple refactor. Yes, it's a bit more code for staying consistent and making further additions easier - but a lot of the new lines are also for tests we simply didn't have before and it also adds the geopoint feature (we can of course discuss to split it out of this PR if you prefer, it's basically a demonstrator for the concept) |
87589de to
a25f04a
Compare
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 348 |
| Duplication | 18 |
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.
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
The PR introduces a unified reflection-driven mapping system and spatial query support via 'location_geopoint'. While the architecture aligns with the goal of consolidating Bleve and OpenSearch logic, the implementation introduces critical risks. Specifically, the reflection walkers in the deserialization packages lack pointer handling for embedded structs, which will lead to runtime panics. \n\nFurthermore, although the codebase remains 'up to standards' according to Codacy, there are substantial increases in cyclomatic complexity in 'driveitems.go' (+114) and 'service.go' (+129). These core files should be monitored for maintainability. The change to drop entire facets upon encountering malformed metadata is a deliberate shift to ensure data integrity, though it differs from previous 'fail-soft' patterns. Note that existing indexes must be recreated to adopt the new 'location_geopoint' and analyzer changes.
About this PR
- This refactor introduces behavioral changes in indexing (e.g., custom analyzers and geopoint fields). Existing indexes will not be automatically migrated; users must recreate their indexes to benefit from these changes.
- Jira ticket key and description are missing from the PR metadata.
Test suggestions
- Geo-distance query on location_geopoint field in Bleve\n- [x] Numeric range queries on longitude, latitude, and altitude sub-fields\n- [x] Inference of mapping types (keyword, numeric, boolean, datetime) from Go struct fields\n- [x] Application of field mapping overrides (e.g., custom analyzers, explicit types)\n- [x] Deserialization of flat map[string]any (Bleve results) into nested Go structs\n- [x] Deserialization of flat map[string]string (CS3 metadata) into typed facets with error bubbling\n- [x] Verification that PrepareForIndex correctly adds sibling geopoint fields for OpenSearch/Bleve
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
f03a98e to
9e1e56b
Compare
|
relevant #2715 |
370ac9d to
4e8381a
Compare
There was a problem hiding this comment.
Pull request overview
This PR consolidates Bleve and OpenSearch index mapping generation into a shared, reflection-driven services/search/pkg/mapping package based on search.Resource + an overrides map, and extends indexing to support spatial queries by adding a *_geopoint sibling field (while preserving the original libregraph {longitude, latitude, altitude} object).
Changes:
- Introduce
services/search/pkg/mappingto infer field types via reflection, validate override keys, and build Bleve/OpenSearch mappings consistently. - Switch Bleve/OpenSearch indexing and Bleve hit decoding to use the shared mapping + (de)serialization helpers.
- Enable geo queries by adding
location_geopoint(and related serialization support) while keepinglocation.*numeric subfields intact.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| services/search/pkg/search/service.go | Refactors facet metadata flattening into a generic helper. |
| services/search/pkg/search/search.go | Adds JSON tags to Resource and introduces shared field override map for mapping generation. |
| services/search/pkg/query/bleve/compiler.go | Derives lowercase-query fields from mapping overrides to keep query tokenization aligned with index analyzers. |
| services/search/pkg/opensearch/internal/indexes/resource_v2.json | Removes static OpenSearch mapping JSON (replaced by generator). |
| services/search/pkg/opensearch/internal/indexes/resource_v1.json | Removes legacy static OpenSearch mapping JSON. |
| services/search/pkg/opensearch/internal/convert/opensearch.go | Simplifies facet conversion from indexed source to protobuf via a generic helper. |
| services/search/pkg/opensearch/index.go | Replaces embedded JSON templates with generated OpenSearch index mapping built from shared reflection mapping + overrides. |
| services/search/pkg/opensearch/batch.go | Uses mapping.PrepareForIndex to keep document shape in sync with generated mappings (incl. geopoint sibling). |
| services/search/pkg/mapping/opts.go | Defines mapping type constants and override options. |
| services/search/pkg/mapping/infer.go | Adds reflection-based type inference and shared field walking helpers (incl. embedded flattening). |
| services/search/pkg/mapping/infer_test.go | Tests type inference and field walking behavior. |
| services/search/pkg/mapping/validate.go | Adds override-key validation against reflected JSON-tag field names (incl. dotted paths). |
| services/search/pkg/mapping/validate_test.go | Tests override validation behavior. |
| services/search/pkg/mapping/bleve.go | Builds Bleve document mappings via reflection with support for object + geopoint sibling mapping. |
| services/search/pkg/mapping/bleve_test.go | Tests Bleve mapping inference/overrides and geopoint sibling mapping. |
| services/search/pkg/mapping/opensearch.go | Builds OpenSearch mapping properties via reflection with support for geopoint sibling mapping. |
| services/search/pkg/mapping/opensearch_test.go | Tests OpenSearch mapping inference/overrides and geopoint sibling mapping. |
| services/search/pkg/mapping/geo.go | Adds serialization support to splice {lat, lon} geopoint siblings into indexed documents. |
| services/search/pkg/mapping/geo_test.go | Tests geopoint sibling insertion behavior (top-level and dotted nested paths). |
| services/search/pkg/mapping/serialize.go | Adds a shared “prepare document for indexing” step (struct→map + geopoint sibling injection). |
| services/search/pkg/mapping/serialize_test.go | Tests serialization behavior for embedded flattening and omitempty handling. |
| services/search/pkg/mapping/deserialize.go | Adds reflection-based fail-soft deserialization from Bleve hit fields into typed structs. |
| services/search/pkg/mapping/deserialize_test.go | Tests fail-soft typed deserialization from Bleve hit fields. |
| services/search/pkg/mapping/deserialize_string.go | Adds reflection-based fail-soft deserialization from map[string]string (CS3 arbitrary metadata) into typed structs. |
| services/search/pkg/mapping/deserialize_string_test.go | Tests fail-soft string-map deserialization semantics. |
| services/search/pkg/content/content.go | Adds JSON tags to content.Document fields to align serialization and mapping generation. |
| services/search/pkg/bleve/index.go | Builds Bleve index mapping via the shared mapping package (reflection + overrides). |
| services/search/pkg/bleve/geo_verify_test.go | Adds end-to-end verification for location subfields + geopoint geo-distance queries in Bleve. |
| services/search/pkg/bleve/bleve.go | Switches resource reconstruction from Bleve hits to mapping.Deserialize. |
| services/search/pkg/bleve/batch.go | Uses mapping.PrepareForIndex before indexing into Bleve batches (incl. geopoint siblings). |
| services/search/pkg/bleve/backend.go | Uses mapping.DeserializeAt to populate facets from Bleve hit fields. |
| services/graph/pkg/service/v0/driveitems.go | Switches facet decoding from CS3 arbitrary metadata to mapping.DeserializeStringMap. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aa9a500 to
1c13f04
Compare
8c6a3b6 to
429a9c0
Compare
| Name string `json:"Name"` | ||
| Content string `json:"Content"` | ||
| Size uint64 `json:"Size"` | ||
| Mtime *time.Time `json:"Mtime"` |
There was a problem hiding this comment.
Mtime was the only Go string mapped as a date. Basic.Extract leaves it unset when the resource info carries no mtime (TSToTime(nil) is the zero time and it deliberately does not index a made up 0001-01-01), which serialised to "Mtime": "" -- and an empty string is not a date, so OpenSearch rejected the whole document. The bulk API reports that per item, so Batch.Push returned nil and the file silently vanished from the index (that swallowing is #3142).
Typing it as *time.Time fixes the cause rather than the symptom: inferType picks the date mapping up by itself, so the TypeDatetime override goes, and convert gets its time.Time without the time.Parse detour. Net -4 lines of production code.
Reproduced against a real cluster: before this, upsert without mtime indexed 1 of 2 documents.
|
Sorry for the long waiting time, since the PR contains a breaking change, it's just taking a bit longer than it should! Here's the problem: we can only ship the PR as a major version because the index is change breaking! Our idea is that in the case of breaking index changes (which will come sooner or later regardless of the PR), we can further guarantee that the instance is still running; we can do this by creating a new, fresh index and having the operator re-index the data. Downside: the search returns no results for some time, so we need the following beforehand:
|
|
I've worked on parts of that already, let's have a call about everything asap :) |
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.
6da4d8d to
831f8cf
Compare
…penSearch OpenSearch lowercased every KQL query value, so exact-match queries on case-preserved keyword fields (facet values, ids) never matched their stored token. Fold the value only for fields with a lowercasing analyzer, mirroring the bleve backend. The field set is derived once in search.LowercaseValueFields and shared by both backends (bleve's local buildLowercaseFields is dropped).
5f038d9 to
54f0d80
Compare
The KQL parser produced its own validation errors but imported them from the search service's query package. Move them into pkg/kql and let the search backend consume kql.IsValidationError, so the parser stops depending on a service package.
…ource struct mapping.FieldNameIndex walks the struct and maps a lowercased field path to the real field name, including nested facet sub-fields. Backend-neutral.
query.Normalize resolves field names (query.ResolveField, from the derived index + a small alias overlay) and expands media-type restrictions (mimetype.Expand) once, between parse and backend compilation.
The bleve Creator runs query.Normalize before compiling; the compiler consumes a canonical AST with no field resolution or media-type special-casing.
KQLToOpenSearchBoolQuery runs query.Normalize, then only value lowercasing stays backend-specific; remapKey and unfoldValue are gone.
85b3e01 to
6edf8f1
Compare
Keyword and path fields always index their case-preserved base and, when CaseInsensitive is set, an additional <field>_lowercase sibling used only for matching. The KQL lowering marks a restriction case-insensitive; each backend searches the sibling and lowercases the query value the same way the sibling is precomputed at index time (Go strings.ToLower on both sides, so non-ASCII stays consistent). Search always returns the case-preserved base, so the sibling never has to be read back. In bleve it is indexed but not stored, kept out of _all, and without doc values. In OpenSearch it deliberately stays in _source: excluding it would make every update-by-query script rebuild all siblings from the document via painless toLowerCase, which lowercases differently than Go and would drift from the query side. Keeping it in _source avoids that, and a lowercased copy of a name or path is negligible disk in a cluster. The OpenSearch move script keeps the base and its sibling in sync by swapping the moved prefix in Path_lowercase and setting Name_lowercase from Go-lowercased params, so case-insensitive search still finds a file after it moves (previously the sibling went stale). bleve re-indexes the whole document on move/delete/restore, so its siblings stay fresh for free. This also repairs OpenSearch path search (the query value was no longer folded to lowercase, so path:<Foo> returned nothing) and makes bleve path queries match a folder and its descendants like OpenSearch's path_hierarchy. The Path base stays case-preserved so the move/delete descendant update (an exact TermQuery on Path) matches mixed-case folders.
6edf8f1 to
1ea73e6
Compare
…bleve Single-term `content:` built an unanalyzed term query, so once this branch dropped the blanket query-value lowercasing, `content:Foo` missed on OpenSearch (bleve was unaffected, its query analyzes). Fielded full-text queries now use a match query. OpenSearch `Content` also gets a porter stemming analyzer (it used the default standard analyzer and never stemmed), so full-text search matches bleve on both case and stemming.
bleve compiled a path restriction to a DisjunctionQuery, which mapBinary redistributes as an OR-chain, so `path:/Foo AND name:bar` matched the folder itself unconditionally. It is now a BooleanQuery (should: folder OR descendants), which mapBinary keeps atomic under an enclosing AND. The OpenSearch full-text branch ran before the wildcard check, so `content:foo*` degraded to a phrase match and diverged from bleve; the wildcard check now comes first. Adds the missing coverage the review flagged: path AND term, content wildcard, case-insensitive tags (the array sibling branch), and a spaced path with descendants on OpenSearch.
…rays The []any branch skipped the sibling for an empty array while the []string branch wrote an empty one; both now write it, matching the base field.
CaseInsensitive routes queries to a <field>_lowercase sibling that is only generated for keyword/path fields, so marking any other type CaseInsensitive would silently match nothing. Validate now rejects it up front.
…ends Adds bleve and OpenSearch coverage for category (image), literal MIME (image/svg+xml, with + and /), and raw MimeType: queries. Documents why MimeType skips the bleve escaper: it is not a bug, bleve treats / and + as literals mid-term, so a literal MIME still matches exactly while the category wildcard image/* keeps its *.
mediatype:Folder / mediatype:IMAGE resolved to a literal MimeType search and matched nothing because Expand switched on the raw value. The value is now lowercased in the lowering pass, so categories and literal MIME types match regardless of case, consistently on both backends.
…them resolveField marked every anonymous field embedded, so walkFields (mapping, field index, validate) and fillStruct (deserializer) flattened a json-tagged embedded struct, while conversions.To/encoding/json on the write path nests it under the tag, mapping and deserializing it at the wrong path. An anonymous field is now embedded only without a json tag name, matching encoding/json; fillStruct also recurses into a value nested struct. No current type has a tagged embedded struct, so runtime behavior is unchanged; this hardens the reflection walker.
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)
Existing indexes keep their stored mapping; the new shape only applies to newly-created indexes.
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 →keyword-only. 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; aggregations now produce correct case-preserving buckets on both backends.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.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.OpenSearch Name/Tags tokenization now matches bleve: a single case-preserved keyword token (was word-tokenized), with case-insensitive matching handled by the
_lowercasesibling. No impact on the product: web always searches wildcarded (name:"*term*", see webuseSearch.ts), which matches regardless of tokenization; bleve has always been single-token. Only affects non-web clients sending a barename:reportand expecting a substring match.Mtime is typed as a date on both backends, so
mtime:>...ranges are chronological (was a keyword field / lexicographic compare on OpenSearch).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. Every keyword/path field indexes its case-preserved base (returned to clients, used for exact ops like the move/delete cascade) plus, when enabled, a lowercased sibling used only for matching. 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.Contenton OpenSearch also gets a porter stemming analyzer matching bleve's (it previously used the defaultstandardanalyzer and never stemmed, a pre-existing inconsistency), socontent:running/content:run/content:RUNNINGbehave the same on both backends.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 OpenSearch mapping (index
resource_v2) now lists all properties explicitly, so it differs from any existingresource_v2index. On startupApplyreturnsErrManualActionRequiredwith a clear message; operators upgrading in place must drop and reindexresource_v2. Fresh installs and the bleve backend are unaffected.Follow-ups (not in this PR)
_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.?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.