-
Notifications
You must be signed in to change notification settings - Fork 112
fix: handle clientContextParam collisions with builtin config keys #1788
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
base: main
Are you sure you want to change the base?
Changes from all commits
13a1bca
f2c3c14
1d493e5
d0657db
6dc79b1
c81cf3d
c453d61
182326a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@smithy/middleware-endpoint": minor | ||
| --- | ||
|
|
||
| handle clientContextParam collisions with builtin config keys | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,8 @@ describe("local model integration test for cbor eventstreams", () => { | |
| it("should read and write cbor event streams", async () => { | ||
| const client = new XYZService({ | ||
| endpoint: "https://localhost", | ||
| }); | ||
| apiKey: { apiKey: "test-api-key" }, | ||
| } as any); | ||
|
Contributor
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. there should be no |
||
|
|
||
| const body = cbor.serialize({ | ||
| id: "alpha", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,4 +58,31 @@ describe(createConfigValueProvider.name, () => { | |
| expect(await createConfigValueProvider("v1", "endpoint", config)()).toEqual(sampleUrl); | ||
| expect(await createConfigValueProvider("v2", "endpoint", config)()).toEqual(sampleUrl); | ||
| }); | ||
|
|
||
| it("should prioritize clientContextParams over direct properties", async () => { | ||
| const config = { | ||
| apiKey: "direct-api-key", | ||
| clientContextParams: { | ||
| apiKey: "nested-api-key", | ||
|
Contributor
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. Although conflicts don't matter at the unit level (and this code is not aware of what is considered a conflict), change this test key to something else that doesn't conflict, like |
||
| }, | ||
| }; | ||
| expect(await createConfigValueProvider("apiKey", "apiKey", config, true)()).toEqual("nested-api-key"); | ||
| }); | ||
|
|
||
| it("should fall back to direct property when clientContextParams is not provided", async () => { | ||
| const config = { | ||
| customParam: "direct-value", | ||
| }; | ||
smilkuri marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| expect(await createConfigValueProvider("customParam", "customParam", config)()).toEqual("direct-value"); | ||
| }); | ||
|
|
||
| it("should fall back to direct property when clientContextParams exists but param is not in it", async () => { | ||
| const config = { | ||
| customParam: "direct-value", | ||
| clientContextParams: { | ||
| otherParam: "other-value", | ||
| }, | ||
| }; | ||
| expect(await createConfigValueProvider("customParam", "customParam", config)()).toEqual("direct-value"); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,8 @@ describe("retries", () => { | |
| it("should retry throttling and transient-error status codes", async () => { | ||
| const client = new XYZService({ | ||
| endpoint: "https://localhost/nowhere", | ||
| }); | ||
| apiKey: { apiKey: "test-api-key" }, | ||
| } as any); | ||
|
Contributor
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. remove
Contributor
Author
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. If I removed |
||
|
|
||
| requireRequestsFrom(client) | ||
| .toMatch({ | ||
|
|
@@ -50,7 +51,8 @@ describe("retries", () => { | |
| it("should retry when a retryable trait is modeled", async () => { | ||
| const client = new XYZService({ | ||
| endpoint: "https://localhost/nowhere", | ||
| }); | ||
| apiKey: { apiKey: "test-api-key" }, | ||
| } as any); | ||
|
|
||
| requireRequestsFrom(client) | ||
| .toMatch({ | ||
|
|
@@ -80,7 +82,8 @@ describe("retries", () => { | |
| it("should retry retryable trait with throttling", async () => { | ||
| const client = new XYZService({ | ||
| endpoint: "https://localhost/nowhere", | ||
| }); | ||
| apiKey: { apiKey: "test-api-key" }, | ||
| } as any); | ||
|
|
||
| requireRequestsFrom(client) | ||
| .toMatch({ | ||
|
|
@@ -110,7 +113,8 @@ describe("retries", () => { | |
| it("should not retry if the error is not modeled with retryable trait and is not otherwise retryable", async () => { | ||
| const client = new XYZService({ | ||
| endpoint: "https://localhost/nowhere", | ||
| }); | ||
| apiKey: { apiKey: "test-api-key" }, | ||
| } as any); | ||
|
|
||
| requireRequestsFrom(client) | ||
| .toMatch({ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,16 @@ | ||
| // smithy-typescript generated code | ||
| import type { | ||
| HandlerExecutionContext, | ||
| HttpAuthOption, | ||
| HttpAuthScheme, | ||
| HttpAuthSchemeParameters, | ||
| HttpAuthSchemeParametersProvider, | ||
| HttpAuthSchemeProvider, | ||
| Provider, | ||
| import { doesIdentityRequireRefresh, isIdentityExpired, memoizeIdentityProvider } from "@smithy/core"; | ||
| import { | ||
| type HandlerExecutionContext, | ||
| type HttpAuthOption, | ||
| type HttpAuthScheme, | ||
| type HttpAuthSchemeParameters, | ||
| type HttpAuthSchemeParametersProvider, | ||
| type HttpAuthSchemeProvider, | ||
| type Provider, | ||
| ApiKeyIdentity, | ||
| ApiKeyIdentityProvider, | ||
| HttpApiKeyAuthLocation, | ||
|
Contributor
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. these new imports look like types. Change the codegen part to use |
||
| } from "@smithy/types"; | ||
| import { getSmithyContext, normalizeProvider } from "@smithy/util-middleware"; | ||
|
|
||
|
|
@@ -41,9 +45,16 @@ export const defaultXYZServiceHttpAuthSchemeParametersProvider = async ( | |
| }; | ||
| }; | ||
|
|
||
| function createSmithyApiNoAuthHttpAuthOption(authParameters: XYZServiceHttpAuthSchemeParameters): HttpAuthOption { | ||
| function createSmithyApiHttpApiKeyAuthHttpAuthOption( | ||
| authParameters: XYZServiceHttpAuthSchemeParameters | ||
| ): HttpAuthOption { | ||
| return { | ||
| schemeId: "smithy.api#noAuth", | ||
| schemeId: "smithy.api#httpApiKeyAuth", | ||
| signingProperties: { | ||
| name: "X-Api-Key", | ||
| in: HttpApiKeyAuthLocation.HEADER, | ||
| scheme: undefined, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
|
|
@@ -59,7 +70,7 @@ export const defaultXYZServiceHttpAuthSchemeProvider: XYZServiceHttpAuthSchemePr | |
| const options: HttpAuthOption[] = []; | ||
| switch (authParameters.operation) { | ||
| default: { | ||
| options.push(createSmithyApiNoAuthHttpAuthOption(authParameters)); | ||
| options.push(createSmithyApiHttpApiKeyAuthHttpAuthOption(authParameters)); | ||
| } | ||
| } | ||
| return options; | ||
|
|
@@ -88,6 +99,11 @@ export interface HttpAuthSchemeInputConfig { | |
| * @internal | ||
| */ | ||
| httpAuthSchemeProvider?: XYZServiceHttpAuthSchemeProvider; | ||
|
|
||
| /** | ||
| * The API key to use when making requests. | ||
| */ | ||
| apiKey?: ApiKeyIdentity | ApiKeyIdentityProvider; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -113,6 +129,11 @@ export interface HttpAuthSchemeResolvedConfig { | |
| * @internal | ||
| */ | ||
| readonly httpAuthSchemeProvider: XYZServiceHttpAuthSchemeProvider; | ||
|
|
||
| /** | ||
| * The API key to use when making requests. | ||
| */ | ||
| readonly apiKey?: ApiKeyIdentityProvider; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -121,7 +142,9 @@ export interface HttpAuthSchemeResolvedConfig { | |
| export const resolveHttpAuthSchemeConfig = <T>( | ||
| config: T & HttpAuthSchemeInputConfig | ||
| ): T & HttpAuthSchemeResolvedConfig => { | ||
| const apiKey = memoizeIdentityProvider(config.apiKey, isIdentityExpired, doesIdentityRequireRefresh); | ||
| return Object.assign(config, { | ||
| authSchemePreference: normalizeProvider(config.authSchemePreference ?? []), | ||
| apiKey, | ||
| }) as T & HttpAuthSchemeResolvedConfig; | ||
| }; | ||
Uh oh!
There was an error while loading. Please reload this page.