fix(1-3928): prevent overwriting existing values in instance store (#10360)

Fixes a bug in the instance store where insert and bulkUpsert would
overwrite existing properties if there was a row there already. Now
it'll ignore any properties that are undefined.

The implementation is lifted directly from
`src/lib/db/client-applications-store.ts` (line 107 atm).

Additionally, I've renamed the `insert` method to `upsert` to make it
clearer what it does (and because we already have `bulkUpsert`). The
method seems to only be used in tests, anyway. I do not anticipate any
changes to be required in enterprise (I've checked).

## Discussion points:

This implementation uses `delete` to remove properties from the object.
Why didn't I do it some other way? Two main reasons:
1. We've had this implementation for 4 years in the client applications
store. If there were serious issues with it, we'd probably know by know.
(Probably.)
2. The only way I can think of without deleting, would be to use
`Object.fromEntries` and `Object.toEntries` and either map or reduce.
That'll double the amount of property iterations we'll need to do.

So naively, this strikes me as being more efficient. If you know better
solutions, I will of course be happy to take them. If not, I'd like to
leave this as is and then change it if we see that it's causing issues.

src/lib/db/client-instance-store.ts

@@ -21,25 +21,37 @@ const COLUMNS = [
 ];
 const TABLE = 'client_instances';
 
-const mapRow = (row) => ({
+const mapRow = (row): IClientInstance => ({
     appName: row.app_name,
     instanceId: row.instance_id,
     sdkVersion: row.sdk_version,
+    sdkType: row.sdk_type,
     clientIp: row.client_ip,
     lastSeen: row.last_seen,
     createdAt: row.created_at,
     environment: row.environment,
 });
 
-const mapToDb = (client) => ({
-    app_name: client.appName,
-    instance_id: client.instanceId,
-    sdk_version: client.sdkVersion || '',
-    sdk_type: client.sdkType,
-    client_ip: client.clientIp,
-    last_seen: client.lastSeen || 'now()',
-    environment: client.environment || 'default',
-});
+const mapToDb = (client: INewClientInstance) => {
+    const temp = {
+        app_name: client.appName,
+        instance_id: client.instanceId,
+        sdk_version: client.sdkVersion,
+        sdk_type: client.sdkType,
+        client_ip: client.clientIp,
+        last_seen: client.lastSeen || 'now()',
+        environment: client.environment,
+    };
+
+    const result = {};
+    for (const [key, value] of Object.entries(temp)) {
+        if (value !== undefined) {
+            result[key] = value;
+        }
+    }
+
+    return result;
+};
 
 export default class ClientInstanceStore implements IClientInstanceStore {
     private db: Db;
@@ -127,7 +139,7 @@ export default class ClientInstanceStore implements IClientInstanceStore {
         return present;
     }
 
-    async insert(details: INewClientInstance): Promise<void> {
+    async upsert(details: INewClientInstance): Promise<void> {
         const stopTimer = this.metricTimer('insert');
 
         await this.db(TABLE)

src/lib/types/stores/client-instance-store.ts

@@ -11,6 +11,7 @@ export interface INewClientInstance {
     clientIp?: string;
     lastSeen?: Date;
     environment?: string;
+    sdkType?: 'backend' | 'frontend' | null;
 }
 export interface IClientInstanceStore
     extends Store<
@@ -18,7 +19,7 @@ export interface IClientInstanceStore
         Pick<INewClientInstance, 'appName' | 'instanceId'>
     > {
     bulkUpsert(instances: INewClientInstance[]): Promise<void>;
-    insert(details: INewClientInstance): Promise<void>;
+    upsert(details: INewClientInstance): Promise<void>;
     getByAppName(appName: string): Promise<IClientInstance[]>;
     getRecentByAppNameAndEnvironment(
         appName: string,

src/test/e2e/api/admin/applications.e2e.test.ts

@@ -275,7 +275,7 @@ test('should not return instances older than 24h', async () => {
     await db.stores.clientApplicationsStore.upsert({
         appName: metrics.appName,
     });
-    await db.stores.clientInstanceStore.insert({
+    await db.stores.clientInstanceStore.upsert({
         appName: metrics.appName,
         clientIp: '127.0.0.1',
         instanceId: 'old-instance',

src/test/e2e/api/admin/metrics.e2e.test.ts

@@ -47,15 +47,15 @@ beforeEach(async () => {
         announced: true,
     });
 
-    await db.stores.clientInstanceStore.insert({
+    await db.stores.clientInstanceStore.upsert({
         appName: 'demo-app-1',
         instanceId: 'test-1',
     });
-    await db.stores.clientInstanceStore.insert({
+    await db.stores.clientInstanceStore.upsert({
         appName: 'demo-seed-2',
         instanceId: 'test-2',
     });
-    await db.stores.clientInstanceStore.insert({
+    await db.stores.clientInstanceStore.upsert({
         appName: 'deletable-app',
         instanceId: 'inst-1',
     });

src/test/e2e/stores/client-instance-store.e2e.test.ts

@@ -0,0 +1,60 @@
+import faker from 'faker';
+import dbInit, { type ITestDb } from '../helpers/database-init.js';
+import getLogger from '../../fixtures/no-logger.js';
+import type {
+    IClientInstanceStore,
+    IUnleashStores,
+} from '../../../lib/types/index.js';
+import type { INewClientInstance } from '../../../lib/types/stores/client-instance-store.js';
+
+let db: ITestDb;
+let stores: IUnleashStores;
+let clientInstanceStore: IClientInstanceStore;
+
+beforeAll(async () => {
+    db = await dbInit('client_application_store_e2e_serial', getLogger);
+    stores = db.stores;
+    clientInstanceStore = stores.clientInstanceStore;
+});
+
+afterAll(async () => {
+    await db.destroy();
+});
+
+test('Upserting an application keeps values not provided intact', async () => {
+    const clientInstance: INewClientInstance = {
+        appName: faker.internet.domainName(),
+        instanceId: faker.datatype.uuid(),
+        environment: 'development',
+        sdkVersion: 'unleash-client-node:6.6.0',
+        sdkType: 'backend',
+    };
+    await clientInstanceStore.upsert(clientInstance);
+
+    const initial = await clientInstanceStore.get(clientInstance);
+
+    expect(initial).toMatchObject(clientInstance);
+
+    const update: INewClientInstance = {
+        appName: clientInstance.appName,
+        instanceId: clientInstance.instanceId,
+        environment: clientInstance.environment,
+        clientIp: '::2',
+    };
+
+    await clientInstanceStore.upsert(update);
+
+    const updated = await clientInstanceStore.get(clientInstance);
+
+    const expectedAfterUpdate = {
+        clientIp: '::2',
+        sdkVersion: 'unleash-client-node:6.6.0',
+        sdkType: 'backend',
+    };
+    expect(updated).toMatchObject(expectedAfterUpdate);
+
+    await clientInstanceStore.bulkUpsert([clientInstance]);
+    const doubleUpdated = await clientInstanceStore.get(clientInstance);
+
+    expect(doubleUpdated).toMatchObject(expectedAfterUpdate);
+});

src/test/fixtures/fake-client-instance-store.ts

@@ -109,7 +109,7 @@ export default class FakeClientInstanceStore implements IClientInstanceStore {
         return apps.length;
     }
 
-    async insert(details: INewClientInstance): Promise<void> {
+    async upsert(details: INewClientInstance): Promise<void> {
         this.instances.push({ createdAt: new Date(), ...details });
     }