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.

Sign up for GitHub

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

Revert breaking changes around BotFrameworkClient implementations #1894

Merged
merged 4 commits into from
Mar 12, 2020
Merged

Revert breaking changes around BotFrameworkClient implementations #1894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a704a15
change BotFrameworkClient to interface
stevengum Mar 12, 2020
c415198
fix breaking change in SkillHttpClient
stevengum Mar 12, 2020
c01518d
Merge branch '4.8' into stgum/client
stevengum Mar 12, 2020
70e7b94
fix private isEocComingFromParent in dialogHelper.ts
stevengum Mar 12, 2020
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
6 changes: 5 additions & 1 deletion libraries/botbuilder-core/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,11 @@ export * from './propertyManager';
export * from './recognizerResult';
export * from './showTypingMiddleware';
export * from './signInConstants';
export { BotFrameworkSkill, BotFrameworkClient, SkillConversationIdFactoryBase, SkillConversationReference,
export {
BotFrameworkSkill,
BotFrameworkClient,
SkillConversationIdFactoryBase,
SkillConversationReference,
SkillConversationIdFactoryOptions } from './skills';
export * from './skypeMentionNormalizeMiddleware';
export * from './statusCodes';
Expand Down
4 changes: 2 additions & 2 deletions libraries/botbuilder-core/src/skills/botFrameworkClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import { Activity } from 'botframework-schema';
import { InvokeResponse } from '../invokeResponse';

export abstract class BotFrameworkClient {
export interface BotFrameworkClient {
/**
* Forwards an activity to a another bot.
* @remarks
Expand All @@ -21,5 +21,5 @@ export abstract class BotFrameworkClient {
* @param conversationId A conversation ID to use for the conversation with the skill.
* @param activity Activity to forward.
*/
public abstract postActivity<T>(fromBotId: string, toBotId: string, toUrl: string, serviceUrl: string, conversationId: string, activity: Activity): Promise<InvokeResponse<T>>
postActivity: (<T>(fromBotId: string, toBotId: string, toUrl: string, serviceUrl: string, conversationId: string, activity: Activity) => Promise<InvokeResponse<T>>) | ((fromBotId: string, toBotId: string, toUrl: string, serviceUrl: string, conversationId: string, activity: Activity) => Promise<InvokeResponse>);
}
2 changes: 1 addition & 1 deletion libraries/botbuilder-dialogs/src/dialogHelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,5 +91,5 @@ function getActiveDialogContext(dialogContext: DialogContext): DialogContext {
function isEocComingFromParent(context: TurnContext): boolean {
// To determine the direction we check callerId property which is set to the parent bot
// by the BotFrameworkHttpClient on outgoing requests.
return !(!!context.activity.callerId);
return !!context.activity.callerId;
}
17 changes: 12 additions & 5 deletions libraries/botbuilder/src/botFrameworkHttpClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import axios from 'axios';
import { Activity, BotFrameworkClient, InvokeResponse } from 'botbuilder-core';
import {
AuthenticationConstants,
AppCredentials,
GovernmentConstants,
ICredentialProvider,
JwtTokenValidation,
Expand All @@ -21,7 +22,7 @@ import { USER_AGENT } from './botFrameworkAdapter';
/**
* HttpClient for calling skills from a Node.js BotBuilder V4 SDK bot.
*/
export class BotFrameworkHttpClient extends BotFrameworkClient {
export class BotFrameworkHttpClient implements BotFrameworkClient {

protected readonly channelService: string;

Expand All @@ -33,7 +34,6 @@ export class BotFrameworkHttpClient extends BotFrameworkClient {
private readonly credentialProvider: ICredentialProvider;

public constructor(credentialProvider: ICredentialProvider, channelService?: string) {
super();
if (!credentialProvider) {
throw new Error('BotFrameworkHttpClient(): missing credentialProvider');
}
Expand All @@ -46,19 +46,26 @@ export class BotFrameworkHttpClient extends BotFrameworkClient {
* Forwards an activity to a another bot.
* @remarks
*
* @template T The type of body in the InvokeResponse.
* @param fromBotId The MicrosoftAppId of the bot sending the activity.
* @param toBotId The MicrosoftAppId of the bot receiving the activity.
* @param toUrl The URL of the bot receiving the activity.
* @param serviceUrl The callback Url for the skill host.
* @param conversationId A conversation ID to use for the conversation with the skill.
* @param activity Activity to forward.
*/
public async postActivity<T>(fromBotId: string, toBotId: string, toUrl: string, serviceUrl: string, conversationId: string, activity: Activity): Promise<InvokeResponse<T>> {
public async postActivity<T>(fromBotId: string, toBotId: string, toUrl: string, serviceUrl: string, conversationId: string, activity: Activity): Promise<InvokeResponse<T>>
public async postActivity(fromBotId: string, toBotId: string, toUrl: string, serviceUrl: string, conversationId: string, activity: Activity): Promise<InvokeResponse>
public async postActivity<T = any>(fromBotId: string, toBotId: string, toUrl: string, serviceUrl: string, conversationId: string, activity: Activity): Promise<InvokeResponse<T>> {
const appCredentials = await this.getAppCredentials(fromBotId, toBotId);
if (!appCredentials) {
throw new Error('BotFrameworkHttpClient.postActivity(): Unable to get appCredentials to connect to the skill');
}

if (!activity) {
throw new Error('BotFrameworkHttpClient.postActivity(): missing activity');
}

if (activity.conversation === undefined) {
throw new Error('BotFrameworkHttpClient.postActivity(): Activity must have a ConversationReference');
}
Expand Down Expand Up @@ -116,7 +123,7 @@ export class BotFrameworkHttpClient extends BotFrameworkClient {
}
}

protected async buildCredentials(appId: string, oAuthScope?: string): Promise<MicrosoftAppCredentials> {
protected async buildCredentials(appId: string, oAuthScope?: string): Promise<AppCredentials> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just confirming that 4.7 doesn't have a buildCredentials method with MicrosoftAPpCredentials as a return type. Protected is still public for folks who extend the API.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically this is loosening the the API as MicrosoftAppCredentials subclasses AppCredentials...

But this technically is a breaking change if users now have to cast the return value from their code... Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional caveat, this makes the signature the same as C#: link

const appPassword = await this.credentialProvider.getAppPassword(appId);
let appCredentials;
if (JwtTokenValidation.isGovernment(this.channelService)) {
Expand Down Expand Up @@ -147,7 +154,7 @@ export class BotFrameworkHttpClient extends BotFrameworkClient {
}

// Credentials not found in cache, build them
appCredentials = await this.buildCredentials(appId, oAuthScope);
appCredentials = await this.buildCredentials(appId, oAuthScope) as MicrosoftAppCredentials;

// Cache the credentials for later use
BotFrameworkHttpClient.appCredentialMapCache.set(cacheKey, appCredentials);
Expand Down
29 changes: 18 additions & 11 deletions libraries/botbuilder/src/skills/skillHttpClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ import { BotFrameworkHttpClient } from '../botFrameworkHttpClient';
* A BotFrameworkHttpClient specialized for Skills that encapsulates Conversation ID generation.
*/
export class SkillHttpClient extends BotFrameworkHttpClient {

private readonly conversationIdFactory: SkillConversationIdFactoryBase;

public constructor(credentialProvider: ICredentialProvider, conversationIdFactory: SkillConversationIdFactoryBase, channelService?: string) {
super(credentialProvider, channelService);
if (!conversationIdFactory) {
Expand All @@ -24,19 +24,26 @@ export class SkillHttpClient extends BotFrameworkHttpClient {
this.conversationIdFactory = conversationIdFactory;
}


/**
*
* @param originatingAudience
* @param fromBotId
* @param toSkill
* @param callbackUrl
* @param activity
* Uses the SkillConversationIdFactory to create or retrieve a Skill Conversation Id, and sends the activity.
* @template T The type of body in the InvokeResponse.
* @param originatingAudience The OAuth audience scope, used during token retrieval. (Either https://api.botframework.com or bot app id.)
* @param fromBotId The MicrosoftAppId of the bot sending the activity.
* @param toSkill The skill to create the Conversation Id for.
* @param callbackUrl The callback Url for the skill host.
* @param activity The activity to send.
*/
public async postToSkill<T>(originatingAudience: string, fromBotId: string, toSkill: BotFrameworkSkill, callbackUrl: string, activity: Activity): Promise<InvokeResponse<T>>;
public async postToSkill<T>(fromBotId: string, toSkill: BotFrameworkSkill, callbackUrl: string, activity: Activity): Promise<InvokeResponse>;
public async postToSkill<T>(audienceOrFromBotId: string, fromBotIdOrSkill: string | BotFrameworkSkill, toSkillOrCallbackUrl: BotFrameworkSkill | string, callbackUrlOrActivity: string | Activity, activityToForward?: Activity): Promise<InvokeResponse<T>> {

/**
* Uses the SkillConversationIdFactory to create or retrieve a Skill Conversation Id, and sends the activity.
* @deprecated This overload is deprecated. Please use SkillHttpClient.postToSkill() that takes an `originatingAudience`.
* @param fromBotId The MicrosoftAppId of the bot sending the activity.
* @param toSkill The skill to create the Conversation Id for.
* @param callbackUrl The callback Url for the skill host.
* @param activity The activity to send.
*/
public async postToSkill(fromBotId: string, toSkill: BotFrameworkSkill, callbackUrl: string, activity: Activity): Promise<InvokeResponse>;
public async postToSkill<T = any>(audienceOrFromBotId: string, fromBotIdOrSkill: string | BotFrameworkSkill, toSkillOrCallbackUrl: BotFrameworkSkill | string, callbackUrlOrActivity: string | Activity, activityToForward?: Activity): Promise<InvokeResponse<T>> {
let originatingAudience: string;
let fromBotId: string;
if (typeof fromBotIdOrSkill === 'string') {
Expand Down