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

database/sql: make (rs *Rows) Scan(dest ...interface{}) not stop at first parsing error #34067

Closed
angelandy opened this issue Sep 4, 2019 · 7 comments

Comments

@angelandy
Copy link

@angelandy angelandy commented Sep 4, 2019

What version of Go are you using (go version)?

$ go version
go-1.12

Does this issue reproduce with the latest release?

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/angelandy/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/media/angelandy/4f95d6b7-a5ba-4a10-9232-6017dcced1d8/angelandy/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/lib/go-1.12"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.12/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build827603706=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I had change the source code at "/usr/lib/go-1.12/src/database/sql/sql.go"

old code

func (rs *Rows) Scan(dest ...interface{}) error {
	rs.closemu.RLock()

	if rs.lasterr != nil && rs.lasterr != io.EOF {
		rs.closemu.RUnlock()
		return rs.lasterr
	}
	if rs.closed {
		err := rs.lasterrOrErrLocked(errRowsClosed)
		rs.closemu.RUnlock()
		return err
	}
	rs.closemu.RUnlock()

	if rs.lastcols == nil {
		return errors.New("sql: Scan called without calling Next")
	}
	if len(dest) != len(rs.lastcols) {
		return fmt.Errorf("sql: expected %d destination arguments in Scan, not %d", len(rs.lastcols), len(dest))
	}
	for i, sv := range rs.lastcols {
		err := convertAssignRows(dest[i], sv, rs)
		if err != nil {
			return fmt.Errorf(`sql: Scan error on column index %d, name %q: %v`, i, rs.rowsi.Columns()[i], err)
		}
	}
	return nil
}

new code

unc (rs *Rows) Scan(dest ...interface{}) error {
	rs.closemu.RLock()

	if rs.lasterr != nil && rs.lasterr != io.EOF {
		rs.closemu.RUnlock()
		return rs.lasterr
	}
	if rs.closed {
		err := rs.lasterrOrErrLocked(errRowsClosed)
		rs.closemu.RUnlock()
		return err
	}
	rs.closemu.RUnlock()

	if rs.lastcols == nil {
		return errors.New("sql: Scan called without calling Next")
	}
	if len(dest) != len(rs.lastcols) {
		return fmt.Errorf("sql: expected %d destination arguments in Scan, not %d", len(rs.lastcols), len(dest))
	}
       var e error
	for i, sv := range rs.lastcols {
		err := convertAssignRows(dest[i], sv, rs)
		if err != nil {
			e = fmt.Errorf(`sql: Scan error on column index %d, name %q: %v`, i, rs.rowsi.Columns()[i], err)
		}
	}
	return e
}

reason:
we want it won't stop convert other fields while one field convert failed
I think user can choose how to handle the error by themself

@ALTree
Copy link
Member

@ALTree ALTree commented Sep 4, 2019

Thanks for the request.

It would be better to just post a description of the proposed change (using words), since old/new code copy-pastes are not particularly readable. Here's a diff of the change you propose:

        if len(dest) != len(rs.lastcols) {
                return fmt.Errorf("sql: expected %d destination arguments in Scan, not %d", len(rs.lastcols), len(dest))
        }
+       var e error
        for i, sv := range rs.lastcols {
                err := convertAssignRows(dest[i], sv, rs)
                if err != nil {
-                       return fmt.Errorf(`sql: Scan error on column index %d, name %q: %v`, i, rs.rowsi.Columns()[i], err)
+                       e = fmt.Errorf(`sql: Scan error on column index %d, name %q: %v`, i, rs.rowsi.Columns()[i], err)
                }
        }
-       return nil
+       return e
 }

I have re-titled the issue accordingly.

@ALTree ALTree changed the title It does't make sense for "func (rs *Rows) Scan(dest ...interface{}) " function database/sql: make (rs *Rows) Scan(dest ...interface{}) not stop at first parsing error Sep 4, 2019
@ALTree ALTree added the NeedsDecision label Sep 4, 2019
@ALTree ALTree added this to the Unplanned milestone Sep 4, 2019
@ALTree
Copy link
Member

@ALTree ALTree commented Sep 4, 2019

@kardianos
Copy link
Contributor

@kardianos kardianos commented Sep 4, 2019

This is a behavior change and test may actually depend on this behavior. The proposed change will return the last error rather then the first error and masks if multiple errors happen. This could be worked around with an error list.

If there is a database/sql/v2, scanning will change.

Can you explain why you would want it to continue scanning on error?

@angelandy
Copy link
Author

@angelandy angelandy commented Sep 5, 2019

I want to scan data to struct

just like this

mysql> select role.role_id,r.id,r.name,r.parent_id,r.seq,r.icon,r.url_for,show_nav from rms_resource as r left join rms_role_resource_rel as role on role.resource_id = r.id where r.parent_id = 0 and show_nav = 1 order by r.seq desc \G;
*************************** 1. row ***************************
  role_id: NULL
       id: 1
     name: 权限管理
parent_id: 0
      seq: 1
     icon: icon-bill
  url_for: 
 show_nav: 1
1 row in set (0.00 sec)

ERROR: 
No query specified

role_id Is null because rms_role_resource_rel has no data

here is my code

		rows, err := mysqlclint.GetRead().QueryRows(sql)
		if err != nil {
			panic(err)
		}

		cols, _ := rows.Columns()
		lennum := len(cols)
		for rows.Next() {
			d1 := reflect.New(t.Elem().Elem().Elem())
			ind1 := reflect.Indirect(d1)
			dest := make([]interface{}, 0)
			for i := 0; i < lennum; i++ {
				field := ind1.Field(fslice[i])
				dest = append(dest, field.Addr().Interface())
			}
			rows.Scan(dest...)
			slice = reflect.Append(slice, d1)
		}
		rows.Close()
		ind.Set(slice)

Resource struct

// TableName 设置表名,是否关联
func (a *Resource) TableInfo() *orm.Table {
	join := make([]*orm.Join, 0)
	join = append(join, &orm.Join{
		"rms_role_resource_rel",
		"role",
		"role.resource_id = r.id",
	})

	return &orm.Table{
		TableName: "rms_resource",
		Alias:     "r",
		Join:      join,
	}
}

// Resource 权限控制资源表
type Resource struct {
	RoleId  int64  `field:"role.role_id"`
	Id      int64  `field:"r.id"`
	Name    string `field:"r.name"`
	Parent  int64  `field:"r.parent_id"`
	Seq     int64  `field:"r.seq"`
	Icon    string `field:"r.icon"`
	UrlFor  string `field:"r.url_for"`
	Shownav int64  `field:"show_nav"`
	Sons    []*Resource
	SonNum  int
	Active  bool
	ShowUl	bool
}

in fact, i don't care convertAssignRows is failed because struct field will have default value.
but it failed while RoleId null into int64, and it stop convertAssignRows another field.
so .....

@kardianos
Copy link
Contributor

@kardianos kardianos commented Sep 5, 2019

What you want to do can be accomplished in two ways:

@angelandy
Copy link
Author

@angelandy angelandy commented Sep 6, 2019

OK

@angelandy
Copy link
Author

@angelandy angelandy commented Sep 6, 2019

but i also prefer Scan field one by one, so that i can handle the erorr by myself.

@kardianos kardianos closed this Sep 10, 2019
@golang golang locked and limited conversation to collaborators Sep 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.