Refactor host set fragment arrays (#1464)

* refactor: 💡 change host_ids to a simple attribute

* refactor: 💡 manually manage host set array model attributes

* refactor: 💡 Fix host set IDs usage

* style: 💄 Fix comment and add braces for if statements

* refactor: 💡 Add back in empty array for normalizer

* refactor: 💡 Move attributes to generated model

* refactor: 💡 Update array actions when creating new host sets

* test: 💍 Add a few create tests for azure and aws host sets

* test: 💍 Add update tests for azure and aws host sets

Co-authored-by: Zhihe Li <zhihe.li@hashicorp.com>

addons/api/addon/generated/models/host-set.js

@@ -63,6 +63,28 @@ export default class GeneratedHostSetModel extends BaseModel {
   })
   sync_interval_seconds;
 
+  /**
+   * Host IDs are read-only under normal circumstances.  But these can
+   * be persisted via a dedicated call to `addHosts()`.
+   */
+  @attr({
+    readOnly: true,
+    emptyArrayIfMissing: true,
+  })
+  host_ids;
+
+  @attr('string-array', {
+    emptyArrayIfMissing: true,
+  })
+  preferred_endpoints;
+
+  // AWS specific
+  @attr('string-array', {
+    isNestedAttribute: true,
+    emptyArrayIfMissing: true,
+  })
+  filters;
+
   // Azure specific
   @attr('string', {
     description: '',

addons/api/addon/models/host-set.js

@@ -1,5 +1,4 @@
 import GeneratedHostSetModel from '../generated/models/host-set';
-import { fragmentArray } from 'ember-data-model-fragments/attributes';
 import { types } from './host-catalog';
 
 export default class HostSetModel extends GeneratedHostSetModel {
@@ -67,28 +66,6 @@ export default class HostSetModel extends GeneratedHostSetModel {
     }
   }
 
-  /**
-   * Host IDs are read-only under normal circumstances.  But these can
-   * be persisted via a dedicated call to `addHosts()`.
-   */
-  @fragmentArray('fragment-string', {
-    readOnly: true,
-    emptyArrayIfMissing: true,
-  })
-  host_ids;
-
-  @fragmentArray('fragment-string', {
-    emptyArrayIfMissing: true,
-  })
-  preferred_endpoints;
-
-  // AWS specific
-  @fragmentArray('fragment-string', {
-    emptyArrayIfMissing: true,
-    isNestedAttribute: true,
-  })
-  filters;
-
   // =methods
 
   /**

addons/api/tests/unit/models/host-set-test.js

@@ -24,7 +24,7 @@ module('Unit | Model | host set', function (hooks) {
         attributes: {
           name: 'Host Set',
           description: 'Description',
-          host_ids: [{ value: '1' }, { value: '3' }],
+          host_ids: ['1', '2'],
           version: 1,
           scope: {
             scope_id: 'o_1',
@@ -55,7 +55,7 @@ module('Unit | Model | host set', function (hooks) {
         attributes: {
           name: 'Host Set',
           description: 'Description',
-          host_ids: [{ value: '1' }, { value: '3' }],
+          host_ids: ['1', '3'],
           version: 1,
           scope: {
             scope_id: 'o_1',
@@ -86,7 +86,7 @@ module('Unit | Model | host set', function (hooks) {
         attributes: {
           name: 'Host Set',
           description: 'Description',
-          host_ids: [{ value: '1' }, { value: '3' }],
+          host_ids: ['1', '3'],
           version: 1,
           scope: {
             scope_id: 'o_1',
@@ -117,7 +117,7 @@ module('Unit | Model | host set', function (hooks) {
         attributes: {
           name: 'Host Set',
           description: 'Description',
-          host_ids: [{ value: '1' }, { value: '3' }],
+          host_ids: ['1', '3'],
           version: 1,
           scope: {
             scope_id: 'o_1',

addons/api/tests/unit/serializers/host-set-test.js

@@ -123,7 +123,7 @@ module('Unit | Serializer | host set', function (hooks) {
           type: 'static',
           authorized_actions: [],
           name: 'Host Set 1',
-          host_ids: [{ value: '1' }, { value: '2' }, { value: '3' }],
+          host_ids: ['1', '2', '3'],
           preferred_endpoints: [
             { value: 'endpoint 1' },
             { value: 'endpoint 2' },
@@ -135,39 +135,6 @@ module('Unit | Serializer | host set', function (hooks) {
     });
   });
 
-  test('it normalizes missing host_ids to empty array', function (assert) {
-    assert.expect(1);
-    const store = this.owner.lookup('service:store');
-    const serializer = store.serializerFor('host-set');
-    const hostSet = store.createRecord('host-set').constructor;
-    const payload = {
-      id: '1',
-      name: 'Host Set 1',
-      scope: { id: 'o_123' },
-    };
-    const normalized = serializer.normalizeSingleResponse(
-      store,
-      hostSet,
-      payload
-    );
-    assert.deepEqual(normalized, {
-      included: [],
-      data: {
-        id: '1',
-        type: 'host-set',
-        attributes: {
-          authorized_actions: [],
-          name: 'Host Set 1',
-          scope: { id: 'o_123', scope_id: 'o_123' },
-          host_ids: [],
-          preferred_endpoints: [],
-          filters: [],
-        },
-        relationships: {},
-      },
-    });
-  });
-
   test('it normalizes missing array fields to empty array', function (assert) {
     assert.expect(1);
     const store = this.owner.lookup('service:store');

addons/api/tests/unit/transforms/string-array-test.js

@@ -15,6 +15,13 @@ module('Unit | Transform | string array', function (hooks) {
     ]);
   });
 
+  test('it deserializes null to an empty array', function (assert) {
+    assert.expect(1);
+    const transform = this.owner.lookup('transform:string-array');
+    const deserialized = transform.deserialize(null);
+    assert.deepEqual(deserialized, []);
+  });
+
   test('it serializes an array of `{value}` to an array of strings', function (assert) {
     assert.expect(1);
     const transform = this.owner.lookup('transform:string-array');
@@ -25,4 +32,11 @@ module('Unit | Transform | string array', function (hooks) {
     ]);
     assert.deepEqual(serialized, ['a', 'bee', 'Hello World']);
   });
+
+  test('it serializes null to an empty array', function (assert) {
+    assert.expect(1);
+    const transform = this.owner.lookup('transform:string-array');
+    const serialized = transform.serialize(null);
+    assert.deepEqual(serialized, []);
+  });
 });

addons/rose/addon/components/rose/form/index.js

@@ -77,6 +77,7 @@ export default class RoseFormComponent extends Component {
   @action
   enableEdit() {
     this.isEditable = true;
+    if (this.args.edit) this.args.edit();
   }
 
   /**

ui/admin/app/components/form/host-set/add-hosts/index.js

@@ -1,5 +1,5 @@
 import Component from '@glimmer/component';
-import { computed, action } from '@ember/object';
+import { action, computed } from '@ember/object';
 import { A } from '@ember/array';
 
 export default class FormHostSetAddHostsIndexComponent extends Component {
@@ -27,8 +27,9 @@ export default class FormHostSetAddHostsIndexComponent extends Component {
    */
   @computed('args.{model,model.host_ids,hosts}')
   get filteredHosts() {
-    const currentHostIDs = this.args.model.host_ids.map((host) => host.value);
+    return this.args.hosts.filter(
-    return this.args.hosts.filter(({ id }) => !currentHostIDs.includes(id));
+      ({ id }) => !this.args.model.host_ids.includes(id)
+    );
   }
 
   // =actions

ui/admin/app/components/form/host-set/aws/index.hbs

@@ -1,4 +1,5 @@
 <Rose::Form
+  @edit={{@edit}}
   @onSubmit={{@submit}}
   @cancel={{@cancel}}
   @disabled={{@model.isSaving}}
@@ -100,6 +101,7 @@
               <row.cell>
                 <form.input
                   @type='text'
+                  @name='preferred_endpoints'
                   @value={{endpoint.value}}
                   @error={{@model.errors.preferred_endpoints}}
                   @contextual={{true}}
@@ -131,6 +133,7 @@
             <row.cell>
               <form.input
                 @type='text'
+                @name='preferred_endpoints'
                 @value={{@model.preferred_endpoint}}
                 placeholder='cidr:2001:4860:4860::8888/32'
               />
@@ -186,6 +189,7 @@
               <row.cell>
                 <form.input
                   @type='text'
+                  @name='filters'
                   @value={{filter.value}}
                   @error={{@model.errors.filters}}
                   @contextual={{true}}
@@ -214,7 +218,11 @@
 
           <body.row as |row|>
             <row.cell>
-              <form.input @type='text' @value={{@model.filter}} />
+              <form.input
+                @type='text'
+                @name='filters'
+                @value={{@model.filter}}
+              />
             </row.cell>
             <row.cell @shrink={{true}}>
               <Rose::Button

ui/admin/app/components/form/host-set/azure/index.hbs

@@ -1,4 +1,5 @@
 <Rose::Form
+  @edit={{@edit}}
   @onSubmit={{@submit}}
   @cancel={{@cancel}}
   @disabled={{@model.isSaving}}
@@ -100,6 +101,7 @@
               <row.cell>
                 <form.input
                   @type='text'
+                  @name='preferred_endpoints'
                   @value={{endpoint.value}}
                   @error={{@model.errors.preferred_endpoints}}
                   @contextual={{true}}
@@ -131,6 +133,7 @@
             <row.cell>
               <form.input
                 @type='text'
+                @name='preferred_endpoints'
                 @value={{@model.preferred_endpoint}}
                 placeholder='cidr:2001:4860:4860::8888/32'
               />

ui/admin/app/components/form/host-set/index.hbs

@@ -1,6 +1,7 @@
 {{component
   (concat 'form/host-set/' @model.compositeType)
   model=@model
+  edit=@edit
   submit=@submit
   cancel=@cancel
   removeItemByIndex=@removeItemByIndex

ui/admin/app/routes/scopes/scope/host-catalogs/host-catalog/host-sets/host-set.js

@@ -1,6 +1,7 @@
 import Route from '@ember/routing/route';
 import { action } from '@ember/object';
 import { inject as service } from '@ember/service';
+import { copy } from 'ember-copy';
 
 export default class ScopesScopeHostCatalogsHostCatalogHostSetsHostSetRoute extends Route {
   // =services
@@ -20,16 +21,35 @@ export default class ScopesScopeHostCatalogsHostCatalogHostSetsHostSetRoute exte
   }
 
   /**
-   * Adds a string item to array `property` on the passed `filter`.
+   * Copies the contents of string array fields in order to force the instance
+   * into a dirty state.  This ensures that `model.rollbackAttributes()` reverts
+   * to the original expected array.
+   *
+   * The deep copy implemented here is required to ensure that both the
+   * array itself as well as its members are all new.
+   *
+   * @param {hostSetModel} hostSet
+   */
+  @action
+  edit(hostSet) {
+    if (hostSet.preferred_endpoints) {
+      hostSet.preferred_endpoints = copy(hostSet.preferred_endpoints, true);
+    }
+    if (hostSet.filters) {
+      hostSet.filters = copy(hostSet.filters, true);
+    }
+  }
 
+  /**
+   * Adds a string item `{ value }` to array `property` on passed `hostSet`.
    * @param {hostSetModel} hostSet
    * @param {string} property
    * @param {string} value
    */
   @action
   async addStringItem(hostSet, property, value) {
-    const array = hostSet.get(property);
+    const array = [...hostSet.get(property), { value }];
-    array.addObject({ value });
+    hostSet.set(property, array);
   }
 
   /**
@@ -41,7 +61,7 @@ export default class ScopesScopeHostCatalogsHostCatalogHostSetsHostSetRoute exte
    */
   @action
   async removeItemByIndex(hostSet, property, index) {
-    const array = hostSet.get(property);
+    const array = hostSet.get(property).filter((item, i) => i !== index);
-    array.removeAt(index);
+    hostSet.set(property, array);
   }
 }

ui/admin/app/routes/scopes/scope/host-catalogs/host-catalog/host-sets/host-set/hosts.js

@@ -25,8 +25,8 @@ export default class ScopesScopeHostCatalogsHostCatalogHostSetsHostSetHostsRoute
     return hash({
       hostSet,
       hosts: all(
-        hostSet.host_ids.map((host) =>
+        hostSet.host_ids.map((hostID) =>
-          this.store.findRecord('host', host.value, { reload: true })
+          this.store.findRecord('host', hostID, { reload: true })
         )
       ),
     });

ui/admin/app/routes/scopes/scope/host-catalogs/host-catalog/host-sets/new.js

@@ -32,8 +32,8 @@ export default class ScopesScopeHostCatalogsHostCatalogHostSetsNewRoute extends
    */
   @action
   async addStringItem(hostSet, property, value) {
-    const array = hostSet.get(property);
+    const array = [...(hostSet.get(property) ?? []), { value }];
-    array.addObject({ value });
+    hostSet.set(property, array);
   }
   /**
    * Removes an item from array `property` at `index` on the
@@ -44,7 +44,7 @@ export default class ScopesScopeHostCatalogsHostCatalogHostSetsNewRoute extends
    */
   @action
   async removeItemByIndex(hostSet, property, index) {
-    const array = hostSet.get(property);
+    const array = hostSet.get(property).filter((item, i) => i !== index);
-    array.removeAt(index);
+    hostSet.set(property, array);
   }
 }

ui/admin/app/templates/scopes/scope/host-catalogs/host-catalog/host-sets/host-set/index.hbs

@@ -20,6 +20,7 @@
   <page.body>
     <Form::HostSet
       @model={{@model}}
+      @edit={{route-action 'edit' @model}}
       @submit={{route-action 'save' @model}}
       @cancel={{route-action 'cancel' @model}}
       @removeItemByIndex={{route-action 'removeItemByIndex' @model}}

ui/admin/tests/acceptance/host-catalogs/host-sets/create-test.js

@@ -79,6 +79,67 @@ module('Acceptance | host-catalogs | host sets | create', function (hooks) {
     assert.strictEqual(getHostSetCount(), count + 1);
   });
 
+  test('can create new aws host set', async function (assert) {
+    assert.expect(4);
+    instances.hostCatalog = this.server.create('host-catalog', {
+      scope: instances.scopes.project,
+      type: 'plugin',
+      plugin: {
+        id: `plugin-id-1`,
+        name: 'aws',
+      },
+    });
+
+    const count = getHostSetCount();
+    await visit(
+      `${urls.hostCatalogs}/${instances.hostCatalog.id}/host-sets/new`
+    );
+    const name = 'aws host set';
+    await fillIn('[name="name"]', name);
+    await fillIn('[name="preferred_endpoints"]', 'endpoint');
+    await click('form fieldset:nth-of-type(2) [title="Add"]');
+    await fillIn('[name="filters"]', 'filter');
+    await click('form fieldset:nth-of-type(3) [title="Add"]');
+    await fillIn('[name="sync_interval_seconds"]', 10);
+    await click('[type="submit"]');
+
+    assert.strictEqual(getHostSetCount(), count + 1);
+    const hostSet = this.server.schema.hostSets.findBy({ name });
+    assert.deepEqual(hostSet.preferredEndpoints, ['endpoint']);
+    assert.deepEqual(hostSet.attributes.filters, ['filter']);
+    assert.deepEqual(hostSet.syncIntervalSeconds, 10);
+  });
+
+  test('can create new azure host set', async function (assert) {
+    assert.expect(4);
+    instances.hostCatalog = this.server.create('host-catalog', {
+      scope: instances.scopes.project,
+      type: 'plugin',
+      plugin: {
+        id: `plugin-id-1`,
+        name: 'azure',
+      },
+    });
+
+    const count = getHostSetCount();
+    await visit(
+      `${urls.hostCatalogs}/${instances.hostCatalog.id}/host-sets/new`
+    );
+    const name = 'azure host set';
+    await fillIn('[name="name"]', name);
+    await fillIn('[name="preferred_endpoints"]', 'endpoint');
+    await click('form fieldset:nth-of-type(2) [title="Add"]');
+    await fillIn('[name="filter"]', 'filter');
+    await fillIn('[name="sync_interval_seconds"]', 10);
+    await click('[type="submit"]');
+
+    assert.strictEqual(getHostSetCount(), count + 1);
+    const hostSet = this.server.schema.hostSets.findBy({ name });
+    assert.deepEqual(hostSet.preferredEndpoints, ['endpoint']);
+    assert.deepEqual(hostSet.attributes.filter, 'filter');
+    assert.deepEqual(hostSet.syncIntervalSeconds, 10);
+  });
+
   test('Users cannot create a new host set without proper authorization', async function (assert) {
     assert.expect(2);
     instances.hostCatalog.authorized_collection_actions['host-sets'] = [];

ui/admin/tests/acceptance/host-catalogs/host-sets/update-test.js

@@ -1,5 +1,12 @@
 import { module, test } from 'qunit';
-import { visit, currentURL, find, click, fillIn } from '@ember/test-helpers';
+import {
+  visit,
+  currentURL,
+  find,
+  findAll,
+  click,
+  fillIn,
+} from '@ember/test-helpers';
 import { setupApplicationTest } from 'ember-qunit';
 import setupMirage from 'ember-cli-mirage/test-support/setup-mirage';
 import { Response } from 'miragejs';
@@ -100,7 +107,7 @@ module('Acceptance | host-catalogs | host sets | update', function (hooks) {
     );
   });
 
-  test('can save changes to existing host', async function (assert) {
+  test('can save changes to existing host-set', async function (assert) {
     assert.expect(3);
     assert.notEqual(instances.hostSet.name, 'random string');
     await visit(urls.hostSet);
@@ -109,11 +116,106 @@ module('Acceptance | host-catalogs | host sets | update', function (hooks) {
     await click('.rose-form-actions [type="submit"]');
     assert.strictEqual(currentURL(), urls.hostSet);
     assert.strictEqual(
-      this.server.schema.hostSets.all().models[0].name,
+      this.server.schema.hostSets.first().name,
       'random string'
     );
   });
 
+  test('can save changes to an existing aws host-set', async function (assert) {
+    assert.expect(5);
+    instances.hostCatalog = this.server.create('host-catalog', {
+      scope: instances.scopes.project,
+      type: 'plugin',
+      plugin: {
+        id: `plugin-id-1`,
+        name: 'aws',
+      },
+    });
+    instances.hostSet = this.server.create('host-set', {
+      scope: instances.scopes.project,
+      hostCatalog: instances.hostCatalog,
+    });
+    urls.hostSet = `${urls.hostCatalogs}/${instances.hostCatalog.id}/host-sets/${instances.hostSet.id}`;
+    await visit(urls.hostSet);
+
+    await click(
+      'form .rose-form-actions [type="button"]',
+      'Activate edit mode'
+    );
+
+    const name = 'aws host set';
+    await fillIn('[name="name"]', name);
+    // Remove all the preferred endpoints
+    await Promise.all(
+      findAll('form fieldset:nth-of-type(2) [title="Remove"]').map((element) =>
+        click(element)
+      )
+    );
+    await fillIn('[name="preferred_endpoints"]', 'endpoint');
+    await click('form fieldset:nth-of-type(2) [title="Add"]');
+    // Remove all the filters
+    await Promise.all(
+      findAll('form fieldset:nth-of-type(3) [title="Remove"]').map((element) =>
+        click(element)
+      )
+    );
+    await fillIn('[name="filters"]', 'filter');
+    await click('form fieldset:nth-of-type(3) [title="Add"]');
+    await fillIn('[name="sync_interval_seconds"]', 10);
+    await click('.rose-form-actions [type="submit"]');
+
+    assert.strictEqual(currentURL(), urls.hostSet);
+    const hostSet = this.server.schema.hostSets.findBy({ name });
+    assert.strictEqual(hostSet.name, name);
+    assert.deepEqual(hostSet.preferredEndpoints, ['endpoint']);
+    assert.deepEqual(hostSet.attributes.filters, ['filter']);
+    assert.deepEqual(hostSet.syncIntervalSeconds, 10);
+  });
+
+  test('can save changes to an existing azure host-set', async function (assert) {
+    assert.expect(5);
+    instances.hostCatalog = this.server.create('host-catalog', {
+      scope: instances.scopes.project,
+      type: 'plugin',
+      plugin: {
+        id: `plugin-id-1`,
+        name: 'azure',
+      },
+    });
+    instances.hostSet = this.server.create('host-set', {
+      scope: instances.scopes.project,
+      hostCatalog: instances.hostCatalog,
+    });
+    urls.hostSet = `${urls.hostCatalogs}/${instances.hostCatalog.id}/host-sets/${instances.hostSet.id}`;
+    await visit(urls.hostSet);
+
+    await click(
+      'form .rose-form-actions [type="button"]',
+      'Activate edit mode'
+    );
+
+    const name = 'azure host set';
+    await fillIn('[name="name"]', name);
+    // Remove all the preferred endpoints
+    await Promise.all(
+      findAll('form fieldset:nth-of-type(2) [title="Remove"]').map((element) =>
+        click(element)
+      )
+    );
+    await fillIn('[name="preferred_endpoints"]', 'endpoint');
+    await click('form fieldset:nth-of-type(2) [title="Add"]');
+    await fillIn('[name="filter"]', 'filter');
+    await fillIn('[name="sync_interval_seconds"]', 10);
+    await click('.rose-form-actions [type="submit"]');
+
+    assert.strictEqual(currentURL(), urls.hostSet);
+    const hostSet = this.server.schema.hostSets.findBy({ name });
+    assert.strictEqual(hostSet.name, name);
+    assert.deepEqual(hostSet.preferredEndpoints, ['endpoint']);
+    assert.deepEqual(hostSet.attributes.filter, 'filter');
+    assert.deepEqual(hostSet.syncIntervalSeconds, 10);
+  });
+
   test('cannot make changes to an existing host without proper authorization', async function (assert) {
     assert.expect(1);
     instances.hostSet.authorized_actions =
@@ -122,7 +224,7 @@ module('Acceptance | host-catalogs | host sets | update', function (hooks) {
     assert.notOk(find('.rose-layout-page-actions .rose-button-secondary'));
   });
 
-  test('can cancel changes to existing host', async function (assert) {
+  test('can cancel changes to existing host-set', async function (assert) {
     assert.expect(2);
     await visit(urls.hostSet);
     await click('form [type="button"]', 'Activate edit mode');
@@ -183,7 +285,7 @@ module('Acceptance | host-catalogs | host sets | update', function (hooks) {
       await click('.rose-dialog-footer button:first-child');
       assert.strictEqual(currentURL(), urls.hostSets);
       assert.notEqual(
-        this.server.schema.hostSets.all().models[0].name,
+        this.server.schema.hostSets.first().name,
         'random string'
       );
     }

ui/desktop/app/routes/scopes/scope/projects/targets/target/hosts.js

@@ -27,9 +27,7 @@ export default class ScopesScopeProjectsTargetsTargetHostsRoute extends Route {
     );
 
     // Extract host ids from all host sets
-    const hostIds = hostSets.map(({ host_ids }) =>
+    const hostIds = hostSets.map(({ host_ids }) => host_ids);
-      host_ids.content.map((hostId) => hostId.value)
-    );
 
     // Load unique hosts
     const uniqueHostIds = new Set(hostIds.flat());