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

chore(protocol-http): switch to HttpRequest static clone method #1345

Merged
merged 2 commits into from
Jul 18, 2024
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
8 changes: 8 additions & 0 deletions .changeset/poor-dolls-cheer.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
"@smithy/protocol-http": minor
"@smithy/signature-v4": minor
"@smithy/core": minor
"@smithy/experimental-identity-and-auth": patch
---

switch to static HttpRequest clone method
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export class HttpApiKeyAuthSigner implements HttpSigner {
if (!identity.apiKey) {
throw new Error("request could not be signed with `apiKey` since the `apiKey` is not defined");
}
const clonedRequest = httpRequest.clone();
const clonedRequest = HttpRequest.clone(httpRequest);
if (signingProperties.in === HttpApiKeyAuthLocation.QUERY) {
clonedRequest.query[signingProperties.name] = identity.apiKey;
} else if (signingProperties.in === HttpApiKeyAuthLocation.HEADER) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export class HttpBearerAuthSigner implements HttpSigner {
identity: TokenIdentity,
signingProperties: Record<string, any>
): Promise<IHttpRequest> {
const clonedRequest = httpRequest.clone();
const clonedRequest = HttpRequest.clone(httpRequest);
if (!identity.token) {
throw new Error("request could not be signed with `token` since the `token` is not defined");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export class SigV4Signer implements HttpSigner {
identity: AwsCredentialIdentity,
signingProperties: Record<string, any>
): Promise<IHttpRequest> {
const clonedRequest = httpRequest.clone();
const clonedRequest = HttpRequest.clone(httpRequest);
const signer = new SignatureV4({
applyChecksum: signingProperties.applyChecksum !== undefined ? signingProperties.applyChecksum : true,
credentials: identity,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export class HttpApiKeyAuthSigner implements HttpSigner {
if (!identity.apiKey) {
throw new Error("request could not be signed with `apiKey` since the `apiKey` is not defined");
}
const clonedRequest = httpRequest.clone();
const clonedRequest = HttpRequest.clone(httpRequest);
if (signingProperties.in === HttpApiKeyAuthLocation.QUERY) {
clonedRequest.query[signingProperties.name] = identity.apiKey;
} else if (signingProperties.in === HttpApiKeyAuthLocation.HEADER) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export class HttpBearerAuthSigner implements HttpSigner {
identity: TokenIdentity,
signingProperties: Record<string, any>
): Promise<IHttpRequest> {
const clonedRequest = httpRequest.clone();
const clonedRequest = HttpRequest.clone(httpRequest);
if (!identity.token) {
throw new Error("request could not be signed with `token` since the `token` is not defined");
}
Expand Down
63 changes: 61 additions & 2 deletions packages/protocol-http/src/httpRequest.spec.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { QueryParameterBag } from "@smithy/types";

import { HttpRequest, IHttpRequest } from "./httpRequest";

describe("HttpRequest", () => {
Expand Down Expand Up @@ -44,8 +46,8 @@ describe("HttpRequest", () => {
it("should maintain a deprecated instance clone method", () => {
const httpRequestInstance = new HttpRequest(httpRequest);

const clone1 = httpRequestInstance.clone();
const clone2 = httpRequestInstance.clone();
const clone1 = HttpRequest.clone(httpRequestInstance);
const clone2 = HttpRequest.clone(httpRequestInstance);

expect(httpRequestInstance).toEqual(clone1);
expect(clone1).toEqual(clone2);
Expand All @@ -55,3 +57,60 @@ describe("HttpRequest", () => {
expect(clone1.body).toBe(clone2.body);
});
});

const cloneRequest = HttpRequest.clone;

describe("cloneRequest", () => {
const request: IHttpRequest = Object.freeze({
method: "GET",
protocol: "https:",
hostname: "foo.us-west-2.amazonaws.com",
path: "/",
headers: Object.freeze({
foo: "bar",
compound: "value 1, value 2",
}),
query: Object.freeze({
fizz: "buzz",
snap: ["crackle", "pop"],
}),
});

it("should return an object matching the provided request", () => {
expect(cloneRequest(request)).toEqual(request);
});

it("should return an object that with a different identity", () => {
expect(cloneRequest(request)).not.toBe(request);
});

it("should should deep-copy the headers", () => {
const clone = cloneRequest(request);

delete clone.headers.compound;
expect(Object.keys(request.headers)).toEqual(["foo", "compound"]);
expect(Object.keys(clone.headers)).toEqual(["foo"]);
});

it("should should deep-copy the query", () => {
const clone = cloneRequest(request);

const { snap } = clone.query as QueryParameterBag;
(snap as Array<string>).shift();

expect((request.query as QueryParameterBag).snap).toEqual(["crackle", "pop"]);
expect((clone.query as QueryParameterBag).snap).toEqual(["pop"]);
});

it("should not copy the body", () => {
const body = new Uint8Array(16);
const req = { ...request, body };
const clone = cloneRequest(req);

expect(clone.body).toBe(req.body);
});

it("should handle requests without defined query objects", () => {
expect(cloneRequest({ ...request, query: void 0 }).query).toEqual({});
});
});
2 changes: 1 addition & 1 deletion packages/signature-v4/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"license": "Apache-2.0",
"dependencies": {
"@smithy/is-array-buffer": "workspace:^",
"@smithy/protocol-http": "workspace:^",
"@smithy/types": "workspace:^",
"@smithy/util-hex-encoding": "workspace:^",
"@smithy/util-middleware": "workspace:^",
Expand All @@ -34,7 +35,6 @@
},
"devDependencies": {
"@aws-crypto/sha256-js": "5.2.0",
"@smithy/protocol-http": "workspace:^",
"concurrently": "7.0.0",
"downlevel-dts": "0.10.1",
"rimraf": "3.0.2",
Expand Down
2 changes: 1 addition & 1 deletion packages/signature-v4/src/SignatureV4.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ describe("SignatureV4", () => {
});

it("should sign requests without host header", async () => {
const request = minimalRequest.clone();
const request = HttpRequest.clone(minimalRequest);
delete request.headers[HOST_HEADER];
const { headers } = await signer.sign(request, {
signingDate: new Date("2000-01-01T00:00:00.000Z"),
Expand Down
58 changes: 0 additions & 58 deletions packages/signature-v4/src/cloneRequest.spec.ts

This file was deleted.

19 changes: 0 additions & 19 deletions packages/signature-v4/src/cloneRequest.ts

This file was deleted.

12 changes: 5 additions & 7 deletions packages/signature-v4/src/moveHeadersToQuery.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
import { HttpRequest, QueryParameterBag } from "@smithy/types";

import { cloneRequest } from "./cloneRequest";
import { HttpRequest } from "@smithy/protocol-http";
import type { HttpRequest as IHttpRequest, QueryParameterBag } from "@smithy/types";

/**
* @private
*/
export const moveHeadersToQuery = (
request: HttpRequest,
request: IHttpRequest,
options: { unhoistableHeaders?: Set<string> } = {}
): HttpRequest & { query: QueryParameterBag } => {
const { headers, query = {} as QueryParameterBag } =
typeof (request as any).clone === "function" ? (request as any).clone() : cloneRequest(request);
): IHttpRequest & { query: QueryParameterBag } => {
const { headers, query = {} as QueryParameterBag } = HttpRequest.clone(request);
for (const name of Object.keys(headers)) {
const lname = name.toLowerCase();
if (lname.slice(0, 6) === "x-amz-" && !options.unhoistableHeaders?.has(lname)) {
Expand Down
8 changes: 4 additions & 4 deletions packages/signature-v4/src/prepareRequest.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
import { HttpRequest } from "@smithy/types";
import { HttpRequest } from "@smithy/protocol-http";
import type { HttpRequest as IHttpRequest } from "@smithy/types";

import { cloneRequest } from "./cloneRequest";
import { GENERATED_HEADERS } from "./constants";

/**
* @private
*/
export const prepareRequest = (request: HttpRequest): HttpRequest => {
export const prepareRequest = (request: IHttpRequest): IHttpRequest => {
// Create a clone of the request object that does not clone the body
request = typeof (request as any).clone === "function" ? (request as any).clone() : cloneRequest(request);
request = HttpRequest.clone(request);

for (const headerName of Object.keys(request.headers)) {
if (GENERATED_HEADERS.indexOf(headerName.toLowerCase()) > -1) {
Expand Down
Loading