Skip to content

Commit

Permalink
[BUGFIX relationships] fix infinite retry bug for failed relationship…
Browse files Browse the repository at this point in the history 
… fetches
  • Loading branch information
@runspired
runspired committed May 15, 2019
1 parent 58c3228 commit 0dd8ddb
Show file tree
Hide file tree
Showing 10 changed files with 315 additions and 212 deletions.
38 changes: 22 additions & 16 deletions packages/-ember-data/tests/acceptance/relationships/belongs-to-test.js
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { module, test, skip } from 'qunit';
import { module, test } from 'qunit';
import { setupRenderingTest } from 'ember-qunit';
import JSONAPIAdapter from '@ember-data/adapter/json-api';
import Model from '@ember-data/model';
Expand Down Expand Up @@ -374,8 +374,8 @@ module('async belongs-to rendering tests', function(hooks) {
assert.equal(this.element.textContent.trim(), 'Kevin has two children and one parent');
});

skip('Rendering an async belongs-to whose fetch fails does not trigger a new request', async function(assert) {
assert.expect(15);
test('Rendering an async belongs-to whose fetch fails does not trigger a new request', async function(assert) {
assert.expect(14);
let people = makePeopleWithRelationshipData();
let sedona = store.push({
data: people.dict['5:has-parent-no-children'],
Expand Down Expand Up @@ -412,10 +412,16 @@ module('async belongs-to rendering tests', function(hooks) {
assert.equal(this.element.textContent.trim(), '', 'we have no parent');

let relationshipState = sedona.belongsTo('parent').belongsToRelationship;
let RelationshipPromiseCache = sedona._internalModel._relationshipPromisesCache;
let RelationshipProxyCache = sedona._internalModel._relationshipProxyCache;

assert.equal(relationshipState.isAsync, true, 'The relationship is async');
assert.equal(relationshipState.relationshipIsEmpty, false, 'The relationship is not empty');
assert.equal(relationshipState.relationshipIsStale, true, 'The relationship is still stale');
assert.equal(
relationshipState.hasDematerializedInverse,
true,
'The relationship inverse is dematerialized'
);
assert.equal(
relationshipState.allInverseRecordsAreLoaded,
false,
Expand All @@ -427,9 +433,9 @@ module('async belongs-to rendering tests', function(hooks) {
'The relationship knows which record it needs'
);
assert.equal(
relationshipState.fetchPromise === null,
true,
'The relationship has no fetchPromise'
!!RelationshipPromiseCache['parent'],
false,
'The relationship has no fetch promise'
);
assert.equal(
relationshipState.hasFailedLoadAttempt === true,
Expand All @@ -442,18 +448,18 @@ module('async belongs-to rendering tests', function(hooks) {
'The relationship will not force a reload'
);
assert.equal(
relationshipState._promiseProxy !== null,
!!RelationshipProxyCache['parent'],
true,
'The relationship has a loadingPromise'
'The relationship has a promise proxy'
);
assert.equal(!!relationshipState.link, false, 'The relationship does not have a link');
assert.equal(
relationshipState.shouldMakeRequest(),
false,
'The relationship does not need to make a request'
);
let result = await sedona.get('parent');
assert.ok(result === null, 're-access is safe');

try {
let result = await sedona.get('parent');
assert.ok(result === null, 're-access is safe');
} catch (e) {
assert.ok(false, `Accessing resulted in rejected promise error: ${e.message}`);
}

Ember.onerror = originalOnError;
});
Expand Down
40 changes: 24 additions & 16 deletions packages/-ember-data/tests/acceptance/relationships/has-many-test.js
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { module, test, skip } from 'qunit';
import { module, test } from 'qunit';
import { setupRenderingTest } from 'ember-qunit';
import JSONAPIAdapter from '@ember-data/adapter/json-api';
import Model from '@ember-data/model';
Expand Down Expand Up @@ -287,7 +287,7 @@ module('async has-many rendering tests', function(hooks) {
);
});

skip('Rendering an async hasMany whose fetch fails does not trigger a new request', async function(assert) {
test('Rendering an async hasMany whose fetch fails does not trigger a new request', async function(assert) {
assert.expect(12);
let people = makePeopleWithRelationshipData();
let parent = store.push({
Expand Down Expand Up @@ -330,10 +330,16 @@ module('async has-many rendering tests', function(hooks) {
);

let relationshipState = parent.hasMany('children').hasManyRelationship;
let RelationshipPromiseCache = parent._internalModel._relationshipPromisesCache;
let RelationshipProxyCache = parent._internalModel._relationshipProxyCache;

assert.equal(relationshipState.isAsync, true, 'The relationship is async');
assert.equal(relationshipState.relationshipIsEmpty, false, 'The relationship is not empty');
assert.equal(relationshipState.relationshipIsStale, true, 'The relationship is still stale');
assert.equal(
relationshipState.hasDematerializedInverse,
true,
'The relationship has a dematerialized inverse'
);
assert.equal(
relationshipState.allInverseRecordsAreLoaded,
false,
Expand All @@ -345,19 +351,19 @@ module('async has-many rendering tests', function(hooks) {
'The relationship knows which record it needs'
);
assert.equal(
relationshipState.fetchPromise === null,
true,
'The relationship has no fetchPromise'
!!RelationshipPromiseCache['children'],
false,
'The relationship has no fetch promise'
);
assert.equal(
relationshipState.hasFailedLoadAttempt === true,
true,
'The relationship has attempted a load'
);
assert.equal(
relationshipState._promiseProxy !== null,
!!RelationshipProxyCache['children'],
true,
'The relationship has a loadingPromise'
'The relationship has a promise proxy'
);
assert.equal(!!relationshipState.link, false, 'The relationship does not have a link');

Expand Down Expand Up @@ -444,7 +450,7 @@ module('async has-many rendering tests', function(hooks) {
);
});

skip('Rendering an async hasMany with a link whose fetch fails does not trigger a new request', async function(assert) {
test('Rendering an async hasMany with a link whose fetch fails does not trigger a new request', async function(assert) {
assert.expect(12);
let people = makePeopleWithRelationshipLinks(true);
let parent = store.push({
Expand Down Expand Up @@ -489,6 +495,8 @@ module('async has-many rendering tests', function(hooks) {
assert.deepEqual(names, [], 'We rendered no names');

let relationshipState = parent.hasMany('children').hasManyRelationship;
let RelationshipPromiseCache = parent._internalModel._relationshipPromisesCache;
let RelationshipProxyCache = parent._internalModel._relationshipProxyCache;

assert.equal(relationshipState.isAsync, true, 'The relationship is async');
assert.equal(
Expand All @@ -508,19 +516,19 @@ module('async has-many rendering tests', function(hooks) {
'The relationship knows which record it needs'
);
assert.equal(
relationshipState.fetchPromise === null,
true,
'The relationship has no fetchPromise'
!!RelationshipPromiseCache['children'],
false,
'The relationship has no fetch promise'
);
assert.equal(
relationshipState.hasFailedLoadAttempt === true,
!!RelationshipProxyCache['children'],
true,
'The relationship has attempted a load'
'The relationship has a promise proxy'
);
assert.equal(
relationshipState._promiseProxy !== null,
relationshipState.hasFailedLoadAttempt === true,
true,
'The relationship has a loadingPromise'
'The relationship has attempted a load'
);
assert.equal(!!relationshipState.link, true, 'The relationship has a link');

Expand Down
68 changes: 39 additions & 29 deletions packages/-ember-data/tests/integration/relationships/has-many-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { get } from '@ember/object';
import { run } from '@ember/runloop';
import setupStore from 'dummy/tests/helpers/store';
import testInDebug from 'dummy/tests/helpers/test-in-debug';
import { module, test, skip } from 'qunit';
import { module, test } from 'qunit';
import { relationshipStateFor, relationshipsFor } from 'ember-data/-private';
import DS from 'ember-data';

Expand Down Expand Up @@ -955,16 +955,16 @@ module('integration/relationships/has_many - Has-Many Relationships', function(h
});
});

skip('A hasMany relationship can be reloaded even if it failed at the first time', async function(assert) {
assert.expect(6);
test('A hasMany relationship can be reloaded even if it failed at the first time', async function(assert) {
assert.expect(7);

const { store, adapter } = env;

Post.reopen({
comments: DS.hasMany('comment', { async: true }),
});

adapter.findRecord = function(store, type, id) {
adapter.findRecord = function() {
return resolve({
data: {
id: 1,
Expand All @@ -978,10 +978,10 @@ module('integration/relationships/has_many - Has-Many Relationships', function(h
});
};

let loadingCount = -1;
adapter.findHasMany = function(store, record, link, relationship) {
let loadingCount = 0;
adapter.findHasMany = function() {
loadingCount++;
if (loadingCount % 2 === 0) {
if (loadingCount % 2 === 1) {
return reject({ data: null });
} else {
return resolve({
Expand All @@ -995,16 +995,24 @@ module('integration/relationships/has_many - Has-Many Relationships', function(h

let post = await store.findRecord('post', 1);
let comments = post.get('comments');
let manyArray = await comments.catch(() => {
assert.ok(true, 'An error was thrown on the first reload of comments');
return comments.reload();
});
let manyArray;

try {
manyArray = await comments;
assert.ok(false, 'Expected exception to be raised');
} catch (e) {
assert.ok(true, `An error was thrown on the first reload of comments: ${e.message}`);
manyArray = await comments.reload();
}

assert.equal(manyArray.get('isLoaded'), true, 'the reload worked, comments are now loaded');

await manyArray.reload().catch(() => {
assert.ok(true, 'An error was thrown on the second reload via manyArray');
});
try {
await manyArray.reload();
assert.ok(false, 'Expected exception to be raised');
} catch (e) {
assert.ok(true, `An error was thrown on the second reload via manyArray: ${e.message}`);
}

assert.equal(
manyArray.get('isLoaded'),
Expand All @@ -1020,6 +1028,7 @@ module('integration/relationships/has_many - Has-Many Relationships', function(h
'the third reload worked, comments are loaded again'
);
assert.ok(reloadedManyArray === manyArray, 'the many array stays the same');
assert.equal(loadingCount, 4, 'We only fired 4 requests');
});

test('A hasMany relationship can be directly reloaded if it was fetched via links', function(assert) {
Expand Down Expand Up @@ -2135,25 +2144,26 @@ module('integration/relationships/has_many - Has-Many Relationships', function(h
);
});

skip('A sync hasMany errors out if there are unloaded records in it', function(assert) {
let post = run(() => {
env.store.push({
data: {
type: 'post',
id: '1',
relationships: {
comments: {
data: [{ type: 'comment', id: '1' }, { type: 'comment', id: '2' }],
},
test('A sync hasMany errors out if there are unloaded records in it', function(assert) {
let post = env.store.push({
data: {
type: 'post',
id: '1',
relationships: {
comments: {
data: [{ type: 'comment', id: '1' }, { type: 'comment', id: '2' }],
},
},
});
return env.store.peekRecord('post', 1);
},
});
const assertionMessage = /You looked up the 'comments' relationship on a 'post' with id 1 but some of the associated records were not loaded./;

assert.expectAssertion(() => {
run(post, 'get', 'comments');
}, /You looked up the 'comments' relationship on a 'post' with id 1 but some of the associated records were not loaded. Either make sure they are all loaded together with the parent record, or specify that the relationship is async \(`DS.hasMany\({ async: true }\)`\)/);
try {
post.get('comments');
assert.ok(false, 'expected assertion');
} catch (e) {
assert.ok(assertionMessage.test(e.message), 'correct assertion');
}
});

test('After removing and unloading a record, a hasMany relationship should still be valid', function(assert) {
Expand Down
6 changes: 5 additions & 1 deletion packages/store/addon/-private/system/many-array.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ import recordDataFor from './record-data-for';
@uses Ember.MutableArray, Ember.Evented
*/
export default EmberObject.extend(MutableArray, Evented, {
// here to make TS happy
_inverseIsAsync: false,
isLoaded: false,

init() {
this._super(...arguments);

Expand All @@ -65,7 +69,7 @@ export default EmberObject.extend(MutableArray, Evented, {
@property {Boolean} isLoaded
*/
this.isLoaded = false;
this.isLoaded = this.isLoaded || false;
this.length = 0;

/**
Expand Down
Loading

0 comments on commit 0dd8ddb

Please sign in to comment.