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

Update dynamodb to use the latest API version #209

Merged
merged 2 commits into from
Jan 4, 2016
Merged

Update dynamodb to use the latest API version #209

merged 2 commits into from
Jan 4, 2016

Conversation

dmathieu
Copy link
Contributor

The 20111205 API version is deprecated, and doesn't provide many of the features the latest version offers, such as the conditional parameters.

@dmathieu
Copy link
Contributor Author

Note that this includes several breaking changes. So we might want to release a major version with it, or change it to support both the deprecated and new versions.

#
# See DynamoDB Documentation: http://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_Query.html
#
def query(table_name, options = {})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super familiar with the new stuff here, but is there any way we could take either format here? ie if hash_key is supplied, warn about deprecation but then merge it appropriately into the options and continue? If so, would be great to avoid the backwards-incompatibility here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous API required a HashKeyValue argument, which contained: {:S => 1}.
See http://docs.aws.amazon.com/amazondynamodb/latest/developerguide/API_Query_v20111205.html

DynamoDB would take the id and compare the provided value with it.
The new API requires to do:

:KeyConditionExpression => 'id = :id'

And unless the DynamoDB documentation is wrong, it doesn't appear HashKeyValue is still supported.
Transforming the former hash to the new value seems difficult, as we don't know the name of the primary key.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Maybe we should still catch that case to provide a more explicit/specific error message, even if we just have to bail out at that point. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that works for me. I'll do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated this to show a deprecation notice and fallback to the former api version.

@geemus
Copy link
Member

geemus commented Jan 4, 2016

Thanks, looking good overall.

# See DynamoDB Documentation: http://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_Query.html
#
def query(table_name, options = {}, hash_key_deprecated = nil)
if has_deprecated_hash_key || (options.keys.length == 1 && [:S, :N, :B].include?(options.keys.first.to_sym))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, maybe I'm just missing something, but where is has_deprecated_hash_key defined? Seems like maybe we could just drop that part of the conditional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a method argument. We need it because the method signature changes, as DynamoDB doesn't have any HashKey option anymore. We need to do a string KeyConditionExpression instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. But in that case wouldn't it be if hash_key_deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're absolutely right. I've fixed it. Thanks you.

@geemus
Copy link
Member

geemus commented Jan 4, 2016

Thanks!

geemus added a commit that referenced this pull request Jan 4, 2016
Update dynamodb to use the latest API version
@geemus geemus merged commit dde088c into fog:master Jan 4, 2016
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