From ba5a321ec34f08da2a657f71f72002d3fb105263 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= <markus.makela@mariadb.com>
Date: Sat, 22 Jul 2017 03:57:37 +0300
Subject: [PATCH] MXS-1300: Improve overall error handling

All errors are now returned as rejected Promises. This will make the error
reporting more consistent with the tested output as well as the actual
output.

Added more tests for error cases and fixed minor argument processing bugs.
---
 maxctrl/lib/common.js   | 26 +++++++++---------
 maxctrl/test/special.js | 58 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 69 insertions(+), 15 deletions(-)

diff --git a/maxctrl/lib/common.js b/maxctrl/lib/common.js
index 96bb55e7a..d13f240f8 100644
--- a/maxctrl/lib/common.js
+++ b/maxctrl/lib/common.js
@@ -35,6 +35,10 @@ module.exports = function() {
 
         this.argv = argv
 
+        if (!argv.hosts || argv.hosts.length < 1) {
+            argv.reject("No hosts defined")
+        }
+
         return pingCluster(argv.hosts)
             .then(function() {
                 var promises = []
@@ -45,11 +49,11 @@ module.exports = function() {
 
                 return Promise.all(promises)
                     .catch(function(err) {
-                        argv.reject()
+                        argv.reject(err)
                     })
             }, function(err) {
                 // One of the HTTP request pings to the cluster failed, log the error
-                logError(JSON.stringify(err.error, null, 4))
+                argv.reject(JSON.stringify(err.error, null, 4))
             })
     }
 
@@ -189,15 +193,14 @@ module.exports = function() {
                 }
             }, function(err) {
                 if (err.response && err.response.body) {
-                    logError(JSON.stringify(err.response.body, null, 4))
+                    return error(JSON.stringify(err.response.body, null, 4))
                 } else if (err.statusCode) {
-                    logError('Server responded with ' + err.statusCode)
+                    return error('Server responded with: ' + err.statusCode)
                 } else if (err.error) {
-                    logError(JSON.stringify(err.error, null, 4))
+                    return error(JSON.stringify(err.error, null, 4))
                 } else {
-                    logError('Undefined error: ' + JSON.stringify(err, null, 4))
+                    return error('Undefined error: ' + JSON.stringify(err, null, 4))
                 }
-                return Promise.reject()
             })
     }
 
@@ -206,13 +209,8 @@ module.exports = function() {
             .then(this.argv.resolve, this.argv.reject)
     }
 
-    this.logError = function(err) {
-        this.logger.error(colors.red('Error:'), err)
-    }
-
     this.error = function(err) {
-        logger.log(colors.red('Error:'), err)
-        return Promise.reject()
+        return Promise.reject(colors.red('Error: ') +  err)
     }
 }
 
@@ -270,7 +268,7 @@ function pingCluster(hosts) {
     var promises = []
 
     hosts.forEach(function(i) {
-        promises.push(request('http://' + i + '/v1'))
+        promises.push(request(getUri(i, false, '')))
     })
 
     return Promise.all(promises)
diff --git a/maxctrl/test/special.js b/maxctrl/test/special.js
index 6fc45ab6a..20ae3eb1a 100644
--- a/maxctrl/test/special.js
+++ b/maxctrl/test/special.js
@@ -4,9 +4,9 @@ describe("Library invocation", function() {
     before(startMaxScale)
 
     var ctrl = require('../lib/core.js')
-    var opts = { extra_args: [ '--quiet'] }
 
     it('extra options', function() {
+        var opts = { extra_args: [ '--quiet'] }
         return ctrl.execute('list servers'.split(' '), opts)
             .should.be.fulfilled
     })
@@ -16,5 +16,61 @@ describe("Library invocation", function() {
             .should.be.fulfilled
     })
 
+    it('multiple hosts', function() {
+        var opts = { extra_args: [ '--quiet', '--hosts', '127.0.0.1:8989', 'localhost:8989'] }
+        return ctrl.execute('list servers'.split(' '), opts)
+            .should.be.fulfilled
+    })
+
+    it('no hosts', function() {
+        var opts = { extra_args: [ '--quiet', '--hosts'] }
+        return ctrl.execute('list servers'.split(' '), opts)
+            .should.be.rejected
+    })
+
+    it('TSV output', function() {
+        var opts = { extra_args: [ '--quiet', '--tsv'] }
+        return ctrl.execute('list servers'.split(' '), opts)
+            .then(function() {
+                return ctrl.execute('show server server1'.split(' '), opts)
+            })
+            .should.be.fulfilled
+    })
+
+    it('secure mode', function() {
+        // The test is run in HTTP mode so a HTTPS request should fail
+        var opts = { extra_args: [ '--quiet', '--secure'] }
+        return ctrl.execute('list servers'.split(' '), opts)
+            .should.be.rejected
+    })
+
+    // These should be last
+    it('user credentials', function() {
+        var opts1 = { extra_args: [ '--quiet'] }
+        var opts2 = { extra_args: [ '--quiet', '--user', 'test', '--password', 'test'] }
+        return ctrl.execute('create user test test'.split(' '), opts1)
+            .then(function() {
+                return ctrl.execute('alter maxscale admin_auth true'.split(' '), opts1)
+            })
+            .then(function() {
+                return ctrl.execute('list servers'.split(' '), opts2)
+            })
+            .should.be.fulfilled
+    })
+
+    it('bad user credentials', function() {
+        var opts = { extra_args: [ '--quiet', '--user', 'not-a-user', '--password', 'not-a-password'] }
+        return ctrl.execute('list servers'.split(' '), opts)
+            .should.be.rejected
+    })
+
+    it('connection failure', function() {
+        stopMaxScale()
+            .then(function() {
+                return ctrl.execute('list servers'.split(' '))
+                    .should.be.rejected
+            })
+    })
+
     after(stopMaxScale)
 });