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.
This commit is contained in:
John Cowen 2021-10-07 12:38:04 +01:00 committed by GitHub
parent c856089d2a
commit 21915e600e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
15 changed files with 144 additions and 128 deletions

3
.changelog/11237.txt Normal file
View File

@ -0,0 +1,3 @@
```release-note:bug
ui: Ensure all types of data get reconciled with the backend data
```

View File

@ -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

View File

@ -1,10 +1,8 @@
{{yield}}
<StateChart @src={{chart}} as |State Guard Action dispatch state|>
<Ref @target={{this}} @name="dispatch" @value={{dispatch}} />
<Guard @name="loaded" @cond={{action "isLoaded"}} />
{{did-update (fn dispatch "LOAD") src=src}}
{{#let (hash
data=data
@ -79,4 +77,5 @@
</State>
{{/let}}
{{did-update (fn dispatch "LOAD") src=src}}
</StateChart>

View File

@ -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')) {

View File

@ -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

View File

@ -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);

View File

@ -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;

View File

@ -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);
}
}

View File

@ -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,31 +67,34 @@ 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) {
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 = {}) {

View File

@ -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

View File

@ -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')

View File

@ -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;

View File

@ -2,7 +2,7 @@
@name={{routeName}}
as |route|>
<DataSource
@src={{uri '/${partition}/${nspace}/${dc}/kvs/${key}'
@src={{uri '/${partition}/${nspace}/${dc}/kv/${key}'
(hash
partition=route.params.partition
nspace=route.params.nspace
@ -10,11 +10,10 @@ as |route|>
key=(or route.params.key '/')
)
}}
@onchange={{action (mut items) value="data"}}
@onchange={{action (mut parent) value="data"}}
/>
<DataLoader
@src={{
uri '/${partition}/${nspace}/${dc}/kv/${key}'
@src={{uri '/${partition}/${nspace}/${dc}/kvs/${key}'
(hash
partition=route.params.partition
nspace=route.params.nspace
@ -29,6 +28,30 @@ as |route|>
@login={{route.model.app.login.open}}
/>
</BlockSlot>
<BlockSlot @name="disconnected" as |Notification|>
{{#if (eq loader.error.status "404")}}
<Notification @sticky={{true}}>
<p data-notification role="alert" class="warning notification-update">
<strong>Warning!</strong>
This KV or parent of this KV was deleted.
</p>
</Notification>
{{else if (eq loader.error.status "403")}}
<Notification @sticky={{true}}>
<p data-notification role="alert" class="error notification-update">
<strong>Error!</strong>
You no longer have access to this KV.
</p>
</Notification>
{{else}}
<Notification @sticky={{true}}>
<p data-notification role="alert" class="warning notification-update">
<strong>Warning!</strong>
An error was returned whilst loading this data, refresh to try again.
</p>
</Notification>
{{/if}}
</BlockSlot>
<BlockSlot @name="loaded">
{{#let
@ -45,8 +68,8 @@ as |route|>
)
)
parent
loader.data
items
as |sort filters parent items|}}
<AppView>

View File

@ -1,9 +1,7 @@
<Route
@name={{routeName}}
@title={{item.Node}}
as |route|>
<DataSource @src={{
uri '/${partition}/${nspace}/${dc}/coordinates/for-node/${name}'
<DataSource @src={{uri '/${partition}/${nspace}/${dc}/coordinates/for-node/${name}'
(hash
partition=route.params.partition
nspace=route.params.nspace
@ -21,7 +19,6 @@ as |route|>
)
}}
as |loader|>
<BlockSlot @name="error">
<AppError
@error={{loader.error}}
@ -53,7 +50,6 @@ as |route|>
</Notification>
{{/if}}
</BlockSlot>
<BlockSlot @name="loaded">
{{#let
loader.data

View File

@ -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);
}
);
}
);