fix: lu update section issue (#2272)

* fix lu update section * escape # in lu section body * add test * upgrade package * handle newline * update package url Co-authored-by: Andy Brown <asbrown002@gmail.com>
microsoft · Mar 26, 2020 · 23a84cb · 23a84cb
1 parent c7ca778
commit 23a84cb
Show file tree

Hide file tree

Showing 10 changed files with 256 additions and 65 deletions.
diff --git a/Composer/packages/client/package.json b/Composer/packages/client/package.json
@@ -21,11 +21,11 @@
     "@bfc/indexers": "*",
     "@bfc/shared": "*",
     "@emotion/core": "^10.0.7",
-    "@microsoft/bf-lu": "4.8.0-preview.111952",
+    "@microsoft/bf-lu": "^4.9.0-preview.115707",
     "@reach/router": "^1.2.1",
+    "adaptive-expressions": "^4.8.0-preview-110700",
     "axios": "^0.18.0",
     "botbuilder-lg": "^4.8.0-preview-109324",
-    "adaptive-expressions": "^4.8.0-preview-110700",
     "format-message": "^6.2.3",
     "immer": "^5.2.0",
     "jwt-decode": "^2.2.0",
@@ -101,4 +101,4 @@
     "webpack-manifest-plugin": "2.1.0",
     "workbox-webpack-plugin": "4.3.1"
   }
-}
+}
diff --git a/Composer/packages/client/src/pages/notifications/index.tsx b/Composer/packages/client/src/pages/notifications/index.tsx
@@ -32,7 +32,7 @@ const Notifications: React.FC<RouteComponentProps> = () => {
       navigateTo(url);
     },
     [NotificationType.LU]: (item: INotification) => {
-      let uri = `/bot/${projectId}/language-understanding/${item.id}`;
+      let uri = `/bot/${projectId}/language-understanding/${item.id}/edit#L=${item.diagnostic.range?.start.line || 0}`;
       if (item.dialogPath) {
         uri = convertPathToUrl(item.id, item.dialogPath);
       }

diff --git a/Composer/packages/lib/indexers/__tests__/luUtil.test.ts b/Composer/packages/lib/indexers/__tests__/luUtil.test.ts
@@ -1,7 +1,7 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 
-import { sectionHandler } from '@microsoft/bf-lu/lib/parser';
+import { sectionHandler } from '@microsoft/bf-lu/lib/parser/composerindex';
 
 import { updateIntent, addIntent, removeIntent } from '../src/utils/luUtil';
 
@@ -15,6 +15,19 @@ describe('LU Section CRUD test', () => {
 # CheckEmail
 - check my email
 - show my emails
+`;
+
+  const fileContentError1 = `# Greeting
+> not start with  -
+hi
+- hello
+
+# CheckEmail
+- check my email
+- show my emails
+
+# Foo
+> nothing in body
 `;
 
   it('parse section test', () => {
@@ -32,6 +45,17 @@ describe('LU Section CRUD test', () => {
     expect(luresource.Sections[0].UtteranceAndEntitiesMap[1].utterance).toEqual('hello');
   });
 
+  it('parse section with syntax error test', () => {
+    const luresource = luParser.parse(fileContentError1);
+    const { Sections, Errors, Content } = luresource;
+
+    expect(Content).toEqual(fileContentError1);
+    expect(Errors.length).toEqual(2);
+    expect(Sections.length).toEqual(3);
+    expect(Sections[0].Errors.length).toEqual(1);
+    expect(Sections[2].Errors.length).toEqual(1);
+  });
+
   it('add simpleIntentSection test', () => {
     const intent = {
       Name: 'CheckUnreadEmail',
@@ -79,7 +103,7 @@ describe('LU Section CRUD test', () => {
     expect(luresource.Sections[1].UtteranceAndEntitiesMap[2].utterance).toEqual('check my mail box please');
   });
 
-  it('update section with invalid lu syntax', () => {
+  it('update section with syntax error: missing -', () => {
     const intentName = 'CheckEmail';
 
     const validFileContent = `#CheckEmail
@@ -93,10 +117,31 @@ describe('LU Section CRUD test', () => {
 `,
     };
 
-    const invalidFileContent = `#CheckEmail
-- check my email
+    const invalidIntent = {
+      Name: 'CheckEmail',
+      Body: `check my email
 - show my emails
-@`;
+`,
+    };
+
+    // when intent invalid, after update can still be parsed
+    const updatedContent2 = updateIntent(validFileContent, intentName, invalidIntent);
+    const updatedContent2Parsed = luParser.parse(updatedContent2);
+    expect(updatedContent2Parsed.Sections.length).toEqual(1);
+    expect(updatedContent2Parsed.Errors.length).toBeGreaterThan(0);
+    // when file invalid, update with valid intent should fix error.
+    const updatedContent3 = updateIntent(updatedContent2, intentName, validIntent);
+    const updatedContent3Parsed = luParser.parse(updatedContent3);
+    expect(updatedContent3Parsed.Sections.length).toEqual(1);
+    expect(updatedContent3Parsed.Errors.length).toEqual(0);
+  });
+
+  it('update section with syntax error: end with empty entity @', () => {
+    const intentName = 'CheckEmail';
+
+    const validFileContent = `#CheckEmail
+- check my email
+- show my emails`;
 
     const invalidIntent = {
       Name: 'CheckEmail',
@@ -105,23 +150,39 @@ describe('LU Section CRUD test', () => {
 @`,
     };
 
-    const invalidIntent4 = {
+    // when intent invalid, after update can still be parsed
+    const updatedContent2 = updateIntent(validFileContent, intentName, invalidIntent);
+    const updatedContent2Parsed = luParser.parse(updatedContent2);
+    expect(updatedContent2Parsed.Errors.length).toBeGreaterThan(0);
+    // TODO: update back should fix error.
+    // const updatedContent3 = updateIntent(updatedContent2, intentName, validIntent);
+    // expect(updatedContent3).toEqual(validFileContent);
+  });
+
+  it('update section with syntax error: include # IntentName in body', () => {
+    const intentName = 'CheckEmail';
+
+    const validFileContent = `#CheckEmail
+- check my email
+- show my emails`;
+
+    const invalidIntent = {
       Name: 'CheckEmail',
       Body: `- check my email
 - show my emails
 
 # UnexpectedIntentDefination
-- unexpected intent body
 `,
     };
-    // intent invalid
+    // should auto escape # to \#
     const updatedContent2 = updateIntent(validFileContent, intentName, invalidIntent);
-    expect(updatedContent2).toEqual(validFileContent);
-    const updatedContent4 = updateIntent(validFileContent, intentName, invalidIntent4);
-    expect(updatedContent4).toEqual(validFileContent);
-    // file invalid
-    const updatedContent3 = updateIntent(invalidFileContent, intentName, validIntent);
-    expect(updatedContent3).toEqual(invalidFileContent);
+    const { Sections, Errors } = luParser.parse(updatedContent2);
+    expect(Errors.length).toEqual(0);
+    expect(Sections.length).toEqual(1);
+    expect(Sections[0].Errors.length).toEqual(0);
+    expect(Sections[0].SectionType).toEqual(luSectionTypes.SIMPLEINTENTSECTION);
+    expect(Sections[0].UtteranceAndEntitiesMap.length).toEqual(3);
+    expect(Sections[0].UtteranceAndEntitiesMap[2].utterance).toEqual('\\# UnexpectedIntentDefination');
   });
 
   it('delete section test', () => {
@@ -296,6 +357,80 @@ describe('LU Nested Section CRUD test', () => {
     );
   });
 
+  it('update nestedIntentSection and escape # in body', () => {
+    const intentName = 'CheckTodo/CheckUnreadTodo';
+    const intent = {
+      Name: 'CheckMyUnreadTodo',
+      Body: `- please check my unread todo
+- please show my unread todos
+# Oops
+## Oops
+@ simple todoTitle
+@ simple todoContent
+`,
+    };
+
+    const fileContentUpdated = updateIntent(fileContent, intentName, intent);
+    const luresource = luParser.parse(fileContentUpdated);
+    const { Sections, Errors } = luresource;
+
+    expect(Errors.length).toEqual(0);
+    expect(Sections.length).toEqual(2);
+    expect(Sections[0].SectionType).toEqual(luSectionTypes.MODELINFOSECTION);
+    expect(Sections[1].SectionType).toEqual(luSectionTypes.NESTEDINTENTSECTION);
+    expect(Sections[1].Name).toEqual('CheckTodo');
+    expect(Sections[1].SimpleIntentSections.length).toEqual(2);
+    expect(Sections[1].SimpleIntentSections[0].Name).toEqual('CheckMyUnreadTodo');
+    expect(Sections[1].SimpleIntentSections[0].SectionType).toEqual(luSectionTypes.SIMPLEINTENTSECTION);
+    expect(Sections[1].SimpleIntentSections[0].Errors.length).toEqual(0);
+    expect(Sections[1].SimpleIntentSections[0].Entities.length).toEqual(2);
+    expect(Sections[1].SimpleIntentSections[0].Entities[1].Name).toEqual('todoContent');
+    expect(Sections[1].SimpleIntentSections[0].UtteranceAndEntitiesMap.length).toEqual(4);
+    expect(Sections[1].SimpleIntentSections[0].UtteranceAndEntitiesMap[0].utterance).toEqual(
+      'please check my unread todo'
+    );
+    expect(Sections[1].SimpleIntentSections[0].UtteranceAndEntitiesMap[2].utterance).toEqual('\\# Oops');
+    expect(Sections[1].SimpleIntentSections[0].UtteranceAndEntitiesMap[3].utterance).toEqual('\\## Oops');
+  });
+
+  it('update nestedIntentSection with # ## ### in body', () => {
+    const intentName = 'CheckTodo';
+    const intentBody1 = `# Oops
+## Oops
+### Oops
+`;
+
+    const fileContentUpdated1 = updateIntent(fileContent, intentName, { Name: intentName, Body: intentBody1 });
+    const luresource1 = luParser.parse(fileContentUpdated1);
+    expect(luresource1.Sections.length).toBeGreaterThan(0);
+    expect(luresource1.Errors.length).toBeGreaterThan(0);
+
+    const intentBody2 = `## Oops
+    ### Oops
+    `;
+    const fileContentUpdated2 = updateIntent(fileContent, intentName, { Name: intentName, Body: intentBody2 });
+    const luresource2 = luParser.parse(fileContentUpdated2);
+    expect(luresource2.Sections.length).toEqual(2);
+    expect(luresource2.Errors.length).toBeGreaterThan(0);
+    expect(luresource2.Sections[0].SectionType).toEqual(luSectionTypes.MODELINFOSECTION);
+    expect(luresource2.Sections[1].SectionType).toEqual(luSectionTypes.NESTEDINTENTSECTION);
+
+    // if nestedSection not enable
+    const fileContent3 = `# Greeting
+- hi
+
+# CheckTodo
+- please check my todo
+`;
+    const intentBody3 = `## Oops
+### Oops
+`;
+    const fileContentUpdated3 = updateIntent(fileContent3, intentName, { Name: intentName, Body: intentBody3 });
+    const luresource3 = luParser.parse(fileContentUpdated3);
+    expect(luresource3.Sections.length).toBeGreaterThan(0);
+    expect(luresource3.Errors.length).toBeGreaterThan(0);
+  });
+
   /**
    * this will add #CheckMyTodo
    * in #CheckMyTodo, ##CheckUnreadTodo not exist, then will do add ##CheckMyUnreadTodo

diff --git a/Composer/packages/lib/indexers/package.json b/Composer/packages/lib/indexers/package.json
@@ -29,9 +29,9 @@
   },
   "dependencies": {
     "@bfc/shared": "*",
-    "@microsoft/bf-lu": "4.8.0-preview.111952",
-    "botbuilder-lg": "^4.8.0-preview-109324",
+    "@microsoft/bf-lu": "^4.9.0-preview.115707",
     "adaptive-expressions": "^4.8.0-preview-110700",
+    "botbuilder-lg": "^4.8.0-preview-109324",
     "lodash": "^4.17.15"
   }
-}
+}
diff --git a/Composer/packages/lib/indexers/src/luIndexer.ts b/Composer/packages/lib/indexers/src/luIndexer.ts
@@ -21,8 +21,8 @@ function convertLuDiagnostic(d: any, source: string): Diagnostic {
   };
   const result = new Diagnostic(d.Message, source, severityMap[d.Severity]);
 
-  const start: Position = new Position(d.Range.Start.Line, d.Range.Start.Character);
-  const end: Position = new Position(d.Range.End.Line, d.Range.End.Character);
+  const start: Position = d.Range ? new Position(d.Range.Start.Line, d.Range.Start.Character) : new Position(0, 0);
+  const end: Position = d.Range ? new Position(d.Range.End.Line, d.Range.End.Character) : new Position(0, 0);
   result.range = new Range(start, end);
 
   return result;

diff --git a/Composer/packages/lib/indexers/src/utils/help.ts b/Composer/packages/lib/indexers/src/utils/help.ts
@@ -5,3 +5,25 @@ export function getBaseName(filename: string, sep?: string): string | any {
   if (sep) return filename.substr(0, filename.lastIndexOf(sep));
   return filename.substring(0, filename.lastIndexOf('.')) || filename;
 }
+
+// split text to lines
+export function splitNewlineText(text: string): string[] {
+  return text.split('\n');
+}
+
+// convert lines to \r\n text
+export function buildNewlineText(lineArray: string[]): string {
+  if (lineArray.length < 2) return lineArray.join('');
+  const lastLine = lineArray.pop();
+  const linesWithRN = lineArray.map(line => {
+    if (line.endsWith('\r\n')) {
+      return line;
+    } else if (line.endsWith('\r')) {
+      return line + '\n';
+    } else {
+      return line + '\r\n';
+    }
+  });
+  const lastLineWithRN = lastLine && lastLine.endsWith('\r') ? lastLine + '\n' : lastLine;
+  return [...linesWithRN, lastLineWithRN].join('');
+}