Skip to content

Commit

Permalink
feat(core): allow persisting 1:m collections
Browse files Browse the repository at this point in the history
Previously only initialized items were allowed to be persisted when adding to
1:m collection, due to how propagation works. With this change we now allow
having inverse side collections dirty as well, firing single update query
for those if we see the items are now initialized.

Closes #467
  • Loading branch information
B4nan committed Jul 26, 2020
1 parent ff532fc commit f61c717
Show file tree
Hide file tree
Showing 8 changed files with 143 additions and 19 deletions.
2 changes: 1 addition & 1 deletion packages/core/src/entity/Collection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ export class Collection<T extends AnyEntity<T>, O extends AnyEntity<O> = AnyEnti
}

setDirty(dirty = true): void {
this.dirty = dirty && !!this.property.owner; // set dirty flag only to owning side
this.dirty = dirty;
}

async init(options?: InitOptions<T>): Promise<this>;
Expand Down
37 changes: 26 additions & 11 deletions packages/core/src/unit-of-work/ChangeSetComputer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,35 +52,50 @@ export class ChangeSetComputer {
}

private processReference<T extends AnyEntity<T>>(changeSet: ChangeSet<T>, prop: EntityProperty<T>): void {
const isToOneOwner = prop.reference === ReferenceType.MANY_TO_ONE || (prop.reference === ReferenceType.ONE_TO_ONE && prop.owner);
const target = changeSet.entity[prop.name];

if ([ReferenceType.ONE_TO_MANY, ReferenceType.MANY_TO_MANY].includes(prop.reference) && (changeSet.entity[prop.name] as unknown as Collection<T>).isInitialized()) {
const collection = changeSet.entity[prop.name] as unknown as Collection<AnyEntity>;
collection.getItems()
// remove items from collection based on removeStack
if (Utils.isCollection<T>(target) && target.isInitialized()) {
target.getItems()
.filter(item => this.removeStack.includes(item))
.forEach(item => collection.remove(item));
.forEach(item => target.remove(item));
}

if (prop.reference === ReferenceType.MANY_TO_MANY && prop.owner && (changeSet.entity[prop.name] as unknown as Collection<T>).isDirty()) {
this.collectionUpdates.push(changeSet.entity[prop.name] as unknown as Collection<AnyEntity>);
} else if (isToOneOwner && changeSet.entity[prop.name]) {
this.processManyToOne(prop, changeSet);
if (Utils.isCollection(target)) { // m:n or 1:m
this.processToMany(prop, changeSet);
} else if (prop.reference !== ReferenceType.SCALAR && target) { // m:1 or 1:1
this.processToOne(prop, changeSet);
}

if (prop.reference === ReferenceType.ONE_TO_ONE) {
this.processOneToOne(prop, changeSet);
}
}

private processManyToOne<T extends AnyEntity<T>>(prop: EntityProperty<T>, changeSet: ChangeSet<T>): void {
private processToOne<T extends AnyEntity<T>>(prop: EntityProperty<T>, changeSet: ChangeSet<T>): void {
const pks = this.metadata.get(prop.type).primaryKeys;
const entity = changeSet.entity[prop.name] as unknown as T;
const isToOneOwner = prop.reference === ReferenceType.MANY_TO_ONE || (prop.reference === ReferenceType.ONE_TO_ONE && prop.owner);

if (pks.length === 1 && !Utils.isDefined(entity[pks[0]], true)) {
if (isToOneOwner && pks.length === 1 && !Utils.isDefined(entity[pks[0]], true)) {
changeSet.payload[prop.name] = this.identifierMap[wrap(entity, true).__uuid];
}
}

private processToMany<T extends AnyEntity<T>>(prop: EntityProperty<T>, changeSet: ChangeSet<T>): void {
const target = changeSet.entity[prop.name] as unknown as Collection<any>;

if (!target.isDirty()) {
return;
}

if (prop.owner || target.getItems(false).filter(item => !wrap(item).isInitialized()).length > 0) {
this.collectionUpdates.push(target);
} else {
target.setDirty(false); // inverse side with only populated items, nothing to persist
}
}

private processOneToOne<T extends AnyEntity<T>>(prop: EntityProperty<T>, changeSet: ChangeSet<T>): void {
// check diff, if we had a value on 1:1 before and now it changed (nulled or replaced), we need to trigger orphan removal
const wrapped = wrap(changeSet.entity, true);
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/unit-of-work/UnitOfWork.ts
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ export class UnitOfWork {
return this.processToOneReference(reference, visited);
}

if (Utils.isCollection(reference, prop, ReferenceType.MANY_TO_MANY) && reference.isDirty()) {
if (Utils.isCollection<any>(reference, prop, ReferenceType.MANY_TO_MANY) && reference.isDirty()) {
this.processToManyReference(reference, visited, parent, prop);
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/utils/Utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ export class Utils {
return ret;
}

static isCollection(item: any, prop?: EntityProperty, type?: ReferenceType): item is Collection<AnyEntity> {
static isCollection<T extends AnyEntity<T>, O extends AnyEntity<O> = AnyEntity>(item: any, prop?: EntityProperty<T>, type?: ReferenceType): item is Collection<T, O> {
if (!(item instanceof Collection)) {
return false;
}
Expand Down
12 changes: 11 additions & 1 deletion packages/knex/src/AbstractSqlDriver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,17 @@ export abstract class AbstractSqlDriver<C extends AbstractSqlConnection = Abstra
insertDiff.push(...current);
}

await this.rethrow(this.updateCollectionDiff<T, O>(meta, coll.property, pks, deleteDiff, insertDiff, ctx));
if (coll.property.reference === ReferenceType.ONE_TO_MANY) {
const cols = coll.property.referencedColumnNames;
const cond = cols.length > 1 ? { [Utils.getPrimaryKeyHash(cols)]: { $in: insertDiff } } : { [cols[0]]: { $in: insertDiff.map(i => i[0]) } };
const qb = this.createQueryBuilder(coll.property.type, ctx, true)
.update({ [coll.property.mappedBy]: pks })
.where(cond);

return this.rethrow(qb.execute('run', false));
}

return this.rethrow(this.updateCollectionDiff<T, O>(meta, coll.property, pks, deleteDiff, insertDiff, ctx));
}

async loadFromPivotTable<T extends AnyEntity<T>, O extends AnyEntity<O>>(prop: EntityProperty, owners: Primary<O>[][], where?: FilterQuery<T>, orderBy?: QueryOrderMap, ctx?: Transaction): Promise<Dictionary<T[]>> {
Expand Down
4 changes: 2 additions & 2 deletions tests/EntityFactory.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ describe('EntityFactory', () => {
expect(a1.name).toBe('Jon');
expect(a1.email).toBe('jon@snow.com');
expect(a1.books.isInitialized()).toBe(true);
expect(a1.books.isDirty()).toBe(false); // inverse side
expect(a1.books.isDirty()).toBe(true);
expect(a1.books[0].title).toBe('B1');
expect(a1.books[0].author).toBe(a1); // propagation to owning side
expect(a1.books[0].publisher.id).toBe('5b0d19b28b21c648c2c8a600');
Expand Down Expand Up @@ -211,7 +211,7 @@ describe('EntityFactory', () => {
expect(a2.name).toBe('Jon');
expect(a2.email).toBe('jon2@snow.com');
expect(a2.books.isInitialized()).toBe(true);
expect(a2.books.isDirty()).toBe(false); // inverse side
expect(a2.books.isDirty()).toBe(true);
expect(a2.books[0].title).toBe('B1');
expect(a2.books[0].author).toBe(a2); // propagation to owning side
expect(a2.books[0].publisher.id).toBe('5b0d19b28b21c648c2c8a600');
Expand Down
4 changes: 2 additions & 2 deletions tests/EntityHelper.mongo.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ describe('EntityAssignerMongo', () => {
' publisher: [Reference]\n' +
' },\n' +
' initialized: true,\n' +
' dirty: false\n' +
' dirty: true\n' +
' },\n' +
' friends: Collection { initialized: true, dirty: false },\n' +
" name: 'God',\n" +
Expand All @@ -249,7 +249,7 @@ describe('EntityAssignerMongo', () => {
' favouriteAuthor: Author {\n' +
' hookTest: false,\n' +
' termsAccepted: false,\n' +
" books: Collection { '0': [Book], initialized: true, dirty: false },\n" +
" books: Collection { '0': [Book], initialized: true, dirty: true },\n" +
' friends: Collection { initialized: true, dirty: false },\n' +
" name: 'God',\n" +
" email: 'hello@heaven.god',\n" +
Expand Down
99 changes: 99 additions & 0 deletions tests/issues/GH467.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
import { unlinkSync } from 'fs';
import { Collection, Entity, PrimaryKey, ManyToOne, OneToMany, MikroORM, IdentifiedReference } from '@mikro-orm/core';
import { SqliteDriver } from '@mikro-orm/sqlite';

@Entity()
class A {

@PrimaryKey()
id!: string;

@ManyToOne({ entity: 'B', wrappedReference: true, nullable: true })
b?: IdentifiedReference<B>;

}

@Entity()
class B {

@PrimaryKey()
id!: string;

@OneToMany(() => A, a => a.b)
as = new Collection<A>(this);

}

describe('GH issue 467', () => {

let orm: MikroORM<SqliteDriver>;

beforeAll(async () => {
orm = await MikroORM.init({
entities: [A, B],
dbName: __dirname + '/../../temp/mikro_orm_test_gh467.db',
debug: false,
highlight: false,
type: 'sqlite',
});
await orm.getSchemaGenerator().dropSchema();
await orm.getSchemaGenerator().createSchema();
});

afterAll(async () => {
await orm.close(true);
unlinkSync(orm.config.get('dbName')!);
});

test(`wrap().assign to collections is persisted`, async () => {
const a = new A();
a.id = 'a1';
await orm.em.persistAndFlush(a);

const b = new B();
orm.em.assign(b, {
id: 'b1',
as: ['a1'],
});
await orm.em.persistAndFlush(b);
orm.em.clear();

const b1 = await orm.em.findOneOrFail(B, 'b1', ['as']);
expect(b1.as.getIdentifiers()).toEqual(['a1']);
});

test(`assigning to new collection from inverse side is persisted`, async () => {
const a = new A();
a.id = 'a2';
await orm.em.persistAndFlush(a);

const b = new B();
b.id = 'b2';
b.as.set([orm.em.getReference(A, 'a2')]);
await orm.em.persistAndFlush(b);
orm.em.clear();

const b1 = await orm.em.findOneOrFail(B, 'b2', ['as']);
expect(b1.as.getIdentifiers()).toEqual(['a2']);
});

test(`assigning to loaded collection from inverse side is persisted`, async () => {
const a = new A();
a.id = 'a3';
await orm.em.persistAndFlush(a);

const b = new B();
b.id = 'b3';
await orm.em.persistAndFlush(b);
orm.em.clear();

const b1 = await orm.em.findOneOrFail(B, 'b3', ['as']);
b1.as.set([orm.em.getReference(A, 'a3')]);
await orm.em.flush();
orm.em.clear();

const b2 = await orm.em.findOneOrFail(B, 'b3', ['as']);
expect(b2.as.getIdentifiers()).toEqual(['a3']);
});

});

0 comments on commit f61c717

Please sign in to comment.