fix: DbProvisionsStorage putMany doesnt error on cid col conflict (#517)

Motivation:
* #482

* [x] first draft

After 2023-03-09 review with @Gozala :
* [x] add test for updating Provision with same invocation cid but
different derived
fields. it should result in error
* [x] make test pass, probably by changing the kysely to be 'on
conflict, do nothing as long as the derived columns are identical,
otherwise error'
* it wasn't really possible to do that on conflict logic as far as I
could tell, at least through our `kysely` orm
* kysely doesn't expose methods for all the [`on conflict` clauses that
sqlite should support](https://www.sqlite.org/lang_conflict.html)
* I pursued something like [this using
CTE](https://stackoverflow.com/a/49601238), only to find out [sqlite
doesn't support `insert` in
cte](https://sqlite.org/forum/forumpost/b44ca81f043bed9f)

packages/access-api/src/models/provisions.js

@@ -14,31 +14,31 @@ 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 () => {
     return BigInt(storage.length)
   }
   return {
     count,
-    putMany,
+    put,
     hasStorageProvider,
   }
 }
 
 /**
- * @typedef ProvsionsRow
+ * @typedef ProvisionsRow
  * @property {string} cid
  * @property {string} consumer
  * @property {string} provider
  * @property {string} sponsor - did of actor who authorized for this provision
  */
 
 /**
- * @typedef {import("../types/database").Database<{ provisions: ProvsionsRow }>} ProvisionsDatabase
+ * @typedef {import("../types/database").Database<{ provisions: ProvisionsRow }>} ProvisionsDatabase
  */
 
 /**
@@ -68,24 +68,68 @@ 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)
+      .values(row)
+      .returning(rowColumns)
+
+    let primaryKeyError
+    try {
+      await insert.executeTakeFirstOrThrow()
+    } catch (error) {
+      const d1Error = extractD1Error(error)
+      switch (d1Error?.code) {
+        case 'SQLITE_CONSTRAINT_PRIMARYKEY': {
+          primaryKeyError = error
+          break
+        }
+        default: {
+          throw error
+        }
+      }
+    }
+
+    if (!primaryKeyError) {
+      // 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.
+    // 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`
+      ),
+      {
+        insertion: row,
+        existing,
+      }
+    )
   }
 
   /** @type {Provisions['hasStorageProvider']} */
@@ -112,3 +156,35 @@ 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
+}
+
+/**
+ * @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 }
+}

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,

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

packages/access-api/test/provisions.test.js

@@ -9,10 +9,11 @@ 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 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 +38,8 @@ describe('DbProvisions', () => {
         return provision
       })
     )
-    await storage.putMany(...provisions)
-    assert.deepEqual(await storage.count(), provisions.length)
+    await Promise.all(lastProvisions.map((p) => storage.put(p)))
+    assert.deepEqual(await storage.count(), lastProvisions.length)
 
     const spaceHasStorageProvider = await storage.hasStorageProvider(
       spaceA.did()
@@ -52,5 +53,38 @@ 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.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
+    // fields derived from invocation, it should error
+    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.put(modifiedFirstProvision)
+    await assert.rejects(
+      putModifiedFirstProvision(),
+      'cannot put with same cid but different derived fields'
+    )
+    const provisionForFakeConsumer = await storage.findForConsumer(
+      modifiedFirstProvision.space
+    )
+    assert.deepEqual(provisionForFakeConsumer.length, 0)
+    assert.deepEqual(
+      await storage.count(),
+      count,
+      'count was not increased by put w/ existing cid'
+    )
   })
 })