update netaddr #50
update netaddr #50
Conversation
…error handling paradigms
Codecov Report
@@ Coverage Diff @@
## master #50 +/- ##
==========================================
- Coverage 76.73% 76.27% -0.47%
==========================================
Files 7 6 -1
Lines 546 548 +2
==========================================
- Hits 419 418 -1
Misses 68 68
- Partials 59 62 +3
Continue to review full report at Codecov.
|
sql.go
Outdated
| @@ -133,9 +133,9 @@ func (s *sql) UpdatePrefix(prefix Prefix) (Prefix, error) { | |||
| if rows == 0 { | |||
| err := tx.Rollback() | |||
| if err != nil { | |||
| return Prefix{}, newOptimisticLockError("select for update did not effect any row, but rollback did not work:" + err.Error()) | |||
| return Prefix{}, fmt.Errorf("select for update did not effect any row, but rollback did not work:%w", err) | |||
There was a problem hiding this comment.
This changes the contract, because now the error is no longer the OptimisticLockError but the Error seen at Rollback. The client should really see the OptimisticLockError in my opinion.
We could even remove the rollback since if zero rows had been are affected this makes no sense to rollback "no changes".
There was a problem hiding this comment.
still do the rollback, but error handling is removed.
sql.go
Outdated
| @@ -145,17 +145,17 @@ func (s *sql) UpdatePrefix(prefix Prefix) (Prefix, error) { | |||
| if rows == 0 { | |||
| err := tx.Rollback() | |||
| if err != nil { | |||
| return Prefix{}, newOptimisticLockError("updatePrefix did not effect any row, but rollback did not work:" + err.Error()) | |||
| return Prefix{}, fmt.Errorf("updatePrefix did not effect any row, but rollback did not work:%w", err) | |||
There was a problem hiding this comment.
This changes the contract, because now the error is no longer the OptimisticLockError but the Error seen at Rollback. The client should really see the OptimisticLockError in my opinion.
Could even remove the rollback, see above.
sql.go
Outdated
| } | ||
| return prefix, tx.Commit() | ||
| } | ||
|
|
||
| func (s *sql) DeletePrefix(prefix Prefix) (Prefix, error) { | ||
| tx, err := s.db.Beginx() | ||
| if err != nil { | ||
| return Prefix{}, fmt.Errorf("unable to start transaction:%v", err) | ||
| return Prefix{}, fmt.Errorf("unable to start transaction:%w", err) | ||
| } | ||
| tx.MustExec("DELETE from prefixes WHERE cidr=$1", prefix.Cidr) |
There was a problem hiding this comment.
Whoa, we should really replace this tx.MustExec and do proper error handling here, since MustExec panics if it fails!
There was a problem hiding this comment.
Whoa good catch, we have mustExec everywhere.
There was a problem hiding this comment.
All MustExec are remove now, this was a big mistake in the past.
github.com/pkg/errorswith fmt.Errorfgolangci-lint run -p bugto pass