Skip to content

Commit

Permalink
fix: Remove all instanceof checks
Browse files Browse the repository at this point in the history
This prevents problems with different environments.
Also introduces unit tests to double check HttpError values.
  • Loading branch information
joachimvh committed Jan 25, 2021
1 parent a07fdde commit e752927
Show file tree
Hide file tree
Showing 26 changed files with 124 additions and 25 deletions.
2 changes: 1 addition & 1 deletion src/authorization/WebAclAuthorizer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ export class WebAclAuthorizer extends Authorizer {
const resourceId = await this.aclManager.getAclConstrainedResource(id);
return this.filterData(data, recurse ? ACL.default : ACL.accessTo, resourceId.path);
} catch (error: unknown) {
if (error instanceof NotFoundHttpError) {
if (NotFoundHttpError.isInstance(error)) {
this.logger.debug(`No direct ACL document found for ${id.path}`);
} else {
this.logger.error(`Error reading ACL for ${id.path}: ${(error as Error).message}`, { error });
Expand Down
3 changes: 2 additions & 1 deletion src/ldp/AuthenticatedLdpHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type { HttpHandlerInput } from '../server/HttpHandler';
import { HttpHandler } from '../server/HttpHandler';
import type { HttpRequest } from '../server/HttpRequest';
import type { HttpResponse } from '../server/HttpResponse';
import { isNativeError } from '../util/errors/ErrorUtil';
import type { RequestParser } from './http/RequestParser';
import type { ResponseDescription } from './http/response/ResponseDescription';
import type { ResponseWriter } from './http/ResponseWriter';
Expand Down Expand Up @@ -95,7 +96,7 @@ export class AuthenticatedLdpHandler extends HttpHandler {
try {
writeData = { response: input.response, result: await this.runHandlers(input.request) };
} catch (error: unknown) {
if (error instanceof Error) {
if (isNativeError(error)) {
writeData = { response: input.response, result: error };
} else {
throw error;
Expand Down
3 changes: 2 additions & 1 deletion src/ldp/http/BasicResponseWriter.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { getLoggerFor } from '../../logging/LogUtil';
import type { HttpResponse } from '../../server/HttpResponse';
import { INTERNAL_QUADS } from '../../util/ContentTypes';
import { isNativeError } from '../../util/errors/ErrorUtil';
import { NotImplementedHttpError } from '../../util/errors/NotImplementedHttpError';
import { pipeSafely } from '../../util/StreamUtil';
import type { MetadataWriter } from './metadata/MetadataWriter';
Expand All @@ -20,7 +21,7 @@ export class BasicResponseWriter extends ResponseWriter {
}

public async canHandle(input: { response: HttpResponse; result: ResponseDescription | Error }): Promise<void> {
if (input.result instanceof Error || input.result.metadata?.contentType === INTERNAL_QUADS) {
if (isNativeError(input.result) || input.result.metadata?.contentType === INTERNAL_QUADS) {
throw new NotImplementedHttpError('Only successful binary responses are supported');
}
}
Expand Down
5 changes: 3 additions & 2 deletions src/ldp/http/ErrorResponseWriter.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { getLoggerFor } from '../../logging/LogUtil';
import type { HttpResponse } from '../../server/HttpResponse';
import { isNativeError } from '../../util/errors/ErrorUtil';
import { HttpError } from '../../util/errors/HttpError';
import { NotImplementedHttpError } from '../../util/errors/NotImplementedHttpError';
import type { ResponseDescription } from './response/ResponseDescription';
Expand All @@ -12,14 +13,14 @@ export class ErrorResponseWriter extends ResponseWriter {
protected readonly logger = getLoggerFor(this);

public async canHandle(input: { response: HttpResponse; result: ResponseDescription | Error }): Promise<void> {
if (!(input.result instanceof Error)) {
if (!isNativeError(input.result)) {
throw new NotImplementedHttpError('Only errors are supported');
}
}

public async handle(input: { response: HttpResponse; result: Error }): Promise<void> {
let code = 500;
if (input.result instanceof HttpError) {
if (HttpError.isInstance(input.result)) {
code = input.result.statusCode;
}
input.response.setHeader('content-type', 'text/plain');
Expand Down
4 changes: 2 additions & 2 deletions src/ldp/http/SparqlUpdateBodyParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ import { translate } from 'sparqlalgebrajs';
import { getLoggerFor } from '../../logging/LogUtil';
import { APPLICATION_SPARQL_UPDATE } from '../../util/ContentTypes';
import { BadRequestHttpError } from '../../util/errors/BadRequestHttpError';
import { isNativeError } from '../../util/errors/ErrorUtil';
import { UnsupportedMediaTypeHttpError } from '../../util/errors/UnsupportedMediaTypeHttpError';
import { pipeSafely, readableToString } from '../../util/StreamUtil';
import type { BodyParserArgs } from './BodyParser';
import { BodyParser } from './BodyParser';
import type { SparqlUpdatePatch } from './SparqlUpdatePatch';

/**
* {@link BodyParser} that supports `application/sparql-update` content.
* Will convert the incoming update string to algebra in a {@link SparqlUpdatePatch}.
Expand All @@ -34,7 +34,7 @@ export class SparqlUpdateBodyParser extends BodyParser {
algebra = translate(sparql, { quads: true, baseIRI: metadata.identifier.value });
} catch (error: unknown) {
this.logger.warn('Could not translate SPARQL query to SPARQL algebra', { error });
if (error instanceof Error) {
if (isNativeError(error)) {
throw new BadRequestHttpError(error.message);
}
throw new BadRequestHttpError();
Expand Down
3 changes: 2 additions & 1 deletion src/pods/PodManagerHttpHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { HttpHandler } from '../server/HttpHandler';
import type { HttpRequest } from '../server/HttpRequest';
import type { HttpResponse } from '../server/HttpResponse';
import { BadRequestHttpError } from '../util/errors/BadRequestHttpError';
import { isNativeError } from '../util/errors/ErrorUtil';
import { InternalServerError } from '../util/errors/InternalServerError';
import { NotImplementedHttpError } from '../util/errors/NotImplementedHttpError';
import type { AgentParser } from './agent/AgentParser';
Expand Down Expand Up @@ -59,7 +60,7 @@ export class PodManagerHttpHandler extends HttpHandler {

await this.responseWriter.handleSafe({ response, result: new CreatedResponseDescription(id) });
} catch (error: unknown) {
if (error instanceof Error) {
if (isNativeError(error)) {
await this.responseWriter.handleSafe({ response, result: error });
} else {
await this.responseWriter.handleSafe({ response, result: new InternalServerError('Unexpected error') });
Expand Down
3 changes: 2 additions & 1 deletion src/server/ExpressHttpServerFactory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { Server } from 'http';
import type { Express } from 'express';
import express from 'express';
import { getLoggerFor } from '../logging/LogUtil';
import { isNativeError } from '../util/errors/ErrorUtil';
import { guardStream } from '../util/GuardedStream';
import type { HttpHandler } from './HttpHandler';
import type { HttpServerFactory } from './HttpServerFactory';
Expand All @@ -26,7 +27,7 @@ export class ExpressHttpServerFactory implements HttpServerFactory {
this.logger.info(`Received request for ${request.url}`);
await this.handler.handleSafe({ request: guardStream(request), response });
} catch (error: unknown) {
const errMsg = error instanceof Error ? `${error.name}: ${error.message}\n${error.stack}` : 'Unknown error.';
const errMsg = isNativeError(error) ? `${error.name}: ${error.message}\n${error.stack}` : 'Unknown error.';
this.logger.error(errMsg);
if (response.headersSent) {
response.end();
Expand Down
13 changes: 7 additions & 6 deletions src/storage/DataAccessorBasedStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import type { ResourceIdentifier } from '../ldp/representation/ResourceIdentifie
import { INTERNAL_QUADS } from '../util/ContentTypes';
import { BadRequestHttpError } from '../util/errors/BadRequestHttpError';
import { ConflictHttpError } from '../util/errors/ConflictHttpError';
import { isNativeError } from '../util/errors/ErrorUtil';
import { ForbiddenHttpError } from '../util/errors/ForbiddenHttpError';
import { MethodNotAllowedHttpError } from '../util/errors/MethodNotAllowedHttpError';
import { NotFoundHttpError } from '../util/errors/NotFoundHttpError';
Expand Down Expand Up @@ -194,7 +195,7 @@ export class DataAccessorBasedStore implements ResourceStore {
try {
return await this.accessor.getMetadata(identifier);
} catch (error: unknown) {
if (error instanceof NotFoundHttpError) {
if (NotFoundHttpError.isInstance(error)) {
const otherIdentifier =
{ path: hasSlash ? trimTrailingSlashes(identifier.path) : ensureTrailingSlash(identifier.path) };

Expand All @@ -214,7 +215,7 @@ export class DataAccessorBasedStore implements ResourceStore {
try {
return await this.getNormalizedMetadata(identifier);
} catch (error: unknown) {
if (!(error instanceof NotFoundHttpError)) {
if (!NotFoundHttpError.isInstance(error)) {
throw error;
}
}
Expand Down Expand Up @@ -270,7 +271,7 @@ export class DataAccessorBasedStore implements ResourceStore {
quads = await parseQuads(representation.data, { format: contentType, baseIRI: identifier.value });
}
} catch (error: unknown) {
if (error instanceof Error) {
if (isNativeError(error)) {
throw new BadRequestHttpError(`Can only create containers with RDF data. ${error.message}`);
}
throw error;
Expand Down Expand Up @@ -354,8 +355,8 @@ export class DataAccessorBasedStore implements ResourceStore {
/**
* Checks in a list of types if any of them match a Container type.
*/
protected hasContainerType(types: Term[]): boolean {
return types.some((type): boolean => type.value === LDP.Container || type.value === LDP.BasicContainer);
protected hasContainerType(rdfTypes: Term[]): boolean {
return rdfTypes.some((type): boolean => type.value === LDP.Container || type.value === LDP.BasicContainer);
}

/**
Expand All @@ -382,7 +383,7 @@ export class DataAccessorBasedStore implements ResourceStore {
throw new ForbiddenHttpError(`Creating container ${container.path} conflicts with an existing resource.`);
}
} catch (error: unknown) {
if (error instanceof NotFoundHttpError) {
if (NotFoundHttpError.isInstance(error)) {
// Make sure the parent exists first
await this.createRecursiveContainers(this.identifierStrategy.getParentContainer(container));
await this.writeData(container, new BasicRepresentation([], container), true);
Expand Down
2 changes: 1 addition & 1 deletion src/storage/RoutingResourceStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export class RoutingResourceStore implements ResourceStore {
try {
return await this.rule.handleSafe({ identifier });
} catch (error: unknown) {
if (error instanceof NotImplementedHttpError) {
if (NotImplementedHttpError.isInstance(error)) {
throw new NotFoundHttpError();
}
throw error;
Expand Down
4 changes: 2 additions & 2 deletions src/storage/StoreUtil.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
import type { ResourceIdentifier } from '../ldp/representation/ResourceIdentifier';
import type { ResourceStore } from '../storage/ResourceStore';
import { NotFoundHttpError } from '../util/errors/NotFoundHttpError';
import type { ResourceStore } from './ResourceStore';

export async function containsResource(store: ResourceStore, identifier: ResourceIdentifier): Promise<boolean> {
try {
const result = await store.getRepresentation(identifier, {});
result.data.destroy();
return true;
} catch (error: unknown) {
if (error instanceof NotFoundHttpError) {
if (NotFoundHttpError.isInstance(error)) {
return false;
}
throw error;
Expand Down
2 changes: 1 addition & 1 deletion src/storage/accessors/InMemoryDataAccessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ export class InMemoryDataAccessor implements DataAccessor {
entry.metadata = metadata;
} catch (error: unknown) {
// Create new entry if it didn't exist yet
if (error instanceof NotFoundHttpError) {
if (NotFoundHttpError.isInstance(error)) {
const { parent, name } = this.getParentEntry(identifier);
parent.entries[name] = {
entries: {},
Expand Down
5 changes: 3 additions & 2 deletions src/storage/accessors/SparqlDataAccessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import type { ResourceIdentifier } from '../../ldp/representation/ResourceIdenti
import { getLoggerFor } from '../../logging/LogUtil';
import { INTERNAL_QUADS } from '../../util/ContentTypes';
import { ConflictHttpError } from '../../util/errors/ConflictHttpError';
import { isNativeError } from '../../util/errors/ErrorUtil';
import { NotFoundHttpError } from '../../util/errors/NotFoundHttpError';
import { NotImplementedHttpError } from '../../util/errors/NotImplementedHttpError';
import { UnsupportedMediaTypeHttpError } from '../../util/errors/UnsupportedMediaTypeHttpError';
Expand Down Expand Up @@ -310,7 +311,7 @@ export class SparqlDataAccessor implements DataAccessor {
try {
return guardStream(await this.fetcher.fetchTriples(this.endpoint, query));
} catch (error: unknown) {
if (error instanceof Error) {
if (isNativeError(error)) {
this.logger.error(`SPARQL endpoint ${this.endpoint} error: ${error.message}`);
}
throw error;
Expand All @@ -327,7 +328,7 @@ export class SparqlDataAccessor implements DataAccessor {
try {
return await this.fetcher.fetchUpdate(this.endpoint, query);
} catch (error: unknown) {
if (error instanceof Error) {
if (isNativeError(error)) {
this.logger.error(`SPARQL endpoint ${this.endpoint} error: ${error.message}`);
}
throw error;
Expand Down
2 changes: 1 addition & 1 deletion src/storage/patch/SparqlUpdatePatchHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ export class SparqlUpdatePatchHandler extends PatchHandler {
} catch (error: unknown) {
// Solid, §5.1: "Clients who want to assign a URI to a resource, MUST use PUT and PATCH requests."
// https://solid.github.io/specification/protocol#resource-type-heuristics
if (!(error instanceof NotFoundHttpError)) {
if (!NotFoundHttpError.isInstance(error)) {
throw error;
}
this.logger.debug(`Patching new resource ${identifier.path}.`);
Expand Down
5 changes: 3 additions & 2 deletions src/util/WaterfallHandler.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { getLoggerFor } from '../logging/LogUtil';
import type { AsyncHandler } from './AsyncHandler';
import { BadRequestHttpError } from './errors/BadRequestHttpError';
import { isNativeError } from './errors/ErrorUtil';
import { HttpError } from './errors/HttpError';
import { InternalServerError } from './errors/InternalServerError';

Expand Down Expand Up @@ -84,9 +85,9 @@ export class WaterfallHandler<TIn, TOut> implements AsyncHandler<TIn, TOut> {

return handler;
} catch (error: unknown) {
if (error instanceof HttpError) {
if (HttpError.isInstance(error)) {
errors.push(error);
} else if (error instanceof Error) {
} else if (isNativeError(error)) {
errors.push(new InternalServerError(error.message));
} else {
errors.push(new InternalServerError('Unknown error'));
Expand Down
4 changes: 4 additions & 0 deletions src/util/errors/BadRequestHttpError.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,8 @@ export class BadRequestHttpError extends HttpError {
public constructor(message?: string) {
super(400, 'BadRequestHttpError', message ?? 'The given input is not supported by the server configuration.');
}

public static isInstance(error: any): error is BadRequestHttpError {
return HttpError.isInstance(error) && error.statusCode === 400;
}
}
4 changes: 4 additions & 0 deletions src/util/errors/ConflictHttpError.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,8 @@ export class ConflictHttpError extends HttpError {
public constructor(message?: string) {
super(409, 'ConflictHttpError', message);
}

public static isInstance(error: any): error is ConflictHttpError {
return HttpError.isInstance(error) && error.statusCode === 409;
}
}
8 changes: 8 additions & 0 deletions src/util/errors/ErrorUtil.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { types } from 'util';

/**
* Checks if the input is an {@link Error}.
*/
export function isNativeError(error: any): error is Error {
return types.isNativeError(error);
}
4 changes: 4 additions & 0 deletions src/util/errors/ForbiddenHttpError.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,8 @@ export class ForbiddenHttpError extends HttpError {
public constructor(message?: string) {
super(403, 'ForbiddenHttpError', message);
}

public static isInstance(error: any): error is ForbiddenHttpError {
return HttpError.isInstance(error) && error.statusCode === 403;
}
}
9 changes: 8 additions & 1 deletion src/util/errors/HttpError.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import { isNativeError } from './ErrorUtil';

/**
* A class for all errors that could be thrown by Solid.
* All errors inheriting from this should fix the status code thereby hiding the HTTP internals from other components.
*/
export class HttpError extends Error {
public statusCode: number;
protected static readonly statusCode: number;
public readonly statusCode: number;

/**
* Creates a new HTTP error. Subclasses should call this with their fixed status code.
Expand All @@ -16,4 +19,8 @@ export class HttpError extends Error {
this.statusCode = statusCode;
this.name = name;
}

public static isInstance(error: any): error is HttpError {
return isNativeError(error) && typeof (error as any).statusCode === 'number';
}
}
4 changes: 4 additions & 0 deletions src/util/errors/InternalServerError.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,8 @@ export class InternalServerError extends HttpError {
public constructor(message?: string) {
super(500, 'InternalServerError', message);
}

public static isInstance(error: any): error is InternalServerError {
return HttpError.isInstance(error) && error.statusCode === 500;
}
}
4 changes: 4 additions & 0 deletions src/util/errors/MethodNotAllowedHttpError.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,8 @@ export class MethodNotAllowedHttpError extends HttpError {
public constructor(message?: string) {
super(405, 'MethodNotAllowedHttpError', message);
}

public static isInstance(error: any): error is MethodNotAllowedHttpError {
return HttpError.isInstance(error) && error.statusCode === 405;
}
}
4 changes: 4 additions & 0 deletions src/util/errors/NotFoundHttpError.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,8 @@ export class NotFoundHttpError extends HttpError {
public constructor(message?: string) {
super(404, 'NotFoundHttpError', message);
}

public static isInstance(error: any): error is NotFoundHttpError {
return HttpError.isInstance(error) && error.statusCode === 404;
}
}
4 changes: 4 additions & 0 deletions src/util/errors/NotImplementedHttpError.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,8 @@ export class NotImplementedHttpError extends HttpError {
public constructor(message?: string) {
super(501, 'NotImplementedHttpError', message);
}

public static isInstance(error: any): error is NotImplementedHttpError {
return HttpError.isInstance(error) && error.statusCode === 501;
}
}
4 changes: 4 additions & 0 deletions src/util/errors/UnauthorizedHttpError.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,8 @@ export class UnauthorizedHttpError extends HttpError {
public constructor(message?: string) {
super(401, 'UnauthorizedHttpError', message);
}

public static isInstance(error: any): error is UnauthorizedHttpError {
return HttpError.isInstance(error) && error.statusCode === 401;
}
}
4 changes: 4 additions & 0 deletions src/util/errors/UnsupportedMediaTypeHttpError.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,8 @@ export class UnsupportedMediaTypeHttpError extends HttpError {
public constructor(message?: string) {
super(415, 'UnsupportedMediaTypeHttpError', message);
}

public static isInstance(error: any): error is UnsupportedMediaTypeHttpError {
return HttpError.isInstance(error) && error.statusCode === 415;
}
}

0 comments on commit e752927

Please sign in to comment.