Skip to content

Commit

Permalink
fix: Have AsyncHandlers only check what is necessary
Browse files Browse the repository at this point in the history
  • Loading branch information
joachimvh committed Oct 5, 2020
1 parent 10723bb commit 4d34cdd
Show file tree
Hide file tree
Showing 11 changed files with 45 additions and 55 deletions.
14 changes: 6 additions & 8 deletions src/ldp/http/BasicRequestParser.ts
Expand Up @@ -28,20 +28,18 @@ export class BasicRequestParser extends RequestParser {
Object.assign(this, args);
}

public async canHandle(input: HttpRequest): Promise<void> {
if (!input.url) {
throw new Error('Missing URL.');
}
if (!input.method) {
throw new Error('Missing method.');
}
public async canHandle(): Promise<void> {
// Can handle all requests
}

public async handle(input: HttpRequest): Promise<Operation> {
if (!input.method) {
throw new Error('Missing method.');
}
const target = await this.targetExtractor.handleSafe(input);
const preferences = await this.preferenceParser.handleSafe(input);
const body = await this.bodyParser.handleSafe(input);

return { method: input.method!, target, preferences, body };
return { method: input.method, target, preferences, body };
}
}
6 changes: 2 additions & 4 deletions src/ldp/http/BasicResponseWriter.ts
Expand Up @@ -10,10 +10,8 @@ import { ResponseWriter } from './ResponseWriter';
*/
export class BasicResponseWriter extends ResponseWriter {
public async canHandle(input: { response: HttpResponse; result: ResponseDescription | Error }): Promise<void> {
if (!(input.result instanceof Error)) {
if (input.result.body && !input.result.body.binary) {
throw new UnsupportedHttpError('Only binary results are supported.');
}
if (!(input.result instanceof Error) && input.result.body && !input.result.body.binary) {
throw new UnsupportedHttpError('Only binary results and errors are supported.');
}
}

Expand Down
9 changes: 5 additions & 4 deletions src/ldp/http/BasicTargetExtractor.ts
Expand Up @@ -10,16 +10,17 @@ import { TargetExtractor } from './TargetExtractor';
* TODO: input requires more extensive cleaning/parsing based on headers (see #22).
*/
export class BasicTargetExtractor extends TargetExtractor {
public async canHandle(input: HttpRequest): Promise<void> {
public async canHandle(): Promise<void> {
// Can handle all URLs
}

public async handle(input: HttpRequest): Promise<ResourceIdentifier> {
if (!input.url) {
throw new Error('Missing URL.');
}
if (!input.headers.host) {
throw new Error('Missing host.');
}
}

public async handle(input: HttpRequest): Promise<ResourceIdentifier> {
const isHttps = input.connection && (input.connection as TLSSocket).encrypted;
const url = format({
protocol: `http${isHttps ? 's' : ''}`,
Expand Down
2 changes: 1 addition & 1 deletion src/ldp/http/RawBodyParser.ts
Expand Up @@ -27,7 +27,7 @@ export class RawBodyParser extends BodyParser {
// While RFC7231 allows treating a body without content type as an octet stream,
// such an omission likely signals a mistake, so force clients to make this explicit.
if (!input.headers['content-type']) {
throw new Error('An HTTP request body was passed without Content-Type header');
throw new UnsupportedHttpError('An HTTP request body was passed without Content-Type header');
}

return {
Expand Down
7 changes: 3 additions & 4 deletions src/ldp/http/SparqlUpdateBodyParser.ts
@@ -1,6 +1,7 @@
import { PassThrough } from 'stream';
import { translate } from 'sparqlalgebrajs';
import type { HttpRequest } from '../../server/HttpRequest';
import { APPLICATION_SPARQL_UPDATE } from '../../util/ContentTypes';
import { UnsupportedHttpError } from '../../util/errors/UnsupportedHttpError';
import { UnsupportedMediaTypeHttpError } from '../../util/errors/UnsupportedMediaTypeHttpError';
import { CONTENT_TYPE } from '../../util/UriConstants';
Expand All @@ -16,9 +17,7 @@ import type { SparqlUpdatePatch } from './SparqlUpdatePatch';
*/
export class SparqlUpdateBodyParser extends BodyParser {
public async canHandle(input: HttpRequest): Promise<void> {
const contentType = input.headers['content-type'];

if (!contentType || contentType !== 'application/sparql-update') {
if (input.headers['content-type'] !== APPLICATION_SPARQL_UPDATE) {
throw new UnsupportedMediaTypeHttpError('This parser only supports SPARQL UPDATE data.');
}
}
Expand All @@ -35,7 +34,7 @@ export class SparqlUpdateBodyParser extends BodyParser {
const sparql = await readableToString(toAlgebraStream);
const algebra = translate(sparql, { quads: true });

const metadata = new RepresentationMetadata({ [CONTENT_TYPE]: 'application/sparql-update' });
const metadata = new RepresentationMetadata({ [CONTENT_TYPE]: APPLICATION_SPARQL_UPDATE });

// Prevent body from being requested again
return {
Expand Down
8 changes: 4 additions & 4 deletions src/ldp/operations/PostOperationHandler.ts
Expand Up @@ -20,13 +20,13 @@ export class PostOperationHandler extends OperationHandler {
if (input.method !== 'POST') {
throw new UnsupportedHttpError('This handler only supports POST operations.');
}
if (!input.body) {
throw new UnsupportedHttpError('POST operations require a body.');
}
}

public async handle(input: Operation): Promise<ResponseDescription> {
const identifier = await this.store.addResource(input.target, input.body!);
if (!input.body) {
throw new UnsupportedHttpError('POST operations require a body.');
}
const identifier = await this.store.addResource(input.target, input.body);
return { identifier };
}
}
22 changes: 8 additions & 14 deletions src/storage/conversion/ChainedConverter.ts
@@ -1,8 +1,6 @@
import type { Representation } from '../../ldp/representation/Representation';
import { RepresentationMetadata } from '../../ldp/representation/RepresentationMetadata';
import type { RepresentationPreferences } from '../../ldp/representation/RepresentationPreferences';
import { CONTENT_TYPE } from '../../util/UriConstants';
import { matchingMediaType } from '../../util/Util';
import { checkRequest } from './ConversionUtil';
import type { RepresentationConverterArgs } from './RepresentationConverter';
import { TypedRepresentationConverter } from './TypedRepresentationConverter';

Expand Down Expand Up @@ -44,18 +42,14 @@ export class ChainedConverter extends TypedRepresentationConverter {

public async canHandle(input: RepresentationConverterArgs): Promise<void> {
// We assume a chain can be constructed, otherwise there would be a configuration issue
// Check if the first converter can handle the input
const firstChain = await this.getMatchingType(this.converters[0], this.converters[1]);
const preferences: RepresentationPreferences = { type: [{ value: firstChain, weight: 1 }]};
await this.first.canHandle({ ...input, preferences });
// So we only check if the input can be parsed and the preferred type can be written
const inTypes = this.filterTypes(await this.first.getInputTypes());
const outTypes = this.filterTypes(await this.last.getOutputTypes());
checkRequest(input, inTypes, outTypes);
}

// Check if the last converter can produce the output
const idx = this.converters.length - 1;
const lastChain = await this.getMatchingType(this.converters[idx - 1], this.converters[idx]);
const oldMeta = input.representation.metadata;
const metadata = new RepresentationMetadata(oldMeta, { [CONTENT_TYPE]: lastChain });
const representation: Representation = { ...input.representation, metadata };
await this.last.canHandle({ ...input, representation });
private filterTypes(typeVals: { [contentType: string]: number }): string[] {
return Object.keys(typeVals).filter((name): boolean => typeVals[name] > 0);
}

public async handle(input: RepresentationConverterArgs): Promise<Representation> {
Expand Down
1 change: 1 addition & 0 deletions src/util/ContentTypes.ts
@@ -1,6 +1,7 @@
// Well-known content types
export const TEXT_TURTLE = 'text/turtle';
export const APPLICATION_OCTET_STREAM = 'application/octet-stream';
export const APPLICATION_SPARQL_UPDATE = 'application/sparql-update';

// Internal (non-exposed) content types
export const INTERNAL_QUADS = 'internal/quads';
12 changes: 4 additions & 8 deletions test/unit/ldp/http/BasicRequestParser.test.ts
Expand Up @@ -17,16 +17,12 @@ describe('A BasicRequestParser', (): void => {
requestParser = new BasicRequestParser({ targetExtractor, bodyParser, preferenceParser });
});

it('can handle input with both a URL and a method.', async(): Promise<void> => {
await expect(requestParser.canHandle({ url: 'url', method: 'GET' } as any)).resolves.toBeUndefined();
it('can handle any input.', async(): Promise<void> => {
await expect(requestParser.canHandle()).resolves.toBeUndefined();
});

it('rejects input with no URL.', async(): Promise<void> => {
await expect(requestParser.canHandle({ method: 'GET' } as any)).rejects.toThrow('Missing URL.');
});

it('rejects input with no method.', async(): Promise<void> => {
await expect(requestParser.canHandle({ url: 'url' } as any)).rejects.toThrow('Missing method.');
it('errors if there is no input.', async(): Promise<void> => {
await expect(requestParser.handle({ url: 'url' } as any)).rejects.toThrow('Missing method.');
});

it('returns the output of all input parsers after calling handle.', async(): Promise<void> => {
Expand Down
12 changes: 6 additions & 6 deletions test/unit/ldp/http/BasicTargetExtractor.test.ts
Expand Up @@ -3,16 +3,16 @@ import { BasicTargetExtractor } from '../../../../src/ldp/http/BasicTargetExtrac
describe('A BasicTargetExtractor', (): void => {
const extractor = new BasicTargetExtractor();

it('can handle input with an URL and host.', async(): Promise<void> => {
await expect(extractor.canHandle({ url: 'url', headers: { host: 'test.com' }} as any)).resolves.toBeUndefined();
it('can handle any input.', async(): Promise<void> => {
await expect(extractor.canHandle()).resolves.toBeUndefined();
});

it('rejects input without URL.', async(): Promise<void> => {
await expect(extractor.canHandle({ headers: { host: 'test.com' }} as any)).rejects.toThrow('Missing URL.');
it('errors if there is no URL.', async(): Promise<void> => {
await expect(extractor.handle({ headers: { host: 'test.com' }} as any)).rejects.toThrow('Missing URL.');
});

it('rejects input without host.', async(): Promise<void> => {
await expect(extractor.canHandle({ url: 'url', headers: {}} as any)).rejects.toThrow('Missing host.');
it('errors if there is no host.', async(): Promise<void> => {
await expect(extractor.handle({ url: 'url', headers: {}} as any)).rejects.toThrow('Missing host.');
});

it('returns the input URL.', async(): Promise<void> => {
Expand Down
7 changes: 5 additions & 2 deletions test/unit/ldp/operations/PostOperationHandler.test.ts
Expand Up @@ -10,12 +10,15 @@ describe('A PostOperationHandler', (): void => {
} as unknown as ResourceStore;
const handler = new PostOperationHandler(store);

it('only supports POST operations with a body.', async(): Promise<void> => {
it('only supports POST operations.', async(): Promise<void> => {
await expect(handler.canHandle({ method: 'POST', body: { }} as Operation))
.resolves.toBeUndefined();
await expect(handler.canHandle({ method: 'GET', body: { }} as Operation))
.rejects.toThrow(UnsupportedHttpError);
await expect(handler.canHandle({ method: 'POST' } as Operation)).rejects.toThrow(UnsupportedHttpError);
});

it('errors if there is no body.', async(): Promise<void> => {
await expect(handler.handle({ method: 'POST' } as Operation)).rejects.toThrow(UnsupportedHttpError);
});

it('adds the given representation to the store and returns the new identifier.', async(): Promise<void> => {
Expand Down

0 comments on commit 4d34cdd

Please sign in to comment.