fix: Return 201 when creating new resources

src/http/ldp/PatchOperationHandler.ts

@@ -2,6 +2,7 @@ import { getLoggerFor } from '../../logging/LogUtil';
 import type { ResourceStore } from '../../storage/ResourceStore';
 import { BadRequestHttpError } from '../../util/errors/BadRequestHttpError';
 import { NotImplementedHttpError } from '../../util/errors/NotImplementedHttpError';
+import { CreatedResponseDescription } from '../output/response/CreatedResponseDescription';
 import { ResetResponseDescription } from '../output/response/ResetResponseDescription';
 import type { ResponseDescription } from '../output/response/ResponseDescription';
 import type { Patch } from '../representation/Patch';
@@ -36,7 +37,16 @@ export class PatchOperationHandler extends OperationHandler {
       this.logger.warn('PATCH requests require the Content-Type header to be set');
       throw new BadRequestHttpError('PATCH requests require the Content-Type header to be set');
     }
+    // A more efficient approach would be to have the server return metadata indicating if a resource was new
+    // See https://github.com/solid/community-server/issues/632
+    // RFC7231, §4.3.4: If the target resource does not have a current representation and the
+    //   PUT successfully creates one, then the origin server MUST inform the
+    //   user agent by sending a 201 (Created) response.
+    const exists = await this.store.resourceExists(operation.target, operation.conditions);
     await this.store.modifyResource(operation.target, operation.body as Patch, operation.conditions);
+    if (exists) {
       return new ResetResponseDescription();
     }
+    return new CreatedResponseDescription(operation.target);
+  }
 }

src/http/ldp/PutOperationHandler.ts

@@ -2,6 +2,7 @@ import { getLoggerFor } from '../../logging/LogUtil';
 import type { ResourceStore } from '../../storage/ResourceStore';
 import { BadRequestHttpError } from '../../util/errors/BadRequestHttpError';
 import { NotImplementedHttpError } from '../../util/errors/NotImplementedHttpError';
+import { CreatedResponseDescription } from '../output/response/CreatedResponseDescription';
 import { ResetResponseDescription } from '../output/response/ResetResponseDescription';
 import type { ResponseDescription } from '../output/response/ResponseDescription';
 import type { OperationHandlerInput } from './OperationHandler';
@@ -35,7 +36,13 @@ export class PutOperationHandler extends OperationHandler {
       this.logger.warn('PUT requests require the Content-Type header to be set');
       throw new BadRequestHttpError('PUT requests require the Content-Type header to be set');
     }
+    // A more efficient approach would be to have the server return metadata indicating if a resource was new
+    // See https://github.com/solid/community-server/issues/632
+    const exists = await this.store.resourceExists(operation.target, operation.conditions);
     await this.store.setRepresentation(operation.target, operation.body, operation.conditions);
+    if (exists) {
       return new ResetResponseDescription();
     }
+    return new CreatedResponseDescription(operation.target);
+  }
 }

test/integration/DynamicPods.test.ts

@@ -105,7 +105,8 @@ describe.each(configs)('A dynamic pod server with template config %s', (template
       },
       body: 'this is new data!',
     });
-    expect(res.status).toBe(205);
+    expect(res.status).toBe(201);
+    expect(res.headers.get('location')).toBe(`${podUrl}test`);
 
     res = await fetch(`${podUrl}test`, {
       headers: {

test/integration/LdpHandlerWithAuth.test.ts

@@ -84,7 +84,7 @@ describe.each(stores)('An LDP handler with auth using %s', (name, { storeConfig,
 
     // PUT
     const document = `${baseUrl}test.txt`;
-    await putResource(document, { contentType: 'text/plain', body: 'TESTDATA' });
+    await putResource(document, { contentType: 'text/plain', body: 'TESTDATA', exists: false });
 
     // GET
     const response = await getResource(document);

test/integration/LdpHandlerWithoutAuth.test.ts

@@ -110,7 +110,7 @@ describe.each(stores)('An LDP handler allowing all requests %s', (name, { storeC
     await expect(response.text()).resolves.toBe('TESTFILE0');
 
     // PUT
-    await putResource(documentUrl, { contentType: 'text/plain', body: 'TESTFILE1' });
+    await putResource(documentUrl, { contentType: 'text/plain', body: 'TESTFILE1', exists: true });
 
     // GET
     response = await getResource(documentUrl, {}, { contentType: 'text/plain' });
@@ -253,7 +253,8 @@ describe.each(stores)('An LDP handler allowing all requests %s', (name, { storeC
       },
       body: createReadStream(joinFilePath(__dirname, '../assets/testimage.png')) as any,
     });
-    expect(response.status).toBe(205);
+    expect(response.status).toBe(201);
+    expect(response.headers.get('location')).toBe(documentUrl);
     await expect(response.text()).resolves.toHaveLength(0);
 
     // GET
@@ -291,7 +292,8 @@ describe.each(stores)('An LDP handler allowing all requests %s', (name, { storeC
       headers: { 'content-length': '0', 'content-type': 'text/turtle' },
       body: '',
     });
-    expect(response.status).toBe(205);
+    expect(response.status).toBe(201);
+    expect(response.headers.get('location')).toBe(documentUrl);
 
     // GET
     await getResource(documentUrl);
@@ -312,7 +314,7 @@ describe.each(stores)('An LDP handler allowing all requests %s', (name, { storeC
       'INSERT {<http://test.com/s3> <http://test.com/p3> <http://test.com/o3>}',
       'WHERE {}',
     ].join('\n');
-    await patchResource(documentUrl, query);
+    await patchResource(documentUrl, query, true);
 
     // PATCH using a content-type header with charset
     const query2 = [ 'DELETE { <http://test.com/s2> <http://test.com/p2> <http://test.com/o2> }',
@@ -361,7 +363,7 @@ describe.each(stores)('An LDP handler allowing all requests %s', (name, { storeC
       'INSERT {<http://test.com/s3> <http://test.com/p3> <http://test.com/o3>}',
       'WHERE {}',
     ].join('\n');
-    await patchResource(documentUrl, query);
+    await patchResource(documentUrl, query, true);
 
     // GET
     response = await getResource(documentUrl);

test/integration/RedisResourceLockerIntegration.test.ts

@@ -39,7 +39,7 @@ describeIf('docker', 'A server with a RedisResourceLocker as ResourceLocker', ()
       },
       body: fileData,
     });
-    expect(response.status).toBe(205);
+    expect(response.status).toBe(201);
 
     // Get file
     response = await fetch(fileUrl);
@@ -64,7 +64,7 @@ describeIf('docker', 'A server with a RedisResourceLocker as ResourceLocker', ()
         'content-type': 'text/plain',
       },
     });
-    expect(response.status).toBe(205);
+    expect(response.status).toBe(201);
 
     // GET
     response = await fetch(containerUrl);
@@ -88,7 +88,7 @@ describeIf('docker', 'A server with a RedisResourceLocker as ResourceLocker', ()
       },
       body: fileData,
     });
-    expect(response.status).toBe(205);
+    expect(response.status).toBe(201);
 
     // GET 4 times
     for (let i = 0; i < 4; i++) {

test/integration/RestrictedIdentity.test.ts

@@ -76,7 +76,8 @@ describe('A server with restricted IDP access', (): void => {
       headers: { 'content-type': 'text/turtle' },
       body: restrictedAcl,
     });
-    expect(res.status).toBe(205);
+    expect(res.status).toBe(201);
+    expect(res.headers.get('location')).toBe(`${baseUrl}idp/register/.acl`);
 
     // Registration is now disabled
     res = await fetch(`${baseUrl}idp/register/`);

test/integration/ServerFetch.test.ts

@@ -77,7 +77,8 @@ describe('A Solid server', (): void => {
       },
       body: '<a:b> <a:b> <a:b>.',
     });
-    expect(res.status).toBe(205);
+    expect(res.status).toBe(201);
+    expect(res.headers.get('location')).toBe(url);
   });
 
   it('can PUT to resources.', async(): Promise<void> => {
@@ -89,7 +90,8 @@ describe('A Solid server', (): void => {
       },
       body: '<a:b> <a:b> <a:b>.',
     });
-    expect(res.status).toBe(205);
+    expect(res.status).toBe(201);
+    expect(res.headers.get('location')).toBe(url);
   });
 
   it('can handle PUT errors.', async(): Promise<void> => {

test/integration/Subdomains.test.ts

@@ -79,7 +79,8 @@ describe.each(stores)('A subdomain server with %s', (name, { storeConfig, teardo
         },
         body: 'this is new data!',
       });
-      expect(res.status).toBe(205);
+      expect(res.status).toBe(201);
+      expect(res.headers.get('location')).toBe(`${baseUrl}alice`);
 
       res = await fetch(`${baseUrl}alice`);
       expect(res.status).toBe(200);
@@ -136,7 +137,8 @@ describe.each(stores)('A subdomain server with %s', (name, { storeConfig, teardo
         },
         body: 'this is new data!',
       });
-      expect(res.status).toBe(205);
+      expect(res.status).toBe(201);
+      expect(res.headers.get('location')).toBe(`${podUrl}alice`);
 
       res = await fetch(`${baseUrl}alice`, {
         headers: {

test/unit/http/ldp/PatchOperationHandler.test.ts

@@ -6,17 +6,24 @@ import { BasicConditions } from '../../../../src/storage/BasicConditions';
 import type { ResourceStore } from '../../../../src/storage/ResourceStore';
 import { BadRequestHttpError } from '../../../../src/util/errors/BadRequestHttpError';
 import { NotImplementedHttpError } from '../../../../src/util/errors/NotImplementedHttpError';
+import { SOLID_HTTP } from '../../../../src/util/Vocabularies';
 
 describe('A PatchOperationHandler', (): void => {
   let operation: Operation;
   let body: Representation;
   const conditions = new BasicConditions({});
-  const store = {} as unknown as ResourceStore;
+  let store: jest.Mocked<ResourceStore>;
-  const handler = new PatchOperationHandler(store);
+  let handler: PatchOperationHandler;
   beforeEach(async(): Promise<void> => {
     body = new BasicRepresentation('', 'text/turtle');
     operation = { method: 'PATCH', target: { path: 'http://test.com/foo' }, body, conditions, preferences: {}};
-    store.modifyResource = jest.fn(async(): Promise<any> => undefined);
+
+    store = {
+      resourceExists: jest.fn(),
+      modifyResource: jest.fn(),
+    } as any;
+
+    handler = new PatchOperationHandler(store);
   });
 
   it('only supports PATCH operations.', async(): Promise<void> => {
@@ -30,7 +37,17 @@ describe('A PatchOperationHandler', (): void => {
     await expect(handler.handle({ operation })).rejects.toThrow(BadRequestHttpError);
   });
 
-  it('deletes the resource from the store and returns the correct response.', async(): Promise<void> => {
+  it('creates the representation in the store and returns the correct response.', async(): Promise<void> => {
+    const result = await handler.handle({ operation });
+    expect(store.modifyResource).toHaveBeenCalledTimes(1);
+    expect(store.modifyResource).toHaveBeenLastCalledWith(operation.target, body, conditions);
+    expect(result.statusCode).toBe(201);
+    expect(result.metadata?.get(SOLID_HTTP.location)?.value).toBe(operation.target.path);
+    expect(result.data).toBeUndefined();
+  });
+
+  it('returns the correct response if the resource already exists.', async(): Promise<void> => {
+    store.resourceExists.mockResolvedValueOnce(true);
     const result = await handler.handle({ operation });
     expect(store.modifyResource).toHaveBeenCalledTimes(1);
     expect(store.modifyResource).toHaveBeenLastCalledWith(operation.target, body, conditions);

test/unit/http/ldp/PutOperationHandler.test.ts

@@ -6,18 +6,23 @@ import { BasicConditions } from '../../../../src/storage/BasicConditions';
 import type { ResourceStore } from '../../../../src/storage/ResourceStore';
 import { BadRequestHttpError } from '../../../../src/util/errors/BadRequestHttpError';
 import { NotImplementedHttpError } from '../../../../src/util/errors/NotImplementedHttpError';
+import { SOLID_HTTP } from '../../../../src/util/Vocabularies';
 
 describe('A PutOperationHandler', (): void => {
   let operation: Operation;
   let body: Representation;
   const conditions = new BasicConditions({});
-  const store = {} as unknown as ResourceStore;
+  let store: jest.Mocked<ResourceStore>;
-  const handler = new PutOperationHandler(store);
+  let handler: PutOperationHandler;
   beforeEach(async(): Promise<void> => {
     body = new BasicRepresentation('', 'text/turtle');
     operation = { method: 'PUT', target: { path: 'http://test.com/foo' }, body, conditions, preferences: {}};
-    // eslint-disable-next-line @typescript-eslint/no-empty-function
+    store = {
-    store.setRepresentation = jest.fn(async(): Promise<any> => {});
+      resourceExists: jest.fn(),
+      setRepresentation: jest.fn(),
+    } as any;
+
+    handler = new PutOperationHandler(store);
   });
 
   it('only supports PUT operations.', async(): Promise<void> => {
@@ -31,7 +36,17 @@ describe('A PutOperationHandler', (): void => {
     await expect(handler.handle({ operation })).rejects.toThrow(BadRequestHttpError);
   });
 
-  it('sets the representation in the store and returns the correct response.', async(): Promise<void> => {
+  it('creates the representation in the store and returns the correct response.', async(): Promise<void> => {
+    const result = await handler.handle({ operation });
+    expect(store.setRepresentation).toHaveBeenCalledTimes(1);
+    expect(store.setRepresentation).toHaveBeenLastCalledWith(operation.target, body, conditions);
+    expect(result.statusCode).toBe(201);
+    expect(result.metadata?.get(SOLID_HTTP.location)?.value).toBe(operation.target.path);
+    expect(result.data).toBeUndefined();
+  });
+
+  it('returns the correct response if the resource already exists.', async(): Promise<void> => {
+    store.resourceExists.mockResolvedValueOnce(true);
     const result = await handler.handle({ operation });
     expect(store.setRepresentation).toHaveBeenCalledTimes(1);
     expect(store.setRepresentation).toHaveBeenLastCalledWith(operation.target, body, conditions);

test/util/FetchUtil.ts

@@ -36,7 +36,7 @@ export async function getResource(url: string,
 /**
  * This is specifically for PUT requests which are expected to succeed.
  */
-export async function putResource(url: string, options: { contentType: string; body?: string }):
+export async function putResource(url: string, options: { contentType: string; body?: string; exists?: boolean }):
 Promise<Response> {
   const init: RequestInit = {
     method: 'PUT',
@@ -47,7 +47,10 @@ Promise<Response> {
     (init.headers as Record<string, string>).link = '<http://www.w3.org/ns/ldp#Container>; rel="type"';
   }
   const response = await fetch(url, init);
-  expect(response.status).toBe(205);
+  expect(response.status).toBe(options.exists ? 205 : 201);
+  if (!options.exists) {
+    expect(response.headers.get('location')).toBe(url);
+  }
   await expect(response.text()).resolves.toHaveLength(0);
   return response;
 }
@@ -84,7 +87,7 @@ export async function postResource(container: string, options: CreateOptions): P
 /**
  * This is specifically for PATCH requests which are expected to succeed.
  */
-export async function patchResource(url: string, query: string): Promise<Response> {
+export async function patchResource(url: string, query: string, exists?: boolean): Promise<Response> {
   const response = await fetch(url, {
     method: 'PATCH',
     headers: {
@@ -93,7 +96,10 @@ export async function patchResource(url: string, query: string): Promise<Respons
     body: query,
   });
   await expect(response.text()).resolves.toHaveLength(0);
-  expect(response.status).toBe(205);
+  expect(response.status).toBe(exists ? 205 : 201);
+  if (!exists) {
+    expect(response.headers.get('location')).toBe(url);
+  }
 
   return response;
 }