Skip to content

Fix refresh bug when response status is string #335

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.

Sign up for GitHub

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

Merged
zhiyuanliang-ms merged 1 commit into from
Jun 26, 2026
Merged

Fix refresh bug when response status is string #335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/appConfigurationImpl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@linglingye001 linglingye001 Jun 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Better to have comment in the code/helper

if (isDeleted || isChanged) {
changedSentinel = watchedSetting;
changedSentinelWatcher = { etag: isChanged ? response.etag : undefined };
Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
105 changes: 104 additions & 1 deletion test/refresh.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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[] = [];
Expand Down Expand Up @@ -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 () {
Expand Down Expand Up @@ -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 */
35 changes: 24 additions & 11 deletions test/utils/testHelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand All @@ -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> {
Expand All @@ -95,7 +95,7 @@ function getMockedIterator(pages: ConfigurationSetting[][], kvs: ConfigurationSe
value: {
items,
etag,
_response: { status: statusCode }
_response: { status: useStringStatus ? `${statusCode}` : statusCode }
}
};
}
Expand All @@ -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) {
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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) {
Expand Down Expand Up @@ -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);
}
Expand All @@ -323,6 +335,7 @@ class HttpRequestHeadersPolicy {
export {
sinon,
mockAppConfigurationClientListConfigurationSettings,
mockAppConfigurationClientListConfigurationSettingsWithStringStatus,
mockAppConfigurationClientGetConfigurationSetting,
mockAppConfigurationClientGetSnapshot,
mockAppConfigurationClientListConfigurationSettingsForSnapshot,
Expand Down
Loading