Skip to content
This repository has been archived by the owner on Sep 7, 2021. It is now read-only.
This repository is currently being migrated. It's locked while the migration is in progress.

fix issue #1352 #1354

Closed
wants to merge 2 commits into from
Closed

fix issue #1352 #1354

wants to merge 2 commits into from

Conversation

guangfnian
Copy link

No description provided.

@codecov-io
Copy link

codecov-io commented Jul 11, 2019

Codecov Report

Merging #1354 into master will decrease coverage by <.01%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1354      +/-   ##
==========================================
- Coverage    57.5%   57.49%   -0.01%     
==========================================
  Files          44       44              
  Lines        7833     7837       +4     
==========================================
+ Hits         4504     4506       +2     
- Misses       2773     2774       +1     
- Partials      556      557       +1
Impacted Files Coverage Δ
session_update.go 56.26% <0%> (-0.18%) ⬇️
session_get.go 76% <100%> (+0.16%) ⬆️
session_delete.go 58.49% <100%> (+0.26%) ⬆️
session_find.go 68.01% <100%> (+0.09%) ⬆️
xorm.go 66.66% <0%> (-1.52%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3e6c3a...e5c63d5. Read the comment docs.

@lunny lunny added the kind/bug label Jul 11, 2019
@lunny
Copy link
Member

lunny commented Jul 30, 2019

It is unnecessary since queryRows will always invoke resetStatement.

@guangfnian
Copy link
Author

It is unnecessary since queryRows will always invoke resetStatement.

Have you checked my example in issue #1352 ?

Let's take session.cacheGet() function as an example

func (session *Session) cacheGet(bean interface{}, sqlStr string, args ...interface{}) (has bool, err error) {
	// skip .........
	table := session.statement.RefTable
	ids, err := core.GetCacheSql(cacher, tableName, newsql, args)
	if err != nil {
		var res = make([]string, len(table.PrimaryKeys))
		rows, err := session.NoCache().queryRows(newsql, args...)
		// skip ........
	} else {
		session.engine.logger.Debug("[cacheGet] cache hit sql:", newsql, ids)
	}

The problem is, when cache hit sql, err will be nil, and queryRows won't be called !

@lunny
Copy link
Member

lunny commented Jul 30, 2019

@guangfnian I will send a PR to replace this one and fix that issue.

@BetaCat0
Copy link
Member

Fixed in #1375

@BetaCat0 BetaCat0 closed this Jul 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants