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

fix(endpoint): dedupe clientContext/builtIn params, fix s3 unit test #4051

Merged
merged 3 commits into from Oct 18, 2022
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 3 additions & 4 deletions clients/client-s3/test/S3.spec.ts
Expand Up @@ -19,12 +19,11 @@ describe("endpoint", () => {
expect(request.protocol).to.equal("http:");
expect(request.hostname).to.equal("localhost");
expect(request.port).to.equal(8080);
//query and path should not be overwritten
expect(request.query).not.to.contain({ foo: "bar" });
expect(request.path).not.to.equal("/path");
expect(request.path).to.equal("/path/bucket/key");
return Promise.resolve({ output: {} as any, response: {} as any });
};
const client = new S3({ endpoint: "http://localhost:8080/path?foo=bar" });
const client = new S3({ endpoint: "http://localhost:8080/path", forcePathStyle: true });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we no longer test the omission of the query param, because endpoints 2.0 requires that a query param custom endpoint be rejected instead of modified.


client.middlewareStack.add(endpointValidator, {
step: "serialize",
name: "endpointValidator",
Expand Down
Expand Up @@ -30,7 +30,10 @@ public AddEndpointsV2ParameterNameMap() {
"ForcePathStyle", "forcePathStyle",
"Accelerate", "useAccelerateEndpoint",
"DisableMRAP", "disableMultiregionAccessPoints",
"UseArnRegion", "useArnRegion"
"DisableMultiRegionAccessPoints", "disableMultiregionAccessPoints",
"UseArnRegion", "useArnRegion",
"Endpoint", "endpoint",
"UseGlobalEndpoint", "useGlobalEndpoint"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm setting more param names to match our existing param names.
The codegen has also been modified to default to camelCase if this map does not cover future params.

));
}
}
58 changes: 50 additions & 8 deletions packages/util-endpoints/src/lib/parseURL.spec.ts
@@ -1,23 +1,42 @@
import { Endpoint, EndpointURL, EndpointURLScheme } from "@aws-sdk/types";

import { parseURL } from "./parseURL";

describe(parseURL.name, () => {
it.each([
["https://example.com", { scheme: "https", authority: "example.com", path: "/", normalizedPath: "/", isIp: false }],
const testCases: [string, EndpointURL][] = [
[
"https://example.com",
{ scheme: EndpointURLScheme.HTTPS, authority: "example.com", path: "/", normalizedPath: "/", isIp: false },
],
[
"http://example.com:80/foo/bar",
{ scheme: "http", authority: "example.com:80", path: "/foo/bar", normalizedPath: "/foo/bar/", isIp: false },
{
scheme: EndpointURLScheme.HTTP,
authority: "example.com:80",
path: "/foo/bar",
normalizedPath: "/foo/bar/",
isIp: false,
},
],
[
"https://127.0.0.1",
{ scheme: EndpointURLScheme.HTTPS, authority: "127.0.0.1", path: "/", normalizedPath: "/", isIp: true },
],
["https://127.0.0.1", { scheme: "https", authority: "127.0.0.1", path: "/", normalizedPath: "/", isIp: true }],
[
"https://127.0.0.1:8443",
{ scheme: "https", authority: "127.0.0.1:8443", path: "/", normalizedPath: "/", isIp: true },
{ scheme: EndpointURLScheme.HTTPS, authority: "127.0.0.1:8443", path: "/", normalizedPath: "/", isIp: true },
],
[
"https://[fe80::1]",
{ scheme: EndpointURLScheme.HTTPS, authority: "[fe80::1]", path: "/", normalizedPath: "/", isIp: true },
],
["https://[fe80::1]", { scheme: "https", authority: "[fe80::1]", path: "/", normalizedPath: "/", isIp: true }],
[
"https://[fe80::1]:8443",
{ scheme: "https", authority: "[fe80::1]:8443", path: "/", normalizedPath: "/", isIp: true },
{ scheme: EndpointURLScheme.HTTPS, authority: "[fe80::1]:8443", path: "/", normalizedPath: "/", isIp: true },
],
])("test '%s'", (input, output) => {
];

it.each(testCases)("test '%s'", (input: string, output: EndpointURL) => {
expect(parseURL(input)).toEqual(output);
});

Expand All @@ -32,4 +51,27 @@ describe(parseURL.name, () => {
it("returns null for invalid URL", () => {
expect(parseURL("invalid")).toBeNull();
});

it.each(testCases)("test as URL '%s'", (input: string, output: EndpointURL) => {
const url = new URL(input);
expect(parseURL(url)).toEqual({
...output,
authority: url.hostname + (url.port ? `:${url.port}` : ""),
});
});

it.each(testCases)("test as EndpointV1 '%s'", (input: string, output: EndpointURL) => {
const url = new URL(input);
const endpointV1: Endpoint = {
protocol: url.protocol,
hostname: url.hostname,
port: url.port ? Number(url.port) : undefined,
path: url.pathname,
};

expect(parseURL(endpointV1)).toEqual({
...output,
authority: url.hostname + (url.port ? `:${url.port}` : ""),
});
});
});
27 changes: 23 additions & 4 deletions packages/util-endpoints/src/lib/parseURL.ts
@@ -1,4 +1,4 @@
import { EndpointURL, EndpointURLScheme } from "@aws-sdk/types";
import { Endpoint, EndpointURL, EndpointURLScheme } from "@aws-sdk/types";

import { isIpAddress } from "./isIpAddress";

Expand All @@ -8,21 +8,35 @@ const DEFAULT_PORTS: Record<EndpointURLScheme, number> = {
};

/**
* Parses a string into it’s Endpoint URL components.
* Parses a string, URL, or Endpoint into it’s Endpoint URL components.
*/
export const parseURL = (value: string): EndpointURL | null => {
export const parseURL = (value: string | URL | Endpoint): EndpointURL | null => {
kuhe marked this conversation as resolved.
Show resolved Hide resolved
const whatwgURL = (() => {
try {
if (value instanceof URL) {
return value;
}
if (typeof value === "object" && "hostname" in value) {
const { hostname, port, protocol = "", path = "", query = {} } = value as Endpoint;
const url = new URL(`${protocol}//${hostname}${port ? `:${port}` : ""}${path}`);
url.search = Object.entries(query)
.map(([k, v]) => `${k}=${v}`)
.join("&");
return url;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is to cover deprecated behavior: resolveEndpointConfig.ts changes the endpoint config to return a V1 endpoint for use by Upload and the s3 signer.

Here we convert it back to a URL to enforce consistency in the endpoints 2.0 resolver.

return new URL(value);
} catch (error) {
return null;
}
})();

if (!whatwgURL) {
console.error(`Unable to parse ${JSON.stringify(value)} as a whatwg URL.`);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a message here because the endpoints resolved error gives too little info.

return null;
}

const urlString = whatwgURL.href;

const { host, hostname, pathname, protocol, search } = whatwgURL;

if (search) {
Expand All @@ -35,7 +49,12 @@ export const parseURL = (value: string): EndpointURL | null => {
}

const isIp = isIpAddress(hostname);
const authority = `${host}${value.includes(`${host}:${DEFAULT_PORTS[scheme]}`) ? `:${DEFAULT_PORTS[scheme]}` : ``}`;

const inputContainsDefaultPort =
urlString.includes(`${host}:${DEFAULT_PORTS[scheme]}`) ||
(typeof value === "string" && value.includes(`${host}:${DEFAULT_PORTS[scheme]}`));

const authority = `${host}${inputContainsDefaultPort ? `:${DEFAULT_PORTS[scheme]}` : ``}`;

return {
scheme,
Expand Down