Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 23 additions & 4 deletions sdk/core/core-http/review/core-http.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -187,10 +187,14 @@ export interface DeserializationOptions {
}

// @public
export function deserializationPolicy(deserializationContentTypes?: DeserializationContentTypes): RequestPolicyFactory;
export function deserializationPolicy(deserializationContentTypes?: DeserializationContentTypes, parsingOptions?: {
xmlCharKey?: string;
}): RequestPolicyFactory;

// @public (undocumented)
export function deserializeResponseBody(jsonContentTypes: string[], xmlContentTypes: string[], response: HttpOperationResponse): Promise<HttpOperationResponse>;
export function deserializeResponseBody(jsonContentTypes: string[], xmlContentTypes: string[], response: HttpOperationResponse, options?: {
xmlCharKey?: string;
}): Promise<HttpOperationResponse>;

// @public (undocumented)
export interface DictionaryMapper extends BaseMapper {
Expand Down Expand Up @@ -516,6 +520,7 @@ export interface ParameterValue {
// @public
export function parseXML(str: string, opts?: {
includeRoot?: boolean;
xmlCharKey?: string;
}): Promise<any>;

// @public
Expand Down Expand Up @@ -599,6 +604,9 @@ export interface RequestOptionsBase {
};
onDownloadProgress?: (progress: TransferProgressEvent) => void;
onUploadProgress?: (progress: TransferProgressEvent) => void;
parsingOptions?: {
xmlCharKey?: string;
};
shouldDeserialize?: boolean | ((response: HttpOperationResponse) => boolean);
spanOptions?: SpanOptions;
timeout?: number;
Expand Down Expand Up @@ -728,14 +736,18 @@ 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?: {
xmlCharKey?: string;
}): 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?: {
xmlCharKey?: string;
}): any;
// (undocumented)
validateConstraints(mapper: Mapper, value: any, objectName: string): void;
}
Expand Down Expand Up @@ -787,6 +799,7 @@ export interface SimpleMapperType {
// @public
export function stringifyXML(obj: any, opts?: {
rootName?: string;
xmlCharKey?: string;
}): string;

// @public
Expand Down Expand Up @@ -954,6 +967,12 @@ export interface WebResourceLike {
withCredentials: boolean;
}

// @public
export const XML_ATTRKEY = "$";

// @public
export const XML_CHARKEY = "_";

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel it is useful to export the default value, since we now support overriding it.



// (No @packageDocumentation comment for this package)

Expand Down
1 change: 1 addition & 0 deletions sdk/core/core-http/src/coreHttp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 } from "./util/xml.common";
40 changes: 27 additions & 13 deletions sdk/core/core-http/src/policies/deserializationPolicy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
RequestPolicyFactory,
RequestPolicyOptions
} from "./requestPolicy";
import { XML_CHARKEY } from "../util/xml.common";

/**
* Options to configure API response deserialization.
Expand Down Expand Up @@ -49,11 +50,17 @@ export interface DeserializationContentTypes {
* pass through the HTTP pipeline.
*/
export function deserializationPolicy(
deserializationContentTypes?: DeserializationContentTypes
deserializationContentTypes?: DeserializationContentTypes,
parsingOptions?: { xmlCharKey?: string }
): RequestPolicyFactory {
return {
create: (nextPolicy: RequestPolicy, options: RequestPolicyOptions) => {
return new DeserializationPolicy(nextPolicy, deserializationContentTypes, options);
return new DeserializationPolicy(
nextPolicy,
deserializationContentTypes,
parsingOptions,
options
);
}
};
}
Expand All @@ -75,10 +82,12 @@ 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,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason these are all | undefined instead of making the parameters themselves optional with ??

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last parameter is non-optional. I could move both parameters to be after request policy options

parsingOptions: { xmlCharKey?: string } | undefined,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than putting a literal { xmlCharKey?: string } everywhere, can we write this down as a type? It'll make it much easier to add additional parsingOptions later if needed.

options: RequestPolicyOptions
) {
super(nextPolicy, options);
Expand All @@ -87,14 +96,15 @@ export class DeserializationPolicy extends BaseRequestPolicy {
(deserializationContentTypes && deserializationContentTypes.json) || defaultJsonContentTypes;
this.xmlContentTypes =
(deserializationContentTypes && deserializationContentTypes.xml) || defaultXmlContentTypes;
this.xmlCharKey = parsingOptions?.xmlCharKey || XML_CHARKEY;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might as well be consistently fancy, right? ;)

Suggested change
this.xmlCharKey = parsingOptions?.xmlCharKey || XML_CHARKEY;
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
})
);
}
}

Expand Down Expand Up @@ -137,9 +147,10 @@ function shouldDeserializeResponse(parsedResponse: HttpOperationResponse): boole
export function deserializeResponseBody(
jsonContentTypes: string[],
xmlContentTypes: string[],
response: HttpOperationResponse
response: HttpOperationResponse,
options?: { xmlCharKey?: string }
): Promise<HttpOperationResponse> {
return parse(jsonContentTypes, xmlContentTypes, response).then((parsedResponse) => {
return parse(jsonContentTypes, xmlContentTypes, response, options).then((parsedResponse) => {
if (!shouldDeserializeResponse(parsedResponse)) {
return parsedResponse;
}
Expand Down Expand Up @@ -177,7 +188,8 @@ export function deserializeResponseBody(
parsedResponse.parsedBody = operationSpec.serializer.deserialize(
responseSpec.bodyMapper,
valueToDeserialize,
"operationRes.parsedBody"
"operationRes.parsedBody",
options
);
} catch (error) {
const restError = new RestError(
Expand All @@ -198,7 +210,8 @@ export function deserializeResponseBody(
parsedResponse.parsedHeaders = operationSpec.serializer.deserialize(
responseSpec.headersMapper,
parsedResponse.headers.rawHeaders(),
"operationRes.parsedHeaders"
"operationRes.parsedHeaders",
options
);
}
}
Expand Down Expand Up @@ -300,7 +313,8 @@ function handleErrorResponse(
function parse(
jsonContentTypes: string[],
xmlContentTypes: string[],
operationResponse: HttpOperationResponse
operationResponse: HttpOperationResponse,
opts?: { includeRoot?: boolean; xmlCharKey?: string }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again I wonder if this should be an interface

): Promise<HttpOperationResponse> {
const errorHandler = (err: Error & { code: string }): Promise<never> => {
const msg = `Error "${err}" occurred while parsing the response body - ${operationResponse.bodyAsText}.`;
Expand Down Expand Up @@ -330,7 +344,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;
Expand Down
Loading