resolve conflict

zhiyuanliang-ms · zhiyuanliang-ms · commit 647c7a6132a2 · 2024-12-02T18:40:18.000+08:00
diff --git a/src/AzureAppConfigurationImpl.ts b/src/AzureAppConfigurationImpl.ts
@@ -63,6 +63,8 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration {
     #isFailoverRequest: boolean = false;
 
     // Refresh
+    #refreshInProgress: boolean = false;
+
     #watchAll: boolean = false;
     #refreshInterval: number = DEFAULT_REFRESH_INTERVAL_IN_MS;
     #onRefreshListeners: Array<() => any> = [];
@@ -273,6 +275,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());
diff --git a/test/featureFlag.test.ts b/test/featureFlag.test.ts
@@ -202,7 +202,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
@@ -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
@@ -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
@@ -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
@@ -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 () => {
@@ -120,6 +131,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");
@@ -130,10 +143,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");
     });
 
@@ -148,18 +165,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);
     });
 
@@ -174,13 +195,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");
     });
 
@@ -196,6 +221,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");
@@ -205,6 +232,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");
     });
@@ -290,6 +319,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");
@@ -298,6 +329,8 @@ 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");
     });
@@ -310,6 +343,8 @@ describe("dynamic refresh", function () {
                 refreshIntervalInMs: 2000
             }
         });
+        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");
@@ -320,6 +355,8 @@ describe("dynamic refresh", function () {
         // after refreshInterval, should really refresh
         await sleepInMs(2 * 1000 + 1);
         await settings.refresh();
+        expect(listKvRequestCount).eq(3); // 1 + 2 more requests: one conditional request to detect change and one request to reload all key values
+        expect(getKvRequestCount).eq(0);
         expect(settings.get("app.settings.fontColor")).eq("blue");
     });
 
@@ -331,6 +368,8 @@ describe("dynamic refresh", function () {
                 refreshIntervalInMs: 2000
             }
         });
+        expect(listKvRequestCount).eq(1);
+        expect(getKvRequestCount).eq(0);
 
         let refreshSuccessfulCount = 0;
         settings.onRefresh(() => {
@@ -342,22 +381,60 @@ describe("dynamic refresh", function () {
 
         await sleepInMs(2 * 1000 + 1);
         await settings.refresh();
-        expect(refreshSuccessfulCount).eq(0); // no change in feature flags, because page etags are the same.
+        expect(listKvRequestCount).eq(2); // one more conditional request to detect change
+        expect(getKvRequestCount).eq(0);
+        expect(refreshSuccessfulCount).eq(0); // no change in key values, because page etags are the same.
 
         // change key value
         restoreMocks();
         const changedKVs = [
             { value: "blue", key: "app.settings.fontColor" },
             { value: "40", key: "app.settings.fontSize" }
         ].map(createMockedKeyValue);
-        mockAppConfigurationClientListConfigurationSettings(changedKVs);
-        mockAppConfigurationClientGetConfigurationSetting(changedKVs);
+        mockAppConfigurationClientListConfigurationSettings([changedKVs], listKvCallback);
+        mockAppConfigurationClientGetConfigurationSetting(changedKVs, getKvCallback);
 
         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 key values
+        expect(getKvRequestCount).eq(0);
         expect(refreshSuccessfulCount).eq(1); // change in key values, because page etags are different.
         expect(settings.get("app.settings.fontColor")).eq("blue");
     });
+
+    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 () {
@@ -368,14 +445,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, {
@@ -390,6 +469,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;
@@ -408,6 +489,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);
@@ -424,8 +507,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, {
@@ -440,6 +523,8 @@ describe("dynamic refresh feature flags", function () {
                 }
             }
         });
+        expect(listKvRequestCount).eq(2);
+        expect(getKvRequestCount).eq(0);
 
         let refreshSuccessfulCount = 0;
         settings.onRefresh(() => {
@@ -448,16 +533,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
diff --git a/test/utils/testHelper.ts b/test/utils/testHelper.ts

Original file line number	Diff line number	Diff line change
`@@ -19,7 +19,7 @@ const mockedData = [`
`19`	`19`	`function mockAppConfigurationClient() {`
`20`	`20`	`// eslint-disable-next-line @typescript-eslint/no-unused-vars`
`21`	`21`	`const kvs = mockedData.map(([key, vaultUri, _value]) => createMockedKeyVaultReference(key, vaultUri));`
`22`		`- mockAppConfigurationClientListConfigurationSettings(kvs);`
	`22`	`+ mockAppConfigurationClientListConfigurationSettings([kvs]);`
`23`	`23`	`}`
`24`	`24`
`25`	`25`	`function mockNewlyCreatedKeyVaultSecretClients() {`