Repository navigation
Commit 5dbeb7d
Fixes #20918
Clause-②: yes
On the engine-aggregate (ObjectQL) strategy, a `$not` over a
multi-valued lookup was refused `400 INVALID_FILTER`, while
`engine.find` and the native strategy answered rows. It now gets the
engine's rows. The fix is one producer spelling in
`ObjectQLStrategy.convertFilter`. The `set` / `notSet` leaves reach the
engine as `{ $null: false }` / `{ $null: true }`. They used to arrive as
`{ $ne: null }` and the bare `null`, and `driver-sql` refuses both over
a JSON column. No driver, engine or spec file is touched.
## Mechanism assumptions, measured
All readings use one fixture: a ledger with `title` (text), `owner`
(lookup) and `owners` (multiple lookup). The rows are a `['u1']`, b
`['u2']`, c `['u2','u1']`, d `['u3']`, and e, which has `owner` and
`owners` NULL. The composition is the shipped one: the real
`SecurityPlugin`, `ObjectQL` and `SqlDriver` (better-sqlite3), queried
as a member. The base is `9b0de7de7`, and "after" is this branch.
- **G1, the reference.** `engine.find` for `{ $not: { owners: {
$contains: 'u1' } } }` answers **b, d, e**. The single-valued control `{
$not: { owner: { $contains: 'u1' } } }` also answers b, d, e.
`engine.aggregate` grouped by `title` answers the same rows for every
`where` row below.
- **G2, where the guard comes from.** The analytics `where` door adds `{
owners: { $null: false } }` beside the `$contains` inside the `$not`
operand, and it does so twice:
- the shared lowering, `lowerFilterCondition` rule 3
(`packages/spec/src/data/filter-lowering.ts:445-446`,
`guardNullPolarity`'s `requireValue` arm), applied at
`filter-normalizer.ts:2467`;
- this package's own copy, `nullSafeNegationOperand`
(`filter-normalizer.ts:1436`, then `guardFieldEntry` at `:1652`).
The guard exists because of SQL's three-valued logic. `NOT (owners LIKE
'%u1%')` is UNKNOWN when `owners` is NULL, and a `WHERE` keeps only
TRUE, so row e would drop out of a negation that the JS backends answer
TRUE for. #5146 ruled the JS answer canonical. A total leaf, `NOT
(owners IS NOT NULL AND owners LIKE ...)`, keeps row e. Both copies of
the guard become `set` leaves. `ObjectQLStrategy.convertFilter` turned
`set` into `{ $ne: null }` at `objectql-strategy.ts:1920` on the base
(not `:1872`), and `notSet` into the bare `null` at `:1921`. `notSet`
carries the `allowNull` escape of `$not` operands and of `$ne` / `$nin`
/ `$notContains` (#5298).
- **G3, the engine already carries the guard.** `resolveThenLowerWhere`
(`packages/objectql/src/engine.ts:1204`) runs the same lowering on
`where`, `aggregations[i].filter` and `having`. Measured: `engine.find`
hands `driver-sql` `{ $not: { $and: [{ owners: { $null: false } }, {
owners: { $contains: 'u1' } }] } }` and answers b, d, e. The NULL row e
is IN the answer. That is the guard's intent: the negation answers a row
with no value instead of dropping it. `driver-sql` applies `$null` /
`$exists` on a JSON column (they are absent from
`JSON_COLUMN_INCOMPATIBLE_OPERATORS`). It refuses `$ne` and the bare
equality spelling for any comparand, `null` included.
- **Which route, from measurement.** "Stop adding the guard" would not
serve rows 5 to 7 of the table below, `{ owners: { $null: false } }`, `{
owners: { $exists: true } }` and `{ owners: { $null: true } }`. They
carry no guard at all: the author's own null test becomes the same `set`
/ `notSet` leaf, so the defect is the leaf's spelling, not the guard.
That route would also need the shared normalizer to diverge per
strategy, because the native compile and the `/analytics/sql` echo read
the same tree. The other route, "the guard in a form `driver-sql`
applies", serves every row. It changes one producer, and it speaks the
lowering's own spelling, so the engine now recognises the guard as one
it carries. Measured on the base: the driver received three `owners`
guards for the card's filter, one of them added by the engine. After the
fix it receives two, and the engine adds none. (The two are this
package's lowering and its hand copy, which is #5146's documented
idempotent double.)
- **G4, the native strategy is unchanged.** Every row below answers the
same before and after. The native strategy compiles its own SQL, and
`native-sql-strategy.ts` is untouched.
- **G5, `Clause-②: yes`.** Widening: 9 `where` rows and 3 measure-filter
rows go from 400 to rows on the ObjectQL path, at the cube read and at
the dataset door. Narrowing: none measured. Every row answered before is
answered with the same rows after: every single-valued row, `$contains`
on the multi-valued field, the read scope, and the native strategy on
all rows. The 10 re-judged pins below moved only the engine-input
spelling. None of their row expectations moved.
## Per row
The ObjectQL cell is the same at the cube read
(`AnalyticsService.query`, which `POST /api/v1/analytics/query` relays)
and at the dataset door (`POST /api/v1/analytics/dataset/query`,
`selection.runtimeFilter`). The native strategy answered the same before
and after on every row, so one column carries it.
| # | filter (`where`) | engine (reference) | native, before = after |
ObjectQL before | ObjectQL after |
|--:|:--|:--|:--|:--|:--|
| 1 | `{ $not: { owners: { $contains: 'u1' } } }` | b, d, e | b, d, e |
400 `INVALID_FILTER` (`$ne`) | **b, d, e** |
| 2 | `{ $not: { owners: { $notContains: 'u1' } } }` | a, c | a, c | 400
(bare equality) | **a, c** |
| 3 | `{ $not: { $or: [{ owners: { $contains: 'u1' } }, { title: 'b' }]
} }` | d, e | d, e | 400 (`$ne`) | **d, e** |
| 4 | `{ owners: { $notContains: 'u1' } }` | b, d, e | b, d, e | 400
(bare equality) | **b, d, e** |
| 5 | `{ owners: { $null: false } }` | a, b, c, d | a, b, c, d | 400
(`$ne`) | **a, b, c, d** |
| 6 | `{ owners: { $exists: true } }` | a, b, c, d | a, b, c, d | 400
(`$ne`) | **a, b, c, d** |
| 7 | `{ owners: { $null: true } }` | e | e | 400 (bare equality) |
**e** |
| 8 | `{ owners: { $ne: null } }` | 400 (`$ne`) | a, b, c, d | 400
(`$ne`) | a, b, c, d (see Acceptance notes) |
| 9 | `{ owners: null }` | 400 (bare equality) | e | 400 (bare equality)
| e (see Acceptance notes) |
| 10 | `{ owners: { $contains: 'u1' } }` | a, c | a, c | a, c | a, c |
| 11 | `{ $not: { owner: { $contains: 'u1' } } }` (single-valued
control) | b, d, e | b, d, e | b, d, e | b, d, e |
| 12 | `{ $not: { owner: { $eq: 'u1' } } }` | b, d, e | b, d, e | b, d,
e | b, d, e |
| 13 | `{ $not: { title: { $contains: 'a' } } }` | b, c, d, e | b, c, d,
e | b, c, d, e | b, c, d, e |
| 14 | `{ owner: { $ne: null } }` | a, b, c, d | a, b, c, d | a, b, c, d
| a, b, c, d |
| 15 | `{ owner: null }` | e | e | e | e |
| 16 | `{ owner: { $notContains: 'u1' } }` | b, d, e | b, d, e | b, d, e
| b, d, e |
Two more positions were measured, as the dataset door's count over all
five rows:
| position | filter | `engine.aggregate` filter | `engine.find` rows |
native | ObjectQL before | ObjectQL after |
|:--|:--|:--|:--|:--|:--|:--|
| measure `filter` | `{ $not: { owners: { $contains: 'u1' } } }` | 5 | 3
| 3 | 400 | **3** |
| measure `filter` | `{ owners: { $null: false } }` | 4 | 4 | 4 | 400 |
**4** |
| measure `filter` | `{ owners: { $notContains: 'u1' } }` | 5 | 3 | 3 |
400 | **3** |
| measure `filter` | single-valued `$null: false` / `null` / `$not
$contains` | 4 / 1 / 3 | same | same | same | same |
| host read scope | `$not $contains` multi / `$null: false` multi /
`owner $ne null` | | b,d,e / a,b,c,d / a,b,c,d | same | same | same |
A read scope reaches the engine as written, not through `convertFilter`,
so it was served before and is unchanged.
## What changed
-
`packages/services/service-analytics/src/strategies/objectql-strategy.ts`,
`convertFilter`: `set` becomes `{ $null: false }` and `notSet` becomes
`{ $null: true }`. The comment records why.
- `.changeset/20918-objectql-not-multivalue-served.md`:
`@objectstack/service-analytics` `minor`, `Clause-②: yes`.
The `notSet` arm is fixed beside the `set` arm. It is the same defect
class in the same function, and the card's scope ("a `$not` over a
multi-valued column") includes row 2, which only the `notSet` arm
serves.
## Pins, red first (`f45d92d43`)
- `packages/rest/src/analytics-not-multivalue-null-guard.test.ts`: the
shipped composition, both strategies, the cube read and the dataset
door, against `engine.find`. It covers rows 1, 2, 4, 5 and 7, plus the
single-valued control (row 11), and each row's reference rows are
asserted too. Base: 5 red (ObjectQL, the multi-valued rows) and 7 green
(native, and the control). Now 12 of 12 green.
-
`packages/services/service-analytics/src/__tests__/objectql-null-predicate-json-column.test.ts`:
the ObjectQL face over a real `ObjectQL` on `SqliteWasmDriver`, against
`engine.find`. It covers the same five multi-valued rows and three
single-valued controls, plus the engine-input spelling of `set` /
`notSet`. Base: 6 red and 3 green. Now 9 of 9 green.
## Pins re-judged
Ten cases in five existing files recorded the old engine-input spelling,
or read it through a test stand-in engine. Each was respelled, and no
row expectation moved:
| file | cases | disposition |
|:--|--:|:--|
| `filter-normalizer-not-null-safe.test.ts` | 2 | engine input respelled
to `{ $null: false }` / `{ $null: true }` |
| `filter-value-type-fidelity.test.ts` | 1 | respelled; its
strict-equality stand-in engine reads the `$null` predicate |
| `objectql-contains-canonical-operator.test.ts` | 2 | respelled; its
regex-face stand-in reads `{ $null: flag }` where it read the bare
`null` |
| `objectql-daterange.test.ts` | 1 | respelled; its bridge reads `$null`
|
| `where-boolean-flag-refusal.test.ts` | 4 | the control families'
engine-input table respelled; their real-engine rows are unchanged |
## Ablations, predicted before running
The ablations ran at `b6c4893fd`. Between that commit and the head,
`b5a2e4481`, nothing under `packages/services` or `packages/rest`
changed. Each ablation:
- mutated the committed `objectql-strategy.ts` through
`scripts/ablation-replace.mjs` (anchor 1 to 0, blob `63ce11d10a04` to
`75b05cae8989` / `aeab45c348ec` / `3f893a607ea2`);
- rebuilt `@objectstack/service-analytics`, and passed
`ablation-dist-preflight` with the old arm present in `dist/`;
- ran the unit files and the route pin;
- restored, proving the blob equal to HEAD `63ce11d10a04`, `git diff
HEAD` empty and the tree clean, then rebuilt and passed
`ablation-dist-preflight --absent` for each marker.
Every observed count equals its prediction:
| ablation | mutation | unit (the 6 touched files) | route pin (12) |
|:--|:--|:--|:--|
| A0 | both arms back to `{ $ne: null }` / `null` | 16 red. The full
package suite also went 16 red and nothing else (151 files, 3451 passed)
| 5 red (ObjectQL rows 1, 2, 4, 5, 7) |
| A1 | the `set` arm only | 7 red: new pin 3, not-null-safe 2,
boolean-flag 2 (`$null: false`, `$exists: true`) | 2 red (rows 1, 5) |
| A2 | the `notSet` arm only | 13 red: new pin 4, not-null-safe 1,
type-fidelity 1, contains-canonical 2, daterange 1, boolean-flag 4 | 3
red (rows 2, 4, 7) |
## Verification, at `b5a2e4481` (main `2f2fa11d7` merged, no rebase; it
touched nothing on this surface)
- `@objectstack/service-analytics`: `test` exit 0, 151 files, 3467
passed. `typecheck` exit 0, with 151 of 151 test files in the tsc
program (`--listFiles`).
- `@objectstack/rest`: `typecheck` exit 0 (`tsc --noEmit` and
`check:test-typecheck`). Its 16 `analytics-*.test.ts` files: 225 passed
and 3 skipped.
- Consumer sweep, narrowed to the files that load this package:
`@objectstack/runtime` `analytics-*` plus
`cross-field-refusal-operand-withhold`, 5 files, 38 passed and 4
skipped.
- Gates: `dispatch-gates --commands` derived 62. All 62 were run as one
locked sequential script, plus the 4 roster families
(`check-changeset-fixed`, `check:authz-resolver`,
`check:error-code-casing`, `check:filter-alias-parity`), and each exit
code was captured before any pipe.
- 61 exit 0. `check:type-check-debt` first exited 3 (prerequisite:
`@objectstack/runtime` and `@objectstack/driver-turso` unbuilt). It was
re-run after building that closure and exited 0: "none above its
recorded number".
- `check:dual-build-cjs-loads` exited 3 (prerequisite: 38 packages with
no `dist/`, a whole-workspace build). It is NOT MEASURED here.
- `dispatch-gates --ran`: 62 derived, 61 run, 1 NOT-MEASURED, 0 UNRUN.
- The 4 roster families exit 0.
- Lint, narrowed and proved at `b5a2e4481`. The population is the 8
changed `.ts` files, and none is ignored by eslint's own config
(`isPathIgnored` false for all 8). `parserOptions.project` and
`projectService` are unset for all 8, so no type-aware rule runs and no
untouched file's verdict can move. `eslint --no-inline-config --format
json` over them: 8 files, 0 errors, 0 warnings.
## Acceptance notes
- **Rows 8 and 9 differ from the engine.** `{ owners: { $ne: null } }`
and the bare `{ owners: null }` are null tests at the analytics door:
since #5332 the normalizer reads them as `set` / `notSet`. So both
strategies answer them now (the native one already did). `engine.find`
refuses them, because `driver-sql`'s JSON-column gate reads the
operator, not the comparand. Triage ruled out a driver special case for
`$ne: null` on a JSON column, so this difference stays. `{ $null: false
}` / `{ $null: true }` is the spelling every face reads alike.
- **Measure `filter` against the engine's aggregation evaluator.** For
the multi-valued `$not $contains` and `$notContains` measure filters,
the analytics answer (3) equals `engine.find` and the native strategy.
`engine.aggregate`'s per-aggregation `filter` answers 5. That is the
engine's aggregation evaluator, the position of #20873
(`domain:engine`), which triage kept as a neighbour. #20873 is not
addressed here.
- **Surface.** The claim named new pin files. This PR also edits the
five existing pin files above, all under
`service-analytics/src/__tests__/`, because their assertions recorded
the spelling the fix changes. None of them is a file of #20912 or
#20889.
- **The G3 wording.** The PM's assumption read "excludes NULL rows".
Measured, the guard makes the negation ANSWER the NULL row (row e is in
b, d, e), and the engine already does that.
- **NOT MEASURED:**
- PostgreSQL and MySQL;
- driver-memory, driver-mongodb and driver-turso behind the ObjectQL
strategy. They compile `$null` on every path today because the engine's
own lowering emits it, but that is reasoning, not a measurement;
- an external object's datetime column on the legacy-repair path, where
`{ f: null }` and `{ f: { $null: true } }` take different `driver-sql`
arms;
- the dogfood and integration lanes, the full `@objectstack/rest`
`local` suite, the whole-workspace typecheck, and
`check:dual-build-cjs-loads`.
All are left to CI.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01XY5uCwTjZj7884yYtyur4H)_
---------
Co-authored-by: Claude <noreply@anthropic.com>
1 parent d830d71 commit 5dbeb7d
9 files changed
Lines changed: 535 additions & 29 deletions
File tree
- .changeset
- packages
- rest/src
- services/service-analytics/src
- __tests__
- strategies
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
Lines changed: 277 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
| 270 | + | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
Lines changed: 10 additions & 7 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
601 | 601 | | |
602 | 602 | | |
603 | 603 | | |
604 | | - | |
| 604 | + | |
| 605 | + | |
605 | 606 | | |
606 | 607 | | |
607 | 608 | | |
| |||
890 | 891 | | |
891 | 892 | | |
892 | 893 | | |
893 | | - | |
894 | | - | |
895 | | - | |
896 | | - | |
897 | | - | |
| 894 | + | |
| 895 | + | |
| 896 | + | |
| 897 | + | |
| 898 | + | |
| 899 | + | |
| 900 | + | |
898 | 901 | | |
899 | | - | |
| 902 | + | |
900 | 903 | | |
901 | 904 | | |
902 | 905 | | |
| |||
Lines changed: 12 additions & 6 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
605 | 605 | | |
606 | 606 | | |
607 | 607 | | |
608 | | - | |
609 | | - | |
610 | | - | |
| 608 | + | |
| 609 | + | |
| 610 | + | |
| 611 | + | |
| 612 | + | |
| 613 | + | |
| 614 | + | |
| 615 | + | |
611 | 616 | | |
612 | 617 | | |
613 | 618 | | |
| |||
652 | 657 | | |
653 | 658 | | |
654 | 659 | | |
655 | | - | |
656 | | - | |
657 | | - | |
| 660 | + | |
| 661 | + | |
| 662 | + | |
| 663 | + | |
658 | 664 | | |
659 | 665 | | |
660 | 666 | | |
| |||
0 commit comments