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

Use AWS SDK DynamoDB Document Client to support nested scan #158

Merged
merged 11 commits into from
Jun 19, 2017

Conversation

lrcry
Copy link
Contributor

@lrcry lrcry commented Jun 4, 2017

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:

    var Dog = dynamoose.model('Dog');
    // this should be composed, ref: http://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_Scan.html
    var filter = {
      FilterExpression: 'details.timeWakeUp = :wakeUp',
      ExpressionAttributeValues: {
        ':wakeUp': '8am'
      }
    };

    Dog.scan(filter).exec()
      .then(function(dogs) {
        console.log(dogs);
      })
      .catch(function(err) {
        console.error(err);
      });

It is not necessary to provide TableName in the filter as the library will use the model name as table name by default, if a TableName 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 the DocumentClient to support more features, although it means an amount of work. :)

Limitations:

  • When defining schemas, please use { 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)
  • It seems that aws-sdk document client has not supported usage of attribute name placeholders yet (when I tried to use the plain aws-sdk in my code, it seemed never to work), which means that something as follows (#timeWakeUp) will get error:
{
  "FilterExpression": "#timeWakeUp = :wakeUp",
  "ExpressionAttributeNames": {
    "#timeWakeUp": "details.timeWakeUp"
  },
  "ExpressionAttributeValues": {
    ":wakeUp": "8am"
  }
}

Instead, use the name directly (details.timeWakeUp):

{
  "FilterExpression": "details.timeWakeUp = :wakeUp",
  "ExpressionAttributeValues": {
    ":wakeUp": "8am"
  }
}

Copy link
Contributor

@brandongoode brandongoode left a 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) {
Copy link
Contributor

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' });
Copy link
Contributor

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

Copy link
Contributor Author

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) {
Copy link
Contributor

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')

@lrcry
Copy link
Contributor Author

lrcry commented Jun 15, 2017

@brandongoode Can you please review my latest change as per the last review you've posted? Thanks a lot.

@brandongoode
Copy link
Contributor

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

@lrcry
Copy link
Contributor Author

lrcry commented Jun 19, 2017

@brandongoode
I've done the document update. Please view it at #167

@brandongoode brandongoode merged commit 8ddf907 into dynamoose:master Jun 19, 2017
@brandongoode brandongoode added this to the v0.8.0 milestone Jun 19, 2017
@ghost ghost mentioned this pull request Aug 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants