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

DEVEXP-381: Flexible enums for regions #54

Merged
merged 10 commits into from
Apr 8, 2024
7 changes: 3 additions & 4 deletions examples/simple-examples/src/config.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {
getRegion,
SinchClient,
Region,
SmsRegion,
ConversationService,
FaxService,
NumbersService,
Expand Down Expand Up @@ -37,8 +36,8 @@ export const initSmsServiceWithProjectId = (): SmsService => {
const initSmsClient = (): Pick<SinchClient, 'sms'> => {
const servicePlanId = process.env.SINCH_SERVICE_PLAN_ID || '';
const apiToken = process.env.SINCH_API_TOKEN || '';
const region = getRegion(process.env.SMS_REGION) || Region.UNITED_STATES;
return new SinchClient({ servicePlanId, apiToken, region });
const smsRegion = process.env.SMS_REGION || SmsRegion.UNITED_STATES;
return new SinchClient({ servicePlanId, apiToken, smsRegion });
};

export const initSmsServiceWithServicePlanId = (): SmsService => {
Expand Down
36 changes: 12 additions & 24 deletions packages/conversation/src/rest/v1/conversation-domain-api.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
import {
Api,
ApiClient,
ApiClientOptions,
ApiFetchClient,
buildOAuth2ApiClientOptions,
ConversationRegion,
Oauth2TokenRequest,
Region,
ConversationRegionFlexible,
SinchClientParameters,
UnifiedCredentials,
} from '@sinch/sdk-client';
Expand Down Expand Up @@ -36,10 +35,10 @@ export class ConversationDomainApi implements Api {

/**
* Update the region in the basePath
* @param {Region} region - The new region to send the requests to
* @param {ConversationRegion} region - The new region to send the requests to
*/
public setRegion(region: Region) {
this.sinchClientParameters.region = region;
public setRegion(region: ConversationRegion) {
this.sinchClientParameters.conversationRegion = region;
if (this.client) {
this.client.apiClientOptions.hostname = this.buildHostname(region);
}
Expand Down Expand Up @@ -77,36 +76,25 @@ export class ConversationDomainApi implements Api {
*/
public getSinchClient(): ApiClient {
if (!this.client) {
const region = this.sinchClientParameters.region || Region.UNITED_STATES;
const region = this.sinchClientParameters.conversationRegion ?? ConversationRegion.UNITED_STATES;
if(!Object.values(ConversationRegion).includes((region as unknown) as ConversationRegion)) {
console.warn(`The region '${region}' is not supported for the Conversation API`);
console.warn(`The region "${region}" is not known as a supported region for the Conversation API`);
}
const apiClientOptions = this.buildApiClientOptions(this.sinchClientParameters);
const apiClientOptions = buildOAuth2ApiClientOptions(this.sinchClientParameters, 'Conversation');
this.client = new ApiFetchClient(apiClientOptions);
this.client.apiClientOptions.hostname = this.buildHostname(region);
}
return this.client;
}

private buildApiClientOptions(params: SinchClientParameters): ApiClientOptions {
if (!params.projectId || !params.keyId || !params.keySecret) {
throw new Error('Invalid configuration for the Conversation API: '
+ '"projectId", "keyId" and "keySecret" values must be provided');
}
return {
projectId: params.projectId,
requestPlugins: [new Oauth2TokenRequest( params.keyId, params.keySecret)],
useServicePlanId: false,
};
}

private buildHostname(region: Region) {
private buildHostname(region: ConversationRegionFlexible) {
const formattedRegion = region === '' ? region : `${region}.`;

Choose a reason for hiding this comment

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

I think it come from myself, but thank you to small function usage here: my first reading of this line was cause of reflexion to understand logic and thinking there was a issue:

  • testing against an empty value to use the testing value (the empty one)
  • if not empty: use it too....

🤔
Then I realized the final dot for second case; then use to format a string
Just a personal thought I think I would have written logic as
const formattedRegion = region !== '' ? `${region}.` '';
Which could highlight in a better way the logic and use case.
Again just a personal thought and not a blocker

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, it would be more readable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed for all 4 APIs that use regions

switch (this.apiName) {
case 'TemplatesV1Api':
case 'TemplatesV2Api':
return this.sinchClientParameters.conversationTemplatesHostname ?? `https://${region}.template.api.sinch.com`;
return this.sinchClientParameters.conversationTemplatesHostname ?? `https://${formattedRegion}template.api.sinch.com`;
default:
return this.sinchClientParameters.conversationHostname ?? `https://${region}.conversation.api.sinch.com`;
return this.sinchClientParameters.conversationHostname ?? `https://${formattedRegion}conversation.api.sinch.com`;
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/conversation/src/rest/v1/conversation-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export class ConversationService {

/**
* Update the default hostname for the Templates API
* @param {string} hostname - The new hostname to use for all the APIs.
* @param {string} hostname - The new hostname to use for the Templates APIs.
*/
public setTemplatesHostname(hostname: string) {
this.templatesV1.setHostname(hostname);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { ConversationDomainApi } from '../../../src/rest/v1/conversation-domain-api';
import { TemplatesV2Api } from '../../../src';
import { ApiHostname, Region, UnifiedCredentials } from '@sinch/sdk-client';
import { ApiHostname, ConversationRegion, UnifiedCredentials } from '@sinch/sdk-client';

describe('Conversation API', () => {
let conversationApi: ConversationDomainApi;
Expand All @@ -25,22 +25,20 @@ describe('Conversation API', () => {
});

it('should change the URL when specifying a different region', () => {
params.region = Region.EUROPE;
params.conversationRegion = ConversationRegion.EUROPE;
conversationApi = new ConversationDomainApi(params, 'dummy');
conversationApi.getSinchClient();
expect(conversationApi.client?.apiClientOptions.hostname).toBe('https://eu.conversation.api.sinch.com');
});

it('should log a warning when using an unsupported region', async () => {
params.region = Region.CANADA;
params.conversationRegion = 'bzh';
JPPortier marked this conversation as resolved.
Show resolved Hide resolved
conversationApi = new ConversationDomainApi(params, 'dummy');
const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
console.warn = jest.fn();
conversationApi.getSinchClient();
// Add a small delay to allow jest to capture the warning
setTimeout(() => {
expect(consoleWarnSpy).toHaveBeenCalledWith('The region \'ca\' is not supported for the Conversation API');
consoleWarnSpy.mockRestore();
}, 20);
expect(console.warn).toHaveBeenCalledWith(
'The region "bzh" is not known as a supported region for the Conversation API');
expect(conversationApi.client?.apiClientOptions.hostname).toBe('https://bzh.conversation.api.sinch.com');
});

it('should use the hostname parameter but not for templates', () => {
Expand Down Expand Up @@ -83,7 +81,7 @@ describe('Conversation API', () => {

it ('should update the region', () => {
conversationApi = new ConversationDomainApi(params, 'dummy');
conversationApi.setRegion(Region.EUROPE);
conversationApi.setRegion(ConversationRegion.EUROPE);
conversationApi.getSinchClient();
expect(conversationApi.client).toBeDefined();
expect(conversationApi.client?.apiClientOptions.hostname).toBe('https://eu.conversation.api.sinch.com');
Expand All @@ -94,7 +92,7 @@ describe('Conversation API', () => {
conversationApi.getSinchClient();
expect(conversationApi.client).toBeDefined();
expect(conversationApi.client?.apiClientOptions.hostname).toBe('https://us.template.api.sinch.com');
conversationApi.setRegion(Region.EUROPE);
conversationApi.setRegion(ConversationRegion.EUROPE);
expect(conversationApi.client?.apiClientOptions.hostname).toBe('https://eu.template.api.sinch.com');
});

Expand All @@ -103,7 +101,7 @@ describe('Conversation API', () => {
conversationApi.getSinchClient();
expect(conversationApi.client).toBeDefined();
expect(conversationApi.client?.apiClientOptions.hostname).toBe('https://us.template.api.sinch.com');
conversationApi.setRegion(Region.EUROPE);
conversationApi.setRegion(ConversationRegion.EUROPE);
expect(conversationApi.client?.apiClientOptions.hostname).toBe('https://eu.template.api.sinch.com');
});

Expand Down
25 changes: 9 additions & 16 deletions packages/fax/src/rest/v3/fax-domain-api.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import {
Api,
ApiClient,
ApiClientOptions,
ApiFetchClient,
buildOAuth2ApiClientOptions,
FaxRegion,
SinchClientParameters,
Oauth2TokenRequest,
UnifiedCredentials,
} from '@sinch/sdk-client';

Expand Down Expand Up @@ -59,23 +59,16 @@ export class FaxDomainApi implements Api {
*/
public getSinchClient(): ApiClient {
if (!this.client) {
const apiClientOptions = this.buildApiClientOptions(this.sinchClientParameters);
const apiClientOptions = buildOAuth2ApiClientOptions(this.sinchClientParameters, 'Fax');
this.client = new ApiFetchClient(apiClientOptions);
this.client.apiClientOptions.hostname = this.sinchClientParameters.faxHostname ?? 'https://fax.api.sinch.com';
const region = this.sinchClientParameters.faxRegion ?? FaxRegion.DEFAULT;
if(!Object.values(FaxRegion).includes((region as unknown) as FaxRegion)) {
console.warn(`The region "${region}" is not known as a supported region for the Fax API`);
}
const formattedRegion = region === FaxRegion.DEFAULT ? region : `${region}.`;
this.client.apiClientOptions.hostname = this.sinchClientParameters.faxHostname ?? `https://${formattedRegion}fax.api.sinch.com`;
}
return this.client;
}

private buildApiClientOptions(params: SinchClientParameters): ApiClientOptions {
if (!params.projectId || !params.keyId || !params.keySecret) {
throw new Error('Invalid configuration for the Fax API: '
+ '"projectId", "keyId" and "keySecret" values must be provided');
}
return {
projectId: params.projectId,
requestPlugins: [new Oauth2TokenRequest( params.keyId, params.keySecret)],
useServicePlanId: false,
};
}

}
19 changes: 18 additions & 1 deletion packages/fax/tests/rest/v3/fax-domain-api.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { FaxDomainApi } from '../../../src/rest/v3/fax-domain-api';
import { ApiHostname, UnifiedCredentials } from '@sinch/sdk-client';
import { ApiHostname, FaxRegion, UnifiedCredentials } from '@sinch/sdk-client';

describe('Fax API', () => {
let faxApi: FaxDomainApi;
Expand All @@ -21,6 +21,23 @@ describe('Fax API', () => {
expect(faxApi.client?.apiClientOptions.hostname).toBe('https://fax.api.sinch.com');
});

it('should change the URL when specifying a different region', () => {
params.faxRegion = FaxRegion.SOUTHEAST_ASIA_1;
faxApi = new FaxDomainApi(params, 'dummy');
faxApi.getSinchClient();
expect(faxApi.client?.apiClientOptions.hostname).toBe('https://apse1.fax.api.sinch.com');
});

it('should log a warning when using an unsupported region', async () => {
params.faxRegion = 'bzh';
faxApi = new FaxDomainApi(params, 'dummy');
console.warn = jest.fn();
faxApi.getSinchClient();
expect(console.warn).toHaveBeenCalledWith(
'The region "bzh" is not known as a supported region for the Fax API');
expect(faxApi.client?.apiClientOptions.hostname).toBe('https://bzh.fax.api.sinch.com');
});

it('should use the hostname parameter', () => {
params.faxHostname = CUSTOM_HOSTNAME;
faxApi = new FaxDomainApi(params, 'dummy');
Expand Down
18 changes: 4 additions & 14 deletions packages/numbers/src/rest/v1/numbers-domain-api.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import {
Api,
ApiClient, ApiClientOptions,
ApiClient,
ApiFetchClient,
buildOAuth2ApiClientOptions,
SinchClientParameters,
Oauth2TokenRequest, UnifiedCredentials,
UnifiedCredentials,
} from '@sinch/sdk-client';

export class NumbersDomainApi implements Api {
Expand Down Expand Up @@ -57,22 +58,11 @@ export class NumbersDomainApi implements Api {
*/
public getSinchClient(): ApiClient {
if (!this.client) {
const apiClientOptions = this.buildApiClientOptions(this.sinchClientParameters);
const apiClientOptions = buildOAuth2ApiClientOptions(this.sinchClientParameters, 'Numbers');
this.client = new ApiFetchClient(apiClientOptions);
this.client.apiClientOptions.hostname = this.sinchClientParameters.numbersHostname ?? 'https://numbers.api.sinch.com';
}
return this.client;
}

private buildApiClientOptions(params: SinchClientParameters): ApiClientOptions {
if (!params.projectId || !params.keyId || !params.keySecret) {
throw new Error('Invalid configuration for the Numbers API: '
+ '"projectId", "keyId" and "keySecret" values must be provided');
}
return {
projectId: params.projectId,
requestPlugins: [new Oauth2TokenRequest( params.keyId, params.keySecret)],
useServicePlanId: false,
};
}
}
Loading