Merge pull request #2253 from botpress/fl_kvs_for_bot

feat(kvs): kvs forBot and forGlobal with no breaking changes

src/bp/core/api.ts

@@ -21,7 +21,6 @@ import { BotService } from './services/bot-service'
 import { CMSService } from './services/cms'
 import { DialogEngine } from './services/dialog/dialog-engine'
 import { SessionIdFactory } from './services/dialog/session/id-factory'
-import { ScopedGhostService } from './services/ghost/service'
 import { HookService } from './services/hook/hook-service'
 import { KeyValueStore } from './services/kvs'
 import MediaService from './services/media'
@@ -133,6 +132,12 @@ const users = (userRepo: UserRepository): typeof sdk.users => {
 
 const kvs = (kvs: KeyValueStore): typeof sdk.kvs => {
   return {
+    forBot(botId: string): sdk.KvsService {
+      return kvs.forBot(botId)
+    },
+    global(): sdk.KvsService {
+      return kvs.global()
+    },
     async get(botId: string, key: string, path?: string): Promise<any> {
       return kvs.get(botId, key, path)
     },

src/bp/core/database/tables/bot-specific/kvs.ts

@@ -6,11 +6,11 @@ export class KeyValueStoreTable extends Table {
   async bootstrap() {
     let created = false
     await this.knex.createTableIfNotExists(this.name, table => {
-      table.string('key').primary()
+      table.string('key')
       table.text('value').notNullable()
       table.string('botId').notNullable()
-
       table.timestamp('modified_on')
+      table.primary(['key', 'botId'])
       created = true
     })
     return created

src/bp/core/services/kvs.test.ts

@@ -88,4 +88,25 @@ createDatabaseSuite('KVS', (database: Database) => {
       })
     })
   })
+
+  describe('global/scoped', () => {
+    test('kvs entries of global and scoped should not interfer', async () => {
+      // Arrange && Act
+      const key = 'gordon-ramsay-favorite-number'
+      await kvs.set('bot1', key, '1')
+      await kvs.global().set(key, '2')
+      await kvs.set('bot1', key, '666')
+      await kvs.set('bot2', key, '69')
+      await kvs.global().set(key, '42')
+
+      const globalActual = await kvs.global().get(key)
+      const bot1Actual = await kvs.get('bot1', key)
+      const bot2actual = await kvs.get('bot2', key)
+
+      // Assert
+      expect(globalActual).toEqual('42')
+      expect(bot1Actual).toEqual('666')
+      expect(bot2actual).toEqual('69')
+    })
+  })
 })

src/bp/core/services/kvs.ts

@@ -1,4 +1,4 @@
-import { Logger } from 'botpress/sdk'
+import * as sdk from 'botpress/sdk'
 import { inject, injectable, tagged } from 'inversify'
 import _ from 'lodash'
 import moment from 'moment'
@@ -8,22 +8,81 @@ import Database from '../database'
 import { safeStringify } from '../misc/utils'
 import { TYPES } from '../types'
 
+const GLOBAL = '__global__'
+const TABLE_NAME = 'srv_kvs'
+
 @injectable()
 export class KeyValueStore {
-  private readonly tableName = 'srv_kvs'
+  private services: _.Dictionary<KvsService> = {}
+  private globalKvs: KvsService
 
   constructor(
     @inject(TYPES.Database) private database: Database,
     @inject(TYPES.Logger)
     @tagged('name', 'KVS')
-    private logger: Logger
-  ) {}
+    private logger: sdk.Logger
+  ) {
+    this.globalKvs = new KvsService(this.database, this.logger)
+  }
+
+  public global() {
+    return this.globalKvs
+  }
+
+  public forBot(botId: string) {
+    if (!!this.services[botId]) {
+      return this.services[botId]
+    }
+    const newService = new KvsService(this.database, this.logger, botId)
+    this.services[botId] = newService
+    return newService
+  }
+
+  // All these are deprecated in sdk. Should be removed.
+
+  get = async (botId: string, key: string, path?: string) => {
+    return this.forBot(botId).get(key, path)
+  }
 
-  upsert = (botId: string, key: string, value) => {
+  set = (botId: string, key: string, value, path?: string) => {
+    return this.forBot(botId).set(key, value, path)
+  }
+
+  setStorageWithExpiry = async (botId: string, key: string, value, expiryInMs?: string) => {
+    return this.forBot(botId).setStorageWithExpiry(key, value, expiryInMs)
+  }
+
+  getStorageWithExpiry = async (botId: string, key: string) => {
+    return this.forBot(botId).getStorageWithExpiry(key)
+  }
+
+  removeStorageKeysStartingWith = key => {
+    this.globalKvs.removeStorageKeysStartingWith(key)
+  }
+
+  getConversationStorageKey = (sessionId, variable) => {
+    return this.globalKvs.getConversationStorageKey(sessionId, variable)
+  }
+
+  getUserStorageKey = (userId, variable) => {
+    return this.globalKvs.getUserStorageKey(userId, variable)
+  }
+
+  getGlobalStorageKey = variable => {
+    return this.globalKvs.getGlobalStorageKey(variable)
+  }
+}
+
+export class KvsService implements sdk.KvsService {
+  constructor(private database: Database, private logger: sdk.Logger, private botId: string = GLOBAL) {}
+
+  private _upsert = (key: string, value) => {
     let sql
 
+    const { botId } = this
+
     const params = {
-      tableName: this.tableName,
+      tableName: TABLE_NAME,
       botIdCol: 'botId',
       keyCol: 'key',
       valueCol: 'value',
@@ -43,17 +102,19 @@ export class KeyValueStore {
       sql = `
         INSERT INTO :tableName: (:botIdCol:, :keyCol:, :valueCol:, :modifiedOnCol:)
         VALUES (:botId, :key, :value, :now)
-        ON CONFLICT (:keyCol:) DO UPDATE
+        ON CONFLICT (:keyCol:, :botIdCol:) DO UPDATE
           SET :valueCol: = :value, :modifiedOnCol: = :now
       `
     }
 
     return this.database.knex.raw(sql, params)
   }
 
-  get = async (botId: string, key: string, path?: string) =>
-    this.database
-      .knex(this.tableName)
+  get = async (key: string, path?: string) => {
+    const { botId } = this
+
+    return this.database
+      .knex(TABLE_NAME)
       .where({ botId })
       .andWhere({ key })
       .limit(1)
@@ -70,10 +131,11 @@ export class KeyValueStore {
 
         return _.get(obj, path)
       })
+  }
 
-  set = (botId: string, key: string, value, path?: string) => {
+  set = async (key: string, value, path?: string) => {
     if (!path) {
-      return this.upsert(botId, key, value)
+      return this._upsert(key, value)
     }
 
     const setValue = obj => {
@@ -85,7 +147,7 @@ export class KeyValueStore {
       }
     }
 
-    return this.get(botId, key).then(original => this.upsert(botId, key, setValue(original || {})))
+    return this.get(key).then(original => this._upsert(key, setValue(original || {})))
   }
 
   private boxWithExpiry = (value, expiry = 'never') => {
@@ -102,19 +164,19 @@ export class KeyValueStore {
     return undefined
   }
 
-  setStorageWithExpiry = async (botId: string, key: string, value, expiryInMs?: string) => {
+  setStorageWithExpiry = async (key: string, value, expiryInMs?: string) => {
     const box = this.boxWithExpiry(value, expiryInMs)
-    await this.set(botId, key, box)
+    await this.set(key, box)
   }
 
-  getStorageWithExpiry = async (botId: string, key: string) => {
-    const box = await this.get(botId, key)
+  getStorageWithExpiry = async key => {
+    const box = await this.get(this.botId, key)
     return this.unboxWithExpiry(box)
   }
 
   removeStorageKeysStartingWith = async key => {
     await this.database
-      .knex(this.tableName)
+      .knex(TABLE_NAME)
       .where('key', 'like', key + '%')
       .del()
   }

src/bp/migrations/v12_1_2-1566397053-kvs_fix_primary_keys.ts

@@ -0,0 +1,79 @@
+import * as sdk from 'botpress/sdk'
+import { Migration } from 'core/services/migration'
+
+const migration: Migration = {
+  info: {
+    description: 'alter kvs table primary key for a union of both column key and column botId',
+    target: 'core',
+    type: 'database'
+  },
+  up: async ({
+    bp,
+    configProvider,
+    database,
+    inversify,
+    metadata
+  }: sdk.ModuleMigrationOpts): Promise<sdk.MigrationResult> => {
+    const tableName = 'srv_kvs'
+    const tempName = 'srv_kvs_temp'
+
+    const noMigrationNeeded = { success: true, message: 'No need for migration' }
+    const { client } = database.knex.client.config
+    if (client === 'sqlite3') {
+      const tableInfo = await database.knex.raw(`PRAGMA table_info([${tableName}])`).then(x => x)
+      const numberOfPK = tableInfo.map(column => column.pk).filter(x => !!x).length
+      if (numberOfPK > 1) {
+        return noMigrationNeeded
+      }
+    } else {
+      const pks = await database.knex
+        .raw(
+          `SELECT a.attname as name
+            FROM   pg_index i
+            JOIN   pg_attribute a ON a.attrelid = i.indrelid
+                                AND a.attnum = ANY(i.indkey)
+            WHERE  i.indrelid = '${tableName}'::regclass
+            AND    i.indisprimary`
+        )
+        .then(x => x.rows.map(r => r.name))
+      if (pks.length > 1) {
+        return noMigrationNeeded
+      }
+    }
+
+    let errorStatus
+    await database.knex
+      .transaction(async trx => {
+        await database.knex.schema.transacting(trx).createTable(tempName, table => {
+          table.string('key')
+          table.text('value').notNullable()
+          table.string('botId').notNullable()
+          table.timestamp('modified_on')
+          table.primary(['key', 'botId'])
+        })
+        const rows = await database.knex
+          .transacting(trx)
+          .select()
+          .from(tableName)
+        if (rows.length) {
+          await database.knex
+            .transacting(trx)
+            .table(tempName)
+            .insert(rows)
+        }
+        await database.knex.schema.transacting(trx).dropTable(tableName)
+        await database.knex.schema.transacting(trx).renameTable(tempName, tableName)
+      })
+      .catch(err => {
+        errorStatus = err
+      })
+
+    if (errorStatus) {
+      return { success: false, message: errorStatus }
+    }
+
+    return { success: true, message: 'Database updated successfully' }
+  }
+}
+
+export default migration

src/bp/sdk/botpress.d.ts

@@ -740,6 +740,26 @@ declare module 'botpress/sdk' {
     fileExists(rootFolder: string, file: string): Promise<boolean>
   }
 
+  export interface KvsService {
+    /**
+     * Returns the specified key as JSON object
+     * @example bp.kvs.get('bot123', 'hello/whatsup')
+     */
+    get(key: string, path?: string): Promise<any>
+
+    /**
+     * Saves the specified key as JSON object
+     * @example bp.kvs.set('bot123', 'hello/whatsup', { msg: 'i love you' })
+     */
+    set(key: string, value: any, path?: string): Promise<void>
+    setStorageWithExpiry(key: string, value, expiryInMs?: string)
+    getStorageWithExpiry(key: string)
+    getConversationStorageKey(sessionId: string, variable: string): string
+    getUserStorageKey(userId: string, variable: string): string
+    getGlobalStorageKey(variable: string): string
+    removeStorageKeysStartingWith(key): Promise<void>
+  }
+
   export interface ListenHandle {
     /** Stops listening from the event */
     remove(): void
@@ -1329,22 +1349,57 @@ declare module 'botpress/sdk' {
    * The Key Value Store is perfect to store any type of data as JSON.
    */
   export namespace kvs {
+    /**
+     * Access the KVS Service for a specific bot. Check the {@link ScopedGhostService} for the operations available on the scoped element.
+     */
+    export function forBot(botId: string): KvsService
+    /**
+     * Access the KVS Service globally. Check the {@link ScopedGhostService} for the operations available on the scoped element.
+     */
+    export function global(): KvsService
+
     /**
      * Returns the specified key as JSON object
      * @example bp.kvs.get('bot123', 'hello/whatsup')
+     * @deprecated will be removed, use global or forBot
      */
     export function get(botId: string, key: string, path?: string): Promise<any>
 
     /**
      * Saves the specified key as JSON object
      * @example bp.kvs.set('bot123', 'hello/whatsup', { msg: 'i love you' })
+     * @deprecated will be removed, use global or forBot
      */
     export function set(botId: string, key: string, value: any, path?: string): Promise<void>
+
+    /**
+     * @deprecated will be removed, use global or forBot
+     */
     export function setStorageWithExpiry(botId: string, key: string, value, expiryInMs?: string)
+
+    /**
+     * @deprecated will be removed, use global or forBot
+     */
     export function getStorageWithExpiry(botId: string, key: string)
+
+    /**
+     * @deprecated will be removed, use global or forBot
+     */
     export function getConversationStorageKey(sessionId: string, variable: string): string
+
+    /**
+     * @deprecated will be removed, use global or forBot
+     */
     export function getUserStorageKey(userId: string, variable: string): string
+
+    /**
+     * @deprecated will be removed, use global or forBot
+     */
     export function getGlobalStorageKey(variable: string): string
+
+    /**
+     * @deprecated will be removed, use global or forBot
+     */
     export function removeStorageKeysStartingWith(key): Promise<void>
   }