Fix error handling (treeverse#1247)

guy-har · web-flow · commit f7f7dcd08087 · 2021-01-20T15:18:13.000+02:00
diff --git a/api/api_controller.go b/api/api_controller.go
@@ -748,19 +748,21 @@ func (c *Controller) MergeMergeIntoBranchHandler() refs.MergeIntoBranchHandler {
 			message,
 			metadata)
 
-		switch err {
-		case nil:
+		switch {
+		case err == nil:
 			payload := newMergeResultFromCatalog(res)
 			return refs.NewMergeIntoBranchOK().WithPayload(payload)
-		case catalog.ErrUnsupportedRelation:
+		case errors.Is(err, catalog.ErrUnsupportedRelation):
 			return refs.NewMergeIntoBranchDefault(http.StatusInternalServerError).WithPayload(responseError("branches have no common base"))
-		case catalog.ErrBranchNotFound:
+		case errors.Is(err, catalog.ErrBranchNotFound) || errors.Is(err, graveler.ErrBranchNotFound):
 			return refs.NewMergeIntoBranchDefault(http.StatusInternalServerError).WithPayload(responseError("a branch does not exist "))
-		case catalog.ErrConflictFound:
+		case errors.Is(err, catalog.ErrConflictFound) || errors.Is(err, graveler.ErrConflictFound):
 			payload := newMergeResultFromCatalog(res)
 			return refs.NewMergeIntoBranchConflict().WithPayload(payload)
-		case catalog.ErrNoDifferenceWasFound:
+		case errors.Is(err, catalog.ErrNoDifferenceWasFound):
 			return refs.NewMergeIntoBranchDefault(http.StatusInternalServerError).WithPayload(responseError("no difference was found"))
+		case errors.Is(err, graveler.ErrLockNotAcquired):
+			return refs.NewMergeIntoBranchDefault(http.StatusInternalServerError).WithPayload(responseError("branch is currently locked, try again later"))
 		default:
 			return refs.NewMergeIntoBranchDefault(http.StatusInternalServerError).WithPayload(responseError("internal error"))
 		}
diff --git a/graveler/committed/apply.go b/graveler/committed/apply.go
@@ -71,6 +71,12 @@ func Apply(ctx context.Context, writer MetaRangeWriter, source Iterator, diffs g
 			haveSource = source.Next()
 		}
 	}
+	if err := source.Err(); err != nil {
+		return err
+	}
+	if err := diffs.Err(); err != nil {
+		return err
+	}
 	for haveSource {
 		sourceValue, sourceRange := source.Value()
 		if sourceValue == nil {
@@ -94,6 +100,9 @@ func Apply(ctx context.Context, writer MetaRangeWriter, source Iterator, diffs g
 			haveSource = source.Next()
 		}
 	}
+	if err := source.Err(); err != nil {
+		return err
+	}
 	for haveDiffs {
 		diffValue := diffs.Value()
 		haveDiffs = diffs.Next()
@@ -112,5 +121,5 @@ func Apply(ctx context.Context, writer MetaRangeWriter, source Iterator, diffs g
 			return fmt.Errorf("write added record: %w", err)
 		}
 	}
-	return nil
+	return diffs.Err()
 }
diff --git a/graveler/committed/iterator.go b/graveler/committed/iterator.go
@@ -67,6 +67,9 @@ func (rvi *iterator) NextRange() bool {
 }
 
 func (rvi *iterator) Next() bool {
+	if rvi.err != nil {
+		return false
+	}
 	if !rvi.started {
 		rvi.started = true
 		return rvi.NextRange()
diff --git a/graveler/committed/merge_iterator.go b/graveler/committed/merge_iterator.go
@@ -21,24 +21,31 @@ func NewMergeIterator(diffTheirsToOurs graveler.DiffIterator, base Iterator) (*m
 	return &mergeIterator{diffIt: diffTheirsToOurs, base: base}, nil
 }
 
-func (d *mergeIterator) valueFromBase(key graveler.Key) *graveler.ValueRecord {
+func (d *mergeIterator) valueFromBase(key graveler.Key) (*graveler.ValueRecord, error) {
 	d.base.SeekGE(key)
 	var val *graveler.ValueRecord
 	for d.base.Next() && val == nil {
 		val, _ = d.base.Value()
 	}
+	if err := d.base.Err(); err != nil {
+		return nil, err
+	}
 	if val == nil || !bytes.Equal(val.Key, key) {
-		return nil
+		return nil, nil
 	}
-	return val
+	return val, nil
 }
 
 func (d *mergeIterator) Next() bool {
 	for d.diffIt.Next() {
 		val := d.diffIt.Value()
 		key := val.Key
 		typ := val.Type
-		baseVal := d.valueFromBase(key)
+		baseVal, err := d.valueFromBase(key)
+		if err != nil {
+			d.err = err
+			return false
+		}
 		switch typ {
 		case graveler.DiffTypeAdded:
 			// exists on ours, but not on theirs
@@ -87,10 +94,12 @@ func (d *mergeIterator) Next() bool {
 				}
 				// changed on theirs, removed on ours
 				d.err = graveler.ErrConflictFound
+				return false
 			}
 			// added on theirs, but not on ours - continue
 		}
 	}
+	d.err = d.diffIt.Err()
 	return false
 }
 
@@ -101,6 +110,9 @@ func (d *mergeIterator) SeekGE(id graveler.Key) {
 }
 
 func (d *mergeIterator) Value() *graveler.ValueRecord {
+	if d.err != nil {
+		return nil
+	}
 	return d.val
 }
 

Original file line number	Diff line number	Diff line change
`@@ -71,6 +71,12 @@ func Apply(ctx context.Context, writer MetaRangeWriter, source Iterator, diffs g`
`71`	`71`	`haveSource = source.Next()`
`72`	`72`	`}`
`73`	`73`	`}`
	`74`	`+ if err := source.Err(); err != nil {`
	`75`	`+ return err`
	`76`	`+ }`
	`77`	`+ if err := diffs.Err(); err != nil {`
	`78`	`+ return err`
	`79`	`+ }`
`74`	`80`	`for haveSource {`
`75`	`81`	`sourceValue, sourceRange := source.Value()`
`76`	`82`	`if sourceValue == nil {`
`@@ -94,6 +100,9 @@ func Apply(ctx context.Context, writer MetaRangeWriter, source Iterator, diffs g`
`94`	`100`	`haveSource = source.Next()`
`95`	`101`	`}`
`96`	`102`	`}`
	`103`	`+ if err := source.Err(); err != nil {`
	`104`	`+ return err`
	`105`	`+ }`
`97`	`106`	`for haveDiffs {`
`98`	`107`	`diffValue := diffs.Value()`
`99`	`108`	`haveDiffs = diffs.Next()`
`@@ -112,5 +121,5 @@ func Apply(ctx context.Context, writer MetaRangeWriter, source Iterator, diffs g`
`112`	`121`	`return fmt.Errorf("write added record: %w", err)`
`113`	`122`	`}`
`114`	`123`	`}`
`115`		`- return nil`
	`124`	`+ return diffs.Err()`
`116`	`125`	`}`
Original file line number	Diff line number	Diff line change
`@@ -67,6 +67,9 @@ func (rvi *iterator) NextRange() bool {`
`67`	`67`	`}`
`68`	`68`
`69`	`69`	`func (rvi *iterator) Next() bool {`
	`70`	`+ if rvi.err != nil {`
	`71`	`+ return false`
	`72`	`+ }`
`70`	`73`	`if !rvi.started {`
`71`	`74`	`rvi.started = true`
`72`	`75`	`return rvi.NextRange()`
Original file line number	Diff line number	Diff line change
`@@ -21,24 +21,31 @@ func NewMergeIterator(diffTheirsToOurs graveler.DiffIterator, base Iterator) (*m`
`21`	`21`	`return &mergeIterator{diffIt: diffTheirsToOurs, base: base}, nil`
`22`	`22`	`}`
`23`	`23`
`24`		`-func (d mergeIterator) valueFromBase(key graveler.Key) graveler.ValueRecord {`
	`24`	`+func (d mergeIterator) valueFromBase(key graveler.Key) (graveler.ValueRecord, error) {`
`25`	`25`	`d.base.SeekGE(key)`
`26`	`26`	`var val *graveler.ValueRecord`
`27`	`27`	`for d.base.Next() && val == nil {`
`28`	`28`	`val, _ = d.base.Value()`
`29`	`29`	`}`
	`30`	`+ if err := d.base.Err(); err != nil {`
	`31`	`+ return nil, err`
	`32`	`+ }`
`30`	`33`	`if val == nil \|\| !bytes.Equal(val.Key, key) {`
`31`		`- return nil`
	`34`	`+ return nil, nil`
`32`	`35`	`}`
`33`		`- return val`
	`36`	`+ return val, nil`
`34`	`37`	`}`
`35`	`38`
`36`	`39`	`func (d *mergeIterator) Next() bool {`
`37`	`40`	`for d.diffIt.Next() {`
`38`	`41`	`val := d.diffIt.Value()`
`39`	`42`	`key := val.Key`
`40`	`43`	`typ := val.Type`
`41`		`- baseVal := d.valueFromBase(key)`
	`44`	`+ baseVal, err := d.valueFromBase(key)`
	`45`	`+ if err != nil {`
	`46`	`+ d.err = err`
	`47`	`+ return false`
	`48`	`+ }`
`42`	`49`	`switch typ {`
`43`	`50`	`case graveler.DiffTypeAdded:`
`44`	`51`	`// exists on ours, but not on theirs`
`@@ -87,10 +94,12 @@ func (d *mergeIterator) Next() bool {`
`87`	`94`	`}`
`88`	`95`	`// changed on theirs, removed on ours`
`89`	`96`	`d.err = graveler.ErrConflictFound`
	`97`	`+ return false`
`90`	`98`	`}`
`91`	`99`	`// added on theirs, but not on ours - continue`
`92`	`100`	`}`
`93`	`101`	`}`
	`102`	`+ d.err = d.diffIt.Err()`
`94`	`103`	`return false`
`95`	`104`	`}`
`96`	`105`
`@@ -101,6 +110,9 @@ func (d *mergeIterator) SeekGE(id graveler.Key) {`
`101`	`110`	`}`
`102`	`111`
`103`	`112`	`func (d mergeIterator) Value() graveler.ValueRecord {`
	`113`	`+ if d.err != nil {`
	`114`	`+ return nil`
	`115`	`+ }`
`104`	`116`	`return d.val`
`105`	`117`	`}`
`106`	`118`