fix: one field-resolution law across aggregation hooks, source.where, removeMany, and find() spellings
Four fixes from a consumer conformance report, one root disease — two field-resolution regimes where there must be one: - The delete/update aggregation hooks fed the engine a partial entity view (type/service/data/metadata only), so a reserved-field groupBy (subtype, visibility, ...) resolved to a nonexistent group on the way down: counts drifted upward forever after deletes, and updates moving an entity between reserved-field groups double-counted. The hooks now pass the full-fidelity view via entityForAggFromRawRecord (every reserved field top-level, mirroring the add path); the update sites pass the full get() view instead of a hand-rolled subset. - Aggregation source.where resolved fields only against the custom metadata bag, so where on a reserved field silently matched nothing. The matcher now resolves each filtered field through resolveEntityField — the same single source of truth groupBy uses. - removeMany() with no usable selector (bare array passed positionally, empty params, ids: []) resolved successfully having deleted nothing. All three now throw; the two legacy tests that pinned the silent no-op as 'graceful' now pin the refusal. - find() where keys accept both spellings: a metadata.-prefixed key falls back to its flattened spelling when the prefixed one is not indexed (metadata is flattened at index time). A literal nested custom key named metadata still wins when indexed as spelled. Five regression pins in aggregate-reserved-fields.test.ts (4 of 5 vary red on the unfixed code).
This commit is contained in:
parent
42037d0cd0
commit
945d92d29e
7 changed files with 303 additions and 45 deletions
|
|
@ -88,10 +88,18 @@ function matchesSource(entity: Record<string, unknown>, source: AggregateDefinit
|
|||
if (entity.service !== source.service) return false
|
||||
}
|
||||
|
||||
// Metadata where filter — match against the entity's metadata sub-object
|
||||
// Where filter — resolve each filtered field through resolveEntityField,
|
||||
// the SAME single source of truth groupBy uses (top-level standard fields
|
||||
// + custom metadata). Matching only the metadata sub-object made
|
||||
// where:{subtype}/{visibility}/… a silent no-op: reserved fields never
|
||||
// live in the custom bag, so those filters could never match anything.
|
||||
if (source.where && Object.keys(source.where).length > 0) {
|
||||
const metadata = (entity.metadata ?? entity) as Record<string, unknown>
|
||||
if (!matchesMetadataFilter(metadata, source.where)) return false
|
||||
const e = entity as unknown as HNSWNounWithMetadata
|
||||
const resolved: Record<string, unknown> = {}
|
||||
for (const key of Object.keys(source.where)) {
|
||||
resolved[key] = resolveEntityField(e, key)
|
||||
}
|
||||
if (!matchesMetadataFilter(resolved, source.where)) return false
|
||||
}
|
||||
|
||||
return true
|
||||
|
|
|
|||
104
src/brainy.ts
104
src/brainy.ts
|
|
@ -1883,6 +1883,29 @@ export class Brainy<T = any> implements BrainyInterface<T> {
|
|||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* @description Build the AGGREGATION view of an entity from a stored flat
|
||||
* metadata record — EVERY reserved field mapped to its top-level entity
|
||||
* name (stored `noun` → `type`), custom metadata in `metadata`. This must
|
||||
* mirror the add-path `entityForIndexing` shape exactly: the aggregation
|
||||
* engine resolves groupBy/where fields via `resolveEntityField`
|
||||
* (top-level standard fields + custom metadata), so a view that drops a
|
||||
* reserved field makes every aggregate grouped by that field decrement a
|
||||
* group that does not exist — counts then drift upward forever after
|
||||
* deletes (SELF-AGGREGATE-DELETE-DRIFT). Do not hand-roll subsets of this.
|
||||
* @param record - The stored flat metadata record (before-image or pre-delete read).
|
||||
* @returns The full-fidelity entity view for aggregation hooks.
|
||||
*/
|
||||
private entityForAggFromRawRecord(record: Record<string, unknown>): Record<string, unknown> {
|
||||
const { reserved, custom } = splitNounMetadataRecord(record)
|
||||
const { noun, ...rest } = reserved
|
||||
return {
|
||||
type: noun,
|
||||
...rest,
|
||||
metadata: custom
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* @description Add an entity (noun) to the brain. Embeds `data` into a vector and
|
||||
* indexes the entity across all three intelligences — vector similarity, graph
|
||||
|
|
@ -3125,15 +3148,16 @@ export class Brainy<T = any> implements BrainyInterface<T> {
|
|||
]
|
||||
: undefined)
|
||||
|
||||
// Aggregation hook (outside transaction — derived data)
|
||||
// Aggregation hook (outside transaction — derived data). `existing` is
|
||||
// the full get() view — every reserved field top-level — and must be
|
||||
// passed whole: a subset view makes the old-side decrement miss any
|
||||
// reserved-field group (update would then double-count it).
|
||||
if (this._aggregationIndex) {
|
||||
const oldEntityForAgg = {
|
||||
type: existing.type,
|
||||
service: existing.service,
|
||||
data: existing.data,
|
||||
metadata: existing.metadata
|
||||
}
|
||||
this._aggregationIndex.onEntityUpdated(params.id, entityForIndexing, oldEntityForAgg)
|
||||
this._aggregationIndex.onEntityUpdated(
|
||||
params.id,
|
||||
entityForIndexing,
|
||||
existing as unknown as Record<string, unknown>
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -3245,19 +3269,15 @@ export class Brainy<T = any> implements BrainyInterface<T> {
|
|||
]
|
||||
: undefined)
|
||||
|
||||
// Aggregation hook (outside transaction — derived data)
|
||||
// Aggregation hook (outside transaction — derived data). The view must
|
||||
// carry EVERY reserved field top-level (not a subset): a groupBy on
|
||||
// subtype/visibility/etc. otherwise decrements a nonexistent group and
|
||||
// the real count never comes down.
|
||||
if (this._aggregationIndex && metadata) {
|
||||
// Reconstruct entity-like object from stored metadata via the
|
||||
// canonical reserved/custom split (the hand-rolled destructure here
|
||||
// missed subtype/_rev, leaking them into the aggregation view).
|
||||
const { reserved, custom } = splitNounMetadataRecord(metadata)
|
||||
const entityForAgg = {
|
||||
type: reserved.noun,
|
||||
service: reserved.service,
|
||||
data: reserved.data,
|
||||
metadata: custom
|
||||
}
|
||||
this._aggregationIndex.onEntityDeleted(id, entityForAgg)
|
||||
this._aggregationIndex.onEntityDeleted(
|
||||
id,
|
||||
this.entityForAggFromRawRecord(metadata as Record<string, unknown>)
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -6885,6 +6905,30 @@ export class Brainy<T = any> implements BrainyInterface<T> {
|
|||
this.assertWritable('removeMany')
|
||||
await this.ensureInitialized()
|
||||
|
||||
// Loud selector validation: a call with no usable selector used to
|
||||
// resolve successfully having deleted NOTHING (total: 0) — the classic
|
||||
// silent no-op being a bare array passed positionally
|
||||
// (removeMany([id]) instead of removeMany({ ids: [id] })). The caller
|
||||
// believes the delete happened; every count derived afterwards is "wrong"
|
||||
// while the engine was never even asked. Refuse instead.
|
||||
if (Array.isArray(params)) {
|
||||
throw new Error(
|
||||
`removeMany() takes a params object, not a bare array — use removeMany({ ids: [...] })`
|
||||
)
|
||||
}
|
||||
if (!params || (!params.ids && !params.type && !params.where)) {
|
||||
throw new Error(
|
||||
`removeMany() requires a selector: { ids } and/or { type, where }. ` +
|
||||
`An empty selector would silently delete nothing — refusing.`
|
||||
)
|
||||
}
|
||||
if (params.ids && params.ids.length === 0) {
|
||||
throw new Error(
|
||||
`removeMany() received ids: [] — an empty id list deletes nothing. ` +
|
||||
`Pass the ids to delete, or omit ids and select by { type, where }.`
|
||||
)
|
||||
}
|
||||
|
||||
// Determine what to delete
|
||||
let idsToDelete: string[] = []
|
||||
|
||||
|
|
@ -9388,12 +9432,9 @@ export class Brainy<T = any> implements BrainyInterface<T> {
|
|||
)
|
||||
plan.touchedNouns.push(params.id)
|
||||
|
||||
const oldEntityForAgg = {
|
||||
type: existing.type,
|
||||
service: existing.service,
|
||||
data: existing.data,
|
||||
metadata: existing.metadata
|
||||
}
|
||||
// The full planGetEntity view, passed whole — a subset view makes the
|
||||
// old-side decrement miss reserved-field groups (double-count on update).
|
||||
const oldEntityForAgg = existing as unknown as Record<string, unknown>
|
||||
plan.postCommit.push(() => {
|
||||
if (this._aggregationIndex) {
|
||||
this._aggregationIndex.onEntityUpdated(params.id, entityForIndexing, oldEntityForAgg)
|
||||
|
|
@ -9514,14 +9555,9 @@ export class Brainy<T = any> implements BrainyInterface<T> {
|
|||
}
|
||||
|
||||
if (metadata) {
|
||||
// Canonical reserved/custom split — mirror of remove()'s aggregation hook.
|
||||
const { reserved, custom } = splitNounMetadataRecord(metadata)
|
||||
const entityForAgg = {
|
||||
type: reserved.noun,
|
||||
service: reserved.service,
|
||||
data: reserved.data,
|
||||
metadata: custom
|
||||
}
|
||||
// Mirror of remove()'s aggregation hook — the FULL reserved view, so
|
||||
// reserved-field groupBy decrements find their group.
|
||||
const entityForAgg = this.entityForAggFromRawRecord(metadata as Record<string, unknown>)
|
||||
plan.postCommit.push(() => {
|
||||
if (this._aggregationIndex) {
|
||||
this._aggregationIndex.onEntityDeleted(id, entityForAgg)
|
||||
|
|
|
|||
|
|
@ -1867,9 +1867,26 @@ export class MetadataIndexManager implements MetadataIndexProvider {
|
|||
// not once per AND-clause inside it.
|
||||
const unindexedFields: string[] = []
|
||||
|
||||
for (const [field, condition] of Object.entries(filter)) {
|
||||
for (const [rawField, condition] of Object.entries(filter)) {
|
||||
// Skip logical operators
|
||||
if (field === 'allOf' || field === 'anyOf' || field === 'not') continue
|
||||
if (rawField === 'allOf' || rawField === 'anyOf' || rawField === 'not') continue
|
||||
|
||||
// Metadata is FLATTENED at index time (metadata.entry.title indexes as
|
||||
// entry.title), so a `metadata.`-prefixed where key is almost always
|
||||
// the caller spelling the STORAGE shape rather than the index shape.
|
||||
// Accept both spellings: when the key as spelled is unindexed but its
|
||||
// stripped spelling is, query the stripped one. A literal nested
|
||||
// custom key named `metadata` still wins when indexed as spelled
|
||||
// (checked first), so that rare shape keeps working.
|
||||
let field = rawField
|
||||
if (
|
||||
rawField.startsWith('metadata.') &&
|
||||
this.columnStore &&
|
||||
!this.columnStore.hasField(rawField) &&
|
||||
this.columnStore.hasField(rawField.slice('metadata.'.length))
|
||||
) {
|
||||
field = rawField.slice('metadata.'.length)
|
||||
}
|
||||
|
||||
let fieldResults: string[] = []
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue