From a9fe39e035993cf8f5e7d9ed3803218cfdd915f5 Mon Sep 17 00:00:00 2001 From: John Cowen Date: Thu, 7 Oct 2021 12:38:04 +0100 Subject: [PATCH] ui: Fix up blocking reconciliation for multiple models (#11237) > In the future, this should all be moved to each individual repository now, which will mean we can finally get rid of this service. This PR moves reconciliation to 'each individual repository'. I stopped short of getting rid of the service, but its so small now we pretty much don't need it. I'd rather wait until I look at the equivalent DataSink service and see if we can get rid of both equivalent services together (this also currently dependant on work soon to be merged) Reconciliation of models (basically doing the extra work to clean up the ember-data store and bring our frontend 'truth' into line with the actual backend truth) when blocking/long-polling on different views/filters of data is slightly more complicated due to figuring out what should be cleaned up and what should be left in the store. This is especially apparent for KVs. I built in a such a way to hopefully make sure it will all make sense for the future. I also checked that this all worked nicely with all our models, even KV which has never supported blocking queries. I left all that work in so that if we want to enable blocking queries/live updates for KV it now just involves deleting a couple of lines of code. There is a tonne of old stuff that we can clean up here now (our 'fake headers' that we pass around) and I've added that to my list of thing for a 'Big Cleanup PR' that will remove lots of code that we no longer require. --- .changelog/11237.txt | 3 + ui/packages/consul-ui/app/adapters/kv.js | 4 +- .../app/components/data-loader/index.hbs | 3 +- .../consul-ui/app/components/outlet/index.js | 4 - .../consul-ui/app/components/route/index.hbs | 2 +- .../consul-ui/app/components/route/index.js | 16 +++- ui/packages/consul-ui/app/models/kv.js | 3 + .../services/data-source/protocols/http.js | 36 +-------- .../consul-ui/app/services/repository.js | 79 +++++++++++++------ .../consul-ui/app/services/repository/kv.js | 32 +++----- .../services/repository/service-instance.js | 9 ++- ui/packages/consul-ui/app/services/routlet.js | 2 +- .../consul-ui/app/templates/dc/kv/index.hbs | 33 ++++++-- .../consul-ui/app/templates/dc/nodes/show.hbs | 6 +- .../services/repository/kv-test.js | 40 ++++------ 15 files changed, 144 insertions(+), 128 deletions(-) create mode 100644 .changelog/11237.txt diff --git a/.changelog/11237.txt b/.changelog/11237.txt new file mode 100644 index 0000000000..1c3828b84e --- /dev/null +++ b/.changelog/11237.txt @@ -0,0 +1,3 @@ +```release-note:bug +ui: Ensure all types of data get reconciled with the backend data +``` diff --git a/ui/packages/consul-ui/app/adapters/kv.js b/ui/packages/consul-ui/app/adapters/kv.js index 5f66863579..ebd2e30e73 100644 --- a/ui/packages/consul-ui/app/adapters/kv.js +++ b/ui/packages/consul-ui/app/adapters/kv.js @@ -28,7 +28,7 @@ export default class KvAdapter extends Adapter { if (typeof id === 'undefined') { throw new Error('You must specify an id'); } - return request` + const respond = await request` GET /v1/kv/${keyToArray(id)}?${{ dc }} ${{ @@ -37,6 +37,8 @@ export default class KvAdapter extends Adapter { index, }} `; + await respond((headers, body) => delete headers['x-consul-index']); + return respond; } // TODO: Should we replace text/plain here with x-www-form-encoded? See diff --git a/ui/packages/consul-ui/app/components/data-loader/index.hbs b/ui/packages/consul-ui/app/components/data-loader/index.hbs index 66b07f8e52..79b0e2d724 100644 --- a/ui/packages/consul-ui/app/components/data-loader/index.hbs +++ b/ui/packages/consul-ui/app/components/data-loader/index.hbs @@ -1,10 +1,8 @@ {{yield}} - - {{did-update (fn dispatch "LOAD") src=src}} {{#let (hash data=data @@ -79,4 +77,5 @@ {{/let}} + {{did-update (fn dispatch "LOAD") src=src}} \ No newline at end of file diff --git a/ui/packages/consul-ui/app/components/outlet/index.js b/ui/packages/consul-ui/app/components/outlet/index.js index e86816221a..5e66fd6ae7 100644 --- a/ui/packages/consul-ui/app/components/outlet/index.js +++ b/ui/packages/consul-ui/app/components/outlet/index.js @@ -27,10 +27,6 @@ export default class Outlet extends Component { } setAppRoute(name) { - const nspace = 'nspace.'; - if (name.startsWith(nspace)) { - name = name.substr(nspace.length); - } if (name !== 'loading') { const doc = this.element.ownerDocument.documentElement; if (doc.classList.contains('ember-loading')) { diff --git a/ui/packages/consul-ui/app/components/route/index.hbs b/ui/packages/consul-ui/app/components/route/index.hbs index 783e62db54..d3ea2f07c3 100644 --- a/ui/packages/consul-ui/app/components/route/index.hbs +++ b/ui/packages/consul-ui/app/components/route/index.hbs @@ -1,7 +1,7 @@ {{did-insert this.connect}} {{will-destroy this.disconnect}} {{yield (hash - model=this.model + model=(or this.model this._model) params=this.params currentName=this.router.currentRoute.name diff --git a/ui/packages/consul-ui/app/components/route/index.js b/ui/packages/consul-ui/app/components/route/index.js index a1f56f58ca..cae255e6ca 100644 --- a/ui/packages/consul-ui/app/components/route/index.js +++ b/ui/packages/consul-ui/app/components/route/index.js @@ -7,12 +7,26 @@ export default class RouteComponent extends Component { @service('routlet') routlet; @service('router') router; - @tracked model; + @tracked _model; get params() { return this.routlet.paramsFor(this.args.name); } + get model() { + if(this.args.name) { + const temp = this.args.name.split('.'); + temp.pop(); + const name = temp.join('.'); + let model = this.routlet.modelFor(name); + if(Object.keys(model).length === 0) { + return null; + } + return model; + } + return null; + } + @action connect() { this.routlet.addRoute(this.args.name, this); diff --git a/ui/packages/consul-ui/app/models/kv.js b/ui/packages/consul-ui/app/models/kv.js index ea68c90420..91e3d0a5d3 100644 --- a/ui/packages/consul-ui/app/models/kv.js +++ b/ui/packages/consul-ui/app/models/kv.js @@ -12,6 +12,9 @@ export default class Kv extends Model { @attr('string') uid; @attr('string') Key; + @attr('number') SyncTime; + @attr() meta; // {} + @attr('string') Datacenter; @attr('string') Namespace; @attr('string') Partition; diff --git a/ui/packages/consul-ui/app/services/data-source/protocols/http.js b/ui/packages/consul-ui/app/services/data-source/protocols/http.js index 2a18d2313c..3817cbc25f 100644 --- a/ui/packages/consul-ui/app/services/data-source/protocols/http.js +++ b/ui/packages/consul-ui/app/services/data-source/protocols/http.js @@ -1,47 +1,13 @@ import Service, { inject as service } from '@ember/service'; -import { get } from '@ember/object'; import { getOwner } from '@ember/application'; import { match } from 'consul-ui/decorators/data-source'; -import { singularize } from 'ember-inflector'; export default class HttpService extends Service { - @service('repository/dc') datacenters; - @service('repository/dc') datacenter; - @service('repository/kv') kvs; - @service('repository/kv') kv; - @service('repository/node') leader; - @service('repository/service') gateways; - @service('repository/service-instance') 'proxy-service-instance'; - @service('repository/proxy') 'proxy-instance'; - @service('repository/nspace') namespaces; - @service('repository/nspace') namespace; - @service('repository/metrics') metrics; - @service('repository/oidc-provider') oidc; - @service('ui-config') 'ui-config'; - @service('ui-config') notfound; - @service('data-source/protocols/http/blocking') type; source(src, configuration) { - const [, , , , model] = src.split('/'); - const owner = getOwner(this); const route = match(src); - const find = route.cb(route.params, owner); - - const repo = this[model] || owner.lookup(`service:repository/${singularize(model)}`); - if (typeof repo.reconcile === 'function') { - configuration.createEvent = function(result = {}, configuration) { - const event = { - type: 'message', - data: result, - }; - const meta = get(event, 'data.meta') || {}; - if (typeof meta.range === 'undefined') { - repo.reconcile(meta); - } - return event; - }; - } + const find = route.cb(route.params, getOwner(this)); return this.type.source(find, configuration); } } diff --git a/ui/packages/consul-ui/app/services/repository.js b/ui/packages/consul-ui/app/services/repository.js index 4758c7f1df..f7e9810e0c 100644 --- a/ui/packages/consul-ui/app/services/repository.js +++ b/ui/packages/consul-ui/app/services/repository.js @@ -8,6 +8,7 @@ import { ACCESS_READ } from 'consul-ui/abilities/base'; export default class RepositoryService extends Service { @service('store') store; + @service('env') env; @service('repository/permission') permissions; getModelName() { @@ -66,30 +67,33 @@ export default class RepositoryService extends Service { return item; } - reconcile(meta = {}) { + shouldReconcile(item, params) { + const dc = get(item, 'Datacenter'); + if (dc !== params.dc) { + return false; + } + if (this.env.var('CONSUL_NSPACES_ENABLED')) { + const nspace = get(item, 'Namespace'); + if (typeof nspace !== 'undefined' && nspace !== params.ns) { + return false; + } + } + if (this.env.var('CONSUL_PARTITIONS_ENABLED')) { + const partition = get(item, 'Partition'); + if (typeof partiton !== 'undefined' && partition !== params.partition) { + return false; + } + } + return true; + } + + reconcile(meta = {}, params = {}, configuration = {}) { // unload anything older than our current sync date/time if (typeof meta.date !== 'undefined') { - const checkNspace = meta.nspace !== ''; - const checkPartition = meta.partition !== ''; this.store.peekAll(this.getModelName()).forEach(item => { - const dc = get(item, 'Datacenter'); - if (dc === meta.dc) { - if (checkNspace) { - const nspace = get(item, 'Namespace'); - if (typeof nspace !== 'undefined' && nspace !== meta.nspace) { - return; - } - } - if (checkPartition) { - const partition = get(item, 'Partition'); - if (typeof partiton !== 'undefined' && partition !== meta.partition) { - return; - } - } - const date = get(item, 'SyncTime'); - if (!item.isDeleted && typeof date !== 'undefined' && date != meta.date) { - this.store.unloadRecord(item); - } + const date = get(item, 'SyncTime'); + if (!item.isDeleted && typeof date !== 'undefined' && date != meta.date && this.shouldReconcile(item, params)) { + this.store.unloadRecord(item); } }); } @@ -107,16 +111,43 @@ export default class RepositoryService extends Service { } // @deprecated - findAllByDatacenter(params, configuration = {}) { + async findAllByDatacenter(params, configuration = {}) { return this.findAll(...arguments); } - findAll(params = {}, configuration = {}) { + async findAll(params = {}, configuration = {}) { if (typeof configuration.cursor !== 'undefined') { params.index = configuration.cursor; params.uri = configuration.uri; } - return this.store.query(this.getModelName(), params); + return this.query(params); + } + + async query(params = {}, configuration = {}) { + let error, meta, res; + try { + res = await this.store.query(this.getModelName(), params); + meta = res.meta; + } catch(e) { + switch(get(e, 'errors.firstObject.status')) { + case '404': + case '403': + meta = { + date: Number.POSITIVE_INFINITY + }; + error = e; + break; + default: + throw e; + } + } + if(typeof meta !== 'undefined') { + this.reconcile(meta, params, configuration); + } + if(typeof error !== 'undefined') { + throw error; + } + return res; } async findBySlug(params, configuration = {}) { diff --git a/ui/packages/consul-ui/app/services/repository/kv.js b/ui/packages/consul-ui/app/services/repository/kv.js index e2dbed2353..26e536696f 100644 --- a/ui/packages/consul-ui/app/services/repository/kv.js +++ b/ui/packages/consul-ui/app/services/repository/kv.js @@ -15,6 +15,10 @@ export default class KvService extends RepositoryService { return PRIMARY_KEY; } + shouldReconcile(item, params) { + return super.shouldReconcile(...arguments) && item.Key.startsWith(params.id); + } + // this one gives you the full object so key,values and meta @dataSource('/:partition/:ns/:dc/kv/:id') async findBySlug(params, configuration = {}) { @@ -52,33 +56,17 @@ export default class KvService extends RepositoryService { // https://www.consul.io/api/kv.html @dataSource('/:partition/:ns/:dc/kvs/:id') findAllBySlug(params, configuration = {}) { + params.separator = '/'; if (params.id === '/') { params.id = ''; } return this.authorizeBySlug( async () => { - params.separator = '/'; - if (typeof configuration.cursor !== 'undefined') { - params.index = configuration.cursor; - } - let items; - try { - items = await this.store.query(this.getModelName(), params); - } catch (e) { - if (get(e, 'errors.firstObject.status') === '404') { - // TODO: This very much shouldn't be here, - // needs to eventually use ember-datas generateId thing - // in the meantime at least our fingerprinter - // FIXME: Default/token partition - const uid = JSON.stringify([params.partition, params.ns, params.dc, params.id]); - const record = this.store.peekRecord(this.getModelName(), uid); - if (record) { - record.unloadRecord(); - } - } - throw e; - } - return items.filter(item => params.id !== get(item, 'Key')); + let items = await this.findAll(...arguments); + const meta = items.meta; + items = items.filter(item => params.id !== get(item, 'Key')); + items.meta = meta; + return items; }, ACCESS_LIST, params diff --git a/ui/packages/consul-ui/app/services/repository/service-instance.js b/ui/packages/consul-ui/app/services/repository/service-instance.js index 5e80085166..25e65f7621 100644 --- a/ui/packages/consul-ui/app/services/repository/service-instance.js +++ b/ui/packages/consul-ui/app/services/repository/service-instance.js @@ -11,17 +11,22 @@ export default class ServiceInstanceService extends RepositoryService { return modelName; } + shouldReconcile(item, params) { + return super.shouldReconcile(...arguments) && item.Service.Service === params.id; + } + @dataSource('/:partition/:ns/:dc/service-instances/for-service/:id') async findByService(params, configuration = {}) { if (typeof configuration.cursor !== 'undefined') { params.index = configuration.cursor; params.uri = configuration.uri; } - return this.authorizeBySlug( - async () => this.store.query(this.getModelName(), params), + const instances = await this.authorizeBySlug( + async () => this.query(params), ACCESS_READ, params ); + return instances; } @dataSource('/:partition/:ns/:dc/service-instance/:serviceId/:node/:id') diff --git a/ui/packages/consul-ui/app/services/routlet.js b/ui/packages/consul-ui/app/services/routlet.js index 96d14df151..68accd4b98 100644 --- a/ui/packages/consul-ui/app/services/routlet.js +++ b/ui/packages/consul-ui/app/services/routlet.js @@ -134,7 +134,7 @@ export default class RoutletService extends Service { const key = pos + 1; const outlet = outlets.get(keys[key]); if (typeof outlet !== 'undefined') { - route.model = outlet.model; + route._model = outlet.model; // TODO: Try to avoid the double computation bug schedule('afterRender', () => { outlet.routeName = route.args.name; diff --git a/ui/packages/consul-ui/app/templates/dc/kv/index.hbs b/ui/packages/consul-ui/app/templates/dc/kv/index.hbs index eaf4e5f4df..31067c2b63 100644 --- a/ui/packages/consul-ui/app/templates/dc/kv/index.hbs +++ b/ui/packages/consul-ui/app/templates/dc/kv/index.hbs @@ -2,7 +2,7 @@ @name={{routeName}} as |route|> key=(or route.params.key '/') ) }} - @onchange={{action (mut items) value="data"}} + @onchange={{action (mut parent) value="data"}} /> @login={{route.model.app.login.open}} /> + + {{#if (eq loader.error.status "404")}} + + + + {{else if (eq loader.error.status "403")}} + + + + {{else}} + + + + {{/if}} + {{#let @@ -45,8 +68,8 @@ as |route|> ) ) + parent loader.data - items as |sort filters parent items|}} diff --git a/ui/packages/consul-ui/app/templates/dc/nodes/show.hbs b/ui/packages/consul-ui/app/templates/dc/nodes/show.hbs index d78d314d71..8e138f04ad 100644 --- a/ui/packages/consul-ui/app/templates/dc/nodes/show.hbs +++ b/ui/packages/consul-ui/app/templates/dc/nodes/show.hbs @@ -1,9 +1,7 @@ - ) }} as |loader|> - {{/if}} - {{#let loader.data diff --git a/ui/packages/consul-ui/tests/integration/services/repository/kv-test.js b/ui/packages/consul-ui/tests/integration/services/repository/kv-test.js index 4742aa517d..ee7f5a085f 100644 --- a/ui/packages/consul-ui/tests/integration/services/repository/kv-test.js +++ b/ui/packages/consul-ui/tests/integration/services/repository/kv-test.js @@ -1,6 +1,7 @@ import { moduleFor, test } from 'ember-qunit'; import repo from 'consul-ui/tests/helpers/repo'; import { env } from '../../../../env'; +import { get } from '@ember/object'; const NAME = 'kv'; moduleFor(`service:repository/${NAME}`, `Integration | Service | ${NAME}`, { @@ -9,11 +10,15 @@ moduleFor(`service:repository/${NAME}`, `Integration | Service | ${NAME}`, { }); const dc = 'dc-1'; const id = 'key-name'; +const now = new Date().getTime(); const undefinedNspace = 'default'; const undefinedPartition = 'default'; const partition = 'default'; [undefinedNspace, 'team-1', undefined].forEach(nspace => { test(`findAllBySlug returns the correct data for list endpoint when nspace is ${nspace}`, function(assert) { + get(this.subject(), 'store').serializerFor(NAME).timestamp = function() { + return now; + }; return repo( 'Kv', 'findAllBySlug', @@ -43,20 +48,10 @@ const partition = 'default'; const expectedPartition = env('CONSUL_PARTITIONS_ENABLED') ? partition || undefinedPartition : 'default'; - assert.deepEqual( - actual, - expected(function(payload) { - return payload.map(item => { - return { - Datacenter: dc, - Namespace: expectedNspace, - Partition: expectedPartition, - uid: `["${expectedPartition}","${expectedNspace}","${dc}","${item}"]`, - Key: item, - }; - }); - }) - ); + actual.forEach(item => { + assert.equal(item.uid, `["${expectedPartition}","${expectedNspace}","${dc}","${item.Key}"]`); + assert.equal(item.Datacenter, dc); + }); } ); }); @@ -81,18 +76,13 @@ const partition = 'default'; }); }, function(actual, expected) { - assert.deepEqual( - actual, - expected(function(payload) { + expected( + function(payload) { const item = payload[0]; - return Object.assign({}, item, { - Datacenter: dc, - Namespace: item.Namespace || undefinedNspace, - Partition: item.Partition || undefinedPartition, - uid: `["${item.Partition || undefinedPartition}","${item.Namespace || - undefinedNspace}","${dc}","${item.Key}"]`, - }); - }) + assert.equal(actual.uid, `["${item.Partition || undefinedPartition}","${item.Namespace || + undefinedNspace}","${dc}","${item.Key}"]`); + assert.equal(actual.Datacenter, dc); + } ); } );