Conversation
Summary by CodeRabbit
WalkthroughUpdated exported Config.Models type, refactored repository query option handling to merge variadic inputs and support QueryBuilder predicates in Count/Exist, renamed exported tenancy helper, and applied minor formatting changes in tests and imports. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Repo as Repository[M]
participant DB as GORM DB/Tx
participant QB as QueryBuilder
Caller->>Repo: Count(where, options...)
Repo->>Repo: opt = common.MergeStruct(options...)
alt opt.WithDeleted
Repo->>DB: tx = DB.Unscoped()
else
Repo->>DB: tx = DB
end
alt where is func(qb *QueryBuilder)
Repo->>QB: qb := NewQueryBuilder(tx)
Caller->>QB: where(qb)
QB-->>Repo: qb builds query on tx
Repo->>DB: tx (with qb filters).Count(&count)
else
Repo->>DB: tx.Where(where).Count(&count)
end
DB-->>Repo: count / error
Repo-->>Caller: return count, error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tenancy/tenancy.go (2)
133-165: Fix DB existence check, close leaks, and harden against SQL injection.
- Bug: rs.Find(rec) into a map never populates; len(rec) stays 0, so you always create the DB.
- Leak: the postgres admin connection isn’t closed on the “exists” path.
- Injection: dbName is interpolated into SQL; validate and quote as an identifier.
Apply this diff to CreateDatabaseIfNotExist:
func CreateDatabaseIfNotExist(dbName string, opt ConnectOptions) error { conStr := fmt.Sprintf("host=%s port=%d user=%s password=%s dbname=postgres sslmode=disable TimeZone=Asia/Shanghai", opt.Host, opt.Port, opt.User, opt.Password) db, err := gorm.Open(postgres.Open(conStr), &gorm.Config{}) if err != nil { return err } - // check if db exists - stmt := fmt.Sprintf("SELECT * FROM pg_database WHERE datname = '%s';", dbName) - rs := db.Raw(stmt) - if rs.Error != nil { - return rs.Error - } - - // if not create it - var rec = make(map[string]interface{}) - if rs.Find(rec); len(rec) == 0 { - stmt := fmt.Sprintf("CREATE DATABASE %s;", dbName) - if rs := db.Exec(stmt); rs.Error != nil { - return rs.Error - } - - // close db connection - sql, err := db.DB() - defer func() { - _ = sql.Close() - }() - if err != nil { - return err - } - } + // always close admin connection + sqlDB, err := db.DB() + if err != nil { + return err + } + defer func() { _ = sqlDB.Close() }() + + // validate dbName and check existence safely + if !isValidPGIdentifier(dbName) { + return fmt.Errorf("invalid database name") + } + var cnt int64 + if err := db.Raw("SELECT COUNT(1) FROM pg_database WHERE datname = ?", dbName).Scan(&cnt).Error; err != nil { + return err + } + if cnt == 0 { + // quote identifier to avoid injection + stmt := fmt.Sprintf("CREATE DATABASE %s;", quoteIdentPG(dbName)) + if rs := db.Exec(stmt); rs.Error != nil { + return rs.Error + } + } return nil }Add these helpers (outside this function):
import "strings" import "regexp" var pgIdentRe = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]{0,62}$`) func isValidPGIdentifier(s string) bool { return pgIdentRe.MatchString(s) } // minimal identifier quoting ("" escaping) func quoteIdentPG(s string) string { return `"` + strings.ReplaceAll(s, `"`, `""`) + `"` }
47-71: Guard concurrent access to ConnectMapper and DB creation.CONNECT_MAPPER is a plain map shared across requests. The check-then-create block around mapper[tenantID] is unsynchronized and can panic with “concurrent map writes” and/or double-create connections.
Minimal fix: protect the block with a package-level mutex.
// at file scope var connectMu sync.MutexWrap the critical section in the factory:
- if mapper[tenantID] == nil { + connectMu.Lock() + if mapper[tenantID] == nil { // ... create DB if needed, open conn, migrate, assign mapper[tenantID] - } + } + connectMu.Unlock()Alternative: replace ConnectMapper with sync.Map or a struct { sync.RWMutex; M map[string]*gorm.DB } and use RW locking.
query.go (2)
212-229: FindAllAndCount doesn’t pass options to Count.With the new Count options, WithDeleted (and any future Count options) are ignored here, causing inconsistent totals.
Apply this minimal patch now:
go func() { defer wg.Done() - countRes, countErr = repo.Count(where) + if len(options) > 0 { + countRes, countErr = repo.Count(where, FindOneOptions{WithDeleted: options[0].WithDeleted}) + } else { + countRes, countErr = repo.Count(where) + } }()If you adopt the preferred Count signature (using FindOptions), simply:
- countRes, countErr = repo.Count(where) + countRes, countErr = repo.Count(where, options...)
139-169: Count: align API with FindAll and apply FindOptions (fix Distinct & wire options through).Count currently accepts interface{} + FindOneOptions and doesn't apply Distinct; update to use Query + FindOptions and pass options through from FindAllAndCount.
- Change Count signature and use FindOptions (merge with common.MergeStruct). Add Distinct handling.
- Update FindAllAndCount to call repo.Count(where, options...) so Count receives options (including Distinct).
- File: query.go — update functions Count and FindAllAndCount.
Recommended diff (apply to Count only; also update call site as above):
- func (repo *Repository[M]) Count(where interface{}, options ...FindOneOptions) (int64, error) { + func (repo *Repository[M]) Count(where Query, options ...FindOptions) (int64, error) { var count int64 var model M - var opt FindOneOptions - if len(options) > 0 { - opt = common.MergeStruct(options...) - } + var opt FindOptions + if len(options) > 0 { + opt = common.MergeStruct(options...) + } tx := repo.DB.Model(&model) if opt.WithDeleted { tx = tx.Unscoped() } + if len(opt.Distinct) > 0 { + tx = tx.Distinct(opt.Distinct...) + } if IsQueryBuilder(where) { // unchanged... } else { tx = tx.Where(where) } result := tx.Count(&count) // unchanged... }
🧹 Nitpick comments (2)
module.go (1)
22-22: Switch to []any is OK; consider consistency across packages.any is an alias of interface{} (Go 1.18+). tenancy.Options.Models still uses []interface{} — consider aligning to []any for consistency.
tenancy/tenancy.go (1)
66-76: Consider error handling and config polish.
- Panic on CreateDatabaseIfNotExist/AutoMigrate failures will crash the process; prefer returning nil and letting upstream handle a 5xx.
- DSN hard-codes TimeZone=Asia/Shanghai; make it configurable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
module.go(3 hunks)query.go(3 hunks)tenancy/tenancy.go(2 hunks)tenancy/tenancy_test.go(0 hunks)
💤 Files with no reviewable changes (1)
- tenancy/tenancy_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
query.go (3)
repository.go (1)
Repository(20-22)model.go (1)
Model(11-16)builder.go (2)
IsQueryBuilder(14-20)QueryBuilder(22-24)
🔇 Additional comments (3)
module.go (1)
6-6: Import change is fine.time is used by RetryOptions.Delay; no action needed.
query.go (2)
6-6: Import looks good.Using common.MergeStruct here is fine.
175-189: Merge semantics: confirm precedence for common.MergeStruct.Ensure later options override earlier ones and zero-values don’t clobber set values unintentionally (esp. WithDeleted). Please add/extend tests for Exist/Count option merging.
ren0503
force-pushed
the
feat/ren/80-add-count-options
branch
from
dfd5d03 to
b1da2cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
query.go (3)
139-169: API consistency + safer predicate handling in Count.
- Make Count’s where parameter type consistent with other APIs (Query) to reduce confusion.
- If a function is passed but doesn’t match func(*QueryBuilder), the filter is silently dropped. Fail fast.
Apply this diff:
-import ( - "sync" - - "github.com/tinh-tinh/tinhtinh/v2/common" - "gorm.io/gorm" -) +import ( + "fmt" + "sync" + + "github.com/tinh-tinh/tinhtinh/v2/common" + "gorm.io/gorm" +) @@ -func (repo *Repository[M]) Count(where interface{}, options ...FindOneOptions) (int64, error) { +func (repo *Repository[M]) Count(where Query, options ...FindOneOptions) (int64, error) { @@ - if IsQueryBuilder(where) { - queryFnc, ok := where.(func(qb *QueryBuilder)) - if ok { - qb := &QueryBuilder{qb: tx} - queryFnc(qb) - tx = qb.qb - } - } else { - tx = tx.Where(where) - } + if IsQueryBuilder(where) { + queryFnc, ok := where.(func(qb *QueryBuilder)) + if !ok { + return 0, fmt.Errorf("invalid QueryBuilder predicate; expected func(qb *QueryBuilder)") + } + qb := &QueryBuilder{qb: tx} + queryFnc(qb) + tx = qb.qb + } else if where != nil { + tx = tx.Where(where) + }Replicate the “fail fast or ignore nil” pattern in FindAll/FindOne/Exist for consistency.
220-229: Bug: FindAllAndCount ignores WithDeleted in Count.Totals can disagree with FindAll when WithDeleted is set. Pass the option through.
Apply this diff:
func (repo *Repository[M]) FindAllAndCount(where Query, options ...FindOptions) ([]*M, int64, error) { var wg sync.WaitGroup var findAllRes []*M var countRes int64 var findErr, countErr error + // Merge options once so we can forward WithDeleted to Count + var opt FindOptions + if len(options) > 0 { + opt = common.MergeStruct(options...) + } @@ go func() { defer wg.Done() findAllRes, findErr = repo.FindAll(where, options...) }() go func() { defer wg.Done() - countRes, countErr = repo.Count(where) + countRes, countErr = repo.Count(where, FindOneOptions{WithDeleted: opt.WithDeleted}) }()
67-76: Avoid silently dropping filters when a wrong-sig function is passed.Across methods, if IsQueryBuilder(where) is true but the type assertion fails, no filter is applied. Either error out or fall back to tx.Where(where). Prefer error for debuggability.
Also applies to: 114-123, 153-162, 191-200
🧹 Nitpick comments (2)
query_test.go (1)
137-143: Great: Count now supports WithDeleted; add a matching FindAllAndCount test.Please also assert that FindAllAndCount(nil, sqlorm.FindOptions{WithDeleted: true}) returns a total that includes soft-deleted rows. This will catch a current mismatch where Count ignores WithDeleted when called from FindAllAndCount.
I can draft the test if you want.
query.go (1)
33-36: Confirm MergeStruct precedence; consider a small helper to avoid zero-value clobber.Double-check MergeStruct’s semantics (last-wins? zero-values ignored?). If zero-values can overwrite non-zero fields, wrap it in mergeFindOptions/mergeFindOneOptions to enforce predictable precedence.
Also applies to: 89-92, 143-146, 175-178
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
module.go(3 hunks)query.go(5 hunks)query_test.go(2 hunks)tenancy/tenancy.go(2 hunks)tenancy/tenancy_test.go(0 hunks)
💤 Files with no reviewable changes (1)
- tenancy/tenancy_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- module.go
- tenancy/tenancy.go
🧰 Additional context used
🧬 Code graph analysis (2)
query_test.go (2)
builder.go (1)
QueryBuilder(22-24)query.go (1)
FindOneOptions(10-16)
query.go (2)
repository.go (1)
Repository(20-22)builder.go (2)
IsQueryBuilder(14-20)QueryBuilder(22-24)
🔇 Additional comments (2)
query_test.go (1)
118-121: Good addition: exercising the new QueryBuilder path in Count.This covers the function-based predicate route and avoids the prior map limitation. Nice.
query.go (1)
6-6: Dependency already pinned: github.com/tinh-tinh/tinhtinh/v2 → v2.3.0.
go.mod containsgithub.com/tinh-tinh/tinhtinh/v2 v2.3.0(line 10).
2 participants
No description provided.