fix(thumbnails): pick audio cover art deterministically - #3208
Closed
dschmidt wants to merge 54 commits into
Closed
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
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.
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.
…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).
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.
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.
…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.
mediatype:file expands to a NOT restriction. Spliced inline as `NOT MimeType:httpd/unix-directory`, the bleve compiler's NOT branch left a stale operand, so `mediatype:file AND name:x` dropped `name:x` and matched nothing (the web Files filter). It is now wrapped in a group so the negation stays atomic; verified fixing both bleve and OpenSearch.
The guard only rejected CaseInsensitive when a non-keyword/path Type was set explicitly. With no Type, isCasedType treated the field as cased, so CaseInsensitive on an inferred numeric/bool/datetime field passed validation but produced no _lowercase sibling, and the query would silently match nothing. Validate now falls back to the inferred Go type.
…th start The move script rewrote Path/Path_lowercase with painless String.replace, which replaces every occurrence of the old path, not just the leading prefix. OpenCloud paths are ./-prefixed so the full old path only occurs at the start and the result is byte-identical, but startsWith + substring makes the prefix-only intent explicit and robust to any path format. Not a live bug fix, a hardening.
A path value with spaces went through a match_phrase query, which analyzes the query with the path_hierarchy analyzer; the resulting "." prefix token matches every document in the space, breaking descendant matching and the stale-path check after a move.
Cold boots take well over the 5s startup timeout, and a full host disk tripped the flood-stage create-index block mid-run; test indexes are tiny.
- the additive warnings advertise --all-spaces --force-rescan; a plain walk skips unchanged documents and never backfills the new fields - Apply checks index existence first again, so a pre-provisioned index needs no create privilege and odd create-error shapes (string error bodies, cluster blocks) cannot fail a healthy startup; Create on 404 keeps the typed already-exists swallow as the creation-race backstop - number_of_replicas drift is not breaking, it is runtime-tunable and needs no rebuild - bleve returns the classification alongside post-persist errors and the server warns before the error check, so the one-time additive warning is not lost when close or reopen fails - a golden fixture pins the marshaled bleve mapping so a dependency bump that changes marshaling fails in CI instead of refusing every installation in the field
…the audio-only gate
dschmidt
force-pushed
the
feat/search-tika-facets
branch
from
August 18, 2026 16:28
c7c0551 to
35231b1
Compare
Switch to the dschmidt/tag fork and select the front cover (picture type 0x03) when tagged, else the first available picture, instead of the lib's single non-deterministic Picture().
dschmidt
force-pushed
the
feat/thumbnails-deterministic-cover
branch
from
August 18, 2026 16:34
8ee6ad4 to
ab9a224
Compare
This was referenced Aug 19, 2026
dschmidt
force-pushed
the
feat/search-tika-facets
branch
3 times, most recently
from
August 31, 2026 17:00
19fb88d to
a9c7079
Compare
fschade
force-pushed
the
feat/search-tika-facets
branch
from
September 1, 2026 05:17
2329c62 to
240239d
Compare
dschmidt
force-pushed
the
feat/search-tika-facets
branch
from
September 1, 2026 07:10
240239d to
5e4f124
Compare
Contributor
Author
|
Superseded by the Tika-based thumbnail pipeline (#3332 + upstream TIKA-4801, which makes Tika emit the front cover first): with covers coming from Tika there is no dhowden/tag code path left to make deterministic. Branch stays around in case we revive it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Superseded by #3332 (if we decide to go down that road)
Stacked on #3198.
The thumbnails
AudioDecoderuseddhowden/tag's singlePicture(), which is non-deterministic when an audio file embeds multiple pictures (e.g. front + back cover). This switches to my fork (dschmidt/tag, addsPictures()+ aRawTypebyte) and selects deterministically: the tagged front cover (picture type 0x03) if present, otherwise the first available picture. Many taggers store the cover as type "Other" (0x00) rather than front, so the fallback is the common case.Fork PR: dschmidt/tag#1 (merged to master).