[WIP] LG activity copy behavior: copies all possible lg templates

[yeze322]

Description

Issues:

• Not all LG fields will be copied when copying an action. We should consider covering several kinds of LG activities:

  1. activity (used in SendActivity)
  2. prompt (BotAsks - XXXInput)
  3. invalidPrompt
  4. unrecognizedPrompt
  5. defaultValueResponse

• Resolves missed comments in Handle copy LG activity in visual editor #1096

Changes:

1. Include prompt fields when copying LG template
2. Move the copyLgTemplate handler's implementation from 'copyUtils.ts' in 'shared' lib to Shell (lgHandlers.ts)
3. Pass copyLgTemplate through ExtensionContainer to visual editor
4. Add UT for lgHandlers, update UT of copyUtils

Remaining issue:

• updateLgTemplate is not concurrency-safe
• removeLgTemplate is not concurrency-safe
  (addressed in LG api inside composer client is not concurrency-safe #1210 )

Task Item

fixes #1191
fixes #1213

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Code refactor (non-breaking change which improve code quality, clean up, add tests, etc)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Doc update (document update)

Checklist

• I have added tests that prove my fix is effective or that my feature works
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have functionally tested my change

Screenshots

Please include screenshots or gifs if your PR include UX changes.

[a-b-r-o-w-n]

please refer to my comments on #1096. It seems that was merged before my review was posted.

[yeze322]

I will work on Andy's comments today, #1196 & #1193 both of them. As for the data mutation problem, I want to solve it in another PR (also open an issue before solving it) by leveraging the JsonWalk handler @boydc2014 mentioned, @a-b-r-o-w-n what's your opinion on that?

[boydc2014]

I will work on Andy's comments today, #1196 & #1193 both of them. As for the data mutation problem, I want to solve it in another PR (also open an issue before solving it) by leveraging the JsonWalk handler @boydc2014 mentioned, @a-b-r-o-w-n what's your opinion on that?

I know you use copy then manipulate the data for a reason, it would save some visiting counts and in this context you only touch the filed. But as I said, data-manipulation when walk\travel\iterate is a very obvious anti-pattern, very easily to raise alert for reviewers, and you will need efforts to explain and justify, and maybe more efforts to prevent other contribute to follow this.

You can just copy JsonWalk into this shared/utils, without worrying about the server side, that's OK, i can clean up the server side to leverage the same one. But that's not the core design issue we are discussing here, it's that: can we avoid this anti-pattern easily? can we turn it into a object construction process by walking on another read-only object?

[yeze322]

Agreed, we can involve more changes in this PR to make the copy utils in a good pattern @boydc2014

[boydc2014]

@a-b-r-o-w-n @boydc2014 is this code what we want? An async version json walker which can deep copy json and generate new lg template.

// Implementation
export const JsonWalkAsync = async (
  path: string,
  value: any,
  visitor: (path: string, v: any) => Promise<any>,
  yielder: (path: string, v: any) => any = x => x
) => {
  let result: any;

  visitor(path, value);

  // extract array
  if (Array.isArray(value)) {
    result = await Promise.all(
      value.map(async (child, index) => {
        return await JsonWalkAsync(`${path}[:${index}]`, child, visitor, yielder);
      })
    );

    // extract object
  } else if (typeof value === 'object' && value) {
    const keys = Object.keys(value);
    const fieldResults: any[] = await Promise.all(
      keys.map(async key => {
        return await JsonWalkAsync(`${path}.${key}`, value[key], visitor, yielder);
      })
    );
    result = keys.reduce((acc, key, index) => {
      return {
        ...acc,
        [key]: fieldResults[index],
      };
    }, {});
  } else {
    result = await yielder(path, value);
  }

  return result;
};

// Usage
const copyLgTemplate = async (path: string, value: string) => {
  if (isLgTemplate(value)) {
    return Promise.resolve('[bfdactivity-1234]');
  } else {
    return value;
  }
};

JsonWalkAsync('$', someDialog, () => {/* stop visit if inside $deisgner object*/}, copyLgTemplate).then(copy => {});

Seems it's not as clean as the manipulation solution, we are doing deep copy and field replacing at the same time, meawhile result in a long promise chain. Do you have any advice on the implementation detail? Thanks

Actually a copy pattern don't even have to use JsonWalk, (jsonWalk is more suitable for your override approach)

A copy pattern can be simply achieved as a simple recursive bottom-up copy.

// define a map of copy functions, for example
async CopySendAcitivity(value, shellAPI) {
  // a pure function return a new value, the input is read-only
  return createNewAcitity(value, shellApi)
}

// High-order function design for IfCondition, SwitchElse
function CopyContainerObject([childrenProperties]) => 
return 
   async CopyIfConditon(value, shellAPI) {
     let result = clone(value);  // Optimization, don't clone children properties
     foreach (property in childrenProperites) {
          result[property] = [];
          foreach (idx in value[property])
           result result[idx] = await copyAction(value[property][idx]) // note here, reuse the copy action
          }
     }
   }
}

const copyMap = { 
  'Type.SendActivity': CopySendAcitivity 
  'Type.IfCondition':  CopyConatinerAction(['actions', 'elseActions'])
}

// search copy function, or fallback to lodash
function async copyAction(action, shellAPI) 
{
          if (value.type && value.type in copyMap) {
                return await copyMap[value.type](value, shellApi);
          } else {
                return = lodash.clone(value);
          }
     }
}

[a-b-r-o-w-n]

I like @boydc2014 proposed approach.

[boydc2014]

Let me also show JsonWalk works with copy construtor @yeze322 just for the purpose of illustration. You can compare this approach with the previous recusively copy

// section 1, first you don't have to add "yielder" because visitor is already a very complete abstraction, if we want async feature a simple async version is async on visitor. 
 
const jsonWalk = async (path, value, visitor) {
     ...
    await visitor(path, value);
    ...
    .... await jsonWalk()
}

// section 2 def a copy map like the previous approach

async CopySendAcitivity(value, shellAPI) {
  // a pure function return a new value, the input is read-only
  return createNewAcitity(value, shellApi)
}

const copyMap = { 
  'Type.SendActivity': CopySendAcitivity 
}

// section 3, def a visitor and walk
function copyAction(action, shellAPI) {

   let result = {};
   const visitor = (path, value) => {
       if (value.type and value.type) {
           // if there is a copy constructor defined, let it take over
           jsonPath.set(result, path, await copyMap[value.type](value, shellApi);
           return true;
       } else { 
          // other wise fallback
          jsonPath.set(result, path, lodash.clone(value));
          return false;
       }
   }

   return await jsonWalk('$', action, visitor);
}

So some points, to compare this to the above one

1. JsonWalk + visitor is generic enough here to handle this
2. In JsonWalk approach you don't have to define the copyFunction for (IfContion, Switch..) because jsonWalk contains the logic of recursively visiting (but you can always define that to take over)
3. In JsonWalk approach, actually you can even do DialogCopy, becaue it's generic enough
4. The downside of this approach of JsonWalk is this "jsonPath.set", because we are walking one tree to set another tree, and we don't really listen the in\out event of jsonWalk, so each jpath.set is a new path resolver. The typical solution to this is listener pattern.
5. Another downside of this top-down (pre-order travelsal) approach of jsonWalk is not efficient, we are creating\settings objects multiple times. Solution to this can be switching a generic post-order travel walk, but that way the interface can't be like jsonWalk and the pattern is not visitor pattern. and actually the previous solution is post-order traversal.

Anyway, no matter a generic jsonWalk or a simple recursively solution, i think i showed the principle that,

1. the function is pure and simple (except shellApi call)
2. the input of the function is read-only (not reference passing and data-manipulate)
3. based on 2, there is no data manipulate when visiting the same piece of data, you do have to create and assign, but you don't do that when you are visiting\walking the same data.
4. both approach show certain flexibility to customize certain layer, with the table driven approach.

To me, the first approach is good enough

[yeze322]

I like @boydc2014 proposed approach.

Me too, this solution looks like a compromise solution between mutation & json walk and is easy to be migrated to. The most significant benifit here is we can clearly manage how each $type of action is copied by customizing per schema.

3 pass! happy ending

[a-b-r-o-w-n]

Shouldn't this be an api call? Also, you are importing copyLgTemplate from the actions, but it does not conform to the action interface. This looks like a code smell to me.

Maybe copyLgTemplate is a shared utility, not an action? Also, passing in the get and update functions seem like a smell. Why is this dependency injection needed?

[yeze322]

Agree with you, copyLgTemplate should be an API but it's not provided by server. So I simulated an API here.

[a-b-r-o-w-n]

This file does not belong with the other actions.

[a-b-r-o-w-n]

Can this be typed?

[a-b-r-o-w-n]

We really need to add getLgTemplateByName to the lg api. We do this kind of thing in a few places!

[yeze322]

yes!

[a-b-r-o-w-n]

Might be safer to try/catch this and assign some other value.

[a-b-r-o-w-n]

😞 This has to do with the way typescript does enums at runtime.

[a-b-r-o-w-n]

You'll want to include EditActions as well.

[yeze322]

This PR requires longer time than we expected and may not meet the timeline. Keep it open for tracking those comments but split it into smaller PRs.

Plan:

1. Fix the P0 bug TextInput prompts not copied correctly #1191 [In PR: Copy other 3 prompts when copying TextInput #1281]
2. Optimize the copy constructor's implementation (as we discussed above)
3. Provide better LG api in Shell
4. Share common handlers with LG resource deletion (Relates to Fix LG resource delete in dialog #1145 )

[yeze322]

Want to start a discussion about how to leverage the same walker on both deleting & copying. @boydc2014 @a-b-r-o-w-n

The two scenarios are:

1. When user copys a nested action, traverse all child nodes, deep copy them along with LG templates.
2. When user deletes a nested action, traverse it, delete them along with LG templates

I think ideally they could and should share the same walker, but seems the solution we chose is not suitable for it. I'm considering the second solution Dong proposed:

Under this solution, we implement three parts:

1. a schema walker without return value
2. two visitors:
   • deleteVisitor: query lg templates fileld and call delete api
   • copyVisitor: copy the json -> deep copy lg -> insert to proper position based on json path (we can make sure the parent node always being constructed before children by using preorder traversal)
3. Function map about deletion / copying logic per $type (it will be coupled with lgApi for now)

Here is the interface. We've got enough implementation details so omit them

// Walker
function walker(path: string, input: AdaptiveAction, visitor: Function) {
  visitor(path, input);
  // traverse it.
  .....
}

// Function map
const handlerMap = {
  'Microsoft.IfCondition': {
      copy: () => {....},
      delete: () => {....},
  }
}

// Copy
const result = {}
function copyVisitor(path: string, input) {
  const copy = handlerMap(input.$type).copy(input);
  set(result, path, copy)
}

// Delete
function deleteVisitor(path: string, input) {
  handlerMap(input.$type).delete(input);
}

What's your opinion?

[boydc2014]

function walker(path: string, input: AdaptiveAction, visitor: Function) {

We don't have to share too much of between copy and delete. Because from the copy code, you can see, there are only 1 place inside "CopyContainerAction" which did a very simple foreach on property.

So, it's totally OK to me that, we register another deleteContainerAction. It would looks very clean, the only dup is just a foreach on property.

[yeze322]

We had a discussion offline

[zhixzhan]

This PR requires longer time than we expected and may not meet the timeline. Keep it open for tracking those comments but split it into smaller PRs.

Plan:

3. Provide better LG api in Shell
4. Share common handlers with LG resource deletion (Relates to Fix LG resource delete in dialog #1145 )

will follow up this, we can provide more methods like:

// existed
updateLgTemplate( file, templateName, template ) : void
createLgTemplate( file, template )  : void
deleteLgTemplate( file, templateName )  : void
// new
getLgTemplate( file, templateName )  : Template
copyLgTemplate( file, newTemplateName, template ) : Template
getLgTemplatesInBody( file, templateName): Teamplate[]
deleteLgTemplatesInBody( file, templateName) : void

[zhixzhan]

This PR requires longer time than we expected and may not meet the timeline. Keep it open for tracking those comments but split it into smaller PRs.
Plan:

1. Provide better LG api in Shell
2. Share common handlers with LG resource deletion (Relates to Fix LG resource delete in dialog #1145 )

will follow up this, we can provide more methods like:

// existed
updateLgTemplate( file, templateName, template ) : void
createLgTemplate( file, template )  : void
deleteLgTemplate( file, templateName )  : void
// new
getLgTemplate( file, templateName )  : Template
copyLgTemplate( file, newTemplateName, template ) : Template
getLgTemplatesInBody( file, templateName): Teamplate[]
deleteLgTemplatesInBody( file, templateName) : void

addressed in #1512, add:

1. copyTemplate(content: string, fromTemplateName: string, toTemplateName: string): string 

2. removeTemplates(content: string, templateNames: string[]): string

// if Name exist, throw error.
3. addTemplate(content: string, { Name, Parameters, Body }: Template): string

// if Name exist, add it anyway, with name like `${Name}1` `${Name}2`
4. addTemplateAnyway(
  content: string,
  { Name = 'TemplateName', Parameters = [], Body = '-TemplateBody' }: Template
): string

5. extractTemplateNames(text: string): string[]

6. getTemplate(content: string, templateName: string): LGTemplate | undefined