diff --git a/.changeset/sharp-drinks-confess.md b/.changeset/sharp-drinks-confess.md new file mode 100644 index 00000000..1a415aa4 --- /dev/null +++ b/.changeset/sharp-drinks-confess.md @@ -0,0 +1,8 @@ +--- +"@aeriajs/builtins": patch +"@aeriajs/common": patch +"@aeriajs/types": patch +"@aeriajs/core": patch +--- + +Fix cascading remove diff --git a/packages/builtins/src/collections/file/remove.ts b/packages/builtins/src/collections/file/remove.ts index e6d33e30..bbd09b0b 100644 --- a/packages/builtins/src/collections/file/remove.ts +++ b/packages/builtins/src/collections/file/remove.ts @@ -3,10 +3,7 @@ import type { description } from './description.js' import { remove as originalRemove, type ObjectId } from '@aeriajs/core' import * as fs from 'fs/promises' -export const remove = async ( - payload: RemovePayload>, - context: Context, -) => { +export const remove = async (payload: RemovePayload>, context: Context) => { const file = await context.collection.model.findOne({ _id: payload.filters._id, }, { diff --git a/packages/builtins/src/collections/file/removeAll.ts b/packages/builtins/src/collections/file/removeAll.ts index 0955dd9c..05492c62 100644 --- a/packages/builtins/src/collections/file/removeAll.ts +++ b/packages/builtins/src/collections/file/removeAll.ts @@ -1,15 +1,12 @@ import type { Context, SchemaWithId, PackReferences, RemoveAllPayload } from '@aeriajs/types' import type { description } from './description.js' -import { remove as originalRemoveAll, type ObjectId } from '@aeriajs/core' +import { remove as originalRemoveAll } from '@aeriajs/core' import * as fs from 'fs/promises' -export const removeAll = async ( - payload: RemoveAllPayload, - context: Context, -) => { +export const removeAll = async (payload: RemoveAllPayload, context: Context) => { const files = context.collection.model.find({ _id: { - $in: payload.filters as ObjectId[], + $in: payload.filters, }, }, { projection: { diff --git a/packages/common/src/isReference.ts b/packages/common/src/isReference.ts index 8bd21608..50315c09 100644 --- a/packages/common/src/isReference.ts +++ b/packages/common/src/isReference.ts @@ -1,6 +1,6 @@ -import type { Property, RefProperty, ArrayOfRefs } from '@aeriajs/types' +import type { Property, RefProperty, ArrayProperty } from '@aeriajs/types' -export const isReference = (property: Property): property is RefProperty | ArrayOfRefs => { +export const isReference = (property: Property): property is RefProperty | ArrayProperty => { return 'items' in property ? '$ref' in property.items : '$ref' in property diff --git a/packages/core/src/collection/cascadingRemove.ts b/packages/core/src/collection/cascadingRemove.ts index 8b78b8b0..5448af35 100644 --- a/packages/core/src/collection/cascadingRemove.ts +++ b/packages/core/src/collection/cascadingRemove.ts @@ -1,4 +1,4 @@ -import type { Context } from '@aeriajs/types' +import type { Context, RouteContext } from '@aeriajs/types' import type * as functions from '../functions/index.js' import { ObjectId } from 'mongodb' import { createContext } from '../context.js' @@ -6,11 +6,34 @@ import { getFunction } from '../assets.js' import { getDatabaseCollection } from '../database.js' import { getReferences, type ReferenceMap, type Reference } from './reference.js' -const isObject = (value: unknown): value is Record => { - return typeof value === 'object' +const internalCascadingRemove = async (target: Record, refMap: ReferenceMap, context: RouteContext) => { + for( const refName in refMap ) { + const reference = refMap[refName] + + if( !target[refName] ) { + continue + } + + if( reference.referencedCollection ) { + if( reference.isInline || reference.referencedCollection === 'file' ) { + if( target[refName] instanceof ObjectId || Array.isArray(target[refName]) ) { + await preferredRemove(target[refName], reference, context) + } + } + } else if( reference.deepReferences ) { + if( Array.isArray(target[refName]) ) { + for( const elem of target[refName] ) { + await internalCascadingRemove(elem, reference.deepReferences, context) + } + continue + } + + await internalCascadingRemove(target[refName] as Record, reference.deepReferences, context) + } + } } -const preferredRemove = async (targetId: ObjectId | ObjectId[], reference: Reference, parentContext: Context) => { +export const preferredRemove = async (targetId: ObjectId | (ObjectId | null)[], reference: Reference, parentContext: RouteContext) => { if( !reference.referencedCollection ) { return } @@ -22,16 +45,21 @@ const preferredRemove = async (targetId: ObjectId | ObjectId[], reference: Refer }) if( Array.isArray(targetId) ) { + if( targetId.length === 0 ) { + return + } + + const nonNullable = targetId.filter((id) => !!id) const { result: removeAll } = await getFunction(reference.referencedCollection, 'removeAll') if( removeAll ) { return removeAll({ - filters: targetId, + filters: nonNullable, }, context) } return coll.deleteMany({ _id: { - $in: targetId, + $in: nonNullable, }, }) } @@ -50,34 +78,6 @@ const preferredRemove = async (targetId: ObjectId | ObjectId[], reference: Refer }) } -const internalCascadingRemove = async (target: Record, refMap: ReferenceMap, context: Context) => { - for( const refName in refMap ) { - const reference = refMap[refName] - if( !target[refName] ) { - continue - } - - if( reference.isInline || reference.referencedCollection === 'file' ) { - if( target[refName] instanceof ObjectId ) { - await preferredRemove(target[refName], reference, context) - } - } - - if( reference.deepReferences ) { - if( Array.isArray(target[refName]) ) { - for( const elem of target[refName] ) { - await internalCascadingRemove(elem, reference.deepReferences, context) - } - continue - } - - if( isObject(target[refName]) ) { - await internalCascadingRemove(target[refName], reference.deepReferences, context) - } - } - } -} - export const cascadingRemove = async (target: Record, context: Context) => { const refMap = await getReferences(context.description.properties) return internalCascadingRemove(target, refMap, context) diff --git a/packages/core/src/collection/reference.ts b/packages/core/src/collection/reference.ts index 49db159f..754cc650 100644 --- a/packages/core/src/collection/reference.ts +++ b/packages/core/src/collection/reference.ts @@ -67,8 +67,8 @@ export const getReferences = async (properties: FixedObjectProperty['properties' if( refProperty ) { const description = throwIfError(await getCollectionAsset(refProperty.$ref, 'description')) - if( refProperty.populate ) { - if( refProperty.populate.length === 0 ) { + if( refProperty.inline || refProperty.populate ) { + if( refProperty.populate && refProperty.populate.length === 0 ) { continue } @@ -76,7 +76,9 @@ export const getReferences = async (properties: FixedObjectProperty['properties' depth: depth + 1, maxDepth: refProperty.populateDepth || maxDepth, memoize: `${memoize}.${propName}`, - populate: Array.from(refProperty.populate), + populate: refProperty.populate + ? Array.from(refProperty.populate) + : undefined, isArrayElement: 'items' in property, }) diff --git a/packages/core/src/collection/traverseDocument.ts b/packages/core/src/collection/traverseDocument.ts index d9d2f87f..e97f8ff7 100644 --- a/packages/core/src/collection/traverseDocument.ts +++ b/packages/core/src/collection/traverseDocument.ts @@ -1,12 +1,15 @@ import type { WithId } from 'mongodb' import type { Description, Property, ValidationError, RouteContext } from '@aeriajs/types' import { Result, ACError, ValidationErrorCode, TraverseError } from '@aeriajs/types' -import { throwIfError, pipe, isReference, getValueFromPath, isError } from '@aeriajs/common' +import { throwIfError, pipe, isReference, getReferenceProperty, getValueFromPath, isError } from '@aeriajs/common' import { makeValidationError, validateProperty, validateWholeness } from '@aeriajs/validation' import { getCollection } from '@aeriajs/entrypoint' import { ObjectId } from 'mongodb' import { getCollectionAsset } from '../assets.js' +import { createContext } from '../context.js' import { preloadDescription } from './preload.js' +import { getReferences, type Reference } from './reference.js' +import { preferredRemove } from './cascadingRemove.js' import * as path from 'path' import * as fs from 'fs/promises' @@ -19,6 +22,7 @@ export type TraverseOptionsBase = { undefinedToNull?: boolean preserveHidden?: boolean recurseDeep?: boolean + cleanupReferences?: boolean recurseReferences?: boolean } @@ -85,60 +89,52 @@ const getProperty = (propName: string, parentProperty: Property | Description) = } } -const disposeOldFiles = async (ctx: PhaseContext, options: { preserveIds?: ObjectId[] } = {}) => { - if( !options.preserveIds && Array.isArray(ctx.target[ctx.propName]) ) { - return - } +const cleanupReferences = async (value: unknown, ctx: PhaseContext) => { + if( ctx.root._id ) { + const refProperty = getReferenceProperty(ctx.property) + if( refProperty && (refProperty.$ref === 'file' || refProperty.inline) ) { + if( ctx.isArray && !Array.isArray(value) ) { + return value + } - const context = ctx.options.context! + const context = ctx.options.context! - const doc = await context.collections[ctx.options.description.$id].model.findOne({ - _id: new ObjectId(ctx.root._id), - }, { - projection: { - [ctx.propPath]: 1, - }, - }) + const doc = await context.collections[ctx.options.description.$id].model.findOne({ + _id: new ObjectId(ctx.root._id), + }, { + projection: { + [ctx.propPath]: 1, + }, + }) - if( !doc ) { - return Result.error(TraverseError.InvalidDocumentId) - } + if( !doc ) { + return Result.error(TraverseError.InvalidDocumentId) + } - let fileIds = getValueFromPath<(ObjectId | null)[] | undefined>(doc, ctx.propPath) - if( !fileIds ) { - return - } + let referenceIds = getValueFromPath<(ObjectId | null)[] | ObjectId | undefined>(doc, ctx.propPath) + if( !referenceIds ) { + return value + } - if( options.preserveIds ) { - fileIds = fileIds.filter((id) => !id || !options.preserveIds!.some((fromId) => { - return id.equals(fromId) - })) - } + if( Array.isArray(referenceIds) ) { + if( !Array.isArray(value) ) { + throw new Error + } - const fileFilters = { - _id: { - $in: Array.isArray(fileIds) - ? fileIds - : [fileIds], - }, - } + referenceIds = referenceIds.filter((oldId) => !(value as ObjectId[]).some((valueId) => valueId.equals(oldId))) + } - const files = context.collections.file.model.find(fileFilters, { - projection: { - absolute_path: 1, - }, - }) + const refMap = await getReferences(ctx.options.description.properties) + const reference = getValueFromPath(refMap, ctx.propPath) - let file: Awaited> - while( file = await files.next() ) { - try { - await fs.unlink(file.absolute_path) - } catch( err ) { - console.trace(err) + await preferredRemove(referenceIds, reference, await createContext({ + parentContext: context, + collectionName: refProperty.$ref, + })) } } - return context.collections.file.model.deleteMany(fileFilters) + return value } const autoCast = (value: unknown, ctx: Omit & { options: (TraverseOptions & TraverseNormalized) | {} }): unknown => { @@ -276,9 +272,6 @@ const moveFiles = async (value: unknown, ctx: PhaseContext) => { } if( !value ) { - if( ctx.root._id && !ctx.isArray ) { - await disposeOldFiles(ctx) - } return null } @@ -298,10 +291,6 @@ const moveFiles = async (value: unknown, ctx: PhaseContext) => { return Result.error(TraverseError.InvalidTempfile) } - if( ctx.root._id && !ctx.isArray ) { - await disposeOldFiles(ctx) - } - const { _id: fileId, ...newFile } = tempFile newFile.absolute_path = `${ctx.options.context.config.storage!.fs}/${tempFile.absolute_path.split(path.sep).at(-1)}` newFile.owner = ctx.options.context.token.sub @@ -341,12 +330,6 @@ const recurseDeep = async (value: unknown, ctx: PhaseContext) => { items.push(result) } - if( 'moveFiles' in ctx.options && ctx.options.moveFiles && '$ref' in ctx.property.items && ctx.property.items.$ref === 'file' ) { - await disposeOldFiles(ctx, { - preserveIds: items, - }) - } - return items } @@ -592,6 +575,10 @@ export const traverseDocument = async ( functions.push(validate) } + if( options.cleanupReferences ) { + functions.push(cleanupReferences) + } + if( 'moveFiles' in options && options.moveFiles ) { functions.push(moveFiles) } diff --git a/packages/core/src/database.ts b/packages/core/src/database.ts index d6dd5824..283eb6f2 100644 --- a/packages/core/src/database.ts +++ b/packages/core/src/database.ts @@ -41,6 +41,7 @@ export const getDatabase = async () => { console.debug(inspect(event, { colors: true, compact: false, + depth: Infinity, })) }) } diff --git a/packages/core/src/functions/insert.ts b/packages/core/src/functions/insert.ts index bd336671..dda960a6 100644 --- a/packages/core/src/functions/insert.ts +++ b/packages/core/src/functions/insert.ts @@ -55,6 +55,7 @@ const internalInsert = async ( ? [] : context.description.required, moveFiles: true, + cleanupReferences: true, fromProperties: !isUpdate, undefinedToNull: true, preserveHidden: true, diff --git a/packages/core/src/functions/removeAll.ts b/packages/core/src/functions/removeAll.ts index daf49b89..f0c17be2 100644 --- a/packages/core/src/functions/removeAll.ts +++ b/packages/core/src/functions/removeAll.ts @@ -10,14 +10,11 @@ export type RemoveAllOptions = { } const internalRemoveAll = async (payload: RemoveAllPayload, context: TContext) => { - const filtersWithId = { - ...payload.filters, + const filters = throwIfError(await traverseDocument>({ _id: { $in: payload.filters, }, - } - - const filters = throwIfError(await traverseDocument>(filtersWithId, context.description, { + }, context.description, { autoCast: true, })) diff --git a/packages/core/tests/cascadingRemove.spec.ts b/packages/core/tests/cascadingRemove.spec.ts new file mode 100644 index 00000000..a72de72c --- /dev/null +++ b/packages/core/tests/cascadingRemove.spec.ts @@ -0,0 +1,77 @@ +import type { RouteContext } from '@aeriajs/types' +import { expect, test, beforeAll } from 'vitest' +import { throwIfError } from '@aeriajs/common' +import { createContext, ObjectId } from '../dist/index.js' +import { dbPromise } from './fixtures/database.js' +import { circularDocuments } from './fixtures/circularDocuments.js' + +let context: RouteContext + +beforeAll(async () => { + await dbPromise + context = await createContext({ + config: { + security: {}, + }, + }) +}) + + +test('cleanupReferences() removes replaced inline reference', async () => { + const { db } = await dbPromise + const { circularA2, circularA1, circularB1 } = await circularDocuments + + if( !db ) { + throw new Error + } + + throwIfError(await context.collections.circularA.functions.insert({ + what: { + _id: circularA2._id, + circularB: null, + } + })) + + const a1 = await db.collection('circularA').findOne({ + _id: circularA1, + }) + + const b1 = await db.collection('circularB').findOne({ + _id: circularB1, + }) + + expect(circularA2.circularB._id).toBeInstanceOf(ObjectId) + expect(a1).toBeNull() + expect(b1).toBeNull() +}) + +test('cleanupReferences() removes replaced inline reference inside array', async () => { + const { db } = await dbPromise + const { circularA2, circularA3, circularA4 } = await circularDocuments + + if( !db ) { + throw new Error + } + + throwIfError(await context.collections.circularA.functions.insert({ + what: { + _id: circularA2._id, + circularAs: [ + circularA3, + ], + } + })) + + const a3 = await db.collection('circularA').findOne({ + _id: circularA3, + }) + + const a4 = await db.collection('circularA').findOne({ + _id: circularA4, + }) + + expect(circularA2.circularB._id).toBeInstanceOf(ObjectId) + expect(a3).toBeTruthy() + expect(a4).toBeNull() +}) + diff --git a/packages/core/tests/fixtures/aeriaMain.js b/packages/core/tests/fixtures/aeriaMain.js index 42e125ff..361f1fa5 100644 --- a/packages/core/tests/fixtures/aeriaMain.js +++ b/packages/core/tests/fixtures/aeriaMain.js @@ -1,5 +1,5 @@ // @ts-check -const { init, user, file, tempFile, get } = require('aeria') +const { init, user, file, tempFile, get, insert, remove, removeAll } = require('aeria') exports.default = init({ collections: { @@ -9,6 +9,7 @@ exports.default = init({ circularA: { functions: { get, + insert, }, description: { $id: 'circularA', @@ -19,13 +20,17 @@ exports.default = init({ }, circularA: { $ref: 'circularA', - populate: [ - 'circularB', - 'circularB_array', - ] + }, + circularAs: { + type: 'array', + items: { + $ref: 'circularA', + inline: true, + }, }, circularB: { - $ref: 'circularB' + $ref: 'circularB', + inline: true, }, circularB_array: { type: 'array', @@ -42,16 +47,18 @@ exports.default = init({ circularB: { functions: { get, + remove, }, description: { - $id: 'circularA', + $id: 'circularB', required: [], properties: { name: { type: 'string', }, circularA: { - $ref: 'circularA' + $ref: 'circularA', + inline: true, }, } } diff --git a/packages/core/tests/fixtures/circularDocuments.ts b/packages/core/tests/fixtures/circularDocuments.ts index aacd2564..bc450a3d 100644 --- a/packages/core/tests/fixtures/circularDocuments.ts +++ b/packages/core/tests/fixtures/circularDocuments.ts @@ -19,9 +19,11 @@ export const circularDocuments = (async () => { token, }) - const { insertedId: circularA1 } = await db.collection('circularA').insertOne({ - name: 'rec a1', - }) + const { insertedIds: { '0': circularA1, '1': circularA3, '2': circularA4 } } = await db.collection('circularA').insertMany([ + { name: 'rec a1', }, + { name: 'rec a3', }, + { name: 'rec a4', }, + ]) const { insertedId: circularB1 } = await db.collection('circularB').insertOne({ name: 'rec b1', @@ -33,6 +35,10 @@ export const circularDocuments = (async () => { name: 'rec a2', circularA: circularA1, circularB: circularB1, + circularAs: [ + circularA3, + circularA4, + ], circularB_array: [ circularB1, ], @@ -42,6 +48,8 @@ export const circularDocuments = (async () => { return { circularA1, circularA2, + circularA3, + circularA4, circularB1, } })() diff --git a/packages/core/tests/fixtures/documents.ts b/packages/core/tests/fixtures/documents.ts index 315e4c16..8fdf899d 100644 --- a/packages/core/tests/fixtures/documents.ts +++ b/packages/core/tests/fixtures/documents.ts @@ -1,7 +1,7 @@ import type { Token } from '@aeriajs/types' import { throwIfError } from '@aeriajs/common' import { createContext, insert, ObjectId } from '../../dist/index.js' -import { dbPromise } from './database' +import { dbPromise } from './database.js' const token: Token = { authenticated: false, @@ -19,7 +19,7 @@ export const documents = (async () => { token, }) - const { insertedIds: { "0": file1, "1": file2, } } = await db.collection('file').insertMany([ + const { insertedIds: { '0': file1, '1': file2, } } = await db.collection('file').insertMany([ { name: 'picture1.jpg', }, @@ -28,7 +28,7 @@ export const documents = (async () => { } ]) - const { insertedIds: { "0": user1, "1": user2, "2": user3, } } = await db.collection('user').insertMany([ + const { insertedIds: { '0': user1, '1': user2, '2': user3, } } = await db.collection('user').insertMany([ { name: 'john', email: 'john@test', @@ -48,7 +48,7 @@ export const documents = (async () => { }, ]) - const { insertedIds: { "0": person1, "1": person2, "2": person3, } } = await db.collection('person').insertMany([ + const { insertedIds: { '0': person1, '1': person2, '2': person3, } } = await db.collection('person').insertMany([ { name: 'john', user: user1, @@ -70,7 +70,7 @@ export const documents = (async () => { } ]) - const { insertedIds: { "0": day1, "1": day2 } } = await db.collection('day').insertMany([ + const { insertedIds: { '0': day1, '1': day2 } } = await db.collection('day').insertMany([ { people: [ person1, diff --git a/packages/types/src/property.ts b/packages/types/src/property.ts index 6d4b0884..ce72ca2b 100644 --- a/packages/types/src/property.ts +++ b/packages/types/src/property.ts @@ -46,11 +46,11 @@ export type NonCircularJsonSchema & string indexes?: readonly string[] - populate?: readonly string[] select?: readonly string[] inline?: boolean form?: readonly string[] purge?: boolean + populate?: readonly string[] populateDepth?: number constraints?: Condition } @@ -130,10 +130,6 @@ export type BooleanProperty = { element?: 'checkbox' } -export type ArrayOfRefs = Omit & { - items: RefProperty -} - export type GetterProperty = { getter: (document: unknown & { _id: ObjectId }, context: RouteContext)=> unknown requires?: string[]