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(cached?.state))) {
-        if (force || (cached && cached.hash !== calculateChangeHash(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);
+                    cached.hash = calculateChangeHash(cached.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.
                     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 };
+    // eslint-disable-next-line  @typescript-eslint/no-unused-vars
-    if (cpy.eTag) {
+    const { eTag, ...rest } = item;
-        delete cpy.eTag;
-    }
 
-    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';
+/* eslint-disable @typescript-eslint/no-explicit-any */
-import https from 'https';
 
 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,7 +542,8 @@ function registerQnAComponents(services: ServiceCollection, configuration: Confi
  */
 export async function getRuntimeServices(
     applicationRoot: string,
-    settingsDirectory: string
+    settingsDirectory: string,
+    defaultServices?: Record<string, any>
 ): Promise<[ServiceCollection, Configuration]>;
 
 /**
@@ -550,11 +551,13 @@ export async function getRuntimeServices(
  *
  * @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]>;
 
 /**
@@ -562,7 +565,8 @@ export async function getRuntimeServices(
  */
 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: {
+        connectorClientOptions: undefined,
-            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,
-                }),
-            },
-        },
         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 {
+import { Activity, ActivityTypes, StringUtils, TurnContext } from 'botbuilder';
-    Activity,
-    ActivityTypes,
-    StringUtils,
-    TurnContext,
-    CACHED_BOT_STATE_SKIP_PROPERTIES_HANDLER_KEY,
-} 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';

libraries/botbuilder-stdlib/src/stringify.ts

@@ -0,0 +1,71 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+/* eslint-disable @typescript-eslint/no-explicit-any */
+
+/**
+ * Encapsulates JSON.stringify function to detect and handle different types of errors (eg. Circular Structure).
+ * @remarks
+ * Circular Structure:
+ *   - It detects when the provided value has circular references and replaces them with [Circular *.{path to the value being referenced}].
+ * @example
+ * // Circular Structure:
+ *     {
+ *       "item": {
+ *         "name": "parent",
+ *         "parent": null,
+ *         "child": {
+ *           "name": "child",
+ *           "parent": "[Circular *.item]" // => obj.item.child.parent = obj.item
+ *         }
+ *       }
+ *     }
+ *
+ * @param value — A JavaScript value, usually an object or array, to be converted.
+ * @param replacer — A function that transforms the results.
+ * @param space — Adds indentation, white space, and line break characters to the return-value JSON text to make it easier to read.
+ * @returns {string} The converted JavaScript value to a JavaScript Object Notation (JSON) string.
+ */
+export function stringify(value: any, replacer?: (key: string, value: any) => any, space?: string | number): string {
+    if (!value) {
+        return '';
+    }
+
+    try {
+        return JSON.stringify(value, stringifyReplacer(replacer), space);
+    } catch (error) {
+        if (!error?.message.includes('circular structure')) {
+            throw error;
+        }
+
+        const seen = new WeakMap();
+        return JSON.stringify(
+            value,
+            function stringifyCircularReplacer(key, val) {
+                const value = stringifyReplacer(replacer)(key, val);
+
+                const path = seen.get(value);
+                if (path) {
+                    return `[Circular *${path.join('.')}]`;
+                }
+
+                const parent = seen.get(this) ?? [];
+                seen.set(value, [...parent, key]);
+                return value;
+            },
+            space
+        );
+    }
+}
+
+function stringifyReplacer(replacer?: (key: string, value: any) => any) {
+    return function stringifyReplacer(this: any, key: string, val: any) {
+        const replacerValue = replacer ? replacer(key, val).bind(this) : val;
+        if (replacerValue === null || replacerValue === undefined || typeof replacerValue !== 'object') {
+            return replacerValue;
+        }
+
+        const toJSONValue = replacerValue.toJSON ? replacerValue.toJSON(key) : replacerValue;
+        return toJSONValue._replace ? toJSONValue._replace() : toJSONValue;
+    };
+}

libraries/botframework-connector/src/auth/botFrameworkClientImpl.ts

@@ -4,34 +4,39 @@
 import axios from 'axios';
 import { Activity, ChannelAccount, InvokeResponse, RoleTypes } from 'botframework-schema';
 import { BotFrameworkClient } from '../skills';
+import { ConnectorClientOptions } from '../connectorApi/models';
 import { ConversationIdHttpHeaderName } from '../conversationConstants';
 import { ServiceClientCredentialsFactory } from './serviceClientCredentialsFactory';
 import { USER_AGENT } from './connectorFactoryImpl';
 import { WebResource } from '@azure/ms-rest-js';
 import { assert } from 'botbuilder-stdlib';
-import { AgentSettings } from '@azure/ms-rest-js/es/lib/serviceClient';
 
-const botFrameworkClientFetchImpl = async (
+const botFrameworkClientFetchImpl = (connectorClientOptions: ConnectorClientOptions) => {
-    input: RequestInfo,
+    const { http: httpAgent, https: httpsAgent } = connectorClientOptions?.agentSettings ?? {
-    init?: RequestInit,
+        http: undefined,
-    agentSettings?: AgentSettings
+        https: undefined,
-): Promise<Response> => {
-    const config = {
-        headers: init.headers as Record<string, string>,
-        validateStatus: (): boolean => true,
-        httpAgent: agentSettings?.http,
-        httpsAgent: agentSettings?.https,
     };
+    const axiosInstance = axios.create({
+        httpAgent,
+        httpsAgent,
+        validateStatus: (): boolean => true,
+    });
 
-    assert.string(input, ['input']);
+    return async (input: RequestInfo, init?: RequestInit): Promise<Response> => {
-    assert.string(init.body, ['init']);
+        const config = {
-    const activity = JSON.parse(init.body) as Activity;
+            headers: init.headers as Record<string, string>,
+        };
 
-    const response = await axios.post(input, activity, config);
+        assert.string(input, ['input']);
-    return {
+        assert.string(init.body, ['init']);
-        status: response.status,
+        const activity = JSON.parse(init.body) as Activity;
-        json: async () => response.data,
+
-    } as Response;
+        const response = await axiosInstance.post(input, activity, config);
+        return {
+            status: response.status,
+            json: async () => response.data,
+        } as Response;
+    };
 };
 
 // Internal
@@ -39,14 +44,23 @@ export class BotFrameworkClientImpl implements BotFrameworkClient {
     constructor(
         private readonly credentialsFactory: ServiceClientCredentialsFactory,
         private readonly loginEndpoint: string,
-        private readonly botFrameworkClientFetch: (
+        private readonly botFrameworkClientFetch?: (
             input: RequestInfo,
             init?: RequestInit,
-            agentSettings?: AgentSettings
+            options?: ConnectorClientOptions
-        ) => Promise<Response> = botFrameworkClientFetchImpl,
+        ) => Promise<Response>,
-        private readonly agentSettings?: AgentSettings
+        private readonly connectorClientOptions?: ConnectorClientOptions
     ) {
         assert.maybeFunc(botFrameworkClientFetch, ['botFrameworkClientFetch']);
+
+        this.botFrameworkClientFetch ??= botFrameworkClientFetchImpl(this.connectorClientOptions);
+    }
+
+    private toJSON() {
+        // Ignore ConnectorClientOptions, as it could contain Circular Structure behavior.
+        // eslint-disable-next-line @typescript-eslint/no-unused-vars
+        const { connectorClientOptions, ...rest } = this;
+        return rest;
     }
 
     async postActivity<T>(
@@ -116,7 +130,7 @@ export class BotFrameworkClientImpl implements BotFrameworkClient {
                 body: request.body,
                 headers: request.headers.rawHeaders(),
             };
-            const response = await this.botFrameworkClientFetch(request.url, config, this.agentSettings);
+            const response = await this.botFrameworkClientFetch(request.url, config, this.connectorClientOptions);
 
             return { status: response.status, body: await response.json() };
         } finally {

libraries/botframework-connector/src/auth/parameterizedBotFrameworkAuthentication.ts

@@ -145,7 +145,7 @@ export class ParameterizedBotFrameworkAuthentication extends BotFrameworkAuthent
             this.credentialsFactory,
             this.toChannelFromBotLoginUrl,
             this.botFrameworkClientFetch,
-            this.connectorClientOptions?.agentSettings
+            this.connectorClientOptions
         );
     }