Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: AgentSettings Circular Structure and improve internals #412

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions libraries/botbuilder-core/etc/botbuilder-core.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -277,9 +277,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;
Expand Down
54 changes: 2 additions & 52 deletions libraries/botbuilder-core/src/botState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand All @@ -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.
Expand Down Expand Up @@ -81,11 +78,6 @@ export class BotState implements PropertyManager {
*/
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, (key, properties) =>
this.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) => {
Expand Down Expand Up @@ -118,8 +110,7 @@ export class BotState implements PropertyManager {
*/
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: '' };
Expand All @@ -130,7 +121,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);
context.turnState.set(this.stateKey, cached);
});
});
Expand Down Expand Up @@ -198,45 +189,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]);
}
}
13 changes: 8 additions & 5 deletions libraries/botbuilder-core/src/storage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
*/

import * as z from 'zod';
import { createHash } from 'crypto';
import { stringify } from 'botbuilder-stdlib';
import { TurnContext } from './turnContext';

/**
Expand Down Expand Up @@ -127,10 +129,11 @@ export function assertStoreItems(val: unknown, ..._args: unknown[]): asserts val
* @returns change hash string
*/
export function calculateChangeHash(item: StoreItem): string {
const cpy = { ...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;
}
76 changes: 1 addition & 75 deletions libraries/botbuilder-core/tests/botState.test.js
Original file line number Diff line number Diff line change
@@ -1,52 +1,9 @@
const assert = require('assert');
const {
TurnContext,
BotState,
MemoryStorage,
TestAdapter,
CACHED_BOT_STATE_SKIP_PROPERTIES_HANDLER_KEY,
} = require('../');
const { TurnContext, BotState, MemoryStorage, TestAdapter } = require('../');

const receivedMessage = { text: 'received', type: 'message' };
const storageKey = 'stateKey';

const houseSectionsSample = {
house: {
kitchen: {
refrigerator: {
fridge: 1,
freezer: 1,
},
chair: 6,
table: 1,
},
bathroom: {
toilet: 1,
shower: {
showerhead: 1,
bathtub: 1,
shampoo: 2,
towel: 3,
},
},
bedroom: {
chair: 1,
bed: {
pillow: 3,
sheet: 1,
duvet: 1,
},
closet: {
hanger: {
shirt: 6,
},
shoes: 4,
pants: 5,
},
},
},
};

function cachedState(context, stateKey) {
const cached = context.turnState.get(stateKey);
return cached ? cached.state : undefined;
Expand Down Expand Up @@ -149,35 +106,4 @@ describe('BotState', function () {
const 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('house', ['refrigerator', 'table', 'pants']); // Multiple props.
skipProperties('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);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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.3",
"zod": "^3.22.4"
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import type { ActivityHandlerBase, BotFrameworkHttpAdapter, ChannelServiceRoutes
import type { Server } from 'http';
import type { ServiceCollection } from 'botbuilder-dialogs-adaptive-runtime-core';
import { Configuration, getRuntimeServices } from 'botbuilder-dialogs-adaptive-runtime';
import type { ConnectorClientOptions } from 'botframework-connector';
import { json, urlencoded } from 'body-parser';

// Explicitly fails checks for `""`
Expand Down Expand Up @@ -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({}) as z.ZodObject<any, any, any, ConnectorClientOptions>,
});

/**
Expand All @@ -51,6 +58,7 @@ const defaultOptions: Options = {
skillsEndpointPrefix: '/api/skills',
port: 3978,
staticDirectory: 'wwwroot',
connectorClientOptions: {},
};

/**
Expand All @@ -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();
Expand Down
14 changes: 11 additions & 3 deletions libraries/botbuilder-dialogs-adaptive-runtime/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

/* eslint-disable @typescript-eslint/no-explicit-any */

import * as z from 'zod';
import fs from 'fs';
import path from 'path';
Expand Down Expand Up @@ -529,6 +531,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
Expand Down Expand Up @@ -567,27 +570,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;
Expand Down Expand Up @@ -619,6 +626,7 @@ export async function getRuntimeServices(
middlewares: new MiddlewareSet(),
pathResolvers: [],
serviceClientCredentialsFactory: undefined,
...defaultServices,
});

services.addFactory<ResourceExplorer, { declarativeTypes: ComponentDeclarativeTypes[] }>(
Expand Down
24 changes: 12 additions & 12 deletions libraries/botbuilder-dialogs-adaptive/src/actions/beginSkill.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -162,7 +156,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.
_replacer(): Omit<SkillDialogOptions, 'conversationState' | 'skillClient' | 'conversationIdFactory'> {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { conversationState, skillClient, conversationIdFactory, ...rest } = this;
return rest;
},
})
);
}

/**
Expand Down Expand Up @@ -208,10 +212,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._dialogOptionsStateKey, props);

// Get the activity to send to the skill.
options = {} as BeginSkillDialogOptions;
Expand Down
1 change: 1 addition & 0 deletions libraries/botbuilder-stdlib/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@ export * as stringExt from './stringExt';
export { delay } from './delay';
export { maybeCast } from './maybeCast';
export { retry } from './retry';
export { stringify } from './stringify';
Loading
Loading