Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed NullTime returns always invalid on sqlite3. #203

Merged
merged 62 commits into from Mar 1, 2015

Conversation

umisama
Copy link
Contributor

@umisama umisama commented Oct 17, 2014

gorp.NullTime is always return invalid if using sqlite3.

example:

type Test struct {
        Id    int           `db:"id"`
        Test  gorp.NullTime `db:"test"`
        TestA time.Time     `db:"test2"`
}

_, err = dbmap.Select(&list, `select * from TEST where id=?`, id)
if err != nil {
        panic(err)
for _, v := range list {
}

// always output false
fmt.Println(v.Test.Valid)

I fixed this probrem with handling []byte in NullTime.Scan(). And, I added tests for NullTime to find environment probrems.

thank you!

coopernurse and others added 15 commits November 18, 2013 17:31
…hen a field marked transient matches a column mapped to another field. Fixes go-gorp#175 but reverses go-gorp#146

TestSelectAlias: modify to demonstrate how this behavior can be achieved using two separate structs - one that reflects the actual table structure (for inserts/updates) and one with the extended field, which can be used for selects.
Adding a table from a struct containing a member with name "Version" and type other than int results in a panic: 
2015/01/27 15:42:57 http: panic serving 127.0.0.1:60027: reflect: call of reflect.Value.Int on string Value
goroutine 23 [running]:
net/http.func·011()
        c:/go/src/pkg/net/http/server.go:1100 +0xbe
runtime.panic(0x7059e0, 0xc082007420)
        c:/go/src/pkg/runtime/panic.c:248 +0x1d3
reflect.Value.Int(0x697be0, 0xc082016ca0, 0x0, 0x186, 0x77e3d0)
        c:/go/src/pkg/reflect/value.go:1053 +0xc3
github.com/coopernurse/gorp.bindPlan.createBindInstance(0xc08205ef00, 0x162, 0xc08201e400, 0x12, 0x20, 0x0, 0x0, 0x0, 0x77e3d0, 0x7, ...)
        C:/Users/Christian/Google Drive/Development/go/src/github.com/coopernurse/gorp/gorp.go:289 +0x177
github.com/coopernurse/gorp.(*TableMap).bindInsert(0xc082020b40, 0x761280, 0xc082016c60, 0x0, 0x196, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
        C:/Users/Christian/Google Drive/Development/go/src/github.com/coopernurse/gorp/gorp.go:394 +0x6e0
github.com/coopernurse/gorp.insert(0xc0820104e0, 0x181da0, 0xc0820104e0, 0x3a3a38, 0x1, 0x1, 0x0, 0x0)
        C:/Users/Christian/Google Drive/Development/go/src/github.com/coopernurse/gorp/gorp.go:1913 +0x2dc
github.com/coopernurse/gorp.(*DbMap).Insert(0xc0820104e0, 0x3a3a38, 0x1, 0x1, 0x0, 0x0)
        C:/Users/Christian/Google Drive/Development/go/src/github.com/coopernurse/gorp/gorp.go:955 +0x97

From my POV, this is convention over configuration, just the wrong way around.

My proposed solution is to disable setting the version ColMap in AddTable(i interface{}), forcing the developer to manually set the version column name by using SetVersionCol(). This is a crude fix, but it shows the problem and how to solve it.
Fixed tests by using SetVersionCol on person tablemap
@benprew
Copy link
Contributor

benprew commented Jan 31, 2015

@umisama Can you fix failing tests for this pull request?

https://travis-ci.org/go-gorp/gorp/jobs/38387723

--- FAIL: TestNullTime (0.01 seconds)
gorp_test.go:1413: NullTime returns invalid but expected valid.
gorp_test.go:1416: expect %v but got %v. 0000-01-02 15:04:05 +0000 UTC 0001-01-01 00:00:00 +0000 UTC

benprew and others added 13 commits January 31, 2015 18:14
Remove maintainers notice (since PR is closed).  Add Help/Support section
Also made the markdown headings consistently use prefixed #'s, no suffix.
Add gopkg.in versioned releases information.
Added warning telling users that automatic mapping of Version struct members to version column in database (optimistic locking) will be removed in V2
Changed log output to stderr
Fixed race condition
@GeertJohan
Copy link
Member

@umisama Please fix the failing tests and rebase this PR on the master branch (we've deprecated develop, sorry).

umisama and others added 20 commits February 19, 2015 02:36
Fix old description about GORP_TEST_DIALECT in README.md
Fix expand query on NullTime or other structs.
Adding a table from a struct containing a member with name "Version" and type other than int results in a panic:
2015/01/27 15:42:57 http: panic serving 127.0.0.1:60027: reflect: call of reflect.Value.Int on string Value
goroutine 23 [running]:
net/http.func·011()
        c:/go/src/pkg/net/http/server.go:1100 +0xbe
runtime.panic(0x7059e0, 0xc082007420)
        c:/go/src/pkg/runtime/panic.c:248 +0x1d3
reflect.Value.Int(0x697be0, 0xc082016ca0, 0x0, 0x186, 0x77e3d0)
        c:/go/src/pkg/reflect/value.go:1053 +0xc3
github.com/coopernurse/gorp.bindPlan.createBindInstance(0xc08205ef00, 0x162, 0xc08201e400, 0x12, 0x20, 0x0, 0x0, 0x0, 0x77e3d0, 0x7, ...)
        C:/Users/Christian/Google Drive/Development/go/src/github.com/coopernurse/gorp/gorp.go:289 +0x177
github.com/coopernurse/gorp.(*TableMap).bindInsert(0xc082020b40, 0x761280, 0xc082016c60, 0x0, 0x196, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
        C:/Users/Christian/Google Drive/Development/go/src/github.com/coopernurse/gorp/gorp.go:394 +0x6e0
github.com/coopernurse/gorp.insert(0xc0820104e0, 0x181da0, 0xc0820104e0, 0x3a3a38, 0x1, 0x1, 0x0, 0x0)
        C:/Users/Christian/Google Drive/Development/go/src/github.com/coopernurse/gorp/gorp.go:1913 +0x2dc
github.com/coopernurse/gorp.(*DbMap).Insert(0xc0820104e0, 0x3a3a38, 0x1, 0x1, 0x0, 0x0)
        C:/Users/Christian/Google Drive/Development/go/src/github.com/coopernurse/gorp/gorp.go:955 +0x97

From my POV, this is convention over configuration, just the wrong way around.

My proposed solution is to disable setting the version ColMap in AddTable(i interface{}), forcing the developer to manually set the version column name by using SetVersionCol(). This is a crude fix, but it shows the problem and how to solve it.

Update gorp_test.go

Fixed tests by using SetVersionCol on person tablemap

Remove maintainers notice (since PR is closed).  Add google group and
irc channel to Help/Support section

Use #gorp as channel

Add gopkg.in versioned releases information.

Also made the markdown headings consistently use prefixed #'s, no suffix.

Change coopernurse/gorp to go-gorp/gorp in Makefile, godoc and comments.

Extended the Exec functionality so that it can be used with named parameters.

Unexport the Executor

Removed version column detection in readStructColumns

Update gorp.go

Added warning telling users that automatic mapping of Version struct members to version column in database (optimistic locking) will be removed in V2

Update gorp.go

Changed log output to stderr

Added migration guide and notes

Added migration guide and notes about change in behaviour in Optimistic locking.

Update README.md

Update gorp.go

Corrected vim gone wild
Readme: latest > last, minor > major.

Removed unnecessary imports

Removed unnecessary tests
Remove automatic mapping of version column (optimistic locking)
@umisama
Copy link
Contributor Author

umisama commented Feb 27, 2015

Finally, test passed! (without go1.1)

@GeertJohan
Copy link
Member

Great, thanks!

GeertJohan pushed a commit that referenced this pull request Mar 1, 2015
Fixed NullTime returns always invalid on sqlite3.
@GeertJohan GeertJohan merged commit 1fd7f81 into go-gorp:develop Mar 1, 2015
@umisama
Copy link
Contributor Author

umisama commented Mar 2, 2015

Sorry, you've merged to develop but it is must merged to master isn't it?

@GeertJohan
Copy link
Member

Oh, yes you're right! I forgot to check if the PR was aimed at master..
Could you maybe rebase this on master and open a new PR?

@umisama
Copy link
Contributor Author

umisama commented Mar 3, 2015

Sorry, I had been misunderstanding about PR system. I create new PR. (see #237)

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling d376e08 on umisama:develop-patch1 into * on go-gorp:develop*.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants