Merge pull request #139 from Azure/zhiyuanliang/merge-main-to-preview

Merge main to preview

Author: zhiyuanliang-ms

src/AzureAppConfigurationImpl.ts

@@ -63,6 +63,8 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration {
     #isFailoverRequest: boolean = false;
 
     // Refresh
+    #refreshInProgress: boolean = false;
+
     #refreshInterval: number = DEFAULT_REFRESH_INTERVAL_IN_MS;
     #onRefreshListeners: Array<() => any> = [];
     /**
@@ -436,6 +438,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<void> {
         const refreshTasks: Promise<boolean>[] = [];
         if (this.#refreshEnabled) {
             refreshTasks.push(this.#refreshKeyValues());

test/featureFlag.test.ts

@@ -202,7 +202,7 @@ describe("feature flags", function () {
     this.timeout(10000);
 
     before(() => {
-        mockAppConfigurationClientListConfigurationSettings(mockedKVs);
+        mockAppConfigurationClientListConfigurationSettings([mockedKVs]);
     });
 
     after(() => {

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;

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() {

test/load.test.ts

@@ -80,7 +80,7 @@ describe("load", function () {
     this.timeout(10000);
 
     before(() => {
-        mockAppConfigurationClientListConfigurationSettings(mockedKVs);
+        mockAppConfigurationClientListConfigurationSettings([mockedKVs]);
     });
 
     after(() => {

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,18 +184,22 @@ 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");
 
         // 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,13 +214,17 @@ 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");
 
         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<any>("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<any>("feature_management").feature_flags[0].id).eq("Beta");
         expect(settings.get<any>("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.
     });
 });

test/requestTracing.test.ts

@@ -115,10 +115,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,

test/utils/testHelper.ts

@@ -88,13 +88,16 @@ function getMockedIterator(pages: ConfigurationSetting[][], kvs: ConfigurationSe
  * 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);
+        }
+
         const kvs = _filterKVs(pages.flat(), listOptions);
         return getMockedIterator(pages, kvs, listOptions);
     });
@@ -138,8 +141,12 @@ function mockConfigurationManagerGetClients(fakeClientWrappers: ConfigurationCli
     });
 }
 
-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) {