add ScanStructs, ScanVals to Scanner interface #273
Conversation
Codecov Report
@@ Coverage Diff @@
## master #273 +/- ##
==========================================
+ Coverage 96.76% 96.77% +0.01%
==========================================
Files 60 60
Lines 3306 3319 +13
==========================================
+ Hits 3199 3212 +13
Misses 91 91
Partials 16 16
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
@doug-martin could you please review and merge if there are no questions? |
|
@vlanse I'll review and release tomorrow. Thank you for your all of your contributions! |
exec/query_executor.go
Outdated
| val := reflect.ValueOf(i) | ||
| if !util.IsPointer(val.Kind()) { | ||
| return errUnsupportedScanStructsType | ||
| if _, err := checkScanStructsTarget(i); err != nil { |
There was a problem hiding this comment.
Is there a reason to do this check both here and in the Scanner#ScanStructs? Same for ScanVals?
There was a problem hiding this comment.
I should clarify this, I think we should just do the check in the single place for each to keep it DRY.
There was a problem hiding this comment.
I suggest to call checkScanStructsTarget (checkScanValsTarget) only in corresponding Scanner methods because errors that cause checks to fail seem likely to not occur at runtime (they should be detected early by simple tests in fact, along with errors like invalid SQL) , so it does not matter that check will be done before actual query execution.
Will update PR appropriately
| @@ -67,7 +67,14 @@ func (ds *databaseSuite) TestScanStructs() { | |||
| WithArgs(). | |||
| WillReturnRows(sqlmock.NewRows([]string{"address", "name"}). | |||
| FromCSVString("111 Test Addr,Test1\n211 Test Addr,Test2")) | |||
|
|
|||
| mock.ExpectQuery(`SELECT \* FROM "items"`). | |||
There was a problem hiding this comment.
I might be missing something but I don't see why these additional expects added here and below?
There was a problem hiding this comment.
Now targets validation is done right in ScanRows(), after query is executed by db, thus mock should expect query before error will be returned.
As I wrote above, this should not be a problem because invalid parameters for scan rows is a kind of error that should be detected by basic tests at early stages of development. So in production this error is not likely to occur.
Of course there is an alternative that keeps previous behaviour, but is seem to be ugly, like need some flag to tell scanner that validation was already done.
There was a problem hiding this comment.
Got it, looking at https://github.com/doug-martin/goqu/blob/eda0d817d80bd64ad75c2aa0151b717d0edb492b/exec/query_executor.go#L241:24 it makes sense.
ScanStructs,ScanValsmethods are added toScannerinterface.Rationale is to allow reuse goqu very convenient struct tags in case when goqu is used as query builder only (i.e. queries built with goqu but executed via
sqlmodule API). In my case such approach allows to make some advanced query processing, like metrics collection or scanning results on-by-one - that's why goqu executor is not always suitable.Also, I wanted to reuse code from
internal/utilsas much as possible