Inverse relation unlink bug #383
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.
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
removeInverseRelationpathway when a relation is set tonull/undefinedsounlinkInverseRecordsis strictly unnecessary.Further the code in
unlinkInverseRecordsis flawed (or it would only work correctly forhasOne:Compare to (
removeInverseRelation):Unfortunately simply removing
removeInverseRelationbroke a test in the destroy suite. This is because thehasOnesetter()does not clear correctly, so this was updated as well (see below).