[core-client] Skip parameter overwriting if the path is absolute (#18310

) * Skip parameter overwriting if the path is absolute * add a comment * update changelog * update TA recordings * address feedback
Azure · Oct 22, 2021 · a00a662 · a00a662
1 parent f26bb90
commit a00a662
Show file tree

Hide file tree

Showing 5 changed files with 280 additions and 137 deletions.
diff --git a/sdk/core/core-client/CHANGELOG.md b/sdk/core/core-client/CHANGELOG.md
@@ -8,6 +8,8 @@
 
 ### Bugs Fixed
 
+- Skip query parameter replacement for absolute URLs. [PR #18310](https://github.com/Azure/azure-sdk-for-js/pull/18310)
+
 ### Other Changes
 
 ## 1.3.1 (2021-09-30)

diff --git a/sdk/core/core-client/src/urlHelpers.ts b/sdk/core/core-client/src/urlHelpers.ts
@@ -24,6 +24,8 @@ export function getRequestUrl(
     fallbackObject
   );
 
+  let isAbsolutePath = false;
+
   let requestUrl = replaceAll(baseUri, urlReplacements);
   if (operationSpec.path) {
     const path = replaceAll(operationSpec.path, urlReplacements);
@@ -32,6 +34,7 @@ export function getRequestUrl(
     // ignore the baseUri.
     if (isAbsoluteUrl(path)) {
       requestUrl = path;
+      isAbsolutePath = true;
     } else {
       requestUrl = appendPath(requestUrl, path);
     }
@@ -42,7 +45,13 @@ export function getRequestUrl(
     operationArguments,
     fallbackObject
   );
-  requestUrl = appendQueryParams(requestUrl, queryParams, sequenceParams);
+  /**
+   * Notice that this call sets the `noOverwrite` parameter to true if the `requestUrl`
+   * is an absolute path. This ensures that existing query parameter values in `requestUrl`
+   * do not get overwritten. On the other hand when `requestUrl` is not absolute path, it
+   * is still being built so there is nothing to overwrite.
+   */
+  requestUrl = appendQueryParams(requestUrl, queryParams, sequenceParams, isAbsolutePath);
 
   return requestUrl;
 }
@@ -237,7 +246,8 @@ function simpleParseQueryParams(queryString: string): Map<string, string | strin
 export function appendQueryParams(
   url: string,
   queryParams: Map<string, string | string[]>,
-  sequenceParams: Set<string>
+  sequenceParams: Set<string>,
+  noOverwrite: boolean = false
 ): string {
   if (queryParams.size === 0) {
     return url;
@@ -261,13 +271,14 @@ export function appendQueryParams(
         existingValue.push(value);
       }
     } else if (existingValue) {
-      let newValue = value;
       if (Array.isArray(value)) {
         value.unshift(existingValue);
       } else if (sequenceParams.has(name)) {
-        newValue = [existingValue, value];
+        combinedParams.set(name, [existingValue, value]);
+      }
+      if (!noOverwrite) {
+        combinedParams.set(name, value);
       }
-      combinedParams.set(name, newValue);
     } else {
       combinedParams.set(name, value);
     }

diff --git a/sdk/core/core-client/test/serviceClient.spec.ts b/sdk/core/core-client/test/serviceClient.spec.ts
@@ -15,13 +15,15 @@ import {
   CompositeMapper,
   OperationSpec,
   serializationPolicy,
-  FullOperationResponse
+  FullOperationResponse,
+  OperationQueryParameter
 } from "../src";
 import {
   createHttpHeaders,
   createEmptyPipeline,
   HttpClient,
-  createPipelineRequest
+  createPipelineRequest,
+  PipelineRequest
 } from "@azure/core-rest-pipeline";
 
 import {
@@ -1362,6 +1364,65 @@ describe("ServiceClient", function() {
       assert.include(error.message, "cannot be null or undefined");
     }
   });
+
+  it("should not replace existing queries in request URLs", async function() {
+    let request: PipelineRequest;
+    const topQueryParam: OperationQueryParameter = {
+      parameterPath: ["options", "top"],
+      mapper: {
+        defaultValue: 20,
+        constraints: {
+          InclusiveMaximum: 50,
+          InclusiveMinimum: 1
+        },
+        serializedName: "$top",
+        type: {
+          name: "Number"
+        }
+      }
+    };
+    const skipQueryParam: OperationQueryParameter = {
+      parameterPath: ["options", "skip"],
+      mapper: {
+        defaultValue: 0,
+        constraints: {
+          InclusiveMinimum: 0
+        },
+        serializedName: "$skip",
+        type: {
+          name: "Number"
+        }
+      }
+    };
+    const operationSpec: OperationSpec = {
+      path: "https://example.com/path?$skip=10",
+      queryParameters: [topQueryParam, skipQueryParam],
+      httpMethod: "GET",
+      responses: {
+        200: {
+          bodyMapper: { type: { name: "String" } }
+        }
+      },
+      baseUrl: "http://example.com",
+      serializer: createSerializer()
+    };
+
+    const client = new ServiceClient({
+      httpClient: {
+        sendRequest: (req) => {
+          request = req;
+          return Promise.resolve({
+            request: req,
+            status: 200,
+            headers: createHttpHeaders(),
+            bodyAsText: `"dummy string"`
+          });
+        }
+      }
+    });
+    await client.sendOperationRequest<string>({ options: { top: 10 } as any }, operationSpec);
+    assert.equal(request!.url, "https://example.com/path?$skip=10&$top=10");
+  });
 });
 
 async function testSendOperationRequest(