[core-client] Bugfixes for unblocking data-tables

sdk/core/core-client/review/core-client.api.md

@@ -7,6 +7,7 @@
 import { AbortSignalLike } from '@azure/abort-controller';
 import { HttpMethods } from '@azure/core-https';
 import { HttpsClient } from '@azure/core-https';
+import { InternalPipelineOptions } from '@azure/core-https';
 import { OperationTracingOptions } from '@azure/core-tracing';
 import { Pipeline } from '@azure/core-https';
 import { PipelinePolicy } from '@azure/core-https';
@@ -34,6 +35,15 @@ export interface BaseMapper {
     xmlNamespacePrefix?: string;
 }
 
+// @public
+export interface ClientPipelineOptions extends InternalPipelineOptions {
+    credentialOptions?: {
+        baseUri?: string;
+        credential?: TokenCredential;
+    };
+    deserializationOptions?: DeserializationPolicyOptions;
+}
+
 // @public (undocumented)
 export interface CompositeMapper extends BaseMapper {
     // (undocumented)
@@ -58,6 +68,9 @@ export interface CompositeMapperType {
     uberParent?: string;
 }
 
+// @public
+export function createClientPipeline(options?: ClientPipelineOptions): Pipeline;
+
 // @public
 export function createSerializer(modelMappers?: {
     [key: string]: any;
@@ -324,6 +337,9 @@ export interface ServiceClientOptions {
     baseUri?: string;
     credential?: TokenCredential;
     httpsClient?: HttpsClient;
+    parseXML?: (str: string, opts?: {
+        includeRoot?: boolean;
+    }) => Promise<any>;
     pipeline?: Pipeline;
     requestContentType?: string;
     stringifyXML?: (obj: any, opts?: {

sdk/core/core-client/src/index.ts

@@ -3,7 +3,12 @@
 
 export { createSerializer, MapperTypeNames } from "./serializer";
 export { createSpanFunction } from "./createSpan";
-export { ServiceClient, ServiceClientOptions } from "./serviceClient";
+export {
+  ServiceClient,
+  ServiceClientOptions,
+  createClientPipeline,
+  ClientPipelineOptions
+} from "./serviceClient";
 export {
   OperationSpec,
   OperationArguments,

sdk/core/core-client/src/serviceClient.ts

@@ -1,7 +1,7 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.
 
-import { TokenCredential, isTokenCredential } from "@azure/core-auth";
+import { TokenCredential } from "@azure/core-auth";
 import {
   DefaultHttpsClient,
   HttpsClient,
@@ -10,7 +10,8 @@ import {
   Pipeline,
   createPipelineRequest,
   createPipelineFromOptions,
-  bearerTokenAuthenticationPolicy
+  bearerTokenAuthenticationPolicy,
+  InternalPipelineOptions
 } from "@azure/core-https";
 import {
   OperationResponse,
@@ -27,7 +28,7 @@ import { MapperTypeNames } from "./serializer";
 import { getRequestUrl } from "./urlHelpers";
 import { isPrimitiveType } from "./utils";
 import { getOperationArgumentValueFromParameter } from "./operationHelpers";
-import { deserializationPolicy } from "./deserializationPolicy";
+import { deserializationPolicy, DeserializationPolicyOptions } from "./deserializationPolicy";
 
 /**
  * Options to be provided while creating the client.
@@ -60,6 +61,11 @@ export interface ServiceClientOptions {
    * A method that is able to turn an XML object model into a string.
    */
   stringifyXML?: (obj: any, opts?: { rootName?: string }) => string;
+
+  /**
+   * A method that is able to parse XML.
+   */
+  parseXML?: (str: string, opts?: { includeRoot?: boolean }) => Promise<any>;
 }
 
 /**
@@ -100,10 +106,14 @@ export class ServiceClient {
     this._requestContentType = options.requestContentType;
     this._baseUri = options.baseUri;
     this._httpsClient = options.httpsClient || new DefaultHttpsClient();
+    this._stringifyXML = options.stringifyXML;
     this._pipeline =
       options.pipeline ||
-      createDefaultPipeline({ baseUri: this._baseUri, credential: options.credential });
-    this._stringifyXML = options.stringifyXML;
+      createDefaultPipeline({
+        baseUri: this._baseUri,
+        credential: options.credential,
+        parseXML: options.parseXML
+      });
   }
 
   /**
@@ -351,22 +361,56 @@ function getXmlValueWithNamespace(
 }
 
 function createDefaultPipeline(
-  options: { baseUri?: string; credential?: TokenCredential } = {}
+  options: {
+    baseUri?: string;
+    credential?: TokenCredential;
+    parseXML?: (str: string, opts?: { includeRoot?: boolean }) => Promise<any>;
+  } = {}
 ): Pipeline {
-  const pipeline = createPipelineFromOptions({});
+  return createClientPipeline({
+    credentialOptions: options,
+    deserializationOptions: {
+      parseXML: options.parseXML
+    }
+  });
+}
 
-  const credential = options.credential;
+/**
+ * Options for creating a Pipeline to use with ServiceClient.
+ * Mostly for customizing the auth policy (if using token auth) or
+ * the deserialization options when using XML.
+ */
+export interface ClientPipelineOptions extends InternalPipelineOptions {
+  /**
+   * Options to customize bearerTokenAuthenticationPolicy.
+   */
+  credentialOptions?: { baseUri?: string; credential?: TokenCredential };
+  /**
+   * Options to customize deserializationPolicy.
+   */
+  deserializationOptions?: DeserializationPolicyOptions;
+}
+
+/**
+ * Creates a new Pipeline for use with a Service Client.
+ * Adds in deserializationPolicy by default.
+ * Also adds in bearerTokenAuthenticationPolicy if passed a TokenCredential.
+ * @param options Options to customize the created pipeline.
+ */
+export function createClientPipeline(options: ClientPipelineOptions = {}): Pipeline {
+  const pipeline = createPipelineFromOptions(options ?? {});
+
+  const credential = options.credentialOptions?.credential;
   if (credential) {
-    if (isTokenCredential(credential)) {
-      pipeline.addPolicy(
-        bearerTokenAuthenticationPolicy({ credential, scopes: `${options.baseUri || ""}/.default` })
-      );
-    } else {
-      throw new Error("The credential argument must implement the TokenCredential interface");
-    }
+    pipeline.addPolicy(
+      bearerTokenAuthenticationPolicy({
+        credential,
+        scopes: `${options.credentialOptions?.baseUri || ""}/.default`
+      })
+    );
   }
 
-  pipeline.addPolicy(deserializationPolicy(), { phase: "Serialize" });
+  pipeline.addPolicy(deserializationPolicy(options.deserializationOptions), { phase: "Serialize" });
 
   return pipeline;
 }

sdk/core/core-client/src/urlHelpers.ts

@@ -34,7 +34,7 @@ export function getRequestUrl(
     if (isAbsoluteUrl(path)) {
       requestUrl = path;
     } else {
-      requestUrl = appendPath(requestUrl, operationSpec.path);
+      requestUrl = appendPath(requestUrl, path);
     }
   }
 
@@ -87,21 +87,27 @@ function isAbsoluteUrl(url: string): boolean {
   return url.includes("://");
 }
 
-function appendPath(url: string, path?: string): string {
-  let result = url;
-  let toAppend = path;
-  if (toAppend) {
-    if (!result.endsWith("/")) {
-      result = `${result}/`;
-    }
+function appendPath(url: string, pathToAppend?: string): string {
+  if (!pathToAppend) {
+    return url;
+  }
 
-    if (toAppend.startsWith("/")) {
-      toAppend = toAppend.substring(1);
-    }
+  const parsedUrl = new URL(url);
+  let newPath = parsedUrl.pathname;
 
-    result = result + toAppend;
+  if (!newPath.endsWith("/")) {
+    newPath = `${newPath}/`;
   }
-  return result;
+
+  if (pathToAppend.startsWith("/")) {
+    pathToAppend = pathToAppend.substring(1);
+  }
+
+  newPath = newPath + pathToAppend;
+
+  parsedUrl.pathname = newPath;
+
+  return parsedUrl.toString();
 }
 
 function calculateQueryParameters(
@@ -176,17 +182,41 @@ function calculateQueryParameters(
   return result;
 }
 
+function simpleParseQueryParams(queryString: string): Array<[string, string]> {
+  if (!queryString || queryString[0] !== "?") {
+    return [];
+  }
+
+  // remove the leading ?
+  queryString = queryString.slice(1);
+
+  const pairs = queryString.split("&");
+
+  return pairs.map((pair) => {
+    const [name, value] = pair.split("=", 2);
+    return [name, value];
+  });
+}
+
 function appendQueryParams(url: string, queryParams: Map<string, string | string[]>): string {
+  if (queryParams.size === 0) {
+    return url;
+  }
+
   const parsedUrl = new URL(url);
 
-  const combinedParams = new Map<string, string | string[]>(queryParams);
+  // QUIRK: parsedUrl.searchParams will have their name/value pairs decoded, which
+  // can change their meaning to the server, such as in the case of a SAS signature.
+  // To avoid accidentally un-encoding a query param, we parse the key/values ourselves
+  const existingParams = simpleParseQueryParams(parsedUrl.search);
+  const combinedParams = new Map<string, string | string[]>(existingParams);
 
-  for (const [name, value] of parsedUrl.searchParams) {
+  for (const [name, value] of queryParams) {
     const existingValue = combinedParams.get(name);
     if (Array.isArray(existingValue)) {
-      existingValue.push(value);
+      existingValue.push(...value);
     } else if (existingValue) {
-      combinedParams.set(name, [existingValue, value]);
+      combinedParams.set(name, [existingValue, ...value]);
     } else {
       combinedParams.set(name, value);
     }

sdk/core/core-client/test/serviceClient.spec.ts

@@ -157,7 +157,7 @@ describe("ServiceClient", function() {
     await testSendOperationRequest(["1", "2", "3"], "Multi", false, "?q=1&q=2&q=3");
     await testSendOperationRequest(["1,2", "3,4", "5"], "Multi", false, "?q=1%2C2&q=3%2C4&q=5");
     await testSendOperationRequest(["1,2", "3,4", "5"], "Multi", true, "?q=1,2&q=3,4&q=5");
-    await testSendOperationRequest([], "Multi", true, "https://example.com/");
+    await testSendOperationRequest([], "Multi", true, "https://example.com");
   });
 
   it("should deserialize response bodies", async function() {

sdk/core/core-client/test/urlHelpers.spec.ts

@@ -0,0 +1,86 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+import { assert } from "chai";
+import { getRequestUrl } from "../src/urlHelpers";
+import { OperationSpec, OperationURLParameter, createSerializer } from "../src";
+
+describe("getRequestUrl", function() {
+  const urlParameter: OperationURLParameter = {
+    parameterPath: "url",
+    mapper: {
+      serializedName: "url",
+      required: true,
+      xmlName: "url",
+      type: {
+        name: "String"
+      }
+    },
+    skipEncoding: true
+  };
+
+  const tableParameter: OperationURLParameter = {
+    parameterPath: "table",
+    mapper: {
+      serializedName: "table",
+      required: true,
+      xmlName: "table",
+      type: {
+        name: "String"
+      }
+    }
+  };
+
+  const serializer = createSerializer({}, false);
+
+  const operationSpec: OperationSpec = {
+    path: "/Tables('{table}')",
+    httpMethod: "DELETE",
+    responses: {},
+    urlParameters: [urlParameter, tableParameter],
+    serializer
+  };
+
+  it("should handle nested replacements", function() {
+    const result = getRequestUrl(
+      "{url}",
+      operationSpec,
+      { table: "TestTable" },
+      { url: "https://test.com" }
+    );
+    assert.strictEqual(result, "https://test.com/Tables('TestTable')");
+  });
+
+  it("should handle query parameters on the base url", function() {
+    const result = getRequestUrl(
+      "{url}",
+      operationSpec,
+      { table: "TestTable" },
+      { url: "https://test.com?superSecretKey=awesome" }
+    );
+    assert.strictEqual(result, "https://test.com/Tables('TestTable')?superSecretKey=awesome");
+  });
+
+  it("should not modify needlessly encoded query parameters", function() {
+    const specWithQueryParams: OperationSpec = {
+      ...operationSpec,
+      queryParameters: [
+        {
+          parameterPath: "extraValue",
+          mapper: { type: { name: "String" }, serializedName: "extraValue", required: true },
+          skipEncoding: true
+        }
+      ]
+    };
+    const result = getRequestUrl(
+      "{url}",
+      specWithQueryParams,
+      { table: "TestTable", extraValue: "%27blah%27" },
+      { url: "https://test.com?superSecretKey=Qai%2B4%2FIM%3D" }
+    );
+    assert.strictEqual(
+      result,
+      "https://test.com/Tables('TestTable')?superSecretKey=Qai%2B4%2FIM%3D&extraValue=%27blah%27"
+    );
+  });
+});