-
Notifications
You must be signed in to change notification settings - Fork 283
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
fix: Azure Blob transcript key #4180
Conversation
Pull Request Test Coverage Report for Build 2165663337
💛 - Coveralls |
* Azure Blob sanitizeBlobKey bug fix * attempt to fix w/ backwards compatibility * clean up
* chore(deps): bump moment from 2.29.1 to 2.29.2 (#4184) Bumps [moment](https://github.com/moment/moment) from 2.29.1 to 2.29.2. - [Release notes](https://github.com/moment/moment/releases) - [Changelog](https://github.com/moment/moment/blob/develop/CHANGELOG.md) - [Commits](moment/moment@2.29.1...2.29.2) --- updated-dependencies: - dependency-name: moment dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): bump minimist from 1.2.5 to 1.2.6 (#4182) Bumps [minimist](https://github.com/substack/minimist) from 1.2.5 to 1.2.6. - [Release notes](https://github.com/substack/minimist/releases) - [Commits](https://github.com/substack/minimist/compare/1.2.5...1.2.6) --- updated-dependencies: - dependency-name: minimist dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): bump minimist (#4181) Bumps [minimist](https://github.com/substack/minimist) from 1.2.5 to 1.2.6. - [Release notes](https://github.com/substack/minimist/releases) - [Commits](https://github.com/substack/minimist/compare/1.2.5...1.2.6) --- updated-dependencies: - dependency-name: minimist dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Update update-versions script to support peerDependencies (#4167) * fix: Azure Blob transcript key (#4180) * Azure Blob sanitizeBlobKey bug fix * attempt to fix w/ backwards compatibility * clean up Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Joel Mut <[email protected]> Co-authored-by: Ram Fattah <[email protected]>
Hi @tracyboehrer and @ramfattah . I am trying to use the this from the I think the logActivity in the blobsTranscriptStore should also use What are your thoughts, can we create a new issue for this? |
@tariromukute The main issue is that what options are being used? Blobs options? The transcript middleware doesn't know anything at all about which Storage is being used (Memory, CosmosDB, Blobs, etc...). It only knows about TranscriptLogger. So while the BlobsTranscriptStore has an optional options argument, it's not known to the middleware. One option might be to implement a transcript logger that does have some knowledge about the store being used. Though perhaps a better option would be to create a BlobsTranscriptStore using a particular BlobsStorageOptions. |
Hi @tracyboehrer, I agree with you there, the transcript middleware doesn't know anything about the Storage. I always think it will be reperative work to implement that in the transcript middleware/logger. My suggestion is to update BlobsTranscriptStore so that it uses the configuration that it was created with. Say // index.js
const blobTranscriptStore = new BlobsTranscriptStore(BlobConnectionString, BlobTranscriptContainerName, { decodeTranscriptKey: true })
adapter.use(new TranscriptLoggerMiddleware(blobTranscriptStore)); We have passed export class BlobsTranscriptStore implements TranscriptStore {
private readonly _containerClient: ContainerClient;
private readonly _concurrency = Infinity;
private _initializePromise?: Promise<unknown>;
private _isDecodeTranscriptKey?: boolean = false;
/**
* Constructs a BlobsTranscriptStore instance.
*
* @param {string} connectionString Azure Blob Storage connection string
* @param {string} containerName Azure Blob Storage container name
* @param {BlobsTranscriptStoreOptions} options Other options for BlobsTranscriptStore
*/
constructor(connectionString: string, containerName: string, options?: BlobsTranscriptStoreOptions) {
z.object({ connectionString: z.string(), containerName: z.string() }).parse({
connectionString,
containerName,
});
this._containerClient = new ContainerClient(connectionString, containerName, options?.storagePipelineOptions);
this._isDecodeTranscriptKey = options?.decodeTranscriptKey;
// At most one promise at a time to be friendly to local emulator users
if (connectionString.trim() === 'UseDevelopmentStorage=true;') {
this._concurrency = 1;
}
}
} My suggestion is we update /**
* Log an activity to the transcript.
*
* @param {Activity} activity activity to log
* @param {BlobsTranscriptStoreOptions} options Optional settings for BlobsTranscriptStore
* @returns {Promise<void>} A promise representing the async operation.
*/
async logActivity(activity: Activity, options?: BlobsTranscriptStoreOptions): Promise<void> {
z.object({ activity: z.record(z.unknown()) }).parse({ activity });
await this._initialize();
const blob = this._containerClient.getBlockBlobClient(getBlobKey(activity, options));
const serialized = JSON.stringify(activity);
const metadata: Record<string, string> = {
FromId: activity.from.id,
RecipientId: activity.recipient.id,
};
if (activity.id) {
metadata.Id = activity.id;
}
if (activity.timestamp) {
metadata.Timestamp = activity.timestamp.toJSON();
}
await blob.upload(serialized, serialized.length, { metadata });
} In the change, we update await this._initialize();
if (this._isDecodeTranscriptKey && options && options.decodeTranscriptKey === undefined) {
options = { decodeTranscriptKey: true, ...options };
}
const blob = this._containerClient.getBlockBlobClient(getBlobKey(activity, options));
const serialized = JSON.stringify(activity); |
Fixes #4176
Description
Expecting Azure blob to creat transcript activity and follow the pattern of
container/{channelId]/{conversationId}/{Timestamp.ticks}-{activity.id}.json
instead it was creating directory
container%2F{channelId}%2F{conversationId}%2F{Timestamp.ticks}-{activid.id}.json
due to sanitizeBlobKey method using the encodeURIComponent function.Specific Changes
decodeTranscriptKey
boolean property inBlobsTranscriptStoreOptions
interface to return a new string representing the decoded version of the given encoded blob transcript key. This remains the default behavior to false but can be overridden by setting decodeTranscriptKey to true.Steps to reproduce
Run
git clone --branch ramfattah/blobTranscript https://github.com/microsoft/botbuilder-js.git
Run
yarn
andyarn build
in the root folder of SDKOpen
libraries/botbuilder-azure-blobs/tests/blobsTranscriptStore.test.js
fileconnectionString
andcontainerName
. Note, it will create the container automatically if the name doesn't already exist.decodeTranscriptKey
flag to trueIn terminal, run
mocha <path-to-blobsTranscriptStore.test.js
mocha /Users/ram/Documents/bot/botbuilder-js/libraries/botbuilder-azure-blobs/tests/blobsTranscriptStore.test.js
This will create blob transcript key in the container as the following pattern
container/{channelId]/{conversationId}/{Timestamp.ticks}-{activity.id}.json
Before
After