diff --git a/sdk/core/core-client-rest/CHANGELOG.md b/sdk/core/core-client-rest/CHANGELOG.md index d7cfbe6fa3cd..91bdd46c81f9 100644 --- a/sdk/core/core-client-rest/CHANGELOG.md +++ b/sdk/core/core-client-rest/CHANGELOG.md @@ -5,6 +5,12 @@ - Add options skipUrlEncoding to support skip path parameter encoding. +## 1.0.0-beta.8 (UNRELEASED) + +- Adding more robust handling of request and response body. [#18478](https://github.com/Azure/azure-sdk-for-js/pull/18478) + +### Other Changes + ## 1.0.0-beta.7 (2021-09-02) ### Other Changes diff --git a/sdk/core/core-client-rest/review/core-client.api.md b/sdk/core/core-client-rest/review/core-client.api.md index 862e776e2d26..87f1affc153f 100644 --- a/sdk/core/core-client-rest/review/core-client.api.md +++ b/sdk/core/core-client-rest/review/core-client.api.md @@ -22,16 +22,7 @@ export interface CertificateCredential { // @public export interface Client { path: Function; - pathUnchecked: (path: string, ...args: Array) => { - get: (options?: RequestParameters) => Promise; - post: (options?: RequestParameters) => Promise; - put: (options?: RequestParameters) => Promise; - patch: (options?: RequestParameters) => Promise; - delete: (options?: RequestParameters) => Promise; - head: (options?: RequestParameters) => Promise; - options: (options?: RequestParameters) => Promise; - trace: (options?: RequestParameters) => Promise; - }; + pathUnchecked: PathUnchecked; pipeline: Pipeline; } @@ -46,9 +37,6 @@ export type ClientOptions = PipelineOptions & { allowInsecureConnection?: boolean; }; -// @public -export function createDefaultPipeline(baseUrl: string, credential?: TokenCredential | KeyCredential, options?: ClientOptions): Pipeline; - // @public export function createRestError(message: string, response: PathUncheckedResponse): RestError; @@ -69,6 +57,16 @@ export type HttpResponse = { // @public export function isCertificateCredential(credential: unknown): credential is CertificateCredential; +// @public +export type PathParameters = TRoute extends `${infer _Head}/{${infer _Param}}${infer Tail}` ? [ +pathParameter: string, +...pathParameters: PathParameters +] : [ +]; + +// @public +export type PathUnchecked = (path: TPath, ...args: PathParameters) => ResourceMethods; + // @public export type PathUncheckedResponse = HttpResponse & { body: any; @@ -86,10 +84,15 @@ export type RequestParameters = { }; // @public -export type RouteParams = TRoute extends `${infer _Head}/{${infer _Param}}${infer Tail}` ? [ -pathParam: string, -...pathParams: RouteParams -] : [ -]; +export interface ResourceMethods { + delete: (options?: RequestParameters) => Promise; + get: (options?: RequestParameters) => Promise; + head: (options?: RequestParameters) => Promise; + options: (options?: RequestParameters) => Promise; + patch: (options?: RequestParameters) => Promise; + post: (options?: RequestParameters) => Promise; + put: (options?: RequestParameters) => Promise; + trace: (options?: RequestParameters) => Promise; +} ``` diff --git a/sdk/core/core-client-rest/src/common.ts b/sdk/core/core-client-rest/src/common.ts index 313bd0d1cc94..331471ac2911 100644 --- a/sdk/core/core-client-rest/src/common.ts +++ b/sdk/core/core-client-rest/src/common.ts @@ -1,7 +1,124 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT license. -import { PipelineOptions, PipelineRequest, RawHttpHeaders } from "@azure/core-rest-pipeline"; +import { + Pipeline, + PipelineOptions, + PipelineRequest, + RawHttpHeaders, +} from "@azure/core-rest-pipeline"; +import { RawHttpHeadersInput } from "@azure/core-rest-pipeline"; + +/** + * Shape of the default request parameters, this may be overriden by the specific + * request types to provide strong types + */ +export type RequestParameters = { + /** + * Headers to send along with the request + */ + headers?: RawHttpHeadersInput; + /** + * Sets the accept header to send to the service + * defaults to 'application/json'. If also a header "accept" is set + * this property will take precedence. + */ + accept?: string; + /** + * Body to send with the request + */ + body?: unknown; + /** + * Query parameters to send with the request + */ + queryParameters?: Record; + /** + * Set an explicit content-type to send with the request. If also a header "content-type" is set + * this property will take precedence. + */ + contentType?: string; + /** Set to true if the request is sent over HTTP instead of HTTPS */ + allowInsecureConnection?: boolean; + /** Set to true if you want to skip encoding the path parameters */ + skipUrlEncoding?: boolean; +}; + +/** + * Type to use with pathUnchecked, overrides the body type to any to allow flexibility + */ +export type PathUncheckedResponse = HttpResponse & { body: any }; + +/** + * Shape of a Rest Level Client + */ +export interface Client { + /** + * The pipeline used by this client to make requests + */ + pipeline: Pipeline; + /** + * This method will be used to send request that would check the path to provide + * strong types. When used by the codegen this type gets overriden wit the generated + * types. For example: + * ```typescript + * export type MyClient = Client & { + * path: Routes; + * } + * ``` + */ + // eslint-disable-next-line @typescript-eslint/ban-types + path: Function; + /** + * This method allows arbitrary paths and doesn't provide strong types + */ + pathUnchecked: PathUnchecked; +} + +/** + * Defines the signature for pathUnchecked. + */ +export type PathUnchecked = ( + path: TPath, + ...args: PathParameters +) => ResourceMethods; + +/** + * Defines the methods that can be called on a resource + */ +export interface ResourceMethods { + /** + * Definition of the GET HTTP method for a resource + */ + get: (options?: RequestParameters) => Promise; + /** + * Definition of the POST HTTP method for a resource + */ + post: (options?: RequestParameters) => Promise; + /** + * Definition of the PUT HTTP method for a resource + */ + put: (options?: RequestParameters) => Promise; + /** + * Definition of the PATCH HTTP method for a resource + */ + patch: (options?: RequestParameters) => Promise; + /** + * Definition of the DELETE HTTP method for a resource + */ + delete: (options?: RequestParameters) => Promise; + /** + * Definition of the HEAD HTTP method for a resource + */ + head: (options?: RequestParameters) => Promise; + /** + * Definition of the OPTIONS HTTP method for a resource + */ + options: (options?: RequestParameters) => Promise; + /** + * Definition of the TRACE HTTP method for a resource + */ + trace: (options?: RequestParameters) => Promise; +} /** * General options that a Rest Level Client can take @@ -55,3 +172,27 @@ export type HttpResponse = { */ status: string; }; + +/** + * Helper type used to detect parameters in a path template + * text surrounded by \{\} will be considered a path parameter + */ +export type PathParameters< + TRoute extends string + // This is trying to match the string in TRoute with a template where HEAD/{PARAM}/TAIL + // for example in the followint path: /foo/{fooId}/bar/{barId}/baz the template will infer + // HEAD: /foo + // Param: fooId + // Tail: /bar/{barId}/baz + // The above sample path would return [pathParam: string, pathParam: string] +> = TRoute extends `${infer _Head}/{${infer _Param}}${infer Tail}` + ? // In case we have a match for the template above we know for sure + // that we have at least one pathParameter, that's why we set the first pathParam + // in the tuple. At this point we have only matched up until param, if we want to identify + // additional parameters we can call RouteParameters recursively on the Tail to match the remaining parts, + // in case the Tail has more parameters, it will return a tuple with the parameters found in tail. + // We spread the second path params to end up with a single dimension tuple at the end. + [pathParameter: string, ...pathParameters: PathParameters] + : // When the path doesn't match the template, it means that we have no path parameters so we return + // an empty tuple. + []; diff --git a/sdk/core/core-client-rest/src/getClient.ts b/sdk/core/core-client-rest/src/getClient.ts index 69e8b5e23c60..0298b2b698ed 100644 --- a/sdk/core/core-client-rest/src/getClient.ts +++ b/sdk/core/core-client-rest/src/getClient.ts @@ -5,48 +5,10 @@ import { isTokenCredential, KeyCredential, TokenCredential } from "@azure/core-a import { isCertificateCredential } from "./certificateCredential"; import { HttpMethods, Pipeline, PipelineOptions } from "@azure/core-rest-pipeline"; import { createDefaultPipeline } from "./clientHelpers"; -import { ClientOptions, HttpResponse } from "./common"; -import { RequestParameters } from "./pathClientTypes"; +import { Client, ClientOptions, HttpResponse, RequestParameters } from "./common"; import { sendRequest } from "./sendRequest"; import { buildRequestUrl } from "./urlHelpers"; -/** - * Type to use with pathUnchecked, overrides the body type to any to allow flexibility - */ -export type PathUncheckedResponse = HttpResponse & { body: any }; - -/** - * Shape of a Rest Level Client - */ -export interface Client { - /** - * The pipeline used by this client to make requests - */ - pipeline: Pipeline; - /** - * This method will be used to send request that would check the path to provide - * strong types - */ - // eslint-disable-next-line @typescript-eslint/ban-types - path: Function; - /** - * This method allows arbitrary paths and doesn't provide strong types - */ - pathUnchecked: ( - path: string, - ...args: Array - ) => { - get: (options?: RequestParameters) => Promise; - post: (options?: RequestParameters) => Promise; - put: (options?: RequestParameters) => Promise; - patch: (options?: RequestParameters) => Promise; - delete: (options?: RequestParameters) => Promise; - head: (options?: RequestParameters) => Promise; - options: (options?: RequestParameters) => Promise; - trace: (options?: RequestParameters) => Promise; - }; -} - /** * Creates a client with a default pipeline * @param baseUrl - Base endpoint for the client diff --git a/sdk/core/core-client-rest/src/index.ts b/sdk/core/core-client-rest/src/index.ts index 6835b4e377bb..4a7209cf6597 100644 --- a/sdk/core/core-client-rest/src/index.ts +++ b/sdk/core/core-client-rest/src/index.ts @@ -6,9 +6,7 @@ * @packageDocumentation */ -export { createDefaultPipeline } from "./clientHelpers"; export { CertificateCredential, isCertificateCredential } from "./certificateCredential"; export { createRestError } from "./restError"; -export * from "./common"; export * from "./getClient"; -export * from "./pathClientTypes"; +export * from "./common"; diff --git a/sdk/core/core-client-rest/src/pathClientTypes.ts b/sdk/core/core-client-rest/src/pathClientTypes.ts deleted file mode 100644 index ed4f2f6168a1..000000000000 --- a/sdk/core/core-client-rest/src/pathClientTypes.ts +++ /dev/null @@ -1,60 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. - -import { RawHttpHeadersInput } from "@azure/core-rest-pipeline"; - -/** - * Shape of the default request parameters, this may be overriden by the specific - * request types to provide strong types - */ -export type RequestParameters = { - /** - * Headers to send along with the request - */ - headers?: RawHttpHeadersInput; - /** - * Sets the accept header to send to the service - * defaults to 'application/json' - */ - accept?: string; - /** - * Body to send with the request - */ - body?: unknown; - /** - * Query parameters to send with the request - */ - queryParameters?: Record; - /** - * Set an explicit content-type to send with the request - */ - contentType?: string; - /** Set to true if the request is sent over HTTP instead of HTTPS */ - allowInsecureConnection?: boolean; - /** Set to true if you want to skip encoding the path parameters */ - skipUrlEncoding?: boolean; -}; - -/** - * Helper type used to detect parameters in a path template - * keys surounded by \{\} will be considered a path parameter - */ -export type RouteParams< - TRoute extends string - // This is trying to match the string in TRoute with a template where HEAD/{PARAM}/TAIL - // for example in the followint path: /foo/{fooId}/bar/{barId}/baz the template will infer - // HEAD: /foo - // Param: fooId - // Tail: /bar/{barId}/baz - // The above sample path would return [pathParam: string, pathParam: string] -> = TRoute extends `${infer _Head}/{${infer _Param}}${infer Tail}` - ? // In case we have a match for the template above we know for sure - // that we have at least one pathParameter, that's why we set the first pathParam - // in the tuple. At this point we have only matched up until param, if we want to identify - // additional parameters we can call RouteParameters recursively on the Tail to match the remaining parts, - // in case the Tail has more parameters, it will return a tuple with the parameters found in tail. - // We spread the second path params to end up with a single dimension tuple at the end. - [pathParam: string, ...pathParams: RouteParams] - : // When the path doesn't match the template, it means that we have no path parameters so we return - // an empty tuple. - []; diff --git a/sdk/core/core-client-rest/src/restError.ts b/sdk/core/core-client-rest/src/restError.ts index 1cb18be69502..b2187ca79b91 100644 --- a/sdk/core/core-client-rest/src/restError.ts +++ b/sdk/core/core-client-rest/src/restError.ts @@ -1,8 +1,8 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT license. -import { PathUncheckedResponse } from "./getClient"; import { RestError, PipelineResponse, createHttpHeaders } from "@azure/core-rest-pipeline"; +import { PathUncheckedResponse } from "./common"; /** * Creates a rest error from a PathUnchecked response diff --git a/sdk/core/core-client-rest/src/sendRequest.ts b/sdk/core/core-client-rest/src/sendRequest.ts index 100f2a7d34b2..d553ec2dd240 100644 --- a/sdk/core/core-client-rest/src/sendRequest.ts +++ b/sdk/core/core-client-rest/src/sendRequest.ts @@ -4,13 +4,17 @@ import { createHttpHeaders, createPipelineRequest, + FormDataMap, HttpMethods, Pipeline, + PipelineRequest, + PipelineResponse, RawHttpHeaders, + RequestBodyType, + RestError, } from "@azure/core-rest-pipeline"; import { getCachedDefaultHttpsClient } from "./clientHelpers"; -import { RequestParameters } from "./pathClientTypes"; -import { HttpResponse } from "./common"; +import { HttpResponse, RequestParameters } from "./common"; /** * Helper function to send request used by the client @@ -27,55 +31,130 @@ export async function sendRequest( options: RequestParameters = {} ): Promise { const httpClient = getCachedDefaultHttpsClient(); + const request = buildPipelineRequest(method, url, options); + const response = await pipeline.sendRequest(httpClient, request); + const rawHeaders: RawHttpHeaders = response.headers.toJSON(); - const body = options.body !== undefined ? JSON.stringify(options.body) : undefined; + const parsedBody: RequestBodyType | undefined = getResponseBody(response); + + return { + request, + headers: rawHeaders, + status: `${response.status}`, + body: parsedBody, + }; +} + +/** + * Function to determine the content-type of a body + * this is used if an explicit content-type is not provided + * @param body - body in the request + * @returns returns the content-type + */ +function getContentType(body: any): string { + if (ArrayBuffer.isView(body)) { + return "application/octet-stream"; + } + + // By default return json + return "application/json; charset=UTF-8"; +} + +export interface InternalRequestParameters extends RequestParameters { + responseAsStream?: boolean; +} + +function buildPipelineRequest( + method: HttpMethods, + url: string, + options: InternalRequestParameters = {} +): PipelineRequest { + const { body, formData } = getRequestBody(options.body, options.contentType); + const hasContent = body !== undefined || formData !== undefined; const headers = createHttpHeaders({ + ...(options.headers ? options.headers : {}), accept: options.accept ?? "application/json", - ...(body !== undefined && { + ...(hasContent && { "content-type": options.contentType ?? getContentType(options.body), }), - ...(options.headers ? options.headers : {}), }); - const request = createPipelineRequest({ + return createPipelineRequest({ url, method, body, + formData, headers, allowInsecureConnection: options.allowInsecureConnection, }); +} + +interface RequestBody { + body?: RequestBodyType; + formData?: FormDataMap; +} - const result = await pipeline.sendRequest(httpClient, request); - const rawHeaders: RawHttpHeaders = result.headers.toJSON(); +/** + * Prepares the body before sending the request + */ +function getRequestBody(body?: unknown, contentType: string = "application/json"): RequestBody { + if (body === undefined) { + return { body: undefined }; + } - let parsedBody = undefined; + const firstType = contentType.split(";")[0]; - try { - parsedBody = result.bodyAsText ? JSON.parse(result.bodyAsText) : undefined; - } catch { - parsedBody = undefined; + switch (firstType) { + case "multipart/form-data": + return isFormData(body) ? { formData: body } : { body: JSON.stringify(body) }; + case "text/plain": + return { body: String(body) }; + default: + return { body: JSON.stringify(body) }; } +} - return { - request, - headers: rawHeaders, - status: `${result.status}`, - body: parsedBody, - }; +function isFormData(body: unknown): body is FormDataMap { + return body instanceof Object && Object.keys(body).length > 0; } /** - * Function to determine the content-type of a body - * this is used if an explicit content-type is not provided - * @param body - body in the request - * @returns returns the content-type + * Prepares the response body */ -function getContentType(body: any): string { - if (ArrayBuffer.isView(body)) { - return "application/octet-stream"; +function getResponseBody(response: PipelineResponse): RequestBodyType | undefined { + // Set the default response type + const contentType = response.headers.get("content-type") ?? ""; + const firstType = contentType.split(";")[0]; + const bodyToParse: string = response.bodyAsText ?? ""; + + if (firstType === "text/plain") { + return String(bodyToParse); } - // By default return json - return "application/json; charset=UTF-8"; + // Default to "application/json" and fallback to string; + try { + return bodyToParse ? JSON.parse(bodyToParse) : undefined; + } catch (error) { + // If we were supposed to get a JSON object and failed to + // parse, throw a parse error + if (firstType === "application/json") { + throw createParseError(response, error); + } + + // We are not sure how to handle the response so we return it as + // plain text. + return String(bodyToParse); + } +} + +function createParseError(response: PipelineResponse, err: any): RestError { + const msg = `Error "${err}" occurred while parsing the response body - ${response.bodyAsText}.`; + const errCode = err.code ?? RestError.PARSE_ERROR; + return new RestError(msg, { + code: errCode, + statusCode: response.status, + request: response.request, + response: response, + }); } diff --git a/sdk/core/core-client-rest/src/urlHelpers.ts b/sdk/core/core-client-rest/src/urlHelpers.ts index dc7961b90e79..7ed0dfa9fa43 100644 --- a/sdk/core/core-client-rest/src/urlHelpers.ts +++ b/sdk/core/core-client-rest/src/urlHelpers.ts @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT license. -import { RequestParameters } from "./pathClientTypes"; +import { RequestParameters } from "./common"; import { URL } from "./url"; /** diff --git a/sdk/core/core-client-rest/test/sendRequest.spec.ts b/sdk/core/core-client-rest/test/sendRequest.spec.ts index 0a34fda5bbd9..cf125482eae0 100644 --- a/sdk/core/core-client-rest/test/sendRequest.spec.ts +++ b/sdk/core/core-client-rest/test/sendRequest.spec.ts @@ -8,6 +8,7 @@ import { createHttpHeaders, Pipeline, PipelineResponse, + RestError, } from "@azure/core-rest-pipeline"; describe("sendRequest", () => { const mockBaseUrl = "https://example.org"; @@ -112,4 +113,168 @@ describe("sendRequest", () => { await sendRequest("POST", mockBaseUrl, mockPipeline, { body: "test" }); }); + + it("should give you back a string response", async () => { + const mockPipeline: Pipeline = createEmptyPipeline(); + mockPipeline.sendRequest = async (_client) => { + return { + headers: createHttpHeaders({ "content-type": "text/plain" }), + bodyAsText: "test", + } as PipelineResponse; + }; + + const response = await sendRequest("GET", mockBaseUrl, mockPipeline); + assert.equal(response.body, "test"); + }); + + it("should give you back a JSON object response", async () => { + const mockPipeline: Pipeline = createEmptyPipeline(); + mockPipeline.sendRequest = async (_client) => { + return { + headers: createHttpHeaders({ "content-type": "application/json" }), + bodyAsText: `{"foo": "test"}`, + } as PipelineResponse; + }; + + const response = await sendRequest("GET", mockBaseUrl, mockPipeline); + assert.deepEqual(response.body, { foo: "test" }); + }); + + it("should throw with invalid JSON", async () => { + const mockPipeline: Pipeline = createEmptyPipeline(); + mockPipeline.sendRequest = async (_client) => { + return { + headers: createHttpHeaders({ "content-type": "application/json" }), + bodyAsText: `{"foo": "test"`, + } as PipelineResponse; + }; + + try { + await sendRequest("GET", mockBaseUrl, mockPipeline); + } catch (error) { + assert.equal(error.code, RestError.PARSE_ERROR); + } + }); + + it("should give a string response with non-json and unspecified content-type", async () => { + const mockPipeline: Pipeline = createEmptyPipeline(); + mockPipeline.sendRequest = async (_client) => { + return { + headers: createHttpHeaders(), + bodyAsText: "test", + } as PipelineResponse; + }; + + const response = await sendRequest("GET", mockBaseUrl, mockPipeline); + assert.equal(response.body, "test"); + }); + + it("should send formdata body", async () => { + const testForm = { foo: "test" }; + const mockPipeline: Pipeline = createEmptyPipeline(); + mockPipeline.sendRequest = async (_client, request) => { + assert.deepEqual(request.formData, testForm); + return { + headers: createHttpHeaders(), + } as PipelineResponse; + }; + + await sendRequest("GET", mockBaseUrl, mockPipeline, { + contentType: "multipart/form-data", + body: testForm, + }); + }); + + it("should send string body", async () => { + const testBody = "test"; + const mockPipeline: Pipeline = createEmptyPipeline(); + mockPipeline.sendRequest = async (_client, request) => { + assert.deepEqual(request.body, testBody); + return { + headers: createHttpHeaders(), + } as PipelineResponse; + }; + + await sendRequest("GET", mockBaseUrl, mockPipeline, { + contentType: "text/plain", + body: testBody, + }); + }); + + it("should send json body", async () => { + const testBody = "test"; + const mockPipeline: Pipeline = createEmptyPipeline(); + mockPipeline.sendRequest = async (_client, request) => { + assert.deepEqual(request.body, JSON.stringify(testBody)); + return { + headers: createHttpHeaders(), + } as PipelineResponse; + }; + + await sendRequest("GET", mockBaseUrl, mockPipeline, { + contentType: "application/json", + body: testBody, + }); + }); + + it("should send json when no content-type set", async () => { + const testBody = { foo: "test" }; + const mockPipeline: Pipeline = createEmptyPipeline(); + mockPipeline.sendRequest = async (_client, request) => { + assert.deepEqual(request.body, JSON.stringify(testBody)); + return { + headers: createHttpHeaders(), + } as PipelineResponse; + }; + + await sendRequest("GET", mockBaseUrl, mockPipeline, { + body: testBody, + }); + }); + + it("should not set content-type when there is no body", async () => { + const mockPipeline: Pipeline = createEmptyPipeline(); + mockPipeline.sendRequest = async (_client, request) => { + const contentType = request.headers.get("content-type"); + assert.isUndefined(contentType); + return { + headers: createHttpHeaders(), + } as PipelineResponse; + }; + + await sendRequest("GET", mockBaseUrl, mockPipeline); + }); + + it("should keep accept option over header", async () => { + const mockPipeline: Pipeline = createEmptyPipeline(); + mockPipeline.sendRequest = async (_client, request) => { + const accept = request.headers.get("accept"); + assert.equal(accept, "application/json"); + return { + headers: createHttpHeaders(), + } as PipelineResponse; + }; + + await sendRequest("GET", mockBaseUrl, mockPipeline, { + accept: "application/json", + headers: { accept: "foo" }, + }); + }); + + it("should keep contentType option over header", async () => { + const mockPipeline: Pipeline = createEmptyPipeline(); + mockPipeline.sendRequest = async (_client, request) => { + const contentType = request.headers.get("content-type"); + assert.equal(contentType, "application/json"); + return { + headers: createHttpHeaders(), + } as PipelineResponse; + }; + + await sendRequest("GET", mockBaseUrl, mockPipeline, { + contentType: "application/json", + headers: { "content-type": "foo" }, + body: "test", + }); + }); });