test: Increase coverage publish api tests (#1056)

* refactor: ES6 sugar

* refactor: improves code in publish

* refactor: add tests for upload tarball and add a version flow

* refactor: unpublish endpoint tests

* refactor: publish endpoint test

* docs: adds code blocks to publish functionality

* refactor: rename tests file name

* refactor: improves logic for npm star command

* refactor: replaces assert equal with strictEqual

src/api/endpoint/api/publish.js

@@ -7,57 +7,84 @@ import _ from 'lodash';
 import Path from 'path';
 import mime from 'mime';
 
-import { API_MESSAGE, HEADERS, DIST_TAGS, API_ERROR } from '../../../lib/constants';
+import { API_MESSAGE, HEADERS, DIST_TAGS, API_ERROR, HTTP_STATUS } from '../../../lib/constants';
 import { validateMetadata, isObject, ErrorCode } from '../../../lib/utils';
 import { media, expectJson, allow } from '../../middleware';
 import { notify } from '../../../lib/notify';
 
 import type { Router } from 'express';
-import type { Config, Callback } from '@verdaccio/types';
+import type { Config, Callback, MergeTags, Version } from '@verdaccio/types';
 import type { IAuth, $ResponseExtend, $RequestExtend, $NextFunctionVer, IStorageHandler } from '../../../../types';
 import logger from '../../../lib/logger';
 
-export default function(router: Router, auth: IAuth, storage: IStorageHandler, config: Config) {
+export default function publish(router: Router, auth: IAuth, storage: IStorageHandler, config: Config) {
   const can = allow(auth);
 
   // publishing a package
-  router.put('/:package/:_rev?/:revision?', can('publish'), media(mime.getType('json')), expectJson, function(
-    req: $RequestExtend,
-    res: $ResponseExtend,
-    next: $NextFunctionVer
-  ) {
-    const name = req.params.package;
-    let metadata;
-    const create_tarball = function(filename: string, data, cb: Callback) {
-      let stream = storage.addTarball(name, filename);
+  router.put('/:package/:_rev?/:revision?', can('publish'), media(mime.getType('json')), expectJson, publishPackage(storage, config));
+
+  // un-publishing an entire package
+  router.delete('/:package/-rev/*', can('publish'), unPublishPackage(storage));
+
+  // removing a tarball
+  router.delete('/:package/-/:filename/-rev/:revision', can('publish'), removeTarball(storage));
+
+  // uploading package tarball
+  router.put('/:package/-/:filename/*', can('publish'), media(HEADERS.OCTET_STREAM), uploadPackageTarball(storage));
+
+  // adding a version
+  router.put('/:package/:version/-tag/:tag', can('publish'), media(mime.getType('json')), expectJson, addVersion(storage));
+}
+
+/**
+ * Publish a package
+ */
+export function publishPackage(storage: IStorageHandler, config: Config) {
+  return function(req: $RequestExtend, res: $ResponseExtend, next: $NextFunctionVer) {
+    const packageName = req.params.package;
+    /**
+     * Write tarball of stream data from package clients.
+     */
+    const createTarball = function(filename: string, data, cb: Callback) {
+      let stream = storage.addTarball(packageName, filename);
       stream.on('error', function(err) {
         cb(err);
       });
       stream.on('success', function() {
         cb();
       });
-
       // this is dumb and memory-consuming, but what choices do we have?
       // flow: we need first refactor this file before decides which type use here
       stream.end(new Buffer(data.data, 'base64'));
       stream.done();
     };
 
-    const create_version = function(version, data, cb) {
-      storage.addVersion(name, version, data, null, cb);
+    /**
+     * Add new package version in storage
+     */
+    const createVersion = function(version: string, metadata: Version, cb: Callback) {
+      storage.addVersion(packageName, version, metadata, null, cb);
     };
 
-    const add_tags = function(tags, cb) {
-      storage.mergeTags(name, tags, cb);
+    /**
+     * Add new tags in storage
+     */
+    const addTags = function(tags: MergeTags, cb: Callback) {
+      storage.mergeTags(packageName, tags, cb);
     };
 
-    const after_change = function(err, ok_message) {
-      // old npm behaviour
-      if (_.isNil(metadata._attachments)) {
-        if (err) return next(err);
-        res.status(201);
+    const afterChange = function(error, okMessage, metadata) {
+      let metadataCopy = { ...metadata };
+      const { _attachments, versions } = metadataCopy;
+
+      // old npm behavior, if there is no attachments
+      if (_.isNil(_attachments)) {
+        if (error) {
+          return next(error);
+        }
+        res.status(HTTP_STATUS.CREATED);
         return next({
-          ok: ok_message,
+          ok: okMessage,
           success: true,
         });
       }
@@ -65,106 +92,129 @@ export default function(router: Router, auth: IAuth, storage: IStorageHandler, c
       // npm-registry-client 0.3+ embeds tarball into the json upload
       // https://github.com/isaacs/npm-registry-client/commit/e9fbeb8b67f249394f735c74ef11fe4720d46ca0
       // issue https://github.com/rlidwka/sinopia/issues/31, dealing with it here:
-
-      if (
-        typeof metadata._attachments !== 'object' ||
-        Object.keys(metadata._attachments).length !== 1 ||
-        typeof metadata.versions !== 'object' ||
-        Object.keys(metadata.versions).length !== 1
-      ) {
+      if (isObject(_attachments) === false || Object.keys(_attachments).length !== 1 || isObject(versions) === false || Object.keys(versions).length !== 1) {
         // npm is doing something strange again
         // if this happens in normal circumstances, report it as a bug
         return next(ErrorCode.getBadRequest('unsupported registry call'));
       }
 
-      if (err && err.status != 409) {
-        return next(err);
+      if (error && error.status !== HTTP_STATUS.CONFLICT) {
+        return next(error);
       }
 
       // at this point document is either created or existed before
-      const t1 = Object.keys(metadata._attachments)[0];
-      create_tarball(Path.basename(t1), metadata._attachments[t1], function(err) {
-        if (err) {
-          return next(err);
+      const firstAttachmentKey = Object.keys(_attachments)[0];
+
+      createTarball(Path.basename(firstAttachmentKey), _attachments[firstAttachmentKey], function(error) {
+        if (error) {
+          return next(error);
         }
 
-        const versionToPublish = Object.keys(metadata.versions)[0];
-        metadata.versions[versionToPublish].readme = _.isNil(metadata.readme) === false ? String(metadata.readme) : '';
-        create_version(versionToPublish, metadata.versions[versionToPublish], function(err) {
-          if (err) {
-            return next(err);
+        const versionToPublish = Object.keys(versions)[0];
+
+        versions[versionToPublish].readme = _.isNil(metadataCopy.readme) === false ? String(metadataCopy.readme) : '';
+
+        createVersion(versionToPublish, versions[versionToPublish], function(error) {
+          if (error) {
+            return next(error);
           }
 
-          add_tags(metadata[DIST_TAGS], async function(err) {
-            if (err) {
-              return next(err);
+          addTags(metadataCopy[DIST_TAGS], async function(error) {
+            if (error) {
+              return next(error);
             }
 
             try {
-              await notify(metadata, config, req.remote_user, `${metadata.name}@${versionToPublish}`);
-            } catch (err) {
-              logger.logger.error({ err }, 'notify batch service has failed: @{err}');
+              await notify(metadataCopy, config, req.remote_user, `${metadataCopy.name}@${versionToPublish}`);
+            } catch (error) {
+              logger.logger.error({ error }, 'notify batch service has failed: @{error}');
             }
 
-            res.status(201);
-            return next({ ok: ok_message, success: true });
+            res.status(HTTP_STATUS.CREATED);
+            return next({ ok: okMessage, success: true });
           });
         });
       });
     };
 
-    if (Object.keys(req.body).length === 1 && isObject(req.body.users)) {
-      // 501 status is more meaningful, but npm doesn't show error message for 5xx
-      return next(ErrorCode.getNotFound('npm star|unstar calls are not implemented'));
+    if (Object.prototype.hasOwnProperty.call(req.body, '_rev') && isObject(req.body.users)) {
+      return next(ErrorCode.getNotFound('npm star| un-star calls are not implemented'));
     }
 
     try {
-      metadata = validateMetadata(req.body, name);
-    } catch (err) {
+      const metadata = validateMetadata(req.body, packageName);
+      if (req.params._rev) {
+        storage.changePackage(packageName, metadata, req.params.revision, function(error) {
+          afterChange(error, API_MESSAGE.PKG_CHANGED, metadata);
+        });
+      } else {
+        storage.addPackage(packageName, metadata, function(error) {
+          afterChange(error, API_MESSAGE.PKG_CREATED, metadata);
+        });
+      }
+    } catch (error) {
       return next(ErrorCode.getBadData(API_ERROR.BAD_PACKAGE_DATA));
     }
+  };
+}
 
-    if (req.params._rev) {
-      storage.changePackage(name, metadata, req.params.revision, function(err) {
-        after_change(err, API_MESSAGE.PKG_CHANGED);
-      });
-    } else {
-      storage.addPackage(name, metadata, function(err) {
-        after_change(err, API_MESSAGE.PKG_CREATED);
-      });
-    }
-  });
-
-  // unpublishing an entire package
-  router.delete('/:package/-rev/*', can('publish'), function(req: $RequestExtend, res: $ResponseExtend, next: $NextFunctionVer) {
+/**
+ * un-publish a package
+ */
+export function unPublishPackage(storage: IStorageHandler) {
+  return function(req: $RequestExtend, res: $ResponseExtend, next: $NextFunctionVer) {
     storage.removePackage(req.params.package, function(err) {
       if (err) {
         return next(err);
       }
-      res.status(201);
+      res.status(HTTP_STATUS.CREATED);
       return next({ ok: API_MESSAGE.PKG_REMOVED });
     });
-  });
+  };
+}
 
-  // removing a tarball
-  router.delete('/:package/-/:filename/-rev/:revision', can('publish'), function(req: $RequestExtend, res: $ResponseExtend, next: $NextFunctionVer) {
+/**
+ * Delete tarball
+ */
+export function removeTarball(storage: IStorageHandler) {
+  return function(req: $RequestExtend, res: $ResponseExtend, next: $NextFunctionVer) {
     storage.removeTarball(req.params.package, req.params.filename, req.params.revision, function(err) {
       if (err) {
         return next(err);
       }
-      res.status(201);
+      res.status(HTTP_STATUS.CREATED);
       return next({ ok: API_MESSAGE.TARBALL_REMOVED });
     });
-  });
+  };
+}
+/**
+ * Adds a new version
+ */
+export function addVersion(storage: IStorageHandler) {
+  return function(req: $RequestExtend, res: $ResponseExtend, next: $NextFunctionVer) {
+    const { version, tag } = req.params;
+    const packageName = req.params.package;
 
-  // uploading package tarball
-  router.put('/:package/-/:filename/*', can('publish'), media(HEADERS.OCTET_STREAM), function(
-    req: $RequestExtend,
-    res: $ResponseExtend,
-    next: $NextFunctionVer
-  ) {
-    const name = req.params.package;
-    const stream = storage.addTarball(name, req.params.filename);
+    storage.addVersion(packageName, version, req.body, tag, function(error) {
+      if (error) {
+        return next(error);
+      }
+
+      res.status(HTTP_STATUS.CREATED);
+      return next({
+        ok: API_MESSAGE.PKG_PUBLISHED,
+      });
+    });
+  };
+}
+
+/**
+ * uploadPackageTarball
+ */
+export function uploadPackageTarball(storage: IStorageHandler) {
+  return function(req: $RequestExtend, res: $ResponseExtend, next: $NextFunctionVer) {
+    const packageName = req.params.package;
+    const stream = storage.addTarball(packageName, req.params.filename);
     req.pipe(stream);
 
     // checking if end event came before closing
@@ -185,31 +235,10 @@ export default function(router: Router, auth: IAuth, storage: IStorageHandler, c
     });
 
     stream.on('success', function() {
-      res.status(201);
+      res.status(HTTP_STATUS.CREATED);
       return next({
         ok: API_MESSAGE.TARBALL_UPLOADED,
       });
     });
-  });
-
-  // adding a version
-  router.put('/:package/:version/-tag/:tag', can('publish'), media(mime.getType('json')), expectJson, function(
-    req: $RequestExtend,
-    res: $ResponseExtend,
-    next: $NextFunctionVer
-  ) {
-    const { version, tag } = req.params;
-    const name = req.params.package;
-
-    storage.addVersion(name, version, req.body, tag, function(err) {
-      if (err) {
-        return next(err);
-      }
-
-      res.status(201);
-      return next({
-        ok: API_MESSAGE.PKG_PUBLISHED,
-      });
-    });
-  });
+  };
 }

src/api/index.js

@@ -108,7 +108,7 @@ export default (async function(configHash: any) {
   setup(configHash.logs);
   const config: IConfig = new AppConfig(configHash);
   const storage: IStorageHandler = new Storage(config);
-  // waits until init calls have been intialized
+  // waits until init calls have been initialized
   await storage.init(config);
   return defineAPI(config, storage);
 });

src/lib/utils.js

@@ -92,7 +92,7 @@ export function isObject(obj: any): boolean {
  */
 export function validateMetadata(object: Package, name: string): Object {
   assert(isObject(object), 'not a json object');
-  assert.equal(object.name, name);
+  assert.strictEqual(object.name, name);
 
   if (!isObject(object[DIST_TAGS])) {
     object[DIST_TAGS] = {};

test/unit/api/__snapshots__/api.publish.spec.js.snap

@@ -0,0 +1,48 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`Publish endpoints - publish package should add a new package 1`] = `
+[MockFunction] {
+  "calls": Array [
+    Array [
+      "verdaccio",
+      Object {
+        "dist-tags": Object {},
+        "name": "verdaccio",
+        "time": Object {},
+        "versions": Object {},
+      },
+      [Function],
+    ],
+  ],
+  "results": Array [
+    Object {
+      "isThrow": false,
+      "value": undefined,
+    },
+  ],
+}
+`;
+
+exports[`Publish endpoints - publish package should change the existing package 1`] = `
+[MockFunction] {
+  "calls": Array [
+    Array [
+      "verdaccio",
+      Object {
+        "dist-tags": Object {},
+        "name": "verdaccio",
+        "time": Object {},
+        "versions": Object {},
+      },
+      undefined,
+      [Function],
+    ],
+  ],
+  "results": Array [
+    Object {
+      "isThrow": false,
+      "value": undefined,
+    },
+  ],
+}
+`;

test/unit/api/api.publish.spec.js

@@ -0,0 +1,264 @@
+/**
+ * @prettier
+ */
+import { addVersion, uploadPackageTarball, removeTarball, unPublishPackage, publishPackage } from '../../../src/api/endpoint/api/publish';
+import { HTTP_STATUS, API_ERROR } from '../../../src/lib/constants';
+
+const REVISION_MOCK = '15-e53a77096b0ee33e';
+
+describe('Publish endpoints - add a tag', () => {
+  let req;
+  let res;
+  let next;
+
+  beforeEach(() => {
+    req = {
+      params: {
+        version: '1.0.0',
+        tag: 'tag',
+        package: 'verdaccio',
+      },
+      body: '',
+    };
+    res = {
+      status: jest.fn(),
+    };
+
+    next = jest.fn();
+  });
+
+  test('should add a version', done => {
+    const storage = {
+      addVersion: (packageName, version, body, tag, cb) => {
+        expect(packageName).toEqual(req.params.package);
+        expect(version).toEqual(req.params.version);
+        expect(body).toEqual(req.body);
+        expect(tag).toEqual(req.params.tag);
+        cb();
+        done();
+      },
+    };
+
+    addVersion(storage)(req, res, next);
+
+    expect(res.status).toHaveBeenLastCalledWith(HTTP_STATUS.CREATED);
+    expect(next).toHaveBeenLastCalledWith({ ok: 'package published' });
+  });
+
+  test('when failed to add a version', done => {
+    const storage = {
+      addVersion: (packageName, version, body, tag, cb) => {
+        const error = {
+          message: 'failure',
+        };
+        cb(error);
+        done();
+      },
+    };
+
+    addVersion(storage)(req, res, next);
+
+    expect(next).toHaveBeenLastCalledWith({ message: 'failure' });
+  });
+});
+
+/**
+ * upload package: '/:package/-/:filename/*'
+ */
+describe('Publish endpoints - upload package tarball', () => {
+  let req;
+  let res;
+  let next;
+
+  beforeEach(() => {
+    req = {
+      params: {
+        filename: 'verdaccio.gzip',
+        package: 'verdaccio',
+      },
+      pipe: jest.fn(),
+      on: jest.fn(),
+    };
+    res = { status: jest.fn(), report_error: jest.fn() };
+    next = jest.fn();
+  });
+
+  test('should upload package tarball successfully', () => {
+    const stream = {
+      done: jest.fn(),
+      abort: jest.fn(),
+      on: jest.fn(() => (status, cb) => cb()),
+    };
+    const storage = {
+      addTarball(packageName, filename) {
+        expect(packageName).toEqual(req.params.package);
+        expect(filename).toEqual(req.params.filename);
+        return stream;
+      },
+    };
+
+    uploadPackageTarball(storage)(req, res, next);
+    expect(req.pipe).toHaveBeenCalled();
+    expect(req.on).toHaveBeenCalled();
+  });
+});
+
+/**
+ * Delete tarball: '/:package/-/:filename/-rev/:revision'
+ */
+describe('Publish endpoints - delete tarball', () => {
+  let req;
+  let res;
+  let next;
+
+  beforeEach(() => {
+    req = {
+      params: {
+        filename: 'verdaccio.gzip',
+        package: 'verdaccio',
+        revision: REVISION_MOCK,
+      },
+    };
+    res = { status: jest.fn() };
+    next = jest.fn();
+  });
+
+  test('should delete tarball successfully', done => {
+    const storage = {
+      removeTarball(packageName, filename, revision, cb) {
+        expect(packageName).toEqual(req.params.package);
+        expect(filename).toEqual(req.params.filename);
+        expect(revision).toEqual(req.params.revision);
+        cb();
+        done();
+      },
+    };
+
+    removeTarball(storage)(req, res, next);
+    expect(res.status).toHaveBeenCalledWith(HTTP_STATUS.CREATED);
+    expect(next).toHaveBeenCalledWith({ ok: 'tarball removed' });
+  });
+
+  test('failed while deleting the tarball', done => {
+    const error = {
+      message: 'deletion failed',
+    };
+    const storage = {
+      removeTarball(packageName, filename, revision, cb) {
+        cb(error);
+        done();
+      },
+    };
+
+    removeTarball(storage)(req, res, next);
+    expect(next).toHaveBeenCalledWith(error);
+  });
+});
+
+/**
+ * Un-publish package: '/:package/-rev/*'
+ */
+describe('Publish endpoints - un-publish package', () => {
+  let req;
+  let res;
+  let next;
+
+  beforeEach(() => {
+    req = {
+      params: {
+        package: 'verdaccio',
+      },
+    };
+    res = { status: jest.fn() };
+    next = jest.fn();
+  });
+
+  test('should un-publish package successfully', done => {
+    const storage = {
+      removePackage(packageName, cb) {
+        expect(packageName).toEqual(req.params.package);
+        cb();
+        done();
+      },
+    };
+
+    unPublishPackage(storage)(req, res, next);
+    expect(res.status).toHaveBeenCalledWith(HTTP_STATUS.CREATED);
+    expect(next).toHaveBeenCalledWith({ ok: 'package removed' });
+  });
+
+  test('un-publish failed', done => {
+    const error = {
+      message: 'un-publish failed',
+    };
+    const storage = {
+      removePackage(packageName, cb) {
+        cb(error);
+        done();
+      },
+    };
+
+    unPublishPackage(storage)(req, res, next);
+    expect(next).toHaveBeenCalledWith(error);
+  });
+});
+
+/**
+ * Publish package: '/:package/:_rev?/:revision?'
+ */
+describe('Publish endpoints - publish package', () => {
+  let req;
+  let res;
+  let next;
+
+  beforeEach(() => {
+    req = {
+      body: {
+        name: 'verdaccio',
+      },
+      params: {
+        package: 'verdaccio',
+      },
+    };
+    res = { status: jest.fn() };
+    next = jest.fn();
+  });
+
+  test('should change the existing package', () => {
+    const storage = {
+      changePackage: jest.fn(),
+    };
+
+    req.params._rev = REVISION_MOCK;
+
+    publishPackage(storage)(req, res, next);
+    expect(storage.changePackage).toMatchSnapshot();
+  });
+
+  test('should add a new package', () => {
+    const storage = {
+      addPackage: jest.fn(),
+    };
+
+    publishPackage(storage)(req, res, next);
+    expect(storage.addPackage).toMatchSnapshot();
+  });
+
+  test('should throw an error while publishing package', () => {
+    const storage = {
+      addPackage() {
+        throw new Error();
+      },
+    };
+    publishPackage(storage)(req, res, next);
+    expect(next).toHaveBeenCalledWith(new Error(API_ERROR.BAD_PACKAGE_DATA));
+  });
+
+  test('should throw an error for un-implemented star calls', () => {
+    const storage = {};
+    req.body._rev = REVISION_MOCK;
+    req.body.users = {};
+    publishPackage(storage)(req, res, next);
+    expect(next).toHaveBeenCalledWith(new Error('npm star| un-star calls are not implemented'));
+  });
+});

test/unit/api/notify-request.spec.js

@@ -75,7 +75,7 @@ describe('notifyRequest', () => {
     });
 
     const notification = require('../../../src/lib/notify/notify-request');
-    const infoArgs = [{ content: 'Verdaccio@x.x.x successfully published' }, 'A notification has been shipped: @{content}'];
+    const infoArgs = [{ content }, 'A notification has been shipped: @{content}'];
     const debugArgs = [{ body: 'Successfully delivered' }, ' body: @{body}'];
 
     await expect(notification.notifyRequest(options, content)).resolves.toEqual('Successfully delivered');
@@ -93,7 +93,7 @@ describe('notifyRequest', () => {
     });
 
     const notification = require('../../../src/lib/notify/notify-request');
-    const infoArgs = [{ content: 'Verdaccio@x.x.x successfully published' }, 'A notification has been shipped: @{content}'];
+    const infoArgs = [{ content }, 'A notification has been shipped: @{content}'];
 
     await expect(notification.notifyRequest(options, content)).rejects.toThrowError('body is missing');
     expect(logger.logger.info).toHaveBeenCalledWith(...infoArgs);