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

Can't reuse QueryCommand in loop #1864

Closed
clintfoster opened this issue Jan 4, 2021 · 7 comments · Fixed by smithy-lang/smithy-typescript#259 or #1883
Closed

Can't reuse QueryCommand in loop #1864

clintfoster opened this issue Jan 4, 2021 · 7 comments · Fixed by smithy-lang/smithy-typescript#259 or #1883
Assignees
Labels
bug This issue is a bug.

Comments

@clintfoster
Copy link

clintfoster commented Jan 4, 2021

Describe the bug
This is either a bug or a documentation / usability issue. When attempting to reuse a Dynamo QueryCommand in a typical paging loop I get a "Duplicate middleware" exception:

Loop iteration: 1
Loop iteration: 2
12-05T17:40:45, t2: 2021-01-04T17:40:45 Error: Duplicate middleware name 'deserializerMiddleware'
    at Object.add (MiddlewareStack.js:122)
    at Object.applyToStack (serdePlugin.js:16)
    at Object.use (MiddlewareStack.js:141)
    at QueryCommand.resolveMiddleware (QueryCommand.js:80)
    at DynamoDBClient.Client.send (client.js:10)

SDK version number
3.1.0

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

Details of the browser/Node.js/ReactNative version
Paste output of npx envinfo --browsers

Chrome: 87.0.4280.88

To Reproduce (observed behavior)
Below is a typical loop to scan the sort key of an item with partition key fooId = 123:

const dynamoClient = new DynamoDBClient();
const qParams: Record<string, any> = {
  TableName: 'FooData',
  KeyConditionExpression: 'fooId = :fid',
  ExpressionAttributeValues: {':fid': {N: '123'}}
};

let qCmd = new QueryCommand(qParams);  // shouldn't it be possible for this to be const?
let i = 0;
do {
  console.log('Loop iteration:', ++i);
  // qCmd = new QueryCommand(qParams);  // enabling this line avoids the problem
  const qOutput = await dynamoClient.send(qCmd) as QueryOutput;
  qParams.ExclusiveStartKey = qOutput.LastEvaluatedKey;
} while(qParams.ExclusiveStartKey)

My specific question is why QueryCommand must be recreated each time through the loop to avoid the error. Wouldn't it be natural to reuse it, changing only the ExclusiveStartKey as needed?

I guess this is kind of a nit, and what I'm really looking for is the most efficient and idiomatic way to implement a standard paging loop in the v3 API. An example in the documentation would go a long way.

Expected behavior
It should be possible to express a simple paged query (or scan) without inefficient or confusing constructs.

@clintfoster clintfoster added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 4, 2021
@trivikr
Copy link
Member

trivikr commented Jan 5, 2021

AWS SDK for JavaScript v3 supports Pagination using Async Iterators.

Can you try paginateQuery operation?

@trivikr trivikr added the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Jan 5, 2021
@AllanZhengYP AllanZhengYP self-assigned this Jan 5, 2021
@AllanZhengYP
Copy link
Contributor

The root cause for this issue seems to be duplicated with #1857. We will look further into this.

@AllanZhengYP AllanZhengYP added duplicate This issue is a duplicate. and removed needs-triage This issue or PR still needs to be triaged. labels Jan 5, 2021
@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Jan 6, 2021
@AllanZhengYP
Copy link
Contributor

Hi @clintfoster,

Just like it mentioned by @trivikr, you can still work around it by using Pagination using Async Iterators. The SDK handles making consecutive requests for you.

@clintfoster
Copy link
Author

clintfoster commented Jan 6, 2021

@trivikr @AllanZhengYP Thanks for the pointer to async iterators. I tried a trivial paged query, and it was clean and simple. However, I need the ability to exit the pagination loop and later re-enter it where I left off. I think this should be possible by saving QueryOutput.LastEvaluatedKey at the point where I exit the loop. When I need to restart I can set PaginationConfiguration.startingToken and run paginateQuery() again. However, startingToken is a string and LastEvaluatedKey is of type [key: string]: AttributeValue. No doubt there is a way to convert it to the needed string format. But overall the code was getting more complicated than simply using ExclusiveStartKey and LastEvaluatedKey in a less magical do/while loop.

As a temporary workaround I will stick with creating a new QueryCommand each time through the loop unless you advise switching to async iterators for other reasons.

@AllanZhengYP
Copy link
Contributor

AllanZhengYP commented Jan 6, 2021

Hey @clintfoster,

However, startingToken is a string and LastEvaluatedKey is of type [key: string]: AttributeValue. No doubt there is a way to convert it to the needed string format.

Looks like the startingToken is an any type for now. In fact we can make it more specific using generic. Looking at the implementation, you are able to just supply LastEvaluatedKey to startingToken or input. ExclusiveStartKey.

But overall the code was getting more complicated than simply using ExclusiveStartKey and LastEvaluatedKey in a less magical do/while loop.

You can exist the async iterator and resume it easily in my opinion too, It would look like:

  const iter = paginateScan({client: dynamoClient}, params);
  let res: ScanCommandOutput
  let isEnd = false;
  while(isEnd) {
    const {value, done} = await iter.next();
    isEnd = done;
    res = value;
    if (value.Items[0] === {} /*when you want to exit the pagination*/) {
      break; //exit
    }
  }
  //do something
  //resume paginating; using for loop this time.
  for await (const result of paginateScan(
    {client: dynamoClient},
    {...qParams, ExclusiveStartKey: res.LastEvaluatedKey}
  )) {
    //do something
  }

@AllanZhengYP
Copy link
Contributor

I opened a feature request for adding more types in paginator.

@AllanZhengYP AllanZhengYP removed the duplicate This issue is a duplicate. label Jan 7, 2021
AllanZhengYP added a commit to AllanZhengYP/smithy-typescript that referenced this issue Jan 7, 2021
When sending the same command multiple times, resolveMiddleware()
will be called many times. So adding serde middleware will throw
error because they are already added into stack. This change
prevents adding the serde middleware repeatedly.

Alternative is moving command middleware to the command constructor,
just like in client(that's why client doesn't have the problem). But
the command middleware also have depdency over client configs
supplied from resolveMiddleware(). So serde middleware and
customizations must live here.

ref: aws/aws-sdk-js-v3#1864
AllanZhengYP added a commit to AllanZhengYP/aws-sdk-js-v3 that referenced this issue Jan 7, 2021
When sending the same command multiple times, resolveMiddleware()
will be called many times. So adding serde middleware will throw
error because they are already added into stack. This change
prevents adding the serde middleware repeatedly.

Alternative is moving command middleware to the command constructor,
just like in client(that's why client doesn't have the problem). But
the command middleware also have depdency over client configs
supplied from resolveMiddleware(). So serde middleware and
customizations must live here.

ref: aws#1864
AllanZhengYP added a commit to smithy-lang/smithy-typescript that referenced this issue Jan 8, 2021
When sending the same command multiple times, resolveMiddleware()
will be called many times. So adding serde middleware will throw
error because they are already added into stack. This change
prevents adding the serde middleware repeatedly.

Alternative is moving command middleware to the command constructor,
just like in client(that's why client doesn't have the problem). But
the command middleware also have depdency over client configs
supplied from resolveMiddleware(). So serde middleware and
customizations must live here.

ref: aws/aws-sdk-js-v3#1864
AllanZhengYP added a commit that referenced this issue Jan 8, 2021
When sending the same command multiple times, resolveMiddleware()
will be called many times. So adding serde middleware will throw
error because they are already added into stack. This change
prevents adding the serde middleware repeatedly.

Alternative is moving command middleware to the command constructor,
just like in client(that's why client doesn't have the problem). But
the command middleware also have depdency over client configs
supplied from resolveMiddleware(). So serde middleware and
customizations must live here.

ref: #1864
@github-actions
Copy link

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 Jan 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue is a bug.
Projects
None yet
3 participants