From 1d79b15007f28b23ca3164c77a2faccde7ed53a9 Mon Sep 17 00:00:00 2001 From: James Ward Date: Tue, 29 Jun 2021 18:53:34 -0400 Subject: [PATCH] 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 | 18 +++- src/util/OrmUtils.ts | 77 +++++++++++------ .../columns/value-transformer/entity/Post.ts | 34 ++++++++ .../value-transformer/value-transformer.ts | 58 ++++++++++++- .../mongo-repository/mongo-repository.ts | 24 ++++++ .../repository-actions/entity/Counters.ts | 19 +++++ .../basic/repository-actions/entity/Post.ts | 6 +- .../basic/repository-actions/entity/User.ts | 9 ++ .../mongodb-repository-actions.ts | 28 +++++++ test/functional/util/OrmUtils.ts | 82 +++++++++++++++++++ test/github-issues/5762/entity/User.ts | 24 ++++++ test/github-issues/5762/issue-5762.ts | 39 +++++++++ 12 files changed, 384 insertions(+), 34 deletions(-) create mode 100644 test/functional/mongodb/basic/repository-actions/entity/Counters.ts create mode 100644 test/functional/mongodb/basic/repository-actions/entity/User.ts create mode 100644 test/functional/util/OrmUtils.ts create mode 100644 test/github-issues/5762/entity/User.ts create mode 100644 test/github-issues/5762/issue-5762.ts diff --git a/src/persistence/SubjectExecutor.ts b/src/persistence/SubjectExecutor.ts index 2345b7bfdef..c2c6db2f67b 100644 --- a/src/persistence/SubjectExecutor.ts +++ b/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]; } diff --git a/src/util/OrmUtils.ts b/src/util/OrmUtils.ts index 4687b2e963f..32cd470f626 100644 --- a/src/util/OrmUtils.ts +++ b/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) { + // 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 = 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; } /** diff --git a/test/functional/columns/value-transformer/entity/Post.ts b/test/functional/columns/value-transformer/entity/Post.ts index 5077452d66f..3daca45fca4 100644 --- a/test/functional/columns/value-transformer/entity/Post.ts +++ b/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; } diff --git a/test/functional/columns/value-transformer/value-transformer.ts b/test/functional/columns/value-transformer/value-transformer.ts index 02f5d49bc90..9e49b166aab 100644 --- a/test/functional/columns/value-transformer/value-transformer.ts +++ b/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); + }))); }); diff --git a/test/functional/mongodb/basic/mongo-repository/mongo-repository.ts b/test/functional/mongodb/basic/mongo-repository/mongo-repository.ts index 3d229cd2368..e030381147f 100644 --- a/test/functional/mongodb/basic/mongo-repository/mongo-repository.ts +++ b/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"); + + }))); }); diff --git a/test/functional/mongodb/basic/repository-actions/entity/Counters.ts b/test/functional/mongodb/basic/repository-actions/entity/Counters.ts new file mode 100644 index 00000000000..90572b94871 --- /dev/null +++ b/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; +} diff --git a/test/functional/mongodb/basic/repository-actions/entity/Post.ts b/test/functional/mongodb/basic/repository-actions/entity/Post.ts index 281d15c25fe..5475cfa5d38 100644 --- a/test/functional/mongodb/basic/repository-actions/entity/Post.ts +++ b/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; } diff --git a/test/functional/mongodb/basic/repository-actions/entity/User.ts b/test/functional/mongodb/basic/repository-actions/entity/User.ts new file mode 100644 index 00000000000..79290ca9775 --- /dev/null +++ b/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; +} diff --git a/test/functional/mongodb/basic/repository-actions/mongodb-repository-actions.ts b/test/functional/mongodb/basic/repository-actions/mongodb-repository-actions.ts index 7385ceeaa9a..7d42fa036f5 100644 --- a/test/functional/mongodb/basic/repository-actions/mongodb-repository-actions.ts +++ b/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; diff --git a/test/functional/util/OrmUtils.ts b/test/functional/util/OrmUtils.ts new file mode 100644 index 00000000000..9c1ade28636 --- /dev/null +++ b/test/functional/util/OrmUtils.ts @@ -0,0 +1,82 @@ +import { expect } from "chai"; +import { OrmUtils } from "../../../src/util/OrmUtils"; + +describe("OrmUtils.mergeDeep", () => { + const mergeDeep = OrmUtils.mergeDeep.bind(OrmUtils); + + it("should handle simple values.", () => { + expect(mergeDeep(1, 2)).to.equal(1); + expect(mergeDeep(2, 1)).to.equal(2); + expect(mergeDeep(2, 1, 1)).to.equal(2); + expect(mergeDeep(1, 2, 1)).to.equal(1); + expect(mergeDeep(1, 1, 2)).to.equal(1); + expect(mergeDeep(2, 1, 2)).to.equal(2); + }); + + it("should handle ordering and indempotence.", () => { + const a = { a: 1 }; + const b = { a: 2 }; + expect(mergeDeep(a, b)).to.deep.equal(b); + expect(mergeDeep(b, a)).to.deep.equal(a); + expect(mergeDeep(b, a, a)).to.deep.equal(a); + expect(mergeDeep(a, b, a)).to.deep.equal(a); + expect(mergeDeep(a, a, b)).to.deep.equal(b); + expect(mergeDeep(b, a, b)).to.deep.equal(b); + const c = { a: 3 }; + expect(mergeDeep(a, b, c)).to.deep.equal(c); + expect(mergeDeep(b, c, b)).to.deep.equal(b); + expect(mergeDeep(c, a, a)).to.deep.equal(a); + expect(mergeDeep(c, b, a)).to.deep.equal(a); + expect(mergeDeep(a, c, b)).to.deep.equal(b); + expect(mergeDeep(b, a, c)).to.deep.equal(c); + }); + + it("should skip nested promises in sources.", () => { + expect(mergeDeep({}, { p: Promise.resolve() })).to.deep.equal({}); + expect(mergeDeep({}, { p: { p: Promise.resolve() }})).to.deep.equal({ p: {} }); + const a = { p: Promise.resolve(0) }; + const b = { p: Promise.resolve(1) }; + expect(mergeDeep(a, {})).to.deep.equal(a); + expect(mergeDeep(a, b)).to.deep.equal(a); + expect(mergeDeep(b, a)).to.deep.equal(b); + expect(mergeDeep(b, {})).to.deep.equal(b); + }); + + it("should merge moderately deep objects correctly.", () => { + const a = { a: { b: { c: { d: { e: 123, h: { i: 23 } } } } }, g: 19 }; + const b = { a: { b: { c: { d: { f: 99 } }, f: 31 } } }; + const c = { a: { b: { c: { d: { e: 123, f: 99, h: { i: 23 } } }, f: 31 } }, g: 19 }; + expect(mergeDeep(a, b)).to.deep.equal(c); + expect(mergeDeep(b, a)).to.deep.equal(c); + expect(mergeDeep(b, a, a)).to.deep.equal(c); + expect(mergeDeep(a, b, a)).to.deep.equal(c); + expect(mergeDeep(a, a, b)).to.deep.equal(c); + expect(mergeDeep(b, a, b)).to.deep.equal(c); + }); + + it("should merge recursively deep objects correctly", () => { + let a: Record = {}; + let b: Record = {}; + + a['b'] = b; + a['a'] = a; + b['a'] = a; + + expect(mergeDeep({}, a)); + }); + + it("should reference copy complex instances of classes.", () => { + class Foo { + recursive: Foo; + + constructor() { + this.recursive = this; + } + } + + const foo = new Foo(); + const result = mergeDeep({}, { foo }); + expect(result).to.have.property("foo"); + expect(result.foo).to.equal(foo); + }); + }); diff --git a/test/github-issues/5762/entity/User.ts b/test/github-issues/5762/entity/User.ts new file mode 100644 index 00000000000..a4dc4b0bddf --- /dev/null +++ b/test/github-issues/5762/entity/User.ts @@ -0,0 +1,24 @@ +import {Entity} from "../../../../src/decorator/entity/Entity"; +import {PrimaryColumn, Column} from "../../../../src"; +import { URL } from "url"; + +@Entity() +export class User { + + @PrimaryColumn() + id: number; + + @Column("varchar", { + // marshall + transformer: { + from(value: string): URL { + return new URL(value); + }, + to(value: URL): string { + return value.toString(); + }, + }, + }) + url: URL; + +} diff --git a/test/github-issues/5762/issue-5762.ts b/test/github-issues/5762/issue-5762.ts new file mode 100644 index 00000000000..d3ca010f712 --- /dev/null +++ b/test/github-issues/5762/issue-5762.ts @@ -0,0 +1,39 @@ +import "reflect-metadata"; +import {expect} from "chai"; +import {Connection} from "../../../src"; +import {User} from "./entity/User"; +import {createTestingConnections, reloadTestingDatabases, closeTestingConnections} from "../../utils/test-utils"; +import { URL } from "url"; + +describe("github issues > #5762 `Using URL as a rich column type breaks", () => { + + let connections: Connection[]; + + before(async () => { + connections = await createTestingConnections({ + entities: [User], + schemaCreate: true, + dropSchema: true + }); + }); + beforeEach(() => reloadTestingDatabases(connections)); + after(() => closeTestingConnections(connections)); + + it("should allow assigning URL as a field value", () => + Promise.all(connections.map(async (connection) => { + const userRepository = connection.getRepository(User); + + const url = new URL("https://typeorm.io"); + + const user = new User(); + user.id = 1; + user.url = url; + + const promise = userRepository.save(user); + + return expect(promise).to.eventually.be.deep.equal(user) + .and.to.have.property("url").be.instanceOf(URL) + .and.to.have.nested.property("href").equal(url.href); + }))); + +});