-
-
Notifications
You must be signed in to change notification settings - Fork 361
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
Use AWS SDK DynamoDB Document Client to support nested scan #158
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lrcry Thanks for the PR -- sorry it took so long for me to review the change.
lib/Scan.js
Outdated
@@ -26,7 +26,12 @@ function Scan (Model, filter, options) { | |||
this.buildState = filter; | |||
this.filters[filter] = {name: filter}; | |||
} else if (typeof filter === 'object'){ | |||
this.parseFilterObject(filter); | |||
if (options && options.useRawAwsFilter) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be easier for other if we just check for FilterExpression in the filter.
if(type filter.FilterExpression === 'string')
lib/Scan.js
Outdated
function scanByRawFilter() { | ||
var deferred = Q.defer(); | ||
var AWS = Model.$__.base.AWS; | ||
AWS.config.update({ endpoint: 'http://localhost:8000' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line should be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Let me check this to see what is going on at this point. I remembered I've already moved the AWS things into the model to keep consistency...
lib/Scan.js
Outdated
return deferred.promise.nodeify(next); | ||
} | ||
|
||
if (options.useRawAwsFilter) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(this.filter && type this.filter.FilterExpression === 'string')
@brandongoode Can you please review my latest change as per the last review you've posted? Thanks a lot. |
Looks great! Could you add a section in the Scan documentation and I'll merge it: https://github.com/automategreen/dynamoose/blob/master/docs/_docs/scan.md |
@brandongoode |
Finally I've got one workaround for now to support nested scan via aws-sdk dynamodb document client...
To use it (as a promise), just compose your own filter ~~~and specify the option {useRawAwsFilter: true}~~~ when scanning:
It is not necessary to provide
TableName
in the filter as the library will use the model name as table name by default, if aTableName
is not explicitly specified.I didn't touch anything which used the default
DynamoDB
client. I think it would be a good idea to modify dynamoose to use theDocumentClient
to support more features, although it means an amount of work. :)Limitations:
{ useDocumentTypes: true }
if you would like to use nested scan as this is only supported for documents (I think it is a feature of DocumentClient, I googled that before but I could not remember the link, sorry for that)#timeWakeUp
) will get error:Instead, use the name directly (
details.timeWakeUp
):