suggestion: Refactor Library Encoder/Decoder to use github.com/aws/aws-sdk-go/service/dynamodb/dynamodbattribute

[tamccall]

Aws sdk has their own implementation of the struct tags contained in this library suggest refactoring this library to make use of their struct tags to stay more inline with the official release

See:
https://godoc.org/github.com/aws/aws-sdk-go/service/dynamodb/dynamodbattribute

[mattatcha]

@KingofSouth I currently have a fork which I am refactoring to use dynamodbattribute. I plan on submitting a PR when it is finished.

https://github.com/mattaitchison/dynamo

[guregu]

I think it's a great idea to support the official dynamodbav struct tags.

The reason this library has a custom implementation is because that package simply didn't exist when I made this. There was no other way to easily (un)marshal DynamoDB AV stuff. Later they added an extremely limited version, essentially a thin wrapper around json.Marshal along with its weird inconsistencies, providing no way to add sets, turning []byte into string instead of binary typed values, etc., so I stopped paying attention to it. I hadn't looked at until now but it seems like they've addressed most of those shortcomings. I need to take a closer look but looking at the tests in @MattAitchison's fork it seems like it's basically equivalent, which is great. Thank you for putting in the work.

This library might have some philosophical differences compared to the official one, though. For example, you cannot set a DynamoDB attribute to an empty string. It'll give you an error if you try. Empty strings are super common in Go so this library automatically omits them instead of erroring out. There are a number of types where this library will automatically omit them instead of producing invalid input. Ultimately they are arbitrary decisions I made a long time ago to help me avoid common mistakes.
On the other hand, the official library has its own arbitrary decisions on how to avoid invalid encodings. It will turn an empty string into null:true by default. This "poisons" your data with columns that can either be of the type null or the type string. It's much easier, IMO, to omit it and use the attribute_exists() functions rather than a kludgy type comparison. I'm not sure how it handles other invalid encodings. It also seems we have different ideas of what 'empty' should mean and they have a confusingly separate omitempty for certain types. I think the official package's decisions are a lot less arbitrary than mine, but this package was intended to be more like an opinionated ORM than an ultra-neutral do-anything encoder.

I think our options are:

• support both encodings in this library, either via options or via reflection
• maintain a more 'modern' fork that uses the AWS encodings only. You have a good opportunity to update the methods to refer to hash keys as partition keys and what not. There's a lot of things that could be improved.

Apologies for the wall of text, but I hope it helps explain why I'm not immediately pulling the plug on all of my crappy encoding code. There's a lot of real world software I don't want to break by suddenly dropping support for the dynamo tag, but I am interested in supporting the official way to marshal things. Keep in mind this library is over 2 years old, made back when the AWS libraries were unstable and breaking all the time. I've tried to keep breakages in this library over time relatively minor compared to it (despite the scary warning in the README).

[tamccall]

one other thought that I had thought about encoding that I have had on this library is to add a Marshaller interface that the table uses basically it would define:

These to methods in the interface:

• https://github.com/guregu/dynamo/blob/master/encode.go#L21-L23
• https://github.com/guregu/dynamo/blob/master/decode.go#L20-L22

Then you could have two implementations of that interface one with all of the marshaling code that exists and one that uses the aws default.

Would also allow clients to make a single marshaller for handling things like for instance encryption and use it with every struct that they persist. That way every struct in the package doesn't have some boilerplate MarshalDynamo method.

Just an idea for a potential alternate approach

[guregu]

@tamccall I've implemented something like your idea in #34! Thanks!