Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Weird Collect() issues with maps and other nested values #3

Closed
thisishugo opened this Issue · 4 comments

2 participants

@thisishugo

I've been noticing some weird issues when unmarshalling structs with Collect.

I have json data like this saved to my database:

// thing one
{
    "id": "F06F33CD-DB88-4715-A121-72D1C1DD6747",
    "props": {
        "prop1": "a",
        "prop2": "b",
    }
}
// thing two
{
    "id": "C62FD365-9C26-4124-8A33-1AEAD482655D",
    "props": {
        "prop1": "c",
        "prop2": "d",
    }
}
type Thing struct {
    Id string `json:"id,omitempty"`
    Props map[string]map[string]string `json:"images"`
}
var things []Thing
_ := rethink.Table("things").Run(session).Collect(&things)
fmt.Printf("%v\n", things)

When deserialized both structs either see {prop1: "a", prop2: "b"} or {prop1: "c", prop2: "d"} but I can't figure out how to get both to get their own values. This also happens if you do:

var things []Thing
var thing Thing
_, row := rows.Next(&thing) {
    things = append(things, thing)
}

I think the problem comes from the 'thing' struct into which we are deserializing not being cleared on every iteration. The couchgo library has a Remarshal method that might be worth looking at. I think that exists to avoid this problem.

@christopherhesse

Nice, I will check it out this evening.

@christopherhesse

It looks like there are a couple of issues here, mostly due to my unfamiliarity with go's type system.

1) maps are pass-by-reference, so they are copy-by-reference too, when you say things = append(things, thing), the map is not copied, only the reference to the map
2) json.Unmarshal does not clear a struct before writing to it

type Thing struct {
    Id    int
    Props map[string]int
    Attrs []int
}

func main() {
    var thing Thing
    things := []Thing{}
    json.Unmarshal([]byte(`{"id": 1, "props": {"a":1, "b":2, "c": 3}, "attrs": [1,2]}`), &thing)
    things = append(things, thing)
    json.Unmarshal([]byte(`{"id": 2, "props": {"a":4, "b":5, "d": 6}, "attrs": [2,3]}`), &thing)
    things = append(things, thing)

    for _, item := range things {
        fmt.Println("things:", item)
    }
}

Output:

things: {1 map[a:4 d:6 b:5 c:3] [2 3]}
things: {2 map[c:3 a:4 d:6 b:5] [2 3]}

I see at least two solutions:

One: Go's sql module doesn't let you put the destination variable in the call to .Next(), you must call iter.Scan() inside the loop. This was the original design of rethinkgo, and I am not opposed to it, though it seems a little nicer if the two can be combined in one call.

Two: Clear the variable before writing to it. It seems unlikely that someone would want to update a variable's value with rows.Next(&dest) rather than overwrite the whole thing.

couchgo seems to have the destination variable inside the loop, which is option one. mgo uses rows.Next(&dest) and its bson module appears to overwrite structs passed in.

@christopherhesse

I will change .Next(&dest) to .Next() and add .Scan(&dest) in the style of the sql module when I update to the new protocol spec. A workaround for now is to clear the variable each time, in this case:

thing = Thing{}

inside the for loop.

Thanks for the bug report!

@christopherhesse

Should be "fixed" in the current version, since .Scan() is now separate and inside the loop. It's possible with reflect to instantiate a new version of an object on each .Next(&dest) call, but I went with the sql module's method in case that didn't work in all cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.