-
Notifications
You must be signed in to change notification settings - Fork 2.1k
App Config - Query Param Ordering #46665
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
Merged
Changes from 3 commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
30c39e1
SortsQueryParams
mrm9084 2a863f2
Update CHANGELOG.md
mrm9084 c6e085f
Update QueryParamPolicy.java
mrm9084 1c451cf
Fixing tags
mrm9084 cd062ee
Style fixes
mrm9084 edbecab
Update QueryParamPolicyTest.java
mrm9084 858ae3c
Update QueryParamPolicyTest.java
mrm9084 387731f
Update QueryParamPolicyTest.java
mrm9084 d73ca61
Update assets.json
mrm9084 0efdeb4
review items
mrm9084 e0d02bb
Merge branch 'main' into QueryParamaterPipeline
mrm9084 404b483
Update QueryParamPolicy.java
mrm9084 7193a4a
Merge branch 'QueryParamaterPipeline' of https://github.com/mrm9084/a…
mrm9084 3cbc14e
Update QueryParamPolicy.java
mrm9084 aa6a387
Update QueryParamPolicy.java
mrm9084 267caf3
Update QueryParamPolicy.java
mrm9084 e1907f1
Updating assets.json for azure-monitor-query
jairmyree dd05f82
updating test files to use TME subscription
jairmyree b1570ec
Updating assets.json for azure-monitor-query-metrics
jairmyree 5d43d7b
Changing monitor app-config dependency to current to pull new changes
jairmyree 3255ca4
Changing monitor app-config dependencies
jairmyree 110b832
updating assets.json
jairmyree 5595677
Merge branch 'main' into QueryParamaterPipeline
jairmyree 18dd52e
Changing monitor dependency to unreleased app config
jairmyree f1e8dd6
Merge branch 'main' into QueryParamaterPipeline
jairmyree 6a1e309
Updating assets.json files
jairmyree 6e20621
updated pom and assets.json
jairmyree 94139bc
Update QueryParamPolicyTest.java
mrm9084 e393d1f
Merge branch 'main' into QueryParamaterPipeline
mrm9084 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
65 changes: 65 additions & 0 deletions
65
...ration/src/main/java/com/azure/data/appconfiguration/implementation/QueryParamPolicy.java
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,65 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| package com.azure.data.appconfiguration.implementation; | ||
|
|
||
| import java.net.MalformedURLException; | ||
| import java.util.Map; | ||
| import java.util.TreeMap; | ||
|
|
||
| import com.azure.core.http.HttpPipelineCallContext; | ||
| import com.azure.core.http.HttpPipelineNextPolicy; | ||
| import com.azure.core.http.HttpRequest; | ||
| import com.azure.core.http.HttpResponse; | ||
| import com.azure.core.http.policy.HttpPipelinePolicy; | ||
| import com.azure.core.util.UrlBuilder; | ||
| import com.azure.core.util.logging.ClientLogger; | ||
|
|
||
| import reactor.core.publisher.Mono; | ||
|
|
||
mrm9084 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| public class QueryParamPolicy implements HttpPipelinePolicy { | ||
| private static final ClientLogger LOGGER = new ClientLogger(QueryParamPolicy.class); | ||
|
|
||
mrm9084 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| @Override | ||
| public Mono<HttpResponse> process(HttpPipelineCallContext context, HttpPipelineNextPolicy next) { | ||
| HttpRequest request = context.getHttpRequest(); | ||
|
|
||
| try { | ||
| UrlBuilder urlBuilder = UrlBuilder.parse(request.getUrl()); | ||
| Map<String, String> queryParams = urlBuilder.getQuery(); | ||
|
|
||
| if (queryParams != null && !queryParams.isEmpty()) { | ||
| // Create a new TreeMap to automatically sort by keys alphabetically | ||
| Map<String, String> sortedParams = new TreeMap<>(); | ||
mrm9084 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| // Process each query parameter: convert key to lowercase and add to sorted map | ||
| for (Map.Entry<String, String> entry : queryParams.entrySet()) { | ||
| String key = entry.getKey(); | ||
| String value = entry.getValue(); | ||
|
|
||
| // Convert key to lowercase, but preserve special cases like $Select -> $select | ||
| String lowercaseKey = key.toLowerCase(); | ||
| sortedParams.put(lowercaseKey, value); | ||
mrm9084 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| // Clear existing query parameters and add sorted ones | ||
| urlBuilder.setQuery(null); | ||
| for (Map.Entry<String, String> entry : sortedParams.entrySet()) { | ||
| urlBuilder.addQueryParameter(entry.getKey(), entry.getValue()); | ||
| } | ||
|
|
||
| // Update the request URL with reordered parameters | ||
| request.setUrl(urlBuilder.toUrl()); | ||
| } | ||
| } catch (MalformedURLException e) { | ||
| // If URL parsing fails, continue without modification | ||
| LOGGER.warning( | ||
| "Failed to parse URL for query parameter normalization. " | ||
| + "Request will proceed with original URL. URL: {}, Error: {}", | ||
| request.getUrl(), e.getMessage(), e); | ||
| } | ||
|
|
||
| return next.process(); | ||
| } | ||
|
|
||
| } | ||
280 changes: 280 additions & 0 deletions
280
...on/src/test/java/com/azure/data/appconfiguration/implementation/QueryParamPolicyTest.java
mrm9084 marked this conversation as resolved.
Show resolved
Hide resolved
|
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,280 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| package com.azure.data.appconfiguration.implementation; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| import java.net.MalformedURLException; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import com.azure.core.http.HttpMethod; | ||
| import com.azure.core.http.HttpPipeline; | ||
| import com.azure.core.http.HttpPipelineBuilder; | ||
| import com.azure.core.http.HttpRequest; | ||
| import com.azure.core.http.HttpResponse; | ||
| import com.azure.core.http.policy.HttpPipelinePolicy; | ||
| import com.azure.core.test.SyncAsyncExtension; | ||
| import com.azure.core.test.annotation.SyncAsyncTest; | ||
| import com.azure.core.test.http.NoOpHttpClient; | ||
| import com.azure.core.util.Context; | ||
|
|
||
| import reactor.core.publisher.Mono; | ||
|
|
||
| /** | ||
| * Unit tests for QueryParamPolicy | ||
| */ | ||
| public class QueryParamPolicyTest { | ||
| private static final String BASE_URL = "http://localhost:8080"; | ||
| private static final String ENDPOINT_PATH = "/kv/test"; | ||
|
|
||
| /** | ||
| * Test that query parameters are sorted alphabetically | ||
| */ | ||
| @SyncAsyncTest | ||
| public void queryParametersAreSortedAlphabetically() { | ||
| final String originalUrl = BASE_URL + ENDPOINT_PATH + "?zebra=value1&alpha=value2&beta=value3"; | ||
| final String expectedUrl = BASE_URL + ENDPOINT_PATH + "?alpha=value2&beta=value3&zebra=value1"; | ||
|
|
||
| QueryParamPolicy queryParamPolicy = new QueryParamPolicy(); | ||
|
|
||
| HttpPipelinePolicy auditorPolicy = (context, next) -> { | ||
| final String actualUrl = context.getHttpRequest().getUrl().toString(); | ||
| assertEquals(expectedUrl, actualUrl, "Query parameters should be sorted alphabetically"); | ||
| return next.process(); | ||
| }; | ||
|
|
||
| final HttpPipeline pipeline = new HttpPipelineBuilder().httpClient(new NoOpHttpClient()) | ||
| .policies(queryParamPolicy, auditorPolicy) | ||
| .build(); | ||
|
|
||
| SyncAsyncExtension.execute(() -> sendRequestSync(pipeline, originalUrl), | ||
| () -> sendRequest(pipeline, originalUrl)); | ||
| } | ||
|
|
||
| /** | ||
| * Test that query parameter keys are converted to lowercase | ||
| */ | ||
| @SyncAsyncTest | ||
| public void queryParameterKeysAreConvertedToLowercase() { | ||
| final String originalUrl = BASE_URL + ENDPOINT_PATH + "?SELECT=field1&FILTER=condition&orderBy=field2"; | ||
| final String expectedUrl = BASE_URL + ENDPOINT_PATH + "?filter=condition&orderby=field2&select=field1"; | ||
|
|
||
| QueryParamPolicy queryParamPolicy = new QueryParamPolicy(); | ||
|
|
||
| HttpPipelinePolicy auditorPolicy = (context, next) -> { | ||
| final String actualUrl = context.getHttpRequest().getUrl().toString(); | ||
| assertEquals(expectedUrl, actualUrl, "Query parameter keys should be lowercase and sorted"); | ||
| return next.process(); | ||
| }; | ||
|
|
||
| final HttpPipeline pipeline = new HttpPipelineBuilder().httpClient(new NoOpHttpClient()) | ||
| .policies(queryParamPolicy, auditorPolicy) | ||
| .build(); | ||
|
|
||
| SyncAsyncExtension.execute(() -> sendRequestSync(pipeline, originalUrl), | ||
| () -> sendRequest(pipeline, originalUrl)); | ||
| } | ||
|
|
||
| /** | ||
| * Test that OData-style parameters like $select are handled correctly | ||
| */ | ||
| @SyncAsyncTest | ||
| public void oDataParametersAreHandledCorrectly() { | ||
| final String originalUrl | ||
| = BASE_URL + ENDPOINT_PATH + "?$Select=name,value&$Filter=startsWith(key,'test')&api-version=1.0"; | ||
| final String expectedUrl | ||
| = BASE_URL + ENDPOINT_PATH + "?$filter=startsWith(key,'test')&$select=name,value&api-version=1.0"; | ||
|
|
||
| QueryParamPolicy queryParamPolicy = new QueryParamPolicy(); | ||
|
|
||
| HttpPipelinePolicy auditorPolicy = (context, next) -> { | ||
| final String actualUrl = context.getHttpRequest().getUrl().toString(); | ||
| assertEquals(expectedUrl, actualUrl, "OData parameters should be lowercase and sorted"); | ||
| return next.process(); | ||
| }; | ||
|
|
||
| final HttpPipeline pipeline = new HttpPipelineBuilder().httpClient(new NoOpHttpClient()) | ||
| .policies(queryParamPolicy, auditorPolicy) | ||
| .build(); | ||
|
|
||
| SyncAsyncExtension.execute(() -> sendRequestSync(pipeline, originalUrl), | ||
| () -> sendRequest(pipeline, originalUrl)); | ||
| } | ||
|
|
||
| /** | ||
| * Test that URLs without query parameters are not modified | ||
| */ | ||
| @SyncAsyncTest | ||
| public void urlsWithoutQueryParametersAreNotModified() { | ||
| final String originalUrl = BASE_URL + ENDPOINT_PATH; | ||
|
|
||
| QueryParamPolicy queryParamPolicy = new QueryParamPolicy(); | ||
|
|
||
| HttpPipelinePolicy auditorPolicy = (context, next) -> { | ||
| final String actualUrl = context.getHttpRequest().getUrl().toString(); | ||
| assertEquals(originalUrl, actualUrl, "URLs without query parameters should not be modified"); | ||
| return next.process(); | ||
| }; | ||
|
|
||
| final HttpPipeline pipeline = new HttpPipelineBuilder().httpClient(new NoOpHttpClient()) | ||
| .policies(queryParamPolicy, auditorPolicy) | ||
| .build(); | ||
|
|
||
| SyncAsyncExtension.execute(() -> sendRequestSync(pipeline, originalUrl), | ||
| () -> sendRequest(pipeline, originalUrl)); | ||
| } | ||
|
|
||
| /** | ||
| * Test that empty query parameters are handled correctly | ||
| */ | ||
| @SyncAsyncTest | ||
| public void emptyQueryParametersAreHandledCorrectly() { | ||
| final String originalUrl = BASE_URL + ENDPOINT_PATH + "?"; | ||
|
|
||
| QueryParamPolicy queryParamPolicy = new QueryParamPolicy(); | ||
|
|
||
| HttpPipelinePolicy auditorPolicy = (context, next) -> { | ||
| final String actualUrl = context.getHttpRequest().getUrl().toString(); | ||
| // The URL should either be cleaned up or preserved as is | ||
| assertTrue(actualUrl.equals(BASE_URL + ENDPOINT_PATH) || actualUrl.equals(originalUrl), | ||
| "Empty query parameters should be handled gracefully"); | ||
| return next.process(); | ||
| }; | ||
|
|
||
| final HttpPipeline pipeline = new HttpPipelineBuilder().httpClient(new NoOpHttpClient()) | ||
| .policies(queryParamPolicy, auditorPolicy) | ||
| .build(); | ||
|
|
||
| SyncAsyncExtension.execute(() -> sendRequestSync(pipeline, originalUrl), | ||
| () -> sendRequest(pipeline, originalUrl)); | ||
| } | ||
|
|
||
| /** | ||
| * Test that query parameter values are preserved exactly | ||
| */ | ||
| @SyncAsyncTest | ||
| public void queryParameterValuesArePreserved() { | ||
| final String originalUrl = BASE_URL + ENDPOINT_PATH + "?key1=Value%20With%20Spaces&key2=SimpleValue&key3="; | ||
|
|
||
| QueryParamPolicy queryParamPolicy = new QueryParamPolicy(); | ||
|
|
||
| HttpPipelinePolicy auditorPolicy = (context, next) -> { | ||
| final String actualUrl = context.getHttpRequest().getUrl().toString(); | ||
| // Check that all values are preserved | ||
| assertTrue(actualUrl.contains("Value%20With%20Spaces"), "Values with spaces should be preserved"); | ||
| assertTrue(actualUrl.contains("SimpleValue"), "Simple values should be preserved"); | ||
| assertTrue(actualUrl.contains("key3="), "Empty values should be preserved"); | ||
| return next.process(); | ||
| }; | ||
|
|
||
| final HttpPipeline pipeline = new HttpPipelineBuilder().httpClient(new NoOpHttpClient()) | ||
| .policies(queryParamPolicy, auditorPolicy) | ||
| .build(); | ||
|
|
||
| SyncAsyncExtension.execute(() -> sendRequestSync(pipeline, originalUrl), | ||
| () -> sendRequest(pipeline, originalUrl)); | ||
| } | ||
|
|
||
| /** | ||
| * Test that duplicate query parameter keys are handled correctly | ||
| */ | ||
| @SyncAsyncTest | ||
| public void duplicateQueryParameterKeysAreHandled() { | ||
| final String originalUrl = BASE_URL + ENDPOINT_PATH + "?filter=condition1&select=field1&filter=condition2"; | ||
|
|
||
| QueryParamPolicy queryParamPolicy = new QueryParamPolicy(); | ||
|
|
||
| HttpPipelinePolicy auditorPolicy = (context, next) -> { | ||
| final String actualUrl = context.getHttpRequest().getUrl().toString(); | ||
| // The policy should handle duplicates gracefully (TreeMap behavior) | ||
| assertTrue(actualUrl.contains("filter="), "Filter parameter should be present"); | ||
| assertTrue(actualUrl.contains("select="), "Select parameter should be present"); | ||
| return next.process(); | ||
| }; | ||
|
|
||
| final HttpPipeline pipeline = new HttpPipelineBuilder().httpClient(new NoOpHttpClient()) | ||
| .policies(queryParamPolicy, auditorPolicy) | ||
| .build(); | ||
|
|
||
| SyncAsyncExtension.execute(() -> sendRequestSync(pipeline, originalUrl), | ||
| () -> sendRequest(pipeline, originalUrl)); | ||
| } | ||
|
|
||
| /** | ||
| * Test that malformed URLs are handled gracefully | ||
| */ | ||
| @Test | ||
| public void malformedUrlsAreHandledGracefully() { | ||
| // This test uses a synchronous approach since we're testing error handling | ||
| final String malformedUrl = "not-a-valid-url://[invalid"; | ||
|
|
||
| QueryParamPolicy queryParamPolicy = new QueryParamPolicy(); | ||
|
|
||
| HttpPipelinePolicy auditorPolicy = (context, next) -> { | ||
| // The URL should remain unchanged when malformed | ||
| final String actualUrl = context.getHttpRequest().getUrl().toString(); | ||
| assertEquals(malformedUrl, actualUrl, "Malformed URLs should remain unchanged"); | ||
| return next.process(); | ||
| }; | ||
|
|
||
| final HttpPipeline pipeline = new HttpPipelineBuilder().httpClient(new NoOpHttpClient()) | ||
| .policies(queryParamPolicy, auditorPolicy) | ||
| .build(); | ||
|
|
||
| // Test should not throw an exception | ||
| try { | ||
| HttpRequest request = new HttpRequest(HttpMethod.GET, malformedUrl); | ||
| pipeline.send(request, Context.NONE).block(); | ||
| } catch (Exception e) { | ||
| // If an exception occurs, it should not be from the QueryParamPolicy | ||
| assertTrue(e.getCause() instanceof MalformedURLException || e.getMessage().contains("not-a-valid-url"), | ||
| "Exception should be related to the malformed URL, not the policy"); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Test comprehensive scenario with mixed case, special characters, and sorting | ||
| */ | ||
| @SyncAsyncTest | ||
| public void comprehensiveQueryParameterNormalization() { | ||
| final String originalUrl = BASE_URL + ENDPOINT_PATH | ||
| + "?$TOP=10&API-Version=2023-10-01&$select=key,value&label=prod&$filter=startsWith(key,'app')&maxItems=100"; | ||
|
|
||
| QueryParamPolicy queryParamPolicy = new QueryParamPolicy(); | ||
|
|
||
| HttpPipelinePolicy auditorPolicy = (context, next) -> { | ||
| final String actualUrl = context.getHttpRequest().getUrl().toString(); | ||
|
|
||
| // Verify alphabetical ordering and lowercase conversion | ||
| String[] expectedOrder = { "$filter", "$select", "$top", "api-version", "label", "maxitems" }; | ||
| String queryString = actualUrl.substring(actualUrl.indexOf('?') + 1); | ||
| String[] actualParams = queryString.split("&"); | ||
|
|
||
| for (int i = 0; i < expectedOrder.length && i < actualParams.length; i++) { | ||
| String actualKey = actualParams[i].split("=")[0]; | ||
| assertEquals(expectedOrder[i], actualKey, | ||
| "Parameter at position " + i + " should be " + expectedOrder[i]); | ||
| } | ||
|
|
||
| return next.process(); | ||
| }; | ||
|
|
||
| final HttpPipeline pipeline = new HttpPipelineBuilder().httpClient(new NoOpHttpClient()) | ||
| .policies(queryParamPolicy, auditorPolicy) | ||
| .build(); | ||
|
|
||
| SyncAsyncExtension.execute(() -> sendRequestSync(pipeline, originalUrl), | ||
| () -> sendRequest(pipeline, originalUrl)); | ||
| } | ||
|
|
||
| private Mono<HttpResponse> sendRequest(HttpPipeline pipeline, String url) { | ||
| return pipeline.send(new HttpRequest(HttpMethod.GET, url), Context.NONE); | ||
| } | ||
|
|
||
| private HttpResponse sendRequestSync(HttpPipeline pipeline, String url) { | ||
| return pipeline.send(new HttpRequest(HttpMethod.GET, url), Context.NONE).block(); | ||
| } | ||
| } |
Oops, something went wrong.
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.