From 27c74f7141eebfd49d172c0b7513beea22bb2fc2 Mon Sep 17 00:00:00 2001 From: John Cowen Date: Wed, 9 Dec 2020 13:08:30 +0000 Subject: [PATCH] ui: New search/sort/filtering bar for Node > ServiceInstances (#9326) * Model layer changes to turn Node:ServiceInstances into hasMany We tried to make something that feels a little like ember-data yet not leave our approach of re-shaping the JSON directly from the response. 1. We added transformHasManyResponse for re-shaping JSON for hasMany relationships. we avoided the normalize word as ember-data serialize methods usually return something JSON:API shaped and we distinctly don't want to do that. Transform was the best word we could think of. 2. The integration tests across all of our models here feel very much like those types of tests that aren't really testing much, or assert too much to an extent that they get in the way rather than be of any use. I'd very much like to move a lot of this to unit tests. Currently most of the fingerprinting functionality is unit tested and these integration tests were originally to give confidence that IDs and related properties were being added correctly. 3. We've added a hasMany relationship, but not the corresponding belongsTo - yet at least. We don't require the belongsTo right now, and if we do we can add it later. * Integrate ServiceInstance search bar for Node:ServiceInstances * Hide Node.Meta when on the Node:ServiceINstance page We use a little string replace hack here for a human-like label, this is soon to be replaced with proper i10n replacement * Always ensure that a Namespace is set, and add comment explaining --- ui/packages/consul-ui/app/adapters/http.js | 21 ++-- .../consul/service-instance/list/index.hbs | 30 ++---- .../service-instance/search-bar/index.hbs | 10 +- .../app/controllers/dc/nodes/show/services.js | 28 ------ ui/packages/consul-ui/app/models/node.js | 4 +- .../app/routes/dc/nodes/show/services.js | 12 ++- .../app/routes/dc/services/show/instances.js | 5 +- ui/packages/consul-ui/app/serializers/http.js | 8 ++ ui/packages/consul-ui/app/serializers/node.js | 57 ++++++++++- .../app/serializers/service-instance.js | 72 +++++++++++++- .../app/templates/dc/nodes/show/services.hbs | 62 +++++++++--- .../templates/dc/services/show/instances.hbs | 4 +- .../app/utils/create-fingerprinter.js | 20 ++-- .../components/catalog-filter.feature | 99 ------------------- .../integration/serializers/node-test.js | 35 ++++--- 15 files changed, 250 insertions(+), 217 deletions(-) delete mode 100644 ui/packages/consul-ui/app/controllers/dc/nodes/show/services.js delete mode 100644 ui/packages/consul-ui/tests/acceptance/components/catalog-filter.feature diff --git a/ui/packages/consul-ui/app/adapters/http.js b/ui/packages/consul-ui/app/adapters/http.js index 3e0f38fd42..3794e27a54 100644 --- a/ui/packages/consul-ui/app/adapters/http.js +++ b/ui/packages/consul-ui/app/adapters/http.js @@ -17,11 +17,11 @@ import AdapterError, { // the naming of things (serialized vs query etc) const read = function(adapter, modelName, type, query = {}) { return adapter.rpc( - function(adapter, request, query) { - return adapter[`requestFor${type}`](request, query); + function(adapter, ...rest) { + return adapter[`requestFor${type}`](...rest); }, - function(serializer, respond, query) { - return serializer[`respondFor${type}`](respond, query); + function(serializer, ...rest) { + return serializer[`respondFor${type}`](...rest); }, query, modelName @@ -29,11 +29,11 @@ const read = function(adapter, modelName, type, query = {}) { }; const write = function(adapter, modelName, type, snapshot) { return adapter.rpc( - function(adapter, request, serialized, unserialized) { - return adapter[`requestFor${type}`](request, serialized, unserialized); + function(adapter, ...rest) { + return adapter[`requestFor${type}`](...rest); }, - function(serializer, respond, serialized, unserialized) { - return serializer[`respondFor${type}`](respond, serialized, unserialized); + function(serializer, ...rest) { + return serializer[`respondFor${type}`](...rest); }, snapshot, modelName @@ -49,6 +49,7 @@ export default class HttpAdapter extends Adapter { let unserialized, serialized; const serializer = store.serializerFor(modelName); + const modelClass = store.modelFor(modelName); // workable way to decide whether this is a snapshot // essentially 'is attributable'. // Snapshot is private so we can't do instanceof here @@ -66,14 +67,14 @@ export default class HttpAdapter extends Adapter { return client .request(function(request) { - return req(adapter, request, serialized, unserialized); + return req(adapter, request, serialized, unserialized, modelClass); }) .catch(function(e) { return adapter.error(e); }) .then(function(respond) { // TODO: When HTTPAdapter:responder changes, this will also need to change - return resp(serializer, respond, serialized, unserialized); + return resp(serializer, respond, serialized, unserialized, modelClass); }); // TODO: Potentially add specific serializer errors here // .catch(function(e) { diff --git a/ui/packages/consul-ui/app/components/consul/service-instance/list/index.hbs b/ui/packages/consul-ui/app/components/consul/service-instance/list/index.hbs index 603b29c092..a47ebfa471 100644 --- a/ui/packages/consul-ui/app/components/consul/service-instance/list/index.hbs +++ b/ui/packages/consul-ui/app/components/consul/service-instance/list/index.hbs @@ -5,8 +5,8 @@ as |item index|> {{#if (eq @routeName "dc.services.show")}} - - {{item.ID}} + + {{item.Service.ID}} {{else}} @@ -15,9 +15,9 @@ as |item index|> {{/if}} - {{#if @checks}} - - + {{#if @node}} + + {{else}} @@ -35,7 +35,7 @@ as |item index|> {{/if}} -{{#if (gt item.Node.Node.length 0)}} +{{#if (not @node)}}
@@ -63,24 +63,6 @@ as |item index|>
{{/if}} -{{#if (and @checks item.Port)}} -
-
- Port -
-
- - :{{item.Port}} -
-
-{{/if}} -{{#if checks}} - -{{else}} -{{/if}} \ No newline at end of file diff --git a/ui/packages/consul-ui/app/components/consul/service-instance/search-bar/index.hbs b/ui/packages/consul-ui/app/components/consul/service-instance/search-bar/index.hbs index d4d44b52b7..71fb5c27e7 100644 --- a/ui/packages/consul-ui/app/components/consul/service-instance/search-bar/index.hbs +++ b/ui/packages/consul-ui/app/components/consul/service-instance/search-bar/index.hbs @@ -21,13 +21,9 @@ {{#let components.Optgroup components.Option as |Optgroup Option|}} - - - - - - - + {{#each @searchproperties as |prop|}} + + {{/each}} {{/let}} diff --git a/ui/packages/consul-ui/app/controllers/dc/nodes/show/services.js b/ui/packages/consul-ui/app/controllers/dc/nodes/show/services.js deleted file mode 100644 index 0fb98b410c..0000000000 --- a/ui/packages/consul-ui/app/controllers/dc/nodes/show/services.js +++ /dev/null @@ -1,28 +0,0 @@ -import Controller from '@ember/controller'; -import { get, computed } from '@ember/object'; - -export default class ServicesController extends Controller { - queryParams = { - search: { - as: 'filter', - replace: true, - }, - }; - - @computed('item.Checks.[]') - get checks() { - const checks = {}; - get(this, 'item.Checks') - .filter(item => { - return item.ServiceID !== ''; - }) - .forEach(item => { - if (typeof checks[item.ServiceID] === 'undefined') { - checks[item.ServiceID] = []; - } - checks[item.ServiceID].push(item); - }); - - return checks; - } -} diff --git a/ui/packages/consul-ui/app/models/node.js b/ui/packages/consul-ui/app/models/node.js index 8d2892abd5..2c77cd9798 100644 --- a/ui/packages/consul-ui/app/models/node.js +++ b/ui/packages/consul-ui/app/models/node.js @@ -1,4 +1,4 @@ -import Model, { attr } from '@ember-data/model'; +import Model, { attr, hasMany } from '@ember-data/model'; import { computed } from '@ember/object'; import { fragmentArray } from 'ember-data-model-fragments/attributes'; @@ -18,7 +18,7 @@ export default class Node extends Model { @attr() meta; // {} @attr() Meta; // {} @attr() TaggedAddresses; // {lan, wan} - @attr() Services; // ServiceInstances[] + @hasMany('service-instance') Services; // TODO: Rename to ServiceInstances @fragmentArray('health-check') Checks; @computed('Checks.[]', 'ChecksCritical', 'ChecksPassing', 'ChecksWarning') diff --git a/ui/packages/consul-ui/app/routes/dc/nodes/show/services.js b/ui/packages/consul-ui/app/routes/dc/nodes/show/services.js index 5dbd9b7125..dd97616e57 100644 --- a/ui/packages/consul-ui/app/routes/dc/nodes/show/services.js +++ b/ui/packages/consul-ui/app/routes/dc/nodes/show/services.js @@ -2,6 +2,13 @@ import Route from 'consul-ui/routing/route'; export default class ServicesRoute extends Route { queryParams = { + sortBy: 'sort', + status: 'status', + source: 'source', + searchproperty: { + as: 'searchproperty', + empty: [['Name', 'Tags', 'ID', 'Address', 'Port', 'Service.Meta']], + }, search: { as: 'filter', replace: true, @@ -13,7 +20,10 @@ export default class ServicesRoute extends Route { .split('.') .slice(0, -1) .join('.'); - return this.modelFor(parent); + return { + ...this.modelFor(parent), + searchProperties: this.queryParams.searchproperty.empty[0], + }; } setupController(controller, model) { diff --git a/ui/packages/consul-ui/app/routes/dc/services/show/instances.js b/ui/packages/consul-ui/app/routes/dc/services/show/instances.js index a8b7a1c804..6690bd457c 100644 --- a/ui/packages/consul-ui/app/routes/dc/services/show/instances.js +++ b/ui/packages/consul-ui/app/routes/dc/services/show/instances.js @@ -20,7 +20,10 @@ export default class InstancesRoute extends Route { .split('.') .slice(0, -1) .join('.'); - return this.modelFor(parent); + return { + ...this.modelFor(parent), + searchProperties: this.queryParams.searchproperty.empty[0], + }; } setupController(controller, model) { diff --git a/ui/packages/consul-ui/app/serializers/http.js b/ui/packages/consul-ui/app/serializers/http.js index 2821d468a6..831aeb1ce5 100644 --- a/ui/packages/consul-ui/app/serializers/http.js +++ b/ui/packages/consul-ui/app/serializers/http.js @@ -1,6 +1,14 @@ import Serializer from '@ember-data/serializer/rest'; export default class HttpSerializer extends Serializer { + transformBelongsToResponse(store, relationship, parent, item) { + return item; + } + + transformHasManyResponse(store, relationship, parent, item) { + return item; + } + respondForQuery(respond, query) { return respond((headers, body) => body); } diff --git a/ui/packages/consul-ui/app/serializers/node.js b/ui/packages/consul-ui/app/serializers/node.js index ef028c736d..e65a45cda4 100644 --- a/ui/packages/consul-ui/app/serializers/node.js +++ b/ui/packages/consul-ui/app/serializers/node.js @@ -1,5 +1,7 @@ import Serializer from './application'; +import { EmbeddedRecordsMixin } from '@ember-data/serializer/rest'; import { PRIMARY_KEY, SLUG_KEY } from 'consul-ui/models/node'; +import { classify } from '@ember/string'; // TODO: Looks like ID just isn't used at all consider just using .Node for // the SLUG_KEY @@ -10,25 +12,70 @@ const fillSlug = function(item) { return item; }; -export default class NodeSerializer extends Serializer { +export default class NodeSerializer extends Serializer.extend(EmbeddedRecordsMixin) { primaryKey = PRIMARY_KEY; slugKey = SLUG_KEY; - respondForQuery(respond, query) { - return super.respondForQuery( + attrs = { + Services: { + embedded: 'always', + }, + }; + + transformHasManyResponse(store, relationship, item, parent = null) { + switch (relationship.key) { + case 'Services': + const checks = {}; + (item.Checks || []) + .filter(item => { + return item.ServiceID !== ''; + }) + .forEach(item => { + if (typeof checks[item.ServiceID] === 'undefined') { + checks[item.ServiceID] = []; + } + checks[item.ServiceID].push(item); + }); + const serializer = this.store.serializerFor(relationship.type); + item.Services = item.Services.map(service => + serializer.transformHasManyResponseFromNode(item, service, checks) + ); + return item; + } + return super.transformHasManyResponse(...arguments); + } + + respondForQuery(respond, query, data, modelClass) { + const body = super.respondForQuery( cb => respond((headers, body) => cb(headers, body.map(fillSlug))), query ); + modelClass.eachRelationship((key, relationship) => { + body.forEach(item => + this[`transform${classify(relationship.kind)}Response`]( + this.store, + relationship, + item, + body + ) + ); + }); + return body; } - respondForQueryRecord(respond, query) { - return super.respondForQueryRecord( + respondForQueryRecord(respond, query, data, modelClass) { + const body = super.respondForQueryRecord( cb => respond((headers, body) => { return cb(headers, fillSlug(body)); }), query ); + + modelClass.eachRelationship((key, relationship) => { + this[`transform${classify(relationship.kind)}Response`](this.store, relationship, body); + }); + return body; } respondForQueryLeader(respond, query) { diff --git a/ui/packages/consul-ui/app/serializers/service-instance.js b/ui/packages/consul-ui/app/serializers/service-instance.js index e1b0400a09..9aa47222bc 100644 --- a/ui/packages/consul-ui/app/serializers/service-instance.js +++ b/ui/packages/consul-ui/app/serializers/service-instance.js @@ -5,9 +5,62 @@ export default class ServiceInstanceSerializer extends Serializer { primaryKey = PRIMARY_KEY; slugKey = SLUG_KEY; + hash = JSON.stringify; + + extractUid(item) { + return this.hash([ + item.Namespace || 'default', + item.Datacenter, + item.Node.Node, + item.Service.ID, + ]); + } + + transformHasManyResponseFromNode(node, item, checks) { + const serviceChecks = checks[item.ID] || []; + const statuses = serviceChecks.reduce( + (prev, item) => { + switch (item.Status) { + case 'passing': + prev.ChecksPassing.push(item); + break; + case 'warning': + prev.ChecksWarning.push(item); + break; + case 'critical': + prev.ChecksCritical.push(item); + break; + } + return prev; + }, + { + ChecksPassing: [], + ChecksWarning: [], + ChecksCritical: [], + } + ); + const instance = { + ...statuses, + Service: item, + Checks: serviceChecks, + Node: { + Datacenter: node.Datacenter, + Namespace: node.Namespace, + ID: node.ID, + Node: node.Node, + Address: node.Address, + TaggedAddresses: node.TaggedAddresses, + Meta: node.Meta, + }, + }; + + instance.uid = this.extractUid(instance); + return instance; + } + respondForQuery(respond, query) { - return super.respondForQuery(function(cb) { - return respond(function(headers, body) { + const body = super.respondForQuery(cb => { + return respond((headers, body) => { if (body.length === 0) { const e = new Error(); e.errors = [ @@ -18,14 +71,25 @@ export default class ServiceInstanceSerializer extends Serializer { ]; throw e; } + body.forEach(item => { + item.Datacenter = query.dc; + item.Namespace = query.ns || 'default'; + item.uid = this.extractUid(item); + }); return cb(headers, body); }); }, query); + return body; } respondForQueryRecord(respond, query) { - return super.respondForQueryRecord(function(cb) { - return respond(function(headers, body) { + return super.respondForQueryRecord(cb => { + return respond((headers, body) => { + body.forEach(item => { + item.Datacenter = query.dc; + item.Namespace = query.ns || 'default'; + item.uid = this.extractUid(item); + }); body = body.find(function(item) { return item.Node.Node === query.node && item.Service.ID === query.serviceId; }); diff --git a/ui/packages/consul-ui/app/templates/dc/nodes/show/services.hbs b/ui/packages/consul-ui/app/templates/dc/nodes/show/services.hbs index 76c563841d..462308ec7e 100644 --- a/ui/packages/consul-ui/app/templates/dc/nodes/show/services.hbs +++ b/ui/packages/consul-ui/app/templates/dc/nodes/show/services.hbs @@ -1,19 +1,51 @@ -{{#let item.Services as |items|}} -
+{{#let (hash + statuses=(if status (split status ',') undefined) + sources=(if source (split source ',') undefined) + searchproperties=(if (not-eq searchproperty undefined) + (split searchproperty ',') + searchProperties + ) +) as |filters|}} + {{#let (or sortBy "Status:asc") as |sort|}} + {{#let item.Services as |items|}} +
{{#if (gt items.length 0) }} - - {{/if}} - - - - - + @searchproperties={{searchProperties}} + + @sort={{sort}} + @onsort={{action (mut sortBy) value="target.selected"}} + + @filter={{filters}} + @onfilter={{hash + searchproperty=(action (mut searchproperty) value="target.selectedItems") + status=(action (mut status) value="target.selectedItems") + source=(action (mut source) value="target.selectedItems") + }} + /> + {{/if}} + {{! filter out any sidecar proxies }} + + + + +

@@ -21,8 +53,10 @@

- - + +
+ {{/let}} + {{/let}} {{/let}} \ No newline at end of file diff --git a/ui/packages/consul-ui/app/templates/dc/services/show/instances.hbs b/ui/packages/consul-ui/app/templates/dc/services/show/instances.hbs index b786066d71..e66d998a38 100644 --- a/ui/packages/consul-ui/app/templates/dc/services/show/instances.hbs +++ b/ui/packages/consul-ui/app/templates/dc/services/show/instances.hbs @@ -5,7 +5,7 @@ sources=(if source (split source ',') undefined) searchproperties=(if (not-eq searchproperty undefined) (split searchproperty ',') - (array 'Name' 'Tags' 'ID' 'Address' 'Port' 'Service.Meta' 'Node.Meta') + searchProperties ) ) as |filters|}} {{#let (or sortBy "Status:asc") as |sort|}} @@ -15,6 +15,7 @@ @sources={{externalSources}} @search={{search}} @onsearch={{action (mut search) value="target.value"}} + @searchproperties={{searchProperties}} @sort={{sort}} @onsort={{action (mut sortBy) value="target.selected"}} @@ -27,6 +28,7 @@ }} /> {{/if}} + {{! Service > Service Instance view doesn't require filtering of proxies }} - Object.assign({}, item, { - Datacenter: dc, - // TODO: default isn't required here, once we've - // refactored out our Serializer this can go - Namespace: nspace, - uid: `["${nspace}","${dc}","${item.ID}"]`, - }) - ); const actual = serializer.respondForQuery( function(cb) { const headers = {}; @@ -33,13 +27,22 @@ module('Integration | Serializer | node', function(hooks) { }, { dc: dc, - } + }, + { + dc: dc, + }, + modelClass ); - assert.deepEqual(actual, expected); + assert.equal(actual[0].Datacenter, dc); + assert.equal(actual[0].Namespace, nspace); + assert.equal(actual[0].uid, `["${nspace}","${dc}","${actual[0].ID}"]`); }); }); test('respondForQueryRecord returns the correct data for item endpoint', function(assert) { + const store = this.owner.lookup('service:store'); const serializer = this.owner.lookup('serializer:node'); + serializer.store = store; + const modelClass = store.modelFor('node'); const dc = 'dc-1'; const id = 'node-name'; const request = { @@ -65,9 +68,15 @@ module('Integration | Serializer | node', function(hooks) { }, { dc: dc, - } + }, + { + dc: dc, + }, + modelClass ); - assert.deepEqual(actual, expected); + assert.equal(actual.Datacenter, dc); + assert.equal(actual.Namespace, nspace); + assert.equal(actual.uid, `["${nspace}","${dc}","${actual.ID}"]`); }); }); test('respondForQueryLeader returns the correct data', function(assert) {