feat(contentful): if memoization fails, don't cache the error

It's better to return the previous sucessful result than
the new error

packages/botonic-plugin-contentful/src/contentful/cms-contentful.ts

@@ -30,6 +30,7 @@ import { StartUpDelivery } from './contents/startup'
 import { TextDelivery } from './contents/text'
 import { UrlDelivery } from './contents/url'
 import { CachedClientApi } from './delivery/cache'
+import { ClientApiErrorReporter, ReducedClientApi } from './delivery/client-api'
 import { AdaptorDeliveryApi, DeliveryApi } from './delivery-api'
 import {
   ContentfulEntryUtils,
@@ -62,13 +63,22 @@ export class Contentful implements cms.CMS {
    *  https://www.contentful.com/developers/docs/javascript/tutorials/using-js-cda-sdk/
    */
   constructor(options: ContentfulOptions) {
-    const client = createContentfulClientApi(options)
-    const deliveryApi = new AdaptorDeliveryApi(
-      options.disableCache
-        ? client
-        : new CachedClientApi(client, options.cacheTtlMs),
-      options
-    )
+    const reporter: ClientApiErrorReporter = (
+      msg: string,
+      func: string,
+      args,
+      error: any
+    ) => {
+      console.error(
+        `${msg}. '${func}(${String(args)})' threw '${String(error)}'`
+      )
+      return Promise.resolve()
+    }
+    let client: ReducedClientApi = createContentfulClientApi(options)
+    if (!options.disableCache) {
+      client = new CachedClientApi(client, options.cacheTtlMs, reporter)
+    }
+    const deliveryApi = new AdaptorDeliveryApi(client, options)
     const resumeErrors = options.resumeErrors || false
     const delivery = new IgnoreFallbackDecorator(deliveryApi)
     this._contents = new ContentsDelivery(delivery, resumeErrors)

packages/botonic-plugin-contentful/src/contentful/delivery/cache.ts

@@ -1,33 +1,82 @@
 import * as contentful from 'contentful'
-import { ContentType, Entry } from 'contentful'
+import { ContentType } from 'contentful'
 import memoize from 'memoizee'
 
-import { ReducedClientApi } from './client-api'
+import { sleep } from '../../../src/util/backoff'
+import { rethrowDecorator } from '../../util'
+import { jsonNormalizer } from '../../util/memoizer'
+import {
+  ClientApiErrorReporter,
+  GetEntriesType,
+  GetEntryType,
+  ReducedClientApi,
+} from './client-api'
 
 export class CachedClientApi implements ReducedClientApi {
+  static readonly NO_EXPIRATION = -1
   readonly getAsset: (id: string, query?: any) => Promise<contentful.Asset>
   readonly getAssets: (query?: any) => Promise<contentful.AssetCollection>
-  readonly getEntries: <T>(query: any) => Promise<contentful.EntryCollection<T>>
-  readonly getEntry: <T>(id: string, query?: any) => Promise<Entry<T>>
+  readonly getEntries: GetEntriesType
+  readonly getEntry: GetEntryType
   readonly getContentType: (id: string) => Promise<ContentType>
 
-  constructor(readonly client: ReducedClientApi, readonly cacheTtlMs = 10000) {
-    const options = (length: number) =>
-      ({
-        primitive: true,
-        maxAge: cacheTtlMs,
-        length,
-        normalizer: function (...args: any): string {
-          return args
-            .map((arg: any) => JSON.stringify(arg))
-            .reduce((a: string, b: string) => a + b)
-        },
-      } as memoize.Options<any>)
+  constructor(
+    readonly client: ReducedClientApi,
+    readonly cacheTtlMs = 10000,
+    readonly errorReport: ClientApiErrorReporter
+  ) {
+    this.getAsset = this.memoize(client.getAsset.bind(client), 2)
+    this.getAssets = this.memoize(client.getAssets.bind(client), 1)
+    this.getEntries = this.memoize(
+      client.getEntries.bind(client),
+      1
+    ) as GetEntriesType
+    this.getEntry = this.memoize(
+      client.getEntry.bind(client),
+      2
+    ) as GetEntryType
+    this.getContentType = this.memoize(client.getContentType.bind(client), 1)
+  }
+
+  memoize<
+    Args extends any[],
+    Return,
+    F extends (...args: Args) => Promise<Return>
+  >(func: F, functionLength: number): F {
+    const memo = memoize(func, this.options(functionLength))
+    const dec = rethrowDecorator<Args, Return, F>(
+      memo,
+      async (e, ...args: Args) => {
+        await this.errorReport(
+          'Error calling Contentful API',
+          String(func),
+          args,
+          e
+        )
+        // sleep required to ensure that after a failed invocation, the next one also always fails
+        // https://github.com/medikoo/memoizee/issues/117
+        return sleep(0)
+      }
+    )
+    return dec
+  }
+
+  options(length: number) {
+    return {
+      promise: true,
+      primitive: true,
+      maxAge:
+        this.cacheTtlMs == CachedClientApi.NO_EXPIRATION
+          ? undefined
+          : this.cacheTtlMs,
+      length,
+      normalizer: jsonNormalizer,
+    } as memoize.Options<any>
+  }
 
-    this.getAsset = memoize(client.getAsset, options(2))
-    this.getAssets = memoize(client.getAssets, options(1))
-    this.getEntries = memoize(client.getEntries, options(1))
-    this.getEntry = memoize(client.getEntry, options(2))
-    this.getContentType = memoize(client.getContentType, options(1))
+  static normalizer(...args: any): string {
+    return args
+      .map((arg: any) => JSON.stringify(arg))
+      .reduce((a: string, b: string) => a + b)
   }
 }

packages/botonic-plugin-contentful/src/contentful/delivery/client-api.ts

@@ -1,6 +1,20 @@
-import { ContentfulClientApi } from 'contentful'
+import { ContentfulClientApi, Entry } from 'contentful'
+import * as contentful from 'contentful'
 
 export type ReducedClientApi = Pick<
   ContentfulClientApi,
   'getAsset' | 'getAssets' | 'getEntries' | 'getEntry' | 'getContentType'
 >
+
+export type GetEntriesType = <T>(
+  query: any
+) => Promise<contentful.EntryCollection<T>>
+
+export type GetEntryType = <T>(id: string, query?: any) => Promise<Entry<T>>
+
+export type ClientApiErrorReporter = (
+  description: string,
+  functName: string,
+  args: any[],
+  err: Error
+) => Promise<void>

packages/botonic-plugin-contentful/src/util/exceptions.ts

@@ -2,3 +2,19 @@ export function isError(e: any): e is Error {
   const exception = e as Error
   return !!exception.name && !!exception.message
 }
+
+export function rethrowDecorator<
+  Args extends any[],
+  Return,
+  F extends (...args: Args) => Promise<Return>
+>(func: F, beforeRethrow: (error: any, ...args: Args) => Promise<void>): F {
+  const f = async (...args: Args) => {
+    try {
+      return await func(...args)
+    } catch (e) {
+      await beforeRethrow(e, ...args)
+      throw e
+    }
+  }
+  return f as F
+}

packages/botonic-plugin-contentful/tests/contentful/delivery/cache.test.ts

@@ -1,60 +1,79 @@
-import { Entry, EntryCollection } from 'contentful'
-import { instance, mock, verify, when } from 'ts-mockito'
-
 import { CachedClientApi } from '../../../src/contentful/delivery/cache'
-import { ReducedClientApi } from '../../../src/contentful/delivery/client-api'
+import { MockClientApi } from './mock-client.helper'
 
-test('TEST: CachedDelivery getEntries', async () => {
-  const CACHE_TTL = 30
-  const mockApi = mock<ReducedClientApi>()
-  const query = {}
-  const entryCollection = ({
-    items: [],
-  } as any) as EntryCollection<any>
-  when(mockApi.getEntries(query)).thenResolve(entryCollection)
-  const sut = new CachedClientApi(instance(mockApi), CACHE_TTL)
-
-  await expect(sut.getEntries(query)).resolves.toBe(entryCollection)
-  await expect(sut.getEntries(query)).resolves.toBe(entryCollection)
-  verify(mockApi.getEntries(query)).once()
-
-  await new Promise(resolve => setTimeout(resolve, CACHE_TTL))
-  await expect(sut.getEntries(query)).resolves.toBe(entryCollection)
-  verify(mockApi.getEntries(query)).twice()
+test('TEST: CachedClientApi getAsset is cached', async () => {
+  const id = Math.random().toString()
+  await testHitAndMiss(
+    sut => sut.getAsset(id),
+    sut => sut.asset
+  )
 })
 
-test('TEST: CachedDelivery getEntry', async () => {
-  const CACHE_TTL = 30
-  const mockApi = mock<ReducedClientApi>()
-  const id = Math.random().toString()
-  const query = {}
-  const entry = ({} as any) as Entry<any>
-  when(mockApi.getEntry(id, query)).thenResolve(entry)
-  const sut = new CachedClientApi(instance(mockApi), CACHE_TTL)
+test('TEST: CachedClientApi getAssets is cached', async () => {
+  await testHitAndMiss(
+    sut => sut.getAssets({}),
+    sut => sut.assetCollection
+  )
+})
 
-  await expect(sut.getEntry(id, query)).resolves.toBe(entry)
-  await expect(sut.getEntry(id, query)).resolves.toBe(entry)
-  verify(mockApi.getEntry(id, query)).once()
+test('TEST: CachedClientApi getEntries is cached', async () => {
+  await testHitAndMiss(
+    sut => sut.getEntries({}),
+    sut => sut.entryCollection
+  )
+})
 
-  await new Promise(resolve => setTimeout(resolve, CACHE_TTL))
-  await expect(sut.getEntry(id, query)).resolves.toBe(entry)
-  verify(mockApi.getEntry(id, query)).twice()
+test('TEST: CachedClientApi getEntry is cached', async () => {
+  const id = Math.random().toString()
+  await testHitAndMiss(
+    sut => sut.getEntry(id, {}),
+    sut => sut.entry
+  )
 })
 
-test('TEST: CachedDelivery getAsset', async () => {
+async function testHitAndMiss<R>(
+  call: (api: CachedClientApi) => Promise<R>,
+  expectedReturn: (api: MockClientApi) => R
+) {
   const CACHE_TTL = 30
-  const mockApi = mock<ReducedClientApi>()
+  const mockApi = new MockClientApi()
+  const sut = new CachedClientApi(mockApi, CACHE_TTL, apiFailed)
+  const expected = expectedReturn(mockApi)
+
+  // cache hit does not perform an extra call
+  await expect(call(sut)).resolves.toBe(expected)
+  expect(mockApi.numCalls).toBe(1)
+  await expect(call(sut)).resolves.toBe(expected)
+  expect(mockApi.numCalls).toBe(1)
+
+  // cache miss (due to timeout) performs an extra call
+  await new Promise(resolve => setTimeout(resolve, CACHE_TTL + 10))
+  await expect(call(sut)).resolves.toBe(expected)
+  expect(mockApi.numCalls).toBe(2)
+}
+
+test('TEST: CachedClientApi does not remember exceptions', async () => {
+  const CACHE_TTL = 300000
+  const mockApi = new MockClientApi()
   const id = Math.random().toString()
   const query = {}
-  const entry = ({} as any) as Entry<any>
-  when(mockApi.getAsset(id, query)).thenResolve(entry)
-  const sut = new CachedClientApi(instance(mockApi), CACHE_TTL)
+  const sut = new CachedClientApi(mockApi, CACHE_TTL, apiFailed)
 
-  await expect(sut.getAsset(id, query)).resolves.toBe(entry)
-  await expect(sut.getAsset(id, query)).resolves.toBe(entry)
-  verify(mockApi.getAsset(id, query)).once()
+  mockApi.error = new Error('forced failure')
+  await expect(sut.getEntry(id, query)).rejects.toThrowError(mockApi.error)
 
-  await new Promise(resolve => setTimeout(resolve, CACHE_TTL))
-  await expect(sut.getAsset(id, query)).resolves.toBe(entry)
-  verify(mockApi.getAsset(id, query)).twice()
+  mockApi.error = undefined
+  await expect(sut.getEntry(id, query)).resolves.toBe(mockApi.entry)
 })
+
+export function apiFailed(
+  description: string,
+  funcName: string,
+  args: any[],
+  e: any
+): Promise<void> {
+  console.error(
+    `${description}: ${funcName}(${String(args)}) threw error: ${String(e)}`
+  )
+  return Promise.resolve()
+}

packages/botonic-plugin-contentful/tests/contentful/delivery/mock-client.helper.ts

@@ -0,0 +1,68 @@
+import contentful, {
+  Asset,
+  AssetCollection,
+  ContentType,
+  Entry,
+  EntryCollection,
+} from 'contentful'
+
+import { ReducedClientApi } from '../../../src/contentful/delivery/client-api'
+
+// not using mockito because (maybe due to https://github.com/medikoo/memoizee/issues/117),
+// consecutive calls without a sleep in between were not hitting the cache
+export class MockClientApi implements ReducedClientApi {
+  error: Error | undefined
+  numCalls = 0
+
+  asset = ({ ka: 'va' } as any) as Asset
+  assetCollection = ({
+    items: [this.asset],
+  } as any) as AssetCollection
+
+  contentType = ({ kct: 'vct' } as any) as ContentType
+
+  entry = ({ ke: 've' } as any) as Entry<any>
+  entryCollection = ({
+    items: [],
+  } as any) as EntryCollection<any>
+
+  getAsset(id: string, query: any): Promise<contentful.Asset> {
+    this.numCalls++
+    if (this.error) {
+      return Promise.reject(this.error)
+    }
+    return Promise.resolve(this.asset)
+  }
+
+  getAssets(query: any): Promise<contentful.AssetCollection> {
+    this.numCalls++
+    if (this.error) {
+      return Promise.reject(this.error)
+    }
+    return Promise.resolve(this.assetCollection)
+  }
+
+  getContentType(id: string): Promise<contentful.ContentType> {
+    this.numCalls++
+    if (this.error) {
+      return Promise.reject(this.error)
+    }
+    return Promise.resolve(this.contentType)
+  }
+
+  getEntries<T>(query: any): Promise<EntryCollection<T>> {
+    this.numCalls++
+    if (this.error) {
+      return Promise.reject(this.error)
+    }
+    return Promise.resolve(this.entryCollection)
+  }
+
+  getEntry<T>(id: string, query: any): Promise<Entry<T>> {
+    this.numCalls++
+    if (this.error) {
+      return Promise.reject(this.error)
+    }
+    return Promise.resolve(this.entry)
+  }
+}