From 52eeccf6029a90faff5b3d44f24dc599f9a4de38 Mon Sep 17 00:00:00 2001 From: Jeff Fisher Date: Fri, 15 Jan 2021 15:36:57 -0800 Subject: [PATCH 1/9] Fix an issue where policies could make phases execute out of order. --- sdk/core/core-https/src/pipeline.ts | 33 ++++++++++++++++++----- sdk/core/core-https/test/pipeline.spec.ts | 30 ++++++++++++++++++--- 2 files changed, 54 insertions(+), 9 deletions(-) diff --git a/sdk/core/core-https/src/pipeline.ts b/sdk/core/core-https/src/pipeline.ts index 0366375940e2..d463cb34a208 100644 --- a/sdk/core/core-https/src/pipeline.ts +++ b/sdk/core/core-https/src/pipeline.ts @@ -53,7 +53,6 @@ export interface AddPolicyOptions { afterPolicies?: string[]; /** * The phase that this policy must come after. - * By default, policies without a phase occur first. */ afterPhase?: PipelinePhase; /** @@ -149,7 +148,7 @@ class HttpsPipeline implements Pipeline { throw new Error(`Invalid phase name: ${options.phase}`); } if (options.afterPhase && !ValidPhaseNames.has(options.afterPhase)) { - throw new Error(`Invalid phase name: ${options.afterPhase}`); + throw new Error(`Invalid afterPhase name: ${options.afterPhase}`); } this._policies.push({ policy, @@ -256,6 +255,9 @@ class HttpsPipeline implements Pipeline { const deserializePhase = new Set(); const retryPhase = new Set(); + // a list of phases in order + const orderedPhases = [serializePhase, noPhase, deserializePhase, retryPhase]; + // Small helper function to map phase name to each Set bucket. function getPhase(phase: PipelinePhase | undefined): Set { if (phase === "Retry") { @@ -346,14 +348,33 @@ class HttpsPipeline implements Pipeline { } } + function walkPhases() { + let noPhaseRan = false; + + for (const phase of orderedPhases) { + walkPhase(phase); + if (phase === noPhase) { + noPhaseRan = true; + } + // if the phase isn't complete + if (phase.size > 0 && phase !== noPhase) { + if (noPhaseRan === false) { + // Try running noPhase to see if that unblocks this phase next tick. + // This can happen if a phase that happens before noPhase + // is waiting on a noPhase policy to complete. + walkPhase(noPhase); + } + // Don't proceed to the next phase until this phase finishes. + return; + } + } + } + // Iterate until we've put every node in the result list. while (policyMap.size > 0) { const initialResultLength = result.length; // Keep walking each phase in order until we can order every node. - walkPhase(serializePhase); - walkPhase(noPhase); - walkPhase(deserializePhase); - walkPhase(retryPhase); + walkPhases(); // The result list *should* get at least one larger each time. // Otherwise, we're going to loop forever. if (result.length <= initialResultLength) { diff --git a/sdk/core/core-https/test/pipeline.spec.ts b/sdk/core/core-https/test/pipeline.spec.ts index 5603d4b196bb..ac712dd5ec55 100644 --- a/sdk/core/core-https/test/pipeline.spec.ts +++ b/sdk/core/core-https/test/pipeline.spec.ts @@ -178,6 +178,32 @@ describe("HttpsPipeline", function() { assert.strictEqual(policies[3], testPolicy); }); + it("prevents phases from getting out of order", function() { + const pipeline = createEmptyPipeline(); + const testPolicy: PipelinePolicy = { + sendRequest: (request, next) => next(request), + name: "test" + }; + const testPolicy2: PipelinePolicy = { + sendRequest: (request, next) => next(request), + name: "test2" + }; + const testPolicy3: PipelinePolicy = { + sendRequest: (request, next) => next(request), + name: "test3" + }; + pipeline.addPolicy(testPolicy, { phase: "Serialize" }); + pipeline.addPolicy(testPolicy2, { + beforePolicies: [testPolicy.name], + afterPhase: "Deserialize" + }); + pipeline.addPolicy(testPolicy3, { phase: "Deserialize" }); + + assert.throws(() => { + pipeline.getOrderedPolicies(); + }, /cycle/); + }); + it("addPolicy throws on both phase and afterPhase specified", function() { const pipeline = createEmptyPipeline(); const testPolicy: PipelinePolicy = { @@ -203,11 +229,9 @@ describe("HttpsPipeline", function() { assert.throws(() => { pipeline.addPolicy(testPolicy, { afterPhase: "Cerealize" as any }); - }, /Invalid phase name/); + }, /Invalid afterPhase name/); }); - // bad phase name should throw - it("removePolicy removes named policy", function() { const pipeline = createEmptyPipeline(); const testPolicy: PipelinePolicy = { From f96699b5752b8a6f4a4058f0bd10a067cd39e438 Mon Sep 17 00:00:00 2001 From: Jeff Fisher Date: Fri, 15 Jan 2021 15:43:23 -0800 Subject: [PATCH 2/9] Remove keepAlivePolicy --- sdk/core/core-https/src/index.ts | 5 --- sdk/core/core-https/src/pipeline.ts | 8 ---- .../src/policies/keepAlivePolicy.ts | 37 ------------------- 3 files changed, 50 deletions(-) delete mode 100644 sdk/core/core-https/src/policies/keepAlivePolicy.ts diff --git a/sdk/core/core-https/src/index.ts b/sdk/core/core-https/src/index.ts index be122a8aabeb..05914461e1d5 100644 --- a/sdk/core/core-https/src/index.ts +++ b/sdk/core/core-https/src/index.ts @@ -43,11 +43,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 { diff --git a/sdk/core/core-https/src/pipeline.ts b/sdk/core/core-https/src/pipeline.ts index d463cb34a208..2557566f2142 100644 --- a/sdk/core/core-https/src/pipeline.ts +++ b/sdk/core/core-https/src/pipeline.ts @@ -11,7 +11,6 @@ import { import { LogPolicyOptions, logPolicy } from "./policies/logPolicy"; import { UserAgentPolicyOptions, userAgentPolicy } from "./policies/userAgentPolicy"; import { RedirectPolicyOptions, redirectPolicy } from "./policies/redirectPolicy"; -import { KeepAlivePolicyOptions, keepAlivePolicy } from "./policies/keepAlivePolicy"; import { ExponentialRetryPolicyOptions, exponentialRetryPolicy @@ -425,12 +424,6 @@ export interface PipelineOptions { */ proxyOptions?: ProxySettings; - /** - * Options for how HTTP connections should be maintained for future - * requests. - */ - keepAliveOptions?: KeepAlivePolicyOptions; - /** * Options for how redirect responses are handled. */ @@ -484,7 +477,6 @@ export function createPipelineFromOptions(options: InternalPipelineOptions): Pip pipeline.addPolicy(formDataPolicy()); pipeline.addPolicy(tracingPolicy(options.userAgentOptions)); - pipeline.addPolicy(keepAlivePolicy(options.keepAliveOptions)); pipeline.addPolicy(userAgentPolicy(options.userAgentOptions)); pipeline.addPolicy(setClientRequestIdPolicy()); pipeline.addPolicy(throttlingRetryPolicy(), { phase: "Retry" }); diff --git a/sdk/core/core-https/src/policies/keepAlivePolicy.ts b/sdk/core/core-https/src/policies/keepAlivePolicy.ts deleted file mode 100644 index 761bab90c2c4..000000000000 --- a/sdk/core/core-https/src/policies/keepAlivePolicy.ts +++ /dev/null @@ -1,37 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. - -import { PipelineResponse, PipelineRequest, SendRequest } from "../interfaces"; -import { PipelinePolicy } from "../pipeline"; - -/** - * The programmatic identifier of the keepAlivePolicy. - */ -export const keepAlivePolicyName = "keepAlivePolicy"; - -/** - * Options for how HTTP connections should be maintained for future - * requests. - */ -export interface KeepAlivePolicyOptions { - /** - * When true, connections will be kept alive for multiple requests. - * Defaults to true. - */ - enable?: boolean; -} - -/** - * KeepAlivePolicy is a policy used to control keep alive settings for every request. - */ -export function keepAlivePolicy( - options: KeepAlivePolicyOptions = { enable: true } -): PipelinePolicy { - return { - name: keepAlivePolicyName, - async sendRequest(request: PipelineRequest, next: SendRequest): Promise { - request.keepAlive = options.enable; - return next(request); - } - }; -} From bcbf50ffe8355f1b198c64d724705fb5e4f4e280 Mon Sep 17 00:00:00 2001 From: Jeff Fisher Date: Fri, 15 Jan 2021 15:44:37 -0800 Subject: [PATCH 3/9] Let clients add ndJsonPolicy manually --- sdk/core/core-https/src/pipeline.ts | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/sdk/core/core-https/src/pipeline.ts b/sdk/core/core-https/src/pipeline.ts index 2557566f2142..769d4b6e739d 100644 --- a/sdk/core/core-https/src/pipeline.ts +++ b/sdk/core/core-https/src/pipeline.ts @@ -23,7 +23,6 @@ import { disableResponseDecompressionPolicy } from "./policies/disableResponseDe import { proxyPolicy } from "./policies/proxyPolicy"; import { isNode } from "./util/helpers"; import { formDataPolicy } from "./policies/formDataPolicy"; -import { ndJsonPolicy } from "./policies/ndJsonPolicy"; /** * Policies are executed in phases. @@ -449,11 +448,6 @@ export interface InternalPipelineOptions extends PipelineOptions { * Configure whether to decompress response according to Accept-Encoding header (node-fetch only) */ decompressResponse?: boolean; - - /** - * Send JSON Array payloads as NDJSON. - */ - sendStreamingJson?: boolean; } /** @@ -463,10 +457,6 @@ export interface InternalPipelineOptions extends PipelineOptions { export function createPipelineFromOptions(options: InternalPipelineOptions): Pipeline { const pipeline = HttpsPipeline.create(); - if (options.sendStreamingJson) { - pipeline.addPolicy(ndJsonPolicy()); - } - if (isNode) { pipeline.addPolicy(proxyPolicy(options.proxyOptions)); From 8db50ca872111488096c4329e3df11ea9ea92f92 Mon Sep 17 00:00:00 2001 From: Jeff Fisher Date: Fri, 15 Jan 2021 15:46:22 -0800 Subject: [PATCH 4/9] Disable redirects by removing the policy --- sdk/core/core-https/src/pipeline.ts | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/sdk/core/core-https/src/pipeline.ts b/sdk/core/core-https/src/pipeline.ts index 769d4b6e739d..7070854fe682 100644 --- a/sdk/core/core-https/src/pipeline.ts +++ b/sdk/core/core-https/src/pipeline.ts @@ -392,16 +392,6 @@ export function createEmptyPipeline(): Pipeline { return HttpsPipeline.create(); } -/** - * Options that allow configuring redirect behavior. - */ -export interface PipelineRedirectOptions extends RedirectPolicyOptions { - /** - * If true, disables automatic following of redirects. - */ - disable?: boolean; -} - /** * Defines options that are used to configure the HTTP pipeline for * an SDK client. @@ -426,7 +416,7 @@ export interface PipelineOptions { /** * Options for how redirect responses are handled. */ - redirectOptions?: PipelineRedirectOptions; + redirectOptions?: RedirectPolicyOptions; /** * Options for adding user agent details to outgoing requests. @@ -472,11 +462,7 @@ export function createPipelineFromOptions(options: InternalPipelineOptions): Pip pipeline.addPolicy(throttlingRetryPolicy(), { phase: "Retry" }); pipeline.addPolicy(systemErrorRetryPolicy(options.retryOptions), { phase: "Retry" }); pipeline.addPolicy(exponentialRetryPolicy(options.retryOptions), { phase: "Retry" }); - - if (!options.redirectOptions?.disable) { - pipeline.addPolicy(redirectPolicy(options.redirectOptions), { afterPhase: "Retry" }); - } - + pipeline.addPolicy(redirectPolicy(options.redirectOptions), { afterPhase: "Retry" }); pipeline.addPolicy(logPolicy(options.loggingOptions), { afterPhase: "Retry" }); return pipeline; From ceb200a05e9f6e18a20898ab5a4c57190a581929 Mon Sep 17 00:00:00 2001 From: Jeff Fisher Date: Fri, 15 Jan 2021 16:04:01 -0800 Subject: [PATCH 5/9] invert response decompression policy --- sdk/core/core-https/package.json | 2 +- sdk/core/core-https/src/index.ts | 7 +++---- sdk/core/core-https/src/interfaces.ts | 5 ----- sdk/core/core-https/src/nodeHttpsClient.ts | 9 ++++----- sdk/core/core-https/src/pipeline.ts | 12 ++--------- .../decompressResponsePolicy.browser.ts | 18 +++++++++++++++++ ...nPolicy.ts => decompressResponsePolicy.ts} | 12 +++++------ ...ableResponseDecompressionPolicy.browser.ts | 20 ------------------- 8 files changed, 34 insertions(+), 51 deletions(-) create mode 100644 sdk/core/core-https/src/policies/decompressResponsePolicy.browser.ts rename sdk/core/core-https/src/policies/{disableResponseDecompressionPolicy.ts => decompressResponsePolicy.ts} (51%) delete mode 100644 sdk/core/core-https/src/policies/disableResponseDecompressionPolicy.browser.ts diff --git a/sdk/core/core-https/package.json b/sdk/core/core-https/package.json index 1f2d75409fae..0cb39818d73d 100644 --- a/sdk/core/core-https/package.json +++ b/sdk/core/core-https/package.json @@ -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", diff --git a/sdk/core/core-https/src/index.ts b/sdk/core/core-https/src/index.ts index 05914461e1d5..32fc4e5f881f 100644 --- a/sdk/core/core-https/src/index.ts +++ b/sdk/core/core-https/src/index.ts @@ -23,7 +23,6 @@ export { createEmptyPipeline, InternalPipelineOptions, PipelineOptions, - PipelineRedirectOptions, createPipelineFromOptions } from "./pipeline"; export { DefaultHttpsClient } from "./defaultHttpsClient"; @@ -31,9 +30,9 @@ 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, diff --git a/sdk/core/core-https/src/interfaces.ts b/sdk/core/core-https/src/interfaces.ts index 8384f485a180..eb4e0125d852 100644 --- a/sdk/core/core-https/src/interfaces.ts +++ b/sdk/core/core-https/src/interfaces.ts @@ -153,11 +153,6 @@ export interface PipelineRequest { */ keepAlive?: boolean; - /** - * Disable automatic decompression based on Accept-Encoding header (Node only) - */ - skipDecompressResponse?: boolean; - /** * Used to abort the request later. */ diff --git a/sdk/core/core-https/src/nodeHttpsClient.ts b/sdk/core/core-https/src/nodeHttpsClient.ts index 36a5c60e46ee..52faa3ac74f2 100644 --- a/sdk/core/core-https/src/nodeHttpsClient.ts +++ b/sdk/core/core-https/src/nodeHttpsClient.ts @@ -80,10 +80,9 @@ export class NodeHttpsClient implements HttpsClient { }, request.timeout); } - if (!request.skipDecompressResponse) { - request.headers.set("Accept-Encoding", "gzip,deflate"); - } - + const acceptEncoding = request.headers.get("Accept-Encoding"); + const shouldDecompress = + acceptEncoding?.includes("gzip") || acceptEncoding?.includes("deflate"); let body = request.body; if (body && !request.headers.has("Content-Length")) { @@ -118,7 +117,7 @@ export class NodeHttpsClient implements HttpsClient { request }; - let responseStream = getResponseStream(res, headers, request.skipDecompressResponse); + let responseStream = getResponseStream(res, headers, shouldDecompress); const onDownloadProgress = request.onDownloadProgress; if (onDownloadProgress) { diff --git a/sdk/core/core-https/src/pipeline.ts b/sdk/core/core-https/src/pipeline.ts index 7070854fe682..970f3bff0e1c 100644 --- a/sdk/core/core-https/src/pipeline.ts +++ b/sdk/core/core-https/src/pipeline.ts @@ -19,7 +19,7 @@ import { tracingPolicy } from "./policies/tracingPolicy"; import { setClientRequestIdPolicy } from "./policies/setClientRequestIdPolicy"; import { throttlingRetryPolicy } from "./policies/throttlingRetryPolicy"; import { systemErrorRetryPolicy } from "./policies/systemErrorRetryPolicy"; -import { disableResponseDecompressionPolicy } from "./policies/disableResponseDecompressionPolicy"; +import { decompressResponsePolicy } from "./policies/decompressResponsePolicy"; import { proxyPolicy } from "./policies/proxyPolicy"; import { isNode } from "./util/helpers"; import { formDataPolicy } from "./policies/formDataPolicy"; @@ -433,11 +433,6 @@ export interface InternalPipelineOptions extends PipelineOptions { * Options to configure request/response logging. */ loggingOptions?: LogPolicyOptions; - - /** - * Configure whether to decompress response according to Accept-Encoding header (node-fetch only) - */ - decompressResponse?: boolean; } /** @@ -449,10 +444,7 @@ export function createPipelineFromOptions(options: InternalPipelineOptions): Pip if (isNode) { pipeline.addPolicy(proxyPolicy(options.proxyOptions)); - - if (options.decompressResponse === false) { - pipeline.addPolicy(disableResponseDecompressionPolicy()); - } + pipeline.addPolicy(decompressResponsePolicy()); } pipeline.addPolicy(formDataPolicy()); diff --git a/sdk/core/core-https/src/policies/decompressResponsePolicy.browser.ts b/sdk/core/core-https/src/policies/decompressResponsePolicy.browser.ts new file mode 100644 index 000000000000..1125822475dc --- /dev/null +++ b/sdk/core/core-https/src/policies/decompressResponsePolicy.browser.ts @@ -0,0 +1,18 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +/* + * NOTE: When moving this file, please update "browser" section in package.json + */ + +const NotSupported = new Error("decompressResponsePolicy is not supported in browser environment"); + +export const decompressResponsePolicyName = "decompressResponsePolicy"; + +/** + * decompressResponsePolicy is not supported in the browser and attempting + * to use it will raise an error. + */ +export function decompressResponsePolicy(): never { + throw NotSupported; +} diff --git a/sdk/core/core-https/src/policies/disableResponseDecompressionPolicy.ts b/sdk/core/core-https/src/policies/decompressResponsePolicy.ts similarity index 51% rename from sdk/core/core-https/src/policies/disableResponseDecompressionPolicy.ts rename to sdk/core/core-https/src/policies/decompressResponsePolicy.ts index 7943cdd4e2b2..df1362ae56f4 100644 --- a/sdk/core/core-https/src/policies/disableResponseDecompressionPolicy.ts +++ b/sdk/core/core-https/src/policies/decompressResponsePolicy.ts @@ -5,19 +5,19 @@ import { PipelineResponse, PipelineRequest, SendRequest } from "../interfaces"; import { PipelinePolicy } from "../pipeline"; /** - * The programmatic identifier of the disableResponseDecompressionPolicy. + * The programmatic identifier of the decompressResponsePolicy. */ -export const disableResponseDecompressionPolicyName = "disableResponseDecompressionPolicy"; +export const decompressResponsePolicyName = "decompressResponsePolicy"; /** - * A policy to disable response decompression according to Accept-Encoding header + * A policy to enable response decompression according to Accept-Encoding header * https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept-Encoding */ -export function disableResponseDecompressionPolicy(): PipelinePolicy { +export function decompressResponsePolicy(): PipelinePolicy { return { - name: disableResponseDecompressionPolicyName, + name: decompressResponsePolicyName, async sendRequest(request: PipelineRequest, next: SendRequest): Promise { - request.skipDecompressResponse = true; + request.headers.set("Accept-Encoding", "gzip,deflate"); return next(request); } }; diff --git a/sdk/core/core-https/src/policies/disableResponseDecompressionPolicy.browser.ts b/sdk/core/core-https/src/policies/disableResponseDecompressionPolicy.browser.ts deleted file mode 100644 index ad48e10807fb..000000000000 --- a/sdk/core/core-https/src/policies/disableResponseDecompressionPolicy.browser.ts +++ /dev/null @@ -1,20 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. - -/* - * NOTE: When moving this file, please update "browser" section in package.json - */ - -const NotSupported = new Error( - "disableResponseDecompressionPolicy is not supported in browser environment" -); - -export const disableResponseDecompressionPolicyName = "disableResponseDecompressionPolicy"; - -/** - * disableResponseDecompressionPolicy is not supported in browser and attempting - * to use it will results in error being thrown. - */ -export function disableResponseDecompressionPolicy(): never { - throw NotSupported; -} From 8dc536e4572504e3c9bf2dd18e3b7e0cbd8ca399 Mon Sep 17 00:00:00 2001 From: Jeff Fisher Date: Fri, 15 Jan 2021 16:04:24 -0800 Subject: [PATCH 6/9] fixup bad comments --- sdk/core/core-https/src/policies/ndJsonPolicy.ts | 2 +- sdk/core/core-https/src/policies/proxyPolicy.browser.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/sdk/core/core-https/src/policies/ndJsonPolicy.ts b/sdk/core/core-https/src/policies/ndJsonPolicy.ts index d476205b1ebb..7d6f2873e137 100644 --- a/sdk/core/core-https/src/policies/ndJsonPolicy.ts +++ b/sdk/core/core-https/src/policies/ndJsonPolicy.ts @@ -5,7 +5,7 @@ import { PipelineResponse, PipelineRequest, SendRequest } from "../interfaces"; import { PipelinePolicy } from "../pipeline"; /** - * The programmatic identifier of the keepAlivePolicy. + * The programmatic identifier of the ndJsonPolicy. */ export const ndJsonPolicyName = "ndJsonPolicy"; diff --git a/sdk/core/core-https/src/policies/proxyPolicy.browser.ts b/sdk/core/core-https/src/policies/proxyPolicy.browser.ts index d4aee1036c24..e2d07a2bcca3 100644 --- a/sdk/core/core-https/src/policies/proxyPolicy.browser.ts +++ b/sdk/core/core-https/src/policies/proxyPolicy.browser.ts @@ -14,8 +14,8 @@ export function getDefaultProxySettings(): never { } /** - * proxyPolicy is not supported in browser and attempting - * to use it will results in error being thrown. + * proxyPolicy is not supported in the browser and attempting + * to use it will raise an error. */ export function proxyPolicy(): never { throw NotSupported; From 8a261dccb9b65269d3008a98229439db9df59293 Mon Sep 17 00:00:00 2001 From: Jeff Fisher Date: Fri, 15 Jan 2021 16:09:33 -0800 Subject: [PATCH 7/9] Fix tests and run api-extractor --- sdk/core/core-https/review/core-https.api.md | 32 ++++--------------- .../test/browser/decompressResponsePolicy.ts | 13 ++++++++ ...disableResponseDecompressionPolicy.spec.ts | 13 -------- ...cy.spec.ts => decompressResponsePolicy.ts} | 10 +++--- 4 files changed, 24 insertions(+), 44 deletions(-) create mode 100644 sdk/core/core-https/test/browser/decompressResponsePolicy.ts delete mode 100644 sdk/core/core-https/test/browser/disableResponseDecompressionPolicy.spec.ts rename sdk/core/core-https/test/node/{disableResponseDecompressionPolicy.spec.ts => decompressResponsePolicy.ts} (55%) diff --git a/sdk/core/core-https/review/core-https.api.md b/sdk/core/core-https/review/core-https.api.md index ffeef2533c5d..7c0f9a9f43c3 100644 --- a/sdk/core/core-https/review/core-https.api.md +++ b/sdk/core/core-https/review/core-https.api.md @@ -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; -} +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; +} // @public export function exponentialRetryPolicy(options?: ExponentialRetryPolicyOptions): PipelinePolicy; @@ -102,20 +102,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 @@ -152,9 +139,8 @@ export interface Pipeline { // @public export interface PipelineOptions { httpsClient?: HttpsClient; - keepAliveOptions?: KeepAlivePolicyOptions; proxyOptions?: ProxySettings; - redirectOptions?: PipelineRedirectOptions; + redirectOptions?: RedirectPolicyOptions; retryOptions?: ExponentialRetryPolicyOptions; userAgentOptions?: UserAgentPolicyOptions; } @@ -168,11 +154,6 @@ export interface PipelinePolicy { sendRequest(request: PipelineRequest, next: SendRequest): Promise; } -// @public -export interface PipelineRedirectOptions extends RedirectPolicyOptions { - disable?: boolean; -} - // @public export interface PipelineRequest { abortSignal?: AbortSignalLike; @@ -187,7 +168,6 @@ export interface PipelineRequest { onUploadProgress?: (progress: TransferProgressEvent) => void; proxySettings?: ProxySettings; requestId: string; - skipDecompressResponse?: boolean; spanOptions?: SpanOptions; streamResponseBody?: boolean; timeout: number; diff --git a/sdk/core/core-https/test/browser/decompressResponsePolicy.ts b/sdk/core/core-https/test/browser/decompressResponsePolicy.ts new file mode 100644 index 000000000000..01f4566207ff --- /dev/null +++ b/sdk/core/core-https/test/browser/decompressResponsePolicy.ts @@ -0,0 +1,13 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +import { assert } from "chai"; +import { decompressResponsePolicy } from "../../src"; + +describe("decompressResponsePolicy (browser)", function() { + it("Throws on creation", function() { + assert.throws(() => { + decompressResponsePolicy(); + }, /decompressResponsePolicy is not supported in browser environment/); + }); +}); diff --git a/sdk/core/core-https/test/browser/disableResponseDecompressionPolicy.spec.ts b/sdk/core/core-https/test/browser/disableResponseDecompressionPolicy.spec.ts deleted file mode 100644 index 78ea1eef3ad9..000000000000 --- a/sdk/core/core-https/test/browser/disableResponseDecompressionPolicy.spec.ts +++ /dev/null @@ -1,13 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. - -import { assert } from "chai"; -import { disableResponseDecompressionPolicy } from "../../src"; - -describe("disableResponseDecompressionPolicy (browser)", function() { - it("Throws on creation", function() { - assert.throws(() => { - disableResponseDecompressionPolicy(); - }, /disableResponseDecompressionPolicy is not supported in browser environment/); - }); -}); diff --git a/sdk/core/core-https/test/node/disableResponseDecompressionPolicy.spec.ts b/sdk/core/core-https/test/node/decompressResponsePolicy.ts similarity index 55% rename from sdk/core/core-https/test/node/disableResponseDecompressionPolicy.spec.ts rename to sdk/core/core-https/test/node/decompressResponsePolicy.ts index 9930e4ab25b5..563f945f996a 100644 --- a/sdk/core/core-https/test/node/disableResponseDecompressionPolicy.spec.ts +++ b/sdk/core/core-https/test/node/decompressResponsePolicy.ts @@ -3,23 +3,23 @@ import { assert } from "chai"; import * as sinon from "sinon"; -import { disableResponseDecompressionPolicy, createPipelineRequest, SendRequest } from "../../src"; +import { decompressResponsePolicy, createPipelineRequest, SendRequest } from "../../src"; -describe("disableResponseDecompressionPolicy (node)", function() { +describe("decompressResponsePolicy (node)", function() { it("Sets the expected flag on the request", function() { - const policy = disableResponseDecompressionPolicy(); + const policy = decompressResponsePolicy(); const request = createPipelineRequest({ url: "https://bing.com" }); - assert.isFalse(request.skipDecompressResponse, "skipDecompressResponse is not set."); + assert.isFalse(request.headers.has("Accept-Encoding"), "acceptEncoding is set."); const next = sinon.stub, ReturnType>(); policy.sendRequest(request, next); assert.isTrue(next.calledOnceWith(request), "next called with request"); - assert.isTrue(request.skipDecompressResponse, "skipDecompressResponse is set."); + assert.strictEqual(request.headers.get("Accept-Encoding"), "gzip,deflate"); }); }); From 4f0f3f8d857538097f1bbd2a7fe79d074691af34 Mon Sep 17 00:00:00 2001 From: Jeff Fisher Date: Fri, 15 Jan 2021 16:36:56 -0800 Subject: [PATCH 8/9] Remove request cloning --- sdk/core/core-https/review/core-https.api.md | 2 -- sdk/core/core-https/src/httpHeaders.ts | 7 ------ sdk/core/core-https/src/interfaces.ts | 9 -------- sdk/core/core-https/src/pipelineRequest.ts | 22 ------------------- .../src/policies/exponentialRetryPolicy.ts | 2 +- .../core-https/src/policies/redirectPolicy.ts | 9 ++++---- .../src/policies/systemErrorRetryPolicy.ts | 2 +- .../src/policies/throttlingRetryPolicy.ts | 2 +- 8 files changed, 7 insertions(+), 48 deletions(-) diff --git a/sdk/core/core-https/review/core-https.api.md b/sdk/core/core-https/review/core-https.api.md index 7c0f9a9f43c3..91e1af022ee4 100644 --- a/sdk/core/core-https/review/core-https.api.md +++ b/sdk/core/core-https/review/core-https.api.md @@ -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; @@ -159,7 +158,6 @@ export interface PipelineRequest { abortSignal?: AbortSignalLike; additionalInfo?: AdditionalInfo; body?: RequestBodyType; - clone(): PipelineRequest; formData?: FormDataMap; headers: HttpHeaders; keepAlive?: boolean; diff --git a/sdk/core/core-https/src/httpHeaders.ts b/sdk/core/core-https/src/httpHeaders.ts index 2738a530641e..50241407f956 100644 --- a/sdk/core/core-https/src/httpHeaders.ts +++ b/sdk/core/core-https/src/httpHeaders.ts @@ -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. */ diff --git a/sdk/core/core-https/src/interfaces.ts b/sdk/core/core-https/src/interfaces.ts index eb4e0125d852..795d64be7659 100644 --- a/sdk/core/core-https/src/interfaces.ts +++ b/sdk/core/core-https/src/interfaces.ts @@ -60,10 +60,6 @@ export interface HttpHeaders extends Iterable<[string, string]> { * @param name The name of the header to delete. */ delete(name: string): void; - /** - * Duplicates this collection. - */ - clone(): HttpHeaders; /** * Accesses a raw JS object that acts as a simple map * of header names to values. @@ -163,11 +159,6 @@ export interface PipelineRequest { */ spanOptions?: SpanOptions; - /** - * Clone this request object. - */ - clone(): PipelineRequest; - /** * Callback which fires upon upload progress. */ diff --git a/sdk/core/core-https/src/pipelineRequest.ts b/sdk/core/core-https/src/pipelineRequest.ts index bd8ddb2d3af3..b0a4cf436874 100644 --- a/sdk/core/core-https/src/pipelineRequest.ts +++ b/sdk/core/core-https/src/pipelineRequest.ts @@ -146,28 +146,6 @@ class PipelineRequestImpl implements PipelineRequest Date: Fri, 15 Jan 2021 16:53:13 -0800 Subject: [PATCH 9/9] Remove additionalInfo --- sdk/core/core-client/review/core-client.api.md | 2 +- .../core-client/src/deserializationPolicy.ts | 14 +++++++++----- sdk/core/core-client/src/interfaces.ts | 5 ++--- sdk/core/core-client/src/operationHelpers.ts | 16 +++++++++++++++- sdk/core/core-client/src/serializationPolicy.ts | 10 +++++++--- sdk/core/core-client/src/serviceClient.ts | 9 +++++---- .../test/deserializationPolicy.spec.ts | 6 +++--- sdk/core/core-client/test/serviceClient.spec.ts | 10 ++++++---- sdk/core/core-https/review/core-https.api.md | 6 ++---- sdk/core/core-https/src/interfaces.ts | 8 +------- sdk/core/core-https/src/pipelineRequest.ts | 12 ++---------- 11 files changed, 53 insertions(+), 45 deletions(-) diff --git a/sdk/core/core-client/review/core-client.api.md b/sdk/core/core-client/review/core-client.api.md index c9d42042999c..c3645cd31ea7 100644 --- a/sdk/core/core-client/review/core-client.api.md +++ b/sdk/core/core-client/review/core-client.api.md @@ -221,7 +221,7 @@ export interface OperationQueryParameter extends OperationParameter { } // @public -export type OperationRequest = PipelineRequest; +export type OperationRequest = PipelineRequest; // @public export interface OperationRequestInfo { diff --git a/sdk/core/core-client/src/deserializationPolicy.ts b/sdk/core/core-client/src/deserializationPolicy.ts index 31f969f292c4..24ebfa7e83aa 100644 --- a/sdk/core/core-client/src/deserializationPolicy.ts +++ b/sdk/core/core-client/src/deserializationPolicy.ts @@ -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"]; @@ -104,12 +105,13 @@ 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; @@ -117,7 +119,8 @@ function getOperationResponseMap( 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; @@ -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; } diff --git a/sdk/core/core-client/src/interfaces.ts b/sdk/core/core-client/src/interfaces.ts index 1a3dd02c8ad6..a7c4a89eff73 100644 --- a/sdk/core/core-client/src/interfaces.ts +++ b/sdk/core/core-client/src/interfaces.ts @@ -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; +export type OperationRequest = PipelineRequest; /** * Metadata that is used to properly parse a response. diff --git a/sdk/core/core-client/src/operationHelpers.ts b/sdk/core/core-client/src/operationHelpers.ts index 2bb8feda8a89..8f98c815a4f8 100644 --- a/sdk/core/core-client/src/operationHelpers.ts +++ b/sdk/core/core-client/src/operationHelpers.ts @@ -6,7 +6,9 @@ import { OperationParameter, Mapper, CompositeMapper, - ParameterPath + ParameterPath, + OperationRequestInfo, + OperationRequest } from "./interfaces"; /** @@ -103,3 +105,15 @@ function getPropertyFromParameterPath( } return result; } + +const operationRequestMap = new WeakMap(); + +export function getOperationRequestInfo(request: OperationRequest): OperationRequestInfo { + let info = operationRequestMap.get(request); + + if (!info) { + info = {}; + operationRequestMap.set(request, info); + } + return info; +} diff --git a/sdk/core/core-client/src/serializationPolicy.ts b/sdk/core/core-client/src/serializationPolicy.ts index 96df20d7ee88..250e4f58c491 100644 --- a/sdk/core/core-client/src/serializationPolicy.ts +++ b/sdk/core/core-client/src/serializationPolicy.ts @@ -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. @@ -47,8 +50,9 @@ export function serializationPolicy(options: serializationPolicyOptions = {}): P return { name: serializationPolicyName, async sendRequest(request: OperationRequest, next: SendRequest): Promise { - 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); diff --git a/sdk/core/core-client/src/serviceClient.ts b/sdk/core/core-client/src/serviceClient.ts index 8b4eac4085de..ab4d79e6a240 100644 --- a/sdk/core/core-client/src/serviceClient.ts +++ b/sdk/core/core-client/src/serviceClient.ts @@ -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. @@ -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) { @@ -181,7 +182,7 @@ export class ServiceClient { } if (requestOptions.shouldDeserialize !== undefined) { - request.additionalInfo.shouldDeserialize = requestOptions.shouldDeserialize; + operationInfo.shouldDeserialize = requestOptions.shouldDeserialize; } } diff --git a/sdk/core/core-client/test/deserializationPolicy.spec.ts b/sdk/core/core-client/test/deserializationPolicy.spec.ts index 68f09c1efa53..d776ba761f87 100644 --- a/sdk/core/core-client/test/deserializationPolicy.spec.ts +++ b/sdk/core/core-client/test/deserializationPolicy.spec.ts @@ -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() { @@ -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 = { diff --git a/sdk/core/core-client/test/serviceClient.spec.ts b/sdk/core/core-client/test/serviceClient.spec.ts index 9f1e3c9f146c..d56254b12054 100644 --- a/sdk/core/core-client/test/serviceClient.spec.ts +++ b/sdk/core/core-client/test/serviceClient.spec.ts @@ -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"; @@ -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) => { diff --git a/sdk/core/core-https/review/core-https.api.md b/sdk/core/core-https/review/core-https.api.md index 91e1af022ee4..691a46044be4 100644 --- a/sdk/core/core-https/review/core-https.api.md +++ b/sdk/core/core-https/review/core-https.api.md @@ -154,9 +154,8 @@ export interface PipelinePolicy { } // @public -export interface PipelineRequest { +export interface PipelineRequest { abortSignal?: AbortSignalLike; - additionalInfo?: AdditionalInfo; body?: RequestBodyType; formData?: FormDataMap; headers: HttpHeaders; @@ -174,9 +173,8 @@ export interface PipelineRequest { } // @public -export interface PipelineRequestOptions { +export interface PipelineRequestOptions { abortSignal?: AbortSignalLike; - additionalInfo?: AdditionalInfo; body?: RequestBodyType; formData?: FormDataMap; headers?: HttpHeaders; diff --git a/sdk/core/core-https/src/interfaces.ts b/sdk/core/core-https/src/interfaces.ts index 795d64be7659..42afd7386e82 100644 --- a/sdk/core/core-https/src/interfaces.ts +++ b/sdk/core/core-https/src/interfaces.ts @@ -84,7 +84,7 @@ export type RequestBodyType = /** * Metadata about a request being made by the pipeline. */ -export interface PipelineRequest { +export interface PipelineRequest { /** * The URL to make the request to. */ @@ -118,12 +118,6 @@ export interface PipelineRequest { */ requestId: string; - /** - * Any additional information on the request that - * is policy or client specific. - */ - additionalInfo?: AdditionalInfo; - /** * The HTTP body content (if any) */ diff --git a/sdk/core/core-https/src/pipelineRequest.ts b/sdk/core/core-https/src/pipelineRequest.ts index b0a4cf436874..a2256b83b2c3 100644 --- a/sdk/core/core-https/src/pipelineRequest.ts +++ b/sdk/core/core-https/src/pipelineRequest.ts @@ -19,18 +19,12 @@ import { SpanOptions } from "@azure/core-tracing"; * Settings to initialize a request. * Almost equivalent to Partial, but url is mandatory. */ -export interface PipelineRequestOptions { +export interface PipelineRequestOptions { /** * The URL to make the request to. */ url: string; - /** - * Any additional information on the request that - * is policy or client specific. - */ - additionalInfo?: AdditionalInfo; - /** * The HTTP method to use when making the request. */ @@ -108,7 +102,7 @@ export interface PipelineRequestOptions { onDownloadProgress?: (progress: TransferProgressEvent) => void; } -class PipelineRequestImpl implements PipelineRequest { +class PipelineRequestImpl implements PipelineRequest { public url: string; public method: HttpMethods; public headers: HttpHeaders; @@ -125,7 +119,6 @@ class PipelineRequestImpl implements PipelineRequest void; public onDownloadProgress?: (progress: TransferProgressEvent) => void; - public additionalInfo?: AdditionalInfo; constructor(options: PipelineRequestOptions) { this.url = options.url; @@ -144,7 +137,6 @@ class PipelineRequestImpl implements PipelineRequest