fix: cached response body can be undefined (#185)

If an empty ndjson response has been cached, the response body can
be undefined so handle this case.

Author: achingbrain

packages/client/.aegir.js

@@ -39,7 +39,6 @@ const options = {
       echo.polka.post('/add-providers/:cid', (req, res) => {
         callCount++
         try {
-
           // if request body content-type was json it's already been parsed
           const data = typeof req.body === 'string' ? JSON.parse(req.body) : req.body
 
@@ -81,17 +80,17 @@ const options = {
           const acceptHeader = req.headers.accept
           const data = providerData || { Providers: [] }
 
-          if (providerData?.Providers?.length === 0) {
-            res.statusCode = 404
-            res.end()
-            return
-          }
-
           if (acceptHeader?.includes('application/x-ndjson')) {
             res.setHeader('Content-Type', 'application/x-ndjson')
             const providers = Array.isArray(data.Providers) ? data.Providers : []
             res.end(providers.map(p => JSON.stringify(p)).join('\n'))
           } else {
+            if (providerData?.Providers?.length === 0) {
+              res.statusCode = 404
+              res.end()
+              return
+            }
+
             res.setHeader('Content-Type', 'application/json')
             res.end(JSON.stringify(data))
           }

packages/client/src/client.ts

@@ -121,7 +121,12 @@ export class DelegatedRoutingV1HttpApiClient implements DelegatedRoutingV1HttpAp
       const url = new URL(`${this.url}routing/v1/providers/${cid}`)
 
       this.#addFilterParams(url, options.filterAddrs, options.filterProtocols)
-      const getOptions = { headers: { Accept: 'application/x-ndjson' }, signal }
+      const getOptions = {
+        headers: {
+          accept: 'application/x-ndjson, application/json'
+        },
+        signal
+      }
       const res = await this.#makeRequest(url.toString(), getOptions)
 
       if (!res.ok) {
@@ -142,15 +147,22 @@ export class DelegatedRoutingV1HttpApiClient implements DelegatedRoutingV1HttpAp
         throw new BadResponseError(`Unexpected status code: ${res.status}`)
       }
 
-      if (res.body == null) {
-        throw new BadResponseError('Routing response had no body')
-      }
-
       const contentType = res.headers.get('Content-Type')
+
       if (contentType == null) {
         throw new BadResponseError('No Content-Type header received')
       }
 
+      if (res.body == null) {
+        if (contentType !== 'application/x-ndjson') {
+          throw new BadResponseError('Routing response had no body')
+        }
+
+        // cached ndjson responses have no body property if the gateway returned
+        // no results
+        return
+      }
+
       if (contentType.startsWith('application/json')) {
         const body = await res.json()
         // Handle null/undefined Providers from servers (both old and new may return empty arrays)
@@ -416,16 +428,22 @@ export class DelegatedRoutingV1HttpApiClient implements DelegatedRoutingV1HttpAp
     // Only try to use cache for GET requests
     if (requestMethod === 'GET') {
       const cachedResponse = await this.cache?.match(url)
+
       if (cachedResponse != null) {
         // Check if the cached response has expired
         const expires = parseInt(cachedResponse.headers.get('x-cache-expires') ?? '0', 10)
         if (expires > Date.now()) {
           this.log('returning cached response for %s', key)
+          this.logResponse(cachedResponse)
+
           return cachedResponse
         } else {
+          this.log('evicting cached response for %s', key)
           // Remove expired response from cache
           await this.cache?.delete(url)
         }
+      } else if (this.cache != null) {
+        this.log('cache miss for %s', key)
       }
     }
 
@@ -437,8 +455,14 @@ export class DelegatedRoutingV1HttpApiClient implements DelegatedRoutingV1HttpAp
       return response.clone()
     }
 
+    this.log('outgoing request:')
+    this.logRequest(url, options)
+
     // Create new request and track it
     const requestPromise = fetch(url, options).then(async response => {
+      this.log('incoming response:')
+      this.logResponse(response)
+
       // Only cache successful GET requests
       if (this.cache != null && response.ok && requestMethod === 'GET') {
         const expires = Date.now() + this.cacheTTL
@@ -468,4 +492,21 @@ export class DelegatedRoutingV1HttpApiClient implements DelegatedRoutingV1HttpAp
   toString (): string {
     return `DefaultDelegatedRoutingV1HttpApiClient(${this.url})`
   }
+
+  private logRequest (url: string, init: RequestInit): void {
+    const headers = new Headers(init.headers)
+    this.log('%s %s HTTP/1.1', init.method ?? 'GET', url)
+
+    for (const [key, value] of headers.entries()) {
+      this.log('%s: %s', key, value)
+    }
+  }
+
+  private logResponse (response: Response): void {
+    this.log('HTTP/1.1 %d %s', response.status, response.statusText)
+
+    for (const [key, value] of response.headers.entries()) {
+      this.log('%s: %s', key, value)
+    }
+  }
 }

packages/client/test/index.spec.ts

@@ -2,13 +2,15 @@
 
 import { generateKeyPair } from '@libp2p/crypto/keys'
 import { start, stop } from '@libp2p/interface'
+import { defaultLogger } from '@libp2p/logger'
 import { peerIdFromPrivateKey, peerIdFromString, peerIdFromCID } from '@libp2p/peer-id'
 import { multiaddr } from '@multiformats/multiaddr'
 import { expect } from 'aegir/chai'
 import { createIPNSRecord, marshalIPNSRecord } from 'ipns'
 import all from 'it-all'
 import { CID } from 'multiformats/cid'
-import { createDelegatedRoutingV1HttpApiClient } from '../src/index.js'
+import { isBrowser } from 'wherearewe'
+import { delegatedRoutingV1HttpApiClient } from '../src/index.js'
 import { itBrowser } from './fixtures/it.js'
 import type { DelegatedRoutingV1HttpApiClient } from '../src/index.js'
 
@@ -37,14 +39,27 @@ const serverUrl = process.env.ECHO_SERVER
 
 describe('delegated-routing-v1-http-api-client', () => {
   let client: DelegatedRoutingV1HttpApiClient
+  let clientWithCache: DelegatedRoutingV1HttpApiClient
 
   beforeEach(async () => {
-    client = createDelegatedRoutingV1HttpApiClient(new URL(serverUrl), { cacheTTL: 0 })
-    await start(client)
+    client = delegatedRoutingV1HttpApiClient({
+      url: new URL(serverUrl),
+      cacheTTL: 0
+    })({
+      logger: defaultLogger()
+    })
+    clientWithCache = delegatedRoutingV1HttpApiClient({
+      url: new URL(serverUrl),
+      cacheTTL: 30_000
+    })({
+      logger: defaultLogger()
+    })
+
+    await start(client, clientWithCache)
   })
 
   afterEach(async () => {
-    await stop(client)
+    await stop(client, clientWithCache)
   })
 
   it('should find providers', async () => {
@@ -108,15 +123,40 @@ describe('delegated-routing-v1-http-api-client', () => {
     await fetch(`${process.env.ECHO_SERVER}/add-providers/${cid.toString()}`, {
       method: 'POST',
       headers: {
-        Accept: 'application/x-ndjson'
+        'Content-Type': 'application/json'
       },
-      body: '' // Empty NDJSON stream
+      body: JSON.stringify({ Providers: [] })
     })
 
     const provs = await all(client.getProviders(cid))
     expect(provs).to.be.empty()
   })
 
+  it('should return empty array when no providers found in cached response (200 with empty NDJSON)', async function () {
+    if (!isBrowser) {
+      // 'globalThis.caches are only availale in browser environments'
+      this.skip()
+    }
+
+    const cid = CID.parse('QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn')
+
+    // Clear any providers - send empty NDJSON
+    await fetch(`${process.env.ECHO_SERVER}/add-providers/${cid.toString()}`, {
+      method: 'POST',
+      headers: {
+        'Content-Type': 'application/json'
+      },
+      body: JSON.stringify({ Providers: [] })
+    })
+
+    const provs = await all(clientWithCache.getProviders(cid))
+    expect(provs).to.be.empty()
+
+    // invoke again immediately to get cached response
+    const cachedProvs = await all(clientWithCache.getProviders(cid))
+    expect(cachedProvs).to.be.empty()
+  })
+
   it('should return empty array when server returns 404 for providers (old server behavior)', async () => {
     // Test backward compatibility with old servers that return 404
     const cid = CID.parse(TEST_CIDS.PROVIDERS_404)
@@ -223,9 +263,12 @@ describe('delegated-routing-v1-http-api-client', () => {
   })
 
   it('should add filter parameters the query of the request url based on global filter', async () => {
-    const client = createDelegatedRoutingV1HttpApiClient(new URL(serverUrl), {
+    const client = delegatedRoutingV1HttpApiClient({
+      url: new URL(serverUrl),
       filterProtocols: ['transport-bitswap', 'unknown'],
       filterAddrs: ['tcp', '!p2p-circuit']
+    })({
+      logger: defaultLogger()
     })
     const cid = CID.parse('QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn')
 
@@ -533,8 +576,11 @@ describe('delegated-routing-v1-http-api-client', () => {
 
   itBrowser('should respect cache TTL', async () => {
     const shortTTL = 100 // 100ms TTL for testing
-    const clientWithShortTTL = createDelegatedRoutingV1HttpApiClient(new URL(serverUrl), {
+    const clientWithShortTTL = delegatedRoutingV1HttpApiClient({
+      url: new URL(serverUrl),
       cacheTTL: shortTTL
+    })({
+      logger: defaultLogger()
     })
     await start(clientWithShortTTL)