fix(developer): handle missing model id in package with compiler error

If a `<LexicalModel>` element is found with a missing or empty `<ID>`
element, the compiler will now report `ERROR_MissingModelId` instead of
crashing. The corresponding check for missing `<ID>` for `<Keyboard>`
element has been updated to match this, and now reports a clearer error
message of `ERROR_MissingKeyboardId` rather than depending on the
side-effect of looking up the keyboard in the `<File>` elements.

Fixes: #13783
Fixes: KEYMAN-DEVELOPER-2X7
Test-bot: skip

developer/src/kmc-package/src/compiler/kmp-compiler.ts

@@ -289,11 +289,15 @@ export class KmpCompiler implements KeymanCompiler {
     //
 
     if(kps.Keyboards?.Keyboard?.length) {
+      if(!this.validateKeyboards(kps.Keyboards.Keyboard)) {
+        // error reported by validateKeyboards
+        return null;
+      }
       kmp.keyboards = kps.Keyboards.Keyboard.map((keyboard: KpsFile.KpsFileKeyboard) => ({
         displayFont: keyboard.DisplayFont ? this.callbacks.path.basename(this.normalizePath(keyboard.DisplayFont)) : undefined,
         oskFont: keyboard.OSKFont ? this.callbacks.path.basename(this.normalizePath(keyboard.OSKFont)) : undefined,
         name: '', // filled by PackageMetadataUpdater
-        id:keyboard.ID?.trim(),
+        id:keyboard.ID.trim(),
         version: DEFAULT_KEYBOARD_VERSION,  // filled by PackageMetadataUpdater
         rtl: false, // filled by PackageMetadataUpdater
         languages: keyboard.Languages?.Language?.length ?
@@ -322,6 +326,10 @@ export class KmpCompiler implements KeymanCompiler {
     //
 
     if(kps.LexicalModels?.LexicalModel?.length) {
+      if(!this.validateLexicalModels(kps.LexicalModels.LexicalModel)) {
+        // error reported by validateLexicalModels
+        return null;
+      }
       kmp.lexicalModels = kps.LexicalModels.LexicalModel.map((model: KpsFile.KpsFileLexicalModel) => ({
         name:model.ID.trim(),
         id:model.ID.trim(),
@@ -453,6 +461,30 @@ export class KmpCompiler implements KeymanCompiler {
     return o;
   }
 
+  private validateKeyboards(keyboards: KpsFile.KpsFileKeyboard[]): boolean {
+    let index = 1;
+    for(const keyboard of keyboards) {
+      if(!keyboard.ID || keyboard.ID.trim() == '') {
+        this.callbacks.reportMessage(PackageCompilerMessages.Error_MissingKeyboardId({index}));
+        return false;
+      }
+      index++;
+    }
+    return true;
+  }
+
+  private validateLexicalModels(models: KpsFile.KpsFileLexicalModel[]): boolean {
+    let index = 1;
+    for(const model of models) {
+      if(!model.ID || model.ID.trim() == '') {
+        this.callbacks.reportMessage(PackageCompilerMessages.Error_MissingModelId({index}));
+        return false;
+      }
+      index++;
+    }
+    return true;
+  }
+
   /**
    * @internal
    * Returns a Promise to the serialized data which can then be written to a .kmp file.

developer/src/kmc-package/src/compiler/package-compiler-messages.ts

@@ -207,6 +207,22 @@ export class PackageCompilerMessages {
     may violate this rule transitively.
   `);
 
+  static ERROR_MissingModelId = SevError | 0x0029;
+  static Error_MissingModelId = (o:{index:number}) => m(
+    this.ERROR_MissingModelId, `The lexical model at index ${def(o.index)} has a missing or empty ID field.`, `
+    Each LexicalModel element must have an ID sub-element and a Languages
+    sub-element. The content of the ID sub-element must correspond to the basename
+    of a .model.js file inside the Files element.
+  `);
+
+  static ERROR_MissingKeyboardId = SevError | 0x002A;
+  static Error_MissingKeyboardId = (o:{index:number}) => m(
+    this.ERROR_MissingKeyboardId, `The keyboard at index ${def(o.index)} has a missing or empty ID field.`, `
+    Each Keyboard element must have an ID sub-element sub-element. The content of
+    the ID sub-element must correspond to the basename of a keyboard .kmx or .js
+    file inside the Files element.
+  `);
+
   //------------------------------------------------------------------------------|
   // max length of detail message lines (checked by verifyCompilerMessagesObject) |
   //------------------------------------------------------------------------------|

developer/src/kmc-package/test/fixtures/invalid/missing_model_id.kps

@@ -0,0 +1,30 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Package>
+  <System>
+    <KeymanDeveloperVersion>12.0.1500.0</KeymanDeveloperVersion>
+    <FileVersion>12.0</FileVersion>
+  </System>
+  <Info>
+    <Name URL="">SENĆOŦEN (Saanich Dialect) Lexical Model</Name>
+    <Copyright URL="">© 2019 National Research Council Canada</Copyright>
+    <Author URL="mailto:Eddie.Santos@nrc-cnrc.gc.ca">Eddie Antonio Santos</Author>
+    <Version>1.0.3</Version>
+  </Info>
+  <Files>
+    <File>
+      <Name>example.qaa.sencoten.model.js</Name>
+      <Description>Lexical model example.qaa.sencoten.model.js</Description>
+      <CopyLocation>0</CopyLocation>
+      <FileType>.model.js</FileType>
+    </File>
+  </Files>
+  <LexicalModels>
+    <LexicalModel>
+      <!-- #13783 missing ID (and Name irrelevant per #13640) -->
+      <Languages>
+        <Language ID="qaa">North Straits Salish</Language>
+        <Language ID="qaa-Latn">SENĆOŦEN</Language>
+      </Languages>
+    </LexicalModel>
+  </LexicalModels>
+</Package>

developer/src/kmc-package/test/package-compiler.tests.ts

@@ -86,7 +86,7 @@ describe('KmpCompiler', function () {
               assert.deepEqual(kmpJsonOutput, kmpJsonZippedFixture);
             }),
             zipFile.file(`${modelID}.model.js`).async('uint8array').then(modelJsFile => {
-              assert.deepEqual(modelJsFile, fs.readFileSync(jsPath));
+              assert.deepEqual(modelJsFile, new Uint8Array(fs.readFileSync(jsPath)));
             })
           ]);
         });
@@ -278,8 +278,13 @@ describe('KmpCompiler', function () {
   it(`should load a package with missing keyboard ID metadata`, function () {
     const kmpJson = kmpCompiler.transformKpsToKmpObject(makePathToFixture('invalid', 'missing_keyboard_id.kps'));
     assert.isNull(kmpJson); // with a missing keyboard_id, the package shouldn't load, but it shouldn't crash either
-    assert.deepEqual(callbacks.messages[0].code, PackageCompilerMessages.ERROR_KeyboardContentFileNotFound);
+    assert.deepEqual(callbacks.messages[0].code, PackageCompilerMessages.ERROR_MissingKeyboardId);
+  });
 
+  it(`should load a package with missing model ID metadata`, function () {
+    const kmpJson = kmpCompiler.transformKpsToKmpObject(makePathToFixture('invalid', 'missing_model_id.kps'));
+    assert.isNull(kmpJson); // with a missing model_id, the package shouldn't load, but it shouldn't crash either
+    assert.deepEqual(callbacks.messages[0].code, PackageCompilerMessages.ERROR_MissingModelId);
   });
 
   it(`should load a package with missing keyboard name metadata`, function () {