Fix: OneToManySubjectBuilder generates incorrect subjects

[jbjhjm]

Fixes #8212
Fixes #8175

Edit: On initial PR, I was not 100% right about the source of the issue.
After further investigation, I am now updating the description to better describe what is happening.

In certain cases, OneToManySubjectBuilder will use wrong relation key data which stops it from correctly matching keys of incoming data against database keys.
This will lead to entities being deleted and (re-)inserted instead of being updated and/or staying untouched.

Reason for this is the use of relation.getEntityValue(subject.databaseEntity).
The code assumes that the returned value will only contain the related items key data.
However this can be incorrect. If one uses the afterLoad hook to generate entity properties, the returned object may include additional properties.

Description of change

The required change is simple:
Pass the data within entityDatabaseValues to getEntityIdMap to get a correct relationId map in all cases.

• Code is up-to-date with the master branch
• npm run lint passes with this change -- Passed on CircleCI
• npm run test passes with this change -- Passed on CircleCI
• This pull request links relevant issues as Fixes #0000
• There are new or updated unit tests validating the change -- see info at top
• Documentation has been updated to reflect this change -- not needed
• The new commits follow conventions explained in CONTRIBUTING.md

[imnotjames]

Lots of test failures. Can you look into those?

[jbjhjm]

@imnotjames yes will do, was on holidays last week so I had no time to fix by now. :)

[jbjhjm]

@imnotjames ok fixed it, tests are passing!

[jbjhjm]

@imnotjames would be fantastic if you find a moment to review/accept this!

[pleerock]

hmmmmm I just reverted all your changes locally and run your tests - everything passes. Can you please check it? Or I'm doing something wrong?

[jbjhjm]

@pleerock
hmm yes you are right. Tests are not as good as they should be.

The tests only check for the resulting database changes.
To prove that they actually fix something I'd need to test against things happening during data persisting.
That's because the fix will not change the results, but the process of how they are being persisted.
To see the actual difference, you'd have to use logging to see what kinds of Subjects are being generated.

Before fix, builder generated DeleteSubjects + InsertSubjects for existing items, because it could not match existing items.
After fix, builder generates UpdateSubjects.

I don't see how we could test against which subjects have been generated?
I could add a Subscriber checking if/if not beforeInsert / beforeUpdate is being called.
Any better ideas?

[pleerock]

okay so I had to checkout your branch again, and don't understand where the issue is. You have multiple test cases, which one suppose to show the problem? If only one, probably need to remove others to prevent confusion. As I understood from issue description test case called should correctly update relation items should reproduce your issue. If so, what is the difference in behaviour? If output is the same, then where is the problem?

[pleerock]

Its not possible for me to understand the problem just based on your description - I need to see it.

For example in should correctly update relation items case I don't see any redundant SQL queries:

UPDATE "setting" SET "value" = ? WHERE ((("assetId" = ? AND "name" = ?))) -- PARAMETERS: ["foobar",1,"A"]
UPDATE "setting" SET "value" = ? WHERE ((("assetId" = ? AND "name" = ?))) -- PARAMETERS: ["testvalue",1,"B"]

with and without your changes. Can you create a test case which shows the problem you are describing in your issue?

[jbjhjm]

@pleerock ok, will try to create a better test.
The current tests don't show the difference, they are there to ensure that the actions run with the expected (same) results.
They are also there to ensure relations are working with composed keys too, because I could not find a test case for that anywhere.

[pleerock]

@jbjhjm if its hard to reproduce what you said in the test - create a minimal reproduction github repo, which I can checkout, run and see the issue

[pleerock]

Have you finished with changes? I pulled again, removed changes and still don't see failing tests. Please let me know by pinging me once you finish.

[jbjhjm]

@pleerock sorry, forgot to submit the comment it seems.

you were 100% right when asking me to dig deeper.
I discovered that the fix works well, but I was not right about the source of the issue.
I added new commits to reflect new insights.

• refactored the tests for correctly working child composite keys into a new functional test suite, think it's good to have them to ensure it works fine in general
• written a new test that will fail without the fix being applied
• improved notes in source code

The issue does only appear in certain cases, when using a afterLoad subscriber to set some entity data.
This would add additional properties to the child key data item loaded when fetching the parent databaseEntity.
Until now, OneToManySubjectBuilder did not handle this situation correctly,
it tried to match incoming keys against the database keys possibly polluted with additional props.

In this situation, the key mismatch would lead to children being deleted from database and new ones being inserted, also beforeRemove / beforeInsert would be called instead of the expected beforeUpdate.

---

About your most recent comment, it's irritating to hear that.
I just checked if the test would fail if I roll back OneToManySubjectBuilder, and it does fail as expected:

 1) github issues > #8221
       afterLoad entity modifier must not make relation key matching fail:

      AssertionError: expected 2 to equal 0
      + expected - actual

      -2
      +0

      at J:\Projects\typeorm\test\github-issues\8221\issue-8221.ts:62:42

L62: expect(subscriber.counter.deletes).to.equal(0);
which happens because wrong subjects are being generated.

[pleerock]

Yes, they are failing now.

I guess now you have introduced another bug because your test is invalid and actually ORM must generate & execute DELETE queries, because these lines of code in the test:

await userRepo.save([{
    id:1,
    settings:[
	    {id:1,name:"A",value:"foobar"},
	    {id:1,name:"B",value:"testvalue"},
    ]
}]);

must be:

await userRepo.save([{
    id:1,
    settings:[
	    {assetId:1,name:"A",value:"foobar"},
	    {assetId:1,name:"B",value:"testvalue"},
    ]
}]);

[pleerock]

Okay after hours of debugging I can say your solution looks correct. I found magic code that automatically finds first primary key (assertId) just based on parent entity and second primary key (name). Damn cascades are super complex.

[jbjhjm]

Okay after hours of debugging I can say your solution looks correct. I found magic code that automatically finds first primary key (assertId) just based on parent entity and second primary key (name). Damn cascades are super complex.

Great! Can you give me a hint on where to find the mentioned magic code? I'd want to take a look and maybe add some notes to the WIP documentation update on cascades #8277

[pleerock]

It's subject.identifier gets filled automatically, actually not sure I can explain everything shortly, u need to spend few hours debugging to figure it all out 😆 Also I'm not sure I was completely right in all my assumptions, because some code goes too deeply. Hate cascades. It take too much time and efforts and sometimes I think better not to have this feature TBH.