consul-service-registrator - npm Package Compare versions

Comparing version 2.2.2 to 2.3.0

CHANGELOG.md

 # Changelog
 
+### 2.3.0
+
+- Fix of bug with deregistration of service that wasn't registered
+- Code and test refactoring
+- "consul" module was updated to 0.40.0
+
 ### 2.2.2
@@ -4,0 +10,0 @@

package.json

 {
   "name": "consul-service-registrator",
-  "version": "2.2.2",
+  "version": "2.3.0",
   "description": "Set of classes for service registration in Consul",
@@ -29,4 +29,4 @@ "main": "index.js",
   "dependencies": {
-    "consul": "^0.36.0",
-    "lodash": "^4.17.13"
+    "consul": "0.40.0",
+    "lodash": "4.17.21"
   },
@@ -41,3 +41,3 @@ "devDependencies": {
     "randomstring": "^1.1.5",
-    "sinon": "^7.5.0"
+    "sinon": "13.0.1"
   },
@@ -44,0 +44,0 @@ "bugs": {

src/ServiceRegistrator.js

@@ -106,3 +106,3 @@ 'use strict';
 
-    register(overwrite = false) {
+    async register(overwrite = false) {
         if (!_.isBoolean(overwrite)) {
@@ -131,36 +131,37 @@ return Promise.reject(new Error('overwrite must be a boolean'));
         if (_.isArray(this._checks) && !_.isEmpty(this._checks)) {
-            return this._deregisterOnDemand(overwrite)
-                .then(() => {
-                    return this._consul.agent.service.register(options)
-                        .then(() => {
-                            let checkRegisterPromises = this._checks.map(check => {
-                                return this._consul.agent.check.register(check);
-                            });
-                            return Promise.all(checkRegisterPromises);
-                        })
-                        .then(() => this._active = true)
-                        .catch(err => {
-                            return this.deregister()
-                                .catch(deregisterErr => {
-                                    throw new Error(util.format(
-                                        'Can not register one of checks for the service `%s`, failed with error:' +
-                                        ' %s and failed to deregister just started service due to error: `%s`',
-                                        this._serviceId,
-                                        err,
-                                        deregisterErr
-                                    ));
-                                })
-                                .then(() => {
-                                    throw new Error(util.format(
-                                        'Can not register one of checks for the service `%s`, failed with error: %s',
-                                        this._serviceId,
-                                        err
-                                    ));
-                                });
-                        });
+            await this._deregisterOnDemand(overwrite);
+
+            try {
+                await this._consul.agent.service.register(options);
+
+                let checkRegisterPromises = this._checks.map(check => {
+                    return this._consul.agent.check.register(check);
                 });
+
+                await Promise.all(checkRegisterPromises);
+
+                this._active = true;
+            } catch (err) {
+                try {
+                    await this.deregister();
+                } catch (deregisterErr) {
+                    throw new Error(util.format(
+                        'Can not register one of checks for the service `%s`, failed with error:' +
+                        ' %s and failed to deregister just started service due to error: `%s`',
+                        this._serviceId,
+                        err.message,
+                        deregisterErr.message
+                    ));
+                }
+
+                throw new Error(util.format(
+                    'Can not register one of checks for the service `%s`, failed with error: %s',
+                    this._serviceId,
+                    err
+                ));
+            }
         } else {
-            return this._deregisterOnDemand(overwrite)
-                .then(() => this._consul.agent.service.register(options))
-                .then(() => this._active = true);
+            await this._deregisterOnDemand(overwrite);
+            await this._consul.agent.service.register(options);
+            this._active = true;
         }
@@ -207,21 +208,28 @@ }
 
-    deregister() {
+    async deregister() {
         this._active = false;
-        return this._consul.agent.service.deregister(this._serviceId);
+        try {
+            await this._consul.agent.service.deregister(this._serviceId);
+        } catch (deregisterErr) {
+            if (!this._isUnknownServiceError(deregisterErr)) {
+                // Service wasn't registered
+                throw deregisterErr;
+            }
+        }
     }
 
-    _deregisterOnDemand(deregister) {
-        return Promise.resolve()
-            .then(() => {
-                if (deregister) {
-                    return this.deregister();
-                }
-            })
-            .catch(deregisterErr => {
-                throw new Error(util.format(
-                    'Failed to deregister service `%s` in overwrite mode due to error: `%s`',
-                    this._serviceId,
-                    deregisterErr
-                ));
-            });
+    async _deregisterOnDemand(deregister) {
+        if (!deregister) {
+            return
+        }
+
+        try {
+            await this.deregister();
+        } catch (deregisterErr) {
+            throw new Error(util.format(
+                'Failed to deregister service `%s` in overwrite mode due to error: `%s`',
+                this._serviceId,
+                deregisterErr.message
+            ));
+        }
     }
@@ -242,4 +250,10 @@
     }
+
+    _isUnknownServiceError(err) {
+        return err.message &&
+            _.isString(err.message) &&
+            err.message.includes('Unknown service "' + this._serviceId + '"');
+    }
 }
 
 module.exports = ServiceRegistrator;

tests/Unit/ServiceRegistrator.js

@@ -221,3 +221,7 @@ 'use strict';
 
-            it('register with overwrite', function () {
+            /**
+             * When ServiceRegistrator tries to deregister the service it calls consul.agent.service.deregister
+             * and this call resolves promise if service with that name was already registered.
+             */
+            it('register with overwrite, service was already registered', function () {
                 let consul      = getFakeConsulObject(true);
@@ -244,3 +248,31 @@ let service     = new ServiceRegistrator(CONSUL_OPTIONS, 'name', 'name.' + pid);
 
-            it('register with overwrite, that fail', function () {
+            /**
+             * When ServiceRegistrator tries to deregister the service it calls consul.agent.service.deregister
+             * and this call rejects promise if service with that name wasn't registered yet.
+             */
+            it('register with overwrite, service was not registered', async function () {
+                let consul        = getFakeConsulObject(true);
+                const serviceName = 'serviceName';
+                const serviceId   = serviceName + '.' + pid;
+                let service       = new ServiceRegistrator(CONSUL_OPTIONS, serviceName, serviceId);
+                service._consul   = consul;
+
+                let deregisterError      = new Error('Unknown service "' + serviceId + '"');
+                let consulRegisterStub   = sinon.stub(consul.agent.service, 'register');
+                let consulDeregisterStub = sinon.stub(consul.agent.service, 'deregister');
+                let registerReturn       = Promise.resolve();
+                let deregisterReturn     = Promise.reject(deregisterError);
+
+                consulRegisterStub.returns(registerReturn);
+                consulDeregisterStub.returns(deregisterReturn);
+
+                return service.register(true)
+                    .then(() => {
+                        assert.equal(consulDeregisterStub.callCount, 1, 'must be called once');
+                        assert.equal(consulRegisterStub.callCount, 1, 'must be called once');
+                        assert.isTrue(service._active);
+                    });
+            });
+
+            it('register with overwrite, that fail', async function () {
                 let consul      = getFakeConsulObject(true);
@@ -250,3 +282,3 @@ let service     = new ServiceRegistrator(CONSUL_OPTIONS, 'name', 'name123');
 
-                let deregisterError      = 'Some consul error';
+                let deregisterError      = new Error('Some consul error');
                 let consulRegisterStub   = sinon.stub(consul.agent.service, 'register');
@@ -260,3 +292,6 @@ let consulDeregisterStub = sinon.stub(consul.agent.service, 'deregister');
 
-                service.register(true)
+                return service.register(true)
+                    .then(() => {
+                        assert.fail("Registration must fail if consul return unknown error");
+                    })
                     .catch((err) => {
@@ -297,3 +332,3 @@ assert.equal(consulDeregisterStub.callCount, 1, 'must be called once');
 
-            it('error on register without checks, from consul', function (done) {
+            it('error on register without checks, from consul', async function () {
                 let consul      = getFakeConsulObject(true);
@@ -313,13 +348,18 @@ let service     = new ServiceRegistrator(CONSUL_OPTIONS, 'name', 'name_someId');
                 consulRegisterStub.returns(regPromise);
-                let regResult = service.register();
 
                 let regError = new Error('regerr');
 
-                regResult.catch(err => {
+                // noinspection JSUnusedAssignment
+                rejectRegPromise(regError);
+
+                try {
+                    await service.register();
+                } catch (err) {
                     assert.equal(consulRegisterStub.callCount, 1, 'must be called once');
                     assert.strictEqual(err, regError);
-                    done();
-                });
 
-                rejectRegPromise(regError);
+                    return;
+                }
+
+                assert.fail('Unexpected test behaviour. Registration must fail according to stubs.');
             });
@@ -405,15 +445,17 @@
 
-                service.register()
-                    .then(() => {
-                        assert.isFalse(true, 'Unexpected test behaviour');
-                    })
-                    .catch(err => {
-                        assert.equal(consulRegisterStub.callCount, 1, 'must be called once');
-                        assert.isFalse(service._active);
+                try {
+                    await service.register();
+                } catch (err) {
+                    assert.equal(consulRegisterStub.callCount, 1, 'must be called once');
+                    assert.isFalse(service._active);
 
-                        // eslint-disable-next-line max-len
-                        assert.match(err.message, /^Can not register one of checks for the service `\w+`, failed with error: Error: check reg error$/);
-                        assert.equal(deregisterStub.callCount, 1, 'must be called once');
-                        assert.isTrue(deregisterStub.firstCall.calledWith());
-                    });
+                    // eslint-disable-next-line max-len
+                    assert.match(err.message, /^Can not register one of checks for the service `\w+`, failed with error: Error: check reg error/);
+                    assert.equal(deregisterStub.callCount, 1, 'must be called once');
+                    assert.isTrue(deregisterStub.firstCall.calledWith());
+
+                    return;
+                }
+
+                assert.fail('Unexpected test behaviour. Registration must fail according to stubs.');
             });
@@ -435,3 +477,3 @@
                 let checkError         = new Error('check reg error');
-                let deregisterMsg      = 'deregister fail';
+                let deregisterErr      = new Error('deregister fail');
                 let consulRegisterStub = sinon.stub(consul.agent.service, 'register');
@@ -444,3 +486,3 @@
                 consulCheckRegisterStub.returns(Promise.reject(checkError));
-                deregisterStub.returns(Promise.reject(deregisterMsg));
+                deregisterStub.returns(Promise.reject(deregisterErr));
 
@@ -457,18 +499,20 @@ assert.isNull(service._checks);
 
-                service.register()
-                    .then(() => {
-                        assert.isFalse(true, 'Unexpected test behaviour');
-                    })
-                    .catch(err => {
-                        assert.equal(consulRegisterStub.callCount, 1, 'must be called once');
-                        assert.isFalse(service._active);
-                        assert.match(err.message, new RegExp(
-                            '^Can not register one of checks for the service `\\w+`, failed with error: ' +
-                                'Error: check reg error and failed to deregister just started service due to ' +
-                                'error: `deregister fail`$'
-                        ));
-                        assert.equal(consulCheckRegisterStub.callCount, 1, 'must be called once');
-                        assert.equal(deregisterStub.callCount, 1, 'must be called once');
-                        assert.isTrue(deregisterStub.firstCall.calledWith());
-                    });
+                try {
+                    await service.register();
+                } catch (err) {
+                    assert.equal(consulRegisterStub.callCount, 1, 'must be called once');
+                    assert.isFalse(service._active);
+                    assert.match(err.message, new RegExp(
+                        '^Can not register one of checks for the service `\\w+`, failed with error: ' +
+                        'check reg error and failed to deregister just started service due to ' +
+                        'error: `deregister fail`'
+                    ));
+                    assert.equal(consulCheckRegisterStub.callCount, 1, 'must be called once');
+                    assert.equal(deregisterStub.callCount, 1, 'must be called once');
+                    assert.isTrue(deregisterStub.firstCall.calledWith());
+
+                    return;
+                }
+
+                assert.fail('Unexpected test behaviour. Registration must fail according to stubs.');
             });
@@ -512,14 +556,16 @@
 
-                service.register(true)
-                    .then(() => {
-                        assert.isFalse(true, 'Unexpected test behaviour');
-                    })
-                    .catch(err => {
-                        assert.equal(consulRegisterStub.callCount, 1, 'must be called once');
-                        assert.isFalse(service._active);
+                try {
+                    await service.register(true);
+                } catch (err) {
+                    assert.equal(consulRegisterStub.callCount, 1, 'must be called once');
+                    assert.isFalse(service._active);
 
-                        // eslint-disable-next-line max-len
-                        assert.match(err.message, /^Can not register one of checks for the service `\w+`, failed with error: Error: check reg error$/);
-                        assert.equal(consulDeregisterStub.callCount, 2, 'must be called twice');
-                    });
+                    // eslint-disable-next-line max-len
+                    assert.match(err.message, /^Can not register one of checks for the service `\w+`, failed with error: Error: check reg error/);
+                    assert.equal(consulDeregisterStub.callCount, 2, 'must be called twice');
+
+                    return;
+                }
+
+                assert.fail('Unexpected test behaviour. Registration must fail according to stubs.');
             });
@@ -526,0 +572,0 @@ });

New alerts

License Policy Violation

License

This package is not allowed per your license policy. Review the package's license to ensure compliance.

Found 1 instance in 1 package

Fixed alerts

License Policy Violation

License

This package is not allowed per your license policy. Review the package's license to ensure compliance.

Found 1 instance in 1 package

New author

Supply chain risk

A new npm collaborator published a version of the package for the first time. New collaborators are usually benign additions to a project, but do indicate a change to the security surface area of a package.

Found 1 instance in 1 package

Improved metrics

Total package byte prevSize

63560

increased by10.33%

Number of package files

20

increased by11.11%

Lines of code

1106

increased by6.04%

Number of medium supply chain risk alerts

2

decreased by-33.33%

Dependency changes

• + Addedconsul@0.40.0(transitive)

• - Removedconsul@0.36.0(transitive)

• Updatedconsul@0.40.0

• Updatedlodash@4.17.21