fix: Return ETag in 304 responses

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);
+    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);
 
     return new OkResponseDescription(body.metadata, body.data);
   }

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<ResponseDescription> {
     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
+    // Close the Readable as we will not return it.
-    const etag = this.eTagHandler.getETag(body.metadata);
+    body.data.destroy();
-    body.metadata.set(HH.terms.etag, etag);
 
     return new OkResponseDescription(body.metadata);
   }

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);
 }

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);
   }
 }

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' }});

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<void> => {
     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);
   });
 });

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<void> => {
     conditions.matchesMetadata.mockReturnValue(false);
-    await expect(handler.handle({ operation })).rejects.toThrow(NotModifiedHttpError);
+    let error: unknown;
-    expect(data.destroy).toHaveBeenCalledTimes(2);
+    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);
   });
 });

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<Guarded<Readable>>;
+    const eTagHandler: ETagHandler = {
+      getETag: (): string => 'ETag',
+      matchesETag: jest.fn(),
+      sameResourceState: jest.fn(),
+    };
 
     beforeEach(async(): Promise<void> => {
       data = {
         destroy: jest.fn(),
       } as any;
       representation.data = data;
+      representation.metadata = new RepresentationMetadata();
     });
 
-    it('does nothing if the conditions are undefined.', async(): Promise<void> => {
+    it('adds the ETag to the representation if the conditions are undefined.', async(): Promise<void> => {
-      expect((): any => assertReadConditions(representation)).not.toThrow();
+      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<void> => {
+    it('adds the ETag to the representation i if the conditions match.', async(): Promise<void> => {
       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<void> => {
       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();
     });
   });
 });

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<void> => {
+      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);
+    });
+  });
 });