fix: only operate on simple objects in OrmUtils.mergeDeep

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 createValueMap as well

src/metadata/EntityMetadata.ts

@@ -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);
     }
 

src/persistence/SubjectExecutor.ts

@@ -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];
                 }
@@ -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.
      */
@@ -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];
                 }
@@ -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];
                 }

src/util/OrmUtils.ts

@@ -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;
     }
 
     /**

test/functional/columns/value-transformer/entity/Post.ts

@@ -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 {
 
@@ -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;
 }

test/functional/columns/value-transformer/value-transformer.ts

@@ -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";
@@ -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);
+    })));
 });

test/functional/mongodb/basic/mongo-repository/mongo-repository.ts

@@ -87,5 +87,29 @@ describe("mongodb > MongoRepository", () => {
     })));
 
     // todo: cover other methods as well
+    it("should be able to save and update mongo entities", () => Promise.all(connections.map(async connection => {
+        const postRepository = connection.getMongoRepository(Post);
+
+        // save few posts
+        const firstPost = new Post();
+        firstPost.title = "Post #1";
+        firstPost.text = "Everything about post #1";
+        await postRepository.save(firstPost);
+
+        const secondPost = new Post();
+        secondPost.title = "Post #2";
+        secondPost.text = "Everything about post #2";
+        await postRepository.save(secondPost);
 
+        // save few posts
+        firstPost.text = "Everything and more about post #1";
+        await postRepository.save(firstPost);
+
+        const loadedPosts = await postRepository.find();
+
+        loadedPosts.length.should.be.equal(2);
+        loadedPosts[0].text.should.be.equal("Everything and more about post #1");
+        loadedPosts[1].text.should.be.equal("Everything about post #2");
+
+    })));
 });

test/functional/mongodb/basic/repository-actions/entity/Counters.ts

@@ -0,0 +1,19 @@
+import {Column} from "../../../../../../src/decorator/columns/Column";
+import {Entity} from "../../../../../../src/decorator/entity/Entity";
+import {User} from "./User";
+
+@Entity()
+export class Counters {
+
+    @Column({ name: "_likes" })
+    likes: number;
+
+    @Column({ name: "_comments" })
+    comments: number;
+
+    @Column({ name: "_favorites" })
+    favorites: number;
+
+    @Column(() => User)
+    viewer: User;
+}

test/functional/mongodb/basic/repository-actions/entity/Post.ts

@@ -1,4 +1,5 @@
 import {Entity} from "../../../../../../src/decorator/entity/Entity";
+import {Counters} from "./Counters";
 import {Column} from "../../../../../../src/decorator/columns/Column";
 import {ObjectIdColumn} from "../../../../../../src/decorator/columns/ObjectIdColumn";
 import {ObjectID} from "../../../../../../src/driver/mongodb/typings";
@@ -18,7 +19,6 @@ export class Post {
     @Column()
     index: number;
 
-    // @Column(() => Counters)
-    // counters: Counters;
-
+    @Column(() => Counters)
+    counters: Counters;
 }

test/functional/mongodb/basic/repository-actions/entity/User.ts

@@ -0,0 +1,9 @@
+import {Column} from "../../../../../../src/decorator/columns/Column";
+import {Entity} from "../../../../../../src/decorator/entity/Entity";
+
+@Entity()
+export class User {
+
+    @Column()
+    name: string;
+}