diff --git a/src/appConfigurationImpl.ts b/src/appConfigurationImpl.ts index 1ae18bcd..691a7a4d 100644 --- a/src/appConfigurationImpl.ts +++ b/src/appConfigurationImpl.ts @@ -53,7 +53,7 @@ import { } from "./requestTracing/utils.js"; import { FeatureFlagTracingOptions } from "./requestTracing/featureFlagTracingOptions.js"; import { AIConfigurationTracingOptions } from "./requestTracing/aiConfigurationTracingOptions.js"; -import { KeyFilter, LabelFilter, SettingSelector } from "./types.js"; +import { KeyFilter, LabelFilter, SettingWatcher, SettingSelector, PagedSettingsWatcher, WatchedSetting } from "./types.js"; import { ConfigurationClientManager } from "./configurationClientManager.js"; import { getFixedBackoffDuration, getExponentialBackoffDuration } from "./common/backoffUtils.js"; import { InvalidOperationError, ArgumentError, isFailoverableError, isInputError } from "./common/errors.js"; @@ -61,10 +61,6 @@ import { ErrorMessages } from "./common/errorMessages.js"; const MIN_DELAY_FOR_UNHANDLED_FAILURE = 5_000; // 5 seconds -type PagedSettingSelector = SettingSelector & { - pageEtags?: string[]; -}; - export class AzureAppConfigurationImpl implements AzureAppConfiguration { /** * Hosting key-value pairs in the configuration store. @@ -94,7 +90,7 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration { * Aka watched settings. */ #refreshEnabled: boolean = false; - #sentinels: ConfigurationSettingId[] = []; + #sentinels: Map = new Map(); #watchAll: boolean = false; #kvRefreshInterval: number = DEFAULT_REFRESH_INTERVAL_IN_MS; #kvRefreshTimer: RefreshTimer; @@ -114,11 +110,11 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration { /** * Selectors of key-values obtained from @see AzureAppConfigurationOptions.selectors */ - #kvSelectors: PagedSettingSelector[] = []; + #kvSelectors: PagedSettingsWatcher[] = []; /** * Selectors of feature flags obtained from @see AzureAppConfigurationOptions.featureFlagOptions.selectors */ - #ffSelectors: PagedSettingSelector[] = []; + #ffSelectors: PagedSettingsWatcher[] = []; // Load balancing #lastSuccessfulEndpoint: string = ""; @@ -157,7 +153,7 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration { if (setting.label?.includes("*") || setting.label?.includes(",")) { throw new ArgumentError(ErrorMessages.INVALID_WATCHED_SETTINGS_LABEL); } - this.#sentinels.push(setting); + this.#sentinels.set(setting, { etag: undefined }); } } @@ -386,7 +382,12 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration { let postAttempts = 0; do { // at least try to load once try { - await this.#loadSelectedAndWatchedKeyValues(); + if (this.#refreshEnabled && !this.#watchAll) { + await this.#loadWatchedSettings(); + } + + await this.#loadSelectedKeyValues(); + if (this.#featureFlagEnabled) { await this.#loadFeatureFlags(); } @@ -486,7 +487,7 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration { // the configuration setting loaded by the later selector in the iteration order will override the one from the earlier selector. const loadedSettings: Map = new Map(); // deep copy selectors to avoid modification if current client fails - const selectorsToUpdate = JSON.parse( + const selectorsToUpdate: PagedSettingsWatcher[] = JSON.parse( JSON.stringify(selectors) ); @@ -497,7 +498,7 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration { labelFilter: selector.labelFilter, tagsFilter: selector.tagFilters }; - const pageEtags: string[] = []; + const pageWatchers: SettingWatcher[] = []; const pageIterator = listConfigurationSettingsWithTrace( this.#requestTraceOptions, client, @@ -505,14 +506,14 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration { ).byPage(); for await (const page of pageIterator) { - pageEtags.push(page.etag ?? ""); + pageWatchers.push({ etag: page.etag }); for (const setting of page.items) { if (loadFeatureFlag === isFeatureFlag(setting)) { loadedSettings.set(setting.key, setting); } } } - selector.pageEtags = pageEtags; + selector.pageWatchers = pageWatchers; } else { // snapshot selector const snapshot = await this.#getSnapshot(selector.snapshotName); if (snapshot === undefined) { @@ -549,15 +550,12 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration { } /** - * Loads selected key-values and watched settings (sentinels) for refresh from App Configuration to the local configuration. + * Loads selected key-values from App Configuration to the local configuration. */ - async #loadSelectedAndWatchedKeyValues() { + async #loadSelectedKeyValues() { this.#secretReferences = []; // clear all cached key vault reference configuration settings const keyValues: [key: string, value: unknown][] = []; const loadedSettings: ConfigurationSetting[] = await this.#loadConfigurationSettings(); - if (this.#refreshEnabled && !this.#watchAll) { - await this.#updateWatchedKeyValuesEtag(loadedSettings); - } if (this.#requestTracingEnabled && this.#aiConfigurationTracing !== undefined) { // reset old AI configuration tracing in order to track the information present in the current response from server @@ -587,22 +585,14 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration { } /** - * Updates etag of watched settings from loaded data. If a watched setting is not covered by any selector, a request will be sent to retrieve it. + * Loads watched settings (sentinels) for refresh from App Configuration to the local configuration. */ - async #updateWatchedKeyValuesEtag(existingSettings: ConfigurationSetting[]): Promise { - const updatedSentinels: ConfigurationSettingId[] = []; - for (const sentinel of this.#sentinels) { - const matchedSetting = existingSettings.find(s => s.key === sentinel.key && s.label === sentinel.label); - if (matchedSetting) { - updatedSentinels.push( {...sentinel, etag: matchedSetting.etag} ); - } else { - // Send a request to retrieve key-value since it may be either not loaded or loaded with a different label or different casing - const { key, label } = sentinel; - const response = await this.#getConfigurationSetting({ key, label }); - updatedSentinels.push( {...sentinel, etag: response?.etag} ); - } + async #loadWatchedSettings(): Promise { + for (const watchedSetting of this.#sentinels.keys()) { + const configurationSettingId: ConfigurationSettingId = { key: watchedSetting.key, label: watchedSetting.label }; + const response = await this.#getConfigurationSetting(configurationSettingId, { onlyIfChanged: false }); + this.#sentinels.set(watchedSetting, { etag: response?.etag }); } - this.#sentinels = updatedSentinels; } /** @@ -649,19 +639,26 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration { // try refresh if any of watched settings is changed. let needRefresh = false; + let changedSentinel; + let changedSentinelWatcher; if (this.#watchAll) { needRefresh = await this.#checkConfigurationSettingsChange(this.#kvSelectors); - } - for (const sentinel of this.#sentinels.values()) { - const response = await this.#getConfigurationSetting(sentinel, { - onlyIfChanged: true - }); - - if (response?.statusCode === 200 // created or changed - || (response === undefined && sentinel.etag !== undefined) // deleted - ) { - needRefresh = true; - break; + } else { + for (const watchedSetting of this.#sentinels.keys()) { + const configurationSettingId: ConfigurationSettingId = { key: watchedSetting.key, label: watchedSetting.label, etag: this.#sentinels.get(watchedSetting)?.etag }; + const response = await this.#getConfigurationSetting(configurationSettingId, { + onlyIfChanged: true + }); + + const watcher = this.#sentinels.get(watchedSetting); + if (response?.statusCode === 200 // created or changed + || (response === undefined && watcher?.etag !== undefined) // deleted + ) { + changedSentinel = watchedSetting; + changedSentinelWatcher = watcher; + needRefresh = true; + break; + } } } @@ -669,7 +666,8 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration { for (const adapter of this.#adapters) { await adapter.onChangeDetected(); } - await this.#loadSelectedAndWatchedKeyValues(); + await this.#loadSelectedKeyValues(); + this.#sentinels.set(changedSentinel, changedSentinelWatcher); // update the changed sentinel's watcher } this.#kvRefreshTimer.reset(); @@ -719,17 +717,18 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration { * @param selectors - The @see PagedSettingSelector of the kev-value collection. * @returns true if key-value collection has changed, false otherwise. */ - async #checkConfigurationSettingsChange(selectors: PagedSettingSelector[]): Promise { + async #checkConfigurationSettingsChange(selectors: PagedSettingsWatcher[]): Promise { const funcToExecute = async (client) => { for (const selector of selectors) { if (selector.snapshotName) { // skip snapshot selector continue; } + const pageWatchers: SettingWatcher[] = selector.pageWatchers ?? []; const listOptions: ListConfigurationSettingsOptions = { keyFilter: selector.keyFilter, labelFilter: selector.labelFilter, tagsFilter: selector.tagFilters, - pageEtags: selector.pageEtags + pageEtags: pageWatchers.map(w => w.etag ?? "") }; const pageIterator = listConfigurationSettingsWithTrace( @@ -739,6 +738,7 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration { ).byPage(); 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 return true; } diff --git a/src/refresh/refreshOptions.ts b/src/refresh/refreshOptions.ts index d08d88d6..1dfaf4ee 100644 --- a/src/refresh/refreshOptions.ts +++ b/src/refresh/refreshOptions.ts @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT license. -import { WatchedSetting } from "../watchedSetting.js"; +import { WatchedSetting } from "../types.js"; export const DEFAULT_REFRESH_INTERVAL_IN_MS = 30_000; export const MIN_REFRESH_INTERVAL_IN_MS = 1_000; diff --git a/src/types.ts b/src/types.ts index 21ce23f0..7ef804a8 100644 --- a/src/types.ts +++ b/src/types.ts @@ -58,7 +58,7 @@ export enum KeyFilter { * Matches all key-values. */ Any = "*" -} +}; /** * LabelFilter is used to filter key-values based on labels. @@ -68,7 +68,7 @@ export enum LabelFilter { * Matches key-values without a label. */ Null = "\0" -} +}; /** * TagFilter is used to filter key-values based on tags. @@ -78,4 +78,25 @@ export enum TagFilter { * Represents empty tag value. */ Null = "" +}; + +export type WatchedSetting = { + /** + * The key for this setting. + */ + key: string; + + /** + * The label for this setting. + * Leaving this undefined means this setting does not have a label. + */ + label?: string; } + +export type SettingWatcher = { + etag?: string; +} + +export type PagedSettingsWatcher = SettingSelector & { + pageWatchers?: SettingWatcher[] +}; diff --git a/src/watchedSetting.ts b/src/watchedSetting.ts deleted file mode 100644 index 6c05da3d..00000000 --- a/src/watchedSetting.ts +++ /dev/null @@ -1,18 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. - -/** - * Fields that uniquely identify a watched configuration setting. - */ -export interface WatchedSetting { - /** - * The key for this setting. - */ - key: string; - - /** - * The label for this setting. - * Leaving this undefined means this setting does not have a label. - */ - label?: string; -} diff --git a/test/refresh.test.ts b/test/refresh.test.ts index 6d4aea89..fa1efbac 100644 --- a/test/refresh.test.ts +++ b/test/refresh.test.ts @@ -144,7 +144,7 @@ describe("dynamic refresh", function () { } }); expect(listKvRequestCount).eq(1); - expect(getKvRequestCount).eq(0); + expect(getKvRequestCount).eq(1); expect(settings).not.undefined; expect(settings.get("app.settings.fontColor")).eq("red"); expect(settings.get("app.settings.fontSize")).eq("40"); @@ -156,13 +156,13 @@ describe("dynamic refresh", function () { 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 + expect(getKvRequestCount).eq(1); // 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(getKvRequestCount).eq(2); expect(settings.get("app.settings.fontColor")).eq("blue"); }); @@ -178,7 +178,7 @@ describe("dynamic refresh", function () { } }); expect(listKvRequestCount).eq(1); - expect(getKvRequestCount).eq(0); + expect(getKvRequestCount).eq(1); expect(settings).not.undefined; expect(settings.get("app.settings.fontColor")).eq("red"); expect(settings.get("app.settings.fontSize")).eq("40"); @@ -192,8 +192,8 @@ describe("dynamic refresh", function () { 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); + expect(getKvRequestCount).eq(2); // one conditional request to detect change + expect(settings.get("app.settings.fontColor")).to.be.undefined; }); it("should not update values when unwatched setting changes", async () => { @@ -208,7 +208,7 @@ describe("dynamic refresh", function () { } }); expect(listKvRequestCount).eq(1); - expect(getKvRequestCount).eq(0); + expect(getKvRequestCount).eq(1); expect(settings).not.undefined; expect(settings.get("app.settings.fontColor")).eq("red"); expect(settings.get("app.settings.fontSize")).eq("40"); @@ -217,7 +217,7 @@ describe("dynamic refresh", function () { await sleepInMs(2 * 1000 + 1); await settings.refresh(); expect(listKvRequestCount).eq(1); - expect(getKvRequestCount).eq(1); + expect(getKvRequestCount).eq(2); expect(settings.get("app.settings.fontSize")).eq("40"); }); @@ -234,19 +234,19 @@ describe("dynamic refresh", function () { } }); expect(listKvRequestCount).eq(1); - expect(getKvRequestCount).eq(0); + expect(getKvRequestCount).eq(2); // two getKv requests for two watched settings expect(settings).not.undefined; expect(settings.get("app.settings.fontColor")).eq("red"); expect(settings.get("app.settings.fontSize")).eq("40"); // change setting addSetting("app.settings.bgColor", "white"); - updateSetting("app.settings.fontSize", "50"); + updateSetting("app.settings.fontColor", "blue"); 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(getKvRequestCount).eq(3); + expect(settings.get("app.settings.fontColor")).eq("blue"); expect(settings.get("app.settings.bgColor")).eq("white"); }); @@ -284,7 +284,7 @@ describe("dynamic refresh", function () { await sleepInMs(2 * 1000 + 1); await settings.refresh(); // should continue to refresh even if sentinel key doesn't change now expect(listKvRequestCount).eq(2); - expect(getKvRequestCount).eq(4); + expect(getKvRequestCount).eq(3); expect(settings.get("app.settings.bgColor")).eq("white"); }); @@ -370,7 +370,7 @@ 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(getKvRequestCount).eq(1); expect(settings).not.undefined; expect(settings.get("app.settings.fontColor")).eq("red"); expect(settings.get("app.settings.fontSize")).eq("40"); @@ -464,7 +464,7 @@ describe("dynamic refresh", function () { } }); expect(listKvRequestCount).eq(1); - expect(getKvRequestCount).eq(0); + expect(getKvRequestCount).eq(1); expect(settings).not.undefined; expect(settings.get("app.settings.fontColor")).eq("red"); @@ -477,12 +477,12 @@ describe("dynamic refresh", function () { settings.refresh(); // refresh "concurrently" } expect(listKvRequestCount).to.be.at.most(2); - expect(getKvRequestCount).to.be.at.most(1); + expect(getKvRequestCount).to.be.at.most(2); await sleepInMs(1000); // wait for all 5 refresh attempts to finish expect(listKvRequestCount).eq(2); - expect(getKvRequestCount).eq(1); + expect(getKvRequestCount).eq(2); expect(settings.get("app.settings.fontColor")).eq("blue"); }); diff --git a/test/requestTracing.test.ts b/test/requestTracing.test.ts index ee047bd4..6a69698e 100644 --- a/test/requestTracing.test.ts +++ b/test/requestTracing.test.ts @@ -6,6 +6,7 @@ import * as chai from "chai"; import chaiAsPromised from "chai-as-promised"; chai.use(chaiAsPromised); const expect = chai.expect; +import { AppConfigurationClient } from "@azure/app-configuration"; import { HttpRequestHeadersPolicy, createMockedConnectionString, createMockedKeyValue, createMockedFeatureFlag, createMockedTokenCredential, mockAppConfigurationClientListConfigurationSettings, restoreMocks, sinon, sleepInMs } from "./utils/testHelper.js"; import { ConfigurationClientManager } from "../src/configurationClientManager.js"; import { load } from "../src/index.js"; @@ -171,16 +172,25 @@ describe("request tracing", function () { value: "red" }].map(createMockedKeyValue)]); + const sentinel = createMockedKeyValue({ key: "sentinel", value: "initial value" }); + const getStub = sinon.stub(AppConfigurationClient.prototype, "getConfigurationSetting"); + getStub.onCall(0).returns(Promise.resolve({ statusCode: 200, ...sentinel } as any)); + const settings = await load(createMockedConnectionString(fakeEndpoint), { clientOptions, refreshOptions: { enabled: true, refreshIntervalInMs: 1_000, watchedSettings: [{ - key: "app.settings.fontColor" + key: "sentinel" }] } }); + + expect(settings.get("app.settings.fontColor")).eq("red"); // initial load should succeed + // we only mocked getConfigurationSetting for initial load, so the watch request during refresh will still use the SDK's pipeline, then the headerPolicy can capture the headers + getStub.restore(); + await sleepInMs(1_000 + 1_000); try { await settings.refresh();