|
| 1 | +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. |
| 2 | +// |
| 3 | +// Real-engine regression for #7705 — `protocol.deletePackage` found ZERO |
| 4 | +// `sys_metadata` rows the data plane found three of, and uninstall left them |
| 5 | +// orphaned (the persistence half of #7557; PR #7700 shipped the envelope half |
| 6 | +// and deliberately did not patch this from the consumer side). |
| 7 | + |
| 8 | +import { describe, it, expect, afterEach } from 'vitest'; |
| 9 | +import { mkdtempSync, rmSync } from 'node:fs'; |
| 10 | +import { tmpdir } from 'node:os'; |
| 11 | +import { join } from 'node:path'; |
| 12 | +import { ObjectQL } from '@objectstack/objectql'; |
| 13 | +import { SqlDriver } from '@objectstack/driver-sql'; |
| 14 | +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; |
| 15 | +import { |
| 16 | + SysMetadataObject, |
| 17 | + SysMetadataHistoryObject, |
| 18 | + SysMetadataAuditObject, |
| 19 | +} from '@objectstack/metadata-core'; |
| 20 | + |
| 21 | +/** |
| 22 | + * The mechanism, MEASURED (not the one the card guessed at first). |
| 23 | + * |
| 24 | + * `deletePackage` selected its rows with a strict `organization_id` equality: |
| 25 | + * |
| 26 | + * const where = { package_id: request.packageId }; |
| 27 | + * if (request.organizationId) where.organization_id = request.organizationId; |
| 28 | + * |
| 29 | + * Two candidates were live when this was dispatched, and the second is |
| 30 | + * FALSIFIED by measurement, so it is recorded here rather than left implied: |
| 31 | + * |
| 32 | + * (a) the caller supplies an `organizationId` and strict equality drops rows |
| 33 | + * stored env-wide (`organization_id IS NULL`); |
| 34 | + * (b) the protocol's `this.engine` is scoped differently from the data |
| 35 | + * plane's — an org-injecting wrapper, a separate registration for |
| 36 | + * `sys_metadata`, or a visibility rule — so an IDENTICAL `where` returns |
| 37 | + * different rows on the two seams. |
| 38 | + * |
| 39 | + * (b) is false. `findData` — the `GET /api/v1/data/sys_metadata` path that |
| 40 | + * returned three rows — issues `this.engine.find(object, options)` on the very |
| 41 | + * same engine instance this method uses, and the engine injects no org |
| 42 | + * predicate of its own: a bare `new ObjectQL()` carries zero middlewares, and |
| 43 | + * the driver receives the author-supplied `where` verbatim. So (a) is the |
| 44 | + * mechanism, and it is what these tests pin. |
| 45 | + * |
| 46 | + * Why the miss is the COMMON case rather than a corner: env-wide is where a |
| 47 | + * package's metadata normally lands (the REST `PUT /meta/:type/:name` save |
| 48 | + * path does not thread the session's active org, and AI-authored metadata is |
| 49 | + * written env-wide too), while the door that resolves an org and passes it — |
| 50 | + * the dispatcher twin at `packages/runtime/src/domains/packages.ts`, the door |
| 51 | + * whose `persisted:` envelope the issue quotes — is the one users hit with an |
| 52 | + * active session. So the uninstall selected only whichever rows happened to be |
| 53 | + * org-scoped and reported `success: true` over the survivors. |
| 54 | + * |
| 55 | + * The remedy is the shape this codebase already uses for exactly this defect |
| 56 | + * class: `$or [{organization_id: oid}, {organization_id: null}]`, from the |
| 57 | + * #3115 "orphaned draft" fix in `SysMetadataRepository.listDrafts` |
| 58 | + * (`packages/metadata-protocol/src/sys-metadata-repository.ts`). The SQL |
| 59 | + * driver's own implicit tenant wall already reads this way too (`field = |
| 60 | + * :tenant OR field IS NULL`, #2734) — only author-supplied predicates are |
| 61 | + * strict, which is what made this silent. |
| 62 | + * |
| 63 | + * --------------------------------------------------------------------------- |
| 64 | + * Why this suite uses the REAL engine and the REAL driver |
| 65 | + * --------------------------------------------------------------------------- |
| 66 | + * `{success: true, deletedCount: 0}` against a package that has no rows is |
| 67 | + * indistinguishable from this bug, so an assertion on the CALL — "was |
| 68 | + * `deleteMetaItem` invoked with these arguments" — proves nothing here. Both |
| 69 | + * existing `deletePackage` suites are call-shaped for that reason and neither |
| 70 | + * could have caught this: they stub `engine.find` to hand back the rows the |
| 71 | + * test wants and mock `deleteMetaItem` so nothing is ever deleted. This suite |
| 72 | + * therefore SEEDS rows through the real `saveMetaItem` write path, runs the |
| 73 | + * real uninstall, and asserts on WHICH ROWS SURVIVE in SQLite afterwards. |
| 74 | + * A hand-built double is specifically what cannot answer this: the whole |
| 75 | + * question is whether `organization_id = 'org'` matches a NULL column, which |
| 76 | + * is a property of the driver's SQL, not of a stub's `filter()`. |
| 77 | + * |
| 78 | + * --------------------------------------------------------------------------- |
| 79 | + * Reverse verification, direction predicted BEFORE the revert was run |
| 80 | + * --------------------------------------------------------------------------- |
| 81 | + * Restoring `where.organization_id = request.organizationId` in place of the |
| 82 | + * `$or` was predicted to turn ONLY the org-scoped-uninstall case red — |
| 83 | + * `deletedCount` 4 → 1 with the three env-wide rows surviving — and to leave |
| 84 | + * every negative case green, because strict equality is NARROWER than the |
| 85 | + * `$or`: it cannot reach another org's rows or another package's rows, and it |
| 86 | + * does not touch the no-org branch at all. Measured on revert: exactly that. |
| 87 | + * `deletedCount` came back 1, `reprob_a` / `reprob_b` / `reprob_v` survived, |
| 88 | + * and the three negative cases stayed green. The negatives are the control |
| 89 | + * that keeps a future "fix" from over-widening the predicate — deleting rows |
| 90 | + * that should have stayed is worse than the orphaning this closes. |
| 91 | + */ |
| 92 | + |
| 93 | +const PKG = 'com.repro.b'; |
| 94 | +const OTHER_PKG = 'com.other'; |
| 95 | +const PLATFORM_PKG = '@objectstack/platform-objects'; |
| 96 | +const ACTIVE_ORG = 'org_active'; |
| 97 | +const OTHER_ORG = 'org_other'; |
| 98 | + |
| 99 | +let cleanup: Array<() => void> = []; |
| 100 | +afterEach(() => { |
| 101 | + for (const c of cleanup) c(); |
| 102 | + cleanup = []; |
| 103 | +}); |
| 104 | + |
| 105 | +/** REAL ObjectQL wired to a REAL SqlDriver over on-disk better-sqlite3. */ |
| 106 | +async function boot() { |
| 107 | + const dir = mkdtempSync(join(tmpdir(), 'os-7705-')); |
| 108 | + cleanup.push(() => rmSync(dir, { recursive: true, force: true })); |
| 109 | + |
| 110 | + const driver = new SqlDriver({ |
| 111 | + client: 'better-sqlite3', |
| 112 | + connection: { filename: join(dir, 'data.sqlite') }, |
| 113 | + useNullAsDefault: true, |
| 114 | + }); |
| 115 | + // `sys_metadata` is the table under test; the history/audit tables are the |
| 116 | + // ones `saveMetaItem` and `deleteMetaItem` write through on the real path. |
| 117 | + const objects = [SysMetadataObject, SysMetadataHistoryObject, SysMetadataAuditObject] as any[]; |
| 118 | + await driver.initObjects(objects); |
| 119 | + |
| 120 | + const engine = new ObjectQL(); |
| 121 | + engine.registerDriver(driver as any, true); |
| 122 | + await engine.init(); |
| 123 | + // Registered under the PLATFORM package, which must not be the package under |
| 124 | + // test: `deletePackage` also unregisters its package from the live registry, |
| 125 | + // and owning `sys_metadata` from `PKG` would tear the table out from under |
| 126 | + // the post-uninstall assertions that read the surviving rows back. |
| 127 | + for (const o of objects) engine.registry.registerObject(o, PLATFORM_PKG); |
| 128 | + cleanup.push(() => { void engine.destroy(); }); |
| 129 | + |
| 130 | + // `'package-author'` is the genuine control-plane assembly's channel — the |
| 131 | + // #4463 runtime authoring gate is for environment-channel writes and would |
| 132 | + // otherwise refuse the seeding saves below. |
| 133 | + const protocol = new ObjectStackProtocolImplementation(engine as any, undefined, undefined, 'package-author'); |
| 134 | + return { engine, protocol }; |
| 135 | +} |
| 136 | + |
| 137 | +const viewBody = (name: string) => ({ |
| 138 | + name, |
| 139 | + label: name, |
| 140 | + type: 'grid', |
| 141 | + data: { provider: 'object', object: 'anything' }, |
| 142 | + columns: ['id'], |
| 143 | +}); |
| 144 | + |
| 145 | +/** |
| 146 | + * Seed through the REAL write path so every row carries the checksum and |
| 147 | + * history the real uninstall reads back. Views (not objects) so the assertions |
| 148 | + * stay on row survival rather than on physical-table teardown, which |
| 149 | + * `deleteMetaItem` handles separately and which #7705 is not about. |
| 150 | + */ |
| 151 | +async function seed(protocol: any) { |
| 152 | + const save = (name: string, packageId: string, organizationId?: string) => |
| 153 | + protocol.saveMetaItem({ |
| 154 | + type: 'view', |
| 155 | + name, |
| 156 | + item: viewBody(name), |
| 157 | + packageId, |
| 158 | + ...(organizationId ? { organizationId } : {}), |
| 159 | + }); |
| 160 | + |
| 161 | + // The suspected — and confirmed — miss: env-wide rows, `organization_id IS NULL`. |
| 162 | + await save('reprob_a', PKG); |
| 163 | + await save('reprob_b', PKG); |
| 164 | + await save('reprob_v', PKG); |
| 165 | + // Same package, the caller's OWN org: the only rows the strict equality found. |
| 166 | + await save('reprob_own', PKG, ACTIVE_ORG); |
| 167 | + // Negative 1 — same package, ANOTHER org. Must survive an org-scoped uninstall. |
| 168 | + await save('reprob_foreign', PKG, OTHER_ORG); |
| 169 | + // Negative 2 — ANOTHER package, env-wide. Must survive either way. |
| 170 | + await save('other_a', OTHER_PKG); |
| 171 | +} |
| 172 | + |
| 173 | +/** Every surviving row, as `name[pkg,org]`, read straight back out of SQLite. */ |
| 174 | +async function survivors(engine: any): Promise<string[]> { |
| 175 | + const rows = (await engine.find('sys_metadata', { where: {} })) as any[]; |
| 176 | + return rows.map((r) => `${r.name}[${r.package_id},${r.organization_id ?? 'ENV'}]`).sort(); |
| 177 | +} |
| 178 | + |
| 179 | +const namesFor = async (engine: any, packageId: string): Promise<string[]> => { |
| 180 | + const rows = (await engine.find('sys_metadata', { where: { package_id: packageId } })) as any[]; |
| 181 | + return rows.map((r) => r.name).sort(); |
| 182 | +}; |
| 183 | + |
| 184 | +describe('#7705 — org-scoped uninstall must not orphan env-wide sys_metadata rows', () => { |
| 185 | + it('removes the env-wide rows too, and counts them (was: found 1 of 4, left 3 orphaned)', async () => { |
| 186 | + const { engine, protocol } = await boot(); |
| 187 | + await seed(protocol); |
| 188 | + |
| 189 | + // Precondition: the rows the uninstall is supposed to remove really exist, |
| 190 | + // so a passing assertion below cannot be the vacuous "nothing was there". |
| 191 | + expect(await namesFor(engine, PKG)).toEqual( |
| 192 | + ['reprob_a', 'reprob_b', 'reprob_foreign', 'reprob_own', 'reprob_v'], |
| 193 | + ); |
| 194 | + |
| 195 | + const res: any = await (protocol as any).deletePackage({ |
| 196 | + packageId: PKG, |
| 197 | + organizationId: ACTIVE_ORG, |
| 198 | + }); |
| 199 | + |
| 200 | + // The CONSEQUENCE: no row of this package survives in the caller's scope |
| 201 | + // (its own org + env-wide). Before the fix `reprob_a`, `reprob_b` and |
| 202 | + // `reprob_v` were all still here. |
| 203 | + expect(await namesFor(engine, PKG)).toEqual(['reprob_foreign']); |
| 204 | + |
| 205 | + // …and the receipt matches what was actually seeded in that scope — 3 |
| 206 | + // env-wide + 1 own-org. It reported 1 before, while claiming success. |
| 207 | + expect(res.deletedCount).toBe(4); |
| 208 | + expect(res.failedCount).toBe(0); |
| 209 | + expect(res.success).toBe(true); |
| 210 | + expect(res.deleted.map((d: any) => d.name).sort()).toEqual( |
| 211 | + ['reprob_a', 'reprob_b', 'reprob_own', 'reprob_v'], |
| 212 | + ); |
| 213 | + |
| 214 | + // The complete post-state, so nothing else moved either way. |
| 215 | + expect(await survivors(engine)).toEqual([ |
| 216 | + `other_a[${OTHER_PKG},ENV]`, |
| 217 | + `reprob_foreign[${PKG},${OTHER_ORG}]`, |
| 218 | + ]); |
| 219 | + }); |
| 220 | + |
| 221 | + it('does NOT sweep up another organization’s rows', async () => { |
| 222 | + const { engine, protocol } = await boot(); |
| 223 | + await seed(protocol); |
| 224 | + |
| 225 | + await (protocol as any).deletePackage({ packageId: PKG, organizationId: ACTIVE_ORG }); |
| 226 | + |
| 227 | + // `reprob_foreign` belongs to a different tenant and was never in scope. |
| 228 | + // Over-widening the predicate to catch the env-wide rows would delete data |
| 229 | + // that should have stayed — worse than the bug being closed here. |
| 230 | + const rows = (await engine.find('sys_metadata', { |
| 231 | + where: { package_id: PKG, organization_id: OTHER_ORG }, |
| 232 | + })) as any[]; |
| 233 | + expect(rows.map((r: any) => r.name)).toEqual(['reprob_foreign']); |
| 234 | + }); |
| 235 | + |
| 236 | + it('does NOT sweep up another package’s rows', async () => { |
| 237 | + const { engine, protocol } = await boot(); |
| 238 | + await seed(protocol); |
| 239 | + |
| 240 | + await (protocol as any).deletePackage({ packageId: PKG, organizationId: ACTIVE_ORG }); |
| 241 | + |
| 242 | + expect(await namesFor(engine, OTHER_PKG)).toEqual(['other_a']); |
| 243 | + }); |
| 244 | + |
| 245 | + it('an uninstall with NO org still clears the whole package (the other door)', async () => { |
| 246 | + const { engine, protocol } = await boot(); |
| 247 | + await seed(protocol); |
| 248 | + |
| 249 | + // The direct-mount REST registrar (`packages/rest/src/package-routes.ts`) |
| 250 | + // calls `deletePackage({ packageId })` with no org at all. Narrowing THAT |
| 251 | + // branch to `organization_id IS NULL` — the other half of the #3115 shape |
| 252 | + // — would orphan every org-scoped row instead, i.e. re-create this bug on |
| 253 | + // the other door. This case pins that the no-org branch stays package-wide. |
| 254 | + const res: any = await (protocol as any).deletePackage({ packageId: PKG }); |
| 255 | + |
| 256 | + expect(await namesFor(engine, PKG)).toEqual([]); |
| 257 | + expect(res.deletedCount).toBe(5); |
| 258 | + expect(await namesFor(engine, OTHER_PKG)).toEqual(['other_a']); |
| 259 | + }); |
| 260 | +}); |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?v=4&size=40){kind=avatar}
0 commit comments