diff --git a/src/http/ldp/PatchOperationHandler.ts b/src/http/ldp/PatchOperationHandler.ts index f5393c60d..e17a3da6d 100644 --- a/src/http/ldp/PatchOperationHandler.ts +++ b/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); - return new ResetResponseDescription(); + if (exists) { + return new ResetResponseDescription(); + } + return new CreatedResponseDescription(operation.target); } } diff --git a/src/http/ldp/PutOperationHandler.ts b/src/http/ldp/PutOperationHandler.ts index 086da2985..deb25c221 100644 --- a/src/http/ldp/PutOperationHandler.ts +++ b/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); - return new ResetResponseDescription(); + if (exists) { + return new ResetResponseDescription(); + } + return new CreatedResponseDescription(operation.target); } } diff --git a/test/integration/DynamicPods.test.ts b/test/integration/DynamicPods.test.ts index 7aa22ec26..2274c6f52 100644 --- a/test/integration/DynamicPods.test.ts +++ b/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: { diff --git a/test/integration/LdpHandlerWithAuth.test.ts b/test/integration/LdpHandlerWithAuth.test.ts index 7193c1aac..adb6fad59 100644 --- a/test/integration/LdpHandlerWithAuth.test.ts +++ b/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); diff --git a/test/integration/LdpHandlerWithoutAuth.test.ts b/test/integration/LdpHandlerWithoutAuth.test.ts index a2d3603a7..96dc905ec 100644 --- a/test/integration/LdpHandlerWithoutAuth.test.ts +++ b/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 { }', 'WHERE {}', ].join('\n'); - await patchResource(documentUrl, query); + await patchResource(documentUrl, query, true); // PATCH using a content-type header with charset const query2 = [ 'DELETE { }', @@ -361,7 +363,7 @@ describe.each(stores)('An LDP handler allowing all requests %s', (name, { storeC 'INSERT { }', 'WHERE {}', ].join('\n'); - await patchResource(documentUrl, query); + await patchResource(documentUrl, query, true); // GET response = await getResource(documentUrl); diff --git a/test/integration/RedisResourceLockerIntegration.test.ts b/test/integration/RedisResourceLockerIntegration.test.ts index ebd2719dd..a0e13096d 100644 --- a/test/integration/RedisResourceLockerIntegration.test.ts +++ b/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++) { diff --git a/test/integration/RestrictedIdentity.test.ts b/test/integration/RestrictedIdentity.test.ts index 3d726a1aa..9a0db6593 100644 --- a/test/integration/RestrictedIdentity.test.ts +++ b/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/`); diff --git a/test/integration/ServerFetch.test.ts b/test/integration/ServerFetch.test.ts index d890888c2..5118be9c8 100644 --- a/test/integration/ServerFetch.test.ts +++ b/test/integration/ServerFetch.test.ts @@ -77,7 +77,8 @@ describe('A Solid server', (): void => { }, body: ' .', }); - expect(res.status).toBe(205); + expect(res.status).toBe(201); + expect(res.headers.get('location')).toBe(url); }); it('can PUT to resources.', async(): Promise => { @@ -89,7 +90,8 @@ describe('A Solid server', (): void => { }, body: ' .', }); - expect(res.status).toBe(205); + expect(res.status).toBe(201); + expect(res.headers.get('location')).toBe(url); }); it('can handle PUT errors.', async(): Promise => { diff --git a/test/integration/Subdomains.test.ts b/test/integration/Subdomains.test.ts index f50e7951b..9d25b273f 100644 --- a/test/integration/Subdomains.test.ts +++ b/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: { diff --git a/test/unit/http/ldp/PatchOperationHandler.test.ts b/test/unit/http/ldp/PatchOperationHandler.test.ts index d782c5d70..4bbd6b610 100644 --- a/test/unit/http/ldp/PatchOperationHandler.test.ts +++ b/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; - const handler = new PatchOperationHandler(store); + let store: jest.Mocked; + let handler: PatchOperationHandler; beforeEach(async(): Promise => { body = new BasicRepresentation('', 'text/turtle'); operation = { method: 'PATCH', target: { path: 'http://test.com/foo' }, body, conditions, preferences: {}}; - store.modifyResource = jest.fn(async(): Promise => undefined); + + store = { + resourceExists: jest.fn(), + modifyResource: jest.fn(), + } as any; + + handler = new PatchOperationHandler(store); }); it('only supports PATCH operations.', async(): Promise => { @@ -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 => { + it('creates the representation in the store and returns the correct response.', async(): Promise => { + 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 => { + store.resourceExists.mockResolvedValueOnce(true); const result = await handler.handle({ operation }); expect(store.modifyResource).toHaveBeenCalledTimes(1); expect(store.modifyResource).toHaveBeenLastCalledWith(operation.target, body, conditions); diff --git a/test/unit/http/ldp/PutOperationHandler.test.ts b/test/unit/http/ldp/PutOperationHandler.test.ts index 6f9c81221..09f70a085 100644 --- a/test/unit/http/ldp/PutOperationHandler.test.ts +++ b/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; - const handler = new PutOperationHandler(store); + let store: jest.Mocked; + let handler: PutOperationHandler; beforeEach(async(): Promise => { 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.setRepresentation = jest.fn(async(): Promise => {}); + store = { + resourceExists: jest.fn(), + setRepresentation: jest.fn(), + } as any; + + handler = new PutOperationHandler(store); }); it('only supports PUT operations.', async(): Promise => { @@ -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 => { + it('creates the representation in the store and returns the correct response.', async(): Promise => { + 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 => { + store.resourceExists.mockResolvedValueOnce(true); const result = await handler.handle({ operation }); expect(store.setRepresentation).toHaveBeenCalledTimes(1); expect(store.setRepresentation).toHaveBeenLastCalledWith(operation.target, body, conditions); diff --git a/test/util/FetchUtil.ts b/test/util/FetchUtil.ts index 5b156a675..31715c187 100644 --- a/test/util/FetchUtil.ts +++ b/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 { const init: RequestInit = { method: 'PUT', @@ -47,7 +47,10 @@ Promise { (init.headers as Record).link = '; 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 { +export async function patchResource(url: string, query: string, exists?: boolean): Promise { const response = await fetch(url, { method: 'PATCH', headers: { @@ -93,7 +96,10 @@ export async function patchResource(url: string, query: string): Promise