assetlibrary: proposed fix for dropping attribute updates (v2) (#70)

* test(integration-tests): updated tests for this scenario * fix(assetlibrary): updated process of updating groups and devices so drops work ISSUES CLOSED: #64 * chore(misc): changelogs * test(integration-tests): keep both existing and new test cases for updating device/group * test(integration-tests): test cases for clearing+updating attributes on profile * fix(assetlibrary): fix removing attributes for devices, groups, profiles Co-authored-by: Aaron Pittenger <[email protected]>
aws · Mar 29, 2022 · 8259ad0 · 8259ad0
1 parent 3dcf740
commit 8259ad0
Show file tree

Hide file tree

Showing 10 changed files with 140 additions and 58 deletions.
diff --git a/...ges/@cdf/assetlibrary/fix_asset_library_dropping_attributes_problem_2022-03-10-01-49.json b/...ges/@cdf/assetlibrary/fix_asset_library_dropping_attributes_problem_2022-03-10-01-49.json
@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "packageName": "@cdf/assetlibrary",
+      "comment": "updated process of updating groups and devices so drops work",
+      "type": "patch"
+    }
+  ],
+  "packageName": "@cdf/assetlibrary",
+  "email": "[email protected]"
+}
diff --git a/...cdf/integration-tests/fix_asset_library_dropping_attributes_problem_2022-03-10-01-49.json b/...cdf/integration-tests/fix_asset_library_dropping_attributes_problem_2022-03-10-01-49.json
@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "packageName": "@cdf/integration-tests",
+      "comment": "updated process of updating groups and devices so drops work",
+      "type": "patch"
+    }
+  ],
+  "packageName": "@cdf/integration-tests",
+  "email": "[email protected]"
+}
diff --git a/source/packages/integration-tests/features/assetlibrary/full/deviceProfiles.feature b/source/packages/integration-tests/features/assetlibrary/full/deviceProfiles.feature
@@ -21,12 +21,12 @@ Feature: Device Profiles
     Given draft assetlibrary device template "TEST-deviceProfiles-type" does not exist
     And published assetlibrary device template "TEST-deviceProfiles-type" does not exist
     When I create the assetlibrary device template "TEST-deviceProfiles-type" with attributes
-      | properties | {"serialNumber":{"type":["string","null"]},"model":{"type":"string"}} |
+      | properties | {"serialNumber":{"type":["string","null"]},"model":{"type":"string"},"color":{"type":"string"}} |
       | required | ["model"] |
       | relations | {"out":{"linked_to":["test-deviceProfiles-linkablegroup"]}} |
     And publish assetlibrary device template "TEST-deviceProfiles-type"
     Then published assetlibrary device template "TEST-deviceProfiles-type" exists with attributes
-      | properties | {"serialNumber":{"type":["string","null"]},"model":{"type":"string"}} |
+      | properties | {"serialNumber":{"type":["string","null"]},"model":{"type":"string"},"color":{"type":"string"}} |
       | required | ["model"] |
       | relations | {"out":{"linked_to":["test-deviceProfiles-linkablegroup"]}} |
 
@@ -35,13 +35,13 @@ Feature: Device Profiles
     Given assetlibrary device profile "TEST-deviceProfiles-profile" of "TEST-deviceProfiles-type" does not exist
     When I create the assetlibrary device profile "TEST-deviceProfiles-profile" of "TEST-deviceProfiles-type" with attributes
       | groups | {"out":{"linked_to": ["/TEST-deviceProfiles-linkableGroup001"]}} |
-      | attributes | {"model": "abc123", "serialNumber": "S123"} |
+      | attributes | {"model": "abc123", "serialNumber": "S123", "color": "magenta"} |
     Then assetlibrary device profile "TEST-deviceProfiles-profile" of "TEST-deviceProfiles-type" exists with attributes
       | groups | {"out":{"linked_to": ["/TEST-deviceProfiles-linkableGroup001"]}} |
-      | attributes | {"model": "abc123", "serialNumber": "S123"} |
+      | attributes | {"model": "abc123", "serialNumber": "S123", "color": "magenta"} |
 
 
-  Scenario: Create a new Device Profile with invalid attribute fails
+  Scenario: Create a new Device Profile with only invalid attributes fails
     Given assetlibrary device profile "TEST-deviceProfiles-profile-invalid" of "TEST-deviceProfiles-type" does not exist
     When I create the assetlibrary device profile "TEST-deviceProfiles-profile-invalid" of "TEST-deviceProfiles-type" with attributes
       | groups | {"out":{"linked_to": ["/TEST-deviceProfiles-linkableGroup001"]}} |
@@ -50,6 +50,15 @@ Feature: Device Profiles
     And assetlibrary device profile "TEST-deviceProfiles-profile-invalid" of "TEST-deviceProfiles-type" does not exist
 
 
+  Scenario: Create a new Device Profile with both valid and invalid attribute fails
+    Given assetlibrary device profile "TEST-deviceProfiles-profile-invalid" of "TEST-deviceProfiles-type" does not exist
+    When I create the assetlibrary device profile "TEST-deviceProfiles-profile-invalid" of "TEST-deviceProfiles-type" with attributes
+      | groups | {"out":{"linked_to": ["/TEST-deviceProfiles-linkableGroup001"]}} |
+      | attributes | {"model": "abc123", "invalid_attribute": "abc123"} |
+    Then it fails with a 400
+    And assetlibrary device profile "TEST-deviceProfiles-profile-invalid" of "TEST-deviceProfiles-type" does not exist
+
+
   Scenario: Create a new Device Profile with invalid group relation fails
     Given assetlibrary device profile "TEST-deviceProfiles-profile-invalid" of "TEST-deviceProfiles-type" does not exist
     When I create the assetlibrary device profile "TEST-deviceProfiles-profile-invalid" of "TEST-deviceProfiles-type" with attributes
@@ -68,7 +77,7 @@ Feature: Device Profiles
     Then device "TEST-deviceProfiles-device001" exists with attributes
       | templateId | test-deviceprofiles-type |
       | groups | {"out":{"linked_to":["/test-deviceprofiles-linkablegroup001"]}} |
-      | attributes | {"model": "abc123", "serialNumber": "S123"} |
+      | attributes | {"model": "abc123", "serialNumber": "S123", "color": "magenta"} |
 
 
   Scenario: Create a Device with attributes, applying a profile appends the profile attributes and groups
@@ -81,7 +90,7 @@ Feature: Device Profiles
     Then device "TEST-deviceProfiles-device002" exists with attributes
       | templateId | test-deviceprofiles-type |
       | groups | {"out":{"linked_to":["/test-deviceprofiles-linkablegroup001"]}} |
-      | attributes | {"model": "abc456", "serialNumber": "S123"} |
+      | attributes | {"model": "abc456", "serialNumber": "S123", "color": "magenta"} |
 
 
   Scenario: Create a Device with groups, applying a profile appends the profile attributes and groups
@@ -90,28 +99,46 @@ Feature: Device Profiles
     And device "TEST-deviceProfiles-device003" does not exist
     When I create device "TEST-deviceProfiles-device003" applying profile "TEST-deviceProfiles-profile" with attributes
       | templateId | TEST-deviceProfiles-type |
-      | attributes | {"model": "abc456"} |
+      | attributes | {"model": "abc456", "color": "purple"} |
       | groups | {"out":{"linked_to":["/test-deviceProfiles-linkablegroup002"]}} |
     Then device "TEST-deviceProfiles-device003" exists with attributes
       | templateId | test-deviceprofiles-type |
       | groups | {"out":{"linked_to":["/test-deviceprofiles-linkablegroup002"]}} |
-      | attributes | {"model": "abc456", "serialNumber": "S123"} |
+      | attributes | {"model": "abc456", "serialNumber": "S123", "color": "purple"} |
 
 
-  Scenario: Applying a profile to an existing device appends the profiles groups and attributes
+  Scenario: Re-applying a profile to an existing device appends the profile's groups and attributes and overwrite existing attributes
     Given device "TEST-deviceProfiles-device003" exists
     When I update device "TEST-deviceProfiles-device003" with attributes
       | attributes | {"serialNumber": null} |
     And I remove device "TEST-deviceProfiles-device003" from group "/TEST-deviceProfiles-linkableGroup002" related via "linked_to"
     Then device "TEST-deviceProfiles-device003" exists with attributes
       | templateId | test-deviceprofiles-type |
       | groups | {} |
-      | attributes | {"model": "abc456"} |
+      | attributes | {"model": "abc456", "color": "purple"} |
     When I update device "TEST-deviceProfiles-device003" applying profile "TEST-deviceProfiles-profile"
     Then device "TEST-deviceProfiles-device003" exists with attributes
       | templateId | test-deviceprofiles-type |
       | groups | {} |
-      | attributes | {"model": "abc123", "serialNumber": "S123"} |
+      | attributes | {"model": "abc123", "serialNumber": "S123", "color": "magenta"} |
+
+
+  Scenario: Update existing device profile attributes
+    Given assetlibrary device profile "TEST-deviceProfiles-profile" of "TEST-deviceProfiles-type" exists
+    When I update the assetlibrary device profile "TEST-deviceProfiles-profile" of "TEST-deviceProfiles-type" with attributes
+      | attributes | {"model": "abc456", "serialNumber": "S456"} |
+    Then assetlibrary device profile "TEST-deviceProfiles-profile" of "TEST-deviceProfiles-type" exists with attributes
+      | groups | {"out":{"linked_to":["/TEST-deviceProfiles-linkableGroup001"]}} |
+      | attributes | {"model": "abc456", "serialNumber": "S456", "color": "magenta"} |
+
+
+  Scenario: Clear an existing device profile attribute while updating another
+    Given assetlibrary device profile "TEST-deviceProfiles-profile" of "TEST-deviceProfiles-type" exists
+    When I update the assetlibrary device profile "TEST-deviceProfiles-profile" of "TEST-deviceProfiles-type" with attributes
+      | attributes | {"serialNumber": null, "color": "pink"} |
+    Then assetlibrary device profile "TEST-deviceProfiles-profile" of "TEST-deviceProfiles-type" exists with attributes
+      | groups | {"out":{"linked_to":["/TEST-deviceProfiles-linkableGroup001"]}} |
+      | attributes | {"model": "abc456", "color": "pink"} |
 
 
   Scenario: Deleting a profile removes it

diff --git a/source/packages/integration-tests/features/assetlibrary/full/devices.feature b/source/packages/integration-tests/features/assetlibrary/full/devices.feature
@@ -112,6 +112,18 @@ Feature: Device lifecycle
       | attributes |  {"model":"B"} |
 
 
+  # Covers https://github.com/aws/aws-connected-device-framework/issues/64
+  Scenario: Clear existing custom device attribute while updating others
+    Given device "TEST-devices-device001" exists
+    When I update device "TEST-devices-device001" with attributes
+      | templateId | TEST-devices-type |
+      | attributes | {"serialNumber":null,"model":"C"} |
+    Then device "TEST-devices-device001" exists with attributes
+      | description | My description |
+      | awsIotThingArn | arn:aws:iot:us-east-1:xxxxxxxxxxxx:thing/test-devices-device001 |
+      | attributes |  {"model":"C"} |
+
+
   Scenario: Clear existing top level device attribute
     Given device "TEST-devices-device001" exists
     When I update device "TEST-devices-device001" with attributes
@@ -120,7 +132,7 @@ Feature: Device lifecycle
     Then device "TEST-devices-device001" exists with attributes
       | description | My description |
       | awsIotThingArn | ___undefined___ |
-      | attributes |  {"model":"B"} |
+      | attributes |  {"model":"C"} |
 
 
   Scenario: Device Ids are unique

diff --git a/source/packages/integration-tests/features/assetlibrary/full/groups.feature b/source/packages/integration-tests/features/assetlibrary/full/groups.feature
@@ -152,6 +152,19 @@ Feature: Group lifecycle
       | parentPath | / |
       | attributes |  {"size":"M"} |
 
+  # Covers https://github.com/aws/aws-connected-device-framework/issues/64
+  Scenario: Clear existing custom group attributes while updating others
+    Given group "/TEST-groups-group001" exists
+    When I update group "/TEST-groups-group001" with attributes
+      | templateId | test-groups-grouptemplate001 |
+      | attributes | {"color":null,"size":"L"} |
+    Then group "/TEST-groups-group001" exists with attributes
+      | templateId | test-groups-grouptemplate001 |
+      | name | TEST-groups-group001 |
+      | description | My group |
+      | parentPath | / |
+      | attributes |  {"size":"L"} |
+
   Scenario: Clear existing top level group attribute
     Given group "/TEST-groups-group001" exists
     When I update group "/TEST-groups-group001" with attributes
@@ -162,7 +175,7 @@ Feature: Group lifecycle
       | name | TEST-groups-group001 |
       | description | ___undefined___ |
       | parentPath | / |
-      | attributes |  {"size":"M"} |
+      | attributes |  {"size":"L"} |
 
   Scenario: Group paths are unique
     Given group "/TEST-groups-group001" exists

diff --git a/source/packages/integration-tests/src/step_definitions/assetlibrary/profiles.steps.ts b/source/packages/integration-tests/src/step_definitions/assetlibrary/profiles.steps.ts
@@ -17,14 +17,15 @@ import {
     DeviceProfile20Resource,
     GroupProfile20Resource,
     ASSETLIBRARY_CLIENT_TYPES,
+    DeviceProfileResource,
 } from '@cdf/assetlibrary-client/dist';
 import stringify from 'json-stable-stringify';
 import { fail } from 'assert';
 
 import chai_string = require('chai-string');
 import {expect, use} from 'chai';
 use(chai_string);
-import { RESPONSE_STATUS, AUTHORIZATION_TOKEN } from '../common/common.steps';
+import { RESPONSE_STATUS, AUTHORIZATION_TOKEN, buildModel } from '../common/common.steps';
 import {container} from '../../di/inversify.config';
 import {Dictionary} from '../../../../libraries/core/lambda-invoke/src';
 /*
@@ -78,25 +79,7 @@ Given('assetlibrary {word} profile {string} of {string} exists', async function
 });
 
 When('I create the assetlibrary {word} profile {string} of {string} with attributes', async function (category:string, profileId:string, templateId:string, data:DataTable) {
-    const d = data.rowsHash();
-
-    const profile = {
-        profileId,
-        templateId
-    };
-
-    Object.keys(d).forEach( key => {
-        const value = d[key];
-        if (value.startsWith('{') || value.startsWith('[')) {
-            profile[key] = JSON.parse(d[key]);
-        } else if (value==='___null___') {
-            profile[key] = null;
-        } else if (value==='___undefined___') {
-            delete profile[key];
-        } else {
-            profile[key] = d[key];
-        }
-    });
+    const profile = buildModel<DeviceProfileResource>(data, {profileId, templateId});
 
     try {
         if (isDevice(category)) {
@@ -109,6 +92,20 @@ When('I create the assetlibrary {word} profile {string} of {string} with attribu
     }
 });
 
+When('I update the assetlibrary {word} profile {string} of {string} with attributes', async function (category:string, profileId:string, templateId:string, data:DataTable) {
+    const profile = buildModel<DeviceProfileResource>(data, {profileId, templateId});
+
+    try {
+        if (isDevice(category)) {
+            await profileService.updateDeviceProfile(templateId, profileId, profile, getAdditionalHeaders(this));
+        } else if (isGroup(category)) {
+            await profileService.updateGroupProfile(templateId, profileId, profile, getAdditionalHeaders(this));
+        }
+    } catch (err) {
+        this[RESPONSE_STATUS]=err.status;
+    }
+});
+
 When('I delete assetlibrary {word} profile {string} of {string}', async function (category:string, profileId:string, templateId:string) {
     try {
         if (isDevice(category)) {

diff --git a/source/packages/integration-tests/src/step_definitions/common/common.steps.ts b/source/packages/integration-tests/src/step_definitions/common/common.steps.ts
@@ -120,14 +120,13 @@ export async function streamToString(stream: Readable): Promise<string> {
     });
 }
 
-export function buildModel<T>(data: DataTable): T {
+export function buildModel<T>(data: DataTable, initial: Record<string,string> = {}) : T {
     if (data === undefined) {
         return undefined;
     }
 
     const d = data.rowsHash();
-
-    const resource = {} as T;
+    const resource = { ...initial } as unknown as T;
 
     Object.keys(d).forEach(key => {
         const value = replaceTokens(d[key]);

diff --git a/source/packages/services/assetlibrary/src/devices/devices.full.dao.ts b/source/packages/services/assetlibrary/src/devices/devices.full.dao.ts
@@ -258,25 +258,29 @@ export class DevicesDaoFull extends BaseDaoFull {
         const conn = super.getConnection();
         try {
             const traversal = conn.traversal.V(id);
-
-            for (const key of Object.keys(n.attributes)) {
-                const val = n.attributes[key];
-                if (val!==undefined) {
-                    if (val===null) {
-                        traversal.properties(key).drop();
+            // drop() step terminates a traversal, process all drops as part of a final union step 
+            const dropTraversals: process.GraphTraversal[] = [];
+
+            for (const [key, val] of Object.entries(n.attributes)) {
+                if (val !== undefined) {
+                    if (val === null) {
+                        dropTraversals.push(__.properties(key));
                     } else {
                         traversal.property(process.cardinality.single, key, val);
                     }
                 }
             }
+            if (dropTraversals.length > 0) {
+                traversal.local(__.union(...dropTraversals)).drop();
+            }
 
             await traversal.iterate();
         } finally {
             await conn.close();
         }
 
         logger.debug(`devices.full.dao update: exit:`);
-
+        
     }
 
     public async delete(deviceId: string): Promise<void> {

diff --git a/source/packages/services/assetlibrary/src/groups/groups.full.dao.ts b/source/packages/services/assetlibrary/src/groups/groups.full.dao.ts
@@ -179,19 +179,23 @@ export class GroupsDaoFull extends BaseDaoFull {
         const conn = super.getConnection();
         try {
             const traversal = conn.traversal.V(id);
-
-            for (const key of Object.keys(n.attributes)) {
-                const val = n.attributes[key];
-                if (val!==undefined) {
-                    if (val===null) {
-                        traversal.properties(key).drop();
+            // drop() step terminates a traversal, process all drops as part of a final union step 
+            const dropTraversals: process.GraphTraversal[] = [];
+
+            for (const [key, val] of Object.entries(n.attributes)) {
+                if (val !== undefined) {
+                    if (val === null) {
+                        dropTraversals.push(__.properties(key));
                     } else {
                         traversal.property(process.cardinality.single, key, val);
                     }
                 }
             }
-
-            await traversal.next();
+            if (dropTraversals.length > 0) {
+                traversal.local(__.union(...dropTraversals)).drop();
+            }
+
+            await traversal.iterate();
         } finally {
             await conn.close();
         }

diff --git a/source/packages/services/assetlibrary/src/profiles/profiles.full.dao.ts b/source/packages/services/assetlibrary/src/profiles/profiles.full.dao.ts
@@ -117,17 +117,21 @@ export class ProfilesDaoFull extends BaseDaoFull {
         const conn = super.getConnection();
         try {
             const traversal = conn.traversal.V(id);
-
-            for (const key of Object.keys(n.attributes)) {
-                const val = n.attributes[key];
-                if (val!==undefined) {
-                    if (val===null) {
-                        traversal.properties(key).drop();
+            // drop() step terminates a traversal, process all drops as part of a final union step 
+            const dropTraversals: process.GraphTraversal[] = [];
+
+            for (const [key, val] of Object.entries(n.attributes)) {
+                if (val !== undefined) {
+                    if (val === null) {
+                        dropTraversals.push(__.properties(key));
                     } else {
                         traversal.property(process.cardinality.single, key, val);
                     }
                 }
             }
+            if (dropTraversals.length > 0) {
+                traversal.local(__.union(...dropTraversals)).drop();
+            }
 
             await traversal.iterate();
         } finally {