Repository: solid/community-server (ts)

Issue:
Delete metadata when writing to an existing resource if there is no new metadata
When overwriting a resource, its corresponding metadata should be deleted. For the file system this does not happen in the case the new resource does not have metadata to write. Should also check other backends.

Pull request:
fix: Remove metadata file if no new metadata is stored
Resolves #265 

Patch:
diff --git a/src/storage/accessors/FileDataAccessor.ts b/src/storage/accessors/FileDataAccessor.ts
index c2c2a2ea8..e0f20f2fc 100644
--- a/src/storage/accessors/FileDataAccessor.ts
+++ b/src/storage/accessors/FileDataAccessor.ts
@@ -214,12 +214,28 @@ export class FileDataAccessor implements DataAccessor {
     metadata.removeAll(RDF.type);
     metadata.removeAll(CONTENT_TYPE);
     const quads = metadata.quads();
+    const metadataPath = await this.getMetadataPath(link.identifier);
+    let wroteMetadata: boolean;
+
+    // Write metadata to file if there are quads remaining
     if (quads.length > 0) {
       const serializedMetadata = this.metadataController.serializeQuads(quads);
-      await this.writeDataFile(await this.getMetadataPath(link.identifier), serializedMetadata);
-      return true;
+      await this.writeDataFile(metadataPath, serializedMetadata);
+      wroteMetadata = true;
+
+    // Delete (potentially) existing metadata file if no metadata needs to be stored
+    } else {
+      try {
+        await fsPromises.unlink(metadataPath);
+      } catch (error: unknown) {
+        // Metadata file doesn't exist so nothing needs to be removed
+        if (!isSystemError(error) || error.code !== 'ENOENT') {
+          throw error;
+        }
+      }
+      wroteMetadata = false;
     }
-    return false;
+    return wroteMetadata;
   }
 
   /**


Test patch:
diff --git a/test/unit/storage/accessors/FileDataAccessor.test.ts b/test/unit/storage/accessors/FileDataAccessor.test.ts
index 5e19d7ef6..eaf545e31 100644
--- a/test/unit/storage/accessors/FileDataAccessor.test.ts
+++ b/test/unit/storage/accessors/FileDataAccessor.test.ts
@@ -7,6 +7,7 @@ import { ExtensionBasedMapper } from '../../../../src/storage/ExtensionBasedMapp
 import { APPLICATION_OCTET_STREAM } from '../../../../src/util/ContentTypes';
 import { ConflictHttpError } from '../../../../src/util/errors/ConflictHttpError';
 import { NotFoundHttpError } from '../../../../src/util/errors/NotFoundHttpError';
+import type { SystemError } from '../../../../src/util/errors/SystemError';
 import { UnsupportedMediaTypeHttpError } from '../../../../src/util/errors/UnsupportedMediaTypeHttpError';
 import { MetadataController } from '../../../../src/util/MetadataController';
 import { CONTENT_TYPE, DCTERMS, LDP, POSIX, RDF, XSD } from '../../../../src/util/UriConstants';
@@ -174,6 +175,24 @@ describe('A FileDataAccessor', (): void => {
       expect(cache.data['resource.meta']).toBeUndefined();
     });
 
+    it('deletes existing metadata if nothing new needs to be stored.', async(): Promise<void> => {
+      cache.data = { resource: 'data', 'resource.meta': 'metadata!' };
+      const data = streamifyArray([ 'data' ]);
+      await expect(accessor.writeDocument({ path: `${base}resource` }, data, metadata)).resolves.toBeUndefined();
+      expect(cache.data.resource).toBe('data');
+      expect(cache.data['resource.meta']).toBeUndefined();
+    });
+
+    it('errors if there is a problem deleting the old metadata file.', async(): Promise<void> => {
+      cache.data = { resource: 'data', 'resource.meta': 'metadata!' };
+      jest.requireMock('fs').promises.unlink = (): any => {
+        throw new Error('error');
+      };
+      const data = streamifyArray([ 'data' ]);
+      await expect(accessor.writeDocument({ path: `${base}resource` }, data, metadata))
+        .rejects.toThrow(new Error('error'));
+    });
+
     it('throws if something went wrong writing a file.', async(): Promise<void> => {
       const data = streamifyArray([ 'data' ]);
       data.read = (): any => {
@@ -212,10 +231,13 @@ describe('A FileDataAccessor', (): void => {
     it('throws an error if there is an issue deleting the original file.', async(): Promise<void> => {
       cache.data = { 'resource$.ttl': '<this> <is> <data>.' };
       jest.requireMock('fs').promises.unlink = (): any => {
-        throw new Error('error');
+        const error = new Error('error') as SystemError;
+        error.code = 'ENOENT';
+        error.syscall = 'unlink';
+        throw error;
       };
 
-      // `unlink` should not be called if the content-type does not change
+      // `unlink` throwing ENOENT should not be an issue if the content-type does not change
       metadata.contentType = 'text/turtle';
       await expect(accessor.writeDocument({ path: `${base}resource` }, streamifyArray([ 'text' ]), metadata))
         .resolves.toBeUndefined();