Merge 895b0b7 into 850c590

config/ldp/handler/components/error-handler.json

@@ -2,20 +2,16 @@
   "@context": "https://linkedsoftwaredependencies.org/bundles/npm/@solid/community-server/^1.0.0/components/context.jsonld",
   "@graph": [
     {
-      "comment": "Changes an error into a valid representation to send as a response.",
+      "comment": "Wraps around the main error handler as a fallback in case something goes wrong.",
       "@id": "urn:solid-server:default:ErrorHandler",
-      "@type": "WaterfallHandler",
-      "handlers": [
-        {
-          "@type": "ConvertingErrorHandler",
-          "converter": { "@id": "urn:solid-server:default:RepresentationConverter" },
-          "showStackTrace": { "@id": "urn:solid-server:default:variable:showStackTrace" }
-        },
-        {
-          "@type": "TextErrorHandler",
-          "showStackTrace": { "@id": "urn:solid-server:default:variable:showStackTrace" }
-        }
-      ]
+      "@type": "SafeErrorHandler",
+      "showStackTrace": { "@id": "urn:solid-server:default:variable:showStackTrace" },
+      "errorHandler": {
+        "comment": "Changes an error into a valid representation to send as a response.",
+        "@type": "ConvertingErrorHandler",
+        "converter": { "@id": "urn:solid-server:default:RepresentationConverter" },
+        "showStackTrace": { "@id": "urn:solid-server:default:variable:showStackTrace" }
+      }
     }
   ]
 }

package.json

@@ -166,7 +166,6 @@
     "nodemon": "^2.0.7",
     "rimraf": "^3.0.2",
     "set-cookie-parser": "^2.4.8",
-    "stream-to-string": "^1.2.0",
     "supertest": "^6.1.3",
     "ts-jest": "^27.0.3",
     "typedoc": "^0.21.2",

src/index.ts

@@ -117,10 +117,10 @@ export * from './ldp/http/PreferenceParser';
 export * from './ldp/http/RawBodyParser';
 export * from './ldp/http/RequestParser';
 export * from './ldp/http/ResponseWriter';
+export * from './ldp/http/SafeErrorHandler';
 export * from './ldp/http/SparqlUpdateBodyParser';
 export * from './ldp/http/SparqlUpdatePatch';
 export * from './ldp/http/TargetExtractor';
-export * from './ldp/http/TextErrorHandler';
 
 // LDP/Operations
 export * from './ldp/operations/DeleteOperationHandler';

src/ldp/http/TextErrorHandler.ts → src/ldp/http/SafeErrorHandler.ts

@@ -1,4 +1,5 @@
-import { getStatusCode } from '../../util/errors/ErrorUtil';
+import { getLoggerFor } from '../../logging/LogUtil';
+import { createErrorMessage, getStatusCode } from '../../util/errors/ErrorUtil';
 import { guardedStreamFrom } from '../../util/StreamUtil';
 import { toLiteral } from '../../util/TermUtil';
 import { HTTP, XSD } from '../../util/Vocabularies';
@@ -9,17 +10,27 @@ import type { ResponseDescription } from './response/ResponseDescription';
 
 /**
  * Returns a simple text description of an error.
- * This class is mostly a failsafe in case all other solutions fail.
+ * This class is a failsafe in case the wrapped error handler fails.
  */
-export class TextErrorHandler extends ErrorHandler {
+export class SafeErrorHandler extends ErrorHandler {
+  protected readonly logger = getLoggerFor(this);
+
+  private readonly errorHandler: ErrorHandler;
   private readonly showStackTrace: boolean;
 
-  public constructor(showStackTrace = false) {
+  public constructor(errorHandler: ErrorHandler, showStackTrace = false) {
     super();
+    this.errorHandler = errorHandler;
     this.showStackTrace = showStackTrace;
   }
 
-  public async handle({ error }: ErrorHandlerArgs): Promise<ResponseDescription> {
+  public async handle(input: ErrorHandlerArgs): Promise<ResponseDescription> {
+    try {
+      return await this.errorHandler.handleSafe(input);
+    } catch (error: unknown) {
+      this.logger.debug(`Recovering from error handler failure: ${createErrorMessage(error)}`);
+    }
+    const { error } = input;
     const statusCode = getStatusCode(error);
     const metadata = new RepresentationMetadata('text/plain');
     metadata.add(HTTP.terms.statusCodeNumber, toLiteral(statusCode, XSD.terms.integer));

src/storage/conversion/ChainedConverter.ts

@@ -31,107 +31,43 @@ type MatchedPath = {
   weight: number;
 };
 
-/**
- * An LRU cache for storing `ConversionPath`s.
- */
-class LruPathCache {
-  private readonly maxSize: number;
-  // Contents are ordered from least to most recently used
-  private readonly paths: ConversionPath[] = [];
-
-  public constructor(maxSize: number) {
-    this.maxSize = maxSize;
-  }
-
-  /**
-   * Add the given path to the cache as most recently used.
-   */
-  public add(path: ConversionPath): void {
-    this.paths.push(path);
-    if (this.paths.length > this.maxSize) {
-      this.paths.shift();
-    }
-  }
-
-  /**
-   * Find a path that can convert the given type to the given preferences.
-   * Note that this finds the first matching path in the cache,
-   * not the best one, should there be multiple results.
-   * In practice this should almost never be the case though.
-   */
-  public find(inType: string, outPreferences: ValuePreferences): MatchedPath | undefined {
-    // Last element is most recently used so has more chance of being the correct one
-    for (let i = this.paths.length - 1; i >= 0; --i) {
-      const path = this.paths[i];
-      // Check if `inType` matches the input and `outPreferences` the output types of the path
-      const match = this.getMatchedPath(inType, outPreferences, path);
-      if (match) {
-        // Set matched path to most recent result in the cache
-        this.paths.splice(i, 1);
-        this.paths.push(path);
-        return match;
-      }
-    }
-  }
-
-  /**
-   * Calculates the weights and exact types when using the given path on the given type and preferences.
-   * Undefined if there is no match
-   */
-  private getMatchedPath(inType: string, outPreferences: ValuePreferences, path: ConversionPath):
-  MatchedPath | undefined {
-    const inWeight = getTypeWeight(inType, path.inTypes);
-    if (inWeight === 0) {
-      return;
-    }
-    const outMatch = getBestPreference(path.outTypes, outPreferences);
-    if (!outMatch) {
-      return;
-    }
-    return { path, inType, outType: outMatch.value, weight: inWeight * outMatch.weight };
-  }
-}
-
 /**
  * A meta converter that takes an array of other converters as input.
  * It chains these converters by finding a path of converters
  * that can go from the given content-type to the given type preferences.
  * In case there are multiple paths, the one with the highest weight gets found.
  * Will error in case no path can be found.
  *
- * Generated paths get stored in an internal cache for later re-use on similar requests.
- * Note that due to this caching `RepresentationConverter`s
- * that change supported input/output types at runtime are not supported,
- * unless cache size is set to 0.
- *
  * This is not a TypedRepresentationConverter since the supported output types
  * might depend on what is the input content-type.
  *
+ * This converter should be the last in a WaterfallHandler if there are multiple,
+ * since it will try to convert any representation with a content-type.
+ *
  * Some suggestions on how this class can be even more optimized should this ever be needed in the future.
  * Most of these decrease computation time at the cost of more memory.
- *  - Subpaths that are generated could also be cached.
- *  - When looking for the next step, cached paths could also be considered.
  *  - The algorithm could start on both ends of a possible path and work towards the middle.
  *  - When creating a path, store the list of unused converters instead of checking every step.
+ *  - Caching: https://github.com/solid/community-server/issues/832
  */
 export class ChainedConverter extends RepresentationConverter {
   protected readonly logger = getLoggerFor(this);
 
   private readonly converters: TypedRepresentationConverter[];
-  private readonly cache: LruPathCache;
 
-  public constructor(converters: TypedRepresentationConverter[], maxCacheSize = 50) {
+  public constructor(converters: TypedRepresentationConverter[]) {
     super();
     if (converters.length === 0) {
       throw new Error('At least 1 converter is required.');
     }
     this.converters = [ ...converters ];
-    this.cache = new LruPathCache(maxCacheSize);
   }
 
   public async canHandle(input: RepresentationConverterArgs): Promise<void> {
-    // Will cache the path if found, and error if not
-    await this.findPath(input);
+    const type = input.representation.metadata.contentType;
+    if (!type) {
+      throw new BadRequestHttpError('Missing Content-Type header.');
+    }
   }
 
   public async handle(input: RepresentationConverterArgs): Promise<Representation> {
@@ -156,24 +92,15 @@ export class ChainedConverter extends RepresentationConverter {
     return path.converters.slice(-1)[0].handle(args);
   }
 
-  public async handleSafe(input: RepresentationConverterArgs): Promise<Representation> {
-    // This way we don't run `findPath` twice, even though it would be cached for the second call
-    return this.handle(input);
-  }
-
   private isMatchedPath(path: unknown): path is MatchedPath {
     return typeof (path as MatchedPath).path === 'object';
   }
 
   /**
-   * Finds a conversion path that can handle the given input,
-   * either in the cache or by generating a new one.
+   * Finds a conversion path that can handle the given input.
    */
   private async findPath(input: RepresentationConverterArgs): Promise<MatchedPath | ValuePreference> {
-    const type = input.representation.metadata.contentType;
-    if (!type) {
-      throw new BadRequestHttpError('Missing Content-Type header.');
-    }
+    const type = input.representation.metadata.contentType!;
     const preferences = cleanPreferences(input.preferences.type);
 
     const weight = getTypeWeight(type, preferences);
@@ -182,15 +109,7 @@ export class ChainedConverter extends RepresentationConverter {
       return { value: type, weight };
     }
 
-    // Use a cached solution if we have one.
-    // Note that it's possible that a better one could be generated.
-    // But this is usually highly unlikely.
-    let match = this.cache.find(type, preferences);
-    if (!match) {
-      match = await this.generatePath(type, preferences);
-      this.cache.add(match.path);
-    }
-    return match;
+    return this.generatePath(type, preferences);
   }
 
   /**

test/unit/ldp/http/SafeErrorHandler.test.ts

@@ -0,0 +1,77 @@
+import 'jest-rdf';
+import { DataFactory } from 'n3';
+import type { ErrorHandler } from '../../../../src/ldp/http/ErrorHandler';
+import { SafeErrorHandler } from '../../../../src/ldp/http/SafeErrorHandler';
+import { BasicRepresentation } from '../../../../src/ldp/representation/BasicRepresentation';
+import { NotFoundHttpError } from '../../../../src/util/errors/NotFoundHttpError';
+import { readableToString } from '../../../../src/util/StreamUtil';
+import { HTTP, XSD } from '../../../../src/util/Vocabularies';
+import literal = DataFactory.literal;
+
+describe('A SafeErrorHandler', (): void => {
+  let error: Error;
+  let stack: string | undefined;
+  let errorHandler: jest.Mocked<ErrorHandler>;
+  let handler: SafeErrorHandler;
+
+  beforeEach(async(): Promise<void> => {
+    error = new NotFoundHttpError('not here');
+    ({ stack } = error);
+
+    errorHandler = {
+      handleSafe: jest.fn().mockResolvedValue(new BasicRepresentation('<html>fancy error</html>', 'text/html')),
+    } as any;
+
+    handler = new SafeErrorHandler(errorHandler, true);
+  });
+
+  it('can handle everything.', async(): Promise<void> => {
+    await expect(handler.canHandle({} as any)).resolves.toBeUndefined();
+  });
+
+  it('sends the request to the stored error handler.', async(): Promise<void> => {
+    const prom = handler.handle({ error } as any);
+    await expect(prom).resolves.toBeDefined();
+    const result = await prom;
+    expect(result.metadata?.contentType).toBe('text/html');
+    await expect(readableToString(result.data!)).resolves.toBe('<html>fancy error</html>');
+  });
+
+  describe('where the wrapped error handler fails', (): void => {
+    beforeEach(async(): Promise<void> => {
+      errorHandler.handleSafe.mockRejectedValue(new Error('handler failed'));
+    });
+
+    it('creates a text representation of the error.', async(): Promise<void> => {
+      const prom = handler.handle({ error } as any);
+      await expect(prom).resolves.toBeDefined();
+      const result = await prom;
+      expect(result.statusCode).toBe(404);
+      expect(result.metadata?.get(HTTP.terms.statusCodeNumber)).toEqualRdfTerm(literal(404, XSD.terms.integer));
+      expect(result.metadata?.contentType).toBe('text/plain');
+      await expect(readableToString(result.data!)).resolves.toBe(`${stack}\n`);
+    });
+
+    it('concatenates name and message if there is no stack.', async(): Promise<void> => {
+      delete error.stack;
+      const prom = handler.handle({ error } as any);
+      await expect(prom).resolves.toBeDefined();
+      const result = await prom;
+      expect(result.statusCode).toBe(404);
+      expect(result.metadata?.get(HTTP.terms.statusCodeNumber)).toEqualRdfTerm(literal(404, XSD.terms.integer));
+      expect(result.metadata?.contentType).toBe('text/plain');
+      await expect(readableToString(result.data!)).resolves.toBe(`NotFoundHttpError: not here\n`);
+    });
+
+    it('hides the stack trace if the option is disabled.', async(): Promise<void> => {
+      handler = new SafeErrorHandler(errorHandler);
+      const prom = handler.handle({ error } as any);
+      await expect(prom).resolves.toBeDefined();
+      const result = await prom;
+      expect(result.statusCode).toBe(404);
+      expect(result.metadata?.get(HTTP.terms.statusCodeNumber)).toEqualRdfTerm(literal(404, XSD.terms.integer));
+      expect(result.metadata?.contentType).toBe('text/plain');
+      await expect(readableToString(result.data!)).resolves.toBe(`NotFoundHttpError: not here\n`);
+    });
+  });
+});