Update NewPool logic in regards to poolsize > 1 with inmemory set #125
Conversation
Update what pool params are accepted
Forgot strings package
Makes the logic easier to read
| if poolSize != 1 { | ||
| uriLower := strings.ToLower(uri) | ||
| inMemory := uri == ":memory:" || opts.Flags&sqlite.OpenMemory != 0 | ||
| sharedCache := strings.Contains(uriLower, "cache=shared") || opts.Flags&sqlite.OpenSharedCache != 0 |
There was a problem hiding this comment.
Although unlikely, the cache=shared string could appear in the file name. I think we need to do some minimal parsing of the URI (and ensure that the URI is, in fact, being parsed as a URI). Off-hand, you might be able to get away with splitting on the ? and doing url.ParseQuery, but double-check the docs to make sure there isn't some subtlety that I haven't thought of.
| case err != nil && !test.wantErr: | ||
| t.Errorf("TestInvalidPoolOptions(%s): got err == %s, want err == nil", test.name, err) | ||
| continue | ||
| } |
There was a problem hiding this comment.
For error tests like these, I like including a case that logs the error message in successful cases without failing. This allows manual inspection to see whether there's a case that's failing for an unrelated error cause.
| for _, test := range tests { | ||
| dbpool, err := sqlitex.NewPool(test.uri, sqlitex.PoolOptions{ | ||
| Flags: test.flags, | ||
| PoolSize: 2, |
There was a problem hiding this comment.
We should vary the pool size for some of these tests.
| dbFileShared := filepath.Join(dir, "awal.db?cache=shared") | ||
|
|
||
| tests := []struct { | ||
| name string |
There was a problem hiding this comment.
Names aren't being used and aren't helpful here. Remove, please.
|
|
||
| if poolSize != 1 { | ||
| uriLower := strings.ToLower(uri) | ||
| inMemory := uri == ":memory:" || opts.Flags&sqlite.OpenMemory != 0 |
There was a problem hiding this comment.
mode=memory is another way this condition can be triggered.
|
For whatever reason I don't seem to get emails for comments any longer (I should fix that). I'll have a look at these in the next few days as time allows and make the changes. |
This addressed Issue: #124