8000 Inverse relation unlink bug by pik · Pull Request #383 · js-data/js-data · GitHub
[go: up one dir, main page]

Skip to content

Inverse relation unlink bug #383

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

Closed
wants to merge 5 commits into from
Closed

Conversation

pik
Copy link
Contributor
@pik pik commented Aug 2, 2016

ref: #376 Only the last commit is relevant (will rebase out after) but I built from my previous branch since I think the extraction helps readability. Basically JS-Data already goes down the removeInverseRelation pathway when a relation is set to null/undefined so unlinkInverseRecords is strictly unnecessary.

Further the code in unlinkInverseRecords is flawed (or it would only work correctly for hasOne:

utils.set(record, this.getInverse(this.mapper).localField, undefined)

Compare to (removeInverseRelation):

else if (inverseDef.type === hasManyType) {
  ...
 utils.remove(children, (child) => child === this || id === utils.get(child, idAttribute))

Unfortunately simply removing removeInverseRelation broke a test in the destroy suite. This is because the hasOne setter() does not clear correctly, so this was updated as well (see below).

@@ -156,18 +156,15 @@ utils.addHiddenPropsToTarget(Relation.prototype, {
const localField = this.localField
records.forEach((record) => {
const relatedData = utils.get(record, localField)
this.unlinkInverseRecords(relatedData)
/* CAN DEPRECATE? */
// this.unlinkInverseRecords(relatedData, record)
Copy link
Contributor Author
@pik pik Aug 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is code-reduplicative and incorrect for many cases (also only used in 1 place), optimally there should only be 1 standard correct method called for all pathways which need to unlink.

@pik
Copy link
Contributor Author
pik commented Aug 2, 2016

@jmdobry As you know I'm new to JS-Data but the more favorable solution in my opinion (rather than piece-meal patches) would be to compile a whole set of failing tests against the current relations implementation (I somehow suspect it's more than 1 or 2) and then to do a thorough re-org of the different relation setter() code to increase code reuse/readability, as well as to add some more granular unit-testing. There would be some performance losses from not inlining, but those should only apply to set on relationalFields so I don't think that would be significant.

@pik pik force-pushed the inverse-relation-unlink branch from 13fdb72 to 673bf76 Compare August 3, 2016 13:40
@jmdobry
Copy link
Member
jmdobry commented Aug 3, 2016

@pik I believe I fixed the unlinking issue in 7259f1d (also added a test case).

I like the code re-org you did in this PR, can you rebase against master and keep the code re-org stuff?

@jmdobry
Copy link
Member
jmdobry commented Aug 3, 2016

Oh wait, I guess your other PR (#382) has the code re-org stuff already, I can just merge that.

@pik
Copy link
Contributor Author
pik commented Aug 3, 2016

Actually that might have been naive (on first try), since plunking
relational methods on record may lead to a very fat record.js - let me see
if I can improve that.

On Aug 3, 2016 1:50 PM, "Jason Dobry" notifications@github.com wrote:

Oh wait, I guess your other PR (#382
#382) has the code re-org stuff
already, I can just merge that.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#383 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AJwz-B9Qd09VcBDoVZCv7wZA3qoibEWQks5qcMPqgaJpZM4JbCzO
.

@jmdobry
Copy link
Member
jmdobry commented Aug 3, 2016

Yeah, can you rebase master and refactor the two methods from Record.js to Relation.js?

@pik
Copy link
Contributor Author
pik commented Aug 4, 2016

Will do. Closing this.

@pik pik closed this Aug 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0