fix: check for icon file referenced by preset.icon (#782)

* check for icon file referenced by preset.icon

* add access-point to completeConfig

* add test for invalid icon name

* remove unnecessary icons, fix presets.json

* add icons to complete config

* metter check for `'icon' in field`

* compute `iconFilenames` once, repurpuse icon name regex

* iconFilenames should be `Set<string>`, not `Set<string | undefined>` (#790)

* Fix missing icon name, warn instead of throwing (#791)

---------

Co-authored-by: Tomás Ciccola <[email protected]>
Co-authored-by: Evan Hahn <[email protected]>

Author: 3 people

src/config-import.js

@@ -1,14 +1,15 @@
 import yauzl from 'yauzl-promise'
 import { validate, valueSchemas } from '@mapeo/schema'
 import { json, buffer } from 'node:stream/consumers'
-import { assert } from './utils.js'
+import { assert, isDefined } from './utils.js'
 import path from 'node:path'
 import { parse as parseBCP47 } from 'bcp-47'
 import { SUPPORTED_CONFIG_VERSION } from './constants.js'
 
 // Throw error if a zipfile contains more than 10,000 entries
 const MAX_ENTRIES = 10_000
 const MAX_ICON_SIZE = 10_000_000
+const ICON_NAME_REGEX = /([a-zA-Z0-9-]+)-([a-zA-Z]+)@(\d+)x\.[a-zA-Z]+$/
 
 /**
  * @typedef {yauzl.Entry} Entry
@@ -50,6 +51,7 @@ export async function readConfig(configPath) {
   const presetsFile = await findPresetsFile(entries)
   const translationsFile = await findTranslationsFile(entries)
   const metadataFile = await findMetadataFile(entries)
+  const iconEntries = getIconEntries(entries)
   assert(
     isValidConfigFile(metadataFile),
     `invalid or missing config file version ${metadataFile.fileVersion}. We support version ${SUPPORTED_CONFIG_VERSION}}`
@@ -75,13 +77,6 @@ export async function readConfig(configPath) {
       /** @type {IconData | undefined} */
       let icon
 
-      // we sort the icons by filename so we can group variants together
-      const iconEntries = entries
-        .filter((entry) => entry.filename.match(/^icons\/([^/]+)$/))
-        .sort((icon, nextIcon) =>
-          icon.filename.localeCompare(nextIcon.filename)
-        )
-
       for (const entry of iconEntries) {
         if (entry.uncompressedSize > MAX_ICON_SIZE) {
           warnings.push(
@@ -177,6 +172,18 @@ export async function readConfig(configPath) {
         return sort - nextSort
       })
 
+      const iconFilenames = new Set(
+        iconEntries
+          .map((icon) => {
+            const matches = path.basename(icon.filename).match(ICON_NAME_REGEX)
+            if (matches) {
+              const [_, name] = matches
+              return name
+            }
+          })
+          .filter(isDefined)
+      )
+
       // 5. for each preset get the corresponding fieldId and iconId, add them to the db
       for (const { preset, name } of sortedPresets) {
         /** @type {Record<string, unknown>} */
@@ -193,6 +200,20 @@ export async function readConfig(configPath) {
             presetValue[key] = preset[key]
           }
         }
+
+        if (!('icon' in preset) || typeof preset.icon !== 'string') {
+          warnings.push(new Error(`Preset ${preset.name} doesn't have an icon`))
+          return
+        }
+        if (!iconFilenames.has(preset.icon)) {
+          warnings.push(
+            new Error(
+              `preset references icon with name ${preset.icon} but file doesn't exist`
+            )
+          )
+          return
+        }
+
         if (!validate('preset', presetValue)) {
           warnings.push(new Error(`Invalid preset ${preset.name}`))
           continue
@@ -472,16 +493,23 @@ function translateMessageObject(warnings) {
   }
 }
 
+/**
+ * @param {ReadonlyArray<Entry>} entries
+ */
+function getIconEntries(entries) {
+  return entries
+    .filter((entry) => entry.filename.match(/^icons\/([^/]+)$/))
+    .sort((icon, nextIcon) => icon.filename.localeCompare(nextIcon.filename))
+}
+
 /**
  * @param {string} filename
  * @param {Buffer} buf
  * @returns {{ name: string, variant: IconData['variants'][Number] }}}
  */
 function parseIcon(filename, buf) {
   const parsedFilename = path.parse(filename)
-  const matches = parsedFilename.base.match(
-    /([a-zA-Z0-9-]+)-([a-zA-Z]+)@(\d+)x\.[a-zA-Z]+$/
-  )
+  const matches = parsedFilename.base.match(ICON_NAME_REGEX)
   if (!matches) {
     throw new Error(`Unexpected icon filename ${filename}`)
   }

src/utils.js

@@ -109,6 +109,15 @@ export function setHas(set) {
   return (value) => set.has(/** @type {*} */ (value))
 }
 
+/**
+ * @template T
+ * @param {undefined | T} value
+ * @returns {value is T}
+ */
+export function isDefined(value) {
+  return value !== undefined
+}
+
 /**
  * When reading from SQLite, any optional properties are set to `null`. This
  * converts `null` back to `undefined` to match the input types (e.g. the types

test-e2e/translation-api.js

@@ -1,5 +1,6 @@
 import test from 'node:test'
 import assert from 'node:assert/strict'
+import { isDefined } from '../src/utils.js'
 import { createManagers, ManagerCustodian } from './utils.js'
 import { defaultConfigPath } from '../tests/helpers/default-config.js'
 import {
@@ -387,12 +388,3 @@ test('translation api - re-loading from disk', async (t) => {
 
   assert(hasExpectedNumberOfTranslations)
 })
-
-/**
- * @template T
- * @param {undefined | T} value
- * @returns {value is T}
- */
-function isDefined(value) {
-  return value !== undefined
-}

tests/config-import.js

@@ -223,6 +223,27 @@ test('config import - presets', async () => {
     'the second error is because the preset is null'
   )
 
+  config = await readConfig('./tests/fixtures/config/noIconNameOnPreset.zip')
+  arrayFrom(config.presets())
+  assert.equal(
+    config.warnings.length,
+    1,
+    'we got an error when loading the preset'
+  )
+  assert(
+    /Punto de entrada doesn't have an icon/.test(config.warnings[0].message),
+    "there's a warning because the preset has no icon field"
+  )
+
+  config = await readConfig(
+    './tests/fixtures/config/invalidIconNameOnPreset.zip'
+  )
+  arrayFrom(config.presets())
+  assert(
+    /preset references icon with name/.test(config.warnings[0].message),
+    "there's a warning because the preset references a missing icon"
+  )
+
   config = await readConfig('./tests/fixtures/config/validPreset.zip')
   for (const preset of config.presets()) {
     assert.equal(preset.value.schemaName, 'preset', `schemaName is 'preset'`)

tests/fixtures/config/completeConfig/icons/[email protected]

@@ -0,0 +1,5 @@
+{
+  "name": "test-config",
+  "buildDate": "2024-07-29T17:37:23.382Z",
+  "fileVersion": "1.0"
+}

tests/fixtures/config/invalidIconNameOnPreset/icons/[email protected]

@@ -0,0 +1,47 @@
+{
+  "fields":{
+    "nombre-monitor": {
+      "tagKey": "nombre-monitor",
+      "type": "text",
+      "label": "Nombre del Monitor o Mapeador",
+      "universal": true,
+      "placeholder": "¿Quién está haciendo esta observación?"
+    }
+  },
+  "presets":{
+    "access-point":{
+      "icon": "access-pont",
+      "fields": [
+        "nombre-monitor"
+      ],
+      "geometry": [
+        "point",
+        "area",
+        "line"
+      ],
+      "sort": 1,
+      "tags": {
+        "type": "threat",
+        "threat": "access-point"
+      },
+      "name": "Punto de entrada"
+    },
+    "plant":{
+      "icon": "plant",
+      "fields": [
+        "nombre-monitor"
+      ],
+      "sort": 3,
+      "geometry": [
+        "point",
+        "area"
+      ],
+      "tags": {
+        "type": "natural",
+        "natural": "plant"
+      },
+      "terms": [],
+      "name": "Planta"
+    }
+  }
+}

tests/fixtures/config/invalidIconNameOnPreset/icons/[email protected]

@@ -0,0 +1,5 @@
+{
+  "name": "test-config",
+  "buildDate": "2024-08-28T21:20:00.069Z",
+  "fileVersion": "1.0"
+}