fix: simplify plugin pipeline (#282)

Plugins can return a response or throw an error, in which case we exit
or return `null` in which case we continue.

If the plugin needs to return an error response it can do that after
doing any logging it needs to do.

Author: achingbrain

packages/verified-fetch/README.md

@@ -672,13 +672,13 @@ Each plugin must implement two methods:
   Inspects the current `PluginContext` (which includes the CID, path, query, accept header, etc.)
   and returns `true` if the plugin can operate on the current state of the request.
 
-- **`handle(context: PluginContext): Promise<Response | null>`**
-  Performs the plugin’s work. It may:
-  - **Return a final `Response`**: This stops the pipeline immediately.
-  - **Return `null`**: This indicates that the plugin has only partially processed the request
+- **`handle(context: PluginContext): Promise<Response | undefined>`**
+  Performs the plugin’s work. It will only be executed if `canHandle` previously returned `true`.
+  It may:
+  - **Return a `Response`**: This stops the pipeline immediately and returns the response.
+  - **Return `undefined`**: This indicates that the plugin has only partially processed the request
     (for example, by performing path walking or decoding) and the pipeline should continue.
-  - **Throw a `PluginError`**: This logs a non-fatal error and continues the pipeline.
-  - **Throw a `PluginFatalError`**: This logs a fatal error and stops the pipeline immediately.
+  - **Throw an `Error`**: An Internal Server Error will be returned
 
 ### Plugin Pipeline
 
@@ -816,47 +816,6 @@ To add your own plugin:
    const fetch = await createVerifiedFetch(helia, { plugins })
 ```
 
-***
-
-### Error Handling in the Plugin Pipeline
-
-Verified‑Fetch distinguishes between two types of errors thrown by plugins:
-
-- **PluginError (Non‑Fatal):**
-  - Use this when your plugin encounters an issue that should be logged but does not prevent the pipeline
-    from continuing.
-  - When a plugin throws a `PluginError`, the error is logged and the pipeline continues with the next plugin.
-
-- **PluginFatalError (Fatal):**
-  - Use this when a critical error occurs that should immediately abort the request.
-  - When a plugin throws a `PluginFatalError`, the pipeline immediately terminates and the provided error
-    response is returned.
-
-## Example - Plugin error Handling
-
-```typescript
-import { PluginError, PluginFatalError } from '@helia/verified-fetch'
-
-// async handle(context: PluginContext): Promise<Response | null> {
-const recoverable = Math.random() > 0.5 // Use more sophisticated logic here ;)
-if (recoverable === true) {
-  throw new PluginError('MY_CUSTOM_WARNING', 'A non‑fatal issue occurred', {
-    details: {
-      someKey: 'Additional details here'
-    }
-  })
-}
-
-if (recoverable === false) {
-  throw new PluginFatalError('MY_CUSTOM_FATAL', 'A critical error occurred', {
-    response: new Response('Something happened', { status: 500 })  // Required: supply your own error response
-  })
-}
-
-  // Otherwise, continue processing...
-// }
-```
-
 ### How the Plugin Pipeline Works
 
 - **Shared Context:**
@@ -874,8 +833,7 @@ if (recoverable === false) {
   This means you do not have to specify a rigid order, each plugin simply checks the context and acts if appropriate.
 
 - **Error Handling:**
-  - A thrown `PluginError` is considered non‑fatal and is logged, allowing the pipeline to continue.
-  - A thrown `PluginFatalError` immediately stops the pipeline and returns the error response.
+  - Any thrown error immediately stops the pipeline and returns the error response.
 
 For a detailed explanation of the pipeline, please refer to the discussion in [Issue #167](https://github.com/ipfs/helia-verified-fetch/issues/167).
 

packages/verified-fetch/src/index.ts

@@ -641,13 +641,13 @@
  *   Inspects the current `PluginContext` (which includes the CID, path, query, accept header, etc.)
  *   and returns `true` if the plugin can operate on the current state of the request.
  *
- * - **`handle(context: PluginContext): Promise<Response | null>`**
- *   Performs the plugin’s work. It may:
- *     - **Return a final `Response`**: This stops the pipeline immediately.
- *     - **Return `null`**: This indicates that the plugin has only partially processed the request
- *       (for example, by performing path walking or decoding) and the pipeline should continue.
- *    - **Throw a `PluginError`**: This logs a non-fatal error and continues the pipeline.
- *    - **Throw a `PluginFatalError`**: This logs a fatal error and stops the pipeline immediately.
+ * - **`handle(context: PluginContext): Promise<Response | undefined>`**
+ *   Performs the plugin’s work. It will only be executed if `canHandle` previously returned `true`.
+ *   It may:
+ *   - **Return a `Response`**: This stops the pipeline immediately and returns the response.
+ *   - **Return `undefined`**: This indicates that the plugin has only partially processed the request
+ *     (for example, by performing path walking or decoding) and the pipeline should continue.
+ *   - **Throw an `Error`**: An Internal Server Error will be returned
  *
  * ### Plugin Pipeline
  *
@@ -784,47 +784,6 @@
  *    const fetch = await createVerifiedFetch(helia, { plugins })
  *    ```
  *
- * ---
- *
- * ### Error Handling in the Plugin Pipeline
- *
- * Verified‑Fetch distinguishes between two types of errors thrown by plugins:
- *
- * - **PluginError (Non‑Fatal):**
- *   - Use this when your plugin encounters an issue that should be logged but does not prevent the pipeline
- *     from continuing.
- *   - When a plugin throws a `PluginError`, the error is logged and the pipeline continues with the next plugin.
- *
- * - **PluginFatalError (Fatal):**
- *   - Use this when a critical error occurs that should immediately abort the request.
- *   - When a plugin throws a `PluginFatalError`, the pipeline immediately terminates and the provided error
- *     response is returned.
- *
- * @example Plugin error Handling
- *
- * ```typescript
- * import { PluginError, PluginFatalError } from '@helia/verified-fetch'
- *
- * // async handle(context: PluginContext): Promise<Response | null> {
- * const recoverable = Math.random() > 0.5 // Use more sophisticated logic here ;)
- * if (recoverable === true) {
- *   throw new PluginError('MY_CUSTOM_WARNING', 'A non‑fatal issue occurred', {
- *     details: {
- *       someKey: 'Additional details here'
- *     }
- *   })
- * }
- *
- * if (recoverable === false) {
- *   throw new PluginFatalError('MY_CUSTOM_FATAL', 'A critical error occurred', {
- *     response: new Response('Something happened', { status: 500 })  // Required: supply your own error response
- *   })
- * }
- *
- *   // Otherwise, continue processing...
- * // }
- * ```
- *
  * ### How the Plugin Pipeline Works
  *
  * - **Shared Context:**
@@ -842,8 +801,7 @@
  *   This means you do not have to specify a rigid order, each plugin simply checks the context and acts if appropriate.
  *
  * - **Error Handling:**
- *   - A thrown `PluginError` is considered non‑fatal and is logged, allowing the pipeline to continue.
- *   - A thrown `PluginFatalError` immediately stops the pipeline and returns the error response.
+ *   - Any thrown error immediately stops the pipeline and returns the error response.
  *
  * For a detailed explanation of the pipeline, please refer to the discussion in [Issue #167](https://github.com/ipfs/helia-verified-fetch/issues/167).
  */

packages/verified-fetch/src/plugins/errors.ts

@@ -2,7 +2,6 @@
  * This file is the entry into all things we export from the `src/plugins` directory.
  */
 
-export { PluginError, PluginFatalError } from './errors.js'
 export { BasePlugin } from './plugin-base.js'
 export type { PluginOptions, PluginContext, VerifiedFetchPluginFactory } from './types.js'
 export * from './plugins.js'

packages/verified-fetch/src/plugins/index.ts

@@ -47,8 +47,6 @@ export class CarPlugin extends BasePlugin {
   readonly id = 'car-plugin'
 
   canHandle (context: PluginContext): boolean {
-    this.log('checking if we can handle %c with accept %s', context.cid, context.accept)
-
     if (context.byteRangeContext == null) {
       return false
     }

packages/verified-fetch/src/plugins/plugin-handle-car.ts

@@ -51,13 +51,14 @@ export class DagCborHtmlPreviewPlugin extends BasePlugin {
   readonly codes = [ipldDagCbor.code]
 
   canHandle ({ cid, accept, pathDetails }: PluginContext): boolean {
-    this.log('checking if we can handle %c with accept %s', cid, accept)
     if (pathDetails == null) {
       return false
     }
+
     if (!isObjectNode(pathDetails.terminalElement)) {
       return false
     }
+
     if (cid.code !== ipldDagCbor.code) {
       return false
     }

packages/verified-fetch/src/plugins/plugin-handle-dag-cbor-html-preview.ts

@@ -16,16 +16,18 @@ export class DagCborPlugin extends BasePlugin {
   readonly codes = [ipldDagCbor.code]
 
   canHandle ({ cid, accept, pathDetails, byteRangeContext, plugins }: PluginContext): boolean {
-    this.log('checking if we can handle %c with accept %s', cid, accept)
     if (pathDetails == null) {
       return false
     }
+
     if (!isObjectNode(pathDetails.terminalElement)) {
       return false
     }
+
     if (cid.code !== ipldDagCbor.code) {
       return false
     }
+
     if (byteRangeContext == null) {
       return false
     }

packages/verified-fetch/src/plugins/plugin-handle-dag-cbor.ts

@@ -6,7 +6,7 @@ import { CustomProgressEvent } from 'progress-events'
 import { getContentType } from '../utils/get-content-type.js'
 import { getStreamFromAsyncIterable } from '../utils/get-stream-from-async-iterable.js'
 import { setIpfsRoots } from '../utils/response-headers.js'
-import { badGatewayResponse, badRangeResponse, movedPermanentlyResponse, notSupportedResponse, okRangeResponse } from '../utils/responses.js'
+import { badGatewayResponse, badRangeResponse, movedPermanentlyResponse, okRangeResponse } from '../utils/responses.js'
 import { BasePlugin } from './plugin-base.js'
 import type { PluginContext } from './types.js'
 import type { CIDDetail } from '../index.js'
@@ -18,15 +18,21 @@ import type { UnixFSEntry } from 'ipfs-unixfs-exporter'
 export class DagPbPlugin extends BasePlugin {
   readonly id = 'dag-pb-plugin'
   readonly codes = [dagPbCode]
+
   canHandle ({ cid, accept, pathDetails, byteRangeContext }: PluginContext): boolean {
-    this.log('checking if we can handle %c with accept %s', cid, accept)
     if (pathDetails == null) {
       return false
     }
+
     if (byteRangeContext == null) {
       return false
     }
 
+    // TODO: this may be too restrictive?
+    if (accept != null && accept.mimeType !== 'application/octet-stream') {
+      return false
+    }
+
     return cid.code === dagPbCode
   }
 
@@ -105,21 +111,20 @@ export class DagPbPlugin extends BasePlugin {
         if (options?.signal?.aborted) {
           throw new AbortError(options?.signal?.reason)
         }
-        this.log.error('error loading path %c/%s', dirCid, rootFilePath, err)
+
+        this.log.error('error loading path %c/%s - %e', dirCid, rootFilePath, err)
+
         context.isDirectory = true
         context.directoryEntries = []
         context.modified++
+
         this.log.trace('attempting to get directory entries because index.html was not found')
-        try {
-          for await (const dirItem of fs.ls(dirCid, { signal: options?.signal, onProgress: options?.onProgress, extended: false })) {
-            context.directoryEntries.push(dirItem)
-          }
-          // dir-index-html plugin or dir-index-json (future idea?) plugin should handle this
-          return null
-        } catch (e) {
-          log.error('error listing directory %c', dirCid, e)
-          return notSupportedResponse('Unable to get directory contents')
+        for await (const dirItem of fs.ls(dirCid, { signal: options?.signal, onProgress: options?.onProgress, extended: false })) {
+          context.directoryEntries.push(dirItem)
         }
+
+        // dir-index-html plugin or dir-index-json (future idea?) plugin should handle this
+        return null
       } finally {
         options?.onProgress?.(new CustomProgressEvent<CIDDetail>('verified-fetch:request:end', { cid: dirCid, path: rootFilePath }))
       }
@@ -152,7 +157,7 @@ export class DagPbPlugin extends BasePlugin {
         })
         log('got async iterator for %c/%s', cid, path)
 
-        const streamAndFirstChunk = await context.serverTiming.time('stream-and-chunk', '', getStreamFromAsyncIterable(asyncIter, path ?? '', this.pluginOptions.logger, {
+        const streamAndFirstChunk = await context.serverTiming.time('stream-and-chunk', '', getStreamFromAsyncIterable(asyncIter, path, this.pluginOptions.logger, {
           onProgress: options?.onProgress,
           signal: options?.signal
         }))
@@ -177,11 +182,14 @@ export class DagPbPlugin extends BasePlugin {
       if (options?.signal?.aborted) {
         throw new AbortError(options?.signal?.reason)
       }
-      log.error('error streaming %c/%s', cid, path, err)
+
+      log.error('error streaming %c/%s - %e', cid, path, err)
+
       if (byteRangeContext.isRangeRequest && err.code === 'ERR_INVALID_PARAMS') {
         return badRangeResponse(resource)
       }
-      return badGatewayResponse(resource.toString(), 'Unable to stream content')
+
+      return badGatewayResponse(resource, 'Unable to stream content')
     }
   }
 

packages/verified-fetch/src/plugins/plugin-handle-dag-pb.ts

@@ -6,19 +6,24 @@ import { BasePlugin } from './plugin-base.js'
 import type { PluginContext } from './types.js'
 
 /**
- * This plugin should almost always run first because it's going to handle path walking if needed, and will only say it can handle
- * the request if path walking is possible (path is not empty, terminalCid is unknown, and the path has not been walked yet).
+ * This plugin should almost always run first because it's going to handle path
+ * walking if needed, and will only say it can handle the request if path
+ * walking is possible (path is not empty, terminalCid is unknown, and the path
+ * has not been walked yet).
  *
- * Once this plugin has run, the PluginContext will be updated and then this plugin will return false for canHandle, so it won't run again.
+ * Once this plugin has run, the PluginContext will be updated and then this
+ * plugin will return false for canHandle, so it won't run again.
  */
 export class DagWalkPlugin extends BasePlugin {
   readonly id = 'dag-walk-plugin'
+
   /**
-   * Return false if the path has already been walked, otherwise return true if the CID is encoded with a codec that supports pathing.
+   * Return false if the path has already been walked, otherwise return true if
+   * the CID is encoded with a codec that supports pathing.
    */
   canHandle (context: PluginContext): boolean {
-    this.log('checking if we can handle %c with accept %s', context.cid, context.accept)
     const { pathDetails, cid } = context
+
     if (pathDetails != null) {
       // path has already been walked
       return false