Skip to content

Commit

Permalink
fix(core): do not cascade merge new entities
Browse files Browse the repository at this point in the history
When loading an entity from database, we could end up cascade
merging some other new entity (created via ctor), that should
produce an insert query. With this fix, we ignore new entities
in the `UoW.merge()`.

Closes #811
  • Loading branch information
B4nan committed Sep 8, 2020
1 parent e072983 commit 2b0f208
Show file tree
Hide file tree
Showing 7 changed files with 34 additions and 19 deletions.
7 changes: 4 additions & 3 deletions packages/core/src/entity/EntityFactory.ts
@@ -1,5 +1,5 @@
import { Utils } from '../utils';
import { Dictionary, EntityData, EntityMetadata, EntityName, EntityProperty, New, Populate, Primary } from '../typings';
import { Dictionary, EntityData, EntityMetadata, EntityName, EntityProperty, New, Populate, Primary, AnyEntity } from '../typings';
import { UnitOfWork } from '../unit-of-work';
import { ReferenceType } from './enums';
import { EntityManager, EventType } from '..';
Expand Down Expand Up @@ -104,11 +104,12 @@ export class EntityFactory {
}

// creates new entity instance, bypassing constructor call as its already persisted entity
const entity = Object.create(Entity.prototype);
const entity = Object.create(Entity.prototype) as T & AnyEntity<T>;
entity.__helper!.__managed = true;
this.hydrator.hydrateReference(entity, meta, data, convertCustomTypes);

if (merge) {
this.unitOfWork.merge(entity, new Set([entity]), false);
this.unitOfWork.merge<T>(entity, new Set([entity]), false);
}

return entity;
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/entity/WrappedEntity.ts
Expand Up @@ -16,6 +16,7 @@ export class WrappedEntity<T extends AnyEntity<T>, PK extends keyof T> {
__initialized = true;
__populated = false;
__lazyInitialized = false;
__managed = false;
__em?: EntityManager;

readonly __uuid = uuid();
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/typings.ts
Expand Up @@ -80,6 +80,7 @@ export interface IWrappedEntityInternal<T extends AnyEntity<T>, PK extends keyof
__data: Dictionary;
__em?: EntityManager;
__initialized?: boolean;
__managed: boolean;
__populated: boolean;
__lazyInitialized: boolean;
__primaryKey: PrimaryMap<T>;
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/unit-of-work/ChangeSetPersister.ts
Expand Up @@ -65,6 +65,7 @@ export class ChangeSetPersister {

this.markAsPopulated(changeSet, meta);
wrapped.__initialized = true;
wrapped.__managed = true;
await this.processOptimisticLock(meta, changeSet, res, ctx);
changeSet.persisted = true;
}
Expand Down
5 changes: 5 additions & 0 deletions packages/core/src/unit-of-work/UnitOfWork.ts
Expand Up @@ -40,6 +40,11 @@ export class UnitOfWork {
return;
}

// skip new entities that could be linked from already persisted entity that is being re-fetched
if (!entity.__helper!.__managed) {
return;
}

const root = Utils.getRootEntity(this.metadata, wrapped.__meta);
this.identityMap.set(`${root.name}-${wrapped.__serializedPrimaryKey}`, entity);

Expand Down
9 changes: 3 additions & 6 deletions tests/UnitOfWork.test.ts
Expand Up @@ -85,8 +85,7 @@ describe('UnitOfWork', () => {
});

test('changeSet is null for empty payload', async () => {
const author = new Author('test', 'test');
author.id = '00000001885f0a3cc37dc9f0';
const author = orm.em.create(Author, { id: '00000001885f0a3cc37dc9f0', name: 'test', email: 'test' });
uow.merge(author); // add entity to IM first
const changeSet = await computer.computeChangeSet(author); // then try to persist it again
expect(changeSet).toBeNull();
Expand All @@ -103,8 +102,7 @@ describe('UnitOfWork', () => {
});

test('persist and remove will add entity to given stack only once', async () => {
const author = new Author('test', 'test');
author.id = '00000001885f0a3cc37dc9f0';
const author = orm.em.create(Author, { id: '00000001885f0a3cc37dc9f0', name: 'test', email: 'test' });
uow.persist(author);
expect(uow.getPersistStack().size).toBe(1);
uow.persist(author);
Expand All @@ -120,8 +118,7 @@ describe('UnitOfWork', () => {

test('getters', async () => {
const uow = new UnitOfWork(orm.em);
const author = new Author('test', 'test');
author.id = '00000001885f0a3cc37dc9f0';
const author = orm.em.create(Author, { id: '00000001885f0a3cc37dc9f0', name: 'test', email: 'test' });
uow.persist(author);
expect([...uow.getPersistStack()]).toEqual([author]);
expect([...uow.getRemoveStack()]).toEqual([]);
Expand Down
29 changes: 19 additions & 10 deletions tests/issues/GH811.test.ts
@@ -1,6 +1,5 @@
import { Entity, MikroORM, OneToOne, PrimaryKey, Property } from '@mikro-orm/core';
import { SchemaGenerator, SqliteDriver } from '@mikro-orm/sqlite';

import { PostgreSqlDriver } from '@mikro-orm/postgresql';
import { v4 } from 'uuid';

@Entity()
Expand Down Expand Up @@ -44,22 +43,23 @@ export class Employee {

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

let orm: MikroORM<SqliteDriver>;
let orm: MikroORM<PostgreSqlDriver>;

beforeAll(async () => {
orm = await MikroORM.init({
entities: [Contact, Employee, Address],
dbName: ':memory:',
type: 'sqlite',
dbName: 'mikro_orm_test_gh811',
type: 'postgresql',
});
await new SchemaGenerator(orm.em).dropSchema();
await new SchemaGenerator(orm.em).createSchema();
await orm.getSchemaGenerator().ensureDatabase();
await orm.getSchemaGenerator().dropSchema();
await orm.getSchemaGenerator().createSchema();
});

afterAll(() => orm.close(true));

test('811', async () => {
// Create a Contact and and Employee
test('loading entity will not cascade merge new entities in the entity graph', async () => {
// Create a Contact and an Employee
const contactCreate = new Contact();
contactCreate.name = 'My Contact';
const employeeCreate = new Employee();
Expand All @@ -86,7 +86,16 @@ describe('GH issue 811', () => {
contact.address = address;

// Find my previously created employee
const employee = await orm.em.findOneOrFail(Employee, employeeCreate.id); // This line causes the error!
expect([...orm.em.getUnitOfWork().getOriginalEntityData().values()]).toEqual([
{ id: contact.id, name: 'My Contact', address: null },
]);
const employee = await orm.em.findOneOrFail(Employee, employeeCreate.id);

// previously the `Employee.contact.address` was accidentally cascade merged
expect([...orm.em.getUnitOfWork().getOriginalEntityData().values()]).toEqual([
{ id: contact.id, name: 'My Contact', address: null },
{ id: employee.id, contact: contact.id, name: 'My Employee' },
]);
await orm.em.flush();

expect(employee).toBeInstanceOf(Employee);
Expand Down

0 comments on commit 2b0f208

Please sign in to comment.