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/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.cloneSubjectEntity(subject);
                 if (subject.metadata.objectIdColumn && subject.metadata.objectIdColumn.propertyName) {
                     delete partialEntity[subject.metadata.objectIdColumn.propertyName];
                 }
@@ -540,6 +540,18 @@ export class SubjectExecutor {
         }
     }
 
+    private cloneSubjectEntity(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.cloneSubjectEntity(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.cloneSubjectEntity(subject);
                 if (subject.metadata.objectIdColumn && subject.metadata.objectIdColumn.propertyName) {
                     delete partialEntity[subject.metadata.objectIdColumn.propertyName];
                 }

src/util/OrmUtils.ts

@@ -58,41 +58,64 @@ 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 mergeKey(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;
+        }
+
+        let newValue = value;
+
+        if (this.isPlainObject(value)) {
+            const newValue = Object.create(Object.getPrototypeOf(value));
+
+            if (!target[key]) {
+                Object.assign(target, { [key]: newValue });
+            }
+
+            memo.set(value, newValue);
+            this.merge(target[key], value);
+            memo.delete(value);
+        }
+
+        Object.assign(target, { [key]: newValue });
+    }
+
+    private static merge(target: any, source: any, memo: Map<any, any> = new Map()): any {
+        let keys = [];
+
+        if (this.isPlainObject(target) && this.isPlainObject(source)) {
+            keys.push(...Object.keys(source));
+        }
+
+        for (const key of keys) {
+            this.mergeKey(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[0].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;
+}

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

@@ -3,6 +3,8 @@ import {expect} from "chai";
 import {Connection} from "../../../../../src/connection/Connection";
 import {closeTestingConnections, createTestingConnections, reloadTestingDatabases} from "../../../../utils/test-utils";
 import {Post} from "./entity/Post";
+import {Counters} from "./entity/Counters";
+import {User} from "./entity/User";
 
 describe("mongodb > basic repository actions", () => {
 
@@ -46,6 +48,32 @@ describe("mongodb > basic repository actions", () => {
         mergedPost.text.should.be.equal("And its text is updated as well");
     })));
 
+    it("merge should merge all given recursive partial objects into given source entity", () => Promise.all(connections.map(async connection => {
+        const postRepository = connection.getRepository(Post);
+        const counter1 = new Counters();
+        counter1.likes = 5;
+        const counter2 = new Counters();
+        counter2.likes = 2;
+        counter2.viewer = new User();
+        counter2.viewer.name = "Hello World";
+        const post = postRepository.create({
+            title: "This is created post",
+            text: "All about this post",
+            counters: counter1
+        });
+        const mergedPost = postRepository.merge(post,
+            { title: "This is updated post" },
+            { text: "And its text is updated as well" },
+            { counters: counter2 }
+        );
+        mergedPost.should.be.instanceOf(Post);
+        mergedPost.should.be.equal(post);
+        mergedPost.title.should.be.equal("This is updated post");
+        mergedPost.text.should.be.equal("And its text is updated as well");
+        mergedPost.counters.likes.should.be.equal(2);
+        mergedPost.counters.viewer.name.should.be.equal("Hello World");
+    })));
+
     it("target should be valid", () => Promise.all(connections.map(async connection => {
         const postRepository = connection.getRepository(Post);
         expect(postRepository.target).not.to.be.undefined;