Skip to content

Conversation

@ereyes01
Copy link
Contributor

(i.e. a time.Time)
#301

Please review, expert input is needed :-)

@dancannon
Copy link
Collaborator

Thanks, I had a look over this code and your investigations helped me get a bit of a better idea about what is going on!

I dont particularly like adding more special cases but as you mentioned time.Time already is a special case and I think this is the only solution so good job :) Regarding the failing test this is just likely just because Travis is running RethinkDB 2.3, I will test locally and merge. I will also try to look into the decoding step.

@ereyes01
Copy link
Contributor Author

Yeah these test failures smell like something else unrelated to what I changed. FWIW the tests in encoding all pass locally on my end.

@dancannon
Copy link
Collaborator

Regarding the tests if you rebase then they should start passing.

@ereyes01
Copy link
Contributor Author

Anything else needed to merge this?

@dancannon
Copy link
Collaborator

Hey, after thinking about this PR a bit more I am not sure if it should be merged if the driver does not support decoding back into the same data structure. If you have a use case where just the encoding step is important to you let me know, otherwise I think I will wait to merge this.

@ereyes01
Copy link
Contributor Author

ereyes01 commented Apr 16, 2016

Sorry, I don't think I clearly explained what works and what doesn't in the original analysis of this fix in #301...

So the following works with this patch:

  • encoding struct with embedded anonymous time.Time into rethinkDB
  • decoding struct with embedded anonymous time.Time when retrieving from rethinkDB
  • encoding struct with embedded anonymous int into rethinkDB (no change, unaffected by this patch)

The following does not work with this patch:

  • decoding struct with embedded anonymous int when retrieving from rethinkDB (no change, unaffected by this patch)

I don't really care about the anonymous int case, it was just a curious thing I discovered while investigating my original problem with the anon time.Time, which would be useful to me. I briefly thought they had the same root cause, but they don't. The decoding step clearly doesn't work for the struct-with-anon-int case, but somehow it does work for the struct-with-anon-time case.

Below is a sample program that illustrates the behavior with this patch in place:

package main

import (
    "log"
    "time"

    r "github.com/dancannon/gorethink"
)

type TimeHolder struct {
    time.Time
}

type ANumber struct {
    int
}

type Post struct {
    Title string
    When  TimeHolder
    Value ANumber
}

func main() {
    p := Post{
        Title: "A test",
        When:  TimeHolder{time.Now()},
        Value: ANumber{100},
    }

    log.Printf("%+v\n", p)

    session, err := r.Connect(r.ConnectOpts{
        Address:  "localhost:28015",
        Database: "test",
    })
    if err != nil {
        log.Fatal(err)
    }

    response, err := r.DB("test").Table("time_test").Delete().RunWrite(session)
    if err != nil {
        log.Fatal(err)
    }

    log.Printf("Deleted: %d\n", response.Deleted)

    _, err = r.DB("test").Table("time_test").Insert(p).RunWrite(session)
    if err != nil {
        log.Fatal(err)
    }

    cursor, err := r.DB("test").Table("time_test").Run(session)
    if err != nil {
        log.Fatal(err)
    }

    if err := cursor.One(&p); err != nil {
        log.Fatal(err)
    }

    log.Printf("%+v\n", p)
}

Output:

2016/04/16 14:55:59 {Title:A test When:2016-04-16 14:55:59.149404516 -0500 CDT Value:{int:100}}
2016/04/16 14:55:59 Deleted: 1
2016/04/16 14:55:59 {Title:A test When:2016-04-16 14:55:59.149 -0500 -05:00 Value:{int:0}}

Thanks again for reviewing, appreciate the help!

Eddy

dancannon added a commit that referenced this pull request Apr 18, 2016
Do not attempt to traverse the fields of an anonymous pseudo-type
@dancannon dancannon merged commit 804c10a into rethinkdb:master Apr 18, 2016
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.

2 participants