From 6c650ec7e76cb78097b8b8a625e4ace87be1eddd Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Thu, 31 Aug 2023 22:32:32 -0700 Subject: [PATCH] fix more tests --- @types/ember-data-qunit-asserts/index.d.ts | 2 + packages/graph/src/-private/-diff.ts | 88 ++++- .../operations/add-to-related-records.ts | 2 +- .../operations/remove-from-related-records.ts | 4 + .../operations/replace-related-record.ts | 63 +++- .../operations/replace-related-records.ts | 42 +-- .../qunit-asserts/assert-better.ts | 51 +++ .../addon-test-support/qunit-asserts/index.ts | 2 + .../integration/adapter/rest-adapter-test.js | 6 +- .../relationships/has-many-test.js | 286 ++++++++-------- tests/main/tests/integration/snapshot-test.js | 86 ++++- .../model/relationships/belongs-to-test.js | 99 +++--- .../unit/model/relationships/has-many-test.js | 315 ++++++++++-------- 13 files changed, 662 insertions(+), 384 deletions(-) create mode 100644 packages/unpublished-test-infra/addon-test-support/qunit-asserts/assert-better.ts diff --git a/@types/ember-data-qunit-asserts/index.d.ts b/@types/ember-data-qunit-asserts/index.d.ts index 1247fd980b9..533c72b2f35 100644 --- a/@types/ember-data-qunit-asserts/index.d.ts +++ b/@types/ember-data-qunit-asserts/index.d.ts @@ -22,6 +22,7 @@ declare global { expectNoWarning(callback: () => unknown): Promise; expectAssertion(callback: () => unknown, matcher: string | RegExp): Promise; expectNoAssertion(callback: () => unknown): Promise; + arrayStrictEquals(actual: T[], expected: T[], message: string): void; } namespace QUnit { @@ -33,6 +34,7 @@ declare global { expectNoWarning(callback: () => unknown): Promise; expectAssertion(callback: () => unknown, matcher: string | RegExp): Promise; expectNoAssertion(callback: () => unknown): Promise; + arrayStrictEquals(actual: T[], expected: T[], message: string): void; } } diff --git a/packages/graph/src/-private/-diff.ts b/packages/graph/src/-private/-diff.ts index 9f1233603bf..12286da4675 100644 --- a/packages/graph/src/-private/-diff.ts +++ b/packages/graph/src/-private/-diff.ts @@ -223,7 +223,8 @@ export function _addLocal( graph: Graph, record: StableRecordIdentifier, relationship: CollectionEdge, - value: StableRecordIdentifier + value: StableRecordIdentifier, + index: number | null ): boolean { const { remoteMembers, removals } = relationship; let additions = relationship.additions; @@ -256,7 +257,25 @@ export function _addLocal( } } - relationship.isDirty = true; + // if we have existing localState + // and we have an index + // apply the change, as this is more efficient + // than recomputing localState and + // it allows us to preserve local ordering + // to a small extend. Local ordering should not + // be relied upon as any remote change will blow it away + if (relationship.localState) { + if (index !== null) { + relationship.localState.splice(index, 0, value); + } else { + relationship.localState.push(value); + } + } + assert( + `Expected relationship to be dirty when adding a local mutation`, + relationship.localState || relationship.isDirty + ); + return true; } @@ -284,6 +303,69 @@ export function _removeLocal(relationship: CollectionEdge, value: StableRecordId removals.add(value); } - relationship.isDirty = true; + // if we have existing localState + // apply the change, as this is more efficient + // than recomputing localState and + // it allows us to preserve local ordering + // to a small extend. Local ordering should not + // be relied upon as any remote change will blow it away + if (relationship.localState) { + const index = relationship.localState.indexOf(value); + assert(`Cannot remove a resource that is not present`, index !== -1); + relationship.localState.splice(index, 1); + } + assert( + `Expected relationship to be dirty when performing a local mutation`, + relationship.localState || relationship.isDirty + ); + + return true; +} + +export function _removeRemote(relationship: CollectionEdge, value: StableRecordIdentifier): boolean { + assert(`expected an identifier to remove from the collection relationship`, value); + const { remoteMembers, additions, removals, remoteState } = relationship; + + assert(`Cannot remove a resource that is not present`, remoteMembers.has(value)); + if (!remoteMembers.has(value)) { + return false; + } + + // remove from remote state + remoteMembers.delete(value); + let index = remoteState.indexOf(value); + assert(`Cannot remove a resource that is not present`, index !== -1); + remoteState.splice(index, 1); + + // remove from removals if present + if (removals?.has(value)) { + removals.delete(value); + + // nothing more to do this was our state already + return false; + } + + assert( + `Remote state indicated removal of a resource that was present only as a local mutation`, + !additions?.has(value) + ); + + // if we have existing localState + // and we have an index + // apply the change, as this is more efficient + // than recomputing localState and + // it allows us to preserve local ordering + // to a small extend. Local ordering should not + // be relied upon as any remote change will blow it away + if (relationship.localState) { + index = relationship.localState.indexOf(value); + assert(`Cannot remove a resource that is not present`, index !== -1); + relationship.localState.splice(index, 1); + } + assert( + `Expected relationship to be dirty when performing a local mutation`, + relationship.localState || relationship.isDirty + ); + return true; } diff --git a/packages/graph/src/-private/operations/add-to-related-records.ts b/packages/graph/src/-private/operations/add-to-related-records.ts index 5a350964e6d..5faa110214b 100644 --- a/packages/graph/src/-private/operations/add-to-related-records.ts +++ b/packages/graph/src/-private/operations/add-to-related-records.ts @@ -41,7 +41,7 @@ function addRelatedRecord( ) { assert(`expected an identifier to add to the collection relationship`, value); - if (_addLocal(graph, record, relationship, value)) { + if (_addLocal(graph, record, relationship, value, index ?? null)) { addToInverse(graph, value, relationship.definition.inverseKey, record, isRemote); } } diff --git a/packages/graph/src/-private/operations/remove-from-related-records.ts b/packages/graph/src/-private/operations/remove-from-related-records.ts index f0faaa40b87..c9e88400611 100644 --- a/packages/graph/src/-private/operations/remove-from-related-records.ts +++ b/packages/graph/src/-private/operations/remove-from-related-records.ts @@ -20,6 +20,10 @@ export default function removeFromRelatedRecords(graph: Graph, op: RemoveFromRel `You can only '${op.op}' on a hasMany relationship. ${record.type}.${op.field} is a ${relationship.definition.kind}`, isHasMany(relationship) ); + // TODO we should potentially thread the index information through here + // when available as it may make it faster to remove from the local state + // when trying to patch more efficiently without blowing away the entire + // local state array if (Array.isArray(value)) { for (let i = 0; i < value.length; i++) { removeRelatedRecord(graph, relationship, record, value[i], isRemote); diff --git a/packages/graph/src/-private/operations/replace-related-record.ts b/packages/graph/src/-private/operations/replace-related-record.ts index 081d33c70d4..df64df70f1e 100644 --- a/packages/graph/src/-private/operations/replace-related-record.ts +++ b/packages/graph/src/-private/operations/replace-related-record.ts @@ -1,5 +1,6 @@ -import { assert } from '@ember/debug'; +import { assert, deprecate } from '@ember/debug'; +import { DEPRECATE_RELATIONSHIP_REMOTE_UPDATE_CLEARING_LOCAL_STATE } from '@ember-data/deprecations'; import { DEBUG } from '@ember-data/env'; import type { StableRecordIdentifier } from '@ember-data/types/q/identifier'; @@ -81,10 +82,31 @@ export default function replaceRelatedRecord(graph: Graph, op: ReplaceRelatedRec } if (existingState && localState === existingState) { notifyInverseOfPotentialMaterialization(graph, existingState, definition.inverseKey, op.record, isRemote); - } else { - relationship.localState = existingState; - - notifyChange(graph, relationship.identifier, relationship.definition.key); + } else if (DEPRECATE_RELATIONSHIP_REMOTE_UPDATE_CLEARING_LOCAL_STATE) { + // if localState does not match existingState then we know + // we have a local mutation that has not been persisted yet + if (localState !== op.value) { + relationship.localState = existingState; + + deprecate( + `EmberData is changing the default semantics of updates to the remote state of relationships.\n\nThe following local state was cleared from the <${ + relationship.identifier.type + }>.${ + relationship.definition.key + } belongsTo relationship but will not be once this deprecation is resolved:\n\n\t${ + localState ? 'Added: ' + localState.lid + '\n\t' : '' + }${existingState ? 'Removed: ' + existingState.lid : ''}`, + false, + { + id: 'ember-data:deprecate-relationship-remote-update-clearing-local-state', + for: 'ember-data', + since: { enabled: '5.3', available: '5.3' }, + until: '6.0', + } + ); + + notifyChange(graph, relationship.identifier, relationship.definition.key); + } } } return; @@ -127,9 +149,38 @@ export default function replaceRelatedRecord(graph: Graph, op: ReplaceRelatedRec if (localState && isNew(localState) && !remoteState) { return; } - if (localState !== remoteState) { + // when localState does not match the new remoteState and + // localState === existingState then we had no local mutation + // and we can safely sync the new remoteState to local + if (localState !== remoteState && localState === existingState) { relationship.localState = remoteState; notifyChange(graph, relationship.identifier, relationship.definition.key); + // But when localState does not match the new remoteState and + // and localState !== existingState then we know we have a local mutation + // that has not been persisted yet. + } else if (DEPRECATE_RELATIONSHIP_REMOTE_UPDATE_CLEARING_LOCAL_STATE) { + if (localState !== remoteState && localState !== existingState) { + relationship.localState = existingState; + + deprecate( + `EmberData is changing the default semantics of updates to the remote state of relationships.\n\nThe following local state was cleared from the <${ + relationship.identifier.type + }>.${ + relationship.definition.key + } belongsTo relationship but will not be once this deprecation is resolved:\n\n\t${ + localState ? 'Added: ' + localState.lid + '\n\t' : '' + }${existingState ? 'Removed: ' + existingState.lid : ''}`, + false, + { + id: 'ember-data:deprecate-relationship-remote-update-clearing-local-state', + for: 'ember-data', + since: { enabled: '5.3', available: '5.3' }, + until: '6.0', + } + ); + + notifyChange(graph, relationship.identifier, relationship.definition.key); + } } } else { notifyChange(graph, relationship.identifier, relationship.definition.key); diff --git a/packages/graph/src/-private/operations/replace-related-records.ts b/packages/graph/src/-private/operations/replace-related-records.ts index d62595b22f2..9458f6cb644 100644 --- a/packages/graph/src/-private/operations/replace-related-records.ts +++ b/packages/graph/src/-private/operations/replace-related-records.ts @@ -4,7 +4,7 @@ import { DEPRECATE_RELATIONSHIP_REMOTE_UPDATE_CLEARING_LOCAL_STATE } from '@embe import { DEBUG } from '@ember-data/env'; import type { StableRecordIdentifier } from '@ember-data/types/q/identifier'; -import { _addLocal, _removeLocal, diffCollection } from '../-diff'; +import { _addLocal, _removeLocal, _removeRemote, diffCollection } from '../-diff'; import type { ReplaceRelatedRecordsOperation } from '../-operations'; import { isBelongsTo, isHasMany, isNew, notifyChange } from '../-utils'; import { assertPolymorphicType } from '../debug/assert-polymorphic-type'; @@ -178,12 +178,16 @@ function replaceRelatedRecordsRemote(graph: Graph, op: ReplaceRelatedRecordsOper relationship.remoteMembers = diff.finalSet; relationship.remoteState = diff.finalState; + // changed also indicates a change in order + if (diff.changed) { + relationship.isDirty = true; + } + // TODO unsure if we need this but it // may allow us to more efficiently patch // the associated ManyArray relationship._diff = diff; - debugger; if (DEPRECATE_RELATIONSHIP_REMOTE_UPDATE_CLEARING_LOCAL_STATE) { // only do this for legacy hasMany, not collection // and provide a way to incrementally migrate @@ -198,9 +202,9 @@ function replaceRelatedRecordsRemote(graph: Graph, op: ReplaceRelatedRecordsOper triggered: false, }; if (relationship.removals) { - deprecationInfo.triggered = true; relationship.isDirty = true; relationship.removals.forEach((identifier) => { + deprecationInfo.triggered = true; deprecationInfo.removals.push(identifier); // reverse the removal // if we are still in removals at this point then @@ -232,7 +236,11 @@ function replaceRelatedRecordsRemote(graph: Graph, op: ReplaceRelatedRecordsOper if (deprecationInfo.triggered) { deprecate( - `EmberData is changing the default semantics of updates to the remote state of relationships.\n\nThe following local state was cleared but will not be once this deprecation is resolved:\n\n\tAdded: [${deprecationInfo.additions + `EmberData is changing the default semantics of updates to the remote state of relationships.\n\nThe following local state was cleared from the <${ + relationship.identifier.type + }>.${ + relationship.definition.key + } hasMany relationship but will not be once this deprecation is resolved:\n\n\tAdded: [${deprecationInfo.additions .map((i) => i.lid) .join(', ')}]\n\tRemoved: [${deprecationInfo.removals.map((i) => i.lid).join(', ')}]`, false, @@ -247,14 +255,7 @@ function replaceRelatedRecordsRemote(graph: Graph, op: ReplaceRelatedRecordsOper } } - if (diff.changed) { - flushCanonical(graph, relationship); - } else { - // TODO - // this is because we used to eagerly notify on remote updates which - // has the effect of clearing certain kinds of local changes. - // preserve legacy behavior we want to change but requires some sort - // of deprecation. + if (relationship.isDirty) { flushCanonical(graph, relationship); } } @@ -297,6 +298,11 @@ export function addToInverse( } } else if (isHasMany(relationship)) { if (isRemote) { + // TODO this needs to alert stuffs + // And patch state better + // This is almost definitely wrong + // WARNING WARNING WARNING + if (!relationship.remoteMembers.has(value)) { graph._addToTransaction(relationship); relationship.remoteState.push(value); @@ -310,7 +316,7 @@ export function addToInverse( } } } else { - if (_addLocal(graph, identifier, relationship, value)) { + if (_addLocal(graph, identifier, relationship, value, null)) { notifyChange(graph, identifier, key); } } @@ -363,15 +369,9 @@ export function removeFromInverse( } } else if (isHasMany(relationship)) { if (isRemote) { - // TODO this needs to alert stuffs - // And patch state better - // This is almost definitely wrong - // WARNING WARNING WARNING graph._addToTransaction(relationship); - let index = relationship.remoteState.indexOf(value); - if (index !== -1) { - relationship.remoteMembers.delete(value); - relationship.remoteState.splice(index, 1); + if (_removeRemote(relationship, value)) { + notifyChange(graph, identifier, key); } } else { if (_removeLocal(relationship, value)) { diff --git a/packages/unpublished-test-infra/addon-test-support/qunit-asserts/assert-better.ts b/packages/unpublished-test-infra/addon-test-support/qunit-asserts/assert-better.ts new file mode 100644 index 00000000000..1220dc33650 --- /dev/null +++ b/packages/unpublished-test-infra/addon-test-support/qunit-asserts/assert-better.ts @@ -0,0 +1,51 @@ +import QUnit from 'qunit'; + +let HAS_REGISTERED = false; + +function refFromIndex(index: number, suffix: string): string { + return ``; +} +function getRefForItem(map: Map, item: T, index: number): string { + let ref = map.get(item); + if (ref === undefined) { + ref = refFromIndex(index, 'b'); + } + return ref; +} + +export function configureBetterAsserts() { + if (HAS_REGISTERED === true) { + throw new Error(`Attempting to re-register better assertion tools`); + } + HAS_REGISTERED = true; + + QUnit.assert.arrayStrictEquals = function (actual: T[], expected: T[], message: string): void { + let passed = actual.length === expected.length; + + let actualRefs = new Map(); + let actualSerialized: string[] = actual.map((item, index) => { + let ref = refFromIndex(index, ''); + actualRefs.set(item, ref); + return ref; + }); + let expectedSerialized: string[] = expected.map((item, index) => { + return getRefForItem(actualRefs, item, index); + }); + + if (passed) { + for (let i = 0; i < actual.length; i++) { + if (actual[i] !== expected[i]) { + passed = false; + break; + } + } + } + + this.pushResult({ + result: passed, + actual: actualSerialized, + expected: expectedSerialized, + message, + }); + }; +} diff --git a/packages/unpublished-test-infra/addon-test-support/qunit-asserts/index.ts b/packages/unpublished-test-infra/addon-test-support/qunit-asserts/index.ts index 8993fa5e150..feb60cb78f8 100644 --- a/packages/unpublished-test-infra/addon-test-support/qunit-asserts/index.ts +++ b/packages/unpublished-test-infra/addon-test-support/qunit-asserts/index.ts @@ -1,4 +1,5 @@ import { configureAssertionHandler } from './assert-assertion'; +import { configureBetterAsserts } from './assert-better'; import { configureDeprecationHandler } from './assert-deprecation'; import { configureWarningHandler } from './assert-warning'; @@ -6,4 +7,5 @@ export default function configureAsserts() { configureAssertionHandler(); configureDeprecationHandler(); configureWarningHandler(); + configureBetterAsserts(); } diff --git a/tests/main/tests/integration/adapter/rest-adapter-test.js b/tests/main/tests/integration/adapter/rest-adapter-test.js index be8ae74db30..a184ce1a694 100644 --- a/tests/main/tests/integration/adapter/rest-adapter-test.js +++ b/tests/main/tests/integration/adapter/rest-adapter-test.js @@ -427,9 +427,9 @@ module('integration/adapter/rest_adapter - REST Adapter', function (hooks) { posts: { id: '1', name: 'Not everyone uses Rails', comments: [2] }, }); - await store.findRecord('comment', 2); - let post = await store.findRecord('post', 1); - let newComment = store.peekRecord('comment', 2); + await store.findRecord('comment', '2'); + let post = await store.findRecord('post', '1'); + let newComment = store.peekRecord('comment', '2'); let comments = post.comments; // Replace the comment with a new one diff --git a/tests/main/tests/integration/relationships/has-many-test.js b/tests/main/tests/integration/relationships/has-many-test.js index decb71d27b2..638c18b0114 100644 --- a/tests/main/tests/integration/relationships/has-many-test.js +++ b/tests/main/tests/integration/relationships/has-many-test.js @@ -2461,43 +2461,28 @@ If using this relationship in a polymorphic manner is desired, the relationships assert.strictEqual(run(post, 'get', 'comments.length'), 0); }); - test('If reordered hasMany data has been pushed to the store, the many array reflects the ordering change - sync', function (assert) { + test('If reordered hasMany data has been pushed to the store, the many array reflects the ordering change - sync', async function (assert) { let store = this.owner.lookup('service:store'); - let comment1, comment2, comment3, comment4; - let post; - - run(() => { - store.push({ - data: [ - { - type: 'comment', - id: '1', - }, - { - type: 'comment', - id: '2', - }, - { - type: 'comment', - id: '3', - }, - { - type: 'comment', - id: '4', - }, - ], - }); - - comment1 = store.peekRecord('comment', 1); - comment2 = store.peekRecord('comment', 2); - comment3 = store.peekRecord('comment', 3); - comment4 = store.peekRecord('comment', 4); - }); - - run(() => { - store.push({ - data: { + const [comment1, comment2, comment3, comment4, post] = store.push({ + data: [ + { + type: 'comment', + id: '1', + }, + { + type: 'comment', + id: '2', + }, + { + type: 'comment', + id: '3', + }, + { + type: 'comment', + id: '4', + }, + { type: 'post', id: '1', relationships: { @@ -2509,103 +2494,99 @@ If using this relationship in a polymorphic manner is desired, the relationships }, }, }, - }); - post = store.peekRecord('post', 1); - - assert.deepEqual(post.comments.slice(), [comment1, comment2], 'Initial ordering is correct'); + ], }); - run(() => { - store.push({ - data: { - type: 'post', - id: '1', - relationships: { - comments: { - data: [ - { type: 'comment', id: '2' }, - { type: 'comment', id: '1' }, - ], - }, + assert.arrayStrictEquals(post.comments.slice(), [comment1, comment2], 'Initial ordering is correct'); + + store.push({ + data: { + type: 'post', + id: '1', + relationships: { + comments: { + data: [ + { type: 'comment', id: '2' }, + { type: 'comment', id: '1' }, + ], }, }, - }); + }, }); - assert.deepEqual(post.comments.slice(), [comment2, comment1], 'Updated ordering is correct'); + assert.arrayStrictEquals(post.comments.slice(), [comment2, comment1], 'Updated ordering is correct'); - run(() => { - store.push({ - data: { - type: 'post', - id: '1', - relationships: { - comments: { - data: [{ type: 'comment', id: '2' }], - }, + store.push({ + data: { + type: 'post', + id: '1', + relationships: { + comments: { + data: [{ type: 'comment', id: '2' }], }, }, - }); + }, }); - assert.deepEqual(post.comments.slice(), [comment2], 'Updated ordering is correct'); + assert.arrayStrictEquals(post.comments.slice(), [comment2], 'Updated ordering is correct'); - run(() => { - store.push({ - data: { - type: 'post', - id: '1', - relationships: { - comments: { - data: [ - { type: 'comment', id: '1' }, - { type: 'comment', id: '2' }, - { type: 'comment', id: '3' }, - { type: 'comment', id: '4' }, - ], - }, + store.push({ + data: { + type: 'post', + id: '1', + relationships: { + comments: { + data: [ + { type: 'comment', id: '1' }, + { type: 'comment', id: '2' }, + { type: 'comment', id: '3' }, + { type: 'comment', id: '4' }, + ], }, }, - }); + }, }); - assert.deepEqual(post.comments.slice(), [comment1, comment2, comment3, comment4], 'Updated ordering is correct'); + assert.arrayStrictEquals( + post.comments.slice(), + [comment1, comment2, comment3, comment4], + 'Updated ordering is correct' + ); - run(() => { - store.push({ - data: { - type: 'post', - id: '1', - relationships: { - comments: { - data: [ - { type: 'comment', id: '4' }, - { type: 'comment', id: '3' }, - ], - }, + store.push({ + data: { + type: 'post', + id: '1', + relationships: { + comments: { + data: [ + { type: 'comment', id: '4' }, + { type: 'comment', id: '3' }, + ], }, }, - }); + }, }); - assert.deepEqual(post.comments.slice(), [comment4, comment3], 'Updated ordering is correct'); + assert.arrayStrictEquals(post.comments.slice(), [comment4, comment3], 'Updated ordering is correct'); - run(() => { - store.push({ - data: { - type: 'post', - id: '1', - relationships: { - comments: { - data: [ - { type: 'comment', id: '4' }, - { type: 'comment', id: '2' }, - { type: 'comment', id: '3' }, - { type: 'comment', id: '1' }, - ], - }, + store.push({ + data: { + type: 'post', + id: '1', + relationships: { + comments: { + data: [ + { type: 'comment', id: '4' }, + { type: 'comment', id: '2' }, + { type: 'comment', id: '3' }, + { type: 'comment', id: '1' }, + ], }, }, - }); + }, }); - - assert.deepEqual(post.comments.slice(), [comment4, comment2, comment3, comment1], 'Updated ordering is correct'); + assert.arrayStrictEquals( + post.comments.slice(), + [comment4, comment2, comment3, comment1], + 'Updated ordering is correct' + ); }); test('Rollbacking attributes for deleted record restores implicit relationship correctly when the hasMany side has been deleted - async', async function (assert) { @@ -4043,49 +4024,58 @@ If using this relationship in a polymorphic manner is desired, the relationships assert.strictEqual(commentsAgain.length, 2, 'comments have 2 length'); }); - test('Pushing a relationship with duplicate identifiers results in a single entry for the record in the relationship', async function (assert) { - class PhoneUser extends Model { - @hasMany('phone-number', { async: false, inverse: null }) - phoneNumbers; - @attr name; - } - class PhoneNumber extends Model { - @attr number; - } - const { owner } = this; + deprecatedTest( + 'Pushing a relationship with duplicate identifiers results in a single entry for the record in the relationship', + { + id: 'ember-data:deprecate-non-unique-relationship-entries', + until: '6.0', + count: 1, + refactor: true, // should assert + }, + async function (assert) { + class PhoneUser extends Model { + @hasMany('phone-number', { async: false, inverse: null }) + phoneNumbers; + @attr name; + } + class PhoneNumber extends Model { + @attr number; + } + const { owner } = this; - owner.register('model:phone-user', PhoneUser); - owner.register('model:phone-number', PhoneNumber); + owner.register('model:phone-user', PhoneUser); + owner.register('model:phone-number', PhoneNumber); - const store = owner.lookup('service:store'); + const store = owner.lookup('service:store'); - store.push({ - data: { - id: 'call-me-anytime', - type: 'phone-number', - attributes: { - number: '1-800-DATA', + store.push({ + data: { + id: 'call-me-anytime', + type: 'phone-number', + attributes: { + number: '1-800-DATA', + }, }, - }, - }); + }); - const person = store.push({ - data: { - id: '1', - type: 'phone-user', - attributes: {}, - relationships: { - phoneNumbers: { - data: [ - { type: 'phone-number', id: 'call-me-anytime' }, - { type: 'phone-number', id: 'call-me-anytime' }, - { type: 'phone-number', id: 'call-me-anytime' }, - ], + const person = store.push({ + data: { + id: '1', + type: 'phone-user', + attributes: {}, + relationships: { + phoneNumbers: { + data: [ + { type: 'phone-number', id: 'call-me-anytime' }, + { type: 'phone-number', id: 'call-me-anytime' }, + { type: 'phone-number', id: 'call-me-anytime' }, + ], + }, }, }, - }, - }); + }); - assert.strictEqual(person.phoneNumbers.length, 1); - }); + assert.strictEqual(person.phoneNumbers.length, 1); + } + ); }); diff --git a/tests/main/tests/integration/snapshot-test.js b/tests/main/tests/integration/snapshot-test.js index 2ee8712b4a4..d4d4e9aa8c0 100644 --- a/tests/main/tests/integration/snapshot-test.js +++ b/tests/main/tests/integration/snapshot-test.js @@ -1029,9 +1029,9 @@ module('integration/snapshot - Snapshot', function (hooks) { }); test('snapshot.hasMany() respects the order of items in the relationship', async function (assert) { - assert.expect(3); + assert.expect(10); - store.push({ + const [comment1, comment2, comment3, post] = store.push({ data: [ { type: 'comment', @@ -1072,18 +1072,86 @@ module('integration/snapshot - Snapshot', function (hooks) { }, ], }); - let comment3 = store.peekRecord('comment', 3); - let post = store.peekRecord('post', 4); const comments = await post.comments; + let snapshot = post._createSnapshot(); + let relationship = snapshot.hasMany('comments'); + + assert.arrayStrictEquals(comments, [comment1, comment2, comment3], 'initial relationship order is correct'); + assert.arrayStrictEquals( + relationship.map((s) => s.id), + ['1', '2', '3'], + 'initial relationship reference order is correct' + ); + + // change order locally comments.splice(comments.indexOf(comment3), 1); comments.unshift(comment3); - let snapshot = post._createSnapshot(); - let relationship = snapshot.hasMany('comments'); + snapshot = post._createSnapshot(); + relationship = snapshot.hasMany('comments'); + + assert.arrayStrictEquals(comments, [comment3, comment1, comment2], 'relationship preserved local order'); + assert.arrayStrictEquals( + relationship.map((s) => s.id), + ['3', '1', '2'], + 'relationship reference preserved local order' + ); + + // change order locally again + comments.splice(comments.indexOf(comment1), 1); + + snapshot = post._createSnapshot(); + relationship = snapshot.hasMany('comments'); + + assert.arrayStrictEquals(comments, [comment3, comment2], 'relationship preserved local order'); + assert.arrayStrictEquals( + relationship.map((s) => s.id), + ['3', '2'], + 'relationship reference preserved local order' + ); + + // and again + comments.push(comment1); + + snapshot = post._createSnapshot(); + relationship = snapshot.hasMany('comments'); + + assert.arrayStrictEquals(comments, [comment3, comment2, comment1], 'relationship preserved local order'); + assert.arrayStrictEquals( + relationship.map((s) => s.id), + ['3', '2', '1'], + 'relationship reference preserved local order' + ); - assert.strictEqual(relationship[0].id, '3', 'order of comment 3 is correct'); - assert.strictEqual(relationship[1].id, '1', 'order of comment 1 is correct'); - assert.strictEqual(relationship[2].id, '2', 'order of comment 2 is correct'); + // push a new remote state with a different order + store.push({ + data: { + type: 'post', + id: '4', + attributes: { + title: 'Hello World', + }, + relationships: { + comments: { + data: [ + { type: 'comment', id: '3' }, + { type: 'comment', id: '1' }, + { type: 'comment', id: '2' }, + ], + }, + }, + }, + }); + + snapshot = post._createSnapshot(); + relationship = snapshot.hasMany('comments'); + + assert.arrayStrictEquals(comments, [comment3, comment1, comment2], 'relationship updated to remote order'); + assert.arrayStrictEquals( + relationship.map((s) => s.id), + ['3', '1', '2'], + 'relationship updated to remote order' + ); }); test('snapshot.eachAttribute() proxies to record', function (assert) { diff --git a/tests/main/tests/unit/model/relationships/belongs-to-test.js b/tests/main/tests/unit/model/relationships/belongs-to-test.js index 2e4f6d94ac5..997d20b28e5 100644 --- a/tests/main/tests/unit/model/relationships/belongs-to-test.js +++ b/tests/main/tests/unit/model/relationships/belongs-to-test.js @@ -9,6 +9,7 @@ import { setupTest } from 'ember-qunit'; import Adapter from '@ember-data/adapter'; import Model, { attr, belongsTo, hasMany } from '@ember-data/model'; import JSONAPISerializer from '@ember-data/serializer/json-api'; +import { deprecatedTest } from '@ember-data/unpublished-test-infra/test-support/deprecated-test'; import testInDebug from '@ember-data/unpublished-test-infra/test-support/test-in-debug'; module('unit/model/relationships - belongsTo', function (hooks) { @@ -434,64 +435,72 @@ module('unit/model/relationships - belongsTo', function (hooks) { }); }); - test('when response to saving a belongsTo is a success but includes changes that reset the users change', async function (assert) { - const Tag = Model.extend({ - label: attr(), - }); - const User = Model.extend({ - tag: belongsTo('tag', { async: false, inverse: null }), - }); - - this.owner.register('model:tag', Tag); - this.owner.register('model:user', User); + deprecatedTest( + 'when response to saving a belongsTo is a success but includes changes that reset the users change', + { + id: 'ember-data:deprecate-relationship-remote-update-clearing-local-state', + until: '6.0', + count: 1, + refactor: true, // should probably assert against this scenario in dev at the cache level by comparing to in-flight state + }, + async function (assert) { + class Tag extends Model { + @attr label; + } + class User extends Model { + @belongsTo('tag', { async: false, inverse: null }) tag; + } - let store = this.owner.lookup('service:store'); - let adapter = store.adapterFor('application'); + this.owner.register('model:tag', Tag); + this.owner.register('model:user', User); - const [user, tag1, tag2] = store.push({ - data: [ - { - type: 'user', - id: '1', - relationships: { - tag: { - data: { type: 'tag', id: '1' }, + const store = this.owner.lookup('service:store'); + const adapter = store.adapterFor('application'); + const [user, tag1, tag2] = store.push({ + data: [ + { + type: 'user', + id: '1', + relationships: { + tag: { + data: { type: 'tag', id: '1' }, + }, }, }, - }, - { type: 'tag', id: '1', attributes: { label: 'A' } }, - { type: 'tag', id: '2', attributes: { label: 'B' } }, - ], - }); + { type: 'tag', id: '1', attributes: { label: 'A' } }, + { type: 'tag', id: '2', attributes: { label: 'B' } }, + ], + }); - assert.strictEqual(tag1.label, 'A', 'tag1 is loaded'); - assert.strictEqual(tag2.label, 'B', 'tag2 is loaded'); - assert.strictEqual(user.tag.id, '1', 'user starts with tag1 as tag'); + assert.strictEqual(tag1.label, 'A', 'tag1 is loaded'); + assert.strictEqual(tag2.label, 'B', 'tag2 is loaded'); + assert.strictEqual(user.tag.id, '1', 'user starts with tag1 as tag'); - user.set('tag', tag2); + user.set('tag', tag2); - assert.strictEqual(user.tag.id, '2', 'user tag updated to tag2'); + assert.strictEqual(user.tag.id, '2', 'user tag updated to tag2'); - adapter.updateRecord = function () { - return { - data: { - type: 'user', - id: '1', - relationships: { - tag: { - data: { - id: '1', - type: 'tag', + adapter.updateRecord = function () { + return { + data: { + type: 'user', + id: '1', + relationships: { + tag: { + data: { + id: '1', + type: 'tag', + }, }, }, }, - }, + }; }; - }; - await user.save(); - assert.strictEqual(user.tag.id, '1', 'expected new server state to be applied'); - }); + await user.save(); + assert.strictEqual(user.tag.id, '1', 'expected new server state to be applied'); + } + ); test('calling createRecord and passing in an undefined value for a relationship should be treated as if null', function (assert) { assert.expect(1); diff --git a/tests/main/tests/unit/model/relationships/has-many-test.js b/tests/main/tests/unit/model/relationships/has-many-test.js index 5a1cac43ce6..a6986e134a7 100644 --- a/tests/main/tests/unit/model/relationships/has-many-test.js +++ b/tests/main/tests/unit/model/relationships/has-many-test.js @@ -9,6 +9,7 @@ import Model, { attr, belongsTo, hasMany } from '@ember-data/model'; import { LEGACY_SUPPORT, PromiseManyArray } from '@ember-data/model/-private'; import JSONAPISerializer from '@ember-data/serializer/json-api'; import { recordIdentifierFor } from '@ember-data/store'; +import { deprecatedTest } from '@ember-data/unpublished-test-infra/test-support/deprecated-test'; import testInDebug from '@ember-data/unpublished-test-infra/test-support/test-in-debug'; import todo from '@ember-data/unpublished-test-infra/test-support/todo'; @@ -545,77 +546,86 @@ module('unit/model/relationships - hasMany', function (hooks) { assert.strictEqual(tag.people.length, 0, 'relationship is correct'); }); - test('hasMany with duplicates from payload', function (assert) { - assert.expect(1); - - const Tag = Model.extend({ - name: attr('string'), - people: hasMany('person', { async: false, inverse: 'tag' }), - }); + deprecatedTest( + 'hasMany with duplicates from payload', + { + id: 'ember-data:deprecate-non-unique-relationship-entries', + count: 1, + until: '6.0', + refactor: true, // should assert when stripped + }, + function (assert) { + assert.expect(1); + + const Tag = Model.extend({ + name: attr('string'), + people: hasMany('person', { async: false, inverse: 'tag' }), + }); - Tag.toString = () => { - return 'tag'; - }; + Tag.toString = () => { + return 'tag'; + }; - const Person = Model.extend({ - name: attr('string'), - tag: belongsTo('tag', { async: false, inverse: 'people' }), - }); + const Person = Model.extend({ + name: attr('string'), + tag: belongsTo('tag', { async: false, inverse: 'people' }), + }); - Person.toString = () => { - return 'person'; - }; + Person.toString = () => { + return 'person'; + }; - this.owner.register('model:tag', Tag); - this.owner.register('model:person', Person); + this.owner.register('model:tag', Tag); + this.owner.register('model:person', Person); - let store = this.owner.lookup('service:store'); + let store = this.owner.lookup('service:store'); - // first we push in data with the relationship - store.push({ - data: { - type: 'person', - id: '1', - attributes: { - name: 'David J. Hamilton', - }, - relationships: { - tag: { - data: { - type: 'tag', - id: '1', - }, - }, - }, - }, - included: [ - { - type: 'tag', + // first we push in data with the relationship + store.push({ + data: { + type: 'person', id: '1', attributes: { - name: 'whatever', + name: 'David J. Hamilton', }, relationships: { - people: { - data: [ - { - type: 'person', - id: '1', - }, - { - type: 'person', - id: '1', - }, - ], + tag: { + data: { + type: 'tag', + id: '1', + }, }, }, }, - ], - }); + included: [ + { + type: 'tag', + id: '1', + attributes: { + name: 'whatever', + }, + relationships: { + people: { + data: [ + { + type: 'person', + id: '1', + }, + { + type: 'person', + id: '1', + }, + ], + }, + }, + }, + ], + }); - let tag = store.peekRecord('tag', 1); - assert.strictEqual(tag.people.length, 1, 'relationship does not contain duplicates'); - }); + let tag = store.peekRecord('tag', 1); + assert.strictEqual(tag.people.length, 1, 'relationship does not contain duplicates'); + } + ); test('many2many loads both sides #5140', function (assert) { assert.expect(3); @@ -1973,117 +1983,126 @@ module('unit/model/relationships - hasMany', function (hooks) { the parent record's hasMany is a situation in which this limitation will be encountered should other local changes to the relationship still exist. */ - test('[ASSERTS KNOWN LIMITATION STILL EXISTS] returning new hasMany relationship info from a delete clears local state', async function (assert) { - assert.expect(4); + deprecatedTest( + '[ASSERTS KNOWN LIMITATION STILL EXISTS] returning new hasMany relationship info from a delete clears local state', + { + id: 'ember-data:deprecate-relationship-remote-update-clearing-local-state', + until: '6.0', + count: 1, + refactor: true, + }, + async function (assert) { + assert.expect(4); - const Person = Model.extend({ - name: attr('string'), - pets: hasMany('pet', { async: false, inverse: null }), - }); + const Person = Model.extend({ + name: attr('string'), + pets: hasMany('pet', { async: false, inverse: null }), + }); - const Pet = Model.extend({ - name: attr('string'), - person: belongsTo('person', { async: false, inverse: null }), - }); + const Pet = Model.extend({ + name: attr('string'), + person: belongsTo('person', { async: false, inverse: null }), + }); - this.owner.register('model:person', Person); - this.owner.register('model:pet', Pet); + this.owner.register('model:person', Person); + this.owner.register('model:pet', Pet); - let store = this.owner.lookup('service:store'); - let adapter = store.adapterFor('application'); + let store = this.owner.lookup('service:store'); + let adapter = store.adapterFor('application'); - adapter.shouldBackgroundReloadRecord = () => false; - adapter.deleteRecord = () => { - return Promise.resolve({ - data: null, - included: [ - { - type: 'person', - id: '1', - attributes: { - name: 'Chris Thoburn', - }, - relationships: { - pets: { - data: [{ type: 'pet', id: '2' }], + adapter.shouldBackgroundReloadRecord = () => false; + adapter.deleteRecord = () => { + return Promise.resolve({ + data: null, + included: [ + { + type: 'person', + id: '1', + attributes: { + name: 'Chris Thoburn', + }, + relationships: { + pets: { + data: [{ type: 'pet', id: '2' }], + }, }, }, - }, - ], - }); - }; + ], + }); + }; - store.push({ - data: { - type: 'person', - id: '1', - attributes: { - name: 'Chris Thoburn', - }, - relationships: { - pets: { - data: [ - { type: 'pet', id: '1' }, - { type: 'pet', id: '2' }, - ], - }, - }, - }, - included: [ - { - type: 'pet', + store.push({ + data: { + type: 'person', id: '1', attributes: { - name: 'Shenanigans', + name: 'Chris Thoburn', }, - }, - { - type: 'pet', - id: '2', - attributes: { - name: 'Rambunctious', + relationships: { + pets: { + data: [ + { type: 'pet', id: '1' }, + { type: 'pet', id: '2' }, + ], + }, }, }, - { - type: 'pet', - id: '3', - attributes: { - name: 'Rebel', + included: [ + { + type: 'pet', + id: '1', + attributes: { + name: 'Shenanigans', + }, }, - }, - ], - }); + { + type: 'pet', + id: '2', + attributes: { + name: 'Rambunctious', + }, + }, + { + type: 'pet', + id: '3', + attributes: { + name: 'Rebel', + }, + }, + ], + }); - const person = store.peekRecord('person', '1'); - const pets = await person.pets; + const person = store.peekRecord('person', '1'); + const pets = await person.pets; - const shen = store.peekRecord('pet', '1'); - const rebel = store.peekRecord('pet', '3'); + const shen = store.peekRecord('pet', '1'); + const rebel = store.peekRecord('pet', '3'); - assert.strictEqual(shen.name, 'Shenanigans', 'precond - relationships work'); - assert.deepEqual( - pets.map((p) => p.id), - ['1', '2'], - 'precond - relationship has the correct pets to start' - ); + assert.strictEqual(shen.name, 'Shenanigans', 'precond - relationships work'); + assert.deepEqual( + pets.map((p) => p.id), + ['1', '2'], + 'precond - relationship has the correct pets to start' + ); - pets.push(rebel); - await settled(); + pets.push(rebel); + await settled(); - assert.deepEqual( - pets.map((p) => p.id), - ['1', '2', '3'], - 'precond2 - relationship now has the correct three pets' - ); + assert.deepEqual( + pets.map((p) => p.id), + ['1', '2', '3'], + 'precond2 - relationship now has the correct three pets' + ); - await shen.destroyRecord({}); - // were ember-data to now preserve local edits during a relationship push, this would be 2 pets - assert.deepEqual( - pets.map((p) => p.id), - ['2'], // ['2', '3'], - 'we only have one pet' // 'relationship has two pets, we kept the local change' - ); - }); + await shen.destroyRecord({}); + // were ember-data to now preserve local edits during a relationship push, this would be 2 pets + assert.deepEqual( + pets.map((p) => p.id), + ['2'], // ['2', '3'], + 'we only have one pet' // 'relationship has two pets, we kept the local change' + ); + } + ); test('possible to replace items in a relationship using setObjects w/ Ember Enumerable Array/Object as the argument (GH-2533)', function (assert) { assert.expect(2);