Merge main to release/stable/v2

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@azure/app-configuration-provider",
-  "version": "2.5.0",
+  "version": "2.5.1",
   "description": "The JavaScript configuration provider for Azure App Configuration",
   "files": [
     "dist/",

src/appConfigurationImpl.ts

@@ -659,7 +659,7 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration {
 
                 const watcher: SettingWatcher = this.#sentinels.get(watchedSetting)!; // watcher should always exist for sentinels
                 const isDeleted = response === undefined && watcher.etag !== undefined; // previously existed, now deleted
-                const isChanged = response && response.statusCode === 200 && watcher.etag !== response.etag; // etag changed
+                const isChanged = response && Number(response.statusCode) === 200 && watcher.etag !== response.etag; // etag changed
                 if (isDeleted || isChanged) {
                     changedSentinel = watchedSetting;
                     changedSentinelWatcher = { etag: isChanged ? response.etag : undefined };
@@ -750,7 +750,7 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration {
 
                 for await (const page of pageIterator) {
                     // when conditional request is sent, the response will be 304 if not changed
-                    if (page._response.status === 200) { // created or changed
+                    if (Number(page._response.status) === 200) { // created or changed
                         return true;
                     }
                 }
@@ -779,7 +779,7 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration {
         try {
             response = await this.#executeWithFailoverPolicy(funcToExecute);
         } catch (error) {
-            if (isRestError(error) && error.statusCode === 404) {
+            if (isRestError(error) && Number(error.statusCode) === 404) {
                 response = undefined;
             } else {
                 throw error;
@@ -822,7 +822,7 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration {
         try {
             response = await this.#executeWithFailoverPolicy(funcToExecute);
         } catch (error) {
-            if (isRestError(error) && error.statusCode === 404) {
+            if (isRestError(error) && Number(error.statusCode) === 404) {
                 response = undefined;
             } else {
                 throw error;

src/version.ts

@@ -1,4 +1,4 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.
 
-export const VERSION = "2.5.0";
+export const VERSION = "2.5.1";

test/refresh.test.ts

@@ -7,7 +7,7 @@ import chaiAsPromised from "chai-as-promised";
 chai.use(chaiAsPromised);
 const expect = chai.expect;
 import { load } from "../src/index.js";
-import { mockAppConfigurationClientListConfigurationSettings, mockAppConfigurationClientGetConfigurationSetting, restoreMocks, createMockedConnectionString, createMockedKeyValue, sleepInMs, createMockedFeatureFlag } from "./utils/testHelper.js";
+import { mockAppConfigurationClientListConfigurationSettings, mockAppConfigurationClientListConfigurationSettingsWithStringStatus, mockAppConfigurationClientGetConfigurationSetting, restoreMocks, createMockedConnectionString, createMockedKeyValue, sleepInMs, createMockedFeatureFlag } from "./utils/testHelper.js";
 import * as uuid from "uuid";
 
 let mockedKVs: any[] = [];
@@ -513,6 +513,54 @@ describe("dynamic refresh", function () {
         // Verify changes are reflected
         expect(settings.get("TestTagKey")).eq("newValue");
     });
+
+    it("should refresh key values when page response status is string", async () => {
+        restoreMocks(); // restore the mock set up in beforeEach
+        const initialKVs = [
+            { value: "red", key: "app.settings.fontColor" },
+            { value: "40", key: "app.settings.fontSize" }
+        ].map(createMockedKeyValue);
+        mockAppConfigurationClientListConfigurationSettingsWithStringStatus([initialKVs], listKvCallback);
+        mockAppConfigurationClientGetConfigurationSetting(initialKVs, getKvCallback);
+
+        const connectionString = createMockedConnectionString();
+        const settings = await load(connectionString, {
+            refreshOptions: {
+                enabled: true,
+                refreshIntervalInMs: 2000
+            }
+        });
+        expect(listKvRequestCount).eq(1);
+        expect(settings).not.undefined;
+        expect(settings.get("app.settings.fontColor")).eq("red");
+
+        let refreshSuccessfulCount = 0;
+        settings.onRefresh(() => {
+            refreshSuccessfulCount++;
+        });
+
+        // no change yet, the page response status is "304"
+        await sleepInMs(2 * 1000 + 1);
+        await settings.refresh();
+        expect(listKvRequestCount).eq(2); // one more conditional request to detect change
+        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);
+        mockAppConfigurationClientListConfigurationSettingsWithStringStatus([changedKVs], listKvCallback);
+        mockAppConfigurationClientGetConfigurationSetting(changedKVs, getKvCallback);
+
+        // the page response status is "200" when changed
+        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(refreshSuccessfulCount).eq(1); // change in key values, because page etags are different.
+        expect(settings.get("app.settings.fontColor")).eq("blue");
+    });
 });
 
 describe("dynamic refresh feature flags", function () {
@@ -672,5 +720,60 @@ describe("dynamic refresh feature flags", function () {
         expect(updatedFeatureManagement.feature_flags[0].id).eq("DevFeature");
         expect(updatedFeatureManagement.feature_flags[0].enabled).eq(false);
     });
+
+    it("should refresh feature flags when page response status is string", async () => {
+        // mock multiple pages of feature flags
+        const page1 = [
+            createMockedFeatureFlag("Alpha_1", { enabled: true }),
+            createMockedFeatureFlag("Alpha_2", { enabled: true }),
+        ];
+        const page2 = [
+            createMockedFeatureFlag("Beta_1", { enabled: true }),
+            createMockedFeatureFlag("Beta_2", { enabled: true }),
+        ];
+        mockAppConfigurationClientListConfigurationSettingsWithStringStatus([page1, page2], listKvCallback);
+        mockAppConfigurationClientGetConfigurationSetting([...page1, ...page2], getKvCallback);
+
+        const connectionString = createMockedConnectionString();
+        const settings = await load(connectionString, {
+            featureFlagOptions: {
+                enabled: true,
+                selectors: [{
+                    keyFilter: "*"
+                }],
+                refresh: {
+                    enabled: true,
+                    refreshIntervalInMs: 2000 // 2 seconds for quick test.
+                }
+            }
+        });
+        expect(listKvRequestCount).eq(2);
+        expect(getKvRequestCount).eq(0);
+
+        let refreshSuccessfulCount = 0;
+        settings.onRefresh(() => {
+            refreshSuccessfulCount++;
+        });
+
+        // no change yet, the page response status is "304"
+        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();
+        mockAppConfigurationClientListConfigurationSettingsWithStringStatus([page1, page2], listKvCallback);
+        mockAppConfigurationClientGetConfigurationSetting([...page1, ...page2], getKvCallback);
+
+        // the page response status is "200" when changed
+        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.
+    });
 });
 /* eslint-enable @typescript-eslint/no-unused-expressions */

test/utils/testHelper.ts

@@ -6,7 +6,7 @@ import { AppConfigurationClient, ConfigurationSetting, featureFlagContentType, s
 import { ClientSecretCredential } from "@azure/identity";
 import { KeyVaultSecret, SecretClient } from "@azure/keyvault-secrets";
 import * as uuid from "uuid";
-import { RestError } from "@azure/core-rest-pipeline";
+import { RestError, PipelineRequest, PipelineResponse, SendRequest } from "@azure/core-rest-pipeline";
 import { ConfigurationClientManager } from "../../src/configurationClientManager.js";
 import { ConfigurationClientWrapper } from "../../src/configurationClientWrapper.js";
 
@@ -51,7 +51,7 @@ function _filterKVs(unfilteredKvs: ConfigurationSetting[], listOptions: any) {
         }
         let tagsMatched = true;
         if (tagsFilter.length > 0) {
-            tagsMatched = tagsFilter.every(tag => {
+            tagsMatched = tagsFilter.every((tag: string) => {
                 const [tagName, tagValue] = tag.split("=");
                 if (tagValue === "\0") {
                     return kv.tags && kv.tags[tagName] === null;
@@ -63,7 +63,7 @@ function _filterKVs(unfilteredKvs: ConfigurationSetting[], listOptions: any) {
     });
 }
 
-function getMockedIterator(pages: ConfigurationSetting[][], kvs: ConfigurationSetting[], listOptions: any) {
+function getMockedIterator(pages: ConfigurationSetting[][], kvs: ConfigurationSetting[], listOptions: any, useStringStatus: boolean = false) {
     const mockIterator: AsyncIterableIterator<any> & { byPage(): AsyncIterableIterator<any> } = {
         [Symbol.asyncIterator](): AsyncIterableIterator<any> {
             kvs = _filterKVs(pages.flat(), listOptions);
@@ -74,7 +74,7 @@ function getMockedIterator(pages: ConfigurationSetting[][], kvs: ConfigurationSe
             return Promise.resolve({ done: !value, value });
         },
         byPage(): AsyncIterableIterator<any> {
-            let remainingPages;
+            let remainingPages: ConfigurationSetting[][];
             const pageEtags = listOptions?.pageEtags ? [...listOptions.pageEtags] : undefined; // a copy of the original list
             return {
                 [Symbol.asyncIterator](): AsyncIterableIterator<any> {
@@ -95,7 +95,7 @@ function getMockedIterator(pages: ConfigurationSetting[][], kvs: ConfigurationSe
                             value: {
                                 items,
                                 etag,
-                                _response: { status: statusCode }
+                                _response: { status: useStringStatus ? `${statusCode}` : statusCode }
                             }
                         };
                     }
@@ -114,7 +114,7 @@ function getMockedIterator(pages: ConfigurationSetting[][], kvs: ConfigurationSe
  *
  * @param pages List of pages, each page is a list of ConfigurationSetting
  */
-function mockAppConfigurationClientListConfigurationSettings(pages: ConfigurationSetting[][], customCallback?: (listOptions) => any) {
+function mockAppConfigurationClientListConfigurationSettings(pages: ConfigurationSetting[][], customCallback?: (listOptions: any) => any) {
 
     sinon.stub(AppConfigurationClient.prototype, "listConfigurationSettings").callsFake((listOptions) => {
         if (customCallback) {
@@ -126,6 +126,18 @@ function mockAppConfigurationClientListConfigurationSettings(pages: Configuratio
     });
 }
 
+function mockAppConfigurationClientListConfigurationSettingsWithStringStatus(pages: ConfigurationSetting[][], customCallback?: (listOptions: any) => any) {
+
+    sinon.stub(AppConfigurationClient.prototype, "listConfigurationSettings").callsFake((listOptions) => {
+        if (customCallback) {
+            customCallback(listOptions);
+        }
+
+        const kvs = _filterKVs(pages.flat(), listOptions);
+        return getMockedIterator(pages, kvs, listOptions, true);
+    });
+}
+
 function mockAppConfigurationClientLoadBalanceMode(pages: ConfigurationSetting[][], clientWrapper: ConfigurationClientWrapper, countObject: { count: number }) {
     sinon.stub(clientWrapper.client, "listConfigurationSettings").callsFake((listOptions) => {
         countObject.count += 1;
@@ -163,7 +175,7 @@ function mockConfigurationManagerGetClients(fakeClientWrappers: ConfigurationCli
     });
 }
 
-function mockAppConfigurationClientGetConfigurationSetting(kvList: any[], customCallback?: (options) => any) {
+function mockAppConfigurationClientGetConfigurationSetting(kvList: any[], customCallback?: (options: any) => any) {
     sinon.stub(AppConfigurationClient.prototype, "getConfigurationSetting").callsFake((settingId, options) => {
         if (customCallback) {
             customCallback(options);
@@ -182,7 +194,7 @@ function mockAppConfigurationClientGetConfigurationSetting(kvList: any[], custom
     });
 }
 
-function mockAppConfigurationClientGetSnapshot(snapshotResponses: Map<string, any>, customCallback?: (options) => any) {
+function mockAppConfigurationClientGetSnapshot(snapshotResponses: Map<string, any>, customCallback?: (options: any) => any) {
     sinon.stub(AppConfigurationClient.prototype, "getSnapshot").callsFake((name, options) => {
         if (customCallback) {
             customCallback(options);
@@ -196,7 +208,7 @@ function mockAppConfigurationClientGetSnapshot(snapshotResponses: Map<string, an
     });
 }
 
-function mockAppConfigurationClientListConfigurationSettingsForSnapshot(snapshotResponses: Map<string, ConfigurationSetting[][]>, customCallback?: (options) => any) {
+function mockAppConfigurationClientListConfigurationSettingsForSnapshot(snapshotResponses: Map<string, ConfigurationSetting[][]>, customCallback?: (options: any) => any) {
     sinon.stub(AppConfigurationClient.prototype, "listConfigurationSettingsForSnapshot").callsFake((name, listOptions) => {
         if (customCallback) {
             customCallback(listOptions);
@@ -218,7 +230,7 @@ function mockSecretClientGetSecret(uriValueList: [string, string][]) {
         dict.set(uri, value);
     }
 
-    sinon.stub(SecretClient.prototype, "getSecret").callsFake(async function (secretName, options) {
+    sinon.stub(SecretClient.prototype, "getSecret").callsFake(async function (this: SecretClient, secretName, options) {
         const url = new URL(this.vaultUrl);
         url.pathname = `/secrets/${secretName}`;
         if (options?.version) {
@@ -314,7 +326,7 @@ class HttpRequestHeadersPolicy {
         this.headers = {};
         this.name = "HttpRequestHeadersPolicy";
     }
-    sendRequest(req, next) {
+    sendRequest(req: PipelineRequest, next: SendRequest): Promise<PipelineResponse> {
         this.headers = req.headers;
         return next(req).then(resp => resp);
     }
@@ -323,6 +335,7 @@ class HttpRequestHeadersPolicy {
 export {
     sinon,
     mockAppConfigurationClientListConfigurationSettings,
+    mockAppConfigurationClientListConfigurationSettingsWithStringStatus,
     mockAppConfigurationClientGetConfigurationSetting,
     mockAppConfigurationClientGetSnapshot,
     mockAppConfigurationClientListConfigurationSettingsForSnapshot,