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

Document support #276

Merged
merged 18 commits into from Dec 6, 2014
Merged

Document support #276

merged 18 commits into from Dec 6, 2014

Conversation

cbroglie
Copy link
Contributor

This change adds an API for storing and fetching plain JSON documents (either struct or map[string]interface{} objects), fully abstracting away the details of how DynamoDB represents attribute values.

As of now it only implements the PUT, GET, and DELETE operations, and has undergone only limited testing aside from unit tests. Using typed structs also incurs an extra JSON encode/decode, so performance optimizations may be necessary.

I wouldn't consider this ready to merge, but I'm proposing it now to get some feedback. Is there interest in such a feature?

Chris Broglie added 15 commits November 16, 2014 23:20
This is a prerequisite for typed Query objects
These functions convert in memory objects to and from DyanomDB
attribute value objects.

Reference: http://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_AttributeValue.html
It wasn't testing what we wanted to test, since it was serializing the
data into JSON before comparing. Using DeepEqual directly on the
structs validates that they do have the expected types.
This is required for the typed query object, since structs cannot
contain the members and functions with the same name.
Changes:
- Store the table so it's not required to be passed to the query
builder functions
- Invoke ToDynamo/FromDynamo outside of the query builder since it's
not part of the read path
- Some WIP changes for GetDocument which were hard to separate (I'm
being lazy)
@alimoeeny
Copy link
Contributor

@cbroglie I personally have been using a similar approach for my personal projects (except that I do map[string]string to simplify life even more and put the burden of conversion on the user.
Let's wait a bit and see what other people have to say.
Thanks for sharing.

@cbroglie
Copy link
Contributor Author

cbroglie commented Dec 4, 2014

@alimoeeny These changes have proven stable for us. What are your thoughts on merging to master?

If you're ok to merge, I can update the README with some examples.

@cbroglie
Copy link
Contributor Author

cbroglie commented Dec 4, 2014

I'll merge the latest master into my fork as well first, too.

@alimoeeny
Copy link
Contributor

Thanks @cbroglie is it possible to do a new pull request? I think since there has been other changes since you started the pull request now it cannot be automatically merged.
Alternatively I can do a manual merge but then it means there will be some changes that you need to merge back into your branch.

@cbroglie
Copy link
Contributor Author

cbroglie commented Dec 4, 2014

Yep, I'll merge master into my fork and resolve the conflicts.

Conflicts:
	dynamodb/item.go
	dynamodb/query.go
	dynamodb/query_builder.go
@cbroglie
Copy link
Contributor Author

cbroglie commented Dec 5, 2014

@alimoeeny Merged latest into my fork. All tests passing.

alimoeeny added a commit that referenced this pull request Dec 6, 2014
@alimoeeny alimoeeny merged commit 2e3539e into AdRoll:master Dec 6, 2014
@alimoeeny
Copy link
Contributor

Thanks @cbroglie

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.

None yet

2 participants