Refactor notifications query: cut CRAP, share test double, add membership property

Reduce the notifications-query collector cyclomatic complexity below the CRAP
threshold by extracting prefix-range, txid-fallback, height-selection, and
actor-suppression helpers, and by sharing one per-post index scan between the
like and reply collectors. Reuse the shared FakeDb in the unit and property
suites instead of two local LevelDB doubles, cover every not-found branch, and
add a property test asserting returned notifications map to exactly one
generated interaction.

By refactorer.

psf-memo-db/src/adapters/notifications-query.js

@@ -33,12 +33,48 @@ function padHeight (blockHeight) {
   return String(blockHeight ?? 0).padStart(HEIGHT_PAD, '0')
 }
 
+// The half-open key range covering one prefix's records at or above cutoff:
+// `${prefix}<paddedCutoff>` .. `${prefix}\uffff`. A null cutoff starts at the
+// prefix itself, so the whole history is in range.
+function prefixRange (prefix, cutoff) {
+  const start = cutoff === null ? prefix : `${prefix}${padHeight(cutoff)}`
+  return { start, end: `${prefix}\uffff` }
+}
+
+// The txid carried by a per-post index entry, falling back to the final
+// colon-delimited key segment when the value omits it.
+function txidFromKey (key) {
+  return String(key).slice(String(key).lastIndexOf(':') + 1)
+}
+
+// The first value that is neither null nor undefined, else 0. Used to pick the
+// most specific block height across a record, its parent, and the post.
+function firstDefined (...values) {
+  for (const value of values) {
+    if (value !== null && value !== undefined) return value
+  }
+  return 0
+}
+
+// An actor never notifies themselves, and muted actors never notify.
+function isSuppressedActor (actorAddr, addr, mutedAddrs) {
+  return actorAddr === addr || mutedAddrs.has(actorAddr)
+}
+
+function isNotFoundError (err) {
+  return Boolean(
+    err?.notFound ||
+    err?.code === 'LEVEL_NOT_FOUND' ||
+    err?.response?.status === 404
+  )
+}
+
 // Read a record by key, returning null when it does not exist.
 async function getRecordOrNull (db, key) {
   try {
     return await db.get(key)
   } catch (err) {
-    if (err.notFound || err.code === 'LEVEL_NOT_FOUND' || err.response?.status === 404) return null
+    if (isNotFoundError(err)) return null
     throw err
   }
 }
@@ -91,7 +127,9 @@ class NotificationsQuery {
     this.listNotifications = this.listNotifications.bind(this)
     this._windowCutoff = this._windowCutoff.bind(this)
     this._scanViewerPosts = this._scanViewerPosts.bind(this)
+    this._scanPostIndex = this._scanPostIndex.bind(this)
     this._collectFollowNotifications = this._collectFollowNotifications.bind(this)
+    this._followNotifications = this._followNotifications.bind(this)
     this._collectLikeNotifications = this._collectLikeNotifications.bind(this)
     this._collectReplyNotifications = this._collectReplyNotifications.bind(this)
     this._followerFromKey = this._followerFromKey.bind(this)
@@ -111,40 +149,58 @@ class NotificationsQuery {
   // Prefix-scan the viewer's posts within the window from addrPostHeights,
   // newest-first is not required because likes/replies are scanned per txid.
   async _scanViewerPosts (addr, cutoff) {
-    const prefix = `${addr}:`
+    const { start, end } = prefixRange(`${addr}:`, cutoff)
-    const start = cutoff === null ? prefix : `${addr}:${padHeight(cutoff)}`
-    const end = `${addr}:\uffff`
     const posts = []
 
     for await (const [key, value] of this.addrPostHeightsDb.iterator({ gte: start, lte: end })) {
-      const txid = value?.txid || key.slice(key.lastIndexOf(':') + 1)
+      const txid = value?.txid || txidFromKey(key)
       if (!txid) continue
-      posts.push({ txid, blockHeight: value?.blockHeight ?? 0 })
+      posts.push({ txid, blockHeight: value?.blockHeight })
     }
 
     return posts
   }
 
+  // Walk a per-post index keyed `${postTxid}:${childKey}` for the viewer's
+  // posts only, resolving each entry's txid. The index is never scanned
+  // globally, so this bounds likes and replies to the viewer's content.
+  async _scanPostIndex (db, posts, txidField) {
+    const entries = []
+
+    for (const post of posts) {
+      const prefix = `${post.txid}:`
+      for await (const [key, value] of db.iterator({ gte: prefix, lte: `${prefix}\uffff` })) {
+        const txid = value?.[txidField] || txidFromKey(key)
+        if (txid) entries.push({ txid, value, post })
+      }
+    }
+
+    return entries
+  }
+
   // Collect the viewer's follows from the followeeHeights index within the
   // window, keeping the newest entry per follower and ignoring unfollows.
   async _collectFollowNotifications (addr, cutoff, mutedAddrs) {
     const myHash160 = this.bchjs.Address.toHash160(addr)
-    const start = cutoff === null ? `${myHash160}:` : `${myHash160}:${padHeight(cutoff)}`
+    const { start, end } = prefixRange(`${myHash160}:`, cutoff)
-    const end = `${myHash160}:\uffff`
     const newestByFollower = new Map()
 
     for await (const [key, record] of this.followeeHeightsDb.iterator({ gte: start, lte: end })) {
       const followerAddr = record?.followerAddr || this._followerFromKey(key)
-      if (!followerAddr) continue
+      if (followerAddr) newestByFollower.set(followerAddr, record)
-      // Ascending height order means the last entry for a follower wins.
-      newestByFollower.set(followerAddr, record)
     }
 
+    return this._followNotifications(newestByFollower, addr, mutedAddrs)
+  }
+
+  // Turn the newest follow entry per follower into follow notifications,
+  // dropping unfollows, self-follows, and muted followers.
+  _followNotifications (newestByFollower, addr, mutedAddrs) {
     const notifications = []
+
     for (const [followerAddr, record] of newestByFollower) {
       if (record.unfollow === true) continue
-      if (followerAddr === addr) continue
+      if (isSuppressedActor(followerAddr, addr, mutedAddrs)) continue
-      if (mutedAddrs.has(followerAddr)) continue
 
       notifications.push({
         type: 'follow',
@@ -161,28 +217,22 @@ class NotificationsQuery {
   // Prefix-scan postLikes for each viewer post and load the like record to get
   // the actor and height. The global likes store is never iterated.
   async _collectLikeNotifications (posts, addr, mutedAddrs) {
+    const entries = await this._scanPostIndex(this.postLikesDb, posts, 'txid')
     const notifications = []
 
-    for (const post of posts) {
+    for (const { txid, post } of entries) {
-      const prefix = `${post.txid}:`
+      const like = await getRecordOrNull(this.likesDb, txid)
-      for await (const [key, value] of this.postLikesDb.iterator({ gte: prefix, lte: `${post.txid}:\uffff` })) {
+      if (!like) continue
-        const likeTxid = value?.txid || key.slice(key.lastIndexOf(':') + 1)
+      if (isSuppressedActor(like.addr, addr, mutedAddrs)) continue
-        if (!likeTxid) continue
 
-        const like = await getRecordOrNull(this.likesDb, likeTxid)
+      notifications.push({
-        if (!like) continue
+        type: 'like',
-        if (like.addr === addr) continue
+        txid,
-        if (mutedAddrs.has(like.addr)) continue
+        addr: like.addr,
-
+        postTxid: post.txid,
-        notifications.push({
+        blockHeight: firstDefined(like.blockHeight, post.blockHeight),
-          type: 'like',
+        seen: like.seen ?? 0
-          txid: likeTxid,
+      })
-          addr: like.addr,
-          postTxid: post.txid,
-          blockHeight: like.blockHeight ?? post.blockHeight ?? 0,
-          seen: like.seen ?? 0
-        })
-      }
     }
 
     return notifications
@@ -192,30 +242,25 @@ class NotificationsQuery {
   // the actor and text. postChildren is only read per viewer post, never
   // full-scanned.
   async _collectReplyNotifications (posts, addr, mutedAddrs) {
+    const entries = await this._scanPostIndex(this.postChildrenDb, posts, 'childTxid')
     const notifications = []
 
-    for (const post of posts) {
+    for (const { txid: childTxid, value: child, post } of entries) {
-      const prefix = `${post.txid}:`
+      if (child?.parentTxid && child.parentTxid !== post.txid) continue
-      for await (const [key, child] of this.postChildrenDb.iterator({ gte: prefix, lte: `${post.txid}:\uffff` })) {
-        const childTxid = child?.childTxid || key.slice(key.lastIndexOf(':') + 1)
-        if (!childTxid) continue
-        if (child?.parentTxid && child.parentTxid !== post.txid) continue
 
-        const childPost = await getPostOrNull(this.postsDb, childTxid)
+      const childPost = await getPostOrNull(this.postsDb, childTxid)
-        if (!childPost) continue
+      if (!childPost) continue
-        if (childPost.addr === addr) continue
+      if (isSuppressedActor(childPost.addr, addr, mutedAddrs)) continue
-        if (mutedAddrs.has(childPost.addr)) continue
 
-        notifications.push({
+      notifications.push({
-          type: 'reply',
+        type: 'reply',
-          txid: childTxid,
+        txid: childTxid,
-          addr: childPost.addr,
+        addr: childPost.addr,
-          postTxid: post.txid,
+        postTxid: post.txid,
-          text: childPost.text,
+        text: childPost.text,
-          blockHeight: child?.blockHeight ?? childPost.blockHeight ?? post.blockHeight ?? 0,
+        blockHeight: firstDefined(child?.blockHeight, childPost.blockHeight, post.blockHeight),
-          seen: childPost.seen ?? 0
+        seen: childPost.seen ?? 0
-        })
+      })
-      }
     }
 
     return notifications

psf-memo-db/test/property/notifications-query.property.test.js

@@ -20,6 +20,7 @@
 import test from 'node:test'
 import { seededRandom, forAll, intGen } from './harness.js'
 import NotificationsQuery from '../../src/adapters/notifications-query.js'
+import { FakeDb } from '../support/level-double.js'
 
 const rng = seededRandom(20260918)
 
@@ -31,28 +32,7 @@ const VIEWER_POSTS = ['vp1', 'vp2', 'vp3', 'vp4']
 const PAD = 12
 const pad = (h) => String(h).padStart(PAD, '0')
 
-function makeDb (entries = []) {
+const makeDb = (entries = []) => new FakeDb(entries)
-  const map = new Map(entries)
-  return {
-    map,
-    async get (key) {
-      if (!map.has(key)) {
-        const err = new Error('not found')
-        err.notFound = true
-        throw err
-      }
-      return map.get(key)
-    },
-    iterator (opts = {}) {
-      let keys = [...map.keys()].sort()
-      if (opts.gte !== undefined) keys = keys.filter((key) => key >= opts.gte)
-      if (opts.lte !== undefined) keys = keys.filter((key) => key <= opts.lte)
-      return (async function * () {
-        for (const key of keys) yield [key, map.get(key)]
-      }())
-    }
-  }
-}
 
 function buildQuery () {
   // statusDb null => no window; every generated interaction is in scope.
@@ -97,10 +77,11 @@ function fixtureGen () {
       const follower = OTHERS[Math.floor(rng() * OTHERS.length)]
       const n = randomNotification()
       const unfollow = rng() < 0.25
-      events.push({ follower, ...n, unfollow })
+      const followTxid = `follow${i}`
+      events.push({ follower, txid: followTxid, ...n, unfollow })
       followeeHeightEntries.push([
         `${MY_HASH}:${pad(n.blockHeight)}:${follower}`,
-        { followerAddr: follower, followeePkHash: MY_HASH, unfollow, txid: `follow${i}`, seen: n.seen, blockHeight: n.blockHeight }
+        { followerAddr: follower, followeePkHash: MY_HASH, unfollow, txid: followTxid, seen: n.seen, blockHeight: n.blockHeight }
       ])
     }
 
@@ -112,7 +93,7 @@ function fixtureGen () {
       const actor = OTHERS[Math.floor(rng() * OTHERS.length)]
       const n = randomNotification()
       const likeTxid = `like${i}`
-      likeRecords.push({ likeTxid, addr: actor, ...n })
+      likeRecords.push({ likeTxid, txid: likeTxid, addr: actor, postTxid, ...n })
       postLikesEntries.push([`${postTxid}:${likeTxid}`, { postTxid, txid: likeTxid }])
       likesEntries.push([likeTxid, { addr: actor, postTxid, blockHeight: n.blockHeight, seen: n.seen }])
     }
@@ -125,7 +106,7 @@ function fixtureGen () {
       const childTxid = `child${i}`
       const actor = OTHERS[Math.floor(rng() * OTHERS.length)]
       const n = randomNotification()
-      replyRecords.push({ childTxid, addr: actor, ...n })
+      replyRecords.push({ childTxid, txid: childTxid, addr: actor, parentTxid, ...n })
       postChildrenEntries.push([`${parentTxid}:${childTxid}`, { parentTxid, childTxid, blockHeight: n.blockHeight }])
       postsDbEntries.push([childTxid, { addr: actor, text: 'reply', blockHeight: n.blockHeight, seen: n.seen }])
     }
@@ -183,6 +164,29 @@ function buildExpected ({ events, likeRecords, replyRecords, limit, offset }) {
   return { total: out.length, page: out.slice(offset, offset + limit) }
 }
 
+// The identity of a notification: the txid of the active follow, like, or
+// reply that produced it. Two notifications with the same identity cannot be
+// distinct.
+function identityOf (notification) {
+  return notification.txid
+}
+
+// The set of txids the adapter is allowed to return, derived directly from the
+// generated input. It mirrors the newest-follow-wins rule.
+function expectedIdentities ({ events, likeRecords, replyRecords }) {
+  const ids = new Set()
+  const newestByFollower = new Map()
+  for (const event of [...events].sort((a, b) => a.blockHeight - b.blockHeight)) {
+    newestByFollower.set(event.follower, event)
+  }
+  for (const event of newestByFollower.values()) {
+    if (!event.unfollow) ids.add(event.txid)
+  }
+  for (const like of likeRecords) ids.add(like.txid)
+  for (const reply of replyRecords) ids.add(reply.txid)
+  return ids
+}
+
 test('notifications are sorted newest-first with seen tie-break and exact pagination', async () => {
   await forAll(
     fixtureGen(),
@@ -226,3 +230,24 @@ test('the returned page is globally ordered by blockHeight then seen descending'
     { label: 'notifications global ordering invariant' }
   )
 })
+
+test('every returned notification maps to exactly one generated interaction', async () => {
+  await forAll(
+    fixtureGen(),
+    async (input) => {
+      const query = populate(input)
+      const { notifications } = await query.listNotifications(VIEWER, { limit: 1000, offset: 0 })
+      const expected = expectedIdentities(input)
+      const actual = new Set(notifications.map(identityOf))
+
+      // No duplicate identities and no missing or spurious notifications.
+      if (actual.size !== notifications.length) return false
+      if (actual.size !== expected.size) return false
+      for (const id of actual) {
+        if (!expected.has(id)) return false
+      }
+      return true
+    },
+    { label: 'notification membership matches generated interactions' }
+  )
+})

psf-memo-db/test/unit/adapters/notifications-query.unit.js

@@ -1,6 +1,7 @@
 import { assert } from 'chai'
 import sinon from 'sinon'
 import NotificationsQuery from '../../../src/adapters/notifications-query.js'
+import { FakeDb } from '../../support/level-double.js'
 
 describe('#NotificationsQuery', () => {
   let sandbox
@@ -15,33 +16,9 @@ describe('#NotificationsQuery', () => {
   const HEIGHT_PAD = 12
   const pad = (h) => String(h).padStart(HEIGHT_PAD, '0')
 
-  // A minimal LevelDB double that honors gte/lte range options and records
+  // The adapter only needs get and range iteration, both provided by the
-  // iteration calls so tests can assert that a store was never scanned.
+  // shared in-memory LevelDB double.
-  function makeDb (entries = []) {
+  const makeDb = (entries = []) => new FakeDb(entries)
-    const map = new Map(entries)
-    const iteratorCalls = []
-    return {
-      map,
-      iteratorCalls,
-      async get (key) {
-        if (!map.has(key)) {
-          const err = new Error('not found')
-          err.notFound = true
-          throw err
-        }
-        return map.get(key)
-      },
-      iterator (opts = {}) {
-        iteratorCalls.push(opts)
-        let keys = [...map.keys()].sort()
-        if (opts.gte !== undefined) keys = keys.filter((key) => key >= opts.gte)
-        if (opts.lte !== undefined) keys = keys.filter((key) => key <= opts.lte)
-        return (async function * () {
-          for (const key of keys) yield [key, map.get(key)]
-        }())
-      }
-    }
-  }
 
   function addrPostHeight (addr, height, txid) {
     return [`${addr}:${pad(height)}:${txid}`, { txid, addr, blockHeight: height }]
@@ -327,6 +304,52 @@ describe('#NotificationsQuery', () => {
     assert.equal(result.notifications[0].addr, FOLLOWER)
   })
 
+  it('should recover per-post index txids from keys when values omit them', async () => {
+    const postsDb = makeDb([
+      ['reply-keyed', { addr: REPLIER, text: 'reply' }]
+    ])
+    // addrPostHeights value omits txid; the key's final segment supplies it.
+    const addrPostHeightsDb = makeDb([
+      [`${VIEWER}:${pad(690000)}:post-recent`, { addr: VIEWER, blockHeight: 690000 }]
+    ])
+    // postLikes value omits txid and postChildren value omits childTxid.
+    const postLikesDb = makeDb([
+      ['post-recent:like-keyed', { postTxid: 'post-recent' }]
+    ])
+    const likesDb = makeDb([
+      ['like-keyed', { addr: LIKER, postTxid: 'post-recent', blockHeight: 690100 }]
+    ])
+    const postChildrenDb = makeDb([
+      ['post-recent:reply-keyed', { parentTxid: 'post-recent' }]
+    ])
+
+    const uut = buildQuery({ postsDb, addrPostHeightsDb, postLikesDb, likesDb, postChildrenDb })
+    const result = await uut.listNotifications(VIEWER, { limit: 100, offset: 0 })
+
+    assert.equal(result.total, 2)
+    assert.deepEqual(result.notifications.map((n) => n.type).sort(), ['like', 'reply'])
+  })
+
+  it('should default a notification height to 0 when no source records one', async () => {
+    const postsDb = makeDb([
+      ['reply-bare', { addr: REPLIER, text: 'reply' }]
+    ])
+    const addrPostHeightsDb = makeDb([
+      [`${VIEWER}:${pad(690000)}:post-recent`, { txid: 'post-recent', addr: VIEWER }]
+    ])
+    const postLikesDb = makeDb([postLike('post-recent', 'like-bare')])
+    const likesDb = makeDb([['like-bare', { addr: LIKER, postTxid: 'post-recent' }]])
+    const postChildrenDb = makeDb([
+      ['post-recent:reply-bare', { childTxid: 'reply-bare' }]
+    ])
+
+    const uut = buildQuery({ postsDb, addrPostHeightsDb, postLikesDb, likesDb, postChildrenDb })
+    const result = await uut.listNotifications(VIEWER, { limit: 100, offset: 0 })
+
+    assert.equal(result.total, 2)
+    assert.deepEqual(result.notifications.map((n) => n.blockHeight), [0, 0])
+  })
+
   it('should skip a missing like record', async () => {
     const postsDb = makeDb([['post-recent', { addr: VIEWER, text: 'hi' }]])
     const addrPostHeightsDb = makeDb([addrPostHeight(VIEWER, 690000, 'post-recent')])
@@ -338,6 +361,58 @@ describe('#NotificationsQuery', () => {
     assert.equal(result.total, 0)
   })
 
+  it('should skip a like whose record reports LEVEL_NOT_FOUND', async () => {
+    const postsDb = makeDb([['post-recent', { addr: VIEWER, text: 'hi' }]])
+    const addrPostHeightsDb = makeDb([addrPostHeight(VIEWER, 690000, 'post-recent')])
+    const postLikesDb = makeDb([postLike('post-recent', 'like-missing')])
+    const likesDb = makeDb()
+    likesDb.get = async () => {
+      const err = new Error('level not found')
+      err.code = 'LEVEL_NOT_FOUND'
+      throw err
+    }
+
+    const uut = buildQuery({ postsDb, addrPostHeightsDb, postLikesDb, likesDb })
+    const result = await uut.listNotifications(VIEWER, { limit: 100, offset: 0 })
+
+    assert.equal(result.total, 0)
+  })
+
+  it('should skip a like whose record reports an HTTP 404', async () => {
+    const postsDb = makeDb([['post-recent', { addr: VIEWER, text: 'hi' }]])
+    const addrPostHeightsDb = makeDb([addrPostHeight(VIEWER, 690000, 'post-recent')])
+    const postLikesDb = makeDb([postLike('post-recent', 'like-missing')])
+    const likesDb = makeDb()
+    likesDb.get = async () => {
+      const err = new Error('not found')
+      err.response = { status: 404 }
+      throw err
+    }
+
+    const uut = buildQuery({ postsDb, addrPostHeightsDb, postLikesDb, likesDb })
+    const result = await uut.listNotifications(VIEWER, { limit: 100, offset: 0 })
+
+    assert.equal(result.total, 0)
+  })
+
+  it('should rethrow a non-not-found error from the likes store', async () => {
+    const postsDb = makeDb([['post-recent', { addr: VIEWER, text: 'hi' }]])
+    const addrPostHeightsDb = makeDb([addrPostHeight(VIEWER, 690000, 'post-recent')])
+    const postLikesDb = makeDb([postLike('post-recent', 'like-recent')])
+    const likesDb = makeDb()
+    likesDb.get = async () => { throw new Error('boom') }
+
+    const uut = buildQuery({ postsDb, addrPostHeightsDb, postLikesDb, likesDb })
+    let error
+    try {
+      await uut.listNotifications(VIEWER, { limit: 100, offset: 0 })
+    } catch (err) {
+      error = err
+    }
+
+    assert.equal(error?.message, 'boom')
+  })
+
   it('should default notificationBlockWindow to 25000', async () => {
     const addrPostHeightsDb = makeDb([addrPostHeight(VIEWER, 1, 'post-old')])
     const uut = buildQuery({ addrPostHeightsDb, statusDb: makeDb([['status', { chainBlockHeight: 700000 }]]) })

psf-memo-db/test/unit/lib/backfill-followee-index.unit.js

@@ -80,6 +80,28 @@ describe('#backfillFolloweeIndex', () => {
     assert.deepEqual(db.store, before)
   })
 
+  it('should pad a missing height to zero', () => {
+    assert.equal(
+      followeeHeightKey(VIEWER_HASH, undefined, FOLLOWER_A),
+      `${VIEWER_HASH}:${'0'.repeat(12)}:${FOLLOWER_A}`
+    )
+  })
+
+  it('should skip records that cannot yield a follower and default missing fields', async () => {
+    const db = new FakeDb([
+      [':deadbeef', {}],
+      [`${FOLLOWER_A}:${VIEWER_HASH}`, { txid: 'follow-a' }]
+    ])
+    const followeeHeightsDb = new FakeDb()
+
+    const result = await backfillFolloweeIndex({ followsDb: db, followeeHeightsDb })
+
+    assert.equal(result.follows, 1)
+    const stored = followeeHeightsDb.store.get(followeeHeightKey(VIEWER_HASH, 0, FOLLOWER_A))
+    assert.equal(stored.unfollow, false)
+    assert.equal(stored.blockHeight, 0)
+  })
+
   it('should recover the follower and followee from the key when fields are missing', async () => {
     const db = new FakeDb([
       [`${FOLLOWER_A}:${VIEWER_HASH}`, { unfollow: false, blockHeight: 690400 }]