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

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

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

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

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

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,
+    );
+};

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');
+    });
+});

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"