diff --git a/src/http/ldp/GetOperationHandler.ts b/src/http/ldp/GetOperationHandler.ts index 4e5b5c417..ffc63e4eb 100644 --- a/src/http/ldp/GetOperationHandler.ts +++ b/src/http/ldp/GetOperationHandler.ts @@ -2,7 +2,6 @@ import type { ETagHandler } from '../../storage/conditions/ETagHandler'; import type { ResourceStore } from '../../storage/ResourceStore'; import { NotImplementedHttpError } from '../../util/errors/NotImplementedHttpError'; import { assertReadConditions } from '../../util/ResourceUtil'; -import { HH } from '../../util/Vocabularies'; import { OkResponseDescription } from '../output/response/OkResponseDescription'; import type { ResponseDescription } from '../output/response/ResponseDescription'; import type { OperationHandlerInput } from './OperationHandler'; @@ -32,11 +31,7 @@ export class GetOperationHandler extends OperationHandler { const body = await this.store.getRepresentation(operation.target, operation.preferences, operation.conditions); // Check whether the cached representation is still valid or it is necessary to send a new representation - assertReadConditions(body, operation.conditions); - - // Add the ETag of the returned representation - const etag = this.eTagHandler.getETag(body.metadata); - body.metadata.set(HH.terms.etag, etag); + assertReadConditions(body, this.eTagHandler, operation.conditions); return new OkResponseDescription(body.metadata, body.data); } diff --git a/src/http/ldp/HeadOperationHandler.ts b/src/http/ldp/HeadOperationHandler.ts index c15aa87df..dac2414fa 100644 --- a/src/http/ldp/HeadOperationHandler.ts +++ b/src/http/ldp/HeadOperationHandler.ts @@ -2,7 +2,6 @@ import type { ETagHandler } from '../../storage/conditions/ETagHandler'; import type { ResourceStore } from '../../storage/ResourceStore'; import { NotImplementedHttpError } from '../../util/errors/NotImplementedHttpError'; import { assertReadConditions } from '../../util/ResourceUtil'; -import { HH } from '../../util/Vocabularies'; import { OkResponseDescription } from '../output/response/OkResponseDescription'; import type { ResponseDescription } from '../output/response/ResponseDescription'; import type { OperationHandlerInput } from './OperationHandler'; @@ -31,16 +30,12 @@ export class HeadOperationHandler extends OperationHandler { public async handle({ operation }: OperationHandlerInput): Promise { const body = await this.store.getRepresentation(operation.target, operation.preferences, operation.conditions); - // Close the Readable as we will not return it. - body.data.destroy(); - // Check whether the cached representation is still valid or it is necessary to send a new representation. // Generally it doesn't make much sense to use condition headers with a HEAD request, but it should be supported. - assertReadConditions(body, operation.conditions); + assertReadConditions(body, this.eTagHandler, operation.conditions); - // Add the ETag of the returned representation - const etag = this.eTagHandler.getETag(body.metadata); - body.metadata.set(HH.terms.etag, etag); + // Close the Readable as we will not return it. + body.data.destroy(); return new OkResponseDescription(body.metadata); } diff --git a/src/util/ResourceUtil.ts b/src/util/ResourceUtil.ts index 090b77b97..2ef9504b9 100644 --- a/src/util/ResourceUtil.ts +++ b/src/util/ResourceUtil.ts @@ -4,10 +4,11 @@ import { BasicRepresentation } from '../http/representation/BasicRepresentation' import type { Representation } from '../http/representation/Representation'; import { RepresentationMetadata } from '../http/representation/RepresentationMetadata'; import type { Conditions } from '../storage/conditions/Conditions'; +import type { ETagHandler } from '../storage/conditions/ETagHandler'; import { NotModifiedHttpError } from './errors/NotModifiedHttpError'; import { guardedStreamFrom } from './StreamUtil'; import { toLiteral } from './TermUtil'; -import { CONTENT_TYPE_TERM, DC, LDP, RDF, SOLID_META, XSD } from './Vocabularies'; +import { CONTENT_TYPE_TERM, DC, HH, LDP, RDF, SOLID_META, XSD } from './Vocabularies'; import namedNode = DataFactory.namedNode; /** @@ -70,7 +71,8 @@ export async function cloneRepresentation(representation: Representation): Promi /** * Verify whether the given {@link Representation} matches the given conditions. - * If not, destroy the data stream and throw a {@link NotModifiedHttpError}. + * If true, add the corresponding ETag to the body metadata. + * If not, destroy the data stream and throw a {@link NotModifiedHttpError} with the same ETag. * If `conditions` is not defined, nothing will happen. * * This uses the strict conditions check which takes the content type into account; @@ -81,11 +83,14 @@ export async function cloneRepresentation(representation: Representation): Promi * this is why we have to check ETags after content negotiation. * * @param body - The representation to compare the conditions against. + * @param eTagHandler - Used to generate the ETag to return with the 304 response. * @param conditions - The conditions to assert. */ -export function assertReadConditions(body: Representation, conditions?: Conditions): void { +export function assertReadConditions(body: Representation, eTagHandler: ETagHandler, conditions?: Conditions): void { + const eTag = eTagHandler.getETag(body.metadata); if (conditions && !conditions.matchesMetadata(body.metadata, true)) { body.data.destroy(); - throw new NotModifiedHttpError(); + throw new NotModifiedHttpError(eTag); } + body.metadata.set(HH.terms.etag, eTag); } diff --git a/src/util/errors/NotModifiedHttpError.ts b/src/util/errors/NotModifiedHttpError.ts index 9504de3b7..04fea5b6b 100644 --- a/src/util/errors/NotModifiedHttpError.ts +++ b/src/util/errors/NotModifiedHttpError.ts @@ -1,3 +1,4 @@ +import { HH } from '../Vocabularies'; import type { HttpErrorOptions } from './HttpError'; import { generateHttpErrorClass } from './HttpError'; @@ -8,7 +9,8 @@ const BaseHttpError = generateHttpErrorClass(304, 'NotModifiedHttpError'); * An error is thrown when a request conflicts with the current state of the server. */ export class NotModifiedHttpError extends BaseHttpError { - public constructor(message?: string, options?: HttpErrorOptions) { + public constructor(eTag?: string, message?: string, options?: HttpErrorOptions) { super(message, options); + this.metadata.set(HH.terms.etag, eTag); } } diff --git a/test/integration/Conditions.test.ts b/test/integration/Conditions.test.ts index 574681e95..b2ed92f64 100644 --- a/test/integration/Conditions.test.ts +++ b/test/integration/Conditions.test.ts @@ -182,6 +182,7 @@ describe.each(stores)('A server supporting conditions with %s', (name, { storeCo headers: { 'if-none-match': eTag! }, }); expect(response.status).toBe(304); + expect(response.headers.get('etag')).toBe(eTag); // HEAD fails because of header response = await fetch(baseUrl, { @@ -189,6 +190,7 @@ describe.each(stores)('A server supporting conditions with %s', (name, { storeCo headers: { 'if-none-match': eTag! }, }); expect(response.status).toBe(304); + expect(response.headers.get('etag')).toBe(eTag); // GET succeeds if the ETag header doesn't match response = await fetch(baseUrl, { @@ -236,8 +238,10 @@ describe.each(stores)('A server supporting conditions with %s', (name, { storeCo // Both ETags can be used on the same resource response = await fetch(baseUrl, { headers: { 'if-none-match': eTagTurtle!, accept: 'text/turtle' }}); expect(response.status).toBe(304); + expect(response.headers.get('etag')).toBe(eTagTurtle); response = await fetch(baseUrl, { headers: { 'if-none-match': eTagJson!, accept: 'application/ld+json' }}); expect(response.status).toBe(304); + expect(response.headers.get('etag')).toBe(eTagJson); // But not for the other representation response = await fetch(baseUrl, { headers: { 'if-none-match': eTagTurtle!, accept: 'application/ld+json' }}); diff --git a/test/unit/http/ldp/GetOperationHandler.test.ts b/test/unit/http/ldp/GetOperationHandler.test.ts index d226fa578..acae9e58c 100644 --- a/test/unit/http/ldp/GetOperationHandler.test.ts +++ b/test/unit/http/ldp/GetOperationHandler.test.ts @@ -65,7 +65,14 @@ describe('A GetOperationHandler', (): void => { it('returns a 304 if the conditions do not match.', async(): Promise => { conditions.matchesMetadata.mockReturnValue(false); - await expect(handler.handle({ operation })).rejects.toThrow(NotModifiedHttpError); + let error: unknown; + try { + await handler.handle({ operation }); + } catch (err: unknown) { + error = err; + } + expect(NotModifiedHttpError.isInstance(error)).toBe(true); + expect((error as NotModifiedHttpError).metadata.get(HH.terms.etag)?.value).toBe('ETag'); expect(data.destroy).toHaveBeenCalledTimes(1); }); }); diff --git a/test/unit/http/ldp/HeadOperationHandler.test.ts b/test/unit/http/ldp/HeadOperationHandler.test.ts index bf8d6c4dd..9396ff2bd 100644 --- a/test/unit/http/ldp/HeadOperationHandler.test.ts +++ b/test/unit/http/ldp/HeadOperationHandler.test.ts @@ -68,7 +68,14 @@ describe('A HeadOperationHandler', (): void => { it('returns a 304 if the conditions do not match.', async(): Promise => { conditions.matchesMetadata.mockReturnValue(false); - await expect(handler.handle({ operation })).rejects.toThrow(NotModifiedHttpError); - expect(data.destroy).toHaveBeenCalledTimes(2); + let error: unknown; + try { + await handler.handle({ operation }); + } catch (err: unknown) { + error = err; + } + expect(NotModifiedHttpError.isInstance(error)).toBe(true); + expect((error as NotModifiedHttpError).metadata.get(HH.terms.etag)?.value).toBe('ETag'); + expect(data.destroy).toHaveBeenCalledTimes(1); }); }); diff --git a/test/unit/util/ResourceUtil.test.ts b/test/unit/util/ResourceUtil.test.ts index b23d47c0c..e9c6235dc 100644 --- a/test/unit/util/ResourceUtil.test.ts +++ b/test/unit/util/ResourceUtil.test.ts @@ -5,6 +5,7 @@ import { BasicRepresentation } from '../../../src/http/representation/BasicRepre import type { Representation } from '../../../src/http/representation/Representation'; import { RepresentationMetadata } from '../../../src/http/representation/RepresentationMetadata'; import type { Conditions } from '../../../src/storage/conditions/Conditions'; +import type { ETagHandler } from '../../../src/storage/conditions/ETagHandler'; import { NotModifiedHttpError } from '../../../src/util/errors/NotModifiedHttpError'; import type { Guarded } from '../../../src/util/GuardedStream'; import { @@ -13,7 +14,7 @@ import { cloneRepresentation, updateModifiedDate, } from '../../../src/util/ResourceUtil'; -import { CONTENT_TYPE_TERM, DC, SOLID_META, XSD } from '../../../src/util/Vocabularies'; +import { CONTENT_TYPE_TERM, DC, HH, SOLID_META, XSD } from '../../../src/util/Vocabularies'; describe('ResourceUtil', (): void => { let representation: Representation; @@ -71,33 +72,49 @@ describe('ResourceUtil', (): void => { describe('#assertReadConditions', (): void => { let data: jest.Mocked>; + const eTagHandler: ETagHandler = { + getETag: (): string => 'ETag', + matchesETag: jest.fn(), + sameResourceState: jest.fn(), + }; beforeEach(async(): Promise => { data = { destroy: jest.fn(), } as any; representation.data = data; + representation.metadata = new RepresentationMetadata(); }); - it('does nothing if the conditions are undefined.', async(): Promise => { - expect((): any => assertReadConditions(representation)).not.toThrow(); + it('adds the ETag to the representation if the conditions are undefined.', async(): Promise => { + expect((): any => assertReadConditions(representation, eTagHandler)).not.toThrow(); expect(data.destroy).toHaveBeenCalledTimes(0); + expect(representation.metadata.get(HH.terms.etag)?.value).toBe('ETag'); }); - it('does nothing if the conditions match.', async(): Promise => { + it('adds the ETag to the representation i if the conditions match.', async(): Promise => { const conditions: Conditions = { matchesMetadata: (): boolean => true, }; - expect((): any => assertReadConditions(representation, conditions)).not.toThrow(); + expect((): any => assertReadConditions(representation, eTagHandler, conditions)).not.toThrow(); expect(data.destroy).toHaveBeenCalledTimes(0); + expect(representation.metadata.get(HH.terms.etag)?.value).toBe('ETag'); }); it('throws a NotModifiedHttpError if the conditions do not match.', async(): Promise => { const conditions: Conditions = { matchesMetadata: (): boolean => false, }; - expect((): any => assertReadConditions(representation, conditions)).toThrow(NotModifiedHttpError); + let error: unknown; + try { + assertReadConditions(representation, eTagHandler, conditions); + } catch (err: unknown) { + error = err; + } + expect(NotModifiedHttpError.isInstance(error)).toBe(true); + expect((error as NotModifiedHttpError).metadata.get(HH.terms.etag)?.value).toBe('ETag'); expect(data.destroy).toHaveBeenCalledTimes(1); + expect(representation.metadata.get(HH.terms.etag)).toBeUndefined(); }); }); }); diff --git a/test/unit/util/errors/HttpError.test.ts b/test/unit/util/errors/HttpError.test.ts index 2bdfb75f7..197c28917 100644 --- a/test/unit/util/errors/HttpError.test.ts +++ b/test/unit/util/errors/HttpError.test.ts @@ -15,11 +15,10 @@ import { PreconditionFailedHttpError } from '../../../../src/util/errors/Precond import { UnauthorizedHttpError } from '../../../../src/util/errors/UnauthorizedHttpError'; import { UnprocessableEntityHttpError } from '../../../../src/util/errors/UnprocessableEntityHttpError'; import { UnsupportedMediaTypeHttpError } from '../../../../src/util/errors/UnsupportedMediaTypeHttpError'; -import { HTTP, SOLID_ERROR } from '../../../../src/util/Vocabularies'; +import { HH, HTTP, SOLID_ERROR } from '../../../../src/util/Vocabularies'; describe('HttpError', (): void => { const errors: [string, number, HttpErrorClass][] = [ - [ 'NotModifiedHttpError', 304, NotModifiedHttpError ], [ 'BadRequestHttpError', 400, BadRequestHttpError ], [ 'UnauthorizedHttpError', 401, UnauthorizedHttpError ], [ 'ForbiddenHttpError', 403, ForbiddenHttpError ], @@ -106,4 +105,30 @@ describe('HttpError', (): void => { expect(instance.metadata.get(SOLID_ERROR.terms.disallowedMethod)?.value).toBe('GET'); }); }); + + describe('NotModifiedHttpError', (): void => { + const eTag = 'ETAG'; + const options = { + cause: new Error('cause'), + errorCode: 'E1234', + }; + const instance = new NotModifiedHttpError(eTag, 'my message', options); + + it('is valid.', async(): Promise => { + expect(new NotModifiedHttpError().metadata.get(HH.terms.etag)).toBeUndefined(); + expect(NotModifiedHttpError.isInstance(instance)).toBe(true); + expect(NotModifiedHttpError.uri).toEqualRdfTerm(generateHttpErrorUri(304)); + expect(instance.name).toBe('NotModifiedHttpError'); + expect(instance.statusCode).toBe(304); + expect(instance.message).toBe('my message'); + expect(instance.cause).toBe(options.cause); + expect(instance.errorCode).toBe(options.errorCode); + expect(new NotModifiedHttpError().errorCode).toBe(`H${304}`); + + expect(instance.metadata.get(SOLID_ERROR.terms.errorResponse)?.value) + .toBe(`${SOLID_ERROR.namespace}H304`); + expect(instance.metadata.get(HTTP.terms.statusCodeNumber)?.value).toBe('304'); + expect(instance.metadata.get(HH.terms.etag)?.value).toBe(eTag); + }); + }); });