Skip to content

Commit

Permalink
Make TryInsert functions within the packages module try to insert first
Browse files Browse the repository at this point in the history
The TryInsert* functions within the packages models make incorrect assumptions
about transactional isolation within most databases. It is perfectly possible for
a SELECT to return nothing but an INSERT fail with a duplicate in most DBs as it
is only INSERT that the locking occurs.

This PR changes the code to simply try to insert first and if there is an error
then attempt to SELECT from the table. If the SELECT works then the INSERT error
is assumed to have been related to the unique constraint failure.

This technique avoids us having to parse the error returned from the DBs as
these are varied and different.

If the SELECT fails then the INSERT error is returned to the user.

Fix go-gitea#19586

Signed-off-by: Andrew Thornton <art27@cantab.net>
  • Loading branch information
zeripath committed Sep 4, 2022
1 parent 3963625 commit 7f4e851
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 24 deletions.
13 changes: 5 additions & 8 deletions models/packages/package.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,14 +128,11 @@ func TryInsertPackage(ctx context.Context, p *Package) (*Package, error) {
LowerName: p.LowerName,
}

has, err := e.Get(key)
if err != nil {
return nil, err
}
if has {
return key, ErrDuplicatePackage
}
if _, err = e.Insert(p); err != nil {
if _, err := e.Insert(p); err != nil {
// Try to get the key again
if has, _ := e.Get(key); has {
return key, ErrDuplicatePackage
}
return nil, err
}
return p, nil
Expand Down
12 changes: 4 additions & 8 deletions models/packages/package_file.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,10 @@ func TryInsertFile(ctx context.Context, pf *PackageFile) (*PackageFile, error) {
CompositeKey: pf.CompositeKey,
}

has, err := e.Get(key)
if err != nil {
return nil, err
}
if has {
return pf, ErrDuplicatePackageFile
}
if _, err = e.Insert(pf); err != nil {
if _, err := e.Insert(pf); err != nil {
if has, _ := e.Get(key); has {
return pf, ErrDuplicatePackageFile
}
return nil, err
}
return pf, nil
Expand Down
12 changes: 4 additions & 8 deletions models/packages/package_version.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,10 @@ func GetOrInsertVersion(ctx context.Context, pv *PackageVersion) (*PackageVersio
LowerVersion: pv.LowerVersion,
}

has, err := e.Get(key)
if err != nil {
return nil, err
}
if has {
return key, ErrDuplicatePackageVersion
}
if _, err = e.Insert(pv); err != nil {
if _, err := e.Insert(pv); err != nil {
if has, _ := e.Get(key); has {
return key, ErrDuplicatePackageVersion
}
return nil, err
}
return pv, nil
Expand Down

0 comments on commit 7f4e851

Please sign in to comment.