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

Select: Allow caller to pass pointer-to-slice to have results appended #40

Merged
merged 3 commits into from
May 24, 2013

Conversation

robfig
Copy link
Contributor

@robfig robfig commented May 19, 2013

Maintains backwards compatibility.

(Tested with MySQL only.)

Fixes #36

@coopernurse
Copy link
Contributor

Wow, thanks for the quick turnaround. I don't think I'll have time to review this tonight, but hopefully in the next day or two.

@sqs
Copy link
Contributor

sqs commented May 22, 2013

This patch fails the tests on PostgreSQL due to the use of the bind var ?:

--- FAIL: TestHooks (0.34 seconds)
gorptest: 16:04:15.413071 select * from person_test where id = ? [[1]]
panic: pq: S:"ERROR" C:"42601" M:"syntax error at end of input" P:"39" F:"scan.l" L:"994" R:"scanner_yyerror" [recovered]
    panic: pq: S:"ERROR" C:"42601" M:"syntax error at end of input" P:"39" F:"scan.l" L:"994" R:"scanner_yyerror"

goroutine 12 [running]:
testing.func��004()
    /home/sqs/src/go/src/pkg/testing/testing.go:348 +0xcd
gorp._rawselect(0xc20012f720, 0x5cd140, 0xc200148f20, 0x6d7a70, 0x26, ...)
    /home/sqs/src/gorp/gorp_test.go:961 +0xdf
gorp.TestHooks(0xc20012cab0)
    /home/sqs/src/gorp/gorp_test.go:438 +0x4e9
testing.tRunner(0xc20012cab0, 0xa05680)
    /home/sqs/src/go/src/pkg/testing/testing.go:353 +0x8a
created by testing.RunTests
    /home/sqs/src/go/src/pkg/testing/testing.go:433 +0x86b

After applying this diff, it passes both the PostgreSQL and SQLite tests for me:

diff --git a/gorp_test.go b/gorp_test.go
index 63485dc..6ef3e2a 100644
--- a/gorp_test.go
+++ b/gorp_test.go
@@ -435,7 +435,8 @@ func TestHooks(t *testing.T) {
        }

        var persons []*Person
-       rawselect(dbmap, &persons, "select * from person_test where id = ?", p1.Id)
+       bindVar := dbmap.Dialect.BindVar(0)
+       rawselect(dbmap, &persons, "select * from person_test where id = "+bindVar, p1.Id)
        if persons[0].LName != "postget" {
                t.Errorf("p1.PostGet() didn't run after select: %v", p1)
        }

@robfig
Copy link
Contributor Author

robfig commented May 22, 2013

Thanks for testing it out @sqs ! I applied your patch. Tests continue to pass on MySQL.

@sqs
Copy link
Contributor

sqs commented May 24, 2013

If you try to select into var foo []*MyType instead of a var foo *[]*MyType (e.g., you forget the & in the arg), then you get an error message that is no longer applicable with this new feature:
gorp: Cannot SELECT into non-struct type: []*MyType

I think this error message should be updated in this PR.

BTW, I've been using this new feature for a day and have simplified a bunch of my code. It's great! And it is working well.

@@ -1099,7 +1117,11 @@ func rawselect(m *DbMap, exec SqlExecutor, i interface{}, query string,

conv := m.TypeConverter

list := make([]interface{}, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this breaks backcompat. Previously, an empty SQL result set would yield a empty slice return value. Now, it yields nil. (I discovered this in my code that marshals the JSON of the gorp query results; it used to be [] when empty, and now it's null.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a patch+testcase that ensures that results are always non-nil: https://github.com/sqs/gorp/commit/6dfc2ee1f39270fc9b5d172701610d7c2335a665

@coopernurse
Copy link
Contributor

Hey guys. I'm merging this stuff into master now. @sqs I have integrated your patch+test from 6dfc2ee.

I think the only outstanding item is the error message: "gorp: Cannot SELECT into non-struct type: []*MyType"

I'm inclined to just get these commits into master, and open a separate issue for that. Just so we don't keep this work sequestered too long.

thanks guys for all your work on this! great addition.

-- James

coopernurse added a commit that referenced this pull request May 24, 2013
…ts so they compile post-merge. Passes on all 3 dbs
@coopernurse coopernurse merged commit 4036ce9 into go-gorp:master May 24, 2013
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.

Improve interface for list of results
3 participants