Skip to content

Commit

Permalink
fix(core): fix extra updates for composite FKs that share a column
Browse files Browse the repository at this point in the history
  • Loading branch information
B4nan committed Dec 15, 2023
1 parent 8ffdf08 commit 5897514
Show file tree
Hide file tree
Showing 6 changed files with 171 additions and 22 deletions.
37 changes: 37 additions & 0 deletions packages/core/src/EntityManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1422,6 +1422,38 @@ export class EntityManager<D extends IDatabaseDriver = IDatabaseDriver> {
* parameter matches a property name, its value will be extracted from `data`. If no matching property exists,
* the whole `data` parameter will be passed. This means we can also define `constructor(data: Partial<T>)` and
* `em.create()` will pass the data into it (unless we have a property named `data` too).
*
* The parameters are strictly checked, you need to provide all required properties. You can use `OptionalProps`
* symbol to omit some properties from this check without making them optional. Alternatively, use `partial: true`
* in the options to disable the strict checks for required properties. This option has no effect on runtime.
*/
create<Entity extends object>(entityName: EntityName<Entity>, data: RequiredEntityData<Entity>, options?: CreateOptions): Entity;

/**
* Creates new instance of given entity and populates it with given data.
* The entity constructor will be used unless you provide `{ managed: true }` in the options parameter.
* The constructor will be given parameters based on the defined constructor of the entity. If the constructor
* parameter matches a property name, its value will be extracted from `data`. If no matching property exists,
* the whole `data` parameter will be passed. This means we can also define `constructor(data: Partial<T>)` and
* `em.create()` will pass the data into it (unless we have a property named `data` too).
*
* The parameters are strictly checked, you need to provide all required properties. You can use `OptionalProps`
* symbol to omit some properties from this check without making them optional. Alternatively, use `partial: true`
* in the options to disable the strict checks for required properties. This option has no effect on runtime.
*/
create<Entity extends object>(entityName: EntityName<Entity>, data: EntityData<Entity>, options: CreateOptions & { partial: true }): Entity;

/**
* Creates new instance of given entity and populates it with given data.
* The entity constructor will be used unless you provide `{ managed: true }` in the options parameter.
* The constructor will be given parameters based on the defined constructor of the entity. If the constructor
* parameter matches a property name, its value will be extracted from `data`. If no matching property exists,
* the whole `data` parameter will be passed. This means we can also define `constructor(data: Partial<T>)` and
* `em.create()` will pass the data into it (unless we have a property named `data` too).
*
The parameters are strictly checked, you need to provide all required properties. You can use `OptionalProps`
symbol to omit some properties from this check without making them optional. Alternatively, use `partial: true`
in the options to disable the strict checks for required properties. This option has no effect on runtime.
*/
create<Entity extends object>(entityName: EntityName<Entity>, data: RequiredEntityData<Entity>, options: CreateOptions = {}): Entity {
const em = this.getContext();
Expand Down Expand Up @@ -2105,9 +2137,14 @@ export class EntityManager<D extends IDatabaseDriver = IDatabaseDriver> {
}

export interface CreateOptions {
/** creates a managed entity instance instead, bypassing the constructor call */
managed?: boolean;
/** create entity in a specific schema - alternatively, use `wrap(entity).setSchema()` */
schema?: string;
/** persist the entity automatically - this is the default behavior and is also configurable globally via `persistOnCreate` option */
persist?: boolean;
/** this option disables the strict typing which requires all mandatory properties to have value, it has no effect on runtime */
partial?: boolean;
}

export interface MergeOptions {
Expand Down
2 changes: 2 additions & 0 deletions packages/core/src/entity/EntityAssigner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { Reference } from './Reference';
import { ReferenceKind, SCALAR_TYPES } from '../enums';
import { EntityValidator } from './EntityValidator';
import { helper, wrap } from './wrap';
import { EntityHelper } from './EntityHelper';

const validator = new EntityValidator(false);

Expand All @@ -37,6 +38,7 @@ export class EntityAssigner {
return entity as any;
}

EntityHelper.ensurePropagation(entity);
opts.visited ??= new Set();
opts.visited.add(entity);
const wrapped = helper(entity);
Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/entity/EntityHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,13 +126,13 @@ export class EntityHelper {
},
set(val) {
this.__helper.__data[prop.name] = val;
this.__helper.__touched = true;
this.__helper.__touched = !this.__helper.hydrator.isRunning();
},
enumerable: true,
configurable: true,
});
this.__helper.__data[prop.name] = val;
this.__helper.__touched = true;
this.__helper.__touched = !this.__helper.hydrator.isRunning();
},
configurable: true,
});
Expand Down Expand Up @@ -185,7 +185,7 @@ export class EntityHelper {
if (val && hydrator.isRunning() && wrapped.__originalEntityData && prop.owner) {
wrapped.__originalEntityData[prop.name] = helper(wrapped.__data[prop.name]).getPrimaryKey(true);
} else {
wrapped.__touched = true;
wrapped.__touched = !hydrator.isRunning();
}

EntityHelper.propagate(meta, entity, this, prop, Reference.unwrapReference(val), old);
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/typings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -548,7 +548,7 @@ export class EntityMetadata<T = any> {
if (val && hydrator.isRunning() && wrapped.__originalEntityData && prop.owner) {
wrapped.__originalEntityData[prop.name as string] = val.__helper.getPrimaryKey(true);
} else {
wrapped.__touched = true;
wrapped.__touched = !hydrator.isRunning();
}

EntityHelper.propagate(meta, entity, this, prop, Reference.unwrapReference(val), old);
Expand All @@ -572,7 +572,7 @@ export class EntityMetadata<T = any> {
}

this.__helper.__data[prop.name] = val;
this.__helper.__touched = true;
this.__helper.__touched = !this.__helper.hydrator.isRunning();
},
enumerable: true,
configurable: true,
Expand Down
37 changes: 20 additions & 17 deletions packages/knex/src/AbstractSqlDriver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -349,23 +349,26 @@ export abstract class AbstractSqlDriver<Connection extends AbstractSqlConnection
}
});

meta2.props
.filter(prop => this.platform.shouldHaveColumn(prop, hint.children as any || []))
.forEach(prop => {
if (prop.fieldNames.length > 1) { // composite keys
const fk = prop.fieldNames.map(name => root![`${relationAlias}__${name}` as EntityKey<T>]) as Primary<T>[];
prop.fieldNames.map(name => delete root![`${relationAlias}__${name}` as EntityKey<T>]);
relationPojo[prop.name] = Utils.mapFlatCompositePrimaryKey(fk, prop) as EntityValue<T>;
} else if (prop.runtimeType === 'Date') {
const alias = `${relationAlias}__${prop.fieldNames[0]}` as EntityKey<T>;
relationPojo[prop.name] = (typeof root![alias] === 'string' ? new Date(root![alias] as string) : root![alias]) as EntityValue<T>;
delete root![alias];
} else {
const alias = `${relationAlias}__${prop.fieldNames[0]}` as EntityKey<T>;
relationPojo[prop.name] = root![alias];
delete root![alias];
}
});
const props = meta2.props.filter(prop => this.platform.shouldHaveColumn(prop, hint.children as any || []));

for (const prop1 of props) {
if (prop1.fieldNames.length > 1) { // composite keys
const fk = prop1.fieldNames.map(name => root![`${relationAlias}__${name}` as EntityKey<T>]) as Primary<T>[];
relationPojo[prop1.name] = Utils.mapFlatCompositePrimaryKey(fk, prop1) as EntityValue<T>;
} else if (prop1.runtimeType === 'Date') {
const alias = `${relationAlias}__${prop1.fieldNames[0]}` as EntityKey<T>;
relationPojo[prop1.name] = (typeof root![alias] === 'string' ? new Date(root![alias] as string) : root![alias]) as EntityValue<T>;
} else {
const alias = `${relationAlias}__${prop1.fieldNames[0]}` as EntityKey<T>;
relationPojo[prop1.name] = root![alias];
}
}

// properties can be mapped to multiple places, e.g. when sharing a column in multiple FKs,
// so we need to delete them after everything is mapped from given level
for (const prop1 of props) {
prop1.fieldNames.map(name => delete root![`${relationAlias}__${name}` as EntityKey<T>]);
}

if ([ReferenceKind.MANY_TO_MANY, ReferenceKind.ONE_TO_MANY].includes(prop.kind)) {
result[prop.name] ??= [] as EntityValue<T>;
Expand Down
107 changes: 107 additions & 0 deletions tests/issues/GHx9.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
import { Entity, ManyToOne, OneToOne, Opt, PrimaryKey, Property, Ref, wrap } from '@mikro-orm/core';
import { MikroORM } from '@mikro-orm/sqlite';
import { v4 } from 'uuid';

@Entity()
class Organization {

@PrimaryKey({ columnType: 'uuid' })
id = v4();

}

@Entity()
class Project {

@PrimaryKey({ columnType: 'uuid' })
id = v4();

@ManyToOne({ entity: () => Organization, ref: true, primary: true })
organization!: Ref<Organization>;

@Property({ length: 255 })
name!: string;

@OneToOne({
entity: () => ProjectUpdate,
mappedBy: 'project',
ref: true,
})
projectUpdate!: Ref<ProjectUpdate> & Opt;

}

@Entity()
class ProjectUpdate {

@PrimaryKey({ columnType: 'uuid' })
id = v4();

@ManyToOne({ entity: () => Organization, ref: true, primary: true })
organization!: Ref<Organization>;

@OneToOne({
entity: () => Project,
ref: true,
joinColumns: ['project_id', 'organization_id'],
})
project!: Ref<Project>;

}

let orm: MikroORM;
let org: Organization;
let project: Project;

beforeAll(async () => {
orm = await MikroORM.init({
dbName: ':memory:',
entities: [Project, Organization, ProjectUpdate],
});

await orm.schema.refreshDatabase();

org = new Organization();
project = orm.em.create(Project, {
organization: org,
name: 'init',
});

orm.em.create(ProjectUpdate, {
organization: org.id,
project,
});

await orm.em.flush();
orm.em.clear();
});

afterAll(async () => {
await orm.close();
});

test('extra updates with 1:1 relations (joined)', async () => {
const result = await orm.em.findOneOrFail(Project, { id: project.id, organization: org.id }, {
populate: ['projectUpdate'],
strategy: 'joined',
});

expect(wrap(result.projectUpdate.$).isTouched()).toBe(false);
expect(wrap(result).isTouched()).toBe(false);

orm.em.getUnitOfWork().computeChangeSets();
expect(orm.em.getUnitOfWork().getChangeSets()).toHaveLength(0);
});

test('extra updates with 1:1 relations (select-in)', async () => {
const result = await orm.em.findOneOrFail(Project, { id: project.id, organization: org.id }, {
populate: ['projectUpdate'],
strategy: 'select-in',
});

expect(wrap(result.projectUpdate.$).isTouched()).toBe(false);
expect(wrap(result).isTouched()).toBe(false);

orm.em.getUnitOfWork().computeChangeSets();
expect(orm.em.getUnitOfWork().getChangeSets()).toHaveLength(0);
});

0 comments on commit 5897514

Please sign in to comment.