From cad28674ef89987e2fc7f3c32da03b6ba19cac18 Mon Sep 17 00:00:00 2001 From: zhuangjianguo Date: Sun, 30 Aug 2026 21:31:16 +0000 Subject: [PATCH 1/3] fix(driver-memory,driver-mongodb): stop a lowered operator key from clobbering a sibling Generalise #13195's per-operator guard into one rule for the class: a lowered write whose key is free merges inline, a write whose key is taken becomes its own $and branch. Ranked by the spec's declared operator order so the emitted document is a function of the constraint set, not of the author's key order. Also promotes the analytics face's wholesale per-member clobber the same way. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L --- .../driver-memory/src/memory-analytics.ts | 33 ++- .../driver-memory/src/memory-driver.ts | 229 +++++++++++++---- .../driver-mongodb/src/mongodb-filter.ts | 232 +++++++++++++----- 3 files changed, 384 insertions(+), 110 deletions(-) diff --git a/packages/drivers/driver-memory/src/memory-analytics.ts b/packages/drivers/driver-memory/src/memory-analytics.ts index 8fe02ea749..8c5d6aada8 100644 --- a/packages/drivers/driver-memory/src/memory-analytics.ts +++ b/packages/drivers/driver-memory/src/memory-analytics.ts @@ -675,6 +675,11 @@ export class MemoryAnalyticsService implements IAnalyticsService { const normalizedFilters = this.normalizeFilters(query); if (normalizedFilters.length > 0) { const matchStage: Record = {}; + /** + * [#13524] Predicates for a member that already has one — see the + * promotion below the loop for why they cannot be assigned. + */ + const contested: Record[] = []; for (const filter of normalizedFilters) { const fieldPath = this.resolveFieldPath(cube, filter.member); // [#5374] The operator decides the WHOLE predicate, not just its name — @@ -687,7 +692,7 @@ export class MemoryAnalyticsService implements IAnalyticsService { // FROM: a boolean reaches mingo as a boolean and `null` as `null`, so a // predicate over `is_active` or `closed_at` selects the same rows // `find()` selects instead of none / all of them. - matchStage[fieldPath] = this.mongoPredicateBuilder(filter.operator)({ + const predicate = this.mongoPredicateBuilder(filter.operator)({ comparands: this.comparandsFor(cube, filter.member, filter.values), raw: filter.values, substring: (value) => this.driver.filterSubstringPattern(value), @@ -695,7 +700,33 @@ export class MemoryAnalyticsService implements IAnalyticsService { // than from the driver's Unicode-folding `filterSubstringPattern`. asciiSubstring: (value) => new RegExp(asciiCaseInsensitiveRegexSource(String(value))), }); + // [#13524] This was `matchStage[fieldPath] = …`, and the assignment was + // a WHOLESALE clobber — the widest member of this card's class. The two + // document-shaped translators lose a constraint only when two operators + // happen to lower onto the SAME key; here the stage is keyed by field + // path alone, so the second predicate on a member replaced the first + // ENTIRELY, for every operator pair. And `flattenFilterCondition` folds + // `$and` into this same flat list, so `{$and: [{name: {$contains:'a'}}, + // {name: {$ne:'b'}}]}` — two separate nodes, not one operator map — + // lost a constraint too. Measured on a three-row fixture: + // `{name: {$contains:'a', $ne:'b'}}` aggregated ['1','3'] and its + // key-swapped twin ['1'], while the reference matcher said ['1']. + // + // Same rule as the translators: free member merges inline, a taken one + // becomes its own `$and` branch of the SAME `$match`, where both + // predicates survive. No ranking is needed here — unlike a contested + // operator key, nothing is overwritten, so which predicate sits inline + // changes the document's shape but never its answer. + if (Object.prototype.hasOwnProperty.call(matchStage, fieldPath)) { + contested.push({ [fieldPath]: predicate }); + } else { + matchStage[fieldPath] = predicate; + } } + // A field path can never BE `$and` — `resolveFieldPath` resolves cube + // members, and `flattenFilterCondition` refuses `$or` / `$not` and folds + // `$and` away before this runs — so this cannot collide with a member. + if (contested.length > 0) matchStage.$and = contested; if (Object.keys(matchStage).length > 0) { pipeline.push({ $match: matchStage }); } diff --git a/packages/drivers/driver-memory/src/memory-driver.ts b/packages/drivers/driver-memory/src/memory-driver.ts index 46f55f505b..421e00e694 100644 --- a/packages/drivers/driver-memory/src/memory-driver.ts +++ b/packages/drivers/driver-memory/src/memory-driver.ts @@ -30,6 +30,9 @@ import { danglingLikeEscapeError, // [#10576] The per-aggregation `filter` refusal — this driver evaluates none. refusePerAggregationFilter, + // [#13524] The declared authorable field vocabulary, IN DECLARATION ORDER — + // the canonical order `FIELD_OPERATOR_RANK` below reads. + SUPPORTED_FIELD_OPERATORS, } from './filter-refusal.js'; import { coerceTemporalValue, @@ -47,6 +50,125 @@ import { type MemoryUniqueEnforcement, } from './memory-unique-constraint.js'; +/** + * [#13524] The canonical rank of an authorable field operator — the tie-break + * that decides which writer of a CONTESTED lowered key keeps the inline slot. + * + * Read straight off {@link SUPPORTED_FIELD_OPERATORS}, which is + * `[...FILTER_OPERATORS, '$like', '$ilike']` — the spec's declaration order, + * not a hand-copy of it. That matters twice: a nineteenth operator is ranked + * the day it is declared, and the rank of `$exists` (last in the spec's list) + * is what makes this generalisation emit, byte for byte, the documents + * #13195's guard already emits for the one operator it moved. + * + * An operator absent from the vocabulary cannot reach the assembly — the + * `default:` arm throws first — so the `?? Number.MAX_SAFE_INTEGER` fallback is + * a totality floor, never a live path. + */ +const FIELD_OPERATOR_RANK: ReadonlyMap = new Map( + [...SUPPORTED_FIELD_OPERATORS].map((op, index) => [op, index] as const), +); + +/** One write of one lowered key, tagged with the operator that produced it. */ +interface LoweredWrite { + /** The AUTHORABLE operator this write came from — what ranks it. */ + readonly op: string; + /** The key the backend understands, which is NOT always `op`. */ + readonly key: string; + readonly value: any; +} + +/** + * [#13524] Assemble lowered writes into one operator document, promoting every + * write whose key is already TAKEN into its own condition instead of letting it + * overwrite the sitting one. + * + * ## The defect this closes — a CLASS, not an instance + * + * Several authorable operators do not lower to a key of their own name. They + * write keys an author can ALSO write on the same field constraint, so two + * constraints land on one key of one object literal and the second assignment + * wins. One constraint disappears with no error, no warning and no trace in the + * emitted document — and WHICH one disappears is decided by the author's key + * order, because that is the order `Object.keys` walks. + * + * Enumerated over the whole declared vocabulary (18 operators, probed one at a + * time and intersected — not reasoned), the contested keys on this face are: + * + * | lowered key | written by | + * |---|---| + * | `$eq` | `$eq`, `$null: true`, `$exists: false` | + * | `$ne` | `$ne`, `$null: false`, `$exists: true` | + * | `$gte` | `$gte`, `$between` | + * | `$lte` | `$lte`, `$between` | + * | `$lt` | `$lt`, `$lte` (BARE CALENDAR DAY — #4042's half-open rewrite), `$between` (bare-day max) | + * | `$regex` | the whole string family — promoted by `_multiRegex`, see below | + * + * `$lte` → `$lt` is the member no card had named: a bare `YYYY-MM-DD` upper + * bound compiles half-open, so `{d: {$lte: '2026-07-28', $lt: '2026-07-02'}}` + * and its key-swapped twin answered `['1']` and `['1','2']` on this fixture. + * `$not` is written by `$notContains` and by NOTHING else — it is covered here + * by construction rather than curatively, which is the point of ranging over + * the vocabulary instead of over the three operators that had been noticed. + * + * ## The rule, and why it is this one + * + * Free key → merge inline (the overwhelmingly common case: one operator, one + * key). Taken key → the write becomes its own `$and` branch on the same field, + * where both constraints survive. That is exactly the guard #13195 landed for + * `$exists` alone, generalised to every writer rather than restated per + * operator — the reference matcher (`memory-matcher.ts`), which loops the + * operators and therefore CANNOT express this defect, is the oracle both + * agree with. + * + * ## Why rank, and not author order + * + * The inline slot goes to the LOWEST-RANKED writer, never to the first one the + * author happened to type. Author order would keep the ANSWER correct — `$and` + * is commutative — while leaving the emitted DOCUMENT a function of key order, + * which is the property this card exists to remove. Ranking makes which + * constraint sits inline, and which branches are promoted, a pure function of + * the constraint SET: the two key orders of one predicate emit DEEP-EQUAL + * documents, and a test asserts exactly that. + * + * ⚠️ Deep-equal, not byte-identical, and the difference is deliberate. Writes + * are COLLECTED in author order and only assembled here, so refusals still fire + * in the order the author wrote them and an UNCONTESTED key still lands in its + * original insertion position — `{$between: […], $lte: x}` and its twin emit + * `{$gte, $lte}` with the two keys in opposite insertion order, carrying the + * same constraints. Nothing about a filter with no contested key changes at + * all. + * + * `$and` promotion is expressed as an `_extraAnd` sentinel on the returned + * document, lifted by `normalizeFilterCondition()` — the same shape + * `_multiRegex` uses beside it, and the shape `_presenceAnd` used before this + * subsumed it. + */ +function assembleLoweredWrites(writes: readonly LoweredWrite[]): Record { + const result: Record = {}; + const byKey = new Map(); + for (const write of writes) { + const group = byKey.get(write.key); + if (group) group.push(write); + else byKey.set(write.key, [write]); + } + const extraAnd: Record[] = []; + for (const [key, group] of byKey) { + // Single writer is the common case and must not be perturbed at all. + if (group.length > 1) { + group.sort( + (a, b) => + (FIELD_OPERATOR_RANK.get(a.op) ?? Number.MAX_SAFE_INTEGER) - + (FIELD_OPERATOR_RANK.get(b.op) ?? Number.MAX_SAFE_INTEGER), + ); + } + result[key] = group[0]!.value; + for (let i = 1; i < group.length; i++) extraAnd.push({ [key]: group[i]!.value }); + } + if (extraAnd.length > 0) result._extraAnd = extraAnd; + return result; +} + /** * Persistence adapter interface. * Matches the PersistenceAdapterSchema contract from @objectstack/spec. @@ -1130,14 +1252,17 @@ export class InMemoryDriver implements IDataDriver { continue; } const normalized = this.normalizeFieldOperators(value, this.temporalKind(object, key), key, here); - // [#13195] A lowered `$exists` whose mingo key was already taken by a - // sibling operator on the same field. It cannot be merged without one - // of the two constraints silently overwriting the other, so it becomes - // its own `$and` branch — see the merge in normalizeFieldOperators(). - if (normalized._presenceAnd) { - const presence: Record = normalized._presenceAnd; - delete normalized._presenceAnd; - extraAndConditions.push({ [key]: presence }); + // [#13524] Lowered writes whose mingo key was already taken by a + // sibling operator on the same field. They cannot be merged without one + // of the two constraints silently overwriting the other, so each + // becomes its own `$and` branch — see `assembleLoweredWrites()`. This + // consumed a single `_presenceAnd` when the guard covered `$exists` + // alone (#13195); it is a LIST now because the class has several + // members and one field constraint can contest more than one key. + if (normalized._extraAnd) { + const promoted: Record[] = normalized._extraAnd; + delete normalized._extraAnd; + for (const branch of promoted) extraAndConditions.push({ [key]: branch }); } // Handle multiple regex conditions on the same field (e.g. $startsWith + $endsWith) if (normalized._multiRegex) { @@ -1183,13 +1308,24 @@ export class InMemoryDriver implements IDataDriver { */ private normalizeFieldOperators(ops: Record, kind?: TemporalFieldKind, field = '', path = 'filter'): Record { const store = (v: any) => coerceTemporalValue(v, kind); - const result: Record = {}; const regexConditions: Record[] = []; - /** [#13195] `$exists`, lowered — see the merge at the end of this method. */ - let presence: Record | undefined; + /** + * [#13524] Every lowered write, collected in AUTHOR order and assembled by + * {@link assembleLoweredWrites} after the loop. Collected rather than + * assigned because an arm cannot know whether the key it wants is already + * spoken for by a sibling operator the author wrote LATER — which is the + * whole of the defect this replaces. It also subsumes #13195's + * single-operator `presence` collection: `$exists` is now one writer among + * eighteen, ranked by the same rule as the rest. + */ + const writes: LoweredWrite[] = []; for (const op of Object.keys(ops)) { const val = ops[op]; + /** Record one lowered write for the operator this iteration is on. */ + const put = (loweredKey: string, value: any): void => { + writes.push({ op, key: loweredKey, value }); + }; switch (op) { // [#6682] Case-SENSITIVE, the same four arms as the AST spelling one // method up (`convertConditionToMongo`) and for the same reason — see @@ -1199,7 +1335,7 @@ export class InMemoryDriver implements IDataDriver { regexConditions.push({ $regex: new RegExp(this.escapeRegex(val)) }); break; case '$notContains': - result.$not = { $regex: new RegExp(this.escapeRegex(val)) }; + put('$not', { $regex: new RegExp(this.escapeRegex(val)) }); break; case '$startsWith': regexConditions.push({ $regex: new RegExp(`^${this.escapeRegex(val)}`) }); @@ -1258,20 +1394,23 @@ export class InMemoryDriver implements IDataDriver { // range simply vanished, and no one was told. The shape gate refuses // it now; this throw is the totality floor. if (!Array.isArray(val) || val.length !== 2) throw malformedBetweenError(field, val, `${path}.$between`); - result.$gte = store(val[0]); + put('$gte', store(val[0])); // Bare-day max → half-open, inheriting `$lte`'s whole-day rule (#4042). const betweenNextDay = nextUtcCalendarDay(val[1]); - if (betweenNextDay != null) result.$lt = store(betweenNextDay); - else result.$lte = store(val[1]); + if (betweenNextDay != null) put('$lt', store(betweenNextDay)); + else put('$lte', store(val[1])); break; } case '$lte': { // A bare-day upper bound means "through that whole day" (#4042; the // driver-sql twin is #3777). Order-equivalent to `<=` for plain // `YYYY-MM-DD` values, so it applies without a field-type lookup. + // [#13524] `$lt` here is a key an AUTHOR can also write — this arm is + // the member of the clobber class no card had named. See + // {@link assembleLoweredWrites}. const nextDay = nextUtcCalendarDay(val); - if (nextDay != null) result.$lt = store(nextDay); - else result.$lte = store(val); + if (nextDay != null) put('$lt', store(nextDay)); + else put('$lte', store(val)); break; } case '$null': @@ -1288,16 +1427,16 @@ export class InMemoryDriver implements IDataDriver { // keeps beside it. if (typeof val !== 'boolean') throw nonBooleanNullComparandError(field, val, `${path}.$null`); if (val === true) { - result.$eq = null; + put('$eq', null); } else { - result.$ne = null; + put('$ne', null); } break; // Value comparisons take the field's storage form (#4047); the null / // existence predicates above are value-independent and must not. case '$eq': case '$ne': case '$gt': case '$gte': case '$lt': case '$in': case '$nin': - result[op] = store(val); + put(op, store(val)); break; // [#13195] `$exists` means "the field HAS A VALUE" (`!= null`), never // key presence — #5298 leg 3 / #5369, landed in PR #5962, and ruled @@ -1328,9 +1467,11 @@ export class InMemoryDriver implements IDataDriver { // evaluation arm for a refused operator is exactly what let this // driver's two faces answer one `$regex` differently for so long. case '$exists': - // Collected, not assigned: the lowering below has to know whether the - // key it wants is already spoken for. See the merge at the end. - presence = val === true ? { $ne: null } : { $eq: null }; + // Collected, not assigned: the assembly has to know whether the key + // this lowers to is already spoken for. [#13524] Since the class was + // generalised this arm is no longer special — it `put`s like every + // other writer and the shared rule ranks it. + put(val === true ? '$ne' : '$eq', null); break; default: // [#5324] Was `result[op] = val` — a GENERIC passthrough that handed @@ -1342,38 +1483,18 @@ export class InMemoryDriver implements IDataDriver { } } - // [#13195] Merge the lowered `$exists`, and do NOT let it clobber a sibling. - // - // The lowering the ruling prescribes reuses `$ne` / `$eq` — mingo keys an - // AUTHOR can also write on the same field. `{name: {$exists: true, $ne: - // 'b'}}` would therefore assign `$ne` twice into one object literal, and - // whichever ran last would win: one of the two constraints vanishes, and - // WHICH one depends on the author's key order. Measured on this fixture - // before the guard existed: `{$exists: true, $ne: 'b'}` answered - // `['1','3']` and the key-swapped `{$ne: 'b', $exists: true}` answered - // `['1','2']` — one predicate, two row sets — while the reference matcher - // said `['1']` for both. Four cells that AGREED with the reference matcher - // before the alignment disagreed after it. - // - // So when the key is free the predicate merges inline (the common case — - // `$exists` alone on a field), and when it is taken the field is promoted - // to its own `$and` branch, where both constraints survive. `_presenceAnd` - // is an internal sentinel consumed by normalizeFilterCondition(), the same - // shape `_multiRegex` below uses for the same reason. + // [#13524] Assemble every lowered write, and do NOT let one clobber another. // - // ⚠️ Scope: this guards the operator this card moved, and only it. The - // identical clobber is reachable today through `$null`, `$between` and - // `$notContains`, which lower to `$ne`/`$eq`, `$gte`/`$lte`/`$lt` and - // `$not` respectively — measured, pre-existing, and filed separately rather - // than half-fixed here. - if (presence) { - const presenceKey = Object.keys(presence)[0]!; - if (Object.prototype.hasOwnProperty.call(result, presenceKey)) { - result._presenceAnd = presence; - } else { - Object.assign(result, presence); - } - } + // #13195 landed this rule for `$exists` alone — free key merges inline, a + // taken key becomes its own `$and` branch — and said in this spot that the + // identical clobber was reachable through `$null`, `$between` and + // `$notContains`. Enumerating the declared vocabulary instead of the noticed + // operators found a fourth (`$lte` on a bare calendar day, which lowers onto + // `$lt`) and found `$notContains` NOT reachable, since nothing else writes + // `$not`. So the guard is no longer per-operator: it ranges over every + // writer, and `$exists` is now one of them. See + // {@link assembleLoweredWrites} for the enumeration and the ranking rule. + const result = assembleLoweredWrites(writes); // Merge regex conditions: single → inline, multiple → wrap with $and if (regexConditions.length === 1) { diff --git a/packages/drivers/driver-mongodb/src/mongodb-filter.ts b/packages/drivers/driver-mongodb/src/mongodb-filter.ts index 8163e2f76b..b69a5521cb 100644 --- a/packages/drivers/driver-mongodb/src/mongodb-filter.ts +++ b/packages/drivers/driver-mongodb/src/mongodb-filter.ts @@ -45,6 +45,9 @@ import { RETIRED_FILTER_OPERATORS } from '@objectstack/spec/data'; // live in the pattern source — see its docblock for why `$options: 'i'` is the // wrong tool. import { asciiCaseInsensitiveRegexSource } from '@objectstack/spec/data'; +// [#13524] The declared authorable field vocabulary, IN DECLARATION ORDER — +// the canonical order `FIELD_OPERATOR_RANK` below reads. +import { FILTER_OPERATORS } from '@objectstack/spec/data'; import { coerceTemporalValue, type TemporalFieldKind, @@ -663,14 +666,17 @@ function translateCondition( const hasOps = Object.keys(objValue).some((k) => k.startsWith('$')); if (hasOps) { const translated = translateFieldOperators(objValue, temporalKind?.(key), key, `${path}.${key}`); - // [#13195] A lowered `$exists` whose MongoDB key was already taken - // by a sibling operator on the same field. Merging it would drop one - // of the two constraints silently, so it becomes its own `$and` - // branch — see the merge in translateFieldOperators(). - const presenceAnd = translated._presenceAnd as Record | undefined; - if (presenceAnd) { - delete translated._presenceAnd; - andClauses.push({ [key]: presenceAnd } as Filter); + // [#13524] Lowered writes whose MongoDB key was already taken by a + // sibling operator on the same field. Merging one would drop a + // constraint silently, so each becomes its own `$and` branch — see + // `assembleLoweredWrites()`. This consumed a single `_presenceAnd` + // when the guard covered `$exists` alone (#13195); it is a LIST now + // because the class has several members and one field constraint + // can contest more than one key. + const extraAnd = translated._extraAnd as Record[] | undefined; + if (extraAnd) { + delete translated._extraAnd; + for (const branch of extraAnd) andClauses.push({ [key]: branch } as Filter); } mongoFilter[key] = translated; } else { @@ -697,6 +703,122 @@ function translateCondition( return mongoFilter; } +/** + * [#13524] The canonical rank of an authorable field operator — the tie-break + * that decides which writer of a CONTESTED lowered key keeps the inline slot. + * + * Read straight off the spec's `FILTER_OPERATORS` declaration order rather than + * hand-copied, so a seventeenth operator is ranked the day it is declared. The + * rank of `$exists` (last in that list) is what makes this generalisation emit, + * byte for byte, the documents #13195's guard already emits for the one + * operator it moved. `$like` / `$ilike` are declared but NOT translated by this + * driver — the `default:` arm refuses them before the assembly runs — so the + * fallback below is a totality floor, never a live path. + */ +const FIELD_OPERATOR_RANK: ReadonlyMap = new Map( + FILTER_OPERATORS.map((op, index) => [op as string, index] as const), +); + +/** One write of one lowered key, tagged with the operator that produced it. */ +interface LoweredWrite { + /** The AUTHORABLE operator this write came from — what ranks it. */ + readonly op: string; + /** The key MongoDB understands, which is NOT always `op`. */ + readonly key: string; + readonly value: unknown; +} + +/** + * [#13524] Assemble lowered writes into one operator document, promoting every + * write whose key is already TAKEN into its own condition instead of letting it + * overwrite the sitting one. + * + * ## The defect this closes — a CLASS, not an instance + * + * Several authorable operators do not translate to a key of their own name. + * They write keys an author can ALSO write on the same field constraint, so two + * constraints land on one key of one object literal and the second assignment + * wins. One constraint disappears — no error, no warning, and nothing in the + * emitted document to see it by — and WHICH one disappears is decided by the + * author's key order, because that is the order `Object.entries` walks. + * + * Enumerated over the whole declared vocabulary (probed one operator at a time + * and intersected, not reasoned), the contested keys on this face are: + * + * | lowered key | written by | + * |---|---| + * | `$eq` | `$eq`, `$null: true`, `$exists: false` | + * | `$ne` | `$ne`, `$null: false`, `$exists: true` | + * | `$gte` | `$gte`, `$between` | + * | `$lte` | `$lte`, `$between` | + * | `$lt` | `$lt`, `$lte` (BARE CALENDAR DAY — #4042's half-open rewrite), `$between` (bare-day max) | + * | `$regex` | `$contains`, `$startsWith`, `$endsWith`, `$icontains` | + * + * Two of those rows had not been named anywhere. `$lte` → `$lt` is a clobber on + * a bare `YYYY-MM-DD` upper bound. And the `$regex` row is this driver's alone: + * `driver-memory` promotes its string family to `$and` branches already + * (`_multiRegex`), while here `{name: {$startsWith: 'a', $endsWith: 'z'}}` + * emitted `{name: {$regex: 'z$'}}` and its key-swapped twin `{name: {$regex: + * '^a'}}` — measured, one anchor silently gone in each direction. `$not` is + * written by `$notContains` and by nothing else, so it is covered here by + * construction rather than curatively. + * + * ## The rule, and why it is this one + * + * Free key → merge inline (the overwhelmingly common case). Taken key → the + * write becomes its own `$and` branch on the same field, where both constraints + * survive. That is exactly the guard #13195 landed for `$exists` alone, + * generalised to every writer rather than restated once per operator. + * `driver-memory`'s reference matcher loops the operators and therefore cannot + * express this defect at all; it is the oracle both drivers agree with. + * + * ## Why rank, and not author order + * + * The inline slot goes to the LOWEST-RANKED writer, never to the first one the + * author happened to type. Author order would keep the ANSWER correct — `$and` + * is commutative — while leaving the emitted DOCUMENT a function of key order, + * which is the property this card exists to remove. Ranking makes which + * constraint sits inline, and which branches are promoted, a pure function of + * the constraint SET: the two key orders of one predicate emit DEEP-EQUAL + * documents, and a test asserts exactly that. + * + * ⚠️ Deep-equal, not byte-identical, and the difference is deliberate. Writes + * are COLLECTED in author order and only assembled here, so refusals still fire + * in the order the author wrote them and an UNCONTESTED key still lands in its + * original insertion position — `{$between: […], $lte: x}` and its twin emit + * `{$gte, $lte}` with the two keys in opposite insertion order, carrying the + * same constraints. A filter with no contested key emits exactly what it + * emitted before. + * + * The promotion travels to `translateCondition` as an `_extraAnd` sentinel, + * which lifts each branch into its `$and` list — the shape `_presenceAnd` used + * before this subsumed it. + */ +function assembleLoweredWrites(writes: readonly LoweredWrite[]): Record { + const result: Record = {}; + const byKey = new Map(); + for (const write of writes) { + const group = byKey.get(write.key); + if (group) group.push(write); + else byKey.set(write.key, [write]); + } + const extraAnd: Record[] = []; + for (const [key, group] of byKey) { + // Single writer is the common case and must not be perturbed at all. + if (group.length > 1) { + group.sort( + (a, b) => + (FIELD_OPERATOR_RANK.get(a.op) ?? Number.MAX_SAFE_INTEGER) - + (FIELD_OPERATOR_RANK.get(b.op) ?? Number.MAX_SAFE_INTEGER), + ); + } + result[key] = group[0]!.value; + for (let i = 1; i < group.length; i++) extraAnd.push({ [key]: group[i]!.value }); + } + if (extraAnd.length > 0) result._extraAnd = extraAnd; + return result; +} + /** * Translate ObjectStack field-level operators into MongoDB operators. * @@ -719,12 +841,22 @@ function translateFieldOperators( field = '', path = 'filter', ): Record { - const result: Record = {}; const store = (v: unknown) => coerceTemporalValue(v, kind); - /** [#13195] `$exists`, lowered — see the merge at the end of this function. */ - let presence: Record | undefined; + /** + * [#13524] Every lowered write, collected in AUTHOR order and assembled by + * {@link assembleLoweredWrites} after the loop. Collected rather than + * assigned because an arm cannot know whether the key it wants is already + * spoken for by a sibling operator the author wrote LATER — which is the + * whole of the defect this replaces. It subsumes #13195's single-operator + * `presence` collection: `$exists` is one writer among the rest now. + */ + const writes: LoweredWrite[] = []; for (const [op, value] of Object.entries(ops)) { + /** Record one lowered write for the operator this iteration is on. */ + const put = (loweredKey: string, loweredValue: unknown): void => { + writes.push({ op, key: loweredKey, value: loweredValue }); + }; switch (op) { // Direct mappings (ObjectStack → MongoDB are identical) case '$eq': @@ -734,7 +866,7 @@ function translateFieldOperators( case '$lt': case '$in': case '$nin': - result[op] = store(value); + put(op, store(value)); break; // [#13195] Value-independent — a presence predicate takes a boolean, not @@ -759,9 +891,11 @@ function translateFieldOperators( // already agreed, is unmoved. Measured on a real mongod 8.2.6 while this // cell was pinned. case '$exists': - // Collected, not assigned: the merge at the end of this function has to - // know whether the key this lowers to is already spoken for. - presence = value === true ? { $ne: null } : { $eq: null }; + // Collected, not assigned: the assembly has to know whether the key + // this lowers to is already spoken for. [#13524] Since the class was + // generalised this arm is no longer special — it `put`s like every + // other writer and the shared rule ranks it. + put(value === true ? '$ne' : '$eq', null); break; case '$lte': { @@ -769,9 +903,12 @@ function translateFieldOperators( // driver-sql twin is #3777): `<= '2026-07-28'` compiles half-open // (`< '2026-07-29'`) so instants on the final day stay in; // order-equivalent to `<=` for plain `YYYY-MM-DD` date values. + // [#13524] `$lt` here is a key an AUTHOR can also write — this arm is a + // member of the clobber class that no card had named. See + // {@link assembleLoweredWrites}. const nextDay = nextUtcCalendarDay(value); - if (nextDay != null) result.$lt = store(nextDay); - else result.$lte = store(value); + if (nextDay != null) put('$lt', store(nextDay)); + else put('$lte', store(value)); break; } @@ -794,7 +931,7 @@ function translateFieldOperators( // metacharacters. The deliberate case-insensitive spelling is // `$icontains` below — one operator, one answer, per #5374. case '$contains': - result.$regex = escapeRegex(String(value)); + put('$regex', escapeRegex(String(value))); break; case '$notContains': @@ -802,15 +939,20 @@ function translateFieldOperators( // pattern under `$not` is the same predicate, so a flag left here would // have excluded rows the positive form includes — the negation widening // rather than mirroring. - result.$not = { $regex: escapeRegex(String(value)) }; + put('$not', { $regex: escapeRegex(String(value)) }); break; + // [#13524] These four all write `$regex`, so before the assembly below + // two of them on one field left only the LAST — `driver-memory` promoted + // its string family to `$and` branches years earlier and this face never + // did. Measured: `{$startsWith:'a', $endsWith:'z'}` emitted + // `{$regex:'z$'}` and the key-swapped twin `{$regex:'^a'}`. case '$startsWith': - result.$regex = `^${escapeRegex(String(value))}`; + put('$regex', `^${escapeRegex(String(value))}`); break; case '$endsWith': - result.$regex = `${escapeRegex(String(value))}$`; + put('$regex', `${escapeRegex(String(value))}$`); break; // [#6520] `$icontains` — case-insensitive over ASCII and nothing else. @@ -830,7 +972,7 @@ function translateFieldOperators( // to keep: `$options` is a RETIRED operator (#5702), and the only // sanctioned case-insensitive answer on this driver is the pattern below. case '$icontains': - result.$regex = asciiCaseInsensitiveRegexSource(String(value)); + put('$regex', asciiCaseInsensitiveRegexSource(String(value))); break; // Range operator → $gte + upper bound (half-open on a bare-day max, @@ -854,10 +996,10 @@ function translateFieldOperators( // path spelling, so the wire answer is identical whichever fires. case '$between': { if (!isBetweenRange(value)) throw malformedBetweenError(field, value, `${path}.$between`); - result.$gte = store(value[0]); + put('$gte', store(value[0])); const betweenNextDay = nextUtcCalendarDay(value[1]); - if (betweenNextDay != null) result.$lt = store(betweenNextDay); - else result.$lte = store(value[1]); + if (betweenNextDay != null) put('$lt', store(betweenNextDay)); + else put('$lte', store(value[1])); break; } @@ -885,9 +1027,9 @@ function translateFieldOperators( case '$null': if (typeof value !== 'boolean') throw nonBooleanNullComparandError(field, value, `${path}.$null`); if (value === true) { - result.$eq = null; + put('$eq', null); } else { - result.$ne = null; + put('$ne', null); } break; @@ -935,35 +1077,15 @@ function translateFieldOperators( } } - // [#13195] Merge the lowered `$exists`, and do NOT let it clobber a sibling. - // - // The lowering reuses `$ne` / `$eq` — MongoDB keys an AUTHOR can also write - // on the same field. `{name: {$exists: true, $ne: 'b'}}` would assign `$ne` - // twice into one object, and whichever ran last would win: one constraint - // vanishes, and WHICH one depends on the author's key order. Measured before - // this guard existed: that filter emitted `{name: {$ne: 'b'}}` and the - // key-swapped `{name: {$ne: 'b', $exists: true}}` emitted `{name: {$ne: - // null}}` — one predicate, two different documents, neither carrying both - // constraints. - // - // Free key → merge inline (the common case, `$exists` alone on a field). - // Taken → hand the caller a `_presenceAnd` sentinel, which `translateCondition` - // lifts into its `$and` list, where both constraints survive. + // [#13524] Assemble every lowered write, and do NOT let one clobber another. // - // ⚠️ Scope: this guards the operator #13195 moved, and only it. The identical - // clobber is reachable today through `$null` (`$eq`/`$ne`) and `$between` - // (`$gte`/`$lte`/`$lt`) — measured, pre-existing, filed separately rather - // than half-fixed here. - if (presence) { - const presenceKey = Object.keys(presence)[0]!; - if (Object.prototype.hasOwnProperty.call(result, presenceKey)) { - result._presenceAnd = presence; - } else { - Object.assign(result, presence); - } - } - - return result; + // #13195 landed this rule for `$exists` alone and said in this spot that the + // identical clobber was reachable through `$null` and `$between`. Enumerating + // the declared vocabulary instead of the noticed operators found two more on + // this face: `$lte` on a bare calendar day (it lowers onto `$lt`), and the + // whole `$regex` string family, which `driver-memory` had promoted for years + // and this driver never did. See {@link assembleLoweredWrites}. + return assembleLoweredWrites(writes); } /** From 5daac3bcf9303f4c8db84140a6e9f0127ea91c35 Mon Sep 17 00:00:00 2001 From: zhuangjianguo Date: Sun, 30 Aug 2026 21:36:28 +0000 Subject: [PATCH 2/3] test(drivers): enumerate the operator-key clobber class, both key orders The vocabulary sweep is written against the declared operator set rather than a hand-list, so a new operator cannot join without this coverage being told. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L --- .../src/memory-operator-key-clobber.test.ts | 340 ++++++++++++++++++ .../src/mongodb-operator-key-clobber.test.ts | 271 ++++++++++++++ 2 files changed, 611 insertions(+) create mode 100644 packages/drivers/driver-memory/src/memory-operator-key-clobber.test.ts create mode 100644 packages/drivers/driver-mongodb/src/mongodb-operator-key-clobber.test.ts diff --git a/packages/drivers/driver-memory/src/memory-operator-key-clobber.test.ts b/packages/drivers/driver-memory/src/memory-operator-key-clobber.test.ts new file mode 100644 index 0000000000..9c1d8c6351 --- /dev/null +++ b/packages/drivers/driver-memory/src/memory-operator-key-clobber.test.ts @@ -0,0 +1,340 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#13524] A field operator whose lowering reuses another operator's key — + * measured over the WHOLE declared vocabulary, in BOTH key orders. + * + * ## The defect + * + * `normalizeFieldOperators` translated a field constraint by writing into ONE + * object literal, keyed by the name mingo understands. Several authorable + * operators do not lower to a key of their own name, so two constraints landed + * on one key and the second assignment won: one constraint disappeared with no + * error, no warning and no trace in the emitted document — and WHICH one + * disappeared was decided by the author's key order, since that is the order + * `Object.keys` walks. + * + * Measured on the fixture below, on `origin/main` at `50cf2940b9`, before the + * repair: + * + * | filter | live path | reference matcher | + * |---|---|---| + * | `{name: {$null: false, $ne: 'b'}}` | `['1','3']` | `['1']` | + * | `{name: {$ne: 'b', $null: false}}` | `['1','2']` | `['1']` | + * + * One predicate, written two ways that differ only in key order, returned two + * different row sets — and neither was the answer. That is the card's own + * table, reproduced rather than trusted. + * + * ## The oracle + * + * `memory-matcher.ts`'s `match()` loops the operators and therefore CANNOT + * express this defect, and it is the face #5962 aligned. Every cell below is + * scored against it. + * + * ⚠️ With ONE measured exception, kept deliberately and NOT repaired here: + * `$between` ALONE already disagrees with the reference matcher on a row whose + * value is `null` (live `['1','2']`, matcher `['1','2','4']` on the enumeration + * fixture). That is the reference matcher's own `$between` defect — a + * separately queued card — so the sweep below scores the live path against + * ITSELF (the composition law) rather than against the matcher, and the + * matcher is the oracle for the named cells, where the two agree operator by + * operator. + * + * ## Why the sweep ranges over the vocabulary and not over three operators + * + * The card named `$null`, `$between` and `$notContains`. Enumerating instead of + * exampling changed that list in both directions: + * + * - **`$lte` on a bare calendar day is a FOURTH member.** `#4042`'s whole-day + * rewrite compiles `$lte: '2026-07-28'` half-open, onto `$lt` — a key an + * author writes too. + * - **`$notContains` is NOT reachable.** It lowers onto `$not`, and nothing + * else in the declared vocabulary writes `$not` (`$not` is a LOGICAL + * operator, absent from `SUPPORTED_FIELD_OPERATORS`, so it cannot be + * authored beside it on one field). It is covered by construction, not + * curatively — which is the point of a rule for the class. + * + * The sweep is written against `SUPPORTED_FIELD_OPERATORS` rather than a + * hand-list so a nineteenth operator cannot join the vocabulary without either + * being covered here or failing {@link comparand coverage} loudly. + * + * ## Both key orders, always + * + * Every cell is asserted in both orders. A one-direction test is exactly why + * this survived: on the broken code it passes half the time. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; + +import { InMemoryDriver } from './memory-driver.js'; +import { match } from './memory-matcher.js'; +import { MemoryAnalyticsService } from './memory-analytics.js'; +import { SUPPORTED_FIELD_OPERATORS } from './filter-refusal.js'; + +/** The card's fixture, widened with the two columns its extra cells need. */ +const ROWS: Array> = [ + { id: '1', name: 'a', score: 1, d: '2026-07-01' }, + { id: '2', name: 'b', score: 5, d: '2026-07-28' }, + { id: '3', name: null, score: 9, d: '2026-08-15' }, +]; + +/** + * The sweep's fixture — one string column so every declared operator applies, + * and both readings of "no value" (`null` and an ABSENT key), because the two + * reach different mingo rules. + */ +const SWEEP_ROWS: Array> = [ + { id: '1', v: '2026-07-01' }, + { id: '2', v: '2026-07-15' }, + { id: '3', v: '2026-07-28' }, + { id: '4', v: null }, + { id: '5' }, +]; + +const sorted = (ids: string[]): string[] => [...ids].sort(); + +let driver: InMemoryDriver; +let sweepDriver: InMemoryDriver; + +beforeAll(async () => { + driver = new InMemoryDriver({ persistence: false }); + await driver.connect(); + for (const row of ROWS) await driver.create('t', { ...row }); + + sweepDriver = new InMemoryDriver({ persistence: false }); + await sweepDriver.connect(); + for (const row of SWEEP_ROWS) await sweepDriver.create('t', { ...row }); +}); + +afterAll(async () => { + await driver.disconnect(); + await sweepDriver.disconnect(); +}); + +/** The LIVE query path: `find()` → `normalizeFilterCondition` → mingo. */ +async function liveIds(where: unknown): Promise { + const out = await driver.find('t', { where } as never); + return sorted((out as Array>).map((r) => String(r.id))); +} + +async function sweepIds(where: unknown): Promise { + const out = await sweepDriver.find('t', { where } as never); + return sorted((out as Array>).map((r) => String(r.id))); +} + +/** The ORACLE: the reference matcher, which loops the operators. */ +const matcherIds = (where: unknown): string[] => + sorted(ROWS.filter((r) => match(r, where)).map((r) => String(r.id))); + +/** Both key orders of one two-operator field constraint. */ +function bothOrders( + field: string, + a: readonly [string, unknown], + b: readonly [string, unknown], +): [Record, Record] { + return [ + { [field]: { [a[0]]: a[1], [b[0]]: b[1] } }, + { [field]: { [b[0]]: b[1], [a[0]]: a[1] } }, + ]; +} + +describe('[#13524] the card`s measured table, reproduced and repaired', () => { + it('`$null` + `$ne` — the cell the card demonstrates', async () => { + const [ab, ba] = bothOrders('name', ['$null', false], ['$ne', 'b']); + // Was ['1','3'] / ['1','2'] — two row sets for one predicate, neither the + // answer. '3' has no value (the `$null: false` it violates was dropped); + // '2' is the row `$ne: 'b'` excludes (that constraint was dropped instead). + expect(await liveIds(ab)).toEqual(['1']); + expect(await liveIds(ba)).toEqual(['1']); + expect(matcherIds(ab)).toEqual(['1']); + expect(matcherIds(ba)).toEqual(['1']); + }); + + it('`$null` + `$eq` — the other half of the same contested key pair', async () => { + const [ab, ba] = bothOrders('name', ['$null', true], ['$eq', 'a']); + // Was ['1'] / ['3']. "is null AND equals 'a'" is a contradiction: no row. + expect(await liveIds(ab)).toEqual([]); + expect(await liveIds(ba)).toEqual([]); + expect(matcherIds(ab)).toEqual([]); + expect(matcherIds(ba)).toEqual([]); + }); + + it('`$between` + `$gte` — the range`s lower bound, contested', async () => { + const [ab, ba] = bothOrders('score', ['$between', [1, 5]], ['$gte', 9]); + // Was [] / ['1','2']: one order kept `$gte: 9` (empty, correct by + // accident), the other dropped it and returned the whole range. + expect(await liveIds(ab)).toEqual([]); + expect(await liveIds(ba)).toEqual([]); + expect(matcherIds(ab)).toEqual([]); + expect(matcherIds(ba)).toEqual([]); + }); + + it('`$between` + `$lte` — the range`s upper bound, contested', async () => { + const [ab, ba] = bothOrders('score', ['$between', [1, 5]], ['$lte', 9]); + // Was ['1','2','3'] / ['1','2'] — the first WIDENED past the range. + expect(await liveIds(ab)).toEqual(['1', '2']); + expect(await liveIds(ba)).toEqual(['1', '2']); + expect(matcherIds(ab)).toEqual(['1', '2']); + expect(matcherIds(ba)).toEqual(['1', '2']); + }); + + it('THE FOURTH MEMBER — `$lte` on a bare calendar day lowers onto `$lt`', async () => { + const [ab, ba] = bothOrders('d', ['$lte', '2026-07-28'], ['$lt', '2026-07-02']); + // Was ['1'] / ['1','2']. #4042 compiles a bare `YYYY-MM-DD` upper bound + // half-open — onto `$lt`, which the author is also writing here. No card + // had named this cell; the vocabulary sweep below is what found it. + expect(await liveIds(ab)).toEqual(['1']); + expect(await liveIds(ba)).toEqual(['1']); + expect(matcherIds(ab)).toEqual(['1']); + expect(matcherIds(ba)).toEqual(['1']); + }); + + it('`$between` with a bare-day max contests `$lt` for the same reason', async () => { + const [ab, ba] = bothOrders('d', ['$between', ['2026-07-01', '2026-07-28']], ['$lt', '2026-07-02']); + // Was ['1'] / ['1','2']. + expect(await liveIds(ab)).toEqual(['1']); + expect(await liveIds(ba)).toEqual(['1']); + expect(matcherIds(ab)).toEqual(['1']); + expect(matcherIds(ba)).toEqual(['1']); + }); + + it('`$exists` + `$ne` — #13195`s cell, unmoved by the generalisation', async () => { + const [ab, ba] = bothOrders('name', ['$exists', true], ['$ne', 'b']); + expect(await liveIds(ab)).toEqual(['1']); + expect(await liveIds(ba)).toEqual(['1']); + expect(matcherIds(ab)).toEqual(['1']); + expect(matcherIds(ba)).toEqual(['1']); + }); + + it('`$notContains` is covered by construction — nothing else writes `$not`', () => { + // The enumeration's negative result, pinned so it cannot rot silently: a + // future operator lowering onto `$not` makes this list grow, and the sweep + // below is what would then catch the clobber. + expect(SUPPORTED_FIELD_OPERATORS.has('$not')).toBe(false); + }); +}); + +/** + * The comparand for each declared operator. Chosen so every one selects a + * NON-TRIVIAL subset of the sweep fixture — an operator that matched all rows + * or none would make its pairs pass without discriminating. + * + * `$lte`'s comparand is a bare calendar day on purpose: that is the arm whose + * lowering moves to `$lt`. + */ +const SWEEP_COMPARANDS: Readonly> = Object.freeze({ + $eq: '2026-07-15', + $ne: '2026-07-15', + $gt: '2026-07-01', + $gte: '2026-07-15', + $lt: '2026-07-28', + $lte: '2026-07-15', + $in: ['2026-07-15', '2026-07-28'], + $nin: ['2026-07-01'], + $between: ['2026-07-01', '2026-07-15'], + $contains: '07-15', + $notContains: '07-01', + $startsWith: '2026-07', + $endsWith: '-15', + $icontains: '07-15', + $like: '%07-15', + $ilike: '%07-15', + $null: false, + $exists: true, +}); + +describe('[#13524] the ENUMERATION — every declared operator, every pair, both orders', () => { + it('the comparand table covers the declared vocabulary exactly', () => { + // The sweep is only an enumeration while this holds. A nineteenth operator + // fails HERE, loudly, instead of being skipped silently. + expect(Object.keys(SWEEP_COMPARANDS).sort()).toEqual([...SUPPORTED_FIELD_OPERATORS].sort()); + }); + + it('no pair loses a constraint, and no pair depends on key order', async () => { + const ops = [...SUPPORTED_FIELD_OPERATORS]; + const alone = new Map(); + for (const op of ops) alone.set(op, await sweepIds({ v: { [op]: SWEEP_COMPARANDS[op] } })); + + const failures: string[] = []; + for (const a of ops) { + for (const b of ops) { + if (a === b) continue; + const [ab, ba] = bothOrders('v', [a, SWEEP_COMPARANDS[a]], [b, SWEEP_COMPARANDS[b]]); + const gotAb = await sweepIds(ab); + const gotBa = await sweepIds(ba); + // The COMPOSITION LAW, scored on the live path against itself: two + // constraints on one field select exactly the rows both select alone. + // A clobber breaks it in one direction; scoring the live path against + // itself keeps the sweep independent of `$between`'s separate + // reference-matcher divergence (see the file docblock). + const expected = alone.get(a)!.filter((id) => alone.get(b)!.includes(id)); + if (JSON.stringify(gotAb) !== JSON.stringify(expected)) { + failures.push(`${a}+${b} (a first): ${JSON.stringify(gotAb)} != ${JSON.stringify(expected)}`); + } + if (JSON.stringify(gotBa) !== JSON.stringify(expected)) { + failures.push(`${b}+${a} (b first): ${JSON.stringify(gotBa)} != ${JSON.stringify(expected)}`); + } + } + } + expect(failures).toEqual([]); + }); +}); + +const CUBE = { + name: 'deals', + title: 'Deals', + sql: 't', + measures: { total: { name: 'total', label: 'Total', type: 'count', sql: 'id' } }, + dimensions: { + id: { name: 'id', label: 'Id', type: 'string', sql: 'id' }, + name: { name: 'name', label: 'Name', type: 'string', sql: 'name' }, + }, + public: true, +} as never; + +async function analytics(where: unknown): Promise<{ executed: string[]; sql: string }> { + const service = new MemoryAnalyticsService({ driver, cubes: [CUBE] } as never); + const query = { cube: 'deals', measures: ['total'], dimensions: ['id'], where } as never; + const executed = sorted( + ((await service.query(query)).rows as Array>).map((r) => String(r.id)), + ); + const { sql } = await service.generateSql(query); + return { executed, sql: sql.replace(/\s+/g, ' ') }; +} + +/** + * The THIRD instance the card names, and the widest: `query()` keyed its + * `$match` by FIELD PATH, so a second predicate on a member replaced the first + * ENTIRELY — for every operator pair, not only the ones sharing a lowered key. + */ +describe('[#13524] the analytics face — a WHOLESALE clobber, one level up', () => { + it('two operators on one member, neither sharing a lowered key', async () => { + const [ab, ba] = bothOrders('name', ['$contains', 'a'], ['$ne', 'b']); + // `$contains` lowers to `$regex` and `$ne` to `$ne` — no contested key at + // all, and the translators never lost this one. The analytics face did: + // was ['1','3'] / ['1']. + expect((await analytics(ab)).executed).toEqual(['1']); + expect((await analytics(ba)).executed).toEqual(['1']); + expect(matcherIds(ab)).toEqual(['1']); + expect(matcherIds(ba)).toEqual(['1']); + }); + + it('`$and`-folded nodes on one member clobbered too — that is the common shape', async () => { + // `flattenFilterCondition` folds `$and` into the same flat list, so this is + // the SAME defect written the way a dashboard actually authors it. + const ab = { $and: [{ name: { $contains: 'a' } }, { name: { $ne: 'b' } }] }; + const ba = { $and: [{ name: { $ne: 'b' } }, { name: { $contains: 'a' } }] }; + expect((await analytics(ab)).executed).toEqual(['1']); + expect((await analytics(ba)).executed).toEqual(['1']); + }); + + it('the echoed SQL always carried both — it was `query()` that disagreed with it', async () => { + // `generateSql` pushes into a LIST and so never clobbered. Before the + // repair the echo and the executed answer described different filters, + // which is the shape that makes a widened chart unfalsifiable. + const { sql } = await analytics({ name: { $contains: 'a', $ne: 'b' } }); + expect(sql).toContain('name'); + expect(sql.match(/AND/g)?.length ?? 0).toBeGreaterThanOrEqual(1); + }); +}); diff --git a/packages/drivers/driver-mongodb/src/mongodb-operator-key-clobber.test.ts b/packages/drivers/driver-mongodb/src/mongodb-operator-key-clobber.test.ts new file mode 100644 index 0000000000..740cfffc08 --- /dev/null +++ b/packages/drivers/driver-mongodb/src/mongodb-operator-key-clobber.test.ts @@ -0,0 +1,271 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#13524] A field operator whose lowering reuses another operator's key — + * enumerated over the declared vocabulary, asserted in BOTH key orders. + * + * ## The defect, visible in the emitted document + * + * `translateFieldOperators` wrote every lowered key into ONE object literal. + * Several authorable operators do not translate to a key of their own name, so + * two constraints landed on one key and the second assignment won. This driver + * shows the loss a layer earlier than `driver-memory` does — no server needed: + * + * ``` + * translateFilter({name: {$null: false, $ne: 'b'}}) -> {name: {$ne: 'b'}} + * translateFilter({name: {$ne: 'b', $null: false}}) -> {name: {$ne: null}} + * ``` + * + * One predicate, two documents, neither carrying both constraints — and which + * constraint survived was decided by the author's key order. + * + * ## What enumerating (rather than exampling) changed + * + * The card named `$null`, `$between` and `$notContains`. Probing every declared + * operator one at a time and intersecting the key sets moved that list twice: + * + * - **`$lte` on a bare calendar day is a fourth member** — #4042's whole-day + * rewrite compiles it half-open, onto `$lt`. + * - **The whole `$regex` family is a fifth, and it is THIS DRIVER'S ALONE.** + * `$contains` / `$startsWith` / `$endsWith` / `$icontains` all write + * `$regex`; `driver-memory` has promoted its string family to `$and` + * branches for years (`_multiRegex`) and this face never did. Measured: + * `{name: {$startsWith: 'a', $endsWith: 'z'}}` emitted `{name: {$regex: + * 'z$'}}` and its key-swapped twin `{name: {$regex: '^a'}}` — one anchor + * silently gone in each direction. + * - **`$notContains` is NOT reachable.** Nothing else writes `$not`, so it is + * covered by construction rather than curatively. + * + * {@link LOWERED_KEYS} below is that enumeration, executable: it is asserted + * against what the translator actually emits, so a new operator or a changed + * lowering cannot join the vocabulary without this file being told. + */ + +import { describe, it, expect } from 'vitest'; + +import { FILTER_OPERATORS } from '@objectstack/spec/data'; + +import { translateFilter } from './mongodb-filter.js'; + +const doc = (where: unknown): Record => + translateFilter(where as never) as Record; + +/** Both key orders of one two-operator field constraint. */ +function bothOrders( + field: string, + a: readonly [string, unknown], + b: readonly [string, unknown], +): [Record, Record] { + return [ + { [field]: { [a[0]]: a[1], [b[0]]: b[1] } }, + { [field]: { [b[0]]: b[1], [a[0]]: a[1] } }, + ]; +} + +/** + * Every `field -> operator -> comparand` leaf an emitted document carries, + * `$and` branches included. A constraint that was clobbered is simply absent + * from this set — which is what makes "nothing was dropped" assertable without + * a running server. + */ +function constraintsOf(node: unknown, into = new Set()): Set { + if (!node || typeof node !== 'object') return into; + for (const [key, value] of Object.entries(node as Record)) { + if (key === '$and' || key === '$or' || key === '$nor') { + for (const branch of value as unknown[]) constraintsOf(branch, into); + continue; + } + if (value && typeof value === 'object' && !Array.isArray(value) && !(value instanceof Date)) { + for (const [op, comparand] of Object.entries(value as Record)) { + into.add(`${key}|${op}|${JSON.stringify(comparand)}`); + } + continue; + } + into.add(`${key}|$eq|${JSON.stringify(value)}`); + } + return into; +} + +const setOf = (where: unknown): string[] => [...constraintsOf(doc(where))].sort(); + +describe('[#13524] the ENUMERATION — which lowered key each declared operator writes', () => { + /** + * Measured, one operator at a time. An operator with a comparand-dependent + * lowering is probed with BOTH comparands, because that dependence is where + * two of the five contested keys come from. + */ + const LOWERED_KEYS: ReadonlyArray = [ + ['$eq', 'x', ['$eq']], + ['$ne', 'x', ['$ne']], + ['$gt', 1, ['$gt']], + ['$gte', 1, ['$gte']], + ['$lt', 1, ['$lt']], + ['$lte', 1, ['$lte']], + ['$lte', '2026-07-28', ['$lt']], // BARE CALENDAR DAY (#4042) + ['$in', ['x'], ['$in']], + ['$nin', ['x'], ['$nin']], + ['$between', [1, 2], ['$gte', '$lte']], + ['$between', ['2026-01-01', '2026-07-28'], ['$gte', '$lt']], + ['$contains', 'x', ['$regex']], + ['$notContains', 'x', ['$not']], + ['$startsWith', 'x', ['$regex']], + ['$endsWith', 'x', ['$regex']], + ['$icontains', 'x', ['$regex']], + ['$null', true, ['$eq']], + ['$null', false, ['$ne']], + ['$exists', true, ['$ne']], + ['$exists', false, ['$eq']], + ]; + + it('the probe table covers the declared vocabulary exactly', () => { + // Only an enumeration while this holds. A seventeenth operator fails HERE + // rather than being skipped in silence. + expect([...new Set(LOWERED_KEYS.map(([op]) => op))].sort()).toEqual([...FILTER_OPERATORS].sort()); + }); + + it.each(LOWERED_KEYS)('`%s` with %j lowers onto %j', (op, comparand, keys) => { + const emitted = doc({ f: { [op]: comparand } }); + expect(Object.keys(emitted.f as Record).sort()).toEqual([...keys].sort()); + }); + + it('the contested keys are exactly these five', () => { + const byKey = new Map>(); + for (const [op, , keys] of LOWERED_KEYS) { + for (const key of keys) { + const seen = byKey.get(key) ?? new Set(); + seen.add(op); + byKey.set(key, seen); + } + } + const contested = Object.fromEntries( + [...byKey.entries()] + .filter(([, ops]) => ops.size > 1) + .map(([key, ops]) => [key, [...ops].sort()]), + ); + expect(contested).toEqual({ + $eq: ['$eq', '$exists', '$null'], + $ne: ['$exists', '$ne', '$null'], + $gte: ['$between', '$gte'], + $lt: ['$between', '$lt', '$lte'], + $lte: ['$between', '$lte'], + $regex: ['$contains', '$endsWith', '$icontains', '$startsWith'], + }); + // `$not` is written by `$notContains` and by nothing else — the card's + // third named member, measured NOT reachable. + expect([...(byKey.get('$not') ?? [])]).toEqual(['$notContains']); + }); + + it('`$like` / `$ilike` are declared but not translated here — the boundary, unmoved', () => { + expect(() => doc({ f: { $like: 'x%' } })).toThrowError(/Unsupported filter operator/); + expect(() => doc({ f: { $ilike: 'x%' } })).toThrowError(/Unsupported filter operator/); + }); +}); + +describe('[#13524] the emitted document carries BOTH constraints, in either key order', () => { + it('`$null` + `$ne` — the card`s cell, one layer earlier than the row set', () => { + const [ab, ba] = bothOrders('name', ['$null', false], ['$ne', 'b']); + // Was {name:{$ne:'b'}} and {name:{$ne:null}} — one constraint each. + const expected = { $and: [{ name: { $ne: 'b' } }, { name: { $ne: null } }] }; + expect(doc(ab)).toEqual(expected); + expect(doc(ba)).toEqual(expected); + }); + + it('`$between` + `$gte` — the range`s contested lower bound', () => { + const [ab, ba] = bothOrders('score', ['$between', [1, 5]], ['$gte', 9]); + const expected = { $and: [{ score: { $gte: 9, $lte: 5 } }, { score: { $gte: 1 } }] }; + expect(doc(ab)).toEqual(expected); + expect(doc(ba)).toEqual(expected); + }); + + it('THE FOURTH MEMBER — `$lte` on a bare calendar day contests `$lt`', () => { + const [ab, ba] = bothOrders('d', ['$lte', '2026-07-28'], ['$lt', '2026-07-02']); + // Was {d:{$lt:'2026-07-02'}} and {d:{$lt:'2026-07-29'}} — a whole-day + // upper bound silently replacing the author's own strict bound, or the + // reverse, depending on which was typed second. + const expected = { $and: [{ d: { $lt: '2026-07-02' } }, { d: { $lt: '2026-07-29' } }] }; + expect(doc(ab)).toEqual(expected); + expect(doc(ba)).toEqual(expected); + }); + + it('THE FIFTH MEMBER, THIS DRIVER`S ALONE — two string operators both write `$regex`', () => { + const [ab, ba] = bothOrders('name', ['$startsWith', 'a'], ['$endsWith', 'z']); + // Was {name:{$regex:'z$'}} and {name:{$regex:'^a'}} — the other anchor gone + // with no trace. `driver-memory` answered this pair correctly throughout. + const expected = { $and: [{ name: { $regex: '^a' } }, { name: { $regex: 'z$' } }] }; + expect(doc(ab)).toEqual(expected); + expect(doc(ba)).toEqual(expected); + }); + + it('`$contains` + `$icontains` — the same family, the fold preserved on both', () => { + const [ab, ba] = bothOrders('name', ['$contains', 'a'], ['$icontains', 'z']); + const expected = { $and: [{ name: { $regex: 'a' } }, { name: { $regex: '[Zz]' } }] }; + expect(doc(ab)).toEqual(expected); + expect(doc(ba)).toEqual(expected); + }); + + it('`$exists` + `$ne` — #13195`s cell, emitted exactly as its guard emitted it', () => { + const [ab, ba] = bothOrders('name', ['$exists', true], ['$ne', 'b']); + const expected = { $and: [{ name: { $ne: 'b' } }, { name: { $ne: null } }] }; + expect(doc(ab)).toEqual(expected); + expect(doc(ba)).toEqual(expected); + }); + + it('an UNCONTESTED pair emits exactly what it emitted before — no promotion', () => { + // The repair must not reshape documents it has no business reshaping. + expect(doc({ name: { $ne: 'b', $notContains: 'q' } })).toEqual({ + name: { $ne: 'b', $not: { $regex: 'q' } }, + }); + expect(doc({ name: { $contains: 'a' } })).toEqual({ name: { $regex: 'a' } }); + }); +}); + +describe('[#13524] the sweep — every declared pair, both orders, nothing dropped', () => { + const COMPARANDS: Readonly> = Object.freeze({ + $eq: '2026-07-15', + $ne: '2026-07-15', + $gt: '2026-07-01', + $gte: '2026-07-15', + $lt: '2026-07-28', + $lte: '2026-07-15', // bare calendar day, on purpose + $in: ['2026-07-15'], + $nin: ['2026-07-01'], + $between: ['2026-07-01', '2026-07-15'], + $contains: '07-15', + $notContains: '07-01', + $startsWith: '2026-07', + $endsWith: '-15', + $icontains: '07-15', + $null: false, + $exists: true, + }); + + it('the comparand table covers the declared vocabulary exactly', () => { + expect(Object.keys(COMPARANDS).sort()).toEqual([...FILTER_OPERATORS].sort()); + }); + + it('every pair keeps both constraints and answers the same in either order', () => { + const ops = [...FILTER_OPERATORS]; + const alone = new Map(); + for (const op of ops) alone.set(op, setOf({ v: { [op]: COMPARANDS[op] } })); + + const failures: string[] = []; + for (const a of ops) { + for (const b of ops) { + if (a === b) continue; + const [ab, ba] = bothOrders('v', [a, COMPARANDS[a]], [b, COMPARANDS[b]]); + const gotAb = setOf(ab); + const gotBa = setOf(ba); + if (JSON.stringify(gotAb) !== JSON.stringify(gotBa)) { + failures.push(`${a}+${b}: key order changes the document — ${JSON.stringify(gotAb)} vs ${JSON.stringify(gotBa)}`); + } + // Nothing dropped: the pair carries every leaf either operator emits + // alone. A superset is correct — `$between` and `$gte` legitimately + // contribute two `$gte` leaves. + for (const want of [...alone.get(a)!, ...alone.get(b)!]) { + if (!gotAb.includes(want)) failures.push(`${a}+${b}: dropped ${want}`); + } + } + } + expect(failures).toEqual([]); + }); +}); From 3b7b4d4c4b57a0978901d014c5c3ed3a0d14b681 Mon Sep 17 00:00:00 2001 From: zhuangjianguo Date: Sun, 30 Aug 2026 21:50:34 +0000 Subject: [PATCH 3/3] chore: changeset for the lowered-operator-key clobber repair Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L --- .changeset/lowered-operator-key-clobber.md | 12 ++++++++++++ .../src/memory-operator-key-clobber.test.ts | 13 +++++++++++-- 2 files changed, 23 insertions(+), 2 deletions(-) create mode 100644 .changeset/lowered-operator-key-clobber.md diff --git a/.changeset/lowered-operator-key-clobber.md b/.changeset/lowered-operator-key-clobber.md new file mode 100644 index 0000000000..425fcc6d20 --- /dev/null +++ b/.changeset/lowered-operator-key-clobber.md @@ -0,0 +1,12 @@ +--- +'@objectstack/driver-memory': patch +'@objectstack/driver-mongodb': patch +--- + +Stop a field operator whose lowering reuses another operator's key from silently clobbering it. + +Both document-shaped drivers translated a field constraint by writing every lowered key into one object literal. Several authorable operators do not lower to a key of their own name — `$null` writes `$eq`/`$ne`, `$between` writes `$gte` plus `$lte`/`$lt`, `$lte` on a bare calendar day writes `$lt`, and MongoDB's `$contains`/`$startsWith`/`$endsWith`/`$icontains` all write `$regex` — so two constraints on one field landed on one key and the second assignment won. One constraint disappeared with no error and no trace in the emitted query, and which one disappeared was decided by the author's key order. On a row-level-security read scope, a dropped constraint is a widened one. + +A lowered write whose key is free now merges inline as before; a write whose key is already taken becomes its own `$and` branch, where both constraints survive. Which write keeps the inline slot is decided by the spec's declared operator order rather than by the author's key order, so one predicate emits the same query however it is spelled. `driver-memory`'s analytics (cube) face carried a wider form of the same defect — its `$match` was keyed by field path, so a second predicate on a member replaced the first entirely, for every operator pair — and is promoted the same way. + +Filters with no contested key are unchanged. diff --git a/packages/drivers/driver-memory/src/memory-operator-key-clobber.test.ts b/packages/drivers/driver-memory/src/memory-operator-key-clobber.test.ts index 9c1d8c6351..806e9c01c1 100644 --- a/packages/drivers/driver-memory/src/memory-operator-key-clobber.test.ts +++ b/packages/drivers/driver-memory/src/memory-operator-key-clobber.test.ts @@ -163,7 +163,14 @@ describe('[#13524] the card`s measured table, reproduced and repaired', () => { it('`$between` + `$gte` — the range`s lower bound, contested', async () => { const [ab, ba] = bothOrders('score', ['$between', [1, 5]], ['$gte', 9]); // Was [] / ['1','2']: one order kept `$gte: 9` (empty, correct by - // accident), the other dropped it and returned the whole range. + // ACCIDENT), the other dropped it and returned the whole range. + // + // ⭐ This cell and the `$between` + `$lte` one below fail in OPPOSITE + // directions on the broken code — measured, not reasoned. A suite that + // only ever wrote the operator first would have been green here and red + // below; one that only ever wrote it second, the reverse. Neither + // one-direction suite catches the class, which is why every cell in this + // file is asserted twice. expect(await liveIds(ab)).toEqual([]); expect(await liveIds(ba)).toEqual([]); expect(matcherIds(ab)).toEqual([]); @@ -172,7 +179,9 @@ describe('[#13524] the card`s measured table, reproduced and repaired', () => { it('`$between` + `$lte` — the range`s upper bound, contested', async () => { const [ab, ba] = bothOrders('score', ['$between', [1, 5]], ['$lte', 9]); - // Was ['1','2','3'] / ['1','2'] — the first WIDENED past the range. + // Was ['1','2','3'] / ['1','2'] — the FIRST order widened past the range, + // the second was right by accident. The mirror of the cell above; see its + // ⭐ note for why one direction proves nothing. expect(await liveIds(ab)).toEqual(['1', '2']); expect(await liveIds(ba)).toEqual(['1', '2']); expect(matcherIds(ab)).toEqual(['1', '2']);