Lu file's content altered and wrong error when using unavailable features

[MatteoMeil]

Versions

What CLI version are you using: 4.14.1
What Nodejs version are you using: 14.17.1
What command-line interpreters are you using: CMD (Windows' command prompt)
What OS are you using: Win10 x64

Describe the bug

TL;DR (details and possible fixes in first comment)

• Content of .lu file is altered based on result of this method call during loading of file content, causing wrong error on subsequent passes if Builder class (defined here) is constructed passing a log function

• This call passes empty luis_culture to build function (defined here) and the value is never updated down the road (thus, remaining empty).
  This may cause unexpected behavior in parseLuAndQnaWithAntlr because culture falls back to en-us

Notice that this bug affects BotFramework Composer that outputs a wrong error in cases like this one

To Reproduce

First one is easily reproducible following the steps below, while to address the second issue you must enter debug mode and go along the path until you'll find the bug

1. Create a .lu file (e.g.: MyLUISFile.lu)
   Notice that I explicitly inserted an error: I used geographyV2 with culture it-it even if it isn't available for Italian (Italy) at the time I'm writing the issue

   > LUIS application information
   > !# @app.versionId = 0.1
   > !# @app.culture = it-it
   > !# @app.luis_schema_version = 7.0.0
   
   > # Intent definitions
   
   # MyIntent
   - ciao
   
   
   > # Entity definitions
   @ ml MLEntity
       - @ ml MLSubEntity usesFeature geographyV2
   
   > # PREBUILT Entity definitions
   @ prebuilt geographyV2
   

2. Install @microsoft\botframework-cli@4.14.1, create the following .js file and execute it using Node:

   const {Builder} = require('@microsoft/bf-lu/lib/parser/lubuild/builder');
   
   async function run() {
     const builder = new Builder((message) => {
       console.log(message);
     });
   
     const luContents = await builder.loadContents(['MyLUISFile.lu'], {
       culture: 'it-it',
       log: console.log
     })
   
     const buildResult = await builder.build(luContents, 'AUTHORING_KEY', 'BOT_NAME', {
       endpoint: 'AUTHORING_ENDPOINT'
     });
   }
   
   run().then(console.log).catch(console.log)

3. You will have the following exception logged in console:

   Luis build failed: [ERROR] line 20:0 - line 20:1: Invalid child entity definition found. No definition found for "geographyV2" in child entity definition "- @ ml MLSubEntity usesFeature geographyV2". Features must be defined before they can be added to a child.

Expected behavior

It should throw an error saying the feature isn't available and stop the execution, as it does if log function isn't passed in Builder constructor.

Screenshots

Not applicable

Additional context

Not applicable

[bug]

[MatteoMeil]

Long version

First part: content altered

Content of .lu file is altered based on result of this method call during loading of file content, i.e.: original file is parsed and an internal representation of the file is used to "recreate" its content.

However, this causes a wrong re-creation of the content if the result of this line is null and a log function exists (passed during construction of Builder class).

The wrong re-creation is due to the fact that these steps are executed:

• it's been outputted a warning message on the console stating that the entity isn't available
• the prebuilt entity is not added to the internal representation
• the following passes don't know that the entity is specified but unavailable and thus, treat it as missing.

This causes the output of the wrong error specified above.

Possible fix:
Always throw an error as if log function is never specified

Second part: missing culture in function's call

This call passes empty luis_culture to build function (defined here) and the value is never updated down the road (thus, remaining empty).

This may cause unexpected behavior in parseLuAndQnaWithAntlr because culture falls back to en-us.

Possible fixes

• luOb defined here can be filled with the following object:

  {
    "content": "Lu file content",
    "id": "some id",
    "includeInCollate": false,
    "language": "it-it", // current locale inferred from previous steps
    "path": "path/to/lu/file.lu",
    "name": "filename.lu"
  }

  Thus, this line can be fixed to

  let parsedContent = await parseLuFile(luOb, log, luis_culture || luOb.language)

• Important Note: BF Composer passes content without header. The following fix alone is not enough to solve the problem
  parsedContent.LUISJsonStructure.culture gets filled here with culture defined in .lu file header.
  It can be inserted the following code in the next line to update culture:

  locale = locale || parsedContent.LUISJsonStructure.culture;

• Both of the above proposed. This way it will solve the problem that a .lu file can be submitted without header informations

[srinaath]

@munozemilio could you take a look at this ticket?

[munozemilio]

@cosmicshuai could you please take a look at the lubuilder piece, please?

[mrivera-ms]

@munozemilio, @cosmicshuai, is there any progress on this?

[cosmicshuai]

It still need some time to figure out the root cause. Can we move it to R16. I will do it ASAP.