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

Provide one class/interface for all clients #5856

Closed
2 tasks
workeitel opened this issue Mar 6, 2024 · 8 comments
Closed
2 tasks

Provide one class/interface for all clients #5856

workeitel opened this issue Mar 6, 2024 · 8 comments
Assignees
Labels
closed-for-staleness feature-request New feature or enhancement. May require GitHub community feedback.

Comments

@workeitel
Copy link
Contributor

Describe the feature

Passing aws clients around in TypeScript requires knowing their type. This is easy for one client like DynamoDB but becomes impossible for any client. The fact that there is no single type makes for example middleware helpers difficult.

Use Case

Writing a middleware typically requires knowing the TypeScript type:

const client = new DynamoDB({ region: "us-west-2" });

addMyMiddleware(client);

But typing the addMyMiddleware is difficult as the type of an AWS Client is difficult to describe:

function addMyMiddleware(
    client: Client<
      HttpHandlerOptions,
      any, // defined as ClientInput extends object which is tricky to type as well
      MetadataBearer,
      SmithyResolvedConfiguration<HttpHandlerOptions> &
        RegionResolvedConfig &
        RetryResolvedConfig
    >
  ) { ... }

This is in particular bad as the types change between aws-sdk versions. For example Client was originally part of @aws-sdk/smithy-client but changed between versions to @smithy/smithy-client. Providing a addMyMiddleware method which works with all aws-sdk-js-v3 versions makes typing impossible.

Proposed Solution

I want to have one class/interface which describes all aws-sdk clients such as:

function addMyMiddleware(client: AWSClient) { ... }

This new type is stable across aws-sdk versions so the middleware does not need to be modified on aws-sdk updates.

Other Information

Writing a middleware for a specific aws-sdk-js-v3 version is not a problem. This feature is in particular helpful for vending middleware to many consumers with different aws-sdk-js-v3 versions.

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

SDK version used

all of them

Environment details (OS name and version, etc.)

all of them

@workeitel workeitel added feature-request New feature or enhancement. May require GitHub community feedback. needs-triage This issue or PR still needs to be triaged. labels Mar 6, 2024
@kuhe
Copy link
Contributor

kuhe commented Mar 7, 2024

What parts of the client does your middleware interact with?

Why not use a structural subset?

Client is still exported from @aws-sdk/types. It's an alias of the one from @smithy.

@trivikr trivikr added response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. and removed needs-triage This issue or PR still needs to be triaged. labels Mar 7, 2024
@workeitel
Copy link
Contributor Author

We own many different middleware integrations for different purpose. For example extracting logs/metrics from all aws-sdk calls.

I did not know that Client is reexported from @aws-sdk/types. That makes some things easier. But it still requires for example:

import { SmithyResolvedConfiguration } from "@smithy/smithy-client";
import { RegionResolvedConfig } from "@aws-sdk/config-resolver";
import { RetryResolvedConfig } from "@aws-sdk/middleware-retry";

function addMyMiddleware(
    client:Client<
      any,
      MetadataBearer,
      SmithyResolvedConfiguration<HttpHandlerOptions> &
        RegionResolvedConfig &
        RetryResolvedConfig
    >

the imports changed over time for example from @aws-sdk/smithy-client to @smithy/smithy-client similar as StandardRetryStrategy or AdaptiveRetryStrategy changed from @aws-sdk/util-retry to @smithy/util-retry.

@kuhe
Copy link
Contributor

kuhe commented Mar 12, 2024

We do not have a base type because no particular behavior is guaranteed for a client based on the Smithy code generators, despite there being things that for practical purposes exist on all clients.

Below is an outline of what I recommend, which is to declare any required types of your middleware application function on that function's signature.

import type { LambdaClient } from "@aws-sdk/client-lambda";
import type { S3Client } from "@aws-sdk/client-s3";
import type { AwsCredentialIdentityProvider, Client } from "@smithy/types"; // same as Client @aws-sdk/types.

/**
 * The most generic client is one that has no restrictions on input/output,
 * and no requirements for the resolved config.
 */
type BaseClient = Client<any, any, {}>;

const s3 = null as any as S3Client;
const lambda = null as any as LambdaClient;

/**
 * SDK clients are assignable to this most generic client.
 */
let generic: BaseClient = s3;
generic = lambda;

/**
 * However, I would guess that you're looking for a configuration subset that is guaranteed for
 * all clients.
 *
 * We have not established this type because while Smithy code generation does generate certain
 * config fields for (in practice) all clients like systemClockOffset or requestHandler, it does so explicitly
 * for each client rather than to meet any particular base type.
 *
 * Therefore, there are no particular fields that are in fact guaranteed for all potential
 * clients.
 */

/**
 * In order to create a middleware applicator, you should specify exactly what you require.
 */
type MinimalGenericClient = BaseClient & {
  config: {
    credentials: AwsCredentialIdentityProvider;
    systemClockOffset: number;
  };
};

async function addMiddleware(client: MinimalGenericClient) {
  console.log(client.config.systemClockOffset);
  await client.config.credentials();
  client.middlewareStack.add(null as any, null as any);
  client.middlewareStack.addRelativeTo(null as any, null as any);
}

addMiddleware(s3);
addMiddleware(lambda);

/**
 * For handling changed internals, discern with union.
 */
type ImplementationAlphaIsAvailable = {
  config: {
    propertyA: AwsCredentialIdentityProvider; // pretend this only exists on some SDK versions.
  };
};
type ImplementationBetaIsAvailable = {
  config: {
    propertyB: AwsCredentialIdentityProvider; // pretend this only exists on other SDK versions.
  };
};
type ClientWithDivergentImplementations = BaseClient & (ImplementationAlphaIsAvailable | ImplementationBetaIsAvailable);

/**
 * Middleware applicator handling potentially divergent SDK config implementations.
 */
async function addMiddlewareChecked(client: ClientWithDivergentImplementations) {
  if (hasImplementationAlpha(client)) {
    await client.config.propertyA();
    client.middlewareStack.add(null as any, null as any);
  } else {
    await client.config.propertyB();
    client.middlewareStack.add(null as any, null as any);
  }
}

function hasImplementationAlpha(client: any): client is ImplementationAlphaIsAvailable {
  return !!client?.config?.propertyA;
}

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Mar 13, 2024
@workeitel
Copy link
Contributor Author

Thanks for this suggestion. That's a good temporary solution but I don't think this should be hard-coded by aws-sdk consumers. It might not be possible to provide something for generic smithy generated clients but at least for aws-sdk clients the list of config values and middleware stack is known (example: https://github.com/aws/aws-sdk-js-v3/blame/main/clients/client-appconfig/src/AppConfigClient.ts#L605-L628 ).

Why is it not possible to ship in @aws-sdk/types an interface to at least model the stable part of the contract. For example:

 export interface ServiceClient {
   config: {
     maxAttempts: () => Promise<number>;
     region: () => Promise<string>;
     retryStrategy: () => Promise<RetryStrategy | RetryStrategyV2>;
   };
   middlewareStack: {
     add: MiddlewareStack<any, MetadataBearer>["add"];
   };
 }

That does not solve the full problem because for example moving ServiceException from @aws-sdk/smithy-client to @smithy/smithy-client is still a breaking change which requires consumers to update their code on sdk update and makes a middleware which works across aws-sdk versions impossible. But at least it would define a bit better contract for aws-sdk clients.

@kuhe
Copy link
Contributor

kuhe commented Mar 21, 2024

@aws-sdk/smithy-client re-exports everything from @smithy/smithy-client, so although it is deprecated, it will function in the same way, even on prototype identity if no package duplication exists.

In what way is middleware not working across the package migration of smithy-client?

@rheidari
Copy link

We cannot use @aws-sdk/smithy-client because it is marked as private beginning in 3.375.0

@aBurmeseDev
Copy link
Member

Hi @workeitel - checking in here if you still have any other questions we can answer.

@aBurmeseDev aBurmeseDev added the closing-soon This issue will automatically close in 4 days unless further comments are made. label May 1, 2024
@aBurmeseDev aBurmeseDev self-assigned this May 1, 2024
@github-actions github-actions bot added closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels May 6, 2024
@github-actions github-actions bot closed this as completed May 6, 2024
@workeitel
Copy link
Contributor Author

The problem is not really resolved. @aws-sdk/smithy-client does not work anymore and instead all consumers need to move to @smithy/smithy-client. Similar for other classes. Hard-coding own types could help but as soon as the middleware needs to interact with the client the problem still persists.

For example a middleware accessing the retry strategy similar to:

const retry = await client.config.retryStrategy();
if (retry instanceof AdaptiveRetryStrategy) {
  ...

would again not work as AdaptiveRetryStrategy changed from @aws-sdk/util-retry to @smithy/util-retry.

I understand for a generic smithy client that's not possible but aws-sdk clients are more uniform. I understand you can't guarantee the types won't change between releases but at least the import parts should really stay as otherwise consumers always need a code change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-for-staleness feature-request New feature or enhancement. May require GitHub community feedback.
Projects
None yet
Development

No branches or pull requests

5 participants