diff --git a/CHANGELOG.md b/CHANGELOG.md index dfb6ed5dd..de09a109c 100644 --- a/CHANGELOG.md +++ b/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). diff --git a/docs/api/server/model.md b/docs/api/server/model.md index 9d473c9f3..749477eaf 100644 --- a/docs/api/server/model.md +++ b/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 = { diff --git a/packages/server/src/app/Application.js b/packages/server/src/app/Application.js index 7807689f0..2af6acca4 100644 --- a/packages/server/src/app/Application.js +++ b/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 + }) } } } diff --git a/packages/server/src/models/Model.js b/packages/server/src/models/Model.js index 33810a302..5c81648bd 100644 --- a/packages/server/src/models/Model.js +++ b/packages/server/src/models/Model.js @@ -12,7 +12,6 @@ import { equals, parseDataPath, normalizeDataPath, - getValueAtDataPath, getEntriesAtDataPath, setValueAtDataPath, groupBy, @@ -1074,14 +1073,34 @@ 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)) } } } @@ -1089,21 +1108,10 @@ export class Model extends objection.Model { 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 diff --git a/packages/server/src/models/Model.test.js b/packages/server/src/models/Model.test.js index 3740330b0..3740a7a2b 100644 --- a/packages/server/src/models/Model.test.js +++ b/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( diff --git a/tests/server/assets.test.ts b/tests/server/assets.test.ts index f26e8461b..2e8d27c5e 100644 --- a/tests/server/assets.test.ts +++ b/tests/server/assets.test.ts @@ -808,11 +808,15 @@ describe('asset references', () => { class Album extends Model { declare id: number declare cover: AssetFile | null - declare photos: AssetFile[] | null + declare photos: (AssetFile | null)[] | null static override properties: ModelProperties = { cover: { type: 'object', nullable: true }, - photos: { type: 'array', items: { type: 'object' }, nullable: true } + photos: { + type: 'array', + items: { type: 'object', nullable: true }, + nullable: true + } } static override assets = { @@ -862,7 +866,8 @@ describe('asset references', () => { } static override assets = { - '**.file': { storage: 'memory' } + '**.file': { storage: 'memory' }, + '**.files': { storage: 'memory' } } } @@ -877,12 +882,35 @@ describe('asset references', () => { // Overlapping data paths that both reach the files of `content.items`: static override assets = { 'content.items[**].file': { storage: 'memory', readDimensions: true }, + '**.file': { storage: 'memory' } + } + } + + class Brochure extends Model { + declare id: number + declare content: Record | null + + static override properties: ModelProperties = { + content: { type: 'object', nullable: true } + } + + // Overlapping data paths with different storages: + static override assets = { + 'content.items[**].file': { storage: 'memory' }, '**.file': { storage: 'other' } } } class Asset extends AssetMixin(Model) {} - const app = createApp({ Album, Track, Clip, Page, Article, Asset }) + const app = createApp({ + Album, + Track, + Clip, + Page, + Article, + Brochure, + Asset + }) app.addStorage(new MemoryStorage(app, 'other', {} as StorageConfig)) const storage = app.getStorage('memory') as MemoryStorage @@ -1182,13 +1210,19 @@ describe('asset references', () => { expect(await getCount(file)).toBe(1) await Article.query().patchById(article.id, { content: { items: [] } }) expect(await getCount(file)).toBe(0) + await Article.query().patchById(article.id, { + content: { items: [{ file: copy(file) }] } + }) + expect(await getCount(file)).toBe(1) + await Article.query().deleteById(article.id) + expect(await getCount(file)).toBe(0) }) it('finds each location of overlapping data paths once, through the first', async () => { const file = await upload('overlapping-found.jpg') // Skip the asset hooks, to only test how references are found: await Article.knex()(Article.tableName).insert({ - content: JSON.stringify({ items: [{ file }, { file }] }) + content: JSON.stringify({ header: { file }, items: [{ file }, { file }] }) }) const references = await app.getAssetReferences({ keys: [file.key] }) const config = { storage: 'memory', readDimensions: true } @@ -1197,15 +1231,86 @@ describe('asset references', () => { ).toEqual( expect.arrayContaining([ { dataPath: 'content/items/0/file', config }, - { dataPath: 'content/items/1/file', config } + { dataPath: 'content/items/1/file', config }, + { dataPath: 'content/header/file', config: { storage: 'memory' } } ]) ) - expect(references).toHaveLength(2) + expect(references).toHaveLength(3) + }) + + it('filters overlapping data paths by the storage of the first', async () => { + const file = await upload('overlapping-filtered.jpg') + // Skip the asset hooks, to only test how references are found: + await Brochure.knex()(Brochure.tableName).insert({ + content: JSON.stringify({ items: [{ file }] }) + }) + expect( + await app.getAssetReferences({ keys: [file.key], storage: 'memory' }) + ).toHaveLength(1) expect( await app.getAssetReferences({ keys: [file.key], storage: 'other' }) ).toEqual([]) }) + it('loads files of overlapping data paths with the first storage', async () => { + const file = await upload('overlapping-loaded.jpg') + const brochure = await Brochure.query().insertAndFetch({ + content: { items: [{ file: copy(file) }] } + }) + const loaded = await Brochure.query().findById(brochure.id) + expect(loaded!.content!.items[0].file.storage.name).toBe('memory') + const json = loaded!.toJSON() + expect(storage.verifyAssetFile(json.content!.items[0].file)).toBe(true) + }) + + it('loads and signs the files of loaded records', async () => { + const cover = await upload('signed-cover.jpg') + const photo = await upload('signed-photo.jpg') + const album = await Album.query().insertAndFetch({ + cover: copy(cover), + photos: [copy(photo), null, copy(photo)] + }) + expect(await getCount(photo)).toBe(2) + const loaded = (await Album.query().findById(album.id))! + const [first, empty, last] = loaded.photos! + for (const file of [loaded.cover!, first!, last!]) { + expect(file).toBeInstanceOf(AssetFile) + expect(file.storage).toBe(storage) + } + expect(empty).toBeNull() + const json = loaded.toJSON() + const photos = json.photos! + for (const file of [json.cover!, photos[0]!, photos[2]!]) { + expect(storage.verifyAssetFile(file)).toBe(true) + } + expect(photos[1]).toBeNull() + // Sign copies, leaving the files of the record untouched: + expect(loaded.cover).not.toHaveProperty('signature') + expect(loaded.photos![0]).not.toHaveProperty('signature') + }) + + it('loads and signs the files of wildcard data paths', async () => { + const file = await upload('signed-nested.jpg') + const page = await Page.query().insertAndFetch({ + content: { + header: { file: copy(file) }, + footer: { file: null }, + gallery: { files: [copy(file), copy(file)] } + } + }) + const loaded = (await Page.query().findById(page.id))! + const { header, gallery } = loaded.content! + for (const loadedFile of [header.file, ...gallery.files]) { + expect(loadedFile).toBeInstanceOf(AssetFile) + expect(loadedFile.storage).toBe(storage) + } + const content = loaded.toJSON().content! + for (const signedFile of [content.header.file, ...content.gallery.files]) { + expect(storage.verifyAssetFile(signedFile)).toBe(true) + } + expect(content.footer.file).toBeNull() + }) + it('recounts each location of overlapping data paths once', async () => { const file = await upload('overlapping-recount.jpg') // Skip the asset hooks, to only test how references are counted: