-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: OneToManySubjectBuilder generates incorrect subjects #8221
Fix: OneToManySubjectBuilder generates incorrect subjects #8221
Conversation
… failed matching of relation IDs.
…lting to empty array on falsy return value.
Lots of test failures. Can you look into those? |
@imnotjames yes will do, was on holidays last week so I had no time to fix by now. :) |
@imnotjames ok fixed it, tests are passing! |
@imnotjames would be fantastic if you find a moment to review/accept this! |
@pleerock The tests only check for the resulting database changes. Before fix, builder generated DeleteSubjects + InsertSubjects for existing items, because it could not match existing items. I don't see how we could test against which subjects have been generated? |
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 |
Its not possible for me to understand the problem just based on your description - I need to see it. For example in
with and without your changes. Can you create a test case which shows the problem you are describing in your issue? |
@pleerock ok, will try to create a better test. |
@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 |
…test suite @ functional tests
…ent drivers would mess up the counter within the SettingSubscriber!
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. |
@pleerock sorry, forgot to submit the comment it seems. you were 100% right when asking me to dig deeper.
The issue does only appear in certain cases, when using a afterLoad subscriber to set some entity data. 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.
L62: expect(subscriber.counter.deletes).to.equal(0); |
Yes, they are failing now. I guess now you have introduced another bug because your test is invalid and actually ORM must generate & execute 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"},
]
}]); |
Okay after hours of debugging I can say your solution looks correct. I found magic code that automatically finds first primary key ( |
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 |
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. |
…8221) * Bugfix: OneToManySubjectBuilder generated invalid subjects because of failed matching of relation IDs. * relation.getEntityValue does not always return an array. Fix by defaulting to empty array on falsy return value. * Add tests * test fixes * Refactor tests ensuring composite keys on child side into a separate test suite @ functional tests * Rewrite tests and notes to correctly document+show what's the actual issue * Fix: test must not use Promise.all, parallel execution against different drivers would mess up the counter within the SettingSubscriber! * code updates * okay now I know we need this check Co-authored-by: Jannik <jannik@jannikmewes.de> Co-authored-by: jannik.wjm@gmail.com <> Co-authored-by: Umed Khudoiberdiev <pleerock.me@gmail.com>
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.
master
branchnpm run lint
passes with this change -- Passed on CircleCInpm run test
passes with this change -- Passed on CircleCIFixes #0000