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

MiddlewareStacks added to commands in lib-dynamodb never get applied. #3095

Closed
c0pp3rt0p opened this issue Dec 14, 2021 · 4 comments
Closed
Assignees
Labels
bug This issue is a bug. closed-for-staleness p1 This is a high priority issue

Comments

@c0pp3rt0p
Copy link

c0pp3rt0p commented Dec 14, 2021

Describe the bug

Commands in lib-dynamodb do not add their middelwareStack to the underlying command.

Adding any middleware stack to a command in the lib-dynamodb package has no effect.

Your environment

NodeJs/TS on mac

SDK version number

@aws-sdk/package-name@3.43.0

Is the issue in the browser/Node.js/ReactNative?

no

Details of the browser/Node.js/ReactNative version

$ node -v
v14.17.0

Steps to reproduce

Please share code or minimal repo, and steps to reproduce the behavior.

import { QueryCommand } from '@aws-sdk/lib-dynamodb';
import { dynamoDocClient } from './dynamodb-client';

        const cmd = new QueryCommand({
          TableName: "MyTable",
          KeyConditionExpression: "id = :id",
          ExpressionAttributeValues: {
            ':id': id,
          },
        });
        cmd.middlewareStack.add(
          (next) => async (args) => {
            console.log('You will never see this');
            const result = await next(args);
            // result.response contains data returned from next middleware.
            return result;
          },
          {
            step: 'build',
            name: 'addFooMetadataMiddleware',
            tags: ['METADATA', 'FOO'],
          }
        );
      const dynamoDocClient = await client.send(cmd);
      // 

Observed behavior

The expected console output "You will never see this", is never shown.

Expected behavior

when the command is sent "You will never see this" should be output to the console.

Additional context

It appears the commands in lib-dynamodb are missing the step to concat the current middleware with the newly constructed command. Only the client middlewares get resolved. See comments in the code below.

link to relevant code:

const handler = command.resolveMiddleware(clientStack, configuration, options);

resolveMiddleware(clientStack, configuration, options) {
        const { marshallOptions, unmarshallOptions } = configuration.translateConfig || {};
        const command = new client_dynamodb_1.QueryCommand(utils_1.marshallInput(this.input, this.inputKeyNodes, marshallOptions));
       // need to concat the current middlewareStack with the constructed client.
        const stack = clientStack.concat(this.middlewareStack);
       // resolve the newly combined stack
        const handler = command.resolveMiddleware(stack, configuration, options);
        return async () => {
            const data = await handler(command);
            return {
                ...data,
                output: utils_1.unmarshallOutput(data.output, this.outputKeyNodes, unmarshallOptions),
            };
        };
    }
@c0pp3rt0p c0pp3rt0p added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 14, 2021
@c0pp3rt0p c0pp3rt0p changed the title MiddlewareStacks add to commands in lib-dynamodb never get applied. MiddlewareStacks added to commands in lib-dynamodb never get applied. Dec 14, 2021
@vudh1 vudh1 self-assigned this Dec 16, 2021
@vudh1 vudh1 removed their assignment Mar 15, 2022
@yenfryherrerafeliz yenfryherrerafeliz self-assigned this Jun 17, 2022
@yenfryherrerafeliz
Copy link
Contributor

Hi @c0pp3rt0p, thanks for opening this issue. After looking into this, I found that the problem is caused because when the QueryCommand instance that you have created, which implementation is defined in lib-dynamodb package, create a new instance of the QueryCommand, but of the implementation that is defined in client-dynamodb package, and this new instance is used as the final command to be executed, but just the inputs you provided are given to this new command instance. This means that the middleware stack from the first command instance is ignored.
We can see what I explained above in the following line.

I will mark this issue to be reviewed so we can address this further.

Here is the code that I used to reproduce:

import {DynamoDBDocumentClient, QueryCommand} from "@aws-sdk/lib-dynamodb";
import {DynamoDBClient} from "@aws-sdk/client-dynamodb";

const client = DynamoDBDocumentClient.from(new DynamoDBClient({
    region: 'REGION',
}));
const queryCommandParams = {
    TableName: "test-table",
    KeyConditionExpression: "id = :id",
    ExpressionAttributeValues: {
        ':id': 'id',
    },
};
const queryCommand = new QueryCommand(queryCommandParams);
queryCommand.middlewareStack.add((next) => async (args) => {
        console.log('We should see this output!');
        // result.response contains data returned from next middleware.
        return await next(args);
    },
    {
        step: 'build',
        name: 'addFooMetadataMiddleware',
        tags: ['METADATA', 'FOO'],
    }
);
const response = await client.send(queryCommand);

console.log('Response: ', response);

/**
 * Suggestion:
 *  Since middlewareStack is a readonly variable, we can not override the middlewareStack of the new command instance
 *  with the middlewareStack of the first command instance created. We could maybe create an array in
 *  QueryCommand of lib-dynamodb package, which will hold all the middleware functions provided, and then we
 *  just need to add those functions to the middlewareStack of the final command to be executed. For example:
 *      
 *   New function:
 *      public addToMiddlewareStack(middleware) {
 *          this.middlewareStackItems.push(middleware);
 *      }
 *      
 *   And in the resolveMiddleware function:
 *      const { marshallOptions, unmarshallOptions } = configuration.translateConfig || {};
 *      const command = new __QueryCommand(marshallInput(this.input, this.inputKeyNodes, marshallOptions));
 *
 *      for (let item of this.middlewareStackItems) {
 *          command.middlewareStack.add(item);
 *      }
 *      const handler = command.resolveMiddleware(clientStack, configuration, options);
 *      
 *   And how to use it:
 *      const queryCommand = new QueryCommand(queryCommandParams);
 *      queryCommand.addToMiddlewareStack((next) => async (args) => {
 *              console.log('We should see this output!');
 *              // result.response contains data returned from next middleware.
 *              return await next(args);
 *          },
 *          {
 *              step: 'build',
 *              name: 'addFooMetadataMiddleware',
 *              tags: ['METADATA', 'FOO'],
 *          }
 *      );
 *      
 */

Thanks!

@yenfryherrerafeliz yenfryherrerafeliz added needs-review This issue/pr needs review from an internal developer. and removed needs-triage This issue or PR still needs to be triaged. labels Jun 20, 2022
@yenfryherrerafeliz
Copy link
Contributor

@c0pp3rt0p As workaround, you could add the middleware to the client instead. Please see my code below:

import {DynamoDBDocumentClient, QueryCommand} from "@aws-sdk/lib-dynamodb";
import {DynamoDBClient} from "@aws-sdk/client-dynamodb";

const client = DynamoDBDocumentClient.from(new DynamoDBClient({
    region: 'us-east-2',
}));
client.middlewareStack.add((next) => async (args) => {
        console.log('We should see this output!');
        // result.response contains data returned from next middleware.
        return await next(args);
    },
    {
        step: 'build',
        name: 'addFooMetadataMiddleware',
        tags: ['METADATA', 'FOO'],
    }
);
const queryCommandParams = {
    TableName: "test-table",
    KeyConditionExpression: "id = :id",
    ExpressionAttributeValues: {
        ':id': 'id',
    },
};
const queryCommand = new QueryCommand(queryCommandParams);
const response = await client.send(queryCommand);

console.log('Response: ', response);

Here is the output:

We should see this output!
Response:  {
  '$metadata': {
    httpStatusCode: 200,
    requestId: 'REQUEST-ID',
    extendedRequestId: undefined,
    cfId: undefined,
    attempts: 1,
    totalRetryDelay: 0
  };

@trivikr trivikr added the p1 This is a high priority issue label Jun 20, 2022
@kuhe kuhe self-assigned this Jun 27, 2022
@kuhe kuhe removed the needs-review This issue/pr needs review from an internal developer. label Jun 27, 2022
@kuhe kuhe added the pending-release This issue will be fixed by an approved PR that hasn't been released yet. label Aug 1, 2022
@kuhe
Copy link
Contributor

kuhe commented Aug 1, 2022

This release contains a fix that should make command middlewareStacks useable: https://github.com/aws/aws-sdk-js-v3/releases/tag/v3.141.0

It applies to this package: https://www.npmjs.com/package/@aws-sdk/lib-dynamodb/v/3.141.0

@kuhe kuhe closed this as completed Aug 1, 2022
@kuhe kuhe reopened this Aug 1, 2022
@kuhe kuhe added closing-soon This issue will automatically close in 4 days unless further comments are made. and removed pending-release This issue will be fixed by an approved PR that hasn't been released yet. labels Aug 1, 2022
@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 Aug 6, 2022
@github-actions github-actions bot closed this as completed Aug 6, 2022
@github-actions
Copy link

github-actions bot commented Sep 3, 2022

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue is a bug. closed-for-staleness p1 This is a high priority issue
Projects
None yet
Development

No branches or pull requests

5 participants