feat(db-version): add DBVersion model with table existence check #3
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughIntroduces a new database version model and related infrastructure. Adds a DBVersionModel type for querying database version information, an error sentinel for missing tables, a table existence helper function, SQL seed data, comprehensive test coverage, and integrates the new model into the existing Models package. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/models/db_version_test.go`:
- Around line 54-55: The test assertion message is wrong: it checks that
version.PackageName == "base" but the error text says "want components"; update
the failure message in pkg/models/db_version_test.go so the t.Errorf for the
version.PackageName check reads that it wants "base" (or otherwise matches the
expected value) to reflect the actual expectation when
DBVersionModel.GetCurrentVersion() fails.
In `@pkg/models/errors.go`:
- Around line 31-36: Change tableExists to return (bool, error) instead of
masking all query errors as "table not found": have tableExists(ctx, db
*sqlx.DB, tableName string) perform the QueryRowContext call, return exists and
err (nil on success) so callers can distinguish missing table vs DB/context
errors; then update DBVersionModel.GetCurrentVersion to call tableExists,
propagate any non-nil error to the caller (instead of converting all failures
into ErrTableNotFound), and only return ErrTableNotFound when tableExists
reports false with no underlying error.
🧹 Nitpick comments (1)
pkg/models/db_version.go (1)
60-61: Make “current version” deterministic.
LIMIT 1withoutORDER BYcan return an arbitrary row if multiple entries exist. Consider ordering bycreated_at(or a version column) to ensure you always get the latest record.♻️ Suggested tweak
- err := m.db.QueryRowxContext(ctx, - "SELECT package_name, schema_version, created_at, db_release FROM db_version LIMIT 1").StructScan(&dbVersion) + err := m.db.QueryRowxContext(ctx, + "SELECT package_name, schema_version, created_at, db_release FROM db_version ORDER BY created_at DESC LIMIT 1").StructScan(&dbVersion)
| if version.PackageName != "base" { | ||
| t.Errorf("DBVersionModel.GetCurrentVersion() package_name = %v, want components", version.PackageName) |
There was a problem hiding this comment.
Fix the assertion message to match expected value.
The check expects base, but the failure message says “components”.
💚 Proposed fix
- if version.PackageName != "base" {
- t.Errorf("DBVersionModel.GetCurrentVersion() package_name = %v, want components", version.PackageName)
- }
+ if version.PackageName != "base" {
+ t.Errorf("DBVersionModel.GetCurrentVersion() package_name = %v, want base", version.PackageName)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if version.PackageName != "base" { | |
| t.Errorf("DBVersionModel.GetCurrentVersion() package_name = %v, want components", version.PackageName) | |
| if version.PackageName != "base" { | |
| t.Errorf("DBVersionModel.GetCurrentVersion() package_name = %v, want base", version.PackageName) | |
| } |
🤖 Prompt for AI Agents
In `@pkg/models/db_version_test.go` around lines 54 - 55, The test assertion
message is wrong: it checks that version.PackageName == "base" but the error
text says "want components"; update the failure message in
pkg/models/db_version_test.go so the t.Errorf for the version.PackageName check
reads that it wants "base" (or otherwise matches the expected value) to reflect
the actual expectation when DBVersionModel.GetCurrentVersion() fails.
| func tableExists(ctx context.Context, db *sqlx.DB, tableName string) bool { | ||
| var exists bool | ||
| err := db.QueryRowContext(ctx, | ||
| "SELECT EXISTS(SELECT 1 FROM sqlite_master WHERE type='table' AND name=?)", | ||
| tableName).Scan(&exists) | ||
| return err == nil && exists |
There was a problem hiding this comment.
Don’t mask DB errors as “table not found”.
tableExists treats any query error as “missing table,” which can hide connection/context failures and mislead callers. Return the error and let the caller decide.
🐛 Proposed fix
-func tableExists(ctx context.Context, db *sqlx.DB, tableName string) bool {
- var exists bool
- err := db.QueryRowContext(ctx,
- "SELECT EXISTS(SELECT 1 FROM sqlite_master WHERE type='table' AND name=?)",
- tableName).Scan(&exists)
- return err == nil && exists
-}
+func tableExists(ctx context.Context, db *sqlx.DB, tableName string) (bool, error) {
+ var exists bool
+ err := db.QueryRowContext(ctx,
+ "SELECT EXISTS(SELECT 1 FROM sqlite_master WHERE type='table' AND name=?)",
+ tableName).Scan(&exists)
+ if err != nil {
+ return false, err
+ }
+ return exists, nil
+}Update DBVersionModel.GetCurrentVersion to propagate the error instead of returning ErrTableNotFound on query failures.
🤖 Prompt for AI Agents
In `@pkg/models/errors.go` around lines 31 - 36, Change tableExists to return
(bool, error) instead of masking all query errors as "table not found": have
tableExists(ctx, db *sqlx.DB, tableName string) perform the QueryRowContext
call, return exists and err (nil on success) so callers can distinguish missing
table vs DB/context errors; then update DBVersionModel.GetCurrentVersion to call
tableExists, propagate any non-nil error to the caller (instead of converting
all failures into ErrTableNotFound), and only return ErrTableNotFound when
tableExists reports false with no underlying error.
Signed-off-by: Agustin Isasmendi <agustin.isasmendi@scanoss.com>
Signed-off-by: Agustin Isasmendi <agustin.isasmendi@scanoss.com>
isasmendiagus
force-pushed
the
feat/SP-3636/add-db-version-model
branch
from
6395346 to
48c9ad5
Compare
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.