fix(migrate): improve migration status detection and prevent checksum conflicts#54
Merged
Conversation
Reviewer's GuideRefactors migration detection to distinguish between new, executed, and modified migrations based on checksum and metadata, updates the migrator loop to act on the new statuses, and hardens a database-backed test by resetting its schema more thoroughly. Sequence diagram for migration execution using new migration statusessequenceDiagram
actor Operator
participant Migrator
participant DB as sqle_DB
participant Tx as sqle_Tx
Operator->>Migrator: startMigrate(ctx, db)
Migrator->>DB: Transaction(ctx, callback)
activate DB
DB-->>Migrator: Tx
activate Migrator
loop for each Migration in version.Migrations
Migrator->>Migrator: getMigrationStatus(Tx, version.Name, migration)
alt status is MigrationStatusExecuted
Migrator-->>Operator: log executed [✔]
else status is MigrationStatusModified
Migrator-->>Operator: log modified [!]
else status is MigrationStatusNew
Migrator->>Migrator: buildRotations(migration.Rotate, migration.RotateBegin, migration.RotateEnd)
Migrator->>Tx: execute migration SQL
Tx-->>Migrator: result
end
end
DB-->>Operator: migration transaction committed
deactivate Migrator
deactivate DB
Class diagram for Migrator and migration status handlingclassDiagram
class MigrationStatus {
<<enum>>
MigrationStatusNew
MigrationStatusExecuted
MigrationStatusModified
}
class Migration {
string Name
int Rank
string Checksum
shardid_TableRotate Rotate
time_Time RotateBegin
time_Time RotateEnd
}
class Migrator {
sqle_DB[] dbs
string suffix
string module
startMigrate(ctx context_Context, db sqle_DB) error
getMigrationStatus(tx sqle_Tx, version string, s Migration) MigrationStatus
buildRotations(r shardid_TableRotate, begin time_Time, end time_Time) string[]
}
Migrator --> MigrationStatus : uses
Migrator --> Migration : evaluates
Flow diagram for getMigrationStatus decision logicflowchart TD
A[start getMigrationStatus] --> B[Query sqle_migrations by checksum]
B -->|row found| C[Return MigrationStatusExecuted]
B -->|error not sql.ErrNoRows| D[Return MigrationStatusNew with error]
B -->|no rows| E[Query sqle_migrations by module, version, name, rank]
E -->|no rows| F[Return MigrationStatusNew]
E -->|error not sql.ErrNoRows| G[Return MigrationStatusNew with error]
E -->|row found with different checksum| H[Return MigrationStatusModified]
C --> I[end]
D --> I
F --> I
G --> I
H --> I[end]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `dtc_test.go:13-21` </location>
<code_context>
func TestDTCWithDB(t *testing.T) {
- os.Remove("dtc_db.db")
+ os.Remove("dtc_1.db")
d, err := sql.Open("sqlite3", "file:dtc_1.db?cache=shared&mode=rwc")
require.NoError(t, err)
+ _, err = d.Exec("DROP TABLE IF EXISTS `dtc_1`")
+ require.NoError(t, err)
+
_, err = d.Exec("CREATE TABLE `dtc_1` (`id` int , `email` varchar(50),`created_at` DATETIME, PRIMARY KEY (`id`))")
require.NoError(t, err)
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen `TestDTCWithDB` isolation and cleanup for repeatable runs.
The added `DROP TABLE IF EXISTS` and file removal help, but the test still relies on a fixed file name (`dtc_1.db`) and may leave artifacts if it fails partway through. Please ensure cleanup is guaranteed (e.g., via `t.Cleanup`/`defer` to remove the file and drop the table) and consider using a unique or temp DB filename per run to avoid test interference and flakiness in CI or repeated local runs.
Suggested implementation:
```golang
func TestDTCWithDB(t *testing.T) {
func TestDTCWithDB(t *testing.T) {
dbDir := t.TempDir()
dbPath := filepath.Join(dbDir, "dtc_1.db")
d, err := sql.Open("sqlite3", fmt.Sprintf("file:%s?cache=shared&mode=rwc", dbPath))
require.NoError(t, err)
t.Cleanup(func() {
// Best-effort cleanup to keep runs isolated and avoid DB artifacts.
_, _ = d.Exec("DROP TABLE IF EXISTS `dtc_1`")
_ = d.Close()
_ = os.Remove(dbPath)
})
```
To compile and run successfully, you will also need to:
1. Ensure the import block for this test file includes:
- `fmt`
- `path/filepath`
(and `os` if it is not already imported).
For example:
```go
import (
"database/sql"
"fmt"
"os"
"path/filepath"
"testing"
"github.com/stretchr/testify/require"
)
```
2. Leave the rest of `TestDTCWithDB` (the `CREATE TABLE`, test logic, assertions, etc.) as-is below the shown snippet so it continues to use `d` as before.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+13
to
21
| os.Remove("dtc_1.db") | ||
|
|
||
| d, err := sql.Open("sqlite3", "file:dtc_1.db?cache=shared&mode=rwc") | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = d.Exec("DROP TABLE IF EXISTS `dtc_1`") | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = d.Exec("CREATE TABLE `dtc_1` (`id` int , `email` varchar(50),`created_at` DATETIME, PRIMARY KEY (`id`))") |
There was a problem hiding this comment.
suggestion (testing): Strengthen TestDTCWithDB isolation and cleanup for repeatable runs.
The added DROP TABLE IF EXISTS and file removal help, but the test still relies on a fixed file name (dtc_1.db) and may leave artifacts if it fails partway through. Please ensure cleanup is guaranteed (e.g., via t.Cleanup/defer to remove the file and drop the table) and consider using a unique or temp DB filename per run to avoid test interference and flakiness in CI or repeated local runs.
Suggested implementation:
func TestDTCWithDB(t *testing.T) {
func TestDTCWithDB(t *testing.T) {
dbDir := t.TempDir()
dbPath := filepath.Join(dbDir, "dtc_1.db")
d, err := sql.Open("sqlite3", fmt.Sprintf("file:%s?cache=shared&mode=rwc", dbPath))
require.NoError(t, err)
t.Cleanup(func() {
// Best-effort cleanup to keep runs isolated and avoid DB artifacts.
_, _ = d.Exec("DROP TABLE IF EXISTS `dtc_1`")
_ = d.Close()
_ = os.Remove(dbPath)
})To compile and run successfully, you will also need to:
- Ensure the import block for this test file includes:
fmtpath/filepath
(andosif it is not already imported).
For example:
import ( "database/sql" "fmt" "os" "path/filepath" "testing" "github.com/stretchr/testify/require" )
- Leave the rest of
TestDTCWithDB(theCREATE TABLE, test logic, assertions, etc.) as-is below the shown snippet so it continues to usedas before.
|
Here's the code health analysis summary for commits Analysis Summary
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #54 +/- ##
==========================================
+ Coverage 75.73% 77.58% +1.84%
==========================================
Files 46 46
Lines 2370 1896 -474
==========================================
- Hits 1795 1471 -324
+ Misses 456 304 -152
- Partials 119 121 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Improve migration status detection to distinguish between new, executed, and modified migrations and adjust migration execution accordingly.
Bug Fixes:
Enhancements:
Tests: