[DynamoDB] Support Document Types

[cjwebb]

I have only been using goamz for a few days, but it does not appear to support DynamoDB's document types List and Map. Have I missed something? Is this planned for the future?

Even if it isn't planned, help would be appreciated, as due to the nature of the Attribute struct, I'm not entirely sure how to add it without making breaking changes...

[alimoeeny]

@cjwebb Personally I only use String and convert or unmarshal everything back and forth manually. I know this is a pain, but that is the way I do it.
There has been recent changes in the types that dynamodb accepts (I read something to the effect that basically you have full json support?) but goamz has not caught up yet.
Do you have any suggestions?

[cjwebb]

@alimoeeny thanks for the quick reply. You are indeed correct that dynamodb now basically has full json support.

As mentioned, I'm not sure how I'd go about adding this to goamz without breaking backwards compatibility, so sadly have no suggestions at the moment :( Using String and manually converting is a good suggestion for now though, thanks.

[budadcabrion]

@alimoeeny Can you clarify what you mean when you say you "use String and convert or unmarshal everything back and forth manually"? I have the same issue as @cjwebb - I want to use List and Map and the functionality is not provided.

[alimoeeny]

@budadcabrion What I do is treat everything as string. Which means if I want to store something that is not a string, like a struct, I would use json.Marshal and convert the struct to a string and save that string to dynamodb. For floats and ints and similar, just parse them to string.
I understand that this is painful, and need a great deal of care and you need to keep track of what is what and compiler will not be able to help you if you make a mistake in types but that is the way I started doing it in the old days and stuck with it.
Did I answer your question?

[tgross]

Which means if I want to store something that is not a string, like a struct, I would use json.Marshal and convert the struct to a string and save that string to dynamodb.

If I'm understanding you correctly, the problem with this approach is that you can't update a nested part of the document without reading it back out, updating it, and writing back. Which means we're not really getting all the document features that the API supports. Ideally the DynamoDB semantics for maps could be supported via code that looks like:

q := NewQuery(myTable)
ex := Expression{
    Text: "SET #my.#mapkey = :mapval",
    AttributeNames: map[string]string {
        "#my": "my_document_column",
        "#mapkey": "my_field_name",
    },
    // this doesn't work
    AttributeValues: map[string]*Attribute{
        ":mapval": DynamoAttribute{S: someStringValue},
    },
}
q.AddUpdateExpression(ex)

As far as I can tell we can't currently use the dynamizer package together with the dynamodb.Table.UpdateItem call at all because the dynamodb.Expression type requires []Attribute as the value for AttributeValues. (And partially because the UpdateItem API endpoint isn't implemented!) And the query/scan API won't read out map types at all. Making AttributeValues support map[string]*DynamoAttribute would be a pretty serious breaking change, so if this were to be supported the obvious path would be to extend the sort-of-unfinished dynamodb.DynamoQuery.

I'm working on a project over at DramaFever where this limitation is biting me. I spent a good bit of time looking at the work @cbroglie did in #276 and I've just run into the discussion going on in a related ticket in aws/aws-sdk-go#72. Is AdRoll going to continue to support this project given that there's an official SDK library?

[alimoeeny]

This would be a question for @crowdmatt and @dialtone

[tgross]

This would be a question for @crowdmatt and @dialtone

Ok, and thanks for making that @-mention for me. I also should say that I didn't mean to say "AdRoll should do this" but rather that I'm happy to contribute an extension of DynamoQuery as discussed above if this project is going to continue.

[alimoeeny]

@tgross , as far as I am concerned (I have zero decision making power here), there are many many people who are already using goamz and will be happy to keep using it as long as contributors like you keep making things better and solving issues.
So on behalf of all the other users of goamz I think I can say: "please, by all means, send your solution in", if possible without breaking the old api/functionality.
Someday, we may all sit down and come up with a road map to rewrite things and have a v2 without backward compatibility. Or maybe the "official" SDK will be good for everybody and there will be no need for a v2.

[cbroglie]

@tgross @cjwebb I previously added the PutDocument/GetDocument functions to support this without breaking the API.

I had planned to add a corresponding UpdateDocument function, but haven't yet needed it. I think it would be fairly straight forward to implement using the dynamizer stuff so long as your structs use the omitempty tag.

[tgross]

Ok, I'll take a crack at UpdateDocument.

It looks like Query/Scan APIs don't return the values of map attributes either, so I'll need to provide a QueryDocument and ScanDocument or tweak the internals of Query/Scan. In some very rough preliminary work I found that they were just being ignored, so maybe it's just a matter of deserialization. I'll investigate.

Also, unless I'm mistaken PutDocument/GetDocument don't support conditional expressions or expression attribute name/values. Given the existing goamz API convention for conditional expressions, it looks like we'll need a ConditionExpressionPutDocument, ConditionExpressionUpdateDocument, and ConditionExpressionDeleteDocument. I'll toss those in too.

I painted myself into a corner in my project with this requirement, so I'll almost certainly have something for you in the next couple days. I'll probably open a PR for discussion before it's entirely complete just to make sure I'm not going totally off the rails.

[cbroglie]

That approach sounds reasonable to me. I'm not a maintainer, but I'd be happy to review what you come up with.

[tgross]

Just wanted to follow up on this.

The bad news is that it looks like I can't get the existing API for Query, Scan, etc. to support the same dynamizer semantics as PutDocument/GetDocument. The problem I'm running into is in using FromDynamo with a collection of results because of the way it uses an untyped out-parameter (i.e. interface{}) instead of a return value. I'll admit it's entirely possible that I'm just not clever enough to figure out how to make it work. But as far as I can tell to get full support for Document we'd have to add new functions for Document for each and every one of the existing methods on Table.

The good news is that I've got an almost-entirely-working and backwards-compatible way to improve Attribute and some other supporting functions so that all the existing Query, Scan, etc, etc. functions can support the map type. The last bit I have to do is to fix UntypedQuery.AddUpdates so that it can update a field within the map and I'll be good-to-go.

I'll have a PR for the latter work within 24 hours (including lots of new tests). I honestly like the dynamizer API better from the standpoint of consuming the library, but I can't make it work myself.

[tgross]

Maintainers + @cbroglie I've opened the PR here #339.

Another observation I had associated with this is that the docstrings for the DynamoDB API are a little light. I'd be happy to help improve that as well under a separate issue (if only to help me the next time I have to use this library!)