review

lib/clients/ZookeeperManager.js

@@ -38,57 +38,68 @@ class ZookeeperManager extends EventEmitter {
     _connect() {
         // clean up exists client before reconnect
         if (this.client) {
+            // close and clean up the existing ZooKeeper client connection.
+            this.close();
             this.client.removeAllListeners();
         }
 
         this.client = zookeeper.createClient(this.connectionString, this.options);
 
-        this.client.once('connected', () => {
-            // TODO: ARTESCA-10337 The 'autoCreateNamespace' functionality is currently specific to
-            // Artesca and may be removed in future versions once the Zenko Operator can handle base path creation.
-            // Once removed, we can simply rely on the 'connected' state instead of the 'ready' state and
-            // stop listening on 'error' event.
-            if (this.options && this.options.autoCreateNamespace) {
-                // for some reason this.client.exists() does not return
-                // NO_NODE when base path does not exist, hence use
-                // getData() instead
-                this.getData('/', err => {
-                    if (err && err.name !== 'NO_NODE') {
-                        this.emit('error', err);
-                        return;
-                    }
-                    // NO_NODE error and autoCreateNamespace is enabled
-                    if (err) {
-                        const nsIndex = this.connectionString.indexOf('/');
-                        const hostPort = this.connectionString.slice(0, nsIndex);
-                        const namespace = this.connectionString.slice(nsIndex);
-                        const rootZkClient = zookeeper.createClient(hostPort, this.options);
-                        rootZkClient.connect();
-                        rootZkClient.mkdirp(namespace, err => {
-                            if (err && err.name !== 'NODE_EXISTS') {
-                                this.emit('error');
-                                return;
-                            }
-                            this.emit('ready');
-                            return;
-                        });
-                        return;
+        let nsIndex;
+        let namespace;
+        let rootZkClient;
+        async.series([
+            // Check zookeeper client is connected
+            next => this.client.once('connected', next),
+            // Create namespace if it exists and options.autoCreateNamespace is true
+            next => {
+                // TODO: ARTESCA-10337 The 'autoCreateNamespace' functionality is currently specific to
+                // Artesca and may be removed in future versions once the Zenko Operator can handle base path creation.
+                // Once removed, we can simply rely on the 'connected' state instead of the 'ready' state and
+                // stop listening on 'error' event.
+                if (this.options && this.options.autoCreateNamespace) {
+                    nsIndex = this.connectionString.indexOf('/');
+                    namespace = this.connectionString.slice(nsIndex);
+                    if (nsIndex > -1 && namespace !== '/') {
+                        return process.nextTick(next);
                     }
-                    this.emit('ready');
-                    return;
-                });
-                return;
-            }
-            this.emit('ready');
-            return;
+                }
+                return process.nextTick(() => next({ event: 'ready' }));
+            },
+            // Check that the namespace is not already created.
+            next => this.getData('/', err => {
+                if (err && err.name !== 'NO_NODE') {
+                    return next({ event: 'error', err });
+                }
+                // If not created (NO_NODE error), go to next step.
+                if (err) {
+                    return next();
+                }
+                return next({ event: 'ready' });
+            }),
+            // Since the namespace has not been created, connect to the root zookeeper client
+            next => {
+                const hostPort = this.connectionString.slice(0, nsIndex);
+                rootZkClient = zookeeper.createClient(hostPort, this.options);
+                rootZkClient.connect();
+                rootZkClient.once('connected', next);
+            },
+            // Once connected, use the root zookeeper client to create the namespace
+            next => rootZkClient.mkdirp(namespace, err => {
+                rootZkClient.close();
+                if (err && err.name !== 'NODE_EXISTS') {
+                    return next({ event: 'error', err });
+                }
+                return next({ event: 'ready' });
+            }),
+        ], ({ event, err }) => {
+            this.emit(event, err);
         });
 
         this.client.once('expired', () => {
             this.log.info('zookeeper client expired', {
                 method: 'ZookeeperManager.once.expired',
             });
-            // close and clean up the existing ZooKeeper client connection.
-            this.close();
             // establish a new session with the ZooKeeper.
             this._connect();
         });

tests/functional/lib/ZookeeperManager.js

@@ -34,12 +34,19 @@ describe('ZookeeperManager', () => {
 
         after(() => rootZkClient.close());
 
-        afterEach(done => rootZkClient.remove(basePath, err => {
-            if (err && err.name !== 'NO_NODE') {
-                return done(err);
-            }
-            return done();
-        }));
+        afterEach(done => {
+            rootZkClient.remove(basePath, err => {
+                if (err && err.name !== 'NO_NODE') {
+                    return done(err);
+                }
+                return rootZkClient.remove('/hello', err => {
+                    if (err && err.name !== 'NO_NODE') {
+                        return done(err);
+                    }
+                    return done();
+                });
+            });
+        });
 
         it('should create the base path if autoCreateNamespace set to true', done => {
             const connectionString = `localhost:2181${basePath}`;
@@ -59,6 +66,26 @@ describe('ZookeeperManager', () => {
             });
         });
 
+        it('should skip and fire ready event if autoCreateNamespace is true with no namespace', done => {
+            const connectionStringWithoutNamespace = 'localhost:2181';
+            const options = { autoCreateNamespace: true };
+            zkClient = new ZookeeperManager(connectionStringWithoutNamespace, options, log);
+            zkClient.once('ready', () => zkClient.getData('/', err => {
+                assert.ifError(err);
+                done();
+            }));
+        });
+
+        it('should skip and fire ready event if autoCreateNamespace is true with / namespace', done => {
+            const connectionStringWithoutNamespace = 'localhost:2181/';
+            const options = { autoCreateNamespace: true };
+            zkClient = new ZookeeperManager(connectionStringWithoutNamespace, options, log);
+            zkClient.once('ready', () => zkClient.getData('/', err => {
+                assert.ifError(err);
+                done();
+            }));
+        });
+
         it('should not create the base path if autoCreateNamespace set to false', done => {
             const connectionString = `localhost:2181${basePath}`;
             const options = { autoCreateNamespace: false };

tests/unit/clients/ZookeeperManager.js

@@ -0,0 +1,89 @@
+const sinon = require('sinon');
+const assert = require('assert');
+const zookeeper = require('node-zookeeper-client');
+const { Logger } = require('werelogs');
+const ZookeeperManager = require('../../../lib/clients/ZookeeperManager');
+
+describe('ZookeeperManager', () => {
+    let zkClient;
+    let mockClient;
+    const log = new Logger('ZookeeperManager:unit');
+
+    beforeEach(() => {
+        // Create a mock client
+        mockClient = {
+            on: sinon.stub(),
+            once: sinon.stub(),
+            connect: sinon.stub(),
+            getData: sinon.stub(),
+            mkdirp: sinon.stub(),
+        };
+
+        // Stub the 'once' method for 'connected' event
+        mockClient.once.withArgs('connected').callsFake((event, callback) => callback());
+
+        // Replace the createClient method to return the mock client
+        sinon.stub(zookeeper, 'createClient').returns(mockClient);
+    });
+
+    afterEach(() => {
+        sinon.restore();
+    });
+
+    it('should skip if no namespace', done => {
+        mockClient.getData.callsArgWith(1, null);
+        zkClient = new ZookeeperManager('localhost:2181', { autoCreateNamespace: true }, log);
+
+        zkClient.on('ready', () => {
+            sinon.assert.notCalled(mockClient.getData);
+            sinon.assert.notCalled(mockClient.mkdirp);
+            done();
+        });
+    });
+
+    it('should not create namespace if it already exists', done => {
+        mockClient.getData.callsArgWith(1, null);
+        zkClient = new ZookeeperManager('localhost:2181/mynamespace', { autoCreateNamespace: true }, log);
+
+        zkClient.on('ready', () => {
+            sinon.assert.calledOnce(mockClient.getData);
+            sinon.assert.notCalled(mockClient.mkdirp);
+            done();
+        });
+    });
+
+    it('should create namespace if it does not exist', done => {
+        mockClient.getData.callsArgWith(1, { name: 'NO_NODE' });
+        mockClient.mkdirp.callsArgWith(1, null);
+        zkClient = new ZookeeperManager('localhost:2181/mynamespace', { autoCreateNamespace: true }, log);
+
+        zkClient.on('ready', () => {
+            sinon.assert.calledOnce(mockClient.getData);
+            sinon.assert.calledOnce(mockClient.mkdirp);
+            done();
+        });
+    });
+
+    it('should handle getData error', done => {
+        mockClient.getData.callsArgWith(1, { name: 'Simulated GET data error' });
+        zkClient = new ZookeeperManager('localhost:2181/mynamespace', { autoCreateNamespace: true }, log);
+
+        zkClient.on('error', err => {
+            assert(err, 'Error event should be emitted');
+            assert.strictEqual(err.name, 'Simulated GET data error');
+            done();
+        });
+    });
+
+    it('should handle mkdirp error', done => {
+        mockClient.getData.callsArgWith(1, { name: 'NO_NODE' });
+        mockClient.mkdirp.callsArgWith(1, { name: 'Simulated MKDIRP error' });
+        zkClient = new ZookeeperManager('localhost:2181/mynamespace', { autoCreateNamespace: true }, log);
+
+        zkClient.on('error', err => {
+            assert(err, 'Error event should be emitted');
+            assert.strictEqual(err.name, 'Simulated MKDIRP error');
+            done();
+        });
+    });
+});