fix(packages/core): process clientRequired plugins before clientHosted plugins

Fixes #3191

Author: starpit

packages/core/src/core/command-tree.ts

@@ -20,10 +20,12 @@ debug('loading')
 
 import {
   CommandHandler,
+  CommandOverrideHandler,
   CommandRegistrar,
   CommandTree,
   CommandTreeResolution,
   ExecType,
+  EvaluatorArgs,
   CatchAllOffer,
   Command,
   CommandHandlerWithEvents,
@@ -415,7 +417,7 @@ const _read = async (
     // loaded, yet; so: invoke the plugin resolver and retry
     //
     const route = `/${argv.join('/')}`
-    await resolver.resolve(route)
+    await resolver.resolve(route, { tryCatchalls: !contextRetry || contextRetry.length === 0 })
     leaf = treeMatch(getModelInternal().root, argv, true) // true means read-only, don't modify the context model please
     evaluator = leaf && leaf.$
   }
@@ -550,7 +552,7 @@ export const read = async (
   let cmd: false | CodedError | CommandHandlerWithEvents
 
   if (!noRetry) {
-    await resolver.resolve(`/${argv.join('/')}`)
+    await resolver.resolve(`/${argv.join('/')}`, { tryCatchalls: false })
     cmd = await internalRead(root, argv)
   }
 
@@ -627,11 +629,30 @@ export class ImplForPlugins implements CommandRegistrar {
     return _subtreeSynonym(route, master, options)
   }
 
-  public async find(route: string, noOverride = true) {
+  public async override(
+    route: string,
+    fromPlugin: string,
+    overrideHandler: CommandOverrideHandler,
+    options?: CommandOptions
+  ): Promise<Command> {
+    const currentHandler = (await this.find(route, fromPlugin)).$
+    if (!currentHandler) {
+      throw new Error(`Cannot find desired command handler for ${route} from plugin ${fromPlugin}`)
+    }
+
+    const handler: CommandHandler = (args: EvaluatorArgs) => overrideHandler(args, currentHandler)
+    return this.listen(route, handler, options)
+  }
+
+  public async find(route: string, fromPlugin?: string, noOverride = true) {
     const cmd = match(route.split('/').slice(1), true)
     if (!cmd || cmd.route !== route || (!noOverride && resolver && resolver.isOverridden(cmd.route))) {
       if (resolver) {
-        await resolver.resolve(route)
+        if (fromPlugin) {
+          await resolver.resolveOne(fromPlugin)
+        } else {
+          await resolver.resolve(route, { tryCatchalls: false })
+        }
       }
       return match(route.split('/').slice(1), true)
     } else {

packages/core/src/models/command.ts

@@ -170,6 +170,12 @@ export type Response = Entity
 /** base command handler */
 export type CommandHandler<T = Response, O = ParsedOptions> = (args: EvaluatorArgs<O>) => T | Promise<T>
 
+/** command handler when overriding commands from other plugins */
+export type CommandOverrideHandler<T = Response, O = ParsedOptions> = (
+  args: EvaluatorArgs<O>,
+  underlyingHandler: CommandHandler<T, O>
+) => T | Promise<T>
+
 /**
  * Evaluator
  *
@@ -255,8 +261,14 @@ export interface CatchAllHandler extends Command {
 type CommandListener = (route: string, handler: CommandHandler, options?: CommandOptions) => Command
 
 export interface CommandRegistrar {
-  find: (route: string, noOverride?: boolean) => Promise<Command>
+  find: (route: string, fromPlugin?: string, noOverride?: boolean) => Promise<Command>
   listen: CommandListener
+  override: (
+    route: string,
+    fromPlugin: string,
+    handler: CommandOverrideHandler,
+    options?: CommandOptions
+  ) => Promise<Command>
   synonym: (route: string, handler: CommandHandler, master: Command, options?: CommandOptions) => void
   subtree: (route: string, options: CommandOptions) => Command
   subtreeSynonym: (route: string, masterTree: Command, options?: CommandOptions) => void

packages/core/src/plugins/resolver.ts

@@ -25,11 +25,35 @@ import { KuiPlugin, PluginRegistration } from '../models/plugin'
 const debug = Debug('core/plugins/resolver')
 
 export interface PluginResolver {
-  resolve: (route: string, options?: { subtree: boolean }) => void
+  /**
+   * Look for a plugin that might be able to handle the given
+   * `route`. If `tryCatchalls` is true, then the caller is desperate,
+   * and wants us to see if there are any plugins that might have a
+   * catchall that could possibly service the given `route`.
+   *
+   */
+  resolve: (route: string, options?: { subtree?: boolean; tryCatchalls: boolean }) => void
+
   disambiguate: (route: string) => CommandBase[]
   disambiguatePartial: (partial: string) => string[]
-  resolveOne: (route: string) => Promise<KuiPlugin>
+
+  /**
+   * Unconditionally resolve the given named `plugin`
+   *
+   */
+  resolveOne: (plugin: string) => Promise<KuiPlugin>
+
+  /**
+   * Has the given `route` been overridden by some plugin?
+   *
+   */
   isOverridden: (route: string) => boolean
+
+  /**
+   * Is the given `plugin` the definitive master of the given `route`?
+   * it might not be, if some other plugin has overridden it
+   */
+  isAlpha: (route: string, plugin: string) => boolean
 }
 
 /** export the prequire function */
@@ -120,6 +144,8 @@ export const makeResolver = (prescan: PrescanModel, registrar: Record<string, Ku
   /** a plugin resolver impl */
   const resolver = {
     isOverridden: (route: string): boolean => prescan.overrides[route] !== undefined,
+    isAlpha: (route: string, plugin: string): boolean =>
+      prescan.overrides[route] === undefined || prescan.overrides[route] === plugin,
 
     resolveOne,
 
@@ -143,7 +169,7 @@ export const makeResolver = (prescan: PrescanModel, registrar: Record<string, Ku
     },
 
     /** load any plugins required by the given command */
-    resolve: async (command: string, { subtree = false } = {}): Promise<void> => {
+    resolve: async (command: string, { subtree = false, tryCatchalls = true } = {}): Promise<void> => {
       // subpath if we are looking for plugins for a subtree, e.g. for cd /auth
       let plugin: string
       let matchLen: number
@@ -160,7 +186,7 @@ export const makeResolver = (prescan: PrescanModel, registrar: Record<string, Ku
       }
       if (plugin) {
         await resolveOne(plugin)
-      } else if (prescan.catchalls.length > 0) {
+      } else if (prescan.catchalls.length > 0 && tryCatchalls) {
         // see if we have catchall
         await Promise.all(prescan.catchalls.map(_ => resolveOne(_.plugin))).catch(err => {
           console.error(

packages/core/src/plugins/scanner.ts

@@ -357,9 +357,14 @@ const resolveFromLocalFilesystem = async (scanCache: ScanCache, opts: PrescanOpt
       }
     }
 
-    // we take the union of the client-provided plugins and the client-required plugins
-    plugins = Object.assign({}, clientHosted.plugins, clientRequired.plugins)
-    preloads = Object.assign({}, clientHosted.preloads, clientRequired.preloads)
+    // we take the union of the client-provided plugins and the
+    // client-required plugins note: place clientRequired first, so
+    // that we process the registrations in clientRequired plugins
+    // first (this processing occurs in topologicalSortForScan);
+    // nodejs guarantees FIFO iteration order for object properties
+    // https://github.com/IBM/kui/issues/3191
+    plugins = Object.assign({}, clientRequired.plugins, clientHosted.plugins)
+    preloads = Object.assign({}, clientRequired.preloads, clientHosted.preloads)
   }
 
   debug('availablePlugins %s', JSON.stringify(plugins))

plugins/plugin-apache-composer/src/lib/controller/cmd/session-get.ts

@@ -79,38 +79,40 @@ export default async (commandTree: Commands.Registrar) => {
     { usage: sessionGet('get') }
   )
 
-  // override wsk activation get
-  const activationGet = (await commandTree.find('/wsk/activation/get')).$
-  synonyms('activations').forEach(syn => {
-    commandTree.listen(
-      `/wsk/${syn}/get`,
-      opts => {
-        if (!activationGet) {
-          return Promise.reject(new Error())
-        }
-        const last = opts.parsedOptions.last
+  await Promise.all(
+    synonyms('activations').map(syn => {
+      // override wsk activation get
+      return commandTree.override(
+        `/wsk/${syn}/get`,
+        'plugin-openwhisk',
+        (opts, activationGet) => {
+          if (!activationGet) {
+            return Promise.reject(new Error())
+          }
+          const last = opts.parsedOptions.last
 
-        if (last) {
-          return opts.REPL.qexec<ActivationListTable>(
-            `wsk activation list --limit 1` + (typeof last === 'string' ? ` --name ${last}` : '')
-          )
-            .then(activations => activations.body)
-            .then(activations => {
-              if (activations.length === 0) {
-                throw new Error('No such activation found')
-              } else {
-                return opts.REPL.qexec<Activation>(`wsk activation get ${activations[0].activationId}`)
-              }
-            })
-        }
+          if (last) {
+            return opts.REPL.qexec<ActivationListTable>(
+              `wsk activation list --limit 1` + (typeof last === 'string' ? ` --name ${last}` : '')
+            )
+              .then(activations => activations.body)
+              .then(activations => {
+                if (activations.length === 0) {
+                  throw new Error('No such activation found')
+                } else {
+                  return opts.REPL.qexec<Activation>(`wsk activation get ${activations[0].activationId}`)
+                }
+              })
+          }
 
-        return Promise.resolve(activationGet(opts))
-          .then(response => view.formatSessionGet(opts, response))
-          .catch(err => {
-            throw err
-          })
-      },
-      {}
-    )
-  })
+          return Promise.resolve(activationGet(opts))
+            .then(response => view.formatSessionGet(opts, response))
+            .catch(err => {
+              throw err
+            })
+        },
+        {}
+      )
+    })
+  )
 }

plugins/plugin-openwhisk-editor-extensions/src/lib/cmds/new.ts

@@ -110,7 +110,7 @@ const checkForConformance = action => {
     const err = new Error('Editing of binary actions not yet supported')
     err['code'] = 406 // 406: Not Acceptable http status code
     throw err
-  } else if (action.ast) {
+  } else if (action.ast || action.annotations.find(_ => _.key === 'composerVersion')) {
     // try to find the source for this composition
     debug('trying to find source for composition')
     return compositionPersister.getCode(action)