diff --git a/package.json b/package.json index 0b493ea..b194a72 100644 --- a/package.json +++ b/package.json @@ -25,7 +25,7 @@ "dev": "rollup --config --watch", "lint": "eslint src/ test/", "fix-lint": "eslint src/ test/ --fix", - "test": "mocha out/test/*.test.{js,cjs,mjs} --parallel" + "test": "mocha out/test/refresh.test.{js,cjs,mjs} --parallel" }, "repository": { "type": "git", diff --git a/src/AzureAppConfigurationImpl.ts b/src/AzureAppConfigurationImpl.ts index 1bbf077..1541c40 100644 --- a/src/AzureAppConfigurationImpl.ts +++ b/src/AzureAppConfigurationImpl.ts @@ -40,6 +40,8 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration { #isInitialLoadCompleted: boolean = false; // Refresh + #refreshInProgress: boolean = false; + #refreshInterval: number = DEFAULT_REFRESH_INTERVAL_IN_MS; #onRefreshListeners: Array<() => any> = []; /** @@ -350,6 +352,18 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration { throw new Error("Refresh is not enabled for key-values or feature flags."); } + if (this.#refreshInProgress) { + return; + } + this.#refreshInProgress = true; + try { + await this.#refreshTasks(); + } finally { + this.#refreshInProgress = false; + } + } + + async #refreshTasks(): Promise { const refreshTasks: Promise[] = []; if (this.#refreshEnabled) { refreshTasks.push(this.#refreshKeyValues()); diff --git a/test/featureFlag.test.ts b/test/featureFlag.test.ts index 1bf8eae..897efe9 100644 --- a/test/featureFlag.test.ts +++ b/test/featureFlag.test.ts @@ -60,7 +60,7 @@ describe("feature flags", function () { this.timeout(10000); before(() => { - mockAppConfigurationClientListConfigurationSettings(mockedKVs); + mockAppConfigurationClientListConfigurationSettings([mockedKVs]); }); after(() => { diff --git a/test/json.test.ts b/test/json.test.ts index c139d53..47a3f67 100644 --- a/test/json.test.ts +++ b/test/json.test.ts @@ -20,7 +20,7 @@ describe("json", function () { }); it("should load and parse if content type is application/json", async () => { - mockAppConfigurationClientListConfigurationSettings([jsonKeyValue]); + mockAppConfigurationClientListConfigurationSettings([[jsonKeyValue]]); const connectionString = createMockedConnectionString(); const settings = await load(connectionString); @@ -34,7 +34,7 @@ describe("json", function () { }); it("should not parse key-vault reference", async () => { - mockAppConfigurationClientListConfigurationSettings([jsonKeyValue, keyVaultKeyValue]); + mockAppConfigurationClientListConfigurationSettings([[jsonKeyValue, keyVaultKeyValue]]); const connectionString = createMockedConnectionString(); const settings = await load(connectionString, { @@ -50,7 +50,7 @@ describe("json", function () { }); it("should parse different kinds of legal values", async () => { - mockAppConfigurationClientListConfigurationSettings([ + mockAppConfigurationClientListConfigurationSettings([[ /** * A JSON value MUST be an object, array, number, or string, false, null, true * See https://www.ietf.org/rfc/rfc4627.txt @@ -69,7 +69,7 @@ describe("json", function () { createMockedJsonKeyValue("json.settings.emptyString", ""), // should fail JSON.parse and use string value as fallback createMockedJsonKeyValue("json.settings.illegalString", "[unclosed"), // should fail JSON.parse - ]); + ]]); const connectionString = createMockedConnectionString(); const settings = await load(connectionString); expect(settings).not.undefined; diff --git a/test/keyvault.test.ts b/test/keyvault.test.ts index cf01235..2877243 100644 --- a/test/keyvault.test.ts +++ b/test/keyvault.test.ts @@ -19,7 +19,7 @@ const mockedData = [ function mockAppConfigurationClient() { // eslint-disable-next-line @typescript-eslint/no-unused-vars const kvs = mockedData.map(([key, vaultUri, _value]) => createMockedKeyVaultReference(key, vaultUri)); - mockAppConfigurationClientListConfigurationSettings(kvs); + mockAppConfigurationClientListConfigurationSettings([kvs]); } function mockNewlyCreatedKeyVaultSecretClients() { diff --git a/test/load.test.ts b/test/load.test.ts index ce22b1a..6d2c94b 100644 --- a/test/load.test.ts +++ b/test/load.test.ts @@ -80,7 +80,7 @@ describe("load", function () { this.timeout(10000); before(() => { - mockAppConfigurationClientListConfigurationSettings(mockedKVs); + mockAppConfigurationClientListConfigurationSettings([mockedKVs]); }); after(() => { diff --git a/test/refresh.test.ts b/test/refresh.test.ts index fcffaee..5fbeb97 100644 --- a/test/refresh.test.ts +++ b/test/refresh.test.ts @@ -23,6 +23,15 @@ function addSetting(key: string, value: any) { mockedKVs.push(createMockedKeyValue({ key, value })); } +let listKvRequestCount = 0; +const listKvCallback = () => { + listKvRequestCount++; +}; +let getKvRequestCount = 0; +const getKvCallback = () => { + getKvRequestCount++; +}; + describe("dynamic refresh", function () { this.timeout(10000); @@ -32,12 +41,14 @@ describe("dynamic refresh", function () { { value: "40", key: "app.settings.fontSize" }, { value: "30", key: "app.settings.fontSize", label: "prod" } ].map(createMockedKeyValue); - mockAppConfigurationClientListConfigurationSettings(mockedKVs); - mockAppConfigurationClientGetConfigurationSetting(mockedKVs); + mockAppConfigurationClientListConfigurationSettings([mockedKVs], listKvCallback); + mockAppConfigurationClientGetConfigurationSetting(mockedKVs, getKvCallback); }); afterEach(() => { restoreMocks(); + listKvRequestCount = 0; + getKvRequestCount = 0; }); it("should throw error when refresh is not enabled but refresh is called", async () => { @@ -139,6 +150,8 @@ describe("dynamic refresh", function () { ] } }); + expect(listKvRequestCount).eq(1); + expect(getKvRequestCount).eq(0); expect(settings).not.undefined; expect(settings.get("app.settings.fontColor")).eq("red"); expect(settings.get("app.settings.fontSize")).eq("40"); @@ -149,10 +162,14 @@ describe("dynamic refresh", function () { // within refreshInterval, should not really refresh await settings.refresh(); expect(settings.get("app.settings.fontColor")).eq("red"); + expect(listKvRequestCount).eq(1); // no more request should be sent during the refresh interval + expect(getKvRequestCount).eq(0); // no more request should be sent during the refresh interval // after refreshInterval, should really refresh await sleepInMs(2 * 1000 + 1); await settings.refresh(); + expect(listKvRequestCount).eq(2); + expect(getKvRequestCount).eq(1); expect(settings.get("app.settings.fontColor")).eq("blue"); }); @@ -167,6 +184,8 @@ describe("dynamic refresh", function () { ] } }); + expect(listKvRequestCount).eq(1); + expect(getKvRequestCount).eq(0); expect(settings).not.undefined; expect(settings.get("app.settings.fontColor")).eq("red"); expect(settings.get("app.settings.fontSize")).eq("40"); @@ -174,11 +193,13 @@ describe("dynamic refresh", function () { // delete setting 'app.settings.fontColor' const newMockedKVs = mockedKVs.filter(elem => elem.key !== "app.settings.fontColor"); restoreMocks(); - mockAppConfigurationClientListConfigurationSettings(newMockedKVs); - mockAppConfigurationClientGetConfigurationSetting(newMockedKVs); + mockAppConfigurationClientListConfigurationSettings([newMockedKVs], listKvCallback); + mockAppConfigurationClientGetConfigurationSetting(newMockedKVs, getKvCallback); await sleepInMs(2 * 1000 + 1); await settings.refresh(); + expect(listKvRequestCount).eq(2); + expect(getKvRequestCount).eq(2); // one conditional request to detect change and one request as part of loading all kvs (because app.settings.fontColor doesn't exist in the response of listKv request) expect(settings.get("app.settings.fontColor")).eq(undefined); }); @@ -193,6 +214,8 @@ describe("dynamic refresh", function () { ] } }); + expect(listKvRequestCount).eq(1); + expect(getKvRequestCount).eq(0); expect(settings).not.undefined; expect(settings.get("app.settings.fontColor")).eq("red"); expect(settings.get("app.settings.fontSize")).eq("40"); @@ -200,6 +223,8 @@ describe("dynamic refresh", function () { updateSetting("app.settings.fontSize", "50"); // unwatched setting await sleepInMs(2 * 1000 + 1); await settings.refresh(); + expect(listKvRequestCount).eq(1); + expect(getKvRequestCount).eq(1); expect(settings.get("app.settings.fontSize")).eq("40"); }); @@ -215,6 +240,8 @@ describe("dynamic refresh", function () { ] } }); + expect(listKvRequestCount).eq(1); + expect(getKvRequestCount).eq(0); expect(settings).not.undefined; expect(settings.get("app.settings.fontColor")).eq("red"); expect(settings.get("app.settings.fontSize")).eq("40"); @@ -224,6 +251,8 @@ describe("dynamic refresh", function () { updateSetting("app.settings.fontSize", "50"); await sleepInMs(2 * 1000 + 1); await settings.refresh(); + expect(listKvRequestCount).eq(2); + expect(getKvRequestCount).eq(2); // two getKv request for two watched settings expect(settings.get("app.settings.fontSize")).eq("50"); expect(settings.get("app.settings.bgColor")).eq("white"); }); @@ -309,6 +338,8 @@ describe("dynamic refresh", function () { ] } }); + expect(listKvRequestCount).eq(1); + expect(getKvRequestCount).eq(1); // app.settings.bgColor doesn't exist in the response of listKv request, so an additional getKv request is made to get it. expect(settings).not.undefined; expect(settings.get("app.settings.fontColor")).eq("red"); expect(settings.get("app.settings.fontSize")).eq("40"); @@ -317,10 +348,45 @@ describe("dynamic refresh", function () { updateSetting("app.settings.fontColor", "blue"); await sleepInMs(2 * 1000 + 1); await settings.refresh(); + expect(listKvRequestCount).eq(1); + expect(getKvRequestCount).eq(2); // should not refresh expect(settings.get("app.settings.fontColor")).eq("red"); }); + it("should not refresh any more when there is refresh in progress", async () => { + const connectionString = createMockedConnectionString(); + const settings = await load(connectionString, { + refreshOptions: { + enabled: true, + refreshIntervalInMs: 2000, + watchedSettings: [ + { key: "app.settings.fontColor" } + ] + } + }); + expect(listKvRequestCount).eq(1); + expect(getKvRequestCount).eq(0); + expect(settings).not.undefined; + expect(settings.get("app.settings.fontColor")).eq("red"); + + // change setting + updateSetting("app.settings.fontColor", "blue"); + + // after refreshInterval, should really refresh + await sleepInMs(2 * 1000 + 1); + for (let i = 0; i < 5; i++) { // in practice, refresh should not be used in this way + settings.refresh(); // refresh "concurrently" + } + expect(listKvRequestCount).to.be.at.most(2); + expect(getKvRequestCount).to.be.at.most(1); + + await sleepInMs(1000); // wait for all 5 refresh attempts to finish + + expect(listKvRequestCount).eq(2); + expect(getKvRequestCount).eq(1); + expect(settings.get("app.settings.fontColor")).eq("blue"); + }); }); describe("dynamic refresh feature flags", function () { @@ -331,14 +397,16 @@ describe("dynamic refresh feature flags", function () { afterEach(() => { restoreMocks(); + listKvRequestCount = 0; + getKvRequestCount = 0; }); it("should refresh feature flags when enabled", async () => { mockedKVs = [ createMockedFeatureFlag("Beta", { enabled: true }) ]; - mockAppConfigurationClientListConfigurationSettings(mockedKVs); - mockAppConfigurationClientGetConfigurationSetting(mockedKVs); + mockAppConfigurationClientListConfigurationSettings([mockedKVs], listKvCallback); + mockAppConfigurationClientGetConfigurationSetting(mockedKVs, getKvCallback); const connectionString = createMockedConnectionString(); const settings = await load(connectionString, { @@ -353,6 +421,8 @@ describe("dynamic refresh feature flags", function () { } } }); + expect(listKvRequestCount).eq(2); // one listKv request for kvs and one listKv request for feature flags + expect(getKvRequestCount).eq(0); expect(settings).not.undefined; expect(settings.get("feature_management")).not.undefined; expect(settings.get("feature_management").feature_flags).not.undefined; @@ -371,6 +441,8 @@ describe("dynamic refresh feature flags", function () { await sleepInMs(2 * 1000 + 1); await settings.refresh(); + expect(listKvRequestCount).eq(4); // 2 + 2 more requests: one conditional request to detect change and one request to reload all feature flags + expect(getKvRequestCount).eq(0); expect(settings.get("feature_management").feature_flags[0].id).eq("Beta"); expect(settings.get("feature_management").feature_flags[0].enabled).eq(false); @@ -387,8 +459,8 @@ describe("dynamic refresh feature flags", function () { createMockedFeatureFlag("Beta_1", { enabled: true }), createMockedFeatureFlag("Beta_2", { enabled: true }), ]; - mockAppConfigurationClientListConfigurationSettings(page1, page2); - mockAppConfigurationClientGetConfigurationSetting([...page1, ...page2]); + mockAppConfigurationClientListConfigurationSettings([page1, page2], listKvCallback); + mockAppConfigurationClientGetConfigurationSetting([...page1, ...page2], getKvCallback); const connectionString = createMockedConnectionString(); const settings = await load(connectionString, { @@ -403,6 +475,8 @@ describe("dynamic refresh feature flags", function () { } } }); + expect(listKvRequestCount).eq(2); + expect(getKvRequestCount).eq(0); let refreshSuccessfulCount = 0; settings.onRefresh(() => { @@ -411,16 +485,20 @@ describe("dynamic refresh feature flags", function () { await sleepInMs(2 * 1000 + 1); await settings.refresh(); + expect(listKvRequestCount).eq(3); // one conditional request to detect change + expect(getKvRequestCount).eq(0); expect(refreshSuccessfulCount).eq(0); // no change in feature flags, because page etags are the same. // change feature flag Beta_1 to false page2[0] = createMockedFeatureFlag("Beta_1", { enabled: false }); restoreMocks(); - mockAppConfigurationClientListConfigurationSettings(page1, page2); - mockAppConfigurationClientGetConfigurationSetting([...page1, ...page2]); + mockAppConfigurationClientListConfigurationSettings([page1, page2], listKvCallback); + mockAppConfigurationClientGetConfigurationSetting([...page1, ...page2], getKvCallback); await sleepInMs(2 * 1000 + 1); await settings.refresh(); + expect(listKvRequestCount).eq(5); // 3 + 2 more requests: one conditional request to detect change and one request to reload all feature flags + expect(getKvRequestCount).eq(0); expect(refreshSuccessfulCount).eq(1); // change in feature flags, because page etags are different. }); }); diff --git a/test/requestTracing.test.ts b/test/requestTracing.test.ts index d4e7edc..a64d2c4 100644 --- a/test/requestTracing.test.ts +++ b/test/requestTracing.test.ts @@ -123,10 +123,10 @@ describe("request tracing", function () { }); it("should have request type in correlation-context header when refresh is enabled", async () => { - mockAppConfigurationClientListConfigurationSettings([{ + mockAppConfigurationClientListConfigurationSettings([[{ key: "app.settings.fontColor", value: "red" - }].map(createMockedKeyValue)); + }].map(createMockedKeyValue)]); const settings = await load(createMockedConnectionString(fakeEndpoint), { clientOptions, diff --git a/test/utils/testHelper.ts b/test/utils/testHelper.ts index 6e787dd..bf05882 100644 --- a/test/utils/testHelper.ts +++ b/test/utils/testHelper.ts @@ -42,13 +42,16 @@ function _filterKVs(unfilteredKvs: ConfigurationSetting[], listOptions: any) { * Mocks the listConfigurationSettings method of AppConfigurationClient to return the provided pages of ConfigurationSetting. * E.g. * - mockAppConfigurationClientListConfigurationSettings([item1, item2, item3]) // single page - * - mockAppConfigurationClientListConfigurationSettings([item1, item2], [item3], [item4]) // multiple pages * * @param pages List of pages, each page is a list of ConfigurationSetting */ -function mockAppConfigurationClientListConfigurationSettings(...pages: ConfigurationSetting[][]) { +function mockAppConfigurationClientListConfigurationSettings(pages: ConfigurationSetting[][], customCallback?: (listOptions) => any) { sinon.stub(AppConfigurationClient.prototype, "listConfigurationSettings").callsFake((listOptions) => { + if (customCallback) { + customCallback(listOptions); + } + let kvs = _filterKVs(pages.flat(), listOptions); const mockIterator: AsyncIterableIterator & { byPage(): AsyncIterableIterator } = { [Symbol.asyncIterator](): AsyncIterableIterator { @@ -94,8 +97,12 @@ function mockAppConfigurationClientListConfigurationSettings(...pages: Configura }); } -function mockAppConfigurationClientGetConfigurationSetting(kvList) { +function mockAppConfigurationClientGetConfigurationSetting(kvList, customCallback?: (options) => any) { sinon.stub(AppConfigurationClient.prototype, "getConfigurationSetting").callsFake((settingId, options) => { + if (customCallback) { + customCallback(options); + } + const found = kvList.find(elem => elem.key === settingId.key && elem.label === settingId.label); if (found) { if (options?.onlyIfChanged && settingId.etag === found.etag) { @@ -210,4 +217,4 @@ export { createMockedFeatureFlag, sleepInMs -}; +};