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.

Sign up for GitHub

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

Inverse relation unlink bug #383

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aaf9f2c
belongsTo errors without inverse relation
pik Aug 2, 2016
50fc330
Fix belongsTo error without inverse relationship
pik Aug 2, 2016
7d05e20
Move safeSetProp, safeSetLink to utils
pik Aug 2, 2016
cd3a0bb
Fix inverseRelationUnlink Bug (sort of)
pik Aug 2, 2016
673bf76
remove debug logging
pik Aug 3, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
remove debug logging
  • Loading branch information
@pik
pik committed Aug 3, 2016
commit 673bf769866c6081eb0daab3fad58ae8e71581a3
1 change: 0 additions & 1 deletion src/Collection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -689,7 +689,6 @@ export default Component.extend({
}
}
}
console.log(this.afterRemove)
return this.afterRemove(idOrRecord, opts, record) || record
},

Expand Down
3 changes: 0 additions & 3 deletions src/DataStore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1274,13 +1274,11 @@ const props = {
get: getter,
// e.g. user.profile = someProfile
set (record) {
console.log("IN SET HAS ONE\n", this, path, this._get('links'), localField, "\n")
const current = this._get(path)
if (record === current) {
return current
}
const inverseLocalField = def.getInverse(mapper).localField
console.log("SAFE SET LINK\n", current, inverseLocalField, record, "\n")
if (record) {
// Update (unset) inverse relation
if (current) {
Expand All @@ -1304,7 +1302,6 @@ const props = {
} else {
// Unset locals
if (current) {
console.log("SAFE SET FOREIGN KEY\n", current, foreignKey, "\n")
// Update (unset) inverse relation
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Formerly the above // Update (unset) inverse relation would only get called on update not on clear/delete. I added safeSetLink(current, inverseLocalField, undefined) which fixes the broken destroy test.

On the other-hand attempting to add safeSetProp(current, foreignKey, undefined) (commented out below) which should make sense (Why would we keep foreign keys after clearing the relation?) causes 4 other specs to break in the collection_methods suite.

/* Note that setting foreignKey to null / undefined breaks other specs
* Seems like there are other errors
Expand Down
1 change: 0 additions & 1 deletion src/Record.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,6 @@ export default Component.extend({
},

removeInverseRelation(currentParent, id, inverseDef, idAttribute) {
console.log('REMOVEINVERSERELATION')
if (inverseDef.type === hasOneType) {
safeSetLink(currentParent, inverseDef.localField, undefined)
} else if (inverseDef.type === hasManyType) {
Expand Down
1 change: 0 additions & 1 deletion src/Relation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,6 @@ utils.addHiddenPropsToTarget(Relation.prototype, {
// this.unlinkInverseRecords(relatedData, record)
Copy link
Contributor Author
@pik pik Aug 2, 2016 •
edited
Loading

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.


// descriptor.set will trigger removeInverseRelations
console.log("\nremoveLinkedRecords: RECORD - LOCALFIELD", record, localField, "\n")
utils.set(record, localField, undefined)
})
},
Expand Down
2 changes: 0 additions & 2 deletions test/unit/decorators/hasMany.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,9 +233,7 @@ describe('JSData.hasMany', function () {

store.add('B', [{id: 1, a_id: 1}, {id: 2, a_id: 1}]);
assert.equal(aRecord.b.length, 2, '2 items linked as expected');
console.log(aRecord.b)
store.remove('B', 2);
console.log(aRecord.b)
assert.equal(aRecord.b.length, 1, 'expected 1 item to still be linked but got empty array');
})
})
0