Skip to content

Commit

Permalink
fix: dont early exit loop when filtering duplicates from relationship…
Browse files Browse the repository at this point in the history
… updates (#7882)
  • Loading branch information
runspired committed Feb 19, 2022
1 parent b70624a commit 0a6064d
Show file tree
Hide file tree
Showing 8 changed files with 179 additions and 61 deletions.
10 changes: 1 addition & 9 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ const ImportSortGroups = [

module.exports = {
parser: '@babel/eslint-parser',
reportUnusedDisableDirectives: true,
root: true,
parserOptions: {
ecmaVersion: 2018,
Expand All @@ -39,17 +38,10 @@ module.exports = {
requireConfigFile: false,
},
plugins: ['prettier', 'qunit', 'mocha', 'simple-import-sort', 'import'],
extends: [
'eslint:recommended',
'plugin:eslint-comments/recommended',
'plugin:prettier/recommended',
'plugin:qunit/recommended',
],
extends: ['eslint:recommended', 'plugin:prettier/recommended', 'plugin:qunit/recommended'],
rules: {
eqeqeq: 'error',

'eslint-comments/no-unused-disable': 'error',

// Imports
'import/first': 'error',
'import/newline-after-import': 'error',
Expand Down
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@
"eslint-config-prettier": "^8.3.0",
"eslint-plugin-ember": "^10.5.8",
"eslint-plugin-ember-data": "link:./packages/unpublished-eslint-rules",
"eslint-plugin-eslint-comments": "^3.2.0",
"eslint-plugin-import": "^2.25.4",
"eslint-plugin-mocha": "^9.0.0",
"eslint-plugin-node": "^11.1.0",
Expand Down
1 change: 1 addition & 0 deletions packages/-ember-data/node-tests/docs/test-coverage.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ QUnit.module('Docs coverage', function (hooks) {
if (!process.env.REUSE_DOCS) {
buildDocs();
}
// data.json is generated and not always present. So this disable needs to be preserved.
const docs = require('../../dist/docs/data.json'); // eslint-disable-line node/no-missing-require
const expected = require('../fixtures/expected');

Expand Down
1 change: 1 addition & 0 deletions packages/record-data/addon/-private/graph/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ export class Graph {
assert(`Can only perform the operation updateRelationship on remote state`, isRemote);
if (DEBUG) {
// in debug, assert payload validity eagerly
// TODO add deprecations/assertion here for duplicates
assertValidRelationshipPayload(this, op);
}
updateRelationshipOperation(this, op);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,37 +96,44 @@ function replaceRelatedRecordsLocal(graph: Graph, op: ReplaceRelatedRecordsOpera
const iterationLength = currentLength > identifiersLength ? currentLength : identifiersLength;
const equalLength = currentLength === identifiersLength;

for (let i = 0; i < iterationLength; i++) {
for (let i = 0, j = 0; i < iterationLength; i++) {
let adv = false;
if (i < identifiersLength) {
const identifier = identifiers[i];
if (newMembership.has(identifier)) {
break; // skip processing if we encounter a duplicate identifier in the array
}
if (type !== identifier.type) {
assertPolymorphicType(relationship.identifier, relationship.definition, identifier, graph.store);
graph.registerPolymorphicType(type, identifier.type);
}
newState[i] = identifier;
newMembership.add(identifier);

if (!members.has(identifier)) {
changed = true;
addToInverse(graph, identifier, definition.inverseKey, op.record, isRemote);
// skip processing if we encounter a duplicate identifier in the array
if (!newMembership.has(identifier)) {
if (type !== identifier.type) {
assertPolymorphicType(relationship.identifier, relationship.definition, identifier, graph.store);
graph.registerPolymorphicType(type, identifier.type);
}
newState[j] = identifier;
adv = true;
newMembership.add(identifier);

if (!members.has(identifier)) {
changed = true;
addToInverse(graph, identifier, definition.inverseKey, op.record, isRemote);
}
}
}
if (i < currentLength) {
const identifier = currentState[i];

// detect reordering
if (equalLength && newState[i] !== identifier) {
changed = true;
}

if (!newValues.has(identifier)) {
changed = true;
removeFromInverse(graph, identifier, definition.inverseKey, op.record, isRemote);
if (!newMembership.has(identifier)) {
if (equalLength && newState[i] !== identifier) {
changed = true;
}

if (!newValues.has(identifier)) {
changed = true;
removeFromInverse(graph, identifier, definition.inverseKey, op.record, isRemote);
}
}
}
if (adv) {
j++;
}
}

if (changed) {
Expand Down Expand Up @@ -166,37 +173,43 @@ function replaceRelatedRecordsRemote(graph: Graph, op: ReplaceRelatedRecordsOper
const iterationLength = canonicalLength > identifiersLength ? canonicalLength : identifiersLength;
const equalLength = canonicalLength === identifiersLength;

for (let i = 0; i < iterationLength; i++) {
for (let i = 0, j = 0; i < iterationLength; i++) {
let adv = false;
if (i < identifiersLength) {
const identifier = identifiers[i];
if (newMembership.has(identifier)) {
break;
}
if (type !== identifier.type) {
assertPolymorphicType(relationship.identifier, relationship.definition, identifier, graph.store);
graph.registerPolymorphicType(type, identifier.type);
}
newState[i] = identifier;
newMembership.add(identifier);

if (!canonicalMembers.has(identifier)) {
changed = true;
addToInverse(graph, identifier, definition.inverseKey, op.record, isRemote);
if (!newMembership.has(identifier)) {
if (type !== identifier.type) {
assertPolymorphicType(relationship.identifier, relationship.definition, identifier, graph.store);
graph.registerPolymorphicType(type, identifier.type);
}
newState[j] = identifier;
newMembership.add(identifier);
adv = true;

if (!canonicalMembers.has(identifier)) {
changed = true;
addToInverse(graph, identifier, definition.inverseKey, op.record, isRemote);
}
}
}
if (i < canonicalLength) {
const identifier = canonicalState[i];

// detect reordering
if (equalLength && newState[i] !== identifier) {
changed = true;
}
if (!newMembership.has(identifier)) {
// detect reordering
if (equalLength && newState[j] !== identifier) {
changed = true;
}

if (!newValues.has(identifier)) {
changed = true;
removeFromInverse(graph, identifier, definition.inverseKey, op.record, isRemote);
if (!newValues.has(identifier)) {
changed = true;
removeFromInverse(graph, identifier, definition.inverseKey, op.record, isRemote);
}
}
}
if (adv) {
j++;
}
}

if (changed) {
Expand Down
119 changes: 119 additions & 0 deletions packages/record-data/tests/integration/graph/operations-test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
import { settled } from '@ember/test-helpers';

import { module, test } from 'qunit';

import { setupTest } from 'ember-qunit';

import Model, { attr, hasMany } from '@ember-data/model';
import { graphFor, ManyRelationship } from '@ember-data/record-data/-private';
import Store from '@ember-data/store';

module('Integration | Graph | Operations', function (hooks) {
setupTest(hooks);

test('updateRelationship operation filters duplicates', async function (assert) {
const { owner } = this;

class App extends Model {
@attr name;
// eslint-disable-next-line @typescript-eslint/no-unsafe-call
@hasMany('config', { async: false, inverse: null }) configs;
}

class Config extends Model {
@attr name;
}

owner.register('service:store', Store);
owner.register('model:app', App);
owner.register('model:config', Config);
const store = owner.lookup('service:store') as Store;
const graph = graphFor(store);
const appIdentifier = store.identifierCache.getOrCreateRecordIdentifier({ type: 'app', id: '1' });

graph.push({
op: 'updateRelationship',
field: 'configs',
record: appIdentifier,
value: {
data: [
{ type: 'config', id: '1' },
{ type: 'config', id: '1' },
{ type: 'config', id: '1' },
{ type: 'config', id: '2' },
{ type: 'config', id: '3' },
{ type: 'config', id: '4' },
],
},
});
await settled();

const data = graph.get(appIdentifier, 'configs') as ManyRelationship;
assert.deepEqual(
JSON.parse(JSON.stringify(data.getData())),
{
data: [
{ type: 'config', id: '1', lid: '@ember-data:lid-config-1' },
{ type: 'config', id: '2', lid: '@ember-data:lid-config-2' },
{ type: 'config', id: '3', lid: '@ember-data:lid-config-3' },
{ type: 'config', id: '4', lid: '@ember-data:lid-config-4' },
],
},
'we have the expected data'
);
});

test('replaceRelatedRecords operation filters duplicates in a local replace', async function (assert) {
const { owner } = this;

class App extends Model {
@attr name;
// eslint-disable-next-line @typescript-eslint/no-unsafe-call
@hasMany('config', { async: false, inverse: null }) configs;
}

class Config extends Model {
@attr name;
}

owner.register('service:store', Store);
owner.register('model:app', App);
owner.register('model:config', Config);
const store = owner.lookup('service:store') as Store;
const graph = graphFor(store);
const appIdentifier = store.identifierCache.getOrCreateRecordIdentifier({ type: 'app', id: '1' });
const configIdentifier1 = store.identifierCache.getOrCreateRecordIdentifier({ type: 'config', id: '1' });
const configIdentifier2 = store.identifierCache.getOrCreateRecordIdentifier({ type: 'config', id: '2' });
const configIdentifier3 = store.identifierCache.getOrCreateRecordIdentifier({ type: 'config', id: '3' });
const configIdentifier4 = store.identifierCache.getOrCreateRecordIdentifier({ type: 'config', id: '4' });

graph.update({
op: 'replaceRelatedRecords',
field: 'configs',
record: appIdentifier,
value: [
configIdentifier1,
configIdentifier1,
configIdentifier1,
configIdentifier2,
configIdentifier3,
configIdentifier4,
],
});
await settled();

const data = graph.get(appIdentifier, 'configs') as ManyRelationship;
assert.deepEqual(
JSON.parse(JSON.stringify(data.getData())),
{
data: [
{ type: 'config', id: '1', lid: '@ember-data:lid-config-1' },
{ type: 'config', id: '2', lid: '@ember-data:lid-config-2' },
{ type: 'config', id: '3', lid: '@ember-data:lid-config-3' },
{ type: 'config', id: '4', lid: '@ember-data:lid-config-4' },
],
},
'we have the expected data'
);
});
});
1 change: 1 addition & 0 deletions tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
"packages/record-data/types/@ember/polyfills/index.d.ts",
"packages/record-data/tests/integration/graph/polymorphism/implicit-keys-test.ts",
"packages/record-data/tests/integration/graph/graph-test.ts",
"packages/record-data/tests/integration/graph/operations-test.ts",
"packages/record-data/tests/integration/graph/edge-test.ts",
"packages/record-data/tests/integration/graph/edge-removal/setup.ts",
"packages/record-data/tests/integration/graph/edge-removal/helpers.ts",
Expand Down
10 changes: 1 addition & 9 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -7433,14 +7433,6 @@ eslint-plugin-es@^3.0.0:
eslint-utils "^2.0.0"
regexpp "^3.0.0"

eslint-plugin-eslint-comments@^3.2.0:
version "3.2.0"
resolved "https://registry.npmjs.org/eslint-plugin-eslint-comments/-/eslint-plugin-eslint-comments-3.2.0.tgz#9e1cd7b4413526abb313933071d7aba05ca12ffa"
integrity sha512-0jkOl0hfojIHHmEHgmNdqv4fmh7300NdpA9FFpF7zaoLvB/QeXOGNLIo86oAveJFrfB1p05kC8hpEMHM8DwWVQ==
dependencies:
escape-string-regexp "^1.0.5"
ignore "^5.0.5"

eslint-plugin-import@^2.25.4:
version "2.25.4"
resolved "https://registry.npmjs.org/eslint-plugin-import/-/eslint-plugin-import-2.25.4.tgz#322f3f916a4e9e991ac7af32032c25ce313209f1"
Expand Down Expand Up @@ -9293,7 +9285,7 @@ ignore@^4.0.6:
resolved "https://registry.npmjs.org/ignore/-/ignore-4.0.6.tgz#750e3db5862087b4737ebac8207ffd1ef27b25fc"
integrity sha512-cyFDKrqc/YdcWFniJhzI42+AzS+gNwmUzOSFcRCQYwySuBBBy/KjuxWLZ/FHEH6Moq1NizMOBWyTcv8O4OZIMg==

ignore@^5.0.5, ignore@^5.1.1, ignore@^5.1.4, ignore@^5.1.8, ignore@^5.2.0:
ignore@^5.1.1, ignore@^5.1.4, ignore@^5.1.8, ignore@^5.2.0:
version "5.2.0"
resolved "https://registry.npmjs.org/ignore/-/ignore-5.2.0.tgz#6d3bac8fa7fe0d45d9f9be7bac2fc279577e345a"
integrity sha512-CmxgYGiEPCLhfLnpPp1MoRmifwEIOgjcHXxOBjv7mY96c+eWScsOP9c112ZyLdWHi0FxHjI+4uVhKYp/gcdRmQ==
Expand Down

0 comments on commit 0a6064d

Please sign in to comment.