From 902034648962f7fa945d2ca213d7cf11cb75f466 Mon Sep 17 00:00:00 2001 From: Andrew Kirwin Date: Wed, 4 Nov 2020 15:18:55 +0000 Subject: [PATCH 1/2] don't expose the value of a model's attribute in production builds --- packages/-ember-data/tests/unit/model-test.js | 36 +++++++++++++++++-- .../-private/system/model/internal-model.ts | 6 +++- 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/packages/-ember-data/tests/unit/model-test.js b/packages/-ember-data/tests/unit/model-test.js index f01c4b3077a..7c48fbaaa5a 100644 --- a/packages/-ember-data/tests/unit/model-test.js +++ b/packages/-ember-data/tests/unit/model-test.js @@ -140,9 +140,13 @@ module('unit/model - Model', function(hooks) { 'the deleted person is not removed from store (no unload called)' ); - assert.expectAssertion(() => { - set(record, 'isArchived', true); - }, /Attempted to set 'isArchived' to 'true' on the deleted record /); + assert.expectAssertion( + () => { + set(record, 'isArchived', true); + }, + /Attempted to set 'isArchived' on the deleted record /, + "Assertion does not leak the 'value'" + ); currentState = record._internalModel.currentState; @@ -151,6 +155,32 @@ module('unit/model - Model', function(hooks) { assert.ok(get(record, 'isArchived') === false, 'The record reflects canonical state'); }); + testInDebug('Assertion for dirtying in root.deleted.saved', async function(assert) { + adapter.deleteRecord = () => { + return resolve({ data: null }); + }; + + let record = store.push({ + data: { + type: 'person', + id: '1', + attributes: { + isDrugAddict: false, + }, + }, + }); + + await record.destroyRecord(); + + assert.expectAssertion( + () => { + set(record, 'isDrugAddict', true); + }, + /Attempted to set 'isDrugAddict' to 'true' on the deleted record /, + 'Assertion includes more context when in DEBUG' + ); + }); + test('currentState is accessible when the record is created', async function(assert) { let record = store.push({ data: { diff --git a/packages/store/addon/-private/system/model/internal-model.ts b/packages/store/addon/-private/system/model/internal-model.ts index b57b335760a..f23064bb0c2 100644 --- a/packages/store/addon/-private/system/model/internal-model.ts +++ b/packages/store/addon/-private/system/model/internal-model.ts @@ -913,7 +913,11 @@ export default class InternalModel { setDirtyAttribute(key, value) { if (this.isDeleted()) { - throw new EmberError(`Attempted to set '${key}' to '${value}' on the deleted record ${this}`); + if (DEBUG) { + throw new EmberError(`Attempted to set '${key}' to '${value}' on the deleted record ${this}`); + } else { + throw new EmberError(`Attempted to set '${key}' on the deleted record ${this}`); + } } let currentValue = this.getAttributeValue(key); From 8d108e86b413430627ddc4816c374713c25c9583 Mon Sep 17 00:00:00 2001 From: Andrew Kirwin Date: Tue, 24 Nov 2020 14:43:08 +0000 Subject: [PATCH 2/2] fix failing test: use assert.throws instead of assert.expectAssertion because we want to explicitly test DEBUG vs NOT DEBUG, and assertions are stripped in prod --- packages/-ember-data/tests/unit/model-test.js | 51 +++++++------------ 1 file changed, 18 insertions(+), 33 deletions(-) diff --git a/packages/-ember-data/tests/unit/model-test.js b/packages/-ember-data/tests/unit/model-test.js index 410b8755a61..ee5e5efba00 100644 --- a/packages/-ember-data/tests/unit/model-test.js +++ b/packages/-ember-data/tests/unit/model-test.js @@ -1,6 +1,7 @@ import { computed, get, observer, set } from '@ember/object'; import { guidFor } from '@ember/object/internals'; import { settled } from '@ember/test-helpers'; +import { DEBUG } from '@glimmer/env'; import { module, test } from 'qunit'; import { reject, resolve } from 'rsvp'; @@ -141,13 +142,23 @@ module('unit/model - Model', function(hooks) { 'the deleted person is not removed from store (no unload called)' ); - assert.expectAssertion( - () => { - set(record, 'isArchived', true); - }, - /Attempted to set 'isArchived' on the deleted record /, - "Assertion does not leak the 'value'" - ); + if (DEBUG) { + assert.throws( + () => { + set(record, 'isArchived', true); + }, + /Attempted to set 'isArchived' to 'true' on the deleted record /, + 'Assertion includes more context when in DEBUG' + ); + } else { + assert.throws( + () => { + set(record, 'isArchived', true); + }, + /Attempted to set 'isArchived' on the deleted record /, + "Assertion does not leak the 'value'" + ); + } currentState = record._internalModel.currentState; @@ -156,32 +167,6 @@ module('unit/model - Model', function(hooks) { assert.ok(get(record, 'isArchived') === false, 'The record reflects canonical state'); }); - testInDebug('Assertion for dirtying in root.deleted.saved', async function(assert) { - adapter.deleteRecord = () => { - return resolve({ data: null }); - }; - - let record = store.push({ - data: { - type: 'person', - id: '1', - attributes: { - isDrugAddict: false, - }, - }, - }); - - await record.destroyRecord(); - - assert.expectAssertion( - () => { - set(record, 'isDrugAddict', true); - }, - /Attempted to set 'isDrugAddict' to 'true' on the deleted record /, - 'Assertion includes more context when in DEBUG' - ); - }); - test('currentState is accessible when the record is created', async function(assert) { let record = store.push({ data: {