fix: zip tree container adding duplicate entries (#158)

Author: Bryan Powell

src/metadata-registry/treeContainers.ts

@@ -114,11 +114,13 @@ export class ZipTreeContainer extends BaseTreeContainer {
   }
 
   private populate(directory: unzipper.CentralDirectory): void {
-    for (const { path, stream, buffer } of directory.files) {
-      // normalize path to use OS separator since zip entries always use forward slash
-      const entry = { path: normalize(path), stream, buffer };
-      this.tree.set(entry.path, entry);
-      this.ensureDirPathExists(entry);
+    for (const { path, type, stream, buffer } of directory.files) {
+      if (type === 'File') {
+        // normalize path to use OS separator since zip entries always use forward slash
+        const entry = { path: normalize(path), stream, buffer };
+        this.tree.set(entry.path, entry);
+        this.ensureDirPathExists(entry);
+      }
     }
   }
 

test/metadata-registry/treeContainers.test.ts

@@ -13,7 +13,7 @@ import {
 import { expect, assert } from 'chai';
 import { createSandbox } from 'sinon';
 import * as fs from 'fs';
-import { join } from 'path';
+import { join, normalize } from 'path';
 import { LibraryError } from '../../src/errors';
 import { nls } from '../../src/i18n';
 import { VirtualDirectory } from '../../src';
@@ -110,6 +110,8 @@ describe('Tree Containers', () => {
     let tree: ZipTreeContainer;
     let zipBuffer: Buffer;
 
+    const filesRoot = join('.', 'main', 'default');
+
     before(async () => {
       const pipeline = promisify(cbPipeline);
       const archive = createArchive('zip', { zlib: { level: 3 } });
@@ -120,9 +122,13 @@ describe('Tree Containers', () => {
         cb();
       };
       pipeline(archive, bufferWritable);
-      archive.append('test text', { name: 'test.txt' });
-      archive.append('test text 2', { name: 'files/test2.txt' });
-      archive.append('test text 3', { name: 'files/test3.txt' });
+
+      archive.append(null, { name: 'main/' });
+      archive.append(null, { name: 'main/default/' });
+      archive.append('test text', { name: 'main/default/test.txt' });
+      archive.append('test text 2', { name: 'main/default/test2.txt' });
+      archive.append('test text 3', { name: 'main/default/morefiles/test3.txt' });
+
       await archive.finalize();
 
       zipBuffer = Buffer.concat(buffers);
@@ -131,35 +137,34 @@ describe('Tree Containers', () => {
 
     describe('exists', () => {
       it('should return true for file that exists', () => {
-        const path = join('.', 'test.txt');
+        const path = join(filesRoot, 'test.txt');
         expect(tree.exists(path)).to.be.true;
       });
 
       it('should return true for directory that exists', () => {
-        const path = join('.', 'files');
+        const path = join(filesRoot, 'morefiles');
         expect(tree.exists(path)).to.be.true;
       });
 
       it('should return false for file that does not exist', () => {
-        const path = join('.', 'files', 'test4.txt');
+        const path = join(filesRoot, 'test4.txt');
         expect(tree.exists(path)).to.be.false;
       });
 
       it('should return false for directory that does not exist', () => {
-        const path = join('.', 'otherfiles');
+        const path = join('.', 'dne');
         expect(tree.exists(path)).to.be.false;
       });
     });
 
     describe('isDirectory', () => {
       it('should return false for isDirectory', () => {
-        const path = join('.', 'test.txt');
+        const path = join(filesRoot, 'test.txt');
         expect(tree.isDirectory(path)).to.be.false;
       });
 
       it('should return true for isDirectory', () => {
-        const path = join('.', 'files');
-        expect(tree.isDirectory(path)).to.be.true;
+        expect(tree.isDirectory(filesRoot)).to.be.true;
       });
 
       it('should throw an error if path does not exist', () => {
@@ -173,12 +178,21 @@ describe('Tree Containers', () => {
     });
 
     describe('readDirectory', () => {
-      it('should return directory entries for readDirectory', () => {
-        expect(tree.readDirectory('.')).to.deep.equal(['test.txt', 'files']);
+      it('should return correct directory entries for directory with files and directories', () => {
+        expect(tree.readDirectory(filesRoot)).to.deep.equal(['test.txt', 'test2.txt', 'morefiles']);
+      });
+
+      it('should return correct directory entries for directory only files', () => {
+        const path = join(filesRoot, 'morefiles');
+        expect(tree.readDirectory(path)).to.deep.equal(['test3.txt']);
+      });
+
+      it('should return correct directory entries for directory with only directories', () => {
+        expect(tree.readDirectory('.')).to.deep.equal(['main']);
       });
 
       it('should throw an error if path is not a directory', () => {
-        const path = join('.', 'files', 'test2.txt');
+        const path = join(filesRoot, 'test2.txt');
         assert.throws(
           () => tree.readDirectory(path),
           LibraryError,
@@ -189,35 +203,33 @@ describe('Tree Containers', () => {
 
     describe('readFile', () => {
       it('should read contents of zip entry into buffer', async () => {
-        const path = join('.', 'test.txt');
+        const path = join(filesRoot, 'test.txt');
         const contents = (await tree.readFile(path)).toString();
         expect(contents).to.equal('test text');
       });
 
       it('should throw an error if path is to directory', () => {
-        const path = join('.', 'files');
         assert.throws(
-          () => tree.readFile(path),
+          () => tree.readFile(filesRoot),
           LibraryError,
-          nls.localize('error_expected_file_path', path)
+          nls.localize('error_expected_file_path', filesRoot)
         );
       });
     });
 
     describe('stream', () => {
       it('should return a readable stream', async () => {
-        const path = join('.', 'test.txt');
+        const path = join(filesRoot, 'test.txt');
         const zipDir = await unzipper.Open.buffer(zipBuffer);
-        const expectedStream = zipDir.files.find((f) => f.path === path)?.stream();
+        const expectedStream = zipDir.files.find((f) => normalize(f.path) === path)?.stream();
         const actual = tree.stream(path);
         expect(actual instanceof Readable).to.be.true;
         expect((actual as unzipper.Entry).path).to.equal(expectedStream.path);
       });
 
       it('should throw an error if given path is to a directory', () => {
-        const path = join('.', 'files');
         assert.throws(
-          () => tree.stream(path),
+          () => tree.stream(filesRoot),
           LibraryError,
           nls.localize('error_no_directory_stream', tree.constructor.name)
         );