fix: AgentSettings Circular Structure and improve internals (#4626)

* Add circular structure detection and allow the agent settings to be customizable

* Fix lint, test:compat, and depcheck

* Improve comments

* Fix lint rule

libraries/botbuilder-core/etc/botbuilder-core.api.md

@@ -259,9 +259,6 @@ export class BrowserSessionStorage extends MemoryStorage {
     constructor();
 }
 
-// @public (undocumented)
-export const CACHED_BOT_STATE_SKIP_PROPERTIES_HANDLER_KEY: unique symbol;
-
 // @public
 export interface CachedBotState {
     hash: string;

libraries/botbuilder-core/src/botState.ts

@@ -25,8 +25,6 @@ export interface CachedBotState {
     hash: string;
 }
 
-export const CACHED_BOT_STATE_SKIP_PROPERTIES_HANDLER_KEY = Symbol('cachedBotStateSkipPropertiesHandler');
-
 /**
  * Base class for the frameworks state persistance scopes.
  *
@@ -40,7 +38,6 @@ export const CACHED_BOT_STATE_SKIP_PROPERTIES_HANDLER_KEY = Symbol('cachedBotSta
  */
 export class BotState implements PropertyManager {
     private stateKey = Symbol('state');
-    private skippedProperties = new Map();
 
     /**
      * Creates a new BotState instance.
@@ -77,13 +74,6 @@ export class BotState implements PropertyManager {
      */
     public load(context: TurnContext, force = false): Promise<any> {
         const cached: CachedBotState = context.turnState.get(this.stateKey);
-        if (!context.turnState.get(CACHED_BOT_STATE_SKIP_PROPERTIES_HANDLER_KEY)) {
-            context.turnState.set(
-                CACHED_BOT_STATE_SKIP_PROPERTIES_HANDLER_KEY,
-                (state: BotState, key: string, properties: string[] = null) =>
-                    state.skippedProperties.set(key, properties)
-            );
-        }
         if (force || !cached || !cached.state) {
             return Promise.resolve(this.storageKey(context)).then((key: string) => {
                 return this.storage.read([key]).then((items: StoreItems) => {
@@ -115,8 +105,7 @@ export class BotState implements PropertyManager {
      */
     public saveChanges(context: TurnContext, force = false): Promise<void> {
         let cached: CachedBotState = context.turnState.get(this.stateKey);
-        const state = this.skipProperties(cached?.state);
-        if (force || (cached && cached.hash !== calculateChangeHash(state))) {
+        if (force || (cached && cached.hash !== calculateChangeHash(cached?.state))) {
             return Promise.resolve(this.storageKey(context)).then((key: string) => {
                 if (!cached) {
                     cached = { state: {}, hash: '' };
@@ -127,19 +116,7 @@ export class BotState implements PropertyManager {
 
                 return this.storage.write(changes).then(() => {
                     // Update change hash and cache
-                    cached.hash = calculateChangeHash(state);
-
-                    // why does this happen in JS?  It doesn't in the other platforms.
-                    // the issue is related to 'skipProperties', which nulls out those
-                    // properties in 'cached'.  This replaces the state in 'context',
-                    // and could change the values of references.
-                    //
-                    // Should 'calculateChangeHash' also consider the skipProperties?
-                    //
-                    // See SkillDialog.sendToSkill
-                    //
-                    // At any rate, would not expect saveChanges to alter the members
-                    // of a class in use.
+                    cached.hash = calculateChangeHash(cached.state);
                     context.turnState.set(this.stateKey, cached);
                 });
             });
@@ -204,45 +181,4 @@ export class BotState implements PropertyManager {
 
         return typeof cached === 'object' && typeof cached.state === 'object' ? cached.state : undefined;
     }
-
-    /**
-     * Skips properties from the cached state object.
-     *
-     * @remarks Primarily used to skip properties before calculating the hash value in the calculateChangeHash function.
-     * @param state Dictionary of state values.
-     * @returns Dictionary of state values, without the skipped properties.
-     */
-    private skipProperties(state: CachedBotState['state']): CachedBotState['state'] {
-        if (!state || !this.skippedProperties.size) {
-            return state;
-        }
-
-        const skipHandler = (key: string) => {
-            if (this.skippedProperties.has(key)) {
-                return this.skippedProperties.get(key) ?? [key];
-            }
-        };
-
-        const inner = ([key, value], skip = []) => {
-            if (value === null || value === undefined || skip.includes(key)) {
-                return;
-            }
-
-            if (Array.isArray(value)) {
-                return value.map((e) => inner([null, e], skip));
-            }
-
-            if (typeof value !== 'object') {
-                return value.valueOf();
-            }
-
-            return Object.entries(value).reduce((acc, [k, v]) => {
-                const skipResult = skipHandler(k) ?? [];
-                acc[k] = inner([k, v], [...skip, ...skipResult]);
-                return acc;
-            }, {});
-        };
-
-        return inner([null, state]);
-    }
 }

libraries/botbuilder-core/src/storage.ts

@@ -6,7 +6,8 @@
  * Licensed under the MIT License.
  */
 import { TurnContext } from './turnContext';
-import { Assertion, assert } from 'botbuilder-stdlib';
+import { Assertion, assert, stringify } from 'botbuilder-stdlib';
+import { createHash } from 'crypto';
 
 /**
  * Callback to calculate a storage key.
@@ -115,10 +116,11 @@ export const assertStoreItems: Assertion<StoreItems> = (val, path) => {
  * @param item Item to calculate the change hash for.
  */
 export function calculateChangeHash(item: StoreItem): string {
-    const cpy: any = { ...item };
-    if (cpy.eTag) {
-        delete cpy.eTag;
-    }
+    // eslint-disable-next-line  @typescript-eslint/no-unused-vars
+    const { eTag, ...rest } = item;
 
-    return JSON.stringify(cpy);
+    const result = stringify(rest);
+    const hash = createHash('sha256', { encoding: 'utf-8' });
+    const hashed = hash.update(result).digest('hex');
+    return hashed;
 }

libraries/botbuilder-core/tests/botState.test.js

@@ -4,7 +4,6 @@ const {
     BotState,
     MemoryStorage,
     TestAdapter,
-    CACHED_BOT_STATE_SKIP_PROPERTIES_HANDLER_KEY,
 } = require('../');
 
 const receivedMessage = { text: 'received', type: 'message' };
@@ -146,35 +145,4 @@ describe(`BotState`, function () {
         let count = botState.createProperty('count', 1);
         assert(count !== undefined, `did not successfully create PropertyAccessor.`);
     });
-
-    it('should skip properties in saveChanges()', async function () {
-        // Setup storage base changes.
-        const clone = JSON.parse(JSON.stringify(houseSectionsSample));
-        delete clone.house.kitchen.refrigerator;
-        delete clone.house.kitchen.table;
-        delete clone.house.bedroom.closet.pants;
-        delete clone.house.kitchen.chair;
-        delete clone.house.bedroom.chair;
-        await storage.write({ [storageKey]: clone });
-        await botState.load(context, true);
-
-        // Update bot state.
-        const oldState = context.turnState.get(botState.stateKey);
-        const newState = { ...oldState, state: houseSectionsSample };
-        context.turnState.set(botState.stateKey, newState);
-
-        // Save changes into storage.
-        const skipProperties = context.turnState.get(CACHED_BOT_STATE_SKIP_PROPERTIES_HANDLER_KEY);
-        skipProperties(botState, 'house', ['refrigerator', 'table', 'pants']); // Multiple props.
-        skipProperties(botState, 'chair'); // Single prop (key used as prop).
-        await botState.saveChanges(context);
-        const updatedState = context.turnState.get(botState.stateKey);
-        const storageState = await storage.read([storageKey]);
-
-        // Hash state and storage info shouldn't have changed.
-        const expectedStorage = storageState[storageKey];
-        delete expectedStorage.eTag;
-        assert.equal(oldState.hash, updatedState.hash);
-        assert.deepStrictEqual(clone, expectedStorage);
-    });
 });

libraries/botbuilder-dialogs-adaptive-runtime-integration-express/package.json

@@ -42,6 +42,7 @@
     "botbuilder": "4.1.6",
     "botbuilder-dialogs-adaptive-runtime": "4.1.6",
     "botbuilder-dialogs-adaptive-runtime-core": "4.1.6",
+    "botframework-connector": "4.1.6",
     "express": "^4.17.1",
     "zod": "~1.11.17"
   }

libraries/botbuilder-dialogs-adaptive-runtime-integration-express/src/index.ts

@@ -9,6 +9,7 @@ import type { Server } from 'http';
 import type { ServiceCollection } from 'botbuilder-dialogs-adaptive-runtime-core';
 import { Configuration, getRuntimeServices } from 'botbuilder-dialogs-adaptive-runtime';
 import { json, urlencoded } from 'body-parser';
+import type { ConnectorClientOptions } from 'botframework-connector';
 
 // Explicitly fails checks for `""`
 const NonEmptyString = z.string().refine((str) => str.length > 0, { message: 'must be non-empty string' });
@@ -38,6 +39,12 @@ const TypedOptions = z.object({
      * Path inside applicationRoot that should be served as static files
      */
     staticDirectory: NonEmptyString,
+
+    /**
+     * Used when creating ConnectorClients.
+     */
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    connectorClientOptions: z.object({}).nonstrict() as z.ZodObject<any, any, ConnectorClientOptions>,
 });
 
 /**
@@ -51,6 +58,7 @@ const defaultOptions: Options = {
     skillsEndpointPrefix: '/api/skills',
     port: 3978,
     staticDirectory: 'wwwroot',
+    connectorClientOptions: {},
 };
 
 /**
@@ -65,7 +73,9 @@ export async function start(
     settingsDirectory: string,
     options: Partial<Options> = {}
 ): Promise<void> {
-    const [services, configuration] = await getRuntimeServices(applicationRoot, settingsDirectory);
+    const [services, configuration] = await getRuntimeServices(applicationRoot, settingsDirectory, {
+        connectorClientOptions: options.connectorClientOptions,
+    });
     const [, listen] = await makeApp(services, configuration, applicationRoot, options);
 
     listen();

libraries/botbuilder-dialogs-adaptive-runtime/src/index.ts

@@ -1,8 +1,7 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 
-import http from 'http';
-import https from 'https';
+/* eslint-disable @typescript-eslint/no-explicit-any */
 
 import * as z from 'zod';
 import fs from 'fs';
@@ -504,6 +503,7 @@ function registerQnAComponents(services: ServiceCollection, configuration: Confi
  *
  * @param applicationRoot absolute path to root of application
  * @param settingsDirectory directory where settings files are located
+ * @param defaultServices services to use as default
  * @returns service collection and configuration
  *
  * @remarks
@@ -542,27 +542,31 @@ function registerQnAComponents(services: ServiceCollection, configuration: Confi
  */
 export async function getRuntimeServices(
     applicationRoot: string,
-    settingsDirectory: string
+    settingsDirectory: string,
+    defaultServices?: Record<string, any>
 ): Promise<[ServiceCollection, Configuration]>;
 
 /**
  * Construct all runtime services.
  *
  * @param applicationRoot absolute path to root of application
  * @param configuration a fully initialized configuration instance to use
+ * @param defaultServices services to use as default
  * @returns service collection and configuration
  */
 export async function getRuntimeServices(
     applicationRoot: string,
-    configuration: Configuration
+    configuration: Configuration,
+    defaultServices?: Record<string, any>
 ): Promise<[ServiceCollection, Configuration]>;
 
 /**
  * @internal
  */
 export async function getRuntimeServices(
     applicationRoot: string,
-    configurationOrSettingsDirectory: Configuration | string
+    configurationOrSettingsDirectory: Configuration | string,
+    defaultServices: Record<string, any> = {}
 ): Promise<[ServiceCollection, Configuration]> {
     // Resolve configuration
     let configuration: Configuration;
@@ -587,28 +591,14 @@ export async function getRuntimeServices(
 
     const services = new ServiceCollection({
         botFrameworkClientFetch: undefined,
-        connectorClientOptions: {
-            agentSettings: {
-                http: new http.Agent({
-                    keepAlive: true,
-                    maxSockets: 128,
-                    maxFreeSockets: 32,
-                    timeout: 60000,
-                }),
-                https: new https.Agent({
-                    keepAlive: true,
-                    maxSockets: 128,
-                    maxFreeSockets: 32,
-                    timeout: 60000,
-                }),
-            },
-        },
+        connectorClientOptions: undefined,
         customAdapters: new Map(),
         declarativeTypes: [],
         memoryScopes: [],
         middlewares: new MiddlewareSet(),
         pathResolvers: [],
         serviceClientCredentialsFactory: undefined,
+        ...defaultServices,
     });
 
     services.addFactory<ResourceExplorer, { declarativeTypes: ComponentDeclarativeTypes[] }>(

libraries/botbuilder-dialogs-adaptive/src/actions/beginSkill.ts

@@ -5,13 +5,7 @@
  * Copyright (c) Microsoft Corporation. All rights reserved.
  * Licensed under the MIT License.
  */
-import {
-    Activity,
-    ActivityTypes,
-    StringUtils,
-    TurnContext,
-    CACHED_BOT_STATE_SKIP_PROPERTIES_HANDLER_KEY,
-} from 'botbuilder';
+import { Activity, ActivityTypes, StringUtils, TurnContext } from 'botbuilder';
 import { ActivityTemplate } from '../templates';
 import { ActivityTemplateConverter } from '../converters';
 import { AdaptiveEvents } from '../adaptiveEvents';
@@ -156,7 +150,17 @@ export class BeginSkill extends SkillDialog implements BeginSkillConfiguration {
      * @param options Optional options used to configure the skill dialog.
      */
     constructor(options?: SkillDialogOptions) {
-        super(Object.assign({ skill: {} } as SkillDialogOptions, options));
+        super(
+            Object.assign({ skill: {} } as SkillDialogOptions, options, {
+                // This is an alternative to the toJSON function because when the SkillDialogOptions are saved into the Storage,
+                // when the information is retrieved, it doesn't have the properties that were declared in the toJSON function.
+                _replace(): Omit<SkillDialogOptions, 'conversationState' | 'skillClient' | 'conversationIdFactory'> {
+                    // eslint-disable-next-line @typescript-eslint/no-unused-vars
+                    const { conversationState, skillClient, conversationIdFactory, ...rest } = this;
+                    return rest;
+                },
+            })
+        );
     }
 
     /**
@@ -201,10 +205,6 @@ export class BeginSkill extends SkillDialog implements BeginSkillConfiguration {
 
         // Store the initialized dialogOptions in state so we can restore these values when the dialog is resumed.
         dc.activeDialog.state[this._dialogOptionsStateKey] = this.dialogOptions;
-        // Skip properties from the bot's state cache hash due to unwanted conversationState behavior.
-        const skipProperties = dc.context.turnState.get(CACHED_BOT_STATE_SKIP_PROPERTIES_HANDLER_KEY);
-        const props: (keyof SkillDialogOptions)[] = ['conversationIdFactory', 'conversationState', 'skillClient'];
-        skipProperties(this.dialogOptions.conversationState, this._dialogOptionsStateKey, props);
 
         // Get the activity to send to the skill.
         options = {} as BeginSkillDialogOptions;

libraries/botbuilder-stdlib/src/index.ts

@@ -6,3 +6,4 @@ export * from './types';
 export { delay } from './delay';
 export { maybeCast } from './maybeCast';
 export { retry } from './retry';
+export { stringify } from './stringify';