Allow intent or entities using phraseList features of same name (#1089)

* support output to file for kb:export command

* support name duplication of intent or entity with phraseList

* fix comments and typo

packages/lu/src/parser/lufile/parseFileContents.js

@@ -479,9 +479,10 @@ const validateFeatureAssignment = function(srcItemType, srcItemName, tgtFeatureT
  * @param {String} featureType
  * @param {Object} range
  */
-const addFeatures = function(tgtItem, feature, featureType, range, featureProperties) {
+const addFeatures = function(tgtItem, feature, featureType, range, featureProperties, featureIsPhraseList = false) {
     // target item cannot have the same name as the feature name
-    if (tgtItem.name === feature) {
+    // the only exception case is intent and entity can have same name with phraseList feature
+    if (tgtItem.name === feature && !featureIsPhraseList) {
         // Item must be defined before being added as a feature.
         let errorMsg = `Source and target cannot be the same for usesFeature. e.g. x usesFeature x  is invalid. "${tgtItem.name}" usesFeature "${feature}" is invalid.`;
         let error = BuildDiagnostic({
@@ -490,11 +491,12 @@ const addFeatures = function(tgtItem, feature, featureType, range, featureProper
         })
         throw (new exception(retCode.errorCode.INVALID_INPUT, error.toString(), [error]));
     }
-    let featureAlreadyDefined = (tgtItem.features || []).find(item => item.modelName == feature || item.featureName == feature);
+    let featureToModelAlreadyDefined = (tgtItem.features || []).find(item => item.featureName == feature);
+    let modelToFeatureAlreadyDefined = (tgtItem.features || []).find(item => item.modelName == feature);
     switch (featureType) {
         case featureTypeEnum.featureToModel: {
             if (tgtItem.features) {
-                if (!featureAlreadyDefined) tgtItem.features.push(new helperClass.featureToModel(feature, featureProperties));
+                if (!featureToModelAlreadyDefined) tgtItem.features.push(new helperClass.featureToModel(feature, featureProperties));
             } else {
                 tgtItem.features = new Array(new helperClass.featureToModel(feature, featureProperties));
             }
@@ -502,7 +504,9 @@ const addFeatures = function(tgtItem, feature, featureType, range, featureProper
         }
         case featureTypeEnum.modelToFeature: {
             if (tgtItem.features) {
-                if (!featureAlreadyDefined) tgtItem.features.push(new helperClass.modelToFeature(feature, featureProperties));
+                if (!modelToFeatureAlreadyDefined){
+                    tgtItem.features.push(new helperClass.modelToFeature(feature, featureProperties));
+                } 
             } else {
                 tgtItem.features = new Array(new helperClass.modelToFeature(feature, featureProperties));
             }
@@ -543,14 +547,31 @@ const parseFeatureSections = function(parsedContent, featuresToProcess, config)
             if (intentExists !== undefined) {
                 // verify the list of features requested have all been defined.
                 let featuresList = section.Features.split(/[,;]/g).map(item => item.trim().replace(/^[\'\"]|[\'\"]$/g, ""));
+                let featuresVisited = new Set();
                 (featuresList || []).forEach(feature => {
-                    let entityExists = (parsedContent.LUISJsonStructure.flatListOfEntityAndRoles || []).find(item => item.name == feature || item.name == `${feature}(interchangeable)`);
+                    // usually phraseList has higher priority when searching the existing entities as phraseList is more likely to be used as feature
+                    // but when a ml entity and a phraseList have same name, this assumption will cause a problem in situation that
+                    // users actually want to specify the ml entity as feature rather than phraseList
+                    // currently we can not distinguish them in lu file, as for @ intent A usesFeature B, B can be a ml entity or a phraseList with same name
+                    // the lu format for usesFeatures defintion need to be updated if we want to resolve this confusion completely
+                    let entityExists = (parsedContent.LUISJsonStructure.flatListOfEntityAndRoles || []).find(item => (item.name == feature || item.name == `${feature}(interchangeable)`) && item.type == EntityTypeEnum.PHRASELIST);
+                    // if phraseList is not matched to the feature, search other non phraseList entities
+                    // or this intent use multiple features with same name, e.g., @ intent A usesFeatures B, B.
+                    // and current loop is processiong the second feature B
+                    // this is allowed in luis portal, you can add ml entity and phraseList features of same name to an intent                    
+                    // the exported intent useFeatures will have two features with same name listed, just like above sample @ intent A usesFeatures B, B
+                    if (!entityExists || featuresVisited.has(feature)) {
+                        entityExists = (parsedContent.LUISJsonStructure.flatListOfEntityAndRoles || []).find(item => item.name == feature && item.type !== EntityTypeEnum.PHRASELIST);
+                    }
+
+                    if (!featuresVisited.has(feature)) featuresVisited.add(feature);
+
                     let featureIntentExists = (parsedContent.LUISJsonStructure.intents || []).find(item => item.name == feature);
                     if (entityExists) {
                         if (entityExists.type === EntityTypeEnum.PHRASELIST) {
                             // de-dupe and add features to intent.
                             validateFeatureAssignment(section.Type, section.Name, entityExists.type, feature, section.Range);
-                            addFeatures(intentExists, feature, featureTypeEnum.featureToModel, section.Range, featureProperties.phraseListFeature);
+                            addFeatures(intentExists, feature, featureTypeEnum.featureToModel, section.Range, featureProperties.phraseListFeature, true);
                             // set enabledForAllModels on this phrase list
                             let plEnity = parsedContent.LUISJsonStructure.model_features.find(item => item.name == feature);
                             if (plEnity.enabledForAllModels === undefined) plEnity.enabledForAllModels = false;
@@ -588,9 +609,26 @@ const parseFeatureSections = function(parsedContent, featuresToProcess, config)
                 // Find the source entity from the collection and get its type
                 let srcEntityInFlatList = (parsedContent.LUISJsonStructure.flatListOfEntityAndRoles || []).find(item => item.name == section.Name);
                 let entityType = srcEntityInFlatList ? srcEntityInFlatList.type : undefined;
+                let featuresVisited = new Set();
                 (featuresList || []).forEach(feature => {
                     feature = feature.replace(/[\'\"]/g, "");
-                    let featureExists = (parsedContent.LUISJsonStructure.flatListOfEntityAndRoles || []).find(item => item.name == feature || item.name == `${feature}(interchangeable)`);
+                    // usually phraseList has higher priority when searching the existing entities as phraseList is more likely to be used as feature to a entity
+                    // but when a ml entity and a phraseList have same name, this assumption will cause a problem in situation that
+                    // users actually want to specify the ml entity as feature rather than phraseList
+                    // currently we can not distinguish them in lu file, as for @ ml A usesFeature B, B can be another ml entity or a phraseList with same name
+                    // the lu format for usesFeatures defintion need to be updated if we want to resolve this confusion completely
+                    let featureExists = (parsedContent.LUISJsonStructure.flatListOfEntityAndRoles || []).find(item => (item.name == feature || item.name == `${feature}(interchangeable)`) && item.type == EntityTypeEnum.PHRASELIST);
+                    // if phraseList is not matched to the feature, search other non phraseList entities
+                    // or this intent use multiple features with same name, e.g., @ intent A usesFeatures B, B.
+                    // and current loop is processiong the second feature B
+                    // this is allowed in luis portal, you can add ml entity and phraseList features of same name to an intent                    
+                    // the exported intent useFeatures will have two features with same name listed, just like above sample @ intent A usesFeatures B, B
+                    if (!featureExists || featuresVisited.has(feature)) {
+                        featureExists = (parsedContent.LUISJsonStructure.flatListOfEntityAndRoles || []).find(item => item.name == feature && item.type !== EntityTypeEnum.PHRASELIST);
+                    }
+
+                    if (!featuresVisited.has(feature)) featuresVisited.add(feature);
+
                     let featureIntentExists = (parsedContent.LUISJsonStructure.intents || []).find(item => item.name == feature);
                     // find the entity based on its type.
                     let srcEntity = (parsedContent.LUISJsonStructure[luisEntityTypeMap[entityType]] || []).find(item => item.name == section.Name);
@@ -598,7 +636,7 @@ const parseFeatureSections = function(parsedContent, featuresToProcess, config)
                         if (featureExists.type === EntityTypeEnum.PHRASELIST) {
                             // de-dupe and add features to intent.
                             validateFeatureAssignment(entityType, section.Name, featureExists.type, feature, section.Range);
-                            addFeatures(srcEntity, feature, featureTypeEnum.featureToModel, section.Range, featureProperties.phraseListFeature);
+                            addFeatures(srcEntity, feature, featureTypeEnum.featureToModel, section.Range, featureProperties.phraseListFeature, true);
                             // set enabledForAllModels on this phrase list
                             let plEnity = parsedContent.LUISJsonStructure.model_features.find(item => item.name == feature);
                             if (plEnity.enabledForAllModels === undefined) plEnity.enabledForAllModels = false;
@@ -635,14 +673,15 @@ const updateDependencyList = function(type, parsedContent, dependencyList) {
         let srcName = itemOfType.name;
         let copySrc, copyValue;
         if (itemOfType.features) {
-            (itemOfType.features || []).forEach(feature => {
-                if (feature.modelName) feature = feature.modelName;
-                if (feature.featureName) feature = feature.featureName;
+            (itemOfType.features || []).forEach(featureObj => {
+                let feature = featureObj.modelName ? featureObj.modelName : featureObj.featureName;
+                let type = featureObj.modelType ? featureObj.modelType : featureObj.featureType;
+
                 // find any items where this feature is the target
-                let featureDependencyEx = dependencyList.filter(item => srcName == (item.value ? item.value.slice(-1)[0] : undefined));
+                let featureDependencyEx = dependencyList.filter(item => srcName == (item.value ? item.value.slice(-1)[0].feature : undefined));
                 (featureDependencyEx || []).forEach(item => {
                     item.key = `${item.key.split('::')[0]}::${feature}`;
-                    item.value.push(feature);
+                    item.value.push({feature, type});
                 })
                 // find any items where this feature is the source
                 featureDependencyEx = dependencyList.find(item => feature == (item.value ? item.value.slice(0)[0] : undefined));
@@ -653,7 +692,7 @@ const updateDependencyList = function(type, parsedContent, dependencyList) {
                 let dependencyExists = dependencyList.find(item => item.key == `${srcName}::${feature}`);
                 if (!dependencyExists) {
                     let lKey = copySrc ? `${srcName}::${copySrc}` : `${srcName}::${feature}`;
-                    let lValue = [srcName, feature];
+                    let lValue = [srcName, {feature, type}];
                     if (copyValue) copyValue.forEach(item => lValue.push(item));
                     dependencyList.push({
                         key : lKey,
@@ -661,11 +700,15 @@ const updateDependencyList = function(type, parsedContent, dependencyList) {
                     })
                 } else {
                     dependencyExists.key = `${dependencyExists.key.split('::')[0]}::${feature}`;
-                    dependencyExists.value.push(feature);
+                    dependencyExists.value.push({feature, type});
                 }
-                let circularItemFound = dependencyList.find(item => item.value && item.value.slice(0)[0] == item.value.slice(-1)[0]);
-                if (circularItemFound) {
-                    const errorMsg = `Circular dependency found for usesFeature. ${circularItemFound.value.join(' -> ')}`;
+
+                let circularItemFound = dependencyList.find(item => item.value
+                    && item.value.slice(0)[0] == item.value.slice(-1)[0].feature
+                    && item.value.slice(-1)[0].type !== featureProperties.phraseListFeature);
+
+                    if (circularItemFound) {
+                    const errorMsg = `Circular dependency found for usesFeature. ${circularItemFound.value.map(v => v.feature ? v.feature : v).join(' -> ')}`;
                     const error = BuildDiagnostic({
                         message: errorMsg
                     });

packages/lu/src/parser/utils/helpers.js

@@ -561,11 +561,14 @@ const addIsRequiredProperty = function(item, phraseListInFinal = []) {
     (item.features || []).forEach(feature => {
         if (feature.isRequired === undefined)
             feature.isRequired = false;
-        if (feature.modelName !== undefined && phraseListInFinal.includes(feature.modelName))
-        {
+
+        if (feature.modelName !== undefined
+            && phraseListInFinal.includes(feature.modelName)
+            && !item.features.find(fea => fea.featureName == feature.modelName)) {
             feature.featureName = feature.modelName;
             delete feature.modelName;
-        }    
+        }
+
         delete feature.featureType;
         delete feature.modelType;
     });

packages/lu/test/parser/lufile/parseFileContents.featureToModels.test.js

@@ -240,7 +240,85 @@ describe('Model as feature definitions', function () {
                 })
                 .catch(err => done(err))
         });
-        
+
+        it('phraseList can be added as a feature to an intent of same name', function(done) {
+            let luFile = `
+                # test
+                - one
+
+                @ intent test usesFeature test
+
+                @ phraselist test(interchangeable) =
+                    - one, two
+            `;
+
+            parseFile.parseFile(luFile)
+                .then(res => {
+                    assert.equal(res.LUISJsonStructure.intents.length, 1);
+                    assert.equal(res.LUISJsonStructure.intents[0].name, 'test');
+                    assert.equal(res.LUISJsonStructure.intents[0].features.length, 1);
+                    assert.equal(res.LUISJsonStructure.intents[0].features[0].featureName, 'test');
+                    done();
+                })
+                .catch(err => done(err))
+        });
+
+        it('phraseList can be added as a feature to an intent preferentially when there are both ml entity and phraseList of same name', function(done) {
+            let luFile = `
+                # test
+                - one
+
+                @ intent test usesFeatures abc
+
+                @ ml abc usesFeature abc
+
+                @ phraselist abc(interchangeable) =
+                    - a, b
+            `;
+
+            parseFile.parseFile(luFile)
+                .then(res => {
+                    assert.equal(res.LUISJsonStructure.intents.length, 1);
+                    assert.equal(res.LUISJsonStructure.intents[0].name, 'test');
+                    assert.equal(res.LUISJsonStructure.intents[0].features.length, 1);
+                    assert.equal(res.LUISJsonStructure.intents[0].features[0].featureName, 'abc');
+                    assert.equal(res.LUISJsonStructure.entities.length, 1);
+                    assert.equal(res.LUISJsonStructure.entities[0].name, 'abc');
+                    assert.equal(res.LUISJsonStructure.entities[0].features.length, 1);
+                    assert.equal(res.LUISJsonStructure.entities[0].features[0].featureName, 'abc');
+                    done();
+                })
+                .catch(err => done(err))
+        });
+
+        it('both phraseList and ml entity of same name can be added as a feature to an intent when using two same features', function(done) {
+            let luFile = `
+                # test
+                - one
+
+                @ intent test usesFeatures abc, abc
+
+                @ ml abc usesFeature abc
+
+                @ phraselist abc(interchangeable) =
+                    - a, b
+            `;
+
+            parseFile.parseFile(luFile)
+                .then(res => {
+                    assert.equal(res.LUISJsonStructure.intents.length, 1);
+                    assert.equal(res.LUISJsonStructure.intents[0].name, 'test');
+                    assert.equal(res.LUISJsonStructure.intents[0].features.length, 2);
+                    assert.equal(res.LUISJsonStructure.intents[0].features[0].featureName, 'abc');
+                    assert.equal(res.LUISJsonStructure.intents[0].features[1].modelName, 'abc');
+                    assert.equal(res.LUISJsonStructure.entities.length, 1);
+                    assert.equal(res.LUISJsonStructure.entities[0].name, 'abc');
+                    assert.equal(res.LUISJsonStructure.entities[0].features.length, 1);
+                    assert.equal(res.LUISJsonStructure.entities[0].features[0].featureName, 'abc');
+                    done();
+                })
+                .catch(err => done(err))
+        });
     });
 
     describe('Entity as feature to entity', function() {
@@ -339,6 +417,74 @@ describe('Model as feature definitions', function () {
                 })
                 .catch(err => done(err))
         });
+
+        it('phraseList can be added as a feature to an entity of same name', function(done) {
+            let luFile = `
+                @ ml abc usesFeature abc
+
+                @ phraselist abc(interchangeable) =
+                    - a, b
+            `;
+
+            parseFile.parseFile(luFile)
+                .then(res => {
+                    assert.equal(res.LUISJsonStructure.entities.length, 1);
+                    assert.equal(res.LUISJsonStructure.entities[0].name, 'abc');
+                    assert.equal(res.LUISJsonStructure.entities[0].features.length, 1);
+                    assert.equal(res.LUISJsonStructure.entities[0].features[0].featureName, 'abc');
+                    done();
+                })
+                .catch(err => done(err))
+        });
+
+        it('phraseList can be added as a feature to an entity preferentially when there are both ml entity and phraseList of same name', function(done) {
+            let luFile = `
+                @ ml test usesFeatures abc
+
+                @ ml abc usesFeature abc
+
+                @ phraselist abc(interchangeable) =
+                    - a, b
+            `;
+
+            parseFile.parseFile(luFile)
+                .then(res => {
+                    assert.equal(res.LUISJsonStructure.entities.length, 2);
+                    assert.equal(res.LUISJsonStructure.entities[0].name, 'test');
+                    assert.equal(res.LUISJsonStructure.entities[0].features.length, 1);
+                    assert.equal(res.LUISJsonStructure.entities[0].features[0].featureName, 'abc');
+                    assert.equal(res.LUISJsonStructure.entities[1].name, 'abc');
+                    assert.equal(res.LUISJsonStructure.entities[1].features.length, 1);
+                    assert.equal(res.LUISJsonStructure.entities[1].features[0].featureName, 'abc');
+                    done();
+                })
+                .catch(err => done(err))
+        });
+
+        it('both phraseList and ml entity of same name can be added as a feature to an entity when using two same features', function(done) {
+            let luFile = `
+                @ ml test usesFeatures abc, abc
+
+                @ ml abc usesFeature abc
+
+                @ phraselist abc(interchangeable) =
+                    - a, b
+            `;
+
+            parseFile.parseFile(luFile)
+                .then(res => {
+                    assert.equal(res.LUISJsonStructure.entities.length, 2);
+                    assert.equal(res.LUISJsonStructure.entities[0].name, 'test');
+                    assert.equal(res.LUISJsonStructure.entities[0].features.length, 2);
+                    assert.equal(res.LUISJsonStructure.entities[0].features[0].featureName, 'abc');
+                    assert.equal(res.LUISJsonStructure.entities[0].features[1].modelName, 'abc');
+                    assert.equal(res.LUISJsonStructure.entities[1].name, 'abc');
+                    assert.equal(res.LUISJsonStructure.entities[1].features.length, 1);
+                    assert.equal(res.LUISJsonStructure.entities[1].features[0].featureName, 'abc');
+                    done();
+                })
+                .catch(err => done(err))
+        });
     });
     it('Circular dependency for usesFeature is not allowed - simple case', function(done) {
         let luFile = `