Server: Find asset files through one function, first data path winning

CHANGELOG.md

@@ -14,7 +14,7 @@ version, see the upgrade guides in `docs/`.
 - **admin:** Dialogs focus their submit button, also in confirmations, unless a component sets `autofocus`; pressing Enter submits. They focused their first focusable element, e.g. Cancel in confirmations.
 - **admin:** Fields only validate on blur when they were edited while focused, so passing through empty required fields, or clicking the label of a focused field, no longer shows errors. Submitting still validates all fields. Fields validated on every blur.
 - **router:** Routes with `*` or `**` inside a segment, e.g. `/files/*.json` or `/files/**.json`, or with a param inside a segment after `**`, e.g. `/files/**/tech-doc-:format`, throw an error when added, where they matched any segment, ignoring the rest of it, or never matched.
-- **server:** Files that several overlapping asset data paths reach, e.g. `'content.items[**].file'` and `'**.file'`, count once per place in the data, with the storage and config of the first data path, where they counted once per data path. Apps with overlapping asset data paths should run `db:recount_assets` after upgrading, to correct the stored counts.
+- **server:** A place in the data that several overlapping asset data paths reach, e.g. `'content.items[**].file'` and `'**.file'`, belongs to the first of them: its file counts once, where it counted once per data path, and loads and is signed with the storage of the first data path, where the last one won. Apps with overlapping asset data paths should run `db:recount_assets` after upgrading, to correct the stored counts.
 - **utils:** `debounceAsync()` resolves calls made while a run is pending with the result of their own run, where they received the pending run's result, and `cancel()` only settles calls that wait for the timer, where it also resolved those of a pending run, whose rejection then threw an unhandled `TypeError`. Debounced `search` filters of the admin now get the options of the search term they were called with (#347, #349).
 - **utils:** `equals()` never treats values of different types as equal, e.g. a buffer and an object with the same keys, or a `Uint8Array` and an array, where it compared all objects by their keys. Typed arrays are compared element by element, like arrays (#373).
 - **utils:** `*` in `getValueAtDataPath()` only merges the values it finds into one array when the rest of the path has wildcards too, so values that are arrays themselves are kept, e.g. `'*/x'` returns `[[1, 2], [3]]` where it returned `[1, 2, 3]`, like `**` and `getEntriesAtDataPath()` do. This affects app code that reads array values through `*`, e.g. with `dataPath` in schemas (#357).

docs/api/server/model.md

@@ -278,10 +278,9 @@ data path, `*` and `**` wildcards included. Each names the
 and height of images and videos uploaded through
 [controller assets](controllers.md#assets). A place in the data that several
 data paths reach belongs to the first of them, with its storage and options,
-and counts once. With an asset model registered
-(see [`AssetMixin`](#assetmixin)), files are imported,
-reference-counted and released automatically, see
-[Application assets](application.md#assets).
+and counts once. With an asset model registered (see
+[`AssetMixin`](#assetmixin)), files are imported, reference-counted and
+released automatically, see [Application assets](application.md#assets).
 
 ```js
 static properties = {

packages/server/src/app/Application.js

@@ -29,7 +29,6 @@ import {
   assignDeeply,
   parseDataPath,
   normalizeDataPath,
-  getEntriesAtDataPath,
   groupBy,
   mapConcurrently,
   toPromiseCallback
@@ -1141,34 +1140,24 @@ export class Application extends Koa {
           }
         }
         for (const item of await query) {
-          // Overlapping data paths can reach the same location: count it once,
-          // for the first data path that reaches it, also when `storage`
-          // filters that one out.
-          const paths = new Set()
-          for (const dataPath in assets) {
+          // Find the files through all data paths, as each location belongs
+          // to the first one, also when `storage` filters that one out:
+          const files = modelClass._getAssetFiles(item)
+          for (const { file, path, dataPath } of files) {
             const config = assets[dataPath]
-            const entries = getEntriesAtDataPath(item, dataPath, () => {})
-            for (const [path, value] of Object.entries(entries)) {
-              for (const [index, file] of asArray(value).entries()) {
-                const filePath = isArray(value) ? `${path}/${index}` : path
-                if (file && !paths.has(filePath)) {
-                  paths.add(filePath)
-                  if (
-                    (!storage || config.storage === storage) &&
-                    (!keySet || keySet.has(file.key))
-                  ) {
-                    references.push({
-                      key: file.key,
-                      file,
-                      model: modelName,
-                      id: item.$id(),
-                      dataPath: filePath,
-                      storage: config.storage,
-                      config
-                    })
-                  }
-                }
-              }
+            if (
+              (!storage || config.storage === storage) &&
+              (!keySet || keySet.has(file.key))
+            ) {
+              references.push({
+                key: file.key,
+                file,
+                model: modelName,
+                id: item.$id(),
+                dataPath: path,
+                storage: config.storage,
+                config
+              })
             }
           }
         }

packages/server/src/models/Model.js

@@ -12,7 +12,6 @@ import {
   equals,
   parseDataPath,
   normalizeDataPath,
-  getValueAtDataPath,
   getEntriesAtDataPath,
   setValueAtDataPath,
   groupBy,
@@ -1074,36 +1073,45 @@ export class Model extends objection.Model {
 
   // Assets handling
 
+  static _getAssetFiles(json, dataPaths = Object.keys(this.definition.assets)) {
+    const files = []
+    // Overlapping data paths can reach the same location: it belongs to the
+    // first data path that reaches it.
+    const filePaths = new Set()
+    for (const dataPath of dataPaths) {
+      // Wildcard data paths (e.g. `**.file`) expand to multiple concrete
+      // paths, each with its own location. Ignore data paths that don't
+      // resolve, e.g. missing parents.
+      const entries = getEntriesAtDataPath(json, dataPath, noop)
+      for (const [path, value] of Object.entries(entries)) {
+        for (const [index, file] of asArray(value).entries()) {
+          const filePath = isArray(value) ? `${path}/${index}` : path
+          if (file && !filePaths.has(filePath)) {
+            filePaths.add(filePath)
+            files.push({ file, path: filePath, dataPath })
+          }
+        }
+      }
+    }
+    return files
+  }
+
   static _forEachAssetFile(json, callback) {
     const { assets } = this.definition
     if (assets) {
-      for (const dataPath in assets) {
-        const data = getValueAtDataPath(json, dataPath, noop)
-        if (!data) continue
-        const storage = this.app.getStorage(assets[dataPath].storage)
-        forEachAssetFile(data, storage, callback)
+      for (const { file, dataPath } of this._getAssetFiles(json)) {
+        callback(file, this.app.getStorage(assets[dataPath].storage))
       }
     }
   }
 
   static _mapAssetFiles(json, callback) {
     const { assets } = this.definition
     if (assets) {
-      for (const dataPath in assets) {
+      // Write each file back to its own location:
+      for (const { file, path, dataPath } of this._getAssetFiles(json)) {
         const storage = this.app.getStorage(assets[dataPath].storage)
-        // Wildcard data paths (e.g. `**.file`) expand to multiple concrete
-        // paths; iterate them so each match is written back to its own
-        // location instead of the flattened wildcard result.
-        const entries = getEntriesAtDataPath(json, dataPath, noop)
-        for (const [path, data] of Object.entries(entries)) {
-          if (data) {
-            setValueAtDataPath(
-              json,
-              path,
-              mapAssetFiles(data, storage, callback)
-            )
-          }
-        }
+        setValueAtDataPath(json, path, callback(file, storage))
       }
     }
   }
@@ -1199,10 +1207,12 @@ export class Model extends objection.Model {
             : beforeItems.map(() => inputItems[0])
 
         const beforeFilesPerDataPath = getFilesPerAssetDataPath(
+          this,
           beforeItems,
           dataPaths
         )
         const afterFilesPerDataPath = getFilesPerAssetDataPath(
+          this,
           afterItems,
           dataPaths
         )
@@ -1326,44 +1336,14 @@ function loadAssetDataPaths(query, dataPaths) {
 
 const noop = () => {}
 
-function forEachAssetFile(data, storage, callback) {
-  if (isArray(data)) {
-    for (const item of data) {
-      forEachAssetFile(item, storage, callback)
-    }
-  } else if (data) {
-    callback(data, storage)
-  }
-}
-
-function mapAssetFiles(data, storage, callback) {
-  if (isArray(data)) {
-    return data.map(item => mapAssetFiles(item, storage, callback))
-  }
-  return data ? callback(data, storage) : data
-}
-
-function getFilesPerAssetDataPath(items, dataPaths) {
+function getFilesPerAssetDataPath(modelClass, items, dataPaths) {
   const filesPerDataPath = Object.fromEntries(
     dataPaths.map(dataPath => [dataPath, []])
   )
   for (const item of asArray(items)) {
-    // Overlapping data paths can reach the same location: count it once, for
-    // the first data path that reaches it.
-    const paths = new Set()
-    for (const dataPath of dataPaths) {
-      // Ignore data paths that don't resolve, e.g. missing parents. Wildcard
-      // data paths expand to the entries of their concrete paths.
-      const entries = getEntriesAtDataPath(item, dataPath, noop)
-      for (const [path, value] of Object.entries(entries)) {
-        for (const [index, file] of asArray(value).entries()) {
-          const filePath = isArray(value) ? `${path}/${index}` : path
-          if (file && !paths.has(filePath)) {
-            paths.add(filePath)
-            filesPerDataPath[dataPath].push(file)
-          }
-        }
-      }
+    const files = modelClass._getAssetFiles(item, dataPaths)
+    for (const { file, dataPath } of files) {
+      filesPerDataPath[dataPath].push(file)
     }
   }
   return filesPerDataPath

packages/server/src/models/Model.test.js

@@ -24,132 +24,6 @@ function toSQL(query) {
   return query.toKnexQuery().toSQL().toNative().sql
 }
 
-function makeFakeModel(assets) {
-  const storage = {
-    signAssetFile(file) {
-      file.signature = `sig-of-${file.key}`
-    }
-  }
-  return {
-    app: {
-      getStorage(name) {
-        return name === 'test' ? storage : null
-      }
-    },
-    definition: { assets }
-  }
-}
-
-function signCallback(file, storage) {
-  const signed = { ...file }
-  storage.signAssetFile(signed)
-  return signed
-}
-
-describe('Model._mapAssetFiles: asset signing across data paths', () => {
-  it('writes the signature back to a literal data path', () => {
-    const model = makeFakeModel({ 'image.file': { storage: 'test' } })
-    const json = { image: { file: { key: 'foo.png' } } }
-    Model._mapAssetFiles.call(model, json, signCallback)
-    expect(json.image.file.signature).toBe('sig-of-foo.png')
-  })
-
-  it('writes the signature back to every match of a deep wildcard', () => {
-    const model = makeFakeModel({ '**.file': { storage: 'test' } })
-    const json = {
-      content: {
-        file: { key: 'a.png' },
-        sections: [
-          { file: { key: 'b.png' } },
-          { file: { key: 'c.png' } }
-        ]
-      }
-    }
-    Model._mapAssetFiles.call(model, json, signCallback)
-    expect(json.content.file.signature).toBe('sig-of-a.png')
-    expect(json.content.sections[0].file.signature).toBe('sig-of-b.png')
-    expect(json.content.sections[1].file.signature).toBe('sig-of-c.png')
-  })
-
-  it('writes the signature back into wildcard array values', () => {
-    const model = makeFakeModel({ '**.files': { storage: 'test' } })
-    const json = {
-      content: {
-        files: [{ key: 'x.png' }, { key: 'y.png' }]
-      }
-    }
-    Model._mapAssetFiles.call(model, json, signCallback)
-    expect(json.content.files[0].signature).toBe('sig-of-x.png')
-    expect(json.content.files[1].signature).toBe('sig-of-y.png')
-  })
-
-  it('keeps empty values at asset data paths', () => {
-    const model = makeFakeModel({
-      cover: { storage: 'test' },
-      gallery: { storage: 'test' }
-    })
-    const json = { cover: null, gallery: [{ key: 'a.png' }, null] }
-    Model._mapAssetFiles.call(model, json, signCallback)
-    expect(json).toEqual({
-      cover: null,
-      gallery: [{ key: 'a.png', signature: 'sig-of-a.png' }, null]
-    })
-  })
-
-  it('is a no-op when the wildcard matches nothing', () => {
-    const model = makeFakeModel({ '**.file': { storage: 'test' } })
-    const json = { name: 'no-files-here' }
-    Model._mapAssetFiles.call(model, json, signCallback)
-    expect(json).toEqual({ name: 'no-files-here' })
-  })
-})
-
-describe('Model._signAssetFiles()', () => {
-  it('signs copies of the asset files, leaving the originals untouched', () => {
-    const model = makeFakeModel({ cover: { storage: 'test' } })
-    model._mapAssetFiles = Model._mapAssetFiles
-    const file = { key: 'cover.png' }
-    const json = { cover: file }
-    Model._signAssetFiles.call(model, json)
-    expect(json.cover).toEqual({
-      key: 'cover.png',
-      signature: 'sig-of-cover.png'
-    })
-    expect(json.cover).not.toBe(file)
-    expect(file).toEqual({ key: 'cover.png' })
-  })
-})
-
-describe('Model._forEachAssetFile()', () => {
-  it('calls the callback for each file, including files in arrays', () => {
-    const model = makeFakeModel({
-      cover: { storage: 'test' },
-      gallery: { storage: 'test' },
-      missing: { storage: 'test' }
-    })
-    const json = {
-      cover: { key: 'cover.png' },
-      gallery: [{ key: 'a.png' }, null, [{ key: 'b.png' }]]
-    }
-    const keys = []
-    Model._forEachAssetFile.call(model, json, (file, storage) => {
-      expect(storage).toBe(model.app.getStorage('test'))
-      keys.push(file.key)
-    })
-    expect(keys).toEqual(['cover.png', 'a.png', 'b.png'])
-  })
-
-  it('does nothing for models without assets', () => {
-    const callback = vi.fn()
-    Model._forEachAssetFile.call(
-      makeFakeModel(undefined),
-      { cover: { key: 'cover.png' } },
-      callback
-    )
-    expect(callback).not.toHaveBeenCalled()
-  })
-})
-
 describe('Model.getProperty: type inference for $ref/discriminator schemas', () => {
   function getProperty({ name, properties, definitions = {} }) {
     return Model.getProperty.call(