From 2e13bb9368fc7f7cdfd47b2dca991eaef53196c8 Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Tue, 23 Mar 2021 12:43:33 +0100 Subject: [PATCH] Make sure we keep the announced status of each app (#770) * Make sure we keep the announced status of each app - Since we were running onConflict().merge() we were keeping our entire new object from our remapRow method, and that was overwriting the current announcement status of the row back to false, unless we'd by random chance actually set the announced property on our row to be inserted. * Add migration for cleaning up application-created events - fixes: #769 --- package.json | 2 +- src/lib/db/access-store.ts | 2 +- src/lib/db/client-applications-store.js | 35 ++-- src/lib/db/event-store.ts | 2 +- ...3073508-reset-application-announcements.js | 11 ++ .../client-application-store.e2e.test.js | 175 ++++++++++++++++++ yarn.lock | 54 +++++- 7 files changed, 258 insertions(+), 23 deletions(-) create mode 100644 src/migrations/20210323073508-reset-application-announcements.js create mode 100644 src/test/e2e/stores/client-application-store.e2e.test.js diff --git a/package.json b/package.json index 14461aac9c..63c9f55988 100644 --- a/package.json +++ b/package.json @@ -81,7 +81,7 @@ "helmet": "^4.1.0", "joi": "^17.3.0", "js-yaml": "^3.14.0", - "knex": "0.21.15", + "knex": "0.95.2", "log4js": "^6.0.0", "memoizee": "^0.4.15", "mime": "^2.4.2", diff --git a/src/lib/db/access-store.ts b/src/lib/db/access-store.ts index 6a36a2ff5c..a2d3ca1c63 100644 --- a/src/lib/db/access-store.ts +++ b/src/lib/db/access-store.ts @@ -1,5 +1,5 @@ import { EventEmitter } from 'events'; -import Knex from 'knex'; +import { Knex } from 'knex'; import metricsHelper from '../metrics-helper'; import { DB_TIME } from '../events'; diff --git a/src/lib/db/client-applications-store.js b/src/lib/db/client-applications-store.js index b7f40a38f1..9680a9a56a 100644 --- a/src/lib/db/client-applications-store.js +++ b/src/lib/db/client-applications-store.js @@ -27,17 +27,27 @@ const mapRow = row => ({ icon: row.icon, }); -const remapRow = (input, old = {}) => ({ - app_name: input.appName, - updated_at: input.updatedAt, - description: input.description || old.description, - created_by: input.createdBy || old.createdBy, - announced: input.announced || old.announced || false, - url: input.url || old.url, - color: input.color || old.color, - icon: input.icon || old.icon, - strategies: JSON.stringify(input.strategies || old.strategies), -}); +const remapRow = input => { + const temp = { + app_name: input.appName, + updated_at: input.updatedAt || new Date(), + seen_at: input.lastSeen || new Date(), + description: input.description, + created_by: input.createdBy, + announced: input.announced, + url: input.url, + color: input.color, + icon: input.icon, + strategies: JSON.stringify(input.strategies), + }; + Object.keys(temp).forEach(k => { + if (temp[k] === undefined) { + // not using !temp[k] to allow false and null values to get through + delete temp[k]; + } + }); + return temp; +}; class ClientApplicationsDb { constructor(db, eventBus) { @@ -46,8 +56,9 @@ class ClientApplicationsDb { } async upsert(details) { + const row = remapRow(details); return this.db(TABLE) - .insert(remapRow(details)) + .insert(row) .onConflict('app_name') .merge(); } diff --git a/src/lib/db/event-store.ts b/src/lib/db/event-store.ts index cd3e857072..beccf3ad99 100644 --- a/src/lib/db/event-store.ts +++ b/src/lib/db/event-store.ts @@ -1,5 +1,5 @@ import { EventEmitter } from 'events'; -import Knex from 'knex'; +import { Knex } from 'knex'; import { DROP_FEATURES } from '../event-type'; import { LogProvider, Logger } from '../logger'; diff --git a/src/migrations/20210323073508-reset-application-announcements.js b/src/migrations/20210323073508-reset-application-announcements.js new file mode 100644 index 0000000000..e68d6d2f8a --- /dev/null +++ b/src/migrations/20210323073508-reset-application-announcements.js @@ -0,0 +1,11 @@ +'use strict'; + +exports.up = function(db, cb) { + db.runSql( + ` + DELETE FROM events WHERE type = 'application-created'; + UPDATE client_applications SET announced = false; + `, + cb, + ); +}; diff --git a/src/test/e2e/stores/client-application-store.e2e.test.js b/src/test/e2e/stores/client-application-store.e2e.test.js new file mode 100644 index 0000000000..661432679e --- /dev/null +++ b/src/test/e2e/stores/client-application-store.e2e.test.js @@ -0,0 +1,175 @@ +'use strict'; + +const test = require('ava'); +const faker = require('faker'); +const dbInit = require('../helpers/database-init'); +const getLogger = require('../../fixtures/no-logger'); + +let db; +let stores; +let clientApplicationsStore; + +test.beforeEach(async () => { + db = await dbInit('client_application_store_e2e_serial', getLogger); + stores = db.stores; + clientApplicationsStore = stores.clientApplicationsStore; +}); + +test.afterEach(async () => { + await db.destroy(); +}); + +test.serial("Should be able to keep track of what we've announced", async t => { + const clientRegistration = { + appName: faker.internet.domainName(), + instanceId: faker.random.uuid(), + strategies: ['default'], + started: Date.now(), + interval: faker.random.number(), + sdkVersion: '3.11.2', + icon: '', + description: faker.company.catchPhrase(), + color: faker.internet.color(), + }; + await clientApplicationsStore.upsert(clientRegistration); + let unannounced = await clientApplicationsStore.getUnannounced(); + t.is(unannounced.length, 1); + const announce = await clientApplicationsStore.setUnannouncedToAnnounced(); + t.is(announce.length, 1); + unannounced = await clientApplicationsStore.getUnannounced(); + t.is(unannounced.length, 0); +}); + +test.serial( + 'Multiple instances should still only announce once per app', + async t => { + const clientRegistration = { + appName: faker.internet.domainName(), + instanceId: faker.random.uuid(), + strategies: ['default'], + started: Date.now(), + interval: faker.random.number(), + sdkVersion: '3.11.2', + icon: '', + description: faker.company.catchPhrase(), + color: faker.internet.color(), + }; + const clientReg2 = { ...clientRegistration, instanceId: 'someotherid' }; + await clientApplicationsStore.upsert(clientRegistration); + await clientApplicationsStore.upsert(clientReg2); + let unannounced = await clientApplicationsStore.getUnannounced(); + t.is(unannounced.length, 1); + const announce = await clientApplicationsStore.setUnannouncedToAnnounced(); + t.is(announce.length, 1); + unannounced = await clientApplicationsStore.getUnannounced(); + t.is(unannounced.length, 0); + }, +); + +test.serial( + 'Multiple applications should also be possible to announce', + async t => { + const clients = []; + while (clients.length < 10) { + const clientRegistration = { + appName: `${faker.internet.domainName()}_${clients.length}`, + instanceId: faker.random.uuid(), + strategies: ['default'], + started: Date.now(), + interval: faker.random.number(), + sdkVersion: '3.11.2', + icon: '', + description: faker.company.catchPhrase(), + color: faker.internet.color(), + }; + clients.push(clientRegistration); + } + await clientApplicationsStore.bulkUpsert(clients); + let unannounced = await clientApplicationsStore.getUnannounced(); + t.is(unannounced.length, 10); + const announce = await clientApplicationsStore.setUnannouncedToAnnounced(); + t.is(announce.length, 10); + unannounced = await clientApplicationsStore.getUnannounced(); + t.is(unannounced.length, 0); + }, +); + +test.serial( + 'Same application registered multiple times should still only be announced once', + async t => { + const clientRegistration = { + appName: faker.internet.domainName(), + instanceId: faker.random.uuid(), + strategies: ['default'], + started: Date.now(), + interval: faker.random.number(), + sdkVersion: '3.11.2', + icon: '', + description: faker.company.catchPhrase(), + color: faker.internet.color(), + }; + await clientApplicationsStore.upsert(clientRegistration); + let unannounced = await clientApplicationsStore.getUnannounced(); + t.is(unannounced.length, 1); + let announce = await clientApplicationsStore.setUnannouncedToAnnounced(); + t.is(announce.length, 1); + unannounced = await clientApplicationsStore.getUnannounced(); + t.is(unannounced.length, 0); + + await clientApplicationsStore.upsert(clientRegistration); + announce = await clientApplicationsStore.setUnannouncedToAnnounced(); + t.is(announce.length, 0); + }, +); + +test.serial('Merge keeps value for single row in database', async t => { + const clientRegistration = { + appName: faker.internet.domainName(), + instanceId: faker.random.uuid(), + strategies: ['default'], + started: Date.now(), + icon: faker.internet.color(), + description: faker.company.catchPhrase(), + color: faker.internet.color(), + }; + await clientApplicationsStore.upsert(clientRegistration); + await clientApplicationsStore.upsert({ + appName: clientRegistration.appName, + description: 'new description', + }); + const stored = await clientApplicationsStore.getApplication( + clientRegistration.appName, + ); + t.is(stored.color, clientRegistration.color); + t.is(stored.description, 'new description'); +}); + +test.serial('Multi row merge also works', async t => { + const clients = []; + while (clients.length < 10) { + const clientRegistration = { + appName: `${faker.internet.domainName()}_${clients.length}`, + instanceId: faker.random.uuid(), + strategies: ['default'], + started: Date.now(), + icon: faker.internet.color(), + description: faker.company.catchPhrase(), + color: faker.internet.color(), + }; + clients.push(clientRegistration); + } + await clientApplicationsStore.bulkUpsert(clients); + const alteredClients = clients.map(c => { + return { appName: c.appName, icon: 'red' }; + }); + await clientApplicationsStore.bulkUpsert(alteredClients); + const stored = await Promise.all( + clients.map(async c => + clientApplicationsStore.getApplication(c.appName), + ), + ); + stored.forEach((s, i) => { + t.is(s.description, clients[i].description); + t.is(s.icon, 'red'); + }); +}); diff --git a/yarn.lock b/yarn.lock index aaa199b788..5eb2c3c402 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1442,6 +1442,11 @@ commander@^6.2.0: resolved "https://registry.npmjs.org/commander/-/commander-6.2.1.tgz" integrity sha512-U7VdrJFnJgo4xjrHpTzu0yrHPGImdsmD95ZlgYSEajAn2JKzDhDTPG9kBTefmObL2w/ngeZnilk+OV9CG3d7UA== +commander@^7.1.0: + version "7.2.0" + resolved "https://registry.yarnpkg.com/commander/-/commander-7.2.0.tgz#a36cb57d0b501ce108e4d20559a150a391d97ab7" + integrity sha512-QrWXB+ZQSVPmIWIhtEO9H+gwHaMGYiF5ChvoJ+K9ZGHG/sVsa6yiesAD1GC/x46sET00Xlwo1u49RVVVzvcSkw== + common-path-prefix@^3.0.0: version "3.0.0" resolved "https://registry.npmjs.org/common-path-prefix/-/common-path-prefix-3.0.0.tgz" @@ -2149,6 +2154,11 @@ escalade@^3.0.2: resolved "https://registry.npmjs.org/escalade/-/escalade-3.1.0.tgz" integrity sha512-mAk+hPSO8fLDkhV7V0dXazH5pDc6MrjBTPyD3VeKzxnVFjH1MIxbCdqGZB9O8+EwWakZs3ZCbDS4IpRt79V1ig== +escalade@^3.1.1: + version "3.1.1" + resolved "https://registry.yarnpkg.com/escalade/-/escalade-3.1.1.tgz#d8cfdc7000965c5a0174b4a82eaa5c0552742e40" + integrity sha512-k0er2gUkLf8O0zKJiAhmkTnJlTvINGv7ygDNPbeIsX/TJjGJZHuh9B2UxbsaEkmlEo9MfhrSzmhIlhRlI2GXnw== + escape-goat@^2.0.0: version "2.1.1" resolved "https://registry.npmjs.org/escape-goat/-/escape-goat-2.1.1.tgz" @@ -3452,6 +3462,13 @@ is-ci@^2.0.0: dependencies: ci-info "^2.0.0" +is-core-module@^2.2.0: + version "2.2.0" + resolved "https://registry.yarnpkg.com/is-core-module/-/is-core-module-2.2.0.tgz#97037ef3d52224d85163f5597b2b63d9afed981a" + integrity sha512-XRAfAdyyY5F5cOXn7hYQDqh2Xmii+DEfIcQGxK/uNwMHhIkPWO0g8msXcbzLe+MpGoR951MlqM/2iIlU4vKDdQ== + dependencies: + has "^1.0.3" + is-data-descriptor@^0.1.4: version "0.1.4" resolved "https://registry.npmjs.org/is-data-descriptor/-/is-data-descriptor-0.1.4.tgz" @@ -3928,23 +3945,24 @@ kind-of@^6.0.0, kind-of@^6.0.2, kind-of@^6.0.3: resolved "https://registry.yarnpkg.com/kind-of/-/kind-of-6.0.3.tgz#07c05034a6c349fa06e24fa35aa76db4580ce4dd" integrity sha512-dcS1ul+9tmeD95T+x28/ehLgd9mENa3LsvDTtzm3vyBEO7RPptvAD+t44WVXaUjTBRcrpFeFlC8WCruUR456hw== -knex@0.21.15: - version "0.21.15" - resolved "https://registry.npmjs.org/knex/-/knex-0.21.15.tgz" - integrity sha512-STHnPIIkExZVz0X3zIDaWC/q9EcTAfuuRE5Rev7NpOhF1QVh24K5iT2FXD0nWoJ1BUSeDs5QdgdTaz9H8oEtfg== +knex@0.95.2: + version "0.95.2" + resolved "https://registry.yarnpkg.com/knex/-/knex-0.95.2.tgz#ebc258b23c9b936c7c3c3f73bbada58b4e138059" + integrity sha512-USnukuNnoVAfeV6OXrkHPLcUQRsRbTl3Tk5E3x0hmKs4uEu5vGfZHxD4luyrndIysyDrKXBWfASQ1wFpJWDDFg== dependencies: colorette "1.2.1" - commander "^6.2.0" + commander "^7.1.0" debug "4.3.1" + escalade "^3.1.1" esm "^3.2.25" getopts "2.2.5" interpret "^2.2.0" - liftoff "3.1.0" - lodash "^4.17.20" + lodash "^4.17.21" pg-connection-string "2.4.0" + rechoir "^0.7.0" + resolve-from "^5.0.0" tarn "^3.0.1" tildify "2.0.0" - v8flags "^3.2.0" knex@^0.21.5: version "0.21.17" @@ -4112,6 +4130,11 @@ lodash@^4.17.11, lodash@^4.17.14, lodash@^4.17.15, lodash@^4.17.19, lodash@^4.17 resolved "https://registry.npmjs.org/lodash/-/lodash-4.17.20.tgz" integrity sha512-PlhdFcillOINfeV7Ni6oF1TAEayyZBoZ8bcshTHqOYJYlrqzRK5hagpagky5o4HfCzzd1TRkXPMFq6cKk9rGmA== +lodash@^4.17.21: + version "4.17.21" + resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.21.tgz#679591c564c3bffaae8454cf0b3df370c3d6911c" + integrity sha512-v2kDEe57lecTulaDIuNTPy3Ry4gLGJ6Z1O3vE1krgXZNrsQ+LFTGHVxVjcXPs17LhbZVGedAJv8XZ1tvj5FvSg== + log-driver@^1.2.7: version "1.2.7" resolved "https://registry.npmjs.org/log-driver/-/log-driver-1.2.7.tgz" @@ -5399,6 +5422,13 @@ rechoir@^0.6.2: dependencies: resolve "^1.1.6" +rechoir@^0.7.0: + version "0.7.0" + resolved "https://registry.yarnpkg.com/rechoir/-/rechoir-0.7.0.tgz#32650fd52c21ab252aa5d65b19310441c7e03aca" + integrity sha512-ADsDEH2bvbjltXEP+hTIAmeFekTFK0V2BTxMkok6qILyAJEXV0AFfoWcAq4yfll5VdIMd/RVXq0lR+wQi5ZU3Q== + dependencies: + resolve "^1.9.0" + regenerator-runtime@^0.13.4: version "0.13.7" resolved "https://registry.yarnpkg.com/regenerator-runtime/-/regenerator-runtime-0.13.7.tgz#cac2dacc8a1ea675feaabaeb8ae833898ae46f55" @@ -5526,6 +5556,14 @@ resolve@^1.1.6, resolve@^1.1.7, resolve@^1.10.0, resolve@^1.11.1, resolve@^1.13. dependencies: path-parse "^1.0.6" +resolve@^1.9.0: + version "1.20.0" + resolved "https://registry.yarnpkg.com/resolve/-/resolve-1.20.0.tgz#629a013fb3f70755d6f0b7935cc1c2c5378b1975" + integrity sha512-wENBPt4ySzg4ybFQW2TT1zMQucPK95HSh/nq2CFTZVOGut2+pQvSsgtda4d26YrYcr067wjbmzOG8byDPBX63A== + dependencies: + is-core-module "^2.2.0" + path-parse "^1.0.6" + response-time@^2.3.2: version "2.3.2" resolved "https://registry.npmjs.org/response-time/-/response-time-2.3.2.tgz"