Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] New multipart/form-data primitive in core-client-rest #29047

Merged
merged 18 commits into from
Apr 25, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
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
139 changes: 98 additions & 41 deletions common/config/rush/pnpm-lock.yaml

Large diffs are not rendered by default.

4 changes: 3 additions & 1 deletion sdk/core/core-client-rest/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
# Release History

## 1.4.1 (Unreleased)
## 2.0.0 (Unreleased)

### Features Added

### Breaking Changes

- Changed the format accepted for `multipart/form-data` requests.

### Bugs Fixed

### Other Changes
Expand Down
2 changes: 1 addition & 1 deletion sdk/core/core-client-rest/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@azure-rest/core-client",
"version": "1.4.1",
"version": "2.0.0",
"description": "Core library for interfacing with Azure Rest Clients",
"sdk-type": "client",
"type": "module",
Expand Down
22 changes: 22 additions & 0 deletions sdk/core/core-client-rest/src/helpers/isBinaryBody.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

import { isReadableStream } from "./isReadableStream.js";

export function isBinaryBody(
body: unknown,
): body is
| Uint8Array
| NodeJS.ReadableStream
| ReadableStream<Uint8Array>
| (() => NodeJS.ReadableStream)
Copy link
Member

Choose a reason for hiding this comment

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

just curious when the body would be (() => NodeJS.ReadableStream)?

Copy link
Member Author

Choose a reason for hiding this comment

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

This allows passing a resettable stream, which is a valid body type for core-rest-pipeline. I guess we don't do this in codegen at the moment, but I would like to in future.

| (() => ReadableStream<Uint8Array>)
| Blob {
return (
body !== undefined &&
(body instanceof Uint8Array ||
isReadableStream(body) ||
typeof body === "function" ||
body instanceof Blob)
);
}
186 changes: 186 additions & 0 deletions sdk/core/core-client-rest/src/multipart.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

import {
BodyPart,
MultipartRequestBody,
RestError,
createHttpHeaders,
} from "@azure/core-rest-pipeline";
import { stringToUint8Array } from "@azure/core-util";
import { isBinaryBody } from "./helpers/isBinaryBody.js";

/**
* Describes a single part in a multipart body.
*/
export interface PartDescriptor {
/**
* Content type of this part. If set, this value will be used to set the Content-Type MIME header for this part, although explicitly
* setting the Content-Type header in the headers bag will override this value. If set to `null`, no content type will be inferred from
* the body field. Otherwise, the value of the Content-Type MIME header will be inferred based on the type of the body.
*/
contentType?: string | null;
MaryGao marked this conversation as resolved.
Show resolved Hide resolved

/**
* The disposition type of this part (for example, "form-data" for parts making up a multipart/form-data request). If set, this value
* will be used to set the Content-Disposition MIME header for this part, in addition to the `name` and `filename` properties.
* Explicitly setting the Content-Disposition header in the headers bag will override this value.
*/
dispositionType?: string;

/**
* The field name associated with this part. This value will be appended to the Content-Disposition header if dispositionType is set.
*/
name?: string;
timovv marked this conversation as resolved.
Show resolved Hide resolved

/**
* The file name of the content if it is a file. This will be appended to the Content-Disposition header value if dispositionType is set.
*/
filename?: string;

/**
* The multipart headers for this part of the multipart body. Values of the Content-Type and Content-Disposition headers set in the headers bag
* will take precedence over those computed from the request body or the contentType, dispositionType, name, and filename fields on this object.
*/
headers?: Record<string, string>;
timovv marked this conversation as resolved.
Show resolved Hide resolved

/**
* The body of this part of the multipart request.
*/
body?: unknown;
}

type MultipartBodyType = BodyPart["body"];

/**
* Get value of a header in the part descriptor ignoring case
*/
function getHeaderValue(descriptor: PartDescriptor, headerName: string): string | undefined {
if (descriptor.headers) {
const actualHeaderName = Object.keys(descriptor.headers).find(
(x) => x.toLowerCase() === headerName.toLowerCase(),
);
if (actualHeaderName) {
return descriptor.headers[actualHeaderName];
}
}

return undefined;
}

function getPartContentType(descriptor: PartDescriptor): string | undefined {
const contentTypeHeader = getHeaderValue(descriptor, "content-type");
if (contentTypeHeader) {
return contentTypeHeader;
}

// Special value of null means content type is to be omitted
if (descriptor.contentType === null) {
return undefined;
}

if (descriptor.contentType) {
return descriptor.contentType;
}

const { body } = descriptor;

if (body === null || body === undefined) {
return undefined;
}

if (typeof body === "string" || typeof body === "number" || typeof body === "boolean") {
return "text/plain; charset=UTF-8";
}

if (body instanceof Blob) {
timovv marked this conversation as resolved.
Show resolved Hide resolved
return body.type || "application/octet-stream";
}

if (isBinaryBody(body)) {
return "application/octet-stream";
}

// arbitrary non-text object -> generic JSON content type by default. We will try to JSON.stringify the body.
return "application/json; charset=UTF-8";
}

function getContentDisposition(descriptor: PartDescriptor): string | undefined {
const contentDispositionHeader = getHeaderValue(descriptor, "content-disposition");
if (contentDispositionHeader) {
return contentDispositionHeader;
}

if (
descriptor.dispositionType === undefined &&
descriptor.name === undefined &&
descriptor.filename === undefined
) {
return undefined;
}

const dispositionType = descriptor.dispositionType ?? "form-data";

let disposition = dispositionType;
if (descriptor.name) {
disposition += `; name="${descriptor.name}"`;
}

const filename =
Copy link
Member

@MaryGao MaryGao Apr 23, 2024

Choose a reason for hiding this comment

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

would we trust the name set in Blob also? would we set default as blob?

Copy link
Member Author

Choose a reason for hiding this comment

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

Blob doesn't have a name property.

We don't set the default value of the filename to "blob" anymore with this change; the filename is left unspecified if it's not provided. This brings us in line with the other languages -- previously, we were unable to do that due to limitations of the browser implementation of FormData and fetch. Since we're using our own multipart implementation we no longer have have this limitation.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could set the default to blob just for the case of File and Blob instances without a file name though (and not specify a filename for other binary types when a file name is not provided)? I'm not sure I am a fan of that, it feels inconsistent.

Copy link
Member

Choose a reason for hiding this comment

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

I think we could confirm this with other languages and it should be an consistent behavior cross languages.

descriptor.filename ??
(typeof File !== "undefined" && descriptor.body instanceof File
? descriptor.body.name || undefined
: undefined);
Copy link
Member

Choose a reason for hiding this comment

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

nit: maybe a small helper function for this will help this chunk of code to be more readable

Copy link
Member Author

Choose a reason for hiding this comment

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

I expanded it to not use a ternary, hopefully that make things clearer


if (filename) {
disposition += `; filename="${filename}"`;
timovv marked this conversation as resolved.
Show resolved Hide resolved
}

return disposition;
}

function normalizeBody(body?: unknown, contentType?: string): MultipartBodyType {
if (body === undefined) {
Copy link
Member

@MaryGao MaryGao Apr 23, 2024

Choose a reason for hiding this comment

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

does that mean we would prepare the part data even if the body is set as undefined?

if yes, i was wondering if we should discard this part? undefined means absent?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we do prepare the part even with an undefined body. The headers still get prepared and sent. If the user doesn't want to send the part at all they can just not include it in the array in the first place.

// zero-length body
return new Uint8Array([]);
}

// binary and primitives should go straight on the wire regardless of content type
if (isBinaryBody(body)) {
MaryGao marked this conversation as resolved.
Show resolved Hide resolved
return body;
}
if (typeof body === "string" || typeof body === "number" || typeof body === "boolean") {
return stringToUint8Array(String(body), "utf-8");
}

// stringify objects for JSON-ish content types e.g. application/json, application/merge-patch+json, application/vnd.oci.manifest.v1+json, application.json; charset=UTF-8
if (contentType && /application\/(.+\+)?json(;.+)?/.test(contentType)) {
Copy link
Member

Choose a reason for hiding this comment

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

👍

return stringToUint8Array(JSON.stringify(body), "utf-8");
}

throw new RestError(`Unsupported body/content-type combination: ${body}, ${contentType}`);
}

export function buildBodyPart(descriptor: PartDescriptor): BodyPart {
const contentType = getPartContentType(descriptor);
const contentDisposition = getContentDisposition(descriptor);
const headers = createHttpHeaders(descriptor.headers ?? {});

if (contentType) {
headers.set("content-type", contentType);
}
if (contentDisposition) {
headers.set("content-disposition", contentDisposition);
}

const body = normalizeBody(descriptor.body, contentType);

return {
headers,
body,
};
}

export function buildMultipartBody(parts: PartDescriptor[]): MultipartRequestBody {
return { parts: parts.map(buildBodyPart) };
}
74 changes: 14 additions & 60 deletions sdk/core/core-client-rest/src/sendRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,21 @@

import {
FormDataMap,
FormDataValue,
HttpClient,
HttpMethods,
MultipartRequestBody,
Pipeline,
PipelineRequest,
PipelineResponse,
RequestBodyType,
RestError,
createFile,
createHttpHeaders,
createPipelineRequest,
} from "@azure/core-rest-pipeline";
import { getCachedDefaultHttpsClient } from "./clientHelpers.js";
import { isReadableStream } from "./helpers/isReadableStream.js";
import { HttpResponse, RequestParameters } from "./common.js";
import { PartDescriptor, buildMultipartBody } from "./multipart.js";

/**
* Helper function to send request used by the client
Expand Down Expand Up @@ -103,8 +103,8 @@ function buildPipelineRequest(
options: InternalRequestParameters = {},
): PipelineRequest {
const requestContentType = getRequestContentType(options);
const { body, formData } = getRequestBody(options.body, requestContentType);
const hasContent = body !== undefined || formData !== undefined;
const { body, formData, multipartBody } = getRequestBody(options.body, requestContentType);
timovv marked this conversation as resolved.
Show resolved Hide resolved
const hasContent = body !== undefined || formData !== undefined || multipartBody !== undefined;

const headers = createHttpHeaders({
...(options.headers ? options.headers : {}),
Expand All @@ -120,6 +120,7 @@ function buildPipelineRequest(
method,
body,
formData,
multipartBody,
headers,
allowInsecureConnection: options.allowInsecureConnection,
tracingOptions: options.tracingOptions,
Expand All @@ -137,6 +138,7 @@ function buildPipelineRequest(
interface RequestBody {
body?: RequestBodyType;
formData?: FormDataMap;
timovv marked this conversation as resolved.
Show resolved Hide resolved
multipartBody?: MultipartRequestBody;
}

/**
Expand All @@ -147,6 +149,10 @@ function getRequestBody(body?: unknown, contentType: string = ""): RequestBody {
return { body: undefined };
}

if (typeof FormData !== "undefined" && body instanceof FormData) {
return { body };
}

if (isReadableStream(body)) {
return { body };
}
Expand All @@ -163,9 +169,10 @@ function getRequestBody(body?: unknown, contentType: string = ""): RequestBody {

switch (firstType) {
case "multipart/form-data":
return isRLCFormDataInput(body)
? { formData: processFormData(body) }
: { body: JSON.stringify(body) };
if (Array.isArray(body)) {
return { multipartBody: buildMultipartBody(body as PartDescriptor[]) };
timovv marked this conversation as resolved.
Show resolved Hide resolved
}
return { body: JSON.stringify(body) };
Copy link
Member

Choose a reason for hiding this comment

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

let's have a seperated issue to talk about this, maybe we could have a common way to handle un-expected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this is just what we were doing before, I don't think it's good behavior.

case "text/plain":
return { body: String(body) };
default:
Expand All @@ -176,59 +183,6 @@ function getRequestBody(body?: unknown, contentType: string = ""): RequestBody {
}
}

/**
* Union of possible input types for multipart/form-data values that are accepted by RLCs.
* This extends the default FormDataValue type to include Uint8Array, which is accepted as an input by RLCs.
*/
type RLCFormDataValue = FormDataValue | Uint8Array;

/**
* Input shape for a form data body type as generated by an RLC
*/
type RLCFormDataInput = Record<string, RLCFormDataValue | RLCFormDataValue[]>;

function isRLCFormDataValue(value: unknown): value is RLCFormDataValue {
return (
typeof value === "string" ||
value instanceof Uint8Array ||
// We don't do `instanceof Blob` since we should also accept polyfills of e.g. File in Node.
typeof (value as Blob).stream === "function"
);
}

function isRLCFormDataInput(body: unknown): body is RLCFormDataInput {
return (
body !== undefined &&
body instanceof Object &&
Object.values(body).every(
(value) =>
isRLCFormDataValue(value) || (Array.isArray(value) && value.every(isRLCFormDataValue)),
)
);
}

function processFormDataValue(value: RLCFormDataValue): FormDataValue {
return value instanceof Uint8Array ? createFile(value, "blob") : value;
}

/**
* Checks if binary data is in Uint8Array format, if so wrap it in a Blob
* to send over the wire
*/
function processFormData(formData: RLCFormDataInput): FormDataMap {
const processedFormData: FormDataMap = {};

for (const element in formData) {
const value = formData[element];

processedFormData[element] = Array.isArray(value)
? value.map(processFormDataValue)
: processFormDataValue(value);
}

return processedFormData;
}

/**
* Prepares the response body
*/
Expand Down
Loading