[core-http] Support overriding xml parser char key via options (Azure#12065)

* [core-http] Support overriding xml parser char key via options

Currently our xml parser uses hard-coded `_` as key to access parsed
xml element content. This causes issue like storage customer getting
errors listing blobs when they use `_` as Blob metadata key.

This PR add support to allow customizing the xml parser char key via
options. Currently `xmlCharKey` option is supported, with room for
other xml parser options (e.g., attr key for accessing parsed xml
attributes).

* Address CR feedback

- introduce XmlOptions interface

- Make xml options required for private methods

- Remove duplicate by introducing locals

* Rename XmlOptions to more general SerializerOptions

* Make internal options `Required<SerializerOptions>`

* - Fix one missing place

- update CHANGELOG

Author: jeremymeng

sdk/core/core-http/CHANGELOG.md

@@ -3,7 +3,8 @@
 ## 1.2.0 (2020-10-19)
 
 - Explicitly set `manual` redirect handling for node fetch. And fixing redirectPipeline [PR 11863](https://github.com/Azure/azure-sdk-for-js/pull/11863)
-- Add support for multiple error response codes.[PR 11841](https://github.com/Azure/azure-sdk-for-js/)
+- Add support for multiple error response codes. [PR 11841](https://github.com/Azure/azure-sdk-for-js/)
+- Allow customizing serializer behavior via optional `SerialzierOptions` parameters. Particularly allow using a different `XML_CHARKEY` than the default `_` when parsing XML [PR 12065](https://github.com/Azure/azure-sdk-for-js/pull/12065)
 
 ## 1.1.9 (2020-09-30)
 

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

@@ -187,10 +187,10 @@ export interface DeserializationOptions {
 }
 
 // @public
-export function deserializationPolicy(deserializationContentTypes?: DeserializationContentTypes): RequestPolicyFactory;
+export function deserializationPolicy(deserializationContentTypes?: DeserializationContentTypes, parsingOptions?: SerializerOptions): RequestPolicyFactory;
 
 // @public (undocumented)
-export function deserializeResponseBody(jsonContentTypes: string[], xmlContentTypes: string[], response: HttpOperationResponse): Promise<HttpOperationResponse>;
+export function deserializeResponseBody(jsonContentTypes: string[], xmlContentTypes: string[], response: HttpOperationResponse, options?: SerializerOptions): Promise<HttpOperationResponse>;
 
 // @public (undocumented)
 export interface DictionaryMapper extends BaseMapper {
@@ -514,9 +514,7 @@ export interface ParameterValue {
 }
 
 // @public
-export function parseXML(str: string, opts?: {
-    includeRoot?: boolean;
-}): Promise<any>;
+export function parseXML(str: string, opts?: SerializerOptions): Promise<any>;
 
 // @public
 export interface PipelineOptions {
@@ -599,6 +597,7 @@ export interface RequestOptionsBase {
     };
     onDownloadProgress?: (progress: TransferProgressEvent) => void;
     onUploadProgress?: (progress: TransferProgressEvent) => void;
+    serializerOptions?: SerializerOptions;
     shouldDeserialize?: boolean | ((response: HttpOperationResponse) => boolean);
     spanOptions?: SpanOptions;
     timeout?: number;
@@ -728,18 +727,25 @@ export class Serializer {
     constructor(modelMappers?: {
         [key: string]: any;
     }, isXML?: boolean | undefined);
-    deserialize(mapper: Mapper, responseBody: any, objectName: string): any;
+    deserialize(mapper: Mapper, responseBody: any, objectName: string, options?: SerializerOptions): any;
     // (undocumented)
     readonly isXML?: boolean | undefined;
     // (undocumented)
     readonly modelMappers: {
         [key: string]: any;
     };
-    serialize(mapper: Mapper, object: any, objectName?: string): any;
+    serialize(mapper: Mapper, object: any, objectName?: string, options?: SerializerOptions): any;
     // (undocumented)
     validateConstraints(mapper: Mapper, value: any, objectName: string): void;
 }
 
+// @public
+export interface SerializerOptions {
+    includeRoot?: boolean;
+    rootName?: string;
+    xmlCharKey?: string;
+}
+
 // @public
 export interface ServiceCallback<TResult> {
     (err: Error | RestError | null, result?: TResult, request?: WebResourceLike, response?: HttpOperationResponse): void;
@@ -785,9 +791,7 @@ export interface SimpleMapperType {
 }
 
 // @public
-export function stringifyXML(obj: any, opts?: {
-    rootName?: string;
-}): string;
+export function stringifyXML(obj: any, opts?: SerializerOptions): string;
 
 // @public
 export function stripRequest(request: WebResourceLike): WebResourceLike;
@@ -954,6 +958,12 @@ export interface WebResourceLike {
     withCredentials: boolean;
 }
 
+// @public
+export const XML_ATTRKEY = "$";
+
+// @public
+export const XML_CHARKEY = "_";
+
 
 // (No @packageDocumentation comment for this package)
 

sdk/core/core-http/src/coreHttp.ts

@@ -125,3 +125,4 @@ export { TopicCredentials } from "./credentials/topicCredentials";
 export { Authenticator } from "./credentials/credentials";
 
 export { parseXML, stringifyXML } from "./util/xml";
+export { XML_ATTRKEY, XML_CHARKEY, SerializerOptions } from "./util/serializer.common";

sdk/core/core-http/src/policies/deserializationPolicy.ts

@@ -14,6 +14,7 @@ import {
   RequestPolicyFactory,
   RequestPolicyOptions
 } from "./requestPolicy";
+import { XML_CHARKEY, SerializerOptions } from "../util/serializer.common";
 
 /**
  * Options to configure API response deserialization.
@@ -49,11 +50,17 @@ export interface DeserializationContentTypes {
  * pass through the HTTP pipeline.
  */
 export function deserializationPolicy(
-  deserializationContentTypes?: DeserializationContentTypes
+  deserializationContentTypes?: DeserializationContentTypes,
+  parsingOptions?: SerializerOptions
 ): RequestPolicyFactory {
   return {
     create: (nextPolicy: RequestPolicy, options: RequestPolicyOptions) => {
-      return new DeserializationPolicy(nextPolicy, deserializationContentTypes, options);
+      return new DeserializationPolicy(
+        nextPolicy,
+        options,
+        deserializationContentTypes,
+        parsingOptions
+      );
     }
   };
 }
@@ -75,26 +82,29 @@ export const DefaultDeserializationOptions: DeserializationOptions = {
 export class DeserializationPolicy extends BaseRequestPolicy {
   public readonly jsonContentTypes: string[];
   public readonly xmlContentTypes: string[];
+  public readonly xmlCharKey: string;
 
   constructor(
     nextPolicy: RequestPolicy,
-    deserializationContentTypes: DeserializationContentTypes | undefined,
-    options: RequestPolicyOptions
+    requestPolicyOptions: RequestPolicyOptions,
+    deserializationContentTypes?: DeserializationContentTypes,
+    parsingOptions: SerializerOptions = {}
   ) {
-    super(nextPolicy, options);
+    super(nextPolicy, requestPolicyOptions);
 
     this.jsonContentTypes =
       (deserializationContentTypes && deserializationContentTypes.json) || defaultJsonContentTypes;
     this.xmlContentTypes =
       (deserializationContentTypes && deserializationContentTypes.xml) || defaultXmlContentTypes;
+    this.xmlCharKey = parsingOptions.xmlCharKey ?? XML_CHARKEY;
   }
 
   public async sendRequest(request: WebResourceLike): Promise<HttpOperationResponse> {
-    return this._nextPolicy
-      .sendRequest(request)
-      .then((response: HttpOperationResponse) =>
-        deserializeResponseBody(this.jsonContentTypes, this.xmlContentTypes, response)
-      );
+    return this._nextPolicy.sendRequest(request).then((response: HttpOperationResponse) =>
+      deserializeResponseBody(this.jsonContentTypes, this.xmlContentTypes, response, {
+        xmlCharKey: this.xmlCharKey
+      })
+    );
   }
 }
 
@@ -137,74 +147,84 @@ function shouldDeserializeResponse(parsedResponse: HttpOperationResponse): boole
 export function deserializeResponseBody(
   jsonContentTypes: string[],
   xmlContentTypes: string[],
-  response: HttpOperationResponse
+  response: HttpOperationResponse,
+  options: SerializerOptions = {}
 ): Promise<HttpOperationResponse> {
-  return parse(jsonContentTypes, xmlContentTypes, response).then((parsedResponse) => {
-    if (!shouldDeserializeResponse(parsedResponse)) {
-      return parsedResponse;
-    }
+  const updatedOptions: Required<SerializerOptions> = {
+    rootName: options.rootName ?? "",
+    includeRoot: options.includeRoot ?? false,
+    xmlCharKey: options.xmlCharKey ?? XML_CHARKEY
+  };
+  return parse(jsonContentTypes, xmlContentTypes, response, updatedOptions).then(
+    (parsedResponse) => {
+      if (!shouldDeserializeResponse(parsedResponse)) {
+        return parsedResponse;
+      }
 
-    const operationSpec = parsedResponse.request.operationSpec;
-    if (!operationSpec || !operationSpec.responses) {
-      return parsedResponse;
-    }
+      const operationSpec = parsedResponse.request.operationSpec;
+      if (!operationSpec || !operationSpec.responses) {
+        return parsedResponse;
+      }
 
-    const responseSpec = getOperationResponse(parsedResponse);
+      const responseSpec = getOperationResponse(parsedResponse);
 
-    const { error, shouldReturnResponse } = handleErrorResponse(
-      parsedResponse,
-      operationSpec,
-      responseSpec
-    );
-    if (error) {
-      throw error;
-    } else if (shouldReturnResponse) {
-      return parsedResponse;
-    }
+      const { error, shouldReturnResponse } = handleErrorResponse(
+        parsedResponse,
+        operationSpec,
+        responseSpec
+      );
+      if (error) {
+        throw error;
+      } else if (shouldReturnResponse) {
+        return parsedResponse;
+      }
 
-    // An operation response spec does exist for current status code, so
-    // use it to deserialize the response.
-    if (responseSpec) {
-      if (responseSpec.bodyMapper) {
-        let valueToDeserialize: any = parsedResponse.parsedBody;
-        if (operationSpec.isXML && responseSpec.bodyMapper.type.name === MapperType.Sequence) {
-          valueToDeserialize =
-            typeof valueToDeserialize === "object"
-              ? valueToDeserialize[responseSpec.bodyMapper.xmlElementName!]
-              : [];
+      // An operation response spec does exist for current status code, so
+      // use it to deserialize the response.
+      if (responseSpec) {
+        if (responseSpec.bodyMapper) {
+          let valueToDeserialize: any = parsedResponse.parsedBody;
+          if (operationSpec.isXML && responseSpec.bodyMapper.type.name === MapperType.Sequence) {
+            valueToDeserialize =
+              typeof valueToDeserialize === "object"
+                ? valueToDeserialize[responseSpec.bodyMapper.xmlElementName!]
+                : [];
+          }
+          try {
+            parsedResponse.parsedBody = operationSpec.serializer.deserialize(
+              responseSpec.bodyMapper,
+              valueToDeserialize,
+              "operationRes.parsedBody",
+              options
+            );
+          } catch (error) {
+            const restError = new RestError(
+              `Error ${error} occurred in deserializing the responseBody - ${parsedResponse.bodyAsText}`,
+              undefined,
+              parsedResponse.status,
+              parsedResponse.request,
+              parsedResponse
+            );
+            throw restError;
+          }
+        } else if (operationSpec.httpMethod === "HEAD") {
+          // head methods never have a body, but we return a boolean to indicate presence/absence of the resource
+          parsedResponse.parsedBody = response.status >= 200 && response.status < 300;
         }
-        try {
-          parsedResponse.parsedBody = operationSpec.serializer.deserialize(
-            responseSpec.bodyMapper,
-            valueToDeserialize,
-            "operationRes.parsedBody"
-          );
-        } catch (error) {
-          const restError = new RestError(
-            `Error ${error} occurred in deserializing the responseBody - ${parsedResponse.bodyAsText}`,
-            undefined,
-            parsedResponse.status,
-            parsedResponse.request,
-            parsedResponse
+
+        if (responseSpec.headersMapper) {
+          parsedResponse.parsedHeaders = operationSpec.serializer.deserialize(
+            responseSpec.headersMapper,
+            parsedResponse.headers.rawHeaders(),
+            "operationRes.parsedHeaders",
+            options
           );
-          throw restError;
         }
-      } else if (operationSpec.httpMethod === "HEAD") {
-        // head methods never have a body, but we return a boolean to indicate presence/absence of the resource
-        parsedResponse.parsedBody = response.status >= 200 && response.status < 300;
       }
 
-      if (responseSpec.headersMapper) {
-        parsedResponse.parsedHeaders = operationSpec.serializer.deserialize(
-          responseSpec.headersMapper,
-          parsedResponse.headers.rawHeaders(),
-          "operationRes.parsedHeaders"
-        );
-      }
+      return parsedResponse;
     }
-
-    return parsedResponse;
-  });
+  );
 }
 
 function isOperationSpecEmpty(operationSpec: OperationSpec): boolean {
@@ -305,7 +325,8 @@ function handleErrorResponse(
 function parse(
   jsonContentTypes: string[],
   xmlContentTypes: string[],
-  operationResponse: HttpOperationResponse
+  operationResponse: HttpOperationResponse,
+  opts: Required<SerializerOptions>
 ): Promise<HttpOperationResponse> {
   const errorHandler = (err: Error & { code: string }): Promise<never> => {
     const msg = `Error "${err}" occurred while parsing the response body - ${operationResponse.bodyAsText}.`;
@@ -335,7 +356,7 @@ function parse(
         resolve(operationResponse);
       }).catch(errorHandler);
     } else if (contentComponents.some((component) => xmlContentTypes.indexOf(component) !== -1)) {
-      return parseXML(text)
+      return parseXML(text, opts)
         .then((body) => {
           operationResponse.parsedBody = body;
           return operationResponse;