[BUG] SelectOne does not return ErrNoRows when selecting into a value

[robfig]

The documentation for SelectOne says this:

// SelectOne executes the given query (which should be a SELECT statement)
// and binds the result to holder, which must be a pointer.
//
// If no row is found, an an error (sql.ErrNoRows specifically) will be returned
//
// If more than one row is found, an error will be returned.

However, if the caller selects into a primitive, e.g.

var value int
var err = dbm.SelectOne(&value, "select 1 from foo where id = ?", id) 

err will be nil even if no row is found. That is because the code delegates to selectVal for that case, which does not have that requirement.

func SelectOne(m *DbMap, e SqlExecutor, holder interface{}, query string, args ...interface{}) error {
    t := reflect.TypeOf(holder)
    ...
    if t.Kind() == reflect.Struct {
        if list != nil && len(list) > 0 {
            ...
        } else {
            // No rows found, return a proper error.
            return sql.ErrNoRows
        }
        return nil
    }

    return selectVal(e, holder, query, args...)
}

func selectVal(e SqlExecutor, holder interface{}, query string, args ...interface{}) error {
    ...
    rows, err := e.query(query, args...)
    if err != nil {
        return err
    }
    defer rows.Close()

    if rows.Next() {
        err = rows.Scan(holder)
        if err != nil {
            return err
        }
    }

    return nil
}

[coopernurse]

Good catch Rob.

Presumably we don't want the semantics of SelectInt, SelectFloat, SelectStr, etc to change.

If we want those functions to continue to return zero values, then we could:

• modify selectVal() to return sql.ErrNoRows (fixes SelectOne())
• modify the other selectVal() callers to ignore sql.ErrNoRows if err is non-nil

Another option is to return a 2nd value from selectVal() that indicates whether a row was found, but that doesn't seem necessary.

Any opinions?

[robfig]

Yeah -- too bad since it seems like returning ErrNoRows could be helpful for SelectInt too, but I think that ship has sailed

I think your bulleted plan is the right one. It is explicit and roughly the smallest change possible. I think that I or one of my colleagues can provide a fix before long

[seantalts]

is this fixed in the latest gorp?

[coopernurse]

I haven't merged it to master yet. If folks think it looks good I'll promote it to master this week.

[coopernurse]

Hi,

This issue has been resolved in develop. Please test your project
using the latest from github.com/coopernurse/gorp's develop branch
and post a comment on this ticket if things look good to you:

https://github.com/coopernurse/gorp/issues/170

If nothing is found, v1.6 will be merged to master on May 23

thanks, -- James