From 3e7925186a17aad297dea024abe771afbfb4f66a Mon Sep 17 00:00:00 2001 From: Benjamin Goering <171782+gobengo@users.noreply.github.com> Date: Wed, 8 Mar 2023 16:04:37 -0800 Subject: [PATCH 1/5] fix: DbProvisionsStorage putMany doesnt error on cid col conflict --- packages/access-api/src/models/provisions.js | 8 ++++++++ packages/access-api/test/provisions.test.js | 11 ++++++++--- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/packages/access-api/src/models/provisions.js b/packages/access-api/src/models/provisions.js index f78429c1c..bf2b5f8d8 100644 --- a/packages/access-api/src/models/provisions.js +++ b/packages/access-api/src/models/provisions.js @@ -85,6 +85,14 @@ export class DbProvisions { await this.#db .insertInto(this.tableNames.provisions) .values(rows) + .onConflict((oc) => { + // if cid conflicts, update columns w/ values from conflicting insert + return oc.column('cid').doUpdateSet({ + consumer: (eb) => eb.ref('excluded.consumer'), + provider: (eb) => eb.ref('excluded.provider'), + sponsor: (eb) => eb.ref('excluded.sponsor'), + }) + }) .executeTakeFirstOrThrow() } diff --git a/packages/access-api/test/provisions.test.js b/packages/access-api/test/provisions.test.js index 87f7ba47f..dedc0b5e0 100644 --- a/packages/access-api/test/provisions.test.js +++ b/packages/access-api/test/provisions.test.js @@ -12,7 +12,7 @@ describe('DbProvisions', () => { const storage = new DbProvisions(createD1Database(d1)) const count = Math.round(Math.random() * 10) const spaceA = await principal.ed25519.generate() - const provisions = await Promise.all( + const [firstProvision, ...lastProvisions] = await Promise.all( Array.from({ length: count }).map(async () => { const issuerKey = await principal.ed25519.generate() const issuer = issuerKey.withDID('did:mailto:example.com:foo') @@ -37,8 +37,8 @@ describe('DbProvisions', () => { return provision }) ) - await storage.putMany(...provisions) - assert.deepEqual(await storage.count(), provisions.length) + await storage.putMany(...lastProvisions) + assert.deepEqual(await storage.count(), lastProvisions.length) const spaceHasStorageProvider = await storage.hasStorageProvider( spaceA.did() @@ -52,5 +52,10 @@ describe('DbProvisions', () => { 'can parse provision.cid as CID' ) } + + // ensure no error if we try to store same provision twice + // all of lastProvisions are duplicate, but firstProvision is new so that should be added + await storage.putMany(...lastProvisions, firstProvision) + assert.deepEqual(await storage.count(), count) }) }) From 88cac0d2b51775ed307416fe533444dbbcbc8770 Mon Sep 17 00:00:00 2001 From: Benjamin Goering <171782+gobengo@users.noreply.github.com> Date: Thu, 9 Mar 2023 15:04:31 -0800 Subject: [PATCH 2/5] assert that DbStorageProvisions#putMany with cid already in db, but different derived fields, results in error --- packages/access-api/test/provisions.test.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/packages/access-api/test/provisions.test.js b/packages/access-api/test/provisions.test.js index dedc0b5e0..bc145abe3 100644 --- a/packages/access-api/test/provisions.test.js +++ b/packages/access-api/test/provisions.test.js @@ -57,5 +57,18 @@ describe('DbProvisions', () => { // all of lastProvisions are duplicate, but firstProvision is new so that should be added await storage.putMany(...lastProvisions, firstProvision) assert.deepEqual(await storage.count(), count) + + // but if we try to store the same provision (same `cid`) with different + // fields derived from invocation, it should error + const modifiedFirstProvision = { + ...firstProvision, + space: /** @type {const} */ ('did:key:foo'), + } + const putModifiedFirstProvision = () => + storage.putMany(modifiedFirstProvision) + await assert.rejects( + putModifiedFirstProvision(), + 'cannot putMany with same cid but different derived fields' + ) }) }) From efb99b534e11c4e7905aa0d72de7068e54c02774 Mon Sep 17 00:00:00 2001 From: Benjamin Goering <171782+gobengo@users.noreply.github.com> Date: Thu, 9 Mar 2023 18:51:42 -0800 Subject: [PATCH 3/5] change Provisions#putMany to Provisions#put --- packages/access-api/src/models/provisions.js | 107 +++++++++++++----- .../access-api/src/service/provider-add.js | 2 +- packages/access-api/src/types/provisions.ts | 6 +- packages/access-api/test/provisions.test.js | 27 +++-- 4 files changed, 102 insertions(+), 40 deletions(-) diff --git a/packages/access-api/src/models/provisions.js b/packages/access-api/src/models/provisions.js index bf2b5f8d8..159ad6c1b 100644 --- a/packages/access-api/src/models/provisions.js +++ b/packages/access-api/src/models/provisions.js @@ -14,9 +14,9 @@ export function createProvisions(storage = []) { const hasRowWithSpace = storage.some(({ space }) => space === consumerId) return hasRowWithSpace } - /** @type {Provisions['putMany']} */ - const putMany = async (...items) => { - storage.push(...items) + /** @type {Provisions['put']} */ + const put = async (item) => { + storage.push(item) } /** @type {Provisions['count']} */ const count = async () => { @@ -24,13 +24,13 @@ export function createProvisions(storage = []) { } return { count, - putMany, + put, hasStorageProvider, } } /** - * @typedef ProvsionsRow + * @typedef ProvisionsRow * @property {string} cid * @property {string} consumer * @property {string} provider @@ -38,7 +38,7 @@ export function createProvisions(storage = []) { */ /** - * @typedef {import("../types/database").Database<{ provisions: ProvsionsRow }>} ProvisionsDatabase + * @typedef {import("../types/database").Database<{ provisions: ProvisionsRow }>} ProvisionsDatabase */ /** @@ -68,32 +68,66 @@ export class DbProvisions { return BigInt(size) } - /** @type {Provisions['putMany']} */ - async putMany(...items) { - if (items.length === 0) { - return + /** @type {Provisions['put']} */ + async put(item) { + /** @type {ProvisionsRow} */ + const row = { + cid: item.invocation.cid.toString(), + consumer: item.space, + provider: item.provider, + sponsor: item.account, } - /** @type {ProvsionsRow[]} */ - const rows = items.map((item) => { - return { - cid: item.invocation.cid.toString(), - consumer: item.space, - provider: item.provider, - sponsor: item.account, - } - }) - await this.#db + /** @type {Array<keyof ProvisionsRow>} */ + const rowColumns = ['cid', 'consumer', 'provider', 'sponsor'] + const insert = this.#db .insertInto(this.tableNames.provisions) - .values(rows) - .onConflict((oc) => { - // if cid conflicts, update columns w/ values from conflicting insert - return oc.column('cid').doUpdateSet({ - consumer: (eb) => eb.ref('excluded.consumer'), - provider: (eb) => eb.ref('excluded.provider'), - sponsor: (eb) => eb.ref('excluded.sponsor'), - }) - }) + .values(row) + .returning(rowColumns) + + let primaryKeyError + try { + await insert.executeTakeFirstOrThrow() + } catch (error) { + const isD1 = /D1_ALL_ERROR/.test(String(error)) + if (!isD1) throw error + const cause = + error && typeof error === 'object' && 'cause' in error && error.cause + const code = + cause && + typeof cause === 'object' && + 'code' in cause && + typeof cause.code === 'string' && + cause.code + if (code !== 'SQLITE_CONSTRAINT_PRIMARYKEY') throw error + primaryKeyError = error + } + + if (!primaryKeyError) { + // inserted ok + return + } + + // there was already a row with this invocation cid + // as long as the row we tried to insert is same as one already there, no need to error + const existing = await this.#db + .selectFrom(this.tableNames.provisions) + .select(rowColumns) + .where('cid', '=', row.cid) .executeTakeFirstOrThrow() + if (deepEqual(existing, row)) { + return + } + + // this is a sign of something very wrong. throw so error reporters can report on it + throw Object.assign( + new Error( + `Provision with cid ${item.invocation.cid} already exists with different field values` + ), + { + insertion: row, + existing, + } + ) } /** @type {Provisions['hasStorageProvider']} */ @@ -120,3 +154,18 @@ export class DbProvisions { return rows } } + +/** + * @param {Record<string,any>} x + * @param {Record<string,any>} y + * @returns {boolean} + */ +function deepEqual(x, y) { + const ok = Object.keys + const tx = typeof x + const ty = typeof y + return x && y && tx === 'object' && tx === ty + ? ok(x).length === ok(y).length && + ok(x).every((key) => deepEqual(x[key], y[key])) + : x === y +} diff --git a/packages/access-api/src/service/provider-add.js b/packages/access-api/src/service/provider-add.js index 49d78fadc..99bfa1f42 100644 --- a/packages/access-api/src/service/provider-add.js +++ b/packages/access-api/src/service/provider-add.js @@ -35,7 +35,7 @@ export function createProviderAddHandler(options) { message: 'Issuer must be a mailto DID', } } - await options.provisions.putMany({ + await options.provisions.put({ invocation, space: consumer, provider, diff --git a/packages/access-api/src/types/provisions.ts b/packages/access-api/src/types/provisions.ts index 31081078d..a0ec3d134 100644 --- a/packages/access-api/src/types/provisions.ts +++ b/packages/access-api/src/types/provisions.ts @@ -19,11 +19,11 @@ export interface Provision { export interface ProvisionsStorage { hasStorageProvider: (consumer: Ucanto.DID<'key'>) => Promise<boolean> /** - * write several items into storage + * ensure item is stored * - * @param items - provisions to store + * @param item - provision to store */ - putMany: (...items: Provision[]) => Promise<void> + put: (item: Provision) => Promise<void> /** * get number of stored items diff --git a/packages/access-api/test/provisions.test.js b/packages/access-api/test/provisions.test.js index bc145abe3..0c2e9f538 100644 --- a/packages/access-api/test/provisions.test.js +++ b/packages/access-api/test/provisions.test.js @@ -9,8 +9,9 @@ import { CID } from 'multiformats' describe('DbProvisions', () => { it('should persist provisions', async () => { const { d1 } = await context() - const storage = new DbProvisions(createD1Database(d1)) - const count = Math.round(Math.random() * 10) + const db = createD1Database(d1) + const storage = new DbProvisions(db) + const count = 2 + Math.round(Math.random() * 3) const spaceA = await principal.ed25519.generate() const [firstProvision, ...lastProvisions] = await Promise.all( Array.from({ length: count }).map(async () => { @@ -37,7 +38,7 @@ describe('DbProvisions', () => { return provision }) ) - await storage.putMany(...lastProvisions) + await Promise.all(lastProvisions.map((p) => storage.put(p))) assert.deepEqual(await storage.count(), lastProvisions.length) const spaceHasStorageProvider = await storage.hasStorageProvider( @@ -55,7 +56,8 @@ describe('DbProvisions', () => { // ensure no error if we try to store same provision twice // all of lastProvisions are duplicate, but firstProvision is new so that should be added - await storage.putMany(...lastProvisions, firstProvision) + await storage.put(lastProvisions[0]) + await storage.put(firstProvision) assert.deepEqual(await storage.count(), count) // but if we try to store the same provision (same `cid`) with different @@ -63,12 +65,23 @@ describe('DbProvisions', () => { const modifiedFirstProvision = { ...firstProvision, space: /** @type {const} */ ('did:key:foo'), + account: /** @type {const} */ ('did:mailto:foo'), + // note this type assertion is wrong, but useful to set up the test + provider: + /** @type {import('../src/types/provisions.js').AlphaStorageProvider} */ ( + 'did:provider:foo' + ), } - const putModifiedFirstProvision = () => - storage.putMany(modifiedFirstProvision) + const putModifiedFirstProvision = () => storage.put(modifiedFirstProvision) await assert.rejects( putModifiedFirstProvision(), - 'cannot putMany with same cid but different derived fields' + 'cannot put with same cid but different derived fields' ) + const provisionForFakeConsumer = await storage.findForConsumer( + modifiedFirstProvision.space + ) + assert.deepEqual(provisionForFakeConsumer.length, 0) + // count umodified + assert.deepEqual(await storage.count(), count) }) }) From 1cab7b8a04f3bd8c4345b9d3e30fe7aee6f9372a Mon Sep 17 00:00:00 2001 From: Benjamin Goering <171782+gobengo@users.noreply.github.com> Date: Thu, 9 Mar 2023 18:59:14 -0800 Subject: [PATCH 4/5] typo fix --- packages/access-api/test/provisions.test.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/access-api/test/provisions.test.js b/packages/access-api/test/provisions.test.js index 0c2e9f538..4e67ec851 100644 --- a/packages/access-api/test/provisions.test.js +++ b/packages/access-api/test/provisions.test.js @@ -81,7 +81,10 @@ describe('DbProvisions', () => { modifiedFirstProvision.space ) assert.deepEqual(provisionForFakeConsumer.length, 0) - // count umodified - assert.deepEqual(await storage.count(), count) + assert.deepEqual( + await storage.count(), + count, + 'count was not increased by put w/ existing cid' + ) }) }) From c3baa4d8608ab617688df44334d39b33948b4823 Mon Sep 17 00:00:00 2001 From: Benjamin Goering <171782+gobengo@users.noreply.github.com> Date: Fri, 10 Mar 2023 12:32:38 -0800 Subject: [PATCH 5/5] clean up provisions#put error checking --- packages/access-api/src/models/provisions.js | 47 ++++++++++++++------ 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/packages/access-api/src/models/provisions.js b/packages/access-api/src/models/provisions.js index 159ad6c1b..7d3da0408 100644 --- a/packages/access-api/src/models/provisions.js +++ b/packages/access-api/src/models/provisions.js @@ -88,37 +88,39 @@ export class DbProvisions { try { await insert.executeTakeFirstOrThrow() } catch (error) { - const isD1 = /D1_ALL_ERROR/.test(String(error)) - if (!isD1) throw error - const cause = - error && typeof error === 'object' && 'cause' in error && error.cause - const code = - cause && - typeof cause === 'object' && - 'code' in cause && - typeof cause.code === 'string' && - cause.code - if (code !== 'SQLITE_CONSTRAINT_PRIMARYKEY') throw error - primaryKeyError = error + const d1Error = extractD1Error(error) + switch (d1Error?.code) { + case 'SQLITE_CONSTRAINT_PRIMARYKEY': { + primaryKeyError = error + break + } + default: { + throw error + } + } } if (!primaryKeyError) { - // inserted ok + // no error inserting, we're done with put return } // there was already a row with this invocation cid - // as long as the row we tried to insert is same as one already there, no need to error + // as long as the row we tried to insert is same as one already there, no need to error. + // so let's compare the existing row with that cid to the row we tried to insert. const existing = await this.#db .selectFrom(this.tableNames.provisions) .select(rowColumns) .where('cid', '=', row.cid) .executeTakeFirstOrThrow() if (deepEqual(existing, row)) { + // the insert failed, but the existing row is identical to the row that failed to insert. + // so the put is a no-op, and we can consider it a success despite encountering the primaryKeyError return } // this is a sign of something very wrong. throw so error reporters can report on it + // and determine what led to a put() with same invocation cid but new non-cid column values throw Object.assign( new Error( `Provision with cid ${item.invocation.cid} already exists with different field values` @@ -169,3 +171,20 @@ function deepEqual(x, y) { ok(x).every((key) => deepEqual(x[key], y[key])) : x === y } + +/** + * @param {unknown} error + */ +function extractD1Error(error) { + const isD1 = /D1_ALL_ERROR/.test(String(error)) + if (!isD1) return + const cause = + error && typeof error === 'object' && 'cause' in error && error.cause + const code = + cause && + typeof cause === 'object' && + 'code' in cause && + typeof cause.code === 'string' && + cause.code + return { cause, code } +}