feat(blob-uploader): blob_upload table add batch hash #1677
Conversation
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BlobUploader
participant BlobUploadORM
participant BatchORM
Client->>BlobUploader: UploadBlobToS3(platform)
BlobUploader->>BlobUploader: GetFirstUnuploadedBatchByPlatform(startBatch, platform)
BlobUploader->>BlobUploadORM: GetNextBatchIndexToUploadByPlatform(startBatch, platform)
BlobUploadORM-->>BlobUploader: batchIndex
BlobUploader->>BatchORM: GetBatchByIndex(batchIndex)
BatchORM-->>BlobUploader: batch
alt Parent batch not uploaded
BlobUploader->>BlobUploadORM: GetBlobUploads(parentBatchIndex, platform)
BlobUploadORM-->>BlobUploader: upload status
BlobUploader->>BlobUploader: Decrement batchIndex and retry
end
alt Batch committed
BlobUploader-->>Client: batch
else Batch not committed or not found
BlobUploader-->>Client: nil
end
BlobUploader->>BlobUploadORM: InsertOrUpdateBlobUpload(batchIndex, batchHash, platform, status)
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1677 +/- ##
===========================================
- Coverage 40.21% 40.16% -0.05%
===========================================
Files 232 232
Lines 18391 18448 +57
===========================================
+ Hits 7396 7410 +14
- Misses 10275 10315 +40
- Partials 720 723 +3
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:
|
![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
🔭 Outside diff range comments (1)
database/migrate/migrations/00027_ blob_upload.sql (1)
1-1:⚠️ Potential issueFix the filename - remove extra space after migration number.
The filename contains an extra space:
00027_ blob_upload.sql. This could cause issues with migration tools. Rename it to00027_blob_upload.sql.
🧹 Nitpick comments (1)
rollup/internal/orm/blob_upload.go (1)
73-81: Clarify the ordering behavior in GetBlobUploads.The method always appends
ORDER BY batch_index ASCafter applying the providedorderByList. This could lead to unexpected results iforderByListalready contains abatch_indexordering.Consider documenting this behavior or making it conditional:
+// If batch_index is not in orderByList, append it for consistent ordering +hasBatchIndexOrder := false for _, orderBy := range orderByList { + if strings.Contains(strings.ToLower(orderBy), "batch_index") { + hasBatchIndexOrder = true + } db = db.Order(orderBy) } if limit > 0 { db = db.Limit(limit) } -db = db.Order("batch_index ASC") +if !hasBatchIndexOrder { + db = db.Order("batch_index ASC") +}Note: You'll need to import the "strings" package if you implement this suggestion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
database/migrate/migrations/00027_ blob_upload.sql(2 hunks)rollup/internal/controller/blob_uploader/blob_uploader.go(6 hunks)rollup/internal/orm/batch.go(0 hunks)rollup/internal/orm/blob_upload.go(3 hunks)
💤 Files with no reviewable changes (1)
- rollup/internal/orm/batch.go
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
rollup/internal/orm/blob_upload.go (1)
55-55: Wrong struct name in error message (previous feedback still unresolved)The message still says
Batch.GetFirstUnuploadedBatchIndexByPlatform.
ReplaceBatchwithBlobUploadto keep stack traces accurate.- return 0, fmt.Errorf("Batch.GetFirstUnuploadedBatchIndexByPlatform error: %w", err) + return 0, fmt.Errorf("BlobUpload.GetFirstUnuploadedBatchIndexByPlatform error: %w", err)
🧹 Nitpick comments (1)
rollup/internal/orm/blob_upload.go (1)
104-108: Upsert should also refreshupdated_atDuring the conflict update you only set
status, leavingupdated_atstale.
Include it so dashboards and audits reflect the true modification time.- DoUpdates: clause.AssignmentColumns([]string{"status"}), + DoUpdates: clause.AssignmentColumns([]string{"status", "updated_at"}),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rollup/internal/controller/blob_uploader/blob_uploader.go(6 hunks)rollup/internal/orm/blob_upload.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rollup/internal/controller/blob_uploader/blob_uploader.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
rollup/internal/orm/blob_upload.go (2)
common/types/db.go (3)
BlobStoragePlatform(358-358)BlobUploadStatusUploaded(339-339)BlobUploadStatus(331-331)bridge-history-api/internal/orm/migrate/migrate.go (1)
Status(54-56)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
rollup/internal/orm/blob_upload.go (2)
19-22: Declare composite primary key to restore ORM identityThe struct still lacks
primaryKeytags onbatch_index,batch_hash, andplatform.
Without a primary key GORM falls back to “no primary key mode”, disabling important behaviours (updates viaSave, optimistic locking, etc.) and emitting runtime warnings.- BatchIndex uint64 `json:"batch_index" gorm:"column:batch_index"` - BatchHash string `json:"batch_hash" gorm:"column:batch_hash"` - Platform int16 `json:"platform" gorm:"column:platform"` + BatchIndex uint64 `json:"batch_index" gorm:"primaryKey;column:batch_index"` + BatchHash string `json:"batch_hash" gorm:"primaryKey;column:batch_hash"` + Platform int16 `json:"platform" gorm:"primaryKey;column:platform"`
68-70: Use map binding instead of loopingWhereto avoid SQL-injection foot-guns
db.Where(key, value)requireskeyto contain its own placeholder which is brittle and error-prone. A singledb.Where(fields)lets GORM build the placeholders safely and keeps the code shorter.-for key, value := range fields { - db = db.Where(key, value) -} +db = db.Where(fields)
🧹 Nitpick comments (2)
rollup/internal/orm/blob_upload.go (2)
44-45: Redundantdeleted_at IS NULLpredicateGORM automatically adds
deleted_at IS NULLfor soft-deleted models. Re-stating it is harmless but clutters the query builder chain and risks divergence if the column name ever changes.
72-81: Ordering clause can contradict caller’s intentionAppending
db.Order("batch_index ASC")after iteratingorderByListmeans:
- final SQL will sort by caller-supplied order(s) and then by
batch_index ASC;- if caller already ordered by
batch_index DESC, the second clause reverses the previous one’s effect due to precedence.Either prepend this default order before
orderByList, or document the override clearly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rollup/internal/orm/blob_upload.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
rollup/internal/orm/blob_upload.go (1)
common/types/db.go (3)
BlobStoragePlatform(358-358)BlobUploadStatusUploaded(339-339)BlobUploadStatus(331-331)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
rollup/internal/orm/blob_upload.go (3)
19-22: Declare the composite key explicitly to avoid GORM edge-casesNone of the fields is marked as a primary key.
GORM will therefore treat the struct as having no primary key, which disables a few ORM behaviours (e.g.Save, optimistic‐locking, cascading updates) and may even log warnings during auto-migration.
Since the table's uniqueness is(batch_index, batch_hash, platform), mark those three columns as a composite primary key so GORM can reason about identity correctly.- BatchIndex uint64 `json:"batch_index" gorm:"column:batch_index"` - BatchHash string `json:"batch_hash" gorm:"column:batch_hash"` - Platform int16 `json:"platform" gorm:"column:platform"` + BatchIndex uint64 `json:"batch_index" gorm:"primaryKey;column:batch_index"` + BatchHash string `json:"batch_hash" gorm:"primaryKey;column:batch_hash"` + Platform int16 `json:"platform" gorm:"primaryKey;column:platform"`
68-70:Where(key, value)is brittle and bypasses placeholders
db.Where(key, value)expectskeyto contain the full SQL with its own placeholders (e.g."platform = ?") – otherwise the generated SQL becomes invalid and opens room for accidental SQL-injection ifkeyis ever constructed dynamically.Refactor to let GORM build the condition:
-for key, value := range fields { - db = db.Where(key, value) -} +db = db.Where(fields)This accepts a map directly, is safer, and shorter.
98-121: Potential race condition – replace "select-then-insert" with atomic upsertBetween the
SELECTandCREATEanother goroutine/instance can insert the same(batch_index, batch_hash, platform)causing a duplicate-key error.
GORM supports atomic upsert:-var existing BlobUpload -err := db.Where("batch_index = ? AND batch_hash = ? AND platform = ? AND deleted_at IS NULL", - batchIndex, batchHash, int16(platform), -).First(&existing).Error - -if errors.Is(err, gorm.ErrRecordNotFound) { - newRecord := BlobUpload{ - BatchIndex: batchIndex, - BatchHash: batchHash, - Platform: int16(platform), - Status: int16(status), - } - if err := db.Create(&newRecord).Error; err != nil { - return fmt.Errorf("BlobUpload.InsertOrUpdateBlobUpload insert error: %w, batch index: %v, batch_hash: %v, platform: %v", err, batchIndex, batchHash, platform) - } - return nil -} else if err != nil { - return fmt.Errorf("BlobUpload.InsertOrUpdateBlobUpload query error: %w, batch index: %v, batch_hash: %v, platform: %v", err, batchIndex, batchHash, platform) -} - -if err := db.Model(&existing).Update("status", int16(status)).Error; err != nil { - return fmt.Errorf("BlobUpload.InsertOrUpdateBlobUpload update error: %w, batch index: %v, batch_hash: %v, platform: %v", err, batchIndex, batchHash, platform) -} +import "gorm.io/gorm/clause" + +newRecord := BlobUpload{ + BatchIndex: batchIndex, + BatchHash: batchHash, + Platform: int16(platform), + Status: int16(status), +} + +if err := db.Clauses(clause.OnConflict{ + Columns: []clause.Column{{Name: "batch_index"}, {Name: "batch_hash"}, {Name: "platform"}}, + DoUpdates: clause.Assignments(map[string]interface{}{"status": int16(status)}), +}).Create(&newRecord).Error; err != nil { + return fmt.Errorf("BlobUpload.InsertOrUpdateBlobUpload upsert error: %w, batch index: %v, batch_hash: %v, platform: %v", err, batchIndex, batchHash, platform) +}This removes the race and simplifies the flow.
🧹 Nitpick comments (1)
rollup/internal/orm/blob_upload.go (1)
72-80: Avoid redundant ordering by batch_indexThe method always appends
ORDER BY batch_index ASCat line 80, which could create redundant ordering ifbatch_indexis already included inorderByList. Consider checking if batch_index is already in the order list before appending.-for _, orderBy := range orderByList { - db = db.Order(orderBy) -} - -if limit > 0 { - db = db.Limit(limit) -} - -db = db.Order("batch_index ASC") +for _, orderBy := range orderByList { + db = db.Order(orderBy) +} + +// Only add batch_index ordering if not already present +hasBatchIndexOrder := false +for _, orderBy := range orderByList { + if strings.Contains(strings.ToLower(orderBy), "batch_index") { + hasBatchIndexOrder = true + break + } +} +if !hasBatchIndexOrder { + db = db.Order("batch_index ASC") +} + +if limit > 0 { + db = db.Limit(limit) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rollup/internal/controller/blob_uploader/blob_uploader.go(6 hunks)rollup/internal/orm/blob_upload.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rollup/internal/controller/blob_uploader/blob_uploader.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
rollup/internal/orm/blob_upload.go (1)
common/types/db.go (3)
BlobStoragePlatform(358-358)BlobUploadStatusUploaded(339-339)BlobUploadStatus(331-331)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
database/migrate/migrations/00027_ blob_upload.sql (2)
6-6: Specify a maximum length forbatch_hash.Using an unbounded
VARCHARcan lead to unpredictable storage and validation issues. Consider setting an appropriate length (e.g.,VARCHAR(64)) for your hash values.
6-6: Add a primary key constraint.The table currently has no primary key defined. You may enforce uniqueness and optimize lookups by making
(batch_index, batch_hash, platform)a composite primary key or by introducing a dedicatedidcolumn.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
database/migrate/migrations/00027_ blob_upload.sql(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: goimports-lint
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: goimports-lint
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: goimports-lint
🔇 Additional comments (1)
database/migrate/migrations/00027_ blob_upload.sql (1)
17-18: Validate index strategy and naming conventions.You’ve created both a unique index on
(batch_index, batch_hash, platform)and a composite index on(batch_index, batch_hash, status, platform). Please confirm that both are required for your query patterns. Also, ensure index names follow the project’s conventions consistently (e.g.,uindex_vs.idx_prefixes).Also applies to: 24-25
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
rollup/internal/orm/blob_upload.go (3)
18-23: Composite primary key still missing – GORM can’t identify a row
The struct is still missingprimaryKeytags onbatch_index,batch_hash, andplatform, exactly as flagged in a previous review. Without a PK, GORM-generatedUPDATE/DELETEstatements (see line 118) will fail or noop because it has no identifier column.
68-70: UnsafeWhere(key, value)loop – still brittle
Iterating over thefieldsmap and passing raw keys keeps the SQL-injection & syntax‐error foot-guns that were highlighted before. Let GORM build the predicate safely:-for key, value := range fields { - db = db.Where(key, value) -} +db = db.Where(fields)
98-118: Race window remains – still using select-then-insert
The earlier advice to switch to an atomicON CONFLICTupsert hasn’t been applied, so concurrent writers can still trigger duplicate-key errors. GORM’sClauses(clause.OnConflict…)solves this cleanly.
🧹 Nitpick comments (2)
rollup/internal/orm/blob_upload.go (2)
43-61: Trim query payload – select only the column you need
GetNextBatchIndexToUploadByPlatformonly usesblobUpload.BatchIndex, yet the query fetches every column. Narrow the projection to cut I/O and (de)serialization cost:-db = db.Model(&BlobUpload{}) +db = db.Model(&BlobUpload{}).Select("batch_index")or even:
var idx uint64 err := db.Pluck("batch_index", &idx).Error
98-101: Redundantdeleted_at IS NULLclause
GORM already appends the soft-delete filter for models containing aDeletedAtfield. Keeping it in the manual SQL is harmless but clutters the statement and risks diverging behaviour ifUnscoped()is ever added.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rollup/internal/controller/blob_uploader/blob_uploader.go(6 hunks)rollup/internal/orm/blob_upload.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rollup/internal/controller/blob_uploader/blob_uploader.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
rollup/internal/orm/blob_upload.go (1)
common/types/db.go (3)
BlobStoragePlatform(358-358)BlobUploadStatusUploaded(339-339)BlobUploadStatus(331-331)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
4 participants
Purpose or design rationale of this PR
Describe your change. Make sure to answer these three questions: What does this PR do? Why does it do it? How does it do it?
To add batch_hash to table blob_upload, which helps handle batch revert.
PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor