fix: better mergeDeep implementation and tests by ferk6a · Pull Request #5097 · typeorm/typeorm

ferk6a · 2019-11-17T19:52:36Z

The current mergeDeep implementation tries to recursively copy complex objects like class instances. So, a new implementation of mergeDeep and isPlainObject (previously isObject) is given. This fixes #5096, which demonstrates the problem.

As a bonus, this creates tests for these methods which weren't tested before.

ferk6a · 2019-11-17T22:03:59Z

I guess the tests pass, but I'm a bit worried about edge cases, maybe something like passing a class instance to an insert, or some kind of recursive scenario?

pleerock · 2019-11-22T01:48:45Z

Tests can easily pass because we don't have a large test coverage for monogdb driver.

I think your changes won't work properly because in the cases when embedded objects (nested objects) are used, we actually need class instances, we can't skip them.

Solution to this issue is to rewrite current way of "copying object" - traverse over defined columns in the class and all its embeddeds and copy only object properties that are defined in these columns. Columns definition tells us what schema should be used for saving and this schema can't be recursive.

ferk6a · 2019-11-23T14:03:34Z

@pleerock Ok, thanks! I will work some more on this then. I will add more tests regarding having nested embedded objects and objects with more information than the schema allows (should we throw an error or something if we detect that, or just ignore?).

I will work on making a new function to transform nested entities into plain objects using schema (probably using getMetadata and .prototype, I still don't know exactly how and how well it will scale), and then apply that instead of { ... }, and I think then we will be ready!

ferk6a · 2019-11-25T12:26:51Z

Tests can easily pass because we don't have a large test coverage for monogdb driver.

I think your changes won't work properly because in the cases when embedded objects (nested objects) are used, we actually need class instances, we can't skip them.

@pleerock I was looking into this again, and it looks like there is a test for this, and it passed:

typeorm/test/functional/mongodb/basic/embedded-columns/mongodb-embedded-columns.ts

Lines 134 to 158 in 4307a59

    
           it("should transform entity with nested embedded columns correctly", () => Promise.all(connections.map(async connection => { 
        
               const postRepository = connection.getMongoRepository(Post); 
        
               // save few posts 
        
               const post = new Post(); 
        
               post.title = "Post #1"; 
        
               post.text = "Everything about post"; 
        
               post.counters = new Counters(); 
        
               post.counters.likes = 5; 
        
               post.counters.comments = 0; 
        
               post.counters.favorites = 1; 
        
               post.counters.information = new Information(); 
        
               post.counters.information.description = "Hello post"; 
        
               post.counters.extraInformation = new ExtraInformation(); 
        
               post.counters.extraInformation.lastEdit = new EditHistory(); 
        
               post.counters.extraInformation.lastEdit.title = "Old Post Title"; 
        
               post.counters.extraInformation.lastEdit.text = "Not everything about post"; 
        
               await postRepository.save(post); 
        
               const [loadedPost] = await postRepository.find(); 
        
               loadedPost.counters.comments.should.be.equal(0); 
        
               loadedPost.counters.favorites.should.be.equal(1); 
        
               loadedPost.counters.extraInformation.lastEdit.title.should.be.eql("Old Post Title"); 
        
               loadedPost.counters.extraInformation.lastEdit.text.should.be.eql("Not everything about post");

So I don't know exactly how I could break this, since I saw that it passed that, it looks to me to be pretty solid (even though it really doesn't look like it).

papaiatis · 2020-07-20T07:34:48Z

@pleerock is this ever going to get merged any time soon?

imnotjames

Can you rebase & fix the conflict?

imnotjames · 2020-10-17T20:49:09Z

src/persistence/SubjectExecutor.ts

            // for mongodb we have a bit different updation logic
            if (this.queryRunner instanceof MongoQueryRunner) {
-                const partialEntity = OrmUtils.mergeDeep({}, subject.entity!);
+                const partialEntity = OrmUtils.mergeDeep({}, { ...subject.entity! });


Why shallow cloning the object here? Doesn't this code that already?

imnotjames · 2020-10-17T20:50:15Z

src/util/OrmUtils.ts

-    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()
+    static isPlainObject(item: any) {


If you're dropping isObject can you make this private? To reduce the public API footprint.

imnotjames · 2020-10-17T20:51:44Z

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

+    @Column({ type: String, transformer: new ComplexTransformer(), nullable: true })
+    complex: Complex | null;


Is it possible to add this to a different entity in a different entity file?

That way we're not muddying up the original tests

imnotjames · 2020-10-17T20:53:28Z

test/functional/util/OrmUtils.ts

+describe("OrmUtils.isPlainObject", () => {
+    const isPlainObject = OrmUtils.isPlainObject.bind(OrmUtils);
+
+    it("should return `true` if the Object is made by Object.create(null), {} or new Object().", () => {
+        class Foo {}
+
+        expect(isPlainObject(new Object())).to.be.true;
+        expect(isPlainObject(Object.create(null))).to.be.true;
+        expect(isPlainObject(Object.create(Object.prototype))).to.be.true;
+        expect(isPlainObject({ foo: "bar" })).to.be.true;
+        expect(isPlainObject({ constructor: "foo" })).to.be.true;
+        expect(isPlainObject({ constructor: Foo })).to.be.true;
+        expect(isPlainObject({}));
+    });
+
+    it("should return `false` if the Object is not made by Object.create(null), {} or new Object().", () => {
+        class Foo {}
+
+        expect(isPlainObject(Object.create({}))).to.be.false;
+        expect(isPlainObject(Object.create(Object))).to.be.false;
+        expect(isPlainObject(/foo/)).to.be.false;
+        expect(isPlainObject(function() {})).to.be.false;
+        expect(isPlainObject(() => {})).to.be.false;
+        expect(isPlainObject(["foo", "bar"])).to.be.false;
+        expect(isPlainObject([])).to.be.false;
+        expect(isPlainObject(new Foo())).to.be.false;
+        expect(isPlainObject(null)).to.be.false;
+        expect(isPlainObject(Math)).to.be.false;
+        expect(isPlainObject(Error)).to.be.false;
+        expect(isPlainObject(0)).to.be.false;
+        expect(isPlainObject(false)).to.be.false;
+        expect(isPlainObject(NaN)).to.be.false;
+    });
+ });


since we're doing all the tests below - do we need to test isPlainObject if it's just an internal helper? Do the below tests not go through all the possible permutations?

fix: better mergeDeep implementation and tests

a0b454c

ferk6a closed this Nov 17, 2019

ferk6a reopened this Nov 17, 2019

fix: shallow copy Mongo entities for merging

35ea9d3

test: add a complex type value-transformer test

11f0fc2

test: add recursive test to mongodb repo merge

00680af

imnotjames requested changes Oct 17, 2020

View reviewed changes

ferk6a closed this Dec 5, 2020

This was referenced Jun 17, 2021

fix: handle URL objects as column field values (redux) #7762

Closed

fix: update code to cater for custom deep merge #7756

Closed

refactor: only merge simple objects in ORMUtils.mergeDeep #7774

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: better mergeDeep implementation and tests#5097

fix: better mergeDeep implementation and tests#5097
ferk6a wants to merge 4 commits intotypeorm:masterfrom
ferk6a:merge-deep

ferk6a commented Nov 17, 2019

Uh oh!

ferk6a commented Nov 17, 2019

Uh oh!

pleerock commented Nov 22, 2019

Uh oh!

ferk6a commented Nov 23, 2019

Uh oh!

ferk6a commented Nov 25, 2019

Uh oh!

papaiatis commented Jul 20, 2020

Uh oh!

imnotjames left a comment

Uh oh!

imnotjames Oct 17, 2020

Uh oh!

imnotjames Oct 17, 2020 •

edited

Loading

Uh oh!

imnotjames Oct 17, 2020

Uh oh!

imnotjames Oct 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		@Column({ type: String, transformer: new ComplexTransformer(), nullable: true })
		complex: Complex \| null;

Uh oh!

Conversation

ferk6a commented Nov 17, 2019

Uh oh!

ferk6a commented Nov 17, 2019

Uh oh!

pleerock commented Nov 22, 2019

Uh oh!

ferk6a commented Nov 23, 2019

Uh oh!

ferk6a commented Nov 25, 2019

Uh oh!

papaiatis commented Jul 20, 2020

Uh oh!

imnotjames left a comment

Choose a reason for hiding this comment

Uh oh!

imnotjames Oct 17, 2020

Choose a reason for hiding this comment

Uh oh!

imnotjames Oct 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imnotjames Oct 17, 2020

Choose a reason for hiding this comment

Uh oh!

imnotjames Oct 17, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

imnotjames Oct 17, 2020 •

edited

Loading