Skip to content
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 missing inverse #5268

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -132,9 +132,11 @@ export default class RelationshipPayloadsManager {
let modelClass = this._store._modelFor(modelName);
let relationshipsByName = get(modelClass, 'relationshipsByName');
relationshipsByName.forEach((_, relationshipName) => {
let relationship = relationshipsByName.get(relationshipName);
let modelThatOwnsRelationship = relationship.parentType;
Copy link
Contributor

Choose a reason for hiding this comment

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

never trust parentType it may not have been loaded via modelFor yet and thus missing modelName

let relationshipPayloads = this._getRelationshipPayloads(modelName, relationshipName, modelClass, relationshipsByName, false);
if (relationshipPayloads) {
relationshipPayloads.unload(modelName, id, relationshipName);
relationshipPayloads.unload(modelThatOwnsRelationship.modelName || modelName, id, relationshipName);
}
});
}
Expand All @@ -156,7 +158,7 @@ export default class RelationshipPayloadsManager {
relationshipPayloads.get('user', 'hobbies') === relationshipPayloads.get('hobby', 'user');

The signature has a somewhat large arity to avoid extra work, such as
a) string maipulation & allocation with `modelName` and
a) string manipulation & allocation with `modelName` and
`relationshipName`
b) repeatedly getting `relationshipsByName` via `Ember.get`

Expand All @@ -167,9 +169,10 @@ export default class RelationshipPayloadsManager {
_getRelationshipPayloads(modelName, relationshipName, modelClass, relationshipsByName, init) {
if (!relationshipsByName.has(relationshipName)) { return; }

let key = `${modelName}:${relationshipName}`;
let parentType = relationshipsByName.get(relationshipName).parentType;
let key = `${parentType.modelName}:${relationshipName}`;
if (!this._cache[key] && init) {
return this._initializeRelationshipPayloads(modelName, relationshipName, modelClass, relationshipsByName);
return this._initializeRelationshipPayloads(parentType.modelName || modelName, relationshipName, parentType, relationshipsByName);
}

return this._cache[key];
Expand Down
3 changes: 2 additions & 1 deletion addon/-private/system/relationships/state/create.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ export default class Relationships {

if (!rel) { return undefined; }

let relationshipPayload = internalModel.store._relationshipsPayloads.get(internalModel.modelName, internalModel.id, key);
let modelThatOwnsRelationship = rel.parentType;
let relationshipPayload = internalModel.store._relationshipsPayloads.get(modelThatOwnsRelationship.modelName || internalModel.modelName, internalModel.id, key);

relationship = relationships[key] = createRelationshipFor(internalModel, rel, internalModel.store);

Expand Down
133 changes: 130 additions & 3 deletions tests/integration/inverse-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ import { module, test } from 'qunit';

import DS from 'ember-data';

let env, store, User, Job, ReflexiveModel;
let env, store, User, Manager, Job, Company, ReflexiveModel;

const { attr, belongsTo } = DS;
const { attr, belongsTo, hasMany } = DS;

function stringify(string) {
return function() { return string; };
Expand All @@ -19,18 +19,27 @@ module('integration/inverse_test - inverseFor', {
User = DS.Model.extend({
name: attr('string'),
bestFriend: belongsTo('user', { async: true, inverse: null }),
job: belongsTo('job', { async: false })
job: belongsTo('job', { async: false }),
employer: belongsTo('company', {inverse:"employees"})
});

User.toString = stringify('user');

Manager = User.extend({});
Manager.toString = stringify('manager');

Job = DS.Model.extend({
isGood: attr(),
user: belongsTo('user', { async: false })
});

Job.toString = stringify('job');

Company = DS.Model.extend({
employees: hasMany('user', {inverse: "employer", polymorphic: true})
});
Company.toString = stringify('company');

ReflexiveModel = DS.Model.extend({
reflexiveProp: belongsTo('reflexive-model', { async: false })
});
Expand All @@ -39,7 +48,9 @@ module('integration/inverse_test - inverseFor', {

env = setupStore({
user: User,
manager: Manager,
job: Job,
company: Company,
reflexiveModel: ReflexiveModel
});

Expand Down Expand Up @@ -124,6 +135,122 @@ test("Caches findInverseFor return value", function(assert) {
assert.equal(inverseForUser, Job.inverseFor('user', store), 'Inverse cached succesfully');
});

test("polymorphic hasMany inverse is populated after push of base model", function(assert) {
const done = assert.async();

run(() => {
store.push({
data: {
id: 'c1',
type: 'company',
relationships: {
employees: {
data: [
{id: 'u1', type: 'user'}
]
}
}
},
included: [
{
id: 'u1',
type: 'user',
relationships: {}
}
]
});
});

const user = store.peekRecord('user', 'u1');

user.get('employer').then(employer => {
assert.ok(employer);
done();
});
});

test("polymorphic hasMany inverse is populated after push of descendant model", function(assert) {
const done = assert.async();

run(() => {
store.push({
data: {
id: 'c1',
type: 'company',
relationships: {
employees: {
data: [
{id: 'u1', type: 'manager'}
]
}
}
},
included: [
{
id: 'u1',
type: 'manager',
relationships: {}
}
]
});
});

const user = store.peekRecord('manager', 'u1');

user.get('employer').then(employer => {
assert.ok(employer);
done();
});
});

test("polymorphic hasMany contains both models after pushing two different types of model", function(assert) {
const done = assert.async();

run(() => {
store.push({
data: {
id: 'c1',
type: 'company',
relationships: {
employees: {
data: [
]
}
}
}
});
store.push({
data: {
id: 'u1',
type: 'user',
relationships: {
employer: {
data: { id: 'c1', type: 'company'}
}
}
}
});
store.push({
data: {
id: 'm1',
type: 'manager',
relationships: {
employer: {
data: { id: 'c1', type: 'company'}
}
}
}
});
});

const company = store.peekRecord('company', 'c1');

company.get('employees').then(employees => {
assert.equal(employees.length, 2);
done();
});
});

testInDebug("Errors out if you do not define an inverse for a reflexive relationship", function(assert) {

//Maybe store is evaluated lazily, so we need this :(
Expand Down