From cde60fc63499b69757b103334118ca8ea06b53b5 Mon Sep 17 00:00:00 2001
From: Jon Wire <iambipedal@gmail.com>
Date: Fri, 9 Dec 2022 11:36:22 -0600
Subject: [PATCH] chore: log a warning when DataStore is ignoring an attempted
 PK update (#10756)

---
 packages/datastore/__tests__/DataStore.ts     | 24 +++++++++++++++++++
 packages/datastore/src/datastore/datastore.ts | 10 +++++++-
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/packages/datastore/__tests__/DataStore.ts b/packages/datastore/__tests__/DataStore.ts
index 1c20bd37b64..2245cb965c7 100644
--- a/packages/datastore/__tests__/DataStore.ts
+++ b/packages/datastore/__tests__/DataStore.ts
@@ -2096,6 +2096,8 @@ describe('DataStore tests', () => {
 		});
 
 		test('Id cannot be changed inside copyOf', () => {
+			const consoleWarn = jest.spyOn(console, 'warn');
+
 			const { Model } = initSchema(testSchema()) as {
 				Model: PersistentModelConstructor<Model>;
 			};
@@ -2111,6 +2113,16 @@ describe('DataStore tests', () => {
 
 			// ID should be kept the same
 			expect(model1.id).toBe(model2.id);
+
+			// we should always be told *in some way* when an "update" will not actually
+			// be applied. for now, this is a warning, because throwing an error, updating
+			// the record's PK, or creating a new record are all breaking changes.
+			expect(consoleWarn).toHaveBeenCalledWith(
+				expect.stringContaining(
+					"copyOf() does not update PK fields. The 'id' update is being ignored."
+				),
+				expect.objectContaining({ source: model1 })
+			);
 		});
 
 		test('Optional field can be initialized with undefined', () => {
@@ -3629,6 +3641,8 @@ describe('DataStore tests', () => {
 			});
 
 			test('postId cannot be changed inside copyOf', () => {
+				const consoleWarn = jest.spyOn(console, 'warn');
+
 				const { PostCustomPK } = initSchema(testSchema()) as {
 					PostCustomPK: PersistentModelConstructor<PostCustomPKType>;
 				};
@@ -3645,6 +3659,16 @@ describe('DataStore tests', () => {
 
 				// postId should be kept the same
 				expect(model1.postId).toBe(model2.postId);
+
+				// we should always be told *in some way* when an "update" will not actually
+				// be applied. for now, this is a warning, because throwing an error, updating
+				// the record's PK, or creating a new record are all breaking changes.
+				expect(consoleWarn).toHaveBeenCalledWith(
+					expect.stringContaining(
+						"copyOf() does not update PK fields. The 'postId' update is being ignored."
+					),
+					expect.objectContaining({ source: model1 })
+				);
 			});
 
 			test('Optional field can be initialized with undefined', () => {
diff --git a/packages/datastore/src/datastore/datastore.ts b/packages/datastore/src/datastore/datastore.ts
index 1ac185b13aa..7835bc971b6 100644
--- a/packages/datastore/src/datastore/datastore.ts
+++ b/packages/datastore/src/datastore/datastore.ts
@@ -826,7 +826,15 @@ const createModelClass = <T extends PersistentModel>(
 
 					const keyNames = extractPrimaryKeyFieldNames(modelDefinition);
 					// Keys are immutable
-					keyNames.forEach(key => ((draft as Object)[key] = source[key]));
+					keyNames.forEach(key => {
+						if (draft[key] !== source[key]) {
+							logger.warn(
+								`copyOf() does not update PK fields. The '${key}' update is being ignored.`,
+								{ source }
+							);
+						}
+						(draft as Object)[key] = source[key];
+					});
 
 					const modelValidator = validateModelFields(modelDefinition);
 					Object.entries(draft).forEach(([k, v]) => {