Skip to content

Commit

Permalink
[core-https][core-client] Core v2 cleanup work (Azure#13266)
Browse files Browse the repository at this point in the history
* Fix an issue where policies could make phases execute out of order.
* Remove keepAlivePolicy
* Let clients add ndJsonPolicy manually
* Disable redirects by removing the policy instead of an option
* Invert response decompression policy
* Remove request cloning, to optimize pipeline allocations and replace additionalInfo with WeakMaps.
  • Loading branch information
xirzec authored and ljian3377 committed Jan 22, 2021
1 parent f34117d commit ffe8df3
Show file tree
Hide file tree
Showing 30 changed files with 177 additions and 276 deletions.
2 changes: 1 addition & 1 deletion sdk/core/core-client/review/core-client.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ export interface OperationQueryParameter extends OperationParameter {
}

// @public
export type OperationRequest = PipelineRequest<OperationRequestInfo>;
export type OperationRequest = PipelineRequest;

// @public
export interface OperationRequestInfo {
Expand Down
14 changes: 9 additions & 5 deletions sdk/core/core-client/src/deserializationPolicy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
} from "./interfaces";
import { MapperTypeNames } from "./serializer";
import { isStreamOperation } from "./interfaceHelpers";
import { getOperationRequestInfo } from "./operationHelpers";

const defaultJsonContentTypes = ["application/json", "text/json"];
const defaultXmlContentTypes = ["application/xml", "application/atom+xml"];
Expand Down Expand Up @@ -104,20 +105,22 @@ function getOperationResponseMap(
): undefined | OperationResponseMap {
let result: OperationResponseMap | undefined;
const request: OperationRequest = parsedResponse.request;
const operationSpec = request.additionalInfo?.operationSpec;
const operationInfo = getOperationRequestInfo(request);
const operationSpec = operationInfo?.operationSpec;
if (operationSpec) {
if (!request.additionalInfo?.operationResponseGetter) {
if (!operationInfo?.operationResponseGetter) {
result = operationSpec.responses[parsedResponse.status];
} else {
result = request.additionalInfo?.operationResponseGetter(operationSpec, parsedResponse);
result = operationInfo?.operationResponseGetter(operationSpec, parsedResponse);
}
}
return result;
}

function shouldDeserializeResponse(parsedResponse: PipelineResponse): boolean {
const request: OperationRequest = parsedResponse.request;
const shouldDeserialize = request.additionalInfo?.shouldDeserialize;
const operationInfo = getOperationRequestInfo(request);
const shouldDeserialize = operationInfo?.shouldDeserialize;
let result: boolean;
if (shouldDeserialize === undefined) {
result = true;
Expand Down Expand Up @@ -147,7 +150,8 @@ async function deserializeResponseBody(
return parsedResponse;
}

const operationSpec = parsedResponse.request.additionalInfo?.operationSpec;
const operationInfo = getOperationRequestInfo(parsedResponse.request);
const operationSpec = operationInfo?.operationSpec;
if (!operationSpec || !operationSpec.responses) {
return parsedResponse;
}
Expand Down
5 changes: 2 additions & 3 deletions sdk/core/core-client/src/interfaces.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,9 @@ export type RequiredSerializerOptions = {
};

/**
* This interface extends a generic `PipelineRequest` to include
* additional metadata about the request.
* A type alias for future proofing.
*/
export type OperationRequest = PipelineRequest<OperationRequestInfo>;
export type OperationRequest = PipelineRequest;

/**
* Metadata that is used to properly parse a response.
Expand Down
16 changes: 15 additions & 1 deletion sdk/core/core-client/src/operationHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ import {
OperationParameter,
Mapper,
CompositeMapper,
ParameterPath
ParameterPath,
OperationRequestInfo,
OperationRequest
} from "./interfaces";

/**
Expand Down Expand Up @@ -103,3 +105,15 @@ function getPropertyFromParameterPath(
}
return result;
}

const operationRequestMap = new WeakMap<OperationRequest, OperationRequestInfo>();

export function getOperationRequestInfo(request: OperationRequest): OperationRequestInfo {
let info = operationRequestMap.get(request);

if (!info) {
info = {};
operationRequestMap.set(request, info);
}
return info;
}
10 changes: 7 additions & 3 deletions sdk/core/core-client/src/serializationPolicy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ import {
} from "./interfaces";
import { MapperTypeNames } from "./serializer";
import { getPathStringFromParameter } from "./interfaceHelpers";
import { getOperationArgumentValueFromParameter } from "./operationHelpers";
import {
getOperationArgumentValueFromParameter,
getOperationRequestInfo
} from "./operationHelpers";

/**
* The programmatic identifier of the serializationPolicy.
Expand Down Expand Up @@ -47,8 +50,9 @@ export function serializationPolicy(options: serializationPolicyOptions = {}): P
return {
name: serializationPolicyName,
async sendRequest(request: OperationRequest, next: SendRequest): Promise<PipelineResponse> {
const operationSpec = request.additionalInfo?.operationSpec;
const operationArguments = request.additionalInfo?.operationArguments;
const operationInfo = getOperationRequestInfo(request);
const operationSpec = operationInfo?.operationSpec;
const operationArguments = operationInfo?.operationArguments;
if (operationSpec && operationArguments) {
serializeHeaders(request, operationArguments, operationSpec);
serializeRequestBody(request, operationArguments, operationSpec, stringifyXML);
Expand Down
9 changes: 5 additions & 4 deletions sdk/core/core-client/src/serviceClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import { deserializationPolicy, DeserializationPolicyOptions } from "./deseriali
import { URL } from "./url";
import { serializationPolicy, serializationPolicyOptions } from "./serializationPolicy";
import { getCachedDefaultHttpsClient } from "./httpClientCache";
import { getOperationRequestInfo } from "./operationHelpers";

/**
* Options to be provided while creating the client.
Expand Down Expand Up @@ -148,9 +149,9 @@ export class ServiceClient {
url
});
request.method = operationSpec.httpMethod;
request.additionalInfo = {};
request.additionalInfo.operationSpec = operationSpec;
request.additionalInfo.operationArguments = operationArguments;
const operationInfo = getOperationRequestInfo(request);
operationInfo.operationSpec = operationSpec;
operationInfo.operationArguments = operationArguments;

const contentType = operationSpec.contentType || this._requestContentType;
if (contentType && operationSpec.requestBody) {
Expand Down Expand Up @@ -181,7 +182,7 @@ export class ServiceClient {
}

if (requestOptions.shouldDeserialize !== undefined) {
request.additionalInfo.shouldDeserialize = requestOptions.shouldDeserialize;
operationInfo.shouldDeserialize = requestOptions.shouldDeserialize;
}
}

Expand Down
6 changes: 3 additions & 3 deletions sdk/core/core-client/test/deserializationPolicy.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
RawHttpHeaders
} from "@azure/core-https";
import { parseXML } from "@azure/core-xml";
import { getOperationRequestInfo } from "../src/operationHelpers";

describe("deserializationPolicy", function() {
it(`should not modify a request that has no request body mapper`, async function() {
Expand Down Expand Up @@ -709,9 +710,8 @@ async function getDeserializedResponse(
serializerOptions: options.serializerOptions
});
const request: OperationRequest = createPipelineRequest({ url: "https://example.com" });
request.additionalInfo = {
operationSpec: options.operationSpec
};
const operationInfo = getOperationRequestInfo(request);
operationInfo.operationSpec = options.operationSpec;
request.body = options.requestBody;

const res: PipelineResponse = {
Expand Down
10 changes: 6 additions & 4 deletions sdk/core/core-client/test/serviceClient.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,10 @@ import {
createPipelineRequest
} from "@azure/core-https";

import { getOperationArgumentValueFromParameter } from "../src/operationHelpers";
import {
getOperationArgumentValueFromParameter,
getOperationRequestInfo
} from "../src/operationHelpers";
import { deserializationPolicy } from "../src/deserializationPolicy";
import { TokenCredential } from "@azure/core-auth";
import { getCachedDefaultHttpsClient } from "../src/httpClientCache";
Expand Down Expand Up @@ -785,9 +788,8 @@ describe("ServiceClient", function() {
};

let request: OperationRequest = createPipelineRequest({ url: "https://example.com" });
request.additionalInfo = {
operationSpec
};
const operationInfo = getOperationRequestInfo(request);
operationInfo.operationSpec = operationSpec;

const httpsClient: HttpsClient = {
sendRequest: (req) => {
Expand Down
2 changes: 1 addition & 1 deletion sdk/core/core-https/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"module": "dist-esm/src/index.js",
"browser": {
"./dist-esm/src/defaultHttpsClient.js": "./dist-esm/src/defaultHttpsClient.browser.js",
"./dist-esm/src/policies/disableResponseDecompressionPolicy.js": "./dist-esm/src/policies/disableResponseDecompressionPolicy.browser.js",
"./dist-esm/src/policies/decompressResponsePolicy.js": "./dist-esm/src/policies/decompressResponsePolicy.browser.js",
"./dist-esm/src/policies/formDataPolicy.js": "./dist-esm/src/policies/formDataPolicy.browser.js",
"./dist-esm/src/policies/proxyPolicy.js": "./dist-esm/src/policies/proxyPolicy.browser.js",
"./dist-esm/src/util/inspect.js": "./dist-esm/src/util/inspect.browser.js",
Expand Down
40 changes: 8 additions & 32 deletions sdk/core/core-https/review/core-https.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,15 @@ export function createPipelineFromOptions(options: InternalPipelineOptions): Pip
export function createPipelineRequest(options: PipelineRequestOptions): PipelineRequest;

// @public
export class DefaultHttpsClient implements HttpsClient {
sendRequest(request: PipelineRequest): Promise<PipelineResponse>;
}
export function decompressResponsePolicy(): PipelinePolicy;

// @public
export function disableResponseDecompressionPolicy(): PipelinePolicy;
export const decompressResponsePolicyName = "decompressResponsePolicy";

// @public
export const disableResponseDecompressionPolicyName = "disableResponseDecompressionPolicy";
export class DefaultHttpsClient implements HttpsClient {
sendRequest(request: PipelineRequest): Promise<PipelineResponse>;
}

// @public
export function exponentialRetryPolicy(options?: ExponentialRetryPolicyOptions): PipelinePolicy;
Expand Down Expand Up @@ -84,7 +84,6 @@ export function getDefaultProxySettings(proxyUrl?: string): ProxySettings | unde

// @public
export interface HttpHeaders extends Iterable<[string, string]> {
clone(): HttpHeaders;
delete(name: string): void;
get(name: string): string | undefined;
has(name: string): boolean;
Expand All @@ -102,20 +101,7 @@ export interface HttpsClient {

// @public
export interface InternalPipelineOptions extends PipelineOptions {
decompressResponse?: boolean;
loggingOptions?: LogPolicyOptions;
sendStreamingJson?: boolean;
}

// @public
export function keepAlivePolicy(options?: KeepAlivePolicyOptions): PipelinePolicy;

// @public
export const keepAlivePolicyName = "keepAlivePolicy";

// @public
export interface KeepAlivePolicyOptions {
enable?: boolean;
}

// @public
Expand Down Expand Up @@ -152,9 +138,8 @@ export interface Pipeline {
// @public
export interface PipelineOptions {
httpsClient?: HttpsClient;
keepAliveOptions?: KeepAlivePolicyOptions;
proxyOptions?: ProxySettings;
redirectOptions?: PipelineRedirectOptions;
redirectOptions?: RedirectPolicyOptions;
retryOptions?: ExponentialRetryPolicyOptions;
userAgentOptions?: UserAgentPolicyOptions;
}
Expand All @@ -169,16 +154,9 @@ export interface PipelinePolicy {
}

// @public
export interface PipelineRedirectOptions extends RedirectPolicyOptions {
disable?: boolean;
}

// @public
export interface PipelineRequest<AdditionalInfo = any> {
export interface PipelineRequest {
abortSignal?: AbortSignalLike;
additionalInfo?: AdditionalInfo;
body?: RequestBodyType;
clone(): PipelineRequest;
formData?: FormDataMap;
headers: HttpHeaders;
keepAlive?: boolean;
Expand All @@ -187,7 +165,6 @@ export interface PipelineRequest<AdditionalInfo = any> {
onUploadProgress?: (progress: TransferProgressEvent) => void;
proxySettings?: ProxySettings;
requestId: string;
skipDecompressResponse?: boolean;
spanOptions?: SpanOptions;
streamResponseBody?: boolean;
timeout: number;
Expand All @@ -196,9 +173,8 @@ export interface PipelineRequest<AdditionalInfo = any> {
}

// @public
export interface PipelineRequestOptions<AdditionalInfo = any> {
export interface PipelineRequestOptions {
abortSignal?: AbortSignalLike;
additionalInfo?: AdditionalInfo;
body?: RequestBodyType;
formData?: FormDataMap;
headers?: HttpHeaders;
Expand Down
7 changes: 0 additions & 7 deletions sdk/core/core-https/src/httpHeaders.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,6 @@ class HttpHeadersImpl implements HttpHeaders {
return JSON.stringify(this.toJSON());
}

/**
* Create a deep clone/copy of this HttpHeaders collection.
*/
public clone(): HttpHeaders {
return new HttpHeadersImpl(this.toJSON());
}

/**
* Iterate over tuples of header [name, value] pairs.
*/
Expand Down
12 changes: 3 additions & 9 deletions sdk/core/core-https/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,16 @@ export {
createEmptyPipeline,
InternalPipelineOptions,
PipelineOptions,
PipelineRedirectOptions,
createPipelineFromOptions
} from "./pipeline";
export { DefaultHttpsClient } from "./defaultHttpsClient";
export { createHttpHeaders } from "./httpHeaders";
export { createPipelineRequest, PipelineRequestOptions } from "./pipelineRequest";
export { RestError, RestErrorOptions } from "./restError";
export {
disableResponseDecompressionPolicy,
disableResponseDecompressionPolicyName
} from "./policies/disableResponseDecompressionPolicy";
decompressResponsePolicy,
decompressResponsePolicyName
} from "./policies/decompressResponsePolicy";
export {
exponentialRetryPolicy,
ExponentialRetryPolicyOptions,
Expand All @@ -43,11 +42,6 @@ export {
setClientRequestIdPolicy,
setClientRequestIdPolicyName
} from "./policies/setClientRequestIdPolicy";
export {
keepAlivePolicy,
keepAlivePolicyName,
KeepAlivePolicyOptions
} from "./policies/keepAlivePolicy";
export { logPolicy, logPolicyName, LogPolicyOptions } from "./policies/logPolicy";
export { proxyPolicy, proxyPolicyName, getDefaultProxySettings } from "./policies/proxyPolicy";
export {
Expand Down
Loading

0 comments on commit ffe8df3

Please sign in to comment.