sdb compile w/ weekly, bug fixes, features

I updated the sdb code base so it would compile using the latest release version of Go (currently it fails due to changes in Go, specifically moving functions from http to url, such as URL parsing). In addition, I added a function to execute Select statements and retrieve the results.

I branched from ~andychilton/goamz/sdb since he had added some nice functionality to Get and Put attributes

There is one item i'd like to point out for review, which are the Item and Attribute structures. In Andy's implementation, there is no explicit relationship defined between an Item and its Attributes. However, in order to unmarshal the Select response, a relationship must be present. Instead of modifying andy's code, i defined this relationship inline inside the SelectResponse ... may not be the best approach, but for now it works.

https://code.launchpad.net/~brad-rydzewski/goamz/sdb-bug-fixes/+merge/84160

(do not edit description out of merge proposal)

[niemeyer, 2011-12-02 20:34:44]

Thanks a lot for pushing this forward Brad. Sorry if the review is a bit long,
but there's quite a bit in this branch.

In addition to these, can you please merge from the latest revision onto the
branch too?

Also, it'd be good to do a review of the tests to make sure that all fields/etc
are tested, as I haven't done that now, but will have to do on the next pass.

A lot of good stuff, btw, I hope to see this merged soon.

https://codereview.appspot.com/5451067/diff/1/sdb.go
File sdb.go (right):

https://codereview.appspot.com/5451067/diff/1/sdb.go#newcode44
sdb.go:44: // The Item type encapsulates operations that require an item (and
implicitly a Domain and SDB instance).
The documentation here is (and already was before) pretty weak. The developer
will benefit from knowing what an Item _really_ is in the context of SimpleDB.
What is a simple DB item?

We should do the same for Domain above.

https://codereview.appspot.com/5451067/diff/1/sdb.go#newcode57
sdb.go:57: type Attribute struct {
Let's name this as Attr please (and Attrs when plural), to shorten code that is
commonly used everwhere.

https://codereview.appspot.com/5451067/diff/1/sdb.go#newcode62
sdb.go:62: Exists   bool   // Check whether this attribute exists
Only Name and Value are properties of Attr. The rest of them are options for
PutAttribute that have to be handled differently (see the respective comment
below).

There are several reasons for this, but perhaps the most important one is very
pragmatic. This enables you to build attribute lists with something like:

    attrs := []Attr{{"a", "1"}, {"b", "2"}}

https://codereview.appspot.com/5451067/diff/1/sdb.go#newcode76
sdb.go:76: }
Please drop this type. The refactoring you did below is clearly an improvement,
and doesn't need it.

https://codereview.appspot.com/5451067/diff/1/sdb.go#newcode87
sdb.go:87: // ListDomains lists all your domains.
// ListDomains lists all domains in sdb.

https://codereview.appspot.com/5451067/diff/1/sdb.go#newcode94
sdb.go:94: // ListDomainsN lists your domains up to a maximum number specified
and starting at the token provided.
// ListDomainsN lists domains in sdb up to maxDomains.
// If nextToken is not empty, domains listed will start at the given token.

https://codereview.appspot.com/5451067/diff/1/sdb.go#newcode97
sdb.go:97: func (sdb *SDB) ListDomainsN(maxNumberOfDomains int, nextToken
string) (resp *ListDomainsResp, err error) {
s/maxNumberOfDomains/maxDomains/

https://codereview.appspot.com/5451067/diff/1/sdb.go#newcode115
sdb.go:115: type SelectResponse struct {
s/Response/Resp/, as per the existing convention in goamz.

https://codereview.appspot.com/5451067/diff/1/sdb.go#newcode117
sdb.go:117: Item []struct {
s/Item/Items/, use a tag if necessary.

https://codereview.appspot.com/5451067/diff/1/sdb.go#newcode119
sdb.go:119: Attribute []Attribute
s/Attribute/Attrs/ in the field name.

https://codereview.appspot.com/5451067/diff/1/sdb.go#newcode124
sdb.go:124: // Select operation returns a set of Attributes for ItemNames that
match the select expression.
s/Select operation/Select/
s/Attributes/attributes/
s/ItemNames/item names/ (neither of these are actual types)

https://codereview.appspot.com/5451067/diff/1/sdb.go#newcode128
sdb.go:128: func (sdb *SDB) Select(selectExpression string, consistentRead bool)
(resp *SelectResponse, err error) {
s/selectExpression/expr/
s/consistentRead/consistent/

https://codereview.appspot.com/5451067/diff/1/sdb.go#newcode169
sdb.go:169: }
Let's please drop this type.

https://codereview.appspot.com/5451067/diff/1/sdb.go#newcode181
sdb.go:181: func (domain *Domain) DeleteDomain(opt *DeleteDomainOpt) (resp
*DeleteDomainResp, err error) {
Please drop the argument. It's not really doing anything there (I know it was
like that before already).

https://codereview.appspot.com/5451067/diff/1/sdb.go#newcode197
sdb.go:197: attributeNameSizeBytes   int
Why is everything private here? It's of no use to provide the user with a type
that can't be touched in any way. Please note we need tests to all of those
fields as well. Feel free to just drop if it's not useful right now and you
don't have the time, though.

https://codereview.appspot.com/5451067/diff/1/sdb.go#newcode211
sdb.go:211: func (domain *Domain) DomainMetadata() (resp *DomainMetadataResp,
err error) {
s/DomainMetadata()/Metadata()/ in the method name. This will be used as
domain.Metadata()

https://codereview.appspot.com/5451067/diff/1/sdb.go#newcode231
sdb.go:231: // PutAttributes puts a number of attributes into a value.
// PutAttrs adds attrs to item.

https://codereview.appspot.com/5451067/diff/1/sdb.go#newcode234
sdb.go:234: func (item *Item) PutAttributes(attrs []Attribute) (resp
*PutAttributesResp, err error) {
s/PutAttributes/PutAttrs/

Then, let's please use this type as the argument of PutAttrs

type PutAttrs struct {
    Attrs []Attr // The attributes to put

    expected []Attr
    replace map[string]bool
    missing map[string]bool
}

func (pa *PutAttrs) Add(name, value string) {
    pa.Attrs = append(pa.Attrs, Attr{Name, Value})
}

func (pa *PutAttrs) Replace(name, value string) {
    pa.Add(name, value)
    pa.replace[name] = true
}

func (pa *PutAttrs) IfValue(name, value string) {
    // This should set Expected.N.Name=name and Expected.N.Value=value
    pa.expected = append(pa.expected, Attr{Name, Value})
}

func (pa *PutAttrs) IfMissing(name string) {
    // This should set Expected.N.Name=name and Expected.N.Exists=false
    pa.missing[name] = true
}


Then, the function is defined as:

func (item *Item) PutAttrs(attrs *PutAttrs) (resp *PutAttrsResp, err error)

https://codereview.appspot.com/5451067/diff/1/sdb.go#newcode274
sdb.go:274: type GetAttributesResp struct {
s/Attribute/Attr/ here and in the several instances below

[niemeyer, 2011-12-13 17:17:38]

Hi Brad,

I hope you're not discouraged by the review. Is there anything I can do to help
out?

[Brad.Rydzewski, 2011-12-13 17:47:11]

On 2011/12/13 17:17:38, niemeyer wrote:
> Hi Brad,
> 
> I hope you're not discouraged by the review. Is there anything I can do to
help
> out?

Hey Gustavo, not discouraged at all by the amount of changes, just had to focus
on some other areas of my project. Hopefully I will get to spend some time this
week updating the code based on your feedback.

Would it be ok if we split up this patch so I don't have to fix everything at
once? That might help us get some of the changes into trunk sooner, especially
the changes to the signature, which currently doesn't work against live simpledb
(even though it passes unit tests)

[gustavo_niemeyer.net, 2011-12-13 17:58:51]

> Hey Gustavo, not discouraged at all by the amount of changes, just had
> to focus on some other areas of my project. Hopefully I will get to
> spend some time this week updating the code based on your feedback.

Ah, of course, please take your time.

> Would it be ok if we split up this patch so I don't have to fix
> everything at once? That might help us get some of the changes into
> trunk sooner, especially the changes to the signature, which currently
> doesn't work against live simpledb (even though it passes unit tests)

Absolutely, that makes sense. It may make sense to keep this branch
review untouched to serve as a guideline for the changes, and then
gradually create new smaller branches and propose them, so that we can
get them in quickly.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[niemeyer, 2012-02-09 13:50:06]

Brad, how is this going? Is there anything I can do to help getting this in?

[Brad.Rydzewski, 2012-02-09 17:07:30]

On 2012/02/09 13:50:06, niemeyer wrote:
> Brad, how is this going? Is there anything I can do to help getting this in?

hey Gustavo i'm on it. Be ready for a number of commits over the next couple of
days

[niemeyer, 2012-03-09 11:00:23]

This was already integrated via a different branch.