refactor: reflection-based search mapping + location geopoint#2659
refactor: reflection-based search mapping + location geopoint#2659dschmidt wants to merge 7 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 | 229 |
| Duplication | -86 |
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
1c13f04 to
2accce3
Compare
6947cf7 to
98f896b
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.
98f896b to
0046873
Compare
aduffeck
left a comment
aduffeck
left a comment
There was a problem hiding this comment.
I'm fine with the change in general, unifiying and streamlining the index creating and the mapping should be worth the effort and the partly heavy code. I would like to alo hear @fschade 's opinion however before moving this forward (he should be back next week).
| IndexManagerLatest = IndexIndexManagerResourceV2 | ||
| IndexIndexManagerResourceV1 IndexManager = "resource_v1.json" | ||
| IndexIndexManagerResourceV2 IndexManager = "resource_v2.json" | ||
| IndexIndexManagerResourceV2 IndexManager = "resource_v2" |
There was a problem hiding this comment.
Requiring another re-indexing of the whole tree right after the stable 7.2 release is unfortunate, but I understand there's no way to do an in-place migration of the existing index with the field changes we do.
But maybe this is the time to spend some time on making index upgrades less intrusive. It should be possible to add a read alias on the v2 index while building a v3 index and then flip the alias over to the new index once it's done. I have no idea how easy and robust that is to do programmatically keeping all possible deployment scenarios in mind.
At the very least we should use "v3" as the index though, if the full-blown automation isn't possible, so that the new index could be built up in a pre-deployment hook in kubernetes, for example.
There was a problem hiding this comment.
True, but requiring it right before the release would have been risky on the other hand - there's never a perfect timing for this kind of change :)
We'll discuss soft upgrade options with @fschade next week :)
|
Oh, one more remark: it would be great if you would just add commits instead of force-pushing refinements, that makes it easier to figure out what changed after starting the review. |
|
Yes, will do from now on - thought noone had started reviewing and wanted to make a clean start from the consolidated state. Will only add commits on top from now on! |
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.
|
Okay, the PR is huge, but I can understand that it’s hard to logically split up such a massive topic. The only thing bothering me right now is the schema update issue; I can't think of a clean way to handle that independently of the deployment, and a "batteries-included" approach definitely isn't the right approach here. I think we just need to document clearly what needs to be done for schema updates (and we’d face that problem regardless of this PR, other ideas?). From my side: thumbs up. |
|
follow up documentation issue: #3092 |
The code itself looks fine to me now, but we need to come up with a proper upgrade/migration path which will handle the incompatible index before merging this. @fschade is looking into that right now.
4a43497 to
8c6a3b6
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.
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.
Tags/Favorites: dynamickeyword→text + lowercaseKeyword(unified with bleve; the analyzer is registered in the new index settings).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.libre.graph.audio.*metadata is present, regardless of MIME type (the oldaudio/guard is dropped) - on read and write consistently.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:>...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.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.