fix for buggy disambiguation of commands, plus usage models for host commands

starpit · starpit · commit eb5bdbac3506 · 2018-03-28T08:49:48.000-04:00
Fixes #746 usage models for auth commands Fixes #748
diff --git a/app/content/js/command-tree.js b/app/content/js/command-tree.js
@@ -26,19 +26,29 @@ const util = require('util'),
       interior = () => undefined,   // this will trigger a re-parse using Context.current as the path prefix
       newTree = () => ({ $: root(), key: '/', route: '/', children: {}}),
       model = newTree(),    // this is the model of registered listeners, a tree
-      intentions = newTree(),    // this is the model of registered intentional listeners
-      disambiguator = {}    // map from command name to disambiguations
+      intentions = newTree()    // this is the model of registered intentional listeners
+let disambiguator = {}    // map from command name to disambiguations
 
 debug('finished loading modules')
 
 // for plugins.js
-exports.disambiguator = () => {
+exports.startScan = () => {
+    const state = disambiguator
+    disambiguator = {}
+    debug('startScan', disambiguator)
+    return state
+}
+exports.endScan = state => {
+    debug('finishing up', disambiguator)
     const map = {}
     for (let command in disambiguator) {
         map[command] = disambiguator[command].map(({route, options}) => ({
             route, plugin: options && options.plugin
         }))
     }
+    if (state) {
+        disambiguator = state
+    }
     return map
 }
 
@@ -505,7 +515,7 @@ const read = (model, argv) => {
  */
 const disambiguate = (argv, noRetry=false) => {
     let idx
-    const resolutions = ( (((idx=0)||true) && disambiguator[argv[idx]]) || (((idx=argv.length-1)||true) && disambiguator[argv[idx]]) || []).filter(isFileFilter)
+    const resolutions = ( (((idx=0)||true) && resolver.disambiguate(argv[idx])) || (((idx=argv.length-1)||true) && resolver.disambiguate(argv[idx])) || [])
     debug('disambiguate', idx, argv, resolutions)
 
     if (resolutions.length === 0 && !noRetry) {
@@ -516,11 +526,26 @@ const disambiguate = (argv, noRetry=false) => {
 
     } else if (resolutions.length === 1) {
         // one unambiguous resolution! great, but we need to
-        // double-check: if the resolution is a subtree, then it better have a child that matches 
-        const leaf = resolutions[0]
+        // double-check: if the resolution is a subtree, then it better have a child that matches
+        const argvForMatch = resolutions[0].route.split('/').slice(1)
+        const cmdMatch = treeMatch(model, argvForMatch),
+              leaf = cmdMatch && cmdMatch.$ ? cmdMatch : treeMatch(intentions, argvForMatch)
+
+        debug('disambiguate single match', idx, argv, cmdMatch, leaf)
+
+        if (!leaf || !leaf.$) {
+            if (!noRetry && resolutions[0].plugin) {
+                debug('disambiguate attempting to resolve plugins 2')
+                resolver.resolve(`/${argv.join('/')}`)
+                return disambiguate(argv, true)
+            } else {
+                debug('disambiguate nope', intentions)
+                return
+            }
 
-        if (idx < argv.length - 1 && leaf.children) {
+        } else if (idx < argv.length - 1 && leaf.children) {
             // then the match is indeed a subtree
+            debug('validating disambiguation')
             let foundMatch = false
             const next = argv[argv.length - 1]
             for (let cmd in leaf.children) {
@@ -535,7 +560,7 @@ const disambiguate = (argv, noRetry=false) => {
             }
         }
 
-        debug('disambiguate success', leaf)
+        debug(`disambiguate success ${leaf.route}`, leaf)
         return withEvents(leaf.$, leaf)
     }
 }
diff --git a/app/content/js/plugins.js b/app/content/js/plugins.js
@@ -15,7 +15,7 @@
  */
 
 const debug = require('debug')('plugins')
-debug('starting')
+debug('loading')
 
 const fs = require('fs'),
       path = require('path'),
@@ -323,13 +323,19 @@ const loadPrescan = userDataDir => {
     debug('loading prescan %s', prescanned)
 
     return new Promise((resolve, reject) => {
-        fs.readFile(prescanned, (err, data) => {
+        fs.readFile(prescanned, (err, _data) => {
             debug('read done, any errors in the read? %s', !!err)
 
             if (err) {
                 reject(err)
             } else {
-                resolve(JSON.parse(data.toString()))
+                const data = _data.toString()
+                if (data.trim().length === 0) {
+                    // it was empty
+                    resolve({})
+                } else {
+                    resolve(JSON.parse(data))
+                }
             }
         })
     })
@@ -393,8 +399,14 @@ const makeResolver = prescan => {
     const resolver = {
         isOverridden: route => prescan.overrides[route],
 
+        disambiguate: command => {
+            debug('attempting to disambiguate command', command)
+            return prescan.disambiguator[command]
+        },
+
         /** given a partial command, do we have a disambiguation of it? e.g. "gr" => "grid" */
         disambiguatePartial: partial => {
+            debug('attempting to disambiguate partial', partial)
             const matches = []
             if (prescan.disambiguator) {
                 for (let command in prescan.disambiguator) {
@@ -434,7 +446,9 @@ const makeResolver = prescan => {
  *
  */
 exports.scan = opts => {
-    debug('scan')
+    debug('scan', opts)
+
+    const state = opts.externalOnly && commandTree.startScan()
 
     return resolveFromLocalFilesystem(opts).then(() => {
         const disambiguator = {}
@@ -465,7 +479,8 @@ exports.scan = opts => {
                 }
             }
         }
-        return { commandToPlugin, topological, flat, overrides, usage, disambiguator: commandTree.disambiguator() }
+
+        return { commandToPlugin, topological, flat, overrides, usage, disambiguator: commandTree.endScan(state) }
     })
 }
 
diff --git a/app/content/js/usage-error.js b/app/content/js/usage-error.js
@@ -92,7 +92,7 @@ const wrap = div => {
  */
 const usageFromCommand = command => repl.qexec(command)
       .then(_ => {
-          console.error('Invalid usage model', _)
+          console.error('Invalid usage model', command, _)
           throw new Error('Internal Error')
       })
       .catch(usageError => usageError.raw)
diff --git a/app/plugins/modules/plugin/lib/commands.js b/app/plugins/modules/plugin/lib/commands.js
@@ -57,6 +57,15 @@ const doList = (_a, _b, fullArgv, modules, rawCommandString, _2, argvWithoutOpti
                               name,
                               onclick: () => repl.pexec(name)
                             })))
+        .catch(err => {
+            if (err.code === 'ENOENT') {
+                const error = new Error('This plugin is not installed')
+                error.code = 404
+                throw error
+            } else {
+                throw err
+            }
+        })
             //success(false, `offered by the ${plugin} plugin`, commands))
 }
 
diff --git a/app/plugins/modules/plugin/lib/compile.js b/app/plugins/modules/plugin/lib/compile.js
@@ -61,7 +61,7 @@ const writeToFile = (dir, modules) => new Promise((resolve, reject) => {
  *
  */
 const readFile = dir => new Promise((resolve, reject) => {
-    fs.readFile(prescanned(dir), (err, data) => {
+    fs.readFile(prescanned(dir), (err, _data) => {
         if (err) {
             console.error(err.code)
             if (err.code === 'ENOENT') {
@@ -70,7 +70,18 @@ const readFile = dir => new Promise((resolve, reject) => {
                 reject(err)
             }
         } else {
-            resolve(JSON.parse(data.toString()))
+            try {
+                const data = _data.toString()
+                if (!data || data.trim().length === 0) {
+                    // it was empty
+                    resolve({})
+                } else {
+                    resolve(JSON.parse(data))
+                }
+            } catch (err) {
+                console.error(err)
+                resolve({})
+            }
         }
     })
 })
diff --git a/app/plugins/modules/plugin/usage.js b/app/plugins/modules/plugin/usage.js
@@ -18,7 +18,7 @@
 const parents = [{ command: 'plugin' }]
 
 /** required parameter: name of installed plugin */
-const installedPlugin = [{ name: 'plugin', docs: 'the name of an installed plugin' }]
+const installedPlugin = [{ name: 'plugin', docs: 'the name of an installed plugin', entity: 'plugin' }]
 
 /**
  * Usage model for plugin commands
@@ -68,6 +68,7 @@ exports.list = {
     breadcrumb: 'List plugins',
     docs: 'list installed Shell plugins',
     example: 'plugin list',
+    optional: [{ name: '--limit', hidden: true }], // to make tab completion happy
     parents
 }
 
diff --git a/app/plugins/ui/commands/auth.js b/app/plugins/ui/commands/auth.js
@@ -27,6 +27,81 @@ const fs = require('fs'),
       expandHomeDir = require('expand-home-dir'),
       wskpropsFile = expandHomeDir('~/.wskprops')
 
+const usage = {
+    auth: {
+        toplevel: {
+            commandPrefix: 'auth',
+            title: 'Authorization Operations',
+            header: 'Commands to switch, list, and remember OpenWhisk authorization keys',
+            related: ['host']
+        },
+        list: {
+            strict: 'list',
+            command: 'list',
+            title: 'List Auth Keys',
+            header: 'List the OpenWhisk namespaces for which you have authorization keys',
+            parents: ['auth'],
+            related: ['host set']
+        },
+        switch: {
+            strict: 'switch',
+            command: 'switch',
+            title: 'Switch Auth Keys',
+            header: 'Switch to use an OpenWhisk namespace, by name',
+            example: 'auth switch <namespace>',
+            required: [{ name: 'namespace', docs: 'an OpenWhisk namespace', entity: 'namespace' }],
+            parents: ['auth'],
+            related: ['auth list', 'host set']
+        },
+        add: {
+            strict: 'add',
+            command: 'add',
+            title: 'Add Auth Key',
+            header: 'Install an OpenWhisk authorization key',
+            example: 'auth add <auth>',
+            required: [{ name: 'auth', docs: 'an OpenWhisk authorization key' }],
+            parents: ['auth'],
+            related: ['auth switch', 'auth list', 'host set']
+        }
+    },
+    host: {
+        toplevel: {
+            commandPrefix: 'host',
+            title: 'Host Operations',
+            header: 'Commands to switch OpenWhisk API host',
+            related: ['auth']
+        },
+        get: {
+            strict: 'get',
+            command: 'get',
+            commandPrefix: 'host',
+            title: 'Get API Host',
+            header: 'Print the current OpenWhisk API host',
+            example: 'host get',
+            parents: ['host']
+        },
+        set: {
+            strict: 'set',
+            command: 'set',
+            commandPrefix: 'host',
+            title: 'Set API Host',
+            header: 'Change the OpenWhisk API host to a chosen alternative',
+            example: 'host set <which>',
+            nRowsInViewport: 5,
+            oneof: [
+                { name: 'local', docs: 'Use a local OpenWhisk installation' },
+                { name: 'us-south', docs: 'Use the IBM Cloud Dallas installation' },
+                { name: 'eu-gb', docs: 'Use the IBM Cloud London installation' },
+                { name: 'eu-de', docs: 'Use the IBM Cloud Frankrut installation' },
+                { name: 'hostname', docs: 'Use a given hostname or IP address' }
+            ],
+            parents: ['host']
+        }
+    }
+}
+usage.auth.toplevel.available = [usage.auth.add, usage.auth.list, usage.auth.switch]
+usage.host.toplevel.available = [usage.host.get, usage.host.set]
+
 /**
  * The message we will use to inform the user of a auth switch event
  *
@@ -142,8 +217,8 @@ const use = (wsk, commandTree) => verb => (_1, _2, _3, _4, _5, _6, argv) => name
 module.exports = (commandTree, prequire) => {
     const wsk = prequire('/ui/commands/openwhisk-core')
 
-    commandTree.subtree('/host', { docs: 'Commands to switch OpenWhisk API host' })
-    commandTree.subtree('/auth', { docs: 'Commands to switch, list, and remember OpenWhisk authorization keys' })
+    commandTree.subtree('/host', { usage: usage.host.toplevel })
+    commandTree.subtree('/auth', { usage: usage.auth.toplevel })
 
     const clicky = (parent, cmd, exec = repl.pexec) => {
         const dom = document.createElement('span')
@@ -185,22 +260,22 @@ module.exports = (commandTree, prequire) => {
     }
     const add = (_1, _2, _3, _4, _5, _6, argv) => addFn(firstArg(argv, 'add'))
 
-    const listCmd = commandTree.listen('/auth/list', list, { docs: 'List the OpenWhisk authorization keys you have installed' })
+    const listCmd = commandTree.listen('/auth/list', list, { usage: usage.auth.list })
     commandTree.synonym('/auth/ls', list, listCmd)
 
     const useFn = use(wsk, commandTree)
-    const useCmd = commandTree.listen('/auth/use', useFn('use'), { docs: 'Switch to use an OpenWhisk namespace, by name (hint: try auth ls first)' })
-    commandTree.synonym('/auth/switch', useFn('switch'), useCmd)
+    const useCmd = commandTree.listen('/auth/switch', useFn('switch'), { usage: usage.auth.switch })
+    // commandTree.synonym('/auth/switch', useFn('switch'), useCmd)
     //commandTree.synonym('/auth/use', useFn, useCmd)
 
-    const addCmd = commandTree.listen('/auth/add', add, { docs: 'Install an OpenWhisk authorization key' })
-    commandTree.synonym('/auth/install', use, addCmd)
+    const addCmd = commandTree.listen('/auth/add', add, { usage: usage.auth.add })
+    //commandTree.synonym('/auth/install', use, addCmd)
 
     /**
      * OpenWhisk API host: get and set commands
      *
      */
-    commandTree.listen('/host/get', () => wsk.apiHost.get(), { docs: 'Print the current OpenWhisk API host' })
+    commandTree.listen('/host/get', () => wsk.apiHost.get(), { usage: usage.host.get })
     commandTree.listen('/host/set',
                        (_1, _2, _a, _3, _4, _5, argv_without_options, options) => {
                            const argv = slice(argv_without_options, 'set')
@@ -241,7 +316,7 @@ module.exports = (commandTree, prequire) => {
                                host = 'https://192.168.33.13'
                                ignoreCerts = true
                                isLocal = true
-                           } else if (host === 'local') {
+                           } else if (host === 'local' || host === 'localhost') {
                                // try a variety of options
                                const variants = [ 'https://192.168.33.13', 'http://172.17.0.1:10001', 'http://192.168.99.100:10001' ]
                                const request = require('request')
@@ -324,7 +399,7 @@ module.exports = (commandTree, prequire) => {
                                }
                                })))
                        },
-                       { docs: 'Update the current OpenWhisk API host' })
+                       { usage: usage.host.set })
 
     return {
         add: addFn
diff --git a/tests/lib/common.js b/tests/lib/common.js
@@ -56,6 +56,9 @@ exports.before = (ctx, {fuzz, noApp=false}={}) => {
 	        // pass WSKNG_DEBUG on to NODE_DEBUG for the application
 	        opts.env.NODE_DEBUG = process.env.WSKNG_NODE_DEBUG
             }
+            if (process.env.DEBUG) {
+                opts.env.DEBUG = process.env.DEBUG
+            }
 
             ctx.app = new Application(opts)
         }
diff --git a/tests/tests/passes/02/host.js b/tests/tests/passes/02/host.js
@@ -39,6 +39,10 @@ describe('host tests', function() {
 
     it('should have an active repl', () => cli.waitForRepl(this.app))
 
+    it('should command not found on hosts set', () => cli.do('hosts set', this.app)
+       .then(cli.expectError(0, 'Command not found'))
+       .catch(common.oops(this)))
+
     it('bogus host from default context', () => cli.do(`host set xxx`, this.app)
 	.then(cli.expectOKWithCustom({selector: '', expect: `Before you can proceed, please provide an OpenWhisk auth key, using auth add <AUTH_KEY>` })))
 
diff --git a/tests/tests/passes/04/auth.js b/tests/tests/passes/04/auth.js
@@ -70,7 +70,7 @@ describe('auth tests', function() {
         .then(cli.expectContext(undefined, ''))) // don't care about command context (undefined), and selection must be empty ('')
 
     // switch back to second namespace
-    it('should switch to the second namespace, using the CLI use command', () => cli.do(`auth use ${ns2}`, this.app)
+    it('should switch to the second namespace, using the CLI switch command', () => cli.do(`auth switch ${ns2}`, this.app)
 	.then(cli.expectOKWithCustom({selector: '', expect: `namespace ${ns2}` })))
 
     // list should show only foo2

Original file line number	Diff line number	Diff line change
`@@ -56,6 +56,9 @@ exports.before = (ctx, {fuzz, noApp=false}={}) => {`
`56`	`56`	`// pass WSKNG_DEBUG on to NODE_DEBUG for the application`
`57`	`57`	`opts.env.NODE_DEBUG = process.env.WSKNG_NODE_DEBUG`
`58`	`58`	`}`
	`59`	`+ if (process.env.DEBUG) {`
	`60`	`+ opts.env.DEBUG = process.env.DEBUG`
	`61`	`+ }`
`59`	`62`
`60`	`63`	`ctx.app = new Application(opts)`
`61`	`64`	`}`