diff --git a/packages/core/src/entity/ArrayCollection.ts b/packages/core/src/entity/ArrayCollection.ts index 88ebf9334f1f..05d447e8be16 100644 --- a/packages/core/src/entity/ArrayCollection.ts +++ b/packages/core/src/entity/ArrayCollection.ts @@ -48,7 +48,7 @@ export class ArrayCollection, O extends AnyEntity> { add(...items: T[]): void { for (const item of items) { - if (!this.contains(item)) { + if (!this.contains(item, false)) { this.items.push(item); this.propagate(item, 'add'); } @@ -86,7 +86,7 @@ export class ArrayCollection, O extends AnyEntity> { this.remove(...this.items); } - contains(item: T): boolean { + contains(item: T, check?: boolean): boolean { return !!this.items.find(i => { const objectIdentity = i === item; const primaryKeyIdentity = !!wrap(i).__primaryKey && !!wrap(item).__primaryKey && wrap(i).__serializedPrimaryKey === wrap(item).__serializedPrimaryKey; @@ -149,16 +149,16 @@ export class ArrayCollection, O extends AnyEntity> { } protected shouldPropagateToCollection(collection: Collection, method: 'add' | 'remove'): boolean { - if (!collection || !collection.isInitialized()) { + if (!collection) { return false; } if (method === 'add') { - return !collection.contains(this.owner); + return !collection.contains(this.owner, false); } // remove - return collection.contains(this.owner); + return collection.contains(this.owner, false); } } diff --git a/packages/core/src/entity/Collection.ts b/packages/core/src/entity/Collection.ts index de1a1b9e94c2..5b6178bc5f9c 100644 --- a/packages/core/src/entity/Collection.ts +++ b/packages/core/src/entity/Collection.ts @@ -33,8 +33,11 @@ export class Collection, O extends AnyEntity = AnyEnti /** * Returns the items (the collection must be initialized) */ - getItems(): T[] { - this.checkInitialized(); + getItems(check = true): T[] { + if (check) { + this.checkInitialized(); + } + return super.getItems(); } @@ -79,8 +82,11 @@ export class Collection, O extends AnyEntity = AnyEnti } } - contains(item: T): boolean { - this.checkInitialized(); + contains(item: T, check = true): boolean { + if (check) { + this.checkInitialized(); + } + return super.contains(item); } @@ -206,7 +212,10 @@ export class Collection, O extends AnyEntity = AnyEnti } private modify(method: 'add' | 'remove', items: T[]): void { - this.checkInitialized(); + if (method === 'remove') { + this.checkInitialized(); + } + this.validateModification(items); super[method](...items); this.setDirty(); @@ -249,7 +258,7 @@ export class Collection, O extends AnyEntity = AnyEnti // throw if we are modifying inverse side of M:N collection when owning side is initialized (would be ignored when persisting) const manyToManyInverse = this.property.reference === ReferenceType.MANY_TO_MANY && this.property.mappedBy; - if (manyToManyInverse && items.find(item => !item[this.property.mappedBy] || !item[this.property.mappedBy].isInitialized())) { + if (manyToManyInverse && items.find(item => !wrap(item).isInitialized() || !item[this.property.mappedBy])) { throw ValidationError.cannotModifyInverseCollection(this.owner, this.property); } } diff --git a/packages/core/src/unit-of-work/UnitOfWork.ts b/packages/core/src/unit-of-work/UnitOfWork.ts index f586800bbd5a..3f59e4b04247 100644 --- a/packages/core/src/unit-of-work/UnitOfWork.ts +++ b/packages/core/src/unit-of-work/UnitOfWork.ts @@ -241,7 +241,7 @@ export class UnitOfWork { return; } - reference.getItems() + reference.getItems(false) .filter(item => !this.originalEntityData[wrap(item).__uuid]) .forEach(item => this.findNewEntities(item, visited)); } @@ -350,13 +350,16 @@ export class UnitOfWork { const collection = reference as Collection; const requireFullyInitialized = type === Cascade.PERSIST; // only cascade persist needs fully initialized items - if ([ReferenceType.ONE_TO_MANY, ReferenceType.MANY_TO_MANY].includes(prop.reference) && collection && collection.isInitialized(requireFullyInitialized)) { - collection.getItems().forEach(item => this.cascade(item, type, visited, options)); + if ([ReferenceType.ONE_TO_MANY, ReferenceType.MANY_TO_MANY].includes(prop.reference) && collection) { + collection + .getItems(false) + .filter(item => !requireFullyInitialized || wrap(item).isInitialized()) + .forEach(item => this.cascade(item, type, visited, options)); } } private isCollectionSelfReferenced(collection: Collection, visited: AnyEntity[]): boolean { - const filtered = collection.getItems().filter(item => !this.originalEntityData[wrap(item).__uuid]); + const filtered = collection.getItems(false).filter(item => !this.originalEntityData[wrap(item).__uuid]); return filtered.some(items => visited.includes(items)); } diff --git a/packages/knex/src/AbstractSqlDriver.ts b/packages/knex/src/AbstractSqlDriver.ts index 265cb43ecbb2..515c48283905 100644 --- a/packages/knex/src/AbstractSqlDriver.ts +++ b/packages/knex/src/AbstractSqlDriver.ts @@ -138,7 +138,7 @@ export abstract class AbstractSqlDriver wrap(item).__primaryKeys); - const current = coll.getItems().map(item => wrap(item).__primaryKeys); + const current = coll.getItems(false).map(item => wrap(item).__primaryKeys); const deleteDiff = snapshot.filter(item => !current.includes(item)); const insertDiff = current.filter(item => !snapshot.includes(item)); const target = snapshot.filter(item => current.includes(item)).concat(...insertDiff); diff --git a/tests/EntityManager.mongo.test.ts b/tests/EntityManager.mongo.test.ts index 4602f410ed2b..ee14aec2dfff 100644 --- a/tests/EntityManager.mongo.test.ts +++ b/tests/EntityManager.mongo.test.ts @@ -8,6 +8,7 @@ import { AuthorRepository } from './repositories/AuthorRepository'; import { initORMMongo, wipeDatabase } from './bootstrap'; import FooBar from './entities/FooBar'; import { FooBaz } from './entities/FooBaz'; +import { Author2, Book2, BookTag2 } from './entities-sql'; describe('EntityManagerMongo', () => { @@ -626,7 +627,6 @@ describe('EntityManagerMongo', () => { expect(tags[0].books.isInitialized()).toBe(false); expect(tags[0].books.isDirty()).toBe(false); expect(() => tags[0].books.getItems()).toThrowError(/Collection of entity BookTag\[\w{24}] not initialized/); - expect(() => tags[0].books.add(book1)).toThrowError(/Collection of entity BookTag\[\w{24}] not initialized/); expect(() => tags[0].books.remove(book1, book2)).toThrowError(/Collection of entity BookTag\[\w{24}] not initialized/); expect(() => tags[0].books.removeAll()).toThrowError(/Collection of entity BookTag\[\w{24}] not initialized/); expect(() => tags[0].books.contains(book1)).toThrowError(/Collection of entity BookTag\[\w{24}] not initialized/); @@ -1829,6 +1829,49 @@ describe('EntityManagerMongo', () => { expect(res1).toBeNull(); }); + test('adding items to not initialized collection', async () => { + const god = new Author('God', 'hello@heaven.god'); + const b1 = new Book('Bible 1', god); + await orm.em.persistAndFlush(b1); + orm.em.clear(); + + const a = await orm.em.findOneOrFail(Author, god.id); + expect(a.books.isInitialized()).toBe(false); + const b2 = new Book('Bible 2'); + const b3 = new Book('Bible 3'); + a.books.add(b2, b3); + await orm.em.flush(); + orm.em.clear(); + + const a2 = await orm.em.findOneOrFail(Author, god.id, ['books']); + expect(a2.books.count()).toBe(3); + expect(a2.books.getIdentifiers('id')).toEqual([b1.id, b2.id, b3.id]); + + const tag1 = new BookTag('silly'); + const tag2 = new BookTag('funny'); + const tag3 = new BookTag('sick'); + const tag4 = new BookTag('strange'); + let tag5 = new BookTag('sexy'); + a2.books[0].tags.add(tag1); + a2.books[1].tags.add(tag1); + a2.books[2].tags.add(tag5); + await orm.em.flush(); + orm.em.clear(); + + const a3 = await orm.em.findOneOrFail(Author, god.id, ['books']); + tag5 = orm.em.getReference(BookTag, tag5.id); + a3.books[0].tags.add(tag3); + a3.books[1].tags.add(tag2, tag5); + a3.books[2].tags.add(tag4); + await orm.em.flush(); + orm.em.clear(); + + const a4 = await orm.em.findOneOrFail(Author, god.id, ['books.tags']); + expect(a4.books[0].tags.getIdentifiers()).toEqual([tag1.id, tag3.id]); + expect(a4.books[1].tags.getIdentifiers()).toEqual([tag1.id, tag2.id, tag5.id]); + expect(a4.books[2].tags.getIdentifiers()).toEqual([tag5.id, tag4.id]); + }); + // this should run in ~600ms (when running single test locally) test('perf: one to many', async () => { const author = new Author('Jon Snow', 'snow@wall.st'); diff --git a/tests/EntityManager.mysql.test.ts b/tests/EntityManager.mysql.test.ts index d66cf8602c50..d8baa304c06a 100644 --- a/tests/EntityManager.mysql.test.ts +++ b/tests/EntityManager.mysql.test.ts @@ -913,7 +913,6 @@ describe('EntityManagerMySql', () => { expect(tags[0].books.isInitialized()).toBe(false); expect(tags[0].books.isDirty()).toBe(false); expect(() => tags[0].books.getItems()).toThrowError(/Collection of entity BookTag2\[\d+] not initialized/); - expect(() => tags[0].books.add(book1)).toThrowError(/Collection of entity BookTag2\[\d+] not initialized/); expect(() => tags[0].books.remove(book1, book2)).toThrowError(/Collection of entity BookTag2\[\d+] not initialized/); expect(() => tags[0].books.removeAll()).toThrowError(/Collection of entity BookTag2\[\d+] not initialized/); expect(() => tags[0].books.contains(book1)).toThrowError(/Collection of entity BookTag2\[\d+] not initialized/); @@ -1854,6 +1853,56 @@ describe('EntityManagerMySql', () => { expect(() => orm.em.remove(Book2, null)).toThrowError(`You cannot call 'EntityManager.remove()' with empty 'where' parameter. If you want to remove all entities, use 'em.remove(Book2, {})'.`); }); + test('adding items to not initialized collection', async () => { + const god = new Author2('God', 'hello@heaven.god'); + const b1 = new Book2('Bible 1', god); + await orm.em.persistAndFlush(b1); + orm.em.clear(); + + const a = await orm.em.findOneOrFail(Author2, god.id); + expect(a.books.isInitialized()).toBe(false); + const b2 = new Book2('Bible 2', a); + const b3 = new Book2('Bible 3', a); + a.books.add(b2, b3); + await orm.em.flush(); + orm.em.clear(); + + const a2 = await orm.em.findOneOrFail(Author2, god.id, ['books']); + expect(a2.books.count()).toBe(3); + expect(a2.books.getIdentifiers()).toEqual([b1.uuid, b2.uuid, b3.uuid]); + + const tag1 = new BookTag2('silly'); + const tag2 = new BookTag2('funny'); + const tag3 = new BookTag2('sick'); + const tag4 = new BookTag2('strange'); + let tag5 = new BookTag2('sexy'); + a2.books[0].tags.add(tag1); + a2.books[1].tags.add(tag1); + a2.books[2].tags.add(tag5); + await orm.em.flush(); + orm.em.clear(); + + const a3 = await orm.em.findOneOrFail(Author2, god.id, ['books']); + tag5 = orm.em.getReference(BookTag2, tag5.id); + a3.books[0].tags.add(tag3); + a3.books[1].tags.add(tag2, tag5); + a3.books[2].tags.add(tag4); + await orm.em.flush(); + orm.em.clear(); + + const a4 = await orm.em.findOneOrFail(Author2, god.id, ['books.tags']); + expect(a4.books[0].tags.getIdentifiers()).toEqual([tag1.id, tag3.id]); + expect(a4.books[1].tags.getIdentifiers()).toEqual([tag1.id, tag2.id, tag5.id]); + expect(a4.books[2].tags.getIdentifiers()).toEqual([tag5.id, tag4.id]); + orm.em.clear(); + + const tag = await orm.em.findOneOrFail(BookTag2, tag1.id); + const book = await orm.em.findOneOrFail(Book2, b3.uuid); + tag.books.add(book); + await orm.em.flush(); + orm.em.clear(); + }); + afterAll(async () => orm.close(true)); }); diff --git a/tests/EntityManager.postgre.test.ts b/tests/EntityManager.postgre.test.ts index 5d8793ce3b94..8a7861a80e09 100644 --- a/tests/EntityManager.postgre.test.ts +++ b/tests/EntityManager.postgre.test.ts @@ -706,7 +706,6 @@ describe('EntityManagerPostgre', () => { expect(tags[0].books.isInitialized()).toBe(false); expect(tags[0].books.isDirty()).toBe(false); expect(() => tags[0].books.getItems()).toThrowError(/Collection of entity BookTag2\[\d+] not initialized/); - expect(() => tags[0].books.add(book1)).toThrowError(/Collection of entity BookTag2\[\d+] not initialized/); expect(() => tags[0].books.remove(book1, book2)).toThrowError(/Collection of entity BookTag2\[\d+] not initialized/); expect(() => tags[0].books.removeAll()).toThrowError(/Collection of entity BookTag2\[\d+] not initialized/); expect(() => tags[0].books.contains(book1)).toThrowError(/Collection of entity BookTag2\[\d+] not initialized/); diff --git a/tests/EntityManager.sqlite.test.ts b/tests/EntityManager.sqlite.test.ts index d7ae7ff11d57..66d73bcc3e54 100644 --- a/tests/EntityManager.sqlite.test.ts +++ b/tests/EntityManager.sqlite.test.ts @@ -582,7 +582,6 @@ describe('EntityManagerSqlite', () => { expect(tags[0].books.isInitialized()).toBe(false); expect(tags[0].books.isDirty()).toBe(false); expect(() => tags[0].books.getItems()).toThrowError(/Collection of entity BookTag3\[\d+] not initialized/); - expect(() => tags[0].books.add(book1)).toThrowError(/Collection of entity BookTag3\[\d+] not initialized/); expect(() => tags[0].books.remove(book1, book2)).toThrowError(/Collection of entity BookTag3\[\d+] not initialized/); expect(() => tags[0].books.removeAll()).toThrowError(/Collection of entity BookTag3\[\d+] not initialized/); expect(() => tags[0].books.contains(book1)).toThrowError(/Collection of entity BookTag3\[\d+] not initialized/); diff --git a/tests/EntityManager.sqlite2.test.ts b/tests/EntityManager.sqlite2.test.ts index c1177e81acb6..f57d83e4848b 100644 --- a/tests/EntityManager.sqlite2.test.ts +++ b/tests/EntityManager.sqlite2.test.ts @@ -541,7 +541,6 @@ describe('EntityManagerSqlite2', () => { expect(tags[0].books.isInitialized()).toBe(false); expect(tags[0].books.isDirty()).toBe(false); expect(() => tags[0].books.getItems()).toThrowError(/Collection of entity BookTag4\[\d+] not initialized/); - expect(() => tags[0].books.add(book1)).toThrowError(/Collection of entity BookTag4\[\d+] not initialized/); expect(() => tags[0].books.remove(book1, book2)).toThrowError(/Collection of entity BookTag4\[\d+] not initialized/); expect(() => tags[0].books.removeAll()).toThrowError(/Collection of entity BookTag4\[\d+] not initialized/); expect(() => tags[0].books.contains(book1)).toThrowError(/Collection of entity BookTag4\[\d+] not initialized/);