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-rest-pipeline] Fix handling of typed arrays in request bodies #15904

Merged
merged 1 commit into from
Jun 23, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
9 changes: 8 additions & 1 deletion sdk/core/core-rest-pipeline/src/nodeHttpClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,14 @@ class NodeHttpClient implements HttpClient {
if (body && isReadableStream(body)) {
body.pipe(req);
} else if (body) {
req.end(body);
if (typeof body === "string" || Buffer.isBuffer(body)) {
req.end(body);
} else if (isArrayBuffer(body)) {
req.end(ArrayBuffer.isView(body) ? Buffer.from(body.buffer) : Buffer.from(body));
} else {
logger.error("Unrecognized body type", body);
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 is basically a "never" situation since:

  • FormData would be handled by the isReadableStream case above
  • Blob is browser only

There are no other supported body types after that.

Copy link
Member

Choose a reason for hiding this comment

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

How is FormData handled by isReadableStream? It doesn't have pipe function. also I had thought it's browser only (go to definition opens lib.dom.d.ts) and I didn't find it in nodejs docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

FormData (the type you saw) gets converted by this policy when in Node:

It uses an npm package form-data to create another object type (confusingly also called FormData) that implements the Stream interface.

throw new RestError("Unrecognized body type");
}
} else {
// streams don't like "undefined" being passed as data
req.end();
Expand Down
73 changes: 73 additions & 0 deletions sdk/core/core-rest-pipeline/test/node/nodeHttpClient.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,23 @@ class FakeRequest extends PassThrough {
}
}

/**
* Generic NodeJS streams accept typed arrays just fine,
* but `http.ClientRequest` objects *only* support chunks
* of `Buffer` and `string`, so we must convert them first.
*
* This fake asserts we have only passed the correct types.
*/
const httpRequestChecker = {
on() {
/* no op */
},
end(chunk: unknown) {
const isString = typeof chunk === "string";
assert(isString || Buffer.isBuffer(chunk), "Expected either string or Buffer");
}
};

function createResponse(statusCode: number, body = ""): IncomingMessage {
const response = new FakeResponse();
response.headers = {};
Expand Down Expand Up @@ -259,4 +276,60 @@ describe("NodeHttpClient", function() {
assert.strictEqual(response.status, 200);
assert.strictEqual(response.bodyAsText, inputString);
});

it("should handle typed array bodies correctly", async function() {
const client = createDefaultHttpClient();
stubbedHttpsRequest.returns(httpRequestChecker);

const data = new Uint8Array(10);
for (let i = 0; i < 10; i++) {
data[i] = i;
}

const request = createPipelineRequest({ url: "https://example.com", body: data });
const promise = client.sendRequest(request);
stubbedHttpsRequest.yield(createResponse(200));
const response = await promise;
assert.strictEqual(response.status, 200);
});

it("should handle ArrayBuffer bodies correctly", async function() {
const client = createDefaultHttpClient();
stubbedHttpsRequest.returns(httpRequestChecker);

const data = new Uint8Array(10);
for (let i = 0; i < 10; i++) {
data[i] = i;
}

const request = createPipelineRequest({ url: "https://example.com", body: data.buffer });
const promise = client.sendRequest(request);
stubbedHttpsRequest.yield(createResponse(200));
const response = await promise;
assert.strictEqual(response.status, 200);
});

it("should handle Buffer bodies correctly", async function() {
const client = createDefaultHttpClient();
stubbedHttpsRequest.returns(httpRequestChecker);

const data = Buffer.from("example text");

const request = createPipelineRequest({ url: "https://example.com", body: data });
const promise = client.sendRequest(request);
stubbedHttpsRequest.yield(createResponse(200));
const response = await promise;
assert.strictEqual(response.status, 200);
});

it("should handle string bodies correctly", async function() {
const client = createDefaultHttpClient();
stubbedHttpsRequest.returns(httpRequestChecker);

const request = createPipelineRequest({ url: "https://example.com", body: "test data" });
const promise = client.sendRequest(request);
stubbedHttpsRequest.yield(createResponse(200));
const response = await promise;
assert.strictEqual(response.status, 200);
});
});