From d2c102f9eeeebf8ded951d59fd61dfe462847f17 Mon Sep 17 00:00:00 2001 From: Fei Chen <43032123+feich-ms@users.noreply.github.com> Date: Fri, 15 Jan 2021 19:11:29 +0800 Subject: [PATCH] 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 --- .../lu/src/parser/lufile/parseFileContents.js | 81 +++++++--- packages/lu/src/parser/utils/helpers.js | 9 +- .../parseFileContents.featureToModels.test.js | 148 +++++++++++++++++- 3 files changed, 215 insertions(+), 23 deletions(-) diff --git a/packages/lu/src/parser/lufile/parseFileContents.js b/packages/lu/src/parser/lufile/parseFileContents.js index 87e72a1f..b66c0d7b 100644 --- a/packages/lu/src/parser/lufile/parseFileContents.js +++ b/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 }); diff --git a/packages/lu/src/parser/utils/helpers.js b/packages/lu/src/parser/utils/helpers.js index 548a389d..6eebbb26 100644 --- a/packages/lu/src/parser/utils/helpers.js +++ b/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; }); diff --git a/packages/lu/test/parser/lufile/parseFileContents.featureToModels.test.js b/packages/lu/test/parser/lufile/parseFileContents.featureToModels.test.js index 2c7b7ca1..4f8f848d 100644 --- a/packages/lu/test/parser/lufile/parseFileContents.featureToModels.test.js +++ b/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 = `