go-xorm / xorm

Simple and Powerful ORM for Go, support mysql,postgres,tidb,sqlite3,mssql,oracle, Moved to https://gitea.com/xorm/xorm
BSD 3-Clause "New" or "Revised" License
6.67k stars 754 forks source link

Session not completely cleaned #1185

Open zeripath opened 5 years ago

zeripath commented 5 years ago

Hi!

In Gitea at https://github.com/go-gitea/gitea/blob/master/models/issue.go#L1349

func Issues(opts *IssuesOptions) ([]*Issue, error) {
    sess := x.NewSession()
    defer sess.Close()

    if err := opts.setupSession(sess); err != nil {
        return nil, err
    }
    sortIssuesSession(sess, opts.SortType)

    issues := make([]*Issue, 0, setting.UI.IssuePagingNum)
    if err := sess.Find(&issues); err != nil {
        return nil, fmt.Errorf("Find: %v", err)
    }

    if err := IssueList(issues).LoadAttributes(); err != nil {
        return nil, fmt.Errorf("LoadAttributes: %v", err)
    }

    return issues, nil
}

If we change the LoadAttributesfunction to perform its lookups within the same session the results are not the same as those without it, because it appears that at least some of the setup performed in opts.setupSession is not reset following the Find above.

Although this doesn't present a problem at the moment because there is no transaction created and no changes are made to the database, the lack of a complete reset could cause unpredictable heisenbugs elsewhere.

zeripath commented 5 years ago

OK, so the reason this appears to fail in Gitea is that the failure happens because issues.loadAttachments(sess) calls issues.loadLabels(sess):

func (issues IssueList) loadLabels(e Engine) error {
    if len(issues) == 0 {
        return nil
    }

    type LabelIssue struct {
        Label      *Label      `xorm:"extends"`
        IssueLabel *IssueLabel `xorm:"extends"`
    }

    var issueLabels = make(map[int64][]*Label, len(issues)*3)
    var issueIDs = issues.getIssueIDs()
    var left = len(issueIDs)
    for left > 0 {
        var limit = defaultMaxInSize
        if left < limit {
            limit = left
        }
        rows, err := e.Table("label").
            Join("LEFT", "issue_label", "issue_label.label_id = label.id").
            In("issue_label.issue_id", issueIDs[:limit]).
            Asc("label.name").
            Rows(new(LabelIssue))
        if err != nil {
            return err
        }

        for rows.Next() {
            var labelIssue LabelIssue
            err = rows.Scan(&labelIssue)
            if err != nil {
                rows.Close()
                return err
            }
            issueLabels[labelIssue.IssueLabel.IssueID] = append(issueLabels[labelIssue.IssueLabel.IssueID], labelIssue.Label)
        }
        rows.Close()
        left = left - limit
        issueIDs = issueIDs[limit:]
    }

    for _, issue := range issues {
        issue.Labels = issueLabels[issue.ID]
    }
    return nil
}

Now it appears that the line:

err = rows.Scan(&labelIssue)

Causes xorm to issue:

SELECT `id`, `repo_id`, `name`, `description`, `color`, `num_issues`, `num_closed_issues`, `id`, `issue_id`, `label_id` FROM `label_issue` WHERE `id` IN (?) []interface {}{0}

The table label_issue doesn't exist.

Looking at rows.go:

// Scan row record to bean properties
func (rows *Rows) Scan(bean interface{}) error {
    if rows.lastError != nil {
        return rows.lastError
    }

    if !rows.NoTypeCheck && reflect.Indirect(reflect.ValueOf(bean)).Type() != rows.beanType {
        return fmt.Errorf("scan arg is incompatible type to [%v]", rows.beanType)
    }

    if err := rows.session.statement.setRefBean(bean); err != nil {
        return err
    }

    scanResults, err := rows.session.row2Slice(rows.rows, rows.fields, bean)
    if err != nil {
        return err
    }

    dataStruct := rValue(bean)
    _, err = rows.session.slice2Bean(scanResults, rows.fields, bean, &dataStruct, rows.session.statement.RefTable)
    if err != nil {
        return err
    }

    return rows.session.executeProcessors()
}

I suspect the problem lies in the last line: rows.session.executeProcessors()

lunny commented 5 years ago

This should be a bug of xorm and I think it's on Table("table_name").Rows(). The table name haven't been used.