From 21915e600e3eddce931a39d6ba15758c3be35e46 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 000000000..1c3828b84 --- /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 5f6686357..ebd2e30e7 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 66b07f8e5..79b0e2d72 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 e86816221..5e66fd6ae 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 783e62db5..d3ea2f07c 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 a1f56f58c..cae255e6c 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 ea68c9042..91e3d0a5d 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 2a18d2313..3817cbc25 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 4758c7f1d..f7e9810e0 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 e2dbed235..26e536696 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 5e8008516..25e65f762 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 96d14df15..68accd4b9 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 eaf4e5f4d..31067c2b6 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 d78d314d7..8e138f04a 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 4742aa517..ee7f5a085 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); + } ); } );