From d852ee1e4edf3ebe476981afd3ff8eec27c01ecb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=B6rn=20Berkefeld?= <joern.berkefeld@accenture.com>
Date: Tue, 6 Dec 2022 12:07:02 +0100
Subject: [PATCH] #580: Ensure deploy-checks on DE-fields know if DE has data

---
 lib/metadataTypes/DataExtensionField.js | 47 +++++++++++++++++++++----
 test/dataExtension.test.js              |  2 +-
 2 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/lib/metadataTypes/DataExtensionField.js b/lib/metadataTypes/DataExtensionField.js
index f33f784c0..90b96d1ff 100644
--- a/lib/metadataTypes/DataExtensionField.js
+++ b/lib/metadataTypes/DataExtensionField.js
@@ -100,6 +100,19 @@ class DataExtensionField extends MetadataType {
      * @returns {Object.<string, TYPE.DataExtensionFieldItem>} existing fields by their original name to allow re-adding FieldType after update
      */
     static async prepareDeployColumnsOnUpdate(deployColumns, deKey) {
+        // get row count to know which field restrictions apply
+        let hasData = false;
+        try {
+            const rowset = await this.client.rest.get(
+                `/data/v1/customobjectdata/key/${deKey}/rowset?$page=1&$pagesize=1`
+            );
+            const rowCount = rowset.count;
+            hasData = rowCount > 0;
+            Util.logger.debug(`dataExtension ${deKey} row count: ${rowCount}`);
+        } catch (ex) {
+            Util.logger.debug(`Could not retrieve rowcount for ${deKey}: ${ex.message}`);
+        }
+
         // retrieve existing fields to enable updating them
         const response = await this.retrieveForCache(
             {
@@ -129,6 +142,7 @@ class DataExtensionField extends MetadataType {
 
                 // Updating to a new FieldType will result in an error; warn & afterwards remove it
                 if (itemOld.FieldType !== item.FieldType) {
+                    // applicable: with or without data but simply ignored by API
                     Util.logger.warn(
                         ` - The Field Type of an existing field cannot be changed. Keeping the original value for [${deKey}].[${item.Name}]: '${itemOld.FieldType}'`
                     );
@@ -141,14 +155,16 @@ class DataExtensionField extends MetadataType {
                 delete item.FieldType;
 
                 if (itemOld.MaxLength > item.MaxLength) {
+                    // applicable: with or without data (Code 310007)
                     Util.logger.warn(
                         ` - The length of an existing field cannot be decreased. Keeping the original value for [${deKey}].[${item.Name}]: '${itemOld.MaxLength}'`
                     );
                     item.MaxLength = itemOld.MaxLength;
                 }
                 if (Util.isFalse(itemOld.IsRequired) && Util.isTrue(item.IsRequired)) {
+                    // applicable: with or without data (Code 310007)
                     Util.logger.warn(
-                        ` - A field cannot be changed to be required on update after it was created to allow nulls: [${deKey}].[${item.Name}]`
+                        ` - A field cannot be changed to be required on update after it was created to allow nulls. Resetting to not equired: [${deKey}].[${item.Name}]`
                     );
                     item.IsRequired = itemOld.IsRequired;
                 }
@@ -179,11 +195,20 @@ class DataExtensionField extends MetadataType {
                 item.ObjectID = itemOld.ObjectID;
             } else {
                 // field is getting added ---
-                if (Util.isTrue(item.IsRequired) && item.DefaultValue === '') {
-                    Util.logger.warn(
-                        ` - Adding new fields to an existing table requires that these fields are either not-required (nullable) or have a default value set. Changing [${deKey}].[${item.Name}] to be not-required`
-                    );
-                    item.IsRequired = false;
+
+                if (hasData && Util.isTrue(item.IsRequired) && item.DefaultValue === '') {
+                    // applicable: with data only
+                    if (Util.isFalse(item.IsPrimaryKey)) {
+                        Util.logger.warn(
+                            ` - Adding new fields to an existing table requires that these fields are either not-required (nullable) or have a default value set. Changing [${deKey}].[${item.Name}] to be not-required`
+                        );
+                        item.IsRequired = false;
+                    } else {
+                        Util.logger.error(
+                            `- You cannot add a new primary key field to an existing table that has data. Removing [${deKey}].[${item.Name}] from deployment`
+                        );
+                        deployColumns.splice(i, 1);
+                    }
                 }
                 if (item.Name_new) {
                     Util.logger.warn(
@@ -194,6 +219,15 @@ class DataExtensionField extends MetadataType {
                 // Field doesn't exist in target, therefore Remove ObjectID if present
                 delete item.ObjectID;
             }
+            if (Util.isTrue(item.IsPrimaryKey) && Util.isFalse(item.IsRequired)) {
+                // applicable: with or without data
+                Util.logger.warn(
+                    `- Primary Key field [${deKey}].[${item.Name}] cannot be not-required (nullable). Changing field to be required!`
+                );
+                item.IsRequired = true;
+            }
+
+            // filter bad manual changes to the json
             if (!Util.isTrue(item.IsRequired) && !Util.isFalse(item.IsRequired)) {
                 Util.logger.error(
                     `- Invalid value for 'IsRequired' of [${deKey}].[${item.Name}]. Found '${item.IsRequired}' instead of 'true'/'false'. Removing field from deploy!`
@@ -207,6 +241,7 @@ class DataExtensionField extends MetadataType {
                 deployColumns.splice(i, 1);
             }
         }
+
         Util.logger.debug(
             `${deployColumns.length} Fields added/updated for [${deKey}]${
                 deployColumns.length ? ': ' : ''
diff --git a/test/dataExtension.test.js b/test/dataExtension.test.js
index acd16fbb7..6e3c1320d 100644
--- a/test/dataExtension.test.js
+++ b/test/dataExtension.test.js
@@ -66,7 +66,7 @@ describe('dataExtension', () => {
             );
             assert.equal(
                 Object.values(testUtils.getAPIHistory()).flat().length,
-                11,
+                12,
                 'Unexpected number of requests made'
             );
             return;