Skip to content

Commit

Permalink
refactor: only merge simple objects in ORMUtils.mergeDeep (#7774)
Browse files Browse the repository at this point in the history 
The mergeDeep functionality was causing issues where it would
try to merge objects that were not safe to merge.  to correct this
we now only merge plain objects - such as those returned
by createValueMap.

there were three cases where the mongo driver had a split off
code path where it passed in entities, so we've updated to instead
update that to getEntityValueMap as well

adds basic support of array embedded in ColumnMetadata so we can
use getEntityValueMap with mongo

fixes #5096
fixes #5762
fixes #6181
fixes #5487
fixes #5321
  • Loading branch information
@imnotjames
imnotjames committed Jul 5, 2021
1 parent e55cf05 commit 08f9a1c
Show file tree
Hide file tree
Showing 17 changed files with 490 additions and 49 deletions.
6 changes: 6 additions & 0 deletions src/decorator/options/ColumnEmbeddedOptions.ts
View file
Expand Up @@ -9,4 +9,10 @@ export interface ColumnEmbeddedOptions {
*/
prefix?: string | boolean;

/**
* Indicates if this embedded is in array mode.
*
* This option works only in mongodb.
*/
array?: boolean;
}
36 changes: 24 additions & 12 deletions src/metadata/ColumnMetadata.ts
View file
Expand Up @@ -526,31 +526,40 @@ export class ColumnMetadata {

// first step - we extract all parent properties of the entity relative to this column, e.g. [data, information, counters]
const propertyNames = [...this.embeddedMetadata.parentPropertyNames];
const isEmbeddedArray = this.embeddedMetadata.isArray;

// now need to access post[data][information][counters] to get column value from the counters
// and on each step we need to create complex literal object, e.g. first { data },
// then { data: { information } }, then { data: { information: { counters } } },
// then { data: { information: { counters: [this.propertyName]: entity[data][information][counters][this.propertyName] } } }
// this recursive function helps doing that
const extractEmbeddedColumnValue = (propertyNames: string[], value: ObjectLiteral, map: ObjectLiteral): any => {
const extractEmbeddedColumnValue = (propertyNames: string[], value: ObjectLiteral): ObjectLiteral => {
if (value === undefined) {
return {};
}

const propertyName = propertyNames.shift();
if (value === undefined)
return map;

if (propertyName) {
const submap: ObjectLiteral = {};
extractEmbeddedColumnValue(propertyNames, value[propertyName], submap);
const submap = extractEmbeddedColumnValue(propertyNames, value[propertyName]);
if (Object.keys(submap).length > 0) {
map[propertyName] = submap;
return { [propertyName]: submap };
}
return map;
return {};
}
if (value[this.propertyName] !== undefined && (returnNulls === false || value[this.propertyName] !== null))
map[this.propertyName] = value[this.propertyName];
return map;

if (isEmbeddedArray && Array.isArray(value)) {
return value.map(v => ({ [this.propertyName]: v[this.propertyName] }));
}

if (value[this.propertyName] !== undefined && (returnNulls === false || value[this.propertyName] !== null)) {
return { [this.propertyName]: value[this.propertyName] };
}

return {};
};
const map: ObjectLiteral = {};
extractEmbeddedColumnValue(propertyNames, entity, map);
const map = extractEmbeddedColumnValue(propertyNames, entity);

return Object.keys(map).length > 0 ? map : undefined;

} else { // no embeds - no problems. Simply return column property name and its value of the entity
Expand Down Expand Up @@ -589,6 +598,7 @@ export class ColumnMetadata {

// first step - we extract all parent properties of the entity relative to this column, e.g. [data, information, counters]
const propertyNames = [...this.embeddedMetadata.parentPropertyNames];
const isEmbeddedArray = this.embeddedMetadata.isArray;

// next we need to access post[data][information][counters][this.propertyName] to get column value from the counters
// this recursive function takes array of generated property names and gets the post[data][information][counters] embed
Expand Down Expand Up @@ -616,6 +626,8 @@ export class ColumnMetadata {
} else if (this.referencedColumn) {
value = this.referencedColumn.getEntityValue(embeddedObject[this.propertyName]);

} else if (isEmbeddedArray && Array.isArray(embeddedObject)) {
value = embeddedObject.map(o => o[this.propertyName]);
} else {
value = embeddedObject[this.propertyName];
}
Expand Down
2 changes: 1 addition & 1 deletion src/metadata/EntityMetadata.ts
View file
Expand Up @@ -798,7 +798,7 @@ export class EntityMetadata {
if (map === undefined || value === null || value === undefined)
return undefined;

return column.isObjectId ? Object.assign(map, value) : OrmUtils.mergeDeep(map, value);
return OrmUtils.mergeDeep(map, value);
}, {} as ObjectLiteral|undefined);
}

Expand Down
18 changes: 15 additions & 3 deletions src/persistence/SubjectExecutor.ts
View file
Expand Up @@ -387,7 +387,7 @@ export class SubjectExecutor {

// for mongodb we have a bit different updation logic
if (this.queryRunner instanceof MongoQueryRunner) {
const partialEntity = OrmUtils.mergeDeep({}, subject.entity!);
const partialEntity = this.cloneMongoSubjectEntity(subject);
if (subject.metadata.objectIdColumn && subject.metadata.objectIdColumn.propertyName) {
delete partialEntity[subject.metadata.objectIdColumn.propertyName];
}
Expand Down Expand Up @@ -540,6 +540,18 @@ export class SubjectExecutor {
}
}

private cloneMongoSubjectEntity(subject: Subject): ObjectLiteral {
const target: ObjectLiteral = {};

if (subject.entity) {
for (const column of subject.metadata.columns) {
OrmUtils.mergeDeep(target, column.getEntityValueMap(subject.entity));
}
}

return target;
}

/**
* Soft-removes all given subjects in the database.
*/
Expand All @@ -551,7 +563,7 @@ export class SubjectExecutor {

// for mongodb we have a bit different updation logic
if (this.queryRunner instanceof MongoQueryRunner) {
const partialEntity = OrmUtils.mergeDeep({}, subject.entity!);
const partialEntity = this.cloneMongoSubjectEntity(subject);
if (subject.metadata.objectIdColumn && subject.metadata.objectIdColumn.propertyName) {
delete partialEntity[subject.metadata.objectIdColumn.propertyName];
}
Expand Down Expand Up @@ -631,7 +643,7 @@ export class SubjectExecutor {

// for mongodb we have a bit different updation logic
if (this.queryRunner instanceof MongoQueryRunner) {
const partialEntity = OrmUtils.mergeDeep({}, subject.entity!);
const partialEntity = this.cloneMongoSubjectEntity(subject);
if (subject.metadata.objectIdColumn && subject.metadata.objectIdColumn.propertyName) {
delete partialEntity[subject.metadata.objectIdColumn.propertyName];
}
Expand Down
114 changes: 87 additions & 27 deletions src/util/OrmUtils.ts
View file
Expand Up @@ -58,41 +58,101 @@ export class OrmUtils {
}, [] as T[]);
}

static isObject(item: any) {
return (item && typeof item === "object" && !Array.isArray(item));
// Checks if it's an object made by Object.create(null), {} or new Object()
private static isPlainObject(item: any) {
if (item === null || item === undefined) {
return false;
}

return !item.constructor || item.constructor === Object;
}

private static mergeArrayKey(target: any, key: number, value: any, memo: Map<any, any>) {
// Have we seen this before? Prevent infinite recursion.
if (memo.has(value)) {
target[key] = memo.get(value);
return;
}

if (value instanceof Promise) {
// Skip promises entirely.
// This is a hold-over from the old code & is because we don't want to pull in
// the lazy fields. Ideally we'd remove these promises via another function first
// but for now we have to do it here.
return;
}


if (!this.isPlainObject(value) && !Array.isArray(value)) {
target[key] = value;
return;
}

if (!target[key]) {
target[key] = Array.isArray(value) ? [] : {};
}

memo.set(value, target[key]);
this.merge(target[key], value, memo);
memo.delete(value);
}

private static mergeObjectKey(target: any, key: string, value: any, memo: Map<any, any>) {
// Have we seen this before? Prevent infinite recursion.
if (memo.has(value)) {
Object.assign(target, { [key]: memo.get(value) });
return;
}

if (value instanceof Promise) {
// Skip promises entirely.
// This is a hold-over from the old code & is because we don't want to pull in
// the lazy fields. Ideally we'd remove these promises via another function first
// but for now we have to do it here.
return;
}

if (!this.isPlainObject(value) && !Array.isArray(value)) {
Object.assign(target, { [key]: value });
return;
}

if (!target[key]) {
Object.assign(target, { [key]: Array.isArray(value) ? [] : {} });
}

memo.set(value, target[key]);
this.merge(target[key], value, memo);
memo.delete(value);
}

private static merge(target: any, source: any, memo: Map<any, any> = new Map()): any {
if (this.isPlainObject(target) && this.isPlainObject(source)) {
for (const key of Object.keys(source)) {
this.mergeObjectKey(target, key, source[key], memo);
}
}

if (Array.isArray(target) && Array.isArray(source)) {
for (let key = 0; key < source.length; key++) {
this.mergeArrayKey(target, key, source[key], memo);
}
}
}

/**
* Deep Object.assign.
*
* @see http://stackoverflow.com/a/34749873
*/
static mergeDeep(target: any, ...sources: any[]): any {
if (!sources.length) return target;
const source = sources.shift();

if (this.isObject(target) && this.isObject(source)) {
for (const key in source) {
const value = source[key];
if (key === "__proto__" || value instanceof Promise)
continue;

if (this.isObject(value)
&& !(value instanceof Map)
&& !(value instanceof Set)
&& !(value instanceof Date)
&& !(value instanceof Buffer)
&& !(value instanceof RegExp)) {
if (!target[key])
Object.assign(target, { [key]: Object.create(Object.getPrototypeOf(value)) });
this.mergeDeep(target[key], value);
} else {
Object.assign(target, { [key]: value });
}
}
if (!sources.length) {
return target;
}

for (const source of sources) {
OrmUtils.merge(target, source);
}

return this.mergeDeep(target, ...sources);
return target;
}

/**
Expand Down
34 changes: 34 additions & 0 deletions test/functional/columns/value-transformer/entity/Post.ts
View file
Expand Up @@ -15,6 +15,38 @@ class TagTransformer implements ValueTransformer {

}

export class Complex {
x: number;
y: number;
circularReferenceToMySelf: {
complex: Complex
};

constructor(from: String) {
this.circularReferenceToMySelf = { complex: this };
const [x, y] = from.split(" ");
this.x = +x;
this.y = +y;
}

toString() {
return `${this.x} ${this.y}`;
}
}

class ComplexTransformer implements ValueTransformer {

to (value: Complex | null): string | null {
if (value == null) { return value; }
return value.toString();
}

from (value: string | null): Complex | null {
if (value == null) { return value; }
return new Complex(value);
}
}

@Entity()
export class Post {

Expand All @@ -27,4 +59,6 @@ export class Post {
@Column({ type: String, transformer: new TagTransformer() })
tags: string[];

@Column({ type: String, transformer: new ComplexTransformer(), nullable: true })
complex: Complex | null;
}
58 changes: 57 additions & 1 deletion test/functional/columns/value-transformer/value-transformer.ts
View file
Expand Up @@ -4,7 +4,7 @@ import {closeTestingConnections, createTestingConnections, reloadTestingDatabase

import {Connection} from "../../../../src/connection/Connection";
import {PhoneBook} from "./entity/PhoneBook";
import {Post} from "./entity/Post";
import {Complex, Post} from "./entity/Post";
import {User} from "./entity/User";
import {Category} from "./entity/Category";
import {View} from "./entity/View";
Expand Down Expand Up @@ -95,4 +95,60 @@ describe("columns > value-transformer functionality", () => {
dbView && dbView.title.should.be.eql(title);

})));

it("should marshal data using a complex value-transformer", () => Promise.all(connections.map(async connection => {

const postRepository = connection.getRepository(Post);

// create and save a post first
const post = new Post();
post.title = "Complex transformers!";
post.tags = ["complex", "transformer"];
await postRepository.save(post);

let loadedPost = await postRepository.findOne(post.id);
expect(loadedPost!.complex).to.eq(null);

// then update all its properties and save again
post.title = "Complex transformers2!";
post.tags = ["very", "complex", "actually"];
post.complex = new Complex("3 2.5");
await postRepository.save(post);

// check if all columns are updated except for readonly columns
loadedPost = await postRepository.findOne(post.id);
expect(loadedPost!.title).to.be.equal("Complex transformers2!");
expect(loadedPost!.tags).to.deep.eq(["very", "complex", "actually"]);
expect(loadedPost!.complex!.x).to.eq(3);
expect(loadedPost!.complex!.y).to.eq(2.5);

// then update all its properties and save again
post.title = "Complex transformers3!";
post.tags = ["very", "lacking", "actually"];
post.complex = null;
await postRepository.save(post);

loadedPost = await postRepository.findOne(post.id);
expect(loadedPost!.complex).to.eq(null);

// then update all its properties and save again
post.title = "Complex transformers4!";
post.tags = ["very", "here", "again!"];
post.complex = new Complex("0.5 0.5");
await postRepository.save(post);

loadedPost = await postRepository.findOne(post.id);
expect(loadedPost!.complex!.x).to.eq(0.5);
expect(loadedPost!.complex!.y).to.eq(0.5);

// then update all its properties and save again
post.title = "Complex transformers5!";
post.tags = ["now", "really", "lacking!"];
post.complex = new Complex("1.05 2.3");
await postRepository.save(post);

loadedPost = await postRepository.findOne(post.id);
expect(loadedPost!.complex!.x).to.eq(1.05);
expect(loadedPost!.complex!.y).to.eq(2.3);
})));
});
4 changes: 2 additions & 2 deletions test/functional/mongodb/basic/array-columns/entity/Post.ts
View file
Expand Up @@ -25,10 +25,10 @@ export class Post {
@Column()
booleans: boolean[];

@Column(type => Counters)
@Column(type => Counters, { array: true })
other1: Counters[];

@Column(type => Counters)
@Column(type => Counters, { array: true })
other2: Counters[];

}

0 comments on commit 08f9a1c

Please sign in to comment.