-
Notifications
You must be signed in to change notification settings - Fork 138
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
Conversation
@@ -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) |
There was a problem hiding this comment.
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.
@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 |
13fdb72
to
673bf76
Compare
Oh wait, I guess your other PR (#382) has the code re-org stuff already, I can just merge that. |
Actually that might have been naive (on first try), since plunking On Aug 3, 2016 1:50 PM, "Jason Dobry" notifications@github.com wrote:
|
Yeah, can you rebase master and refactor the two methods from Record.js to Relation.js? |
Will do. Closing this. |
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 tonull/undefined
sounlinkInverseRecords
is strictly unnecessary.Further the code in
unlinkInverseRecords
is flawed (or it would only work correctly forhasOne
:Compare to (
removeInverseRelation
):Unfortunately simply removing
removeInverseRelation
broke a test in the destroy suite. This is because thehasOne
setter()
does not clear correctly, so this was updated as well (see below).