-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[Azure App Configuration] - Normalize the query parameters in request url #35962
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.
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 25 commits into
Azure:main
from
zhiyuanliang-ms:zhiyuanliang/sort-query-param
Oct 26, 2025
Merged
Changes from 14 commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
7b0cfd7
add policy
zhiyuanliang-ms 1ffceb0
add testcase
zhiyuanliang-ms bb14d10
remove debug code
zhiyuanliang-ms d81b70a
Merge branch 'main' of https://github.com/zhiyuanliang-ms/azure-sdk-f…
zhiyuanliang-ms 042e19f
update recording
zhiyuanliang-ms a3c0992
fix format
zhiyuanliang-ms b0f8ce2
update
zhiyuanliang-ms aca2dd9
Merge branch 'main' of https://github.com/zhiyuanliang-ms/azure-sdk-f…
zhiyuanliang-ms 387d730
add test for tag query
zhiyuanliang-ms 1bd5a4e
update
zhiyuanliang-ms 141e4f7
Merge branch 'main' of https://github.com/zhiyuanliang-ms/azure-sdk-f…
zhiyuanliang-ms 857c849
update
zhiyuanliang-ms 6376b8c
update
zhiyuanliang-ms a68e541
update
zhiyuanliang-ms 3f06358
update
zhiyuanliang-ms ee92713
Merge branch 'main' of https://github.com/zhiyuanliang-ms/azure-sdk-f…
zhiyuanliang-ms b461666
update
zhiyuanliang-ms dcf63a6
update test
zhiyuanliang-ms ccbb155
update test
zhiyuanliang-ms 5218baf
handle corner case that query doesn't have =
zhiyuanliang-ms 4b538f5
handle corner case that query is empty
zhiyuanliang-ms 43994c3
update testcase
zhiyuanliang-ms 445920a
update testcase
zhiyuanliang-ms 1fdc43a
update testcase
zhiyuanliang-ms e35413a
update
zhiyuanliang-ms File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
sdk/appconfiguration/app-configuration/src/generated/src/models/parameters.ts
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
67 changes: 67 additions & 0 deletions
67
sdk/appconfiguration/app-configuration/src/internal/queryParamPolicy.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| import type { | ||
| PipelinePolicy, | ||
| PipelineRequest, | ||
| PipelineResponse, | ||
| SendRequest, | ||
| } from "@azure/core-rest-pipeline"; | ||
|
|
||
| /** | ||
| * Creates a PipelinePolicy that normalizes query parameters: | ||
| * - Lowercase names | ||
| * - Sort by lowercase name | ||
| * - Preserve the relative order of duplicates | ||
| */ | ||
| export function queryParamPolicy(): PipelinePolicy { | ||
| return { | ||
| name: "queryParamPolicy", | ||
| async sendRequest(request: PipelineRequest, next: SendRequest): Promise<PipelineResponse> { | ||
| try { | ||
| const originalUrl: string = request.url; | ||
| const url = new URL(originalUrl); | ||
|
|
||
| if (url.search === "") { | ||
| return next(request); | ||
| } | ||
|
|
||
| const params: ParamEntry[] = []; | ||
| for (const [name, value] of url.searchParams.entries()) { | ||
| params.push({ lowercaseName: name.toLowerCase(), value }); | ||
| } | ||
zhiyuanliang-ms marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| // Modern JavaScript Array.prototype.sort is stable | ||
| // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort#sort_stability | ||
| params.sort((a, b) => { | ||
| if (a.lowercaseName < b.lowercaseName) { | ||
| return -1; | ||
| } else if (a.lowercaseName > b.lowercaseName) { | ||
| return 1; | ||
| } | ||
| return 0; | ||
| }); | ||
|
|
||
| const newSearchParams = new URLSearchParams(); | ||
| for (const p of params) { | ||
| newSearchParams.append(p.lowercaseName, p.value); | ||
| } | ||
zhiyuanliang-ms marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| const newUrl = url.origin + url.pathname + "?" + newSearchParams.toString() + url.hash; | ||
zhiyuanliang-ms marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| if (newUrl !== originalUrl) { | ||
| request.url = newUrl; | ||
| } | ||
| } catch { | ||
| // If anything goes wrong, fall back to sending the original request. | ||
| console.log("Failed to normalize query parameters."); | ||
zhiyuanliang-ms marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| return next(request); | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| interface ParamEntry { | ||
| lowercaseName: string; | ||
| value: string; | ||
| } | ||
File renamed without changes.
2 changes: 1 addition & 1 deletion
2
sdk/appconfiguration/app-configuration/test/internal/node/http.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
64 changes: 64 additions & 0 deletions
64
sdk/appconfiguration/app-configuration/test/internal/queryParamPolicy.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| import { describe, it, expect } from "vitest"; | ||
| import { queryParamPolicy } from "../../src/internal/queryParamPolicy.js"; | ||
| import { createPipelineRequest, createHttpHeaders } from "@azure/core-rest-pipeline"; | ||
| import type { PipelineRequest, PipelineResponse } from "@azure/core-rest-pipeline"; | ||
|
|
||
| function mockNext(returnStatus: number = 200) { | ||
| return async (request: PipelineRequest): Promise<PipelineResponse> => { | ||
| return { | ||
| request, | ||
| headers: createHttpHeaders({ "url-lookup": request.url }), | ||
| status: returnStatus, | ||
| } as PipelineResponse; | ||
| }; | ||
| } | ||
|
|
||
| describe("urlQueryParamsNormalizationPolicy", () => { | ||
| it("normalizes query parameters", async () => { | ||
| const policy = queryParamPolicy(); | ||
| const request = createPipelineRequest({ | ||
| url: "https://example.azconfig.io/kv?api-version=2023-11-01&After=abcdefg&tags=tag3%3Dvalue3&key=*&label=dev&$Select=key&tags=tag2%3Dvalue2&tags=tag1%3Dvalue1", | ||
| }); | ||
| const response = await policy.sendRequest(request, mockNext()); | ||
| const finalUrl = response.headers.get("url-lookup")!; | ||
| console.log(finalUrl); | ||
zhiyuanliang-ms marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| expect( | ||
| finalUrl.endsWith( | ||
| "?%24select=key&after=abcdefg&api-version=2023-11-01&key=*&label=dev&tags=tag3%3Dvalue3&tags=tag2%3Dvalue2&tags=tag1%3Dvalue1", | ||
| ), | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| it("keeps original order of query parameters", async () => { | ||
| const policy = queryParamPolicy(); | ||
| const request = createPipelineRequest({ | ||
| url: "https://example.azconfig.io/kv?tags=tag2&api-version=2023-11-01&tags=tag1", | ||
| }); | ||
| const response = await policy.sendRequest(request, mockNext()); | ||
| const finalUrl = response.headers.get("url-lookup")!; | ||
| expect(finalUrl.endsWith("?api-version=2023-11-01&tags=tag2&tags=tag1")).toBe(true); | ||
| }); | ||
|
|
||
| it("keeps key with no value", async () => { | ||
| const policy = queryParamPolicy(); | ||
| const request = createPipelineRequest({ | ||
| url: "https://example.azconfig.io/kv?tags&api-version=2023-11-01", | ||
| }); | ||
| const response = await policy.sendRequest(request, mockNext()); | ||
| const finalUrl = response.headers.get("url-lookup")!; | ||
| expect(finalUrl.endsWith("?api-version=2023-11-01&tags=")).toBe(true); | ||
| }); | ||
|
|
||
| it("removes redundant &", async () => { | ||
| const policy = queryParamPolicy(); | ||
| const request = createPipelineRequest({ | ||
| url: "https://example.azconfig.io/kv?&&&", | ||
| }); | ||
| const response = await policy.sendRequest(request, mockNext()); | ||
| const finalUrl = response.headers.get("url-lookup")!; | ||
| expect(finalUrl.endsWith("?")).toBe(true); | ||
| }); | ||
| }); | ||
179 changes: 179 additions & 0 deletions
179
sdk/appconfiguration/app-configuration/test/public/queryParam.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,179 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
zhiyuanliang-ms marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // Licensed under the MIT License. | ||
|
|
||
| import type { AppConfigurationClient } from "../../src/index.js"; | ||
| import type { Recorder } from "@azure-tools/test-recorder"; | ||
| import { isLiveMode } from "@azure-tools/test-recorder"; | ||
| import type { PipelinePolicy } from "@azure/core-rest-pipeline"; | ||
| import { createAppConfigurationClientForTests, startRecorder } from "./utils/testHelpers.js"; | ||
| import { describe, it, assert, beforeEach, afterEach } from "vitest"; | ||
|
|
||
| describe("request url query parameters", () => { | ||
| let recorder: Recorder; | ||
|
|
||
| beforeEach(async (ctx) => { | ||
| recorder = await startRecorder(ctx); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| await recorder.stop(); | ||
| }); | ||
|
|
||
| describe("normalize query parameters", () => { | ||
| it("sort query params in alphabetical order", async () => { | ||
| const key = recorder.variable( | ||
| "sortQueryParams", | ||
| `sortQueryParams${Math.floor(Math.random() * 1000)}`, | ||
| ); | ||
|
|
||
| const { getCapturedUrl, client } = createClientWithUrlCapturePolicy(); | ||
|
|
||
| await client.addConfigurationSetting({ | ||
| key, | ||
| label: "dev", | ||
| value: "some value", | ||
| tags: { tag1: "value1" }, | ||
| }); | ||
|
|
||
| const configurationSetting = await client.getConfigurationSetting( | ||
| { key, label: "dev" }, | ||
| { fields: ["key"] }, | ||
| ); | ||
|
|
||
| assert.ok( | ||
| getCapturedUrl(), | ||
| "Expected to have captured a request URL for getConfigurationSetting", | ||
| ); | ||
| // Regex enforces exact ordering of query params: $select (or %24select), api-version, label | ||
| let queryOrderRegex = /\?(?:\$|%24)select=key&api-version=[^&]+&label=dev$/; | ||
| assert.match( | ||
| getCapturedUrl()!, | ||
| queryOrderRegex, | ||
| `Query parameters not in expected order or values. URL: ${getCapturedUrl()}`, | ||
| ); | ||
|
|
||
| assert.equal(configurationSetting.key, key); | ||
|
|
||
| const listResult = client.listConfigurationSettings({ keyFilter: "*", labelFilter: "dev" }); | ||
|
|
||
| for await (const _ of listResult.byPage()) { | ||
| // do nothing, just drain the iterator | ||
| } | ||
|
|
||
| // Regex enforces exact ordering of query params: api-version, key, label | ||
| queryOrderRegex = /\?api-version=[^&]+&key=\*&label=dev$/; | ||
| assert.match( | ||
| getCapturedUrl()!, | ||
| queryOrderRegex, | ||
| `Query parameters not in expected order or values. URL: ${getCapturedUrl()}`, | ||
| ); | ||
|
|
||
| const listWithTagsResult = client.listConfigurationSettings({ | ||
| keyFilter: "*", | ||
| labelFilter: "dev", | ||
| tagsFilter: ["tag2=value2", "tag1=value1", "tag3=value3"], | ||
| }); | ||
|
|
||
| for await (const _ of listWithTagsResult.byPage()) { | ||
| // do nothing, just drain the iterator | ||
| } | ||
|
|
||
| // Regex enforces exact ordering of query params: api-version, key, label, tags | ||
| queryOrderRegex = | ||
| /\?api-version=[^&]+&key=\*&label=dev&tags=tag2%3Dvalue2&tags=tag1%3Dvalue1&tags=tag3%3Dvalue3$/; | ||
| assert.match( | ||
| getCapturedUrl()!, | ||
| queryOrderRegex, | ||
| `Query parameters not in expected order or values. URL: ${getCapturedUrl()}`, | ||
| ); | ||
|
|
||
| await client.deleteConfigurationSetting({ key, label: "dev" }); | ||
| }); | ||
|
|
||
| // This occasionally hits 429 error (throttling) since we are making 100s of requests in the test to create, get and delete keys. | ||
| // To avoid hitting the service with too many requests, skipping the test in live. | ||
| // More details at https://github.com/Azure/azure-sdk-for-js/issues/16743 | ||
| // | ||
| // Remove the following line if you want to hit the live service. | ||
| it( | ||
| "sort query params in alphabetical order - continuation token", | ||
| { skip: isLiveMode() }, | ||
| async () => { | ||
| const key = recorder.variable( | ||
| "sortQueryParamsMultiplePages", | ||
| `sortQueryParamsMultiplePages${Math.floor(Math.random() * 1000)}`, | ||
| ); | ||
|
|
||
| const { getCapturedUrl, client } = createClientWithUrlCapturePolicy(); | ||
|
|
||
| // this number is arbitrarily chosen to match the size of a page + 1 | ||
| const expectedNumberOfLabels = 101; | ||
|
|
||
| let addSettingPromises = []; | ||
|
|
||
| for (let i = 0; i < expectedNumberOfLabels; i++) { | ||
| addSettingPromises.push( | ||
| client.addConfigurationSetting({ | ||
| key, | ||
| value: `the value for ${i}`, | ||
| label: i.toString(), | ||
| }), | ||
| ); | ||
|
|
||
| if (i !== 0 && i % 2 === 0) { | ||
| await Promise.all(addSettingPromises); | ||
| addSettingPromises = []; | ||
| } | ||
| } | ||
|
|
||
| const listResult = client.listConfigurationSettings({ | ||
| keyFilter: key, | ||
| }); | ||
|
|
||
| for await (const _ of listResult.byPage()) { | ||
| // do nothing, just drain the iterator | ||
| } | ||
|
|
||
| // Regex enforces exact ordering of query params for continuation page: after, api-version, key | ||
| // Note that only the request for the second page has the 'after' query param | ||
| const queryOrderRegex = new RegExp(`\\?after=[^&]+&api-version=[^&]+&key=[^&]+$`); | ||
|
|
||
| assert.match( | ||
| getCapturedUrl()!, | ||
| queryOrderRegex, | ||
| `Query parameters not in expected order or values. URL: ${getCapturedUrl()}`, | ||
| ); | ||
|
|
||
| for (let i = 0; i < expectedNumberOfLabels; i++) { | ||
| await client.deleteConfigurationSetting({ key, label: i.toString() }); | ||
| } | ||
| }, | ||
| ); | ||
|
|
||
| function createClientWithUrlCapturePolicy(): { | ||
| getCapturedUrl: () => string | undefined; | ||
| client: AppConfigurationClient; | ||
| } { | ||
| let capturedUrl: string | undefined; | ||
| const urlCapturePolicy: PipelinePolicy = { | ||
| name: "UrlCapturePolicy", | ||
| async sendRequest(request, next) { | ||
| capturedUrl = request.url; | ||
| return next(request); | ||
| }, | ||
| }; | ||
|
|
||
| const client = createAppConfigurationClientForTests( | ||
| recorder.configureClientOptions({ | ||
| additionalPolicies: [ | ||
| { | ||
| policy: urlCapturePolicy, | ||
| position: "perRetry", | ||
| }, | ||
| ], | ||
| }), | ||
| ); | ||
| return { getCapturedUrl: () => capturedUrl, client }; | ||
| } | ||
| }); | ||
| }); | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.