-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Changes from 14 commits
16e6936
844cdb7
3800693
c76c6c2
d9033bf
5419504
5dc5f9e
07dd30e
9fa99c5
eb9f72c
86c4966
2050797
abebe89
81cafe0
82f1742
8d52335
dfaae73
0374b8c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
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) | ||
| (() => ReadableStream<Uint8Array>) | ||
| Blob { | ||
return ( | ||
body !== undefined && | ||
(body instanceof Uint8Array || | ||
isReadableStream(body) || | ||
typeof body === "function" || | ||
body instanceof Blob) | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,196 @@ | ||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT license. | ||
|
||
import { | ||
BodyPart, | ||
MultipartRequestBody, | ||
RawHttpHeadersInput, | ||
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?: RawHttpHeadersInput; | ||
|
||
/** | ||
* The body of this part of the multipart request. | ||
*/ | ||
body?: unknown; | ||
} | ||
|
||
type MultipartBodyType = BodyPart["body"]; | ||
|
||
type HeaderValue = RawHttpHeadersInput[string]; | ||
|
||
/** | ||
* Get value of a header in the part descriptor ignoring case | ||
*/ | ||
function getHeaderValue(descriptor: PartDescriptor, headerName: string): HeaderValue | 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): HeaderValue | 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"; | ||
} | ||
|
||
/** | ||
* Enclose value in quotes and escape special characters, for use in the Content-Disposition header | ||
*/ | ||
function escapeDispositionField(value: string): string { | ||
return JSON.stringify(value); | ||
} | ||
|
||
function getContentDisposition(descriptor: PartDescriptor): HeaderValue | 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=${escapeDispositionField(descriptor.name)}`; | ||
} | ||
|
||
const filename = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could set the default to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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=${escapeDispositionField(filename)}`; | ||
} | ||
|
||
return disposition; | ||
} | ||
|
||
function normalizeBody(body?: unknown, contentType?: HeaderValue): MultipartBodyType { | ||
if (body === undefined) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 if yes, i was wondering if we should discard this part? undefined means absent? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we do prepare the part even with an |
||
// 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(String(contentType))) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. case sensitive or not? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, yeah, apparently MIME types are case-insensitive. I'll make this case-insensitive, but there are definitely other places where we're just doing a plain equality check on the content type... maybe we shouldn't be doing that? |
||
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) }; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,22 +2,21 @@ | |
// Licensed under the MIT license. | ||
|
||
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 | ||
|
@@ -103,8 +102,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, multipartBody } = getRequestBody(options.body, requestContentType); | ||
const hasContent = body !== undefined || multipartBody !== undefined; | ||
|
||
const headers = createHttpHeaders({ | ||
...(options.headers ? options.headers : {}), | ||
|
@@ -119,7 +118,7 @@ function buildPipelineRequest( | |
url, | ||
method, | ||
body, | ||
formData, | ||
multipartBody, | ||
headers, | ||
allowInsecureConnection: options.allowInsecureConnection, | ||
tracingOptions: options.tracingOptions, | ||
|
@@ -136,7 +135,7 @@ function buildPipelineRequest( | |
|
||
interface RequestBody { | ||
body?: RequestBodyType; | ||
formData?: FormDataMap; | ||
multipartBody?: MultipartRequestBody; | ||
} | ||
|
||
/** | ||
|
@@ -147,6 +146,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 }; | ||
} | ||
|
@@ -163,9 +166,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) }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
@@ -176,59 +180,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 | ||
*/ | ||
|
There was a problem hiding this comment.
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)
?There was a problem hiding this comment.
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.