fix(marketplace): wrap DELETE+loop-CREATE in transaction
Some checks failed
Frontend CI / test (push) Failing after 0s
Some checks failed
Frontend CI / test (push) Failing after 0s
Two seller-facing mutations followed the same buggy pattern:
1. s.db.Delete(...all existing rows...) ← committed immediately
2. for range inputs { s.db.Create(new) } ← if any fails mid-loop,
deletes are already
committed → product
left in an inconsistent
state (0 images or
0 licenses) until the
seller retries.
Affected:
- Service.UpdateProductImages — 0 images = product page broken
- Service.SetProductLicenses — 0 licenses = product unsellable
Fix: wrap each function body in s.db.WithContext(ctx).Transaction,
using tx.* instead of s.db.* throughout. Rollback on any error in
the loop restores the previous images/licenses.
Side benefit: ctx is now propagated into the reads (WithContext on
the transaction root), so timeout middleware applies to the whole
sequence — previously the reads bypassed request timeouts.
Tests: ./internal/core/marketplace/ green (0.478s). go build + vet
clean.
Scope:
- Subscription service already uses Transaction() for multi-step
mutations (service.go:287, :395); its single-row Saves
(scheduleDowngrade, CancelSubscription) are atomic by nature.
- Wishlist / cart / education / discover core services audited —
no matching DELETE+LOOP-CREATE pattern found.
- Single-row mutations (AddProductPreview, UpdateProduct) don't
need wrapping — atomic in Postgres.
Refs: AUDIT_REPORT.md §4.4 "Transactions insuffisantes" + §9 #3
(critical: marketplace/service.go transactions manquantes).
Narrower than the original audit flagged — real bugs were these 2
functions, not the broader "1050+" region.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
ebf3276daa
commit
b5281bec98
1 changed files with 74 additions and 54 deletions
|
|
@ -1114,37 +1114,47 @@ func (s *Service) AddProductPreview(ctx context.Context, productID uuid.UUID, se
|
||||||
|
|
||||||
// UpdateProductImages replaces all images for a product (v0.401 M1)
|
// UpdateProductImages replaces all images for a product (v0.401 M1)
|
||||||
func (s *Service) UpdateProductImages(ctx context.Context, productID uuid.UUID, sellerID uuid.UUID, images []ProductImageInput) ([]ProductImage, error) {
|
func (s *Service) UpdateProductImages(ctx context.Context, productID uuid.UUID, sellerID uuid.UUID, images []ProductImageInput) ([]ProductImage, error) {
|
||||||
var product Product
|
// Wrap DELETE + loop-CREATE in a transaction so a failure mid-loop
|
||||||
if err := s.db.First(&product, "id = ?", productID).Error; err != nil {
|
// doesn't leave the product with zero images (the delete would
|
||||||
if errors.Is(err, gorm.ErrRecordNotFound) {
|
// otherwise already be committed).
|
||||||
return nil, ErrProductNotFound
|
var result []ProductImage
|
||||||
|
err := s.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error {
|
||||||
|
var product Product
|
||||||
|
if err := tx.First(&product, "id = ?", productID).Error; err != nil {
|
||||||
|
if errors.Is(err, gorm.ErrRecordNotFound) {
|
||||||
|
return ErrProductNotFound
|
||||||
|
}
|
||||||
|
return err
|
||||||
}
|
}
|
||||||
|
if product.SellerID != sellerID {
|
||||||
|
return ErrInvalidSeller
|
||||||
|
}
|
||||||
|
if err := tx.Where("product_id = ?", productID).Delete(&ProductImage{}).Error; err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
result = make([]ProductImage, 0, len(images))
|
||||||
|
for i, img := range images {
|
||||||
|
if img.URL == "" {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
pi := &ProductImage{
|
||||||
|
ProductID: productID,
|
||||||
|
URL: img.URL,
|
||||||
|
SortOrder: img.SortOrder,
|
||||||
|
}
|
||||||
|
if pi.SortOrder == 0 && i > 0 {
|
||||||
|
pi.SortOrder = i
|
||||||
|
}
|
||||||
|
if err := tx.Create(pi).Error; err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
result = append(result, *pi)
|
||||||
|
}
|
||||||
|
return nil
|
||||||
|
})
|
||||||
|
if err != nil {
|
||||||
return nil, err
|
return nil, err
|
||||||
}
|
}
|
||||||
if product.SellerID != sellerID {
|
|
||||||
return nil, ErrInvalidSeller
|
|
||||||
}
|
|
||||||
if err := s.db.Where("product_id = ?", productID).Delete(&ProductImage{}).Error; err != nil {
|
|
||||||
return nil, err
|
|
||||||
}
|
|
||||||
result := make([]ProductImage, 0, len(images))
|
|
||||||
for i, img := range images {
|
|
||||||
if img.URL == "" {
|
|
||||||
continue
|
|
||||||
}
|
|
||||||
pi := &ProductImage{
|
|
||||||
ProductID: productID,
|
|
||||||
URL: img.URL,
|
|
||||||
SortOrder: img.SortOrder,
|
|
||||||
}
|
|
||||||
if pi.SortOrder == 0 && i > 0 {
|
|
||||||
pi.SortOrder = i
|
|
||||||
}
|
|
||||||
if err := s.db.Create(pi).Error; err != nil {
|
|
||||||
return nil, err
|
|
||||||
}
|
|
||||||
result = append(result, *pi)
|
|
||||||
}
|
|
||||||
return result, nil
|
return result, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -1159,35 +1169,45 @@ func (s *Service) GetProductLicenses(ctx context.Context, productID uuid.UUID) (
|
||||||
|
|
||||||
// SetProductLicenses replaces all licenses for a product (v0.401 M2)
|
// SetProductLicenses replaces all licenses for a product (v0.401 M2)
|
||||||
func (s *Service) SetProductLicenses(ctx context.Context, productID uuid.UUID, sellerID uuid.UUID, licenses []ProductLicenseInput) ([]ProductLicense, error) {
|
func (s *Service) SetProductLicenses(ctx context.Context, productID uuid.UUID, sellerID uuid.UUID, licenses []ProductLicenseInput) ([]ProductLicense, error) {
|
||||||
var product Product
|
// Same DELETE+LOOP-CREATE atomicity concern as UpdateProductImages:
|
||||||
if err := s.db.First(&product, "id = ?", productID).Error; err != nil {
|
// a failure mid-loop would leave the product with zero licenses,
|
||||||
if errors.Is(err, gorm.ErrRecordNotFound) {
|
// making it unsellable until a retry.
|
||||||
return nil, ErrProductNotFound
|
var result []ProductLicense
|
||||||
|
err := s.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error {
|
||||||
|
var product Product
|
||||||
|
if err := tx.First(&product, "id = ?", productID).Error; err != nil {
|
||||||
|
if errors.Is(err, gorm.ErrRecordNotFound) {
|
||||||
|
return ErrProductNotFound
|
||||||
|
}
|
||||||
|
return err
|
||||||
}
|
}
|
||||||
|
if product.SellerID != sellerID {
|
||||||
|
return ErrInvalidSeller
|
||||||
|
}
|
||||||
|
if err := tx.Where("product_id = ?", productID).Delete(&ProductLicense{}).Error; err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
result = make([]ProductLicense, 0, len(licenses))
|
||||||
|
for _, in := range licenses {
|
||||||
|
if in.LicenseType == "" || in.PriceCents < 0 {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
pl := &ProductLicense{
|
||||||
|
ProductID: productID,
|
||||||
|
LicenseType: in.LicenseType,
|
||||||
|
PriceCents: in.PriceCents,
|
||||||
|
TermsText: in.TermsText,
|
||||||
|
}
|
||||||
|
if err := tx.Create(pl).Error; err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
result = append(result, *pl)
|
||||||
|
}
|
||||||
|
return nil
|
||||||
|
})
|
||||||
|
if err != nil {
|
||||||
return nil, err
|
return nil, err
|
||||||
}
|
}
|
||||||
if product.SellerID != sellerID {
|
|
||||||
return nil, ErrInvalidSeller
|
|
||||||
}
|
|
||||||
if err := s.db.Where("product_id = ?", productID).Delete(&ProductLicense{}).Error; err != nil {
|
|
||||||
return nil, err
|
|
||||||
}
|
|
||||||
result := make([]ProductLicense, 0, len(licenses))
|
|
||||||
for _, in := range licenses {
|
|
||||||
if in.LicenseType == "" || in.PriceCents < 0 {
|
|
||||||
continue
|
|
||||||
}
|
|
||||||
pl := &ProductLicense{
|
|
||||||
ProductID: productID,
|
|
||||||
LicenseType: in.LicenseType,
|
|
||||||
PriceCents: in.PriceCents,
|
|
||||||
TermsText: in.TermsText,
|
|
||||||
}
|
|
||||||
if err := s.db.Create(pl).Error; err != nil {
|
|
||||||
return nil, err
|
|
||||||
}
|
|
||||||
result = append(result, *pl)
|
|
||||||
}
|
|
||||||
return result, nil
|
return result, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue