Address review comments

graph/iterator.go

@@ -100,7 +100,7 @@ type Iterator interface {
 	// Contains returns whether the value is within the set held by the iterator.
 	Contains(Value) bool
 
-	// Err returns the error (if any) encountered during iteration.
+	// Err returns any error that was encountered by the Iterator.
 	Err() error
 
 	// Start iteration from the beginning

graph/iterator/all_iterator.go

@@ -106,7 +106,6 @@ func (it *Int64) Next() bool {
 }
 
 func (it *Int64) Err() error {
-	// This iterator should never error.
 	return nil
 }
 

graph/iterator/and_iterator.go

@@ -29,8 +29,8 @@ type And struct {
 	primaryIt         graph.Iterator
 	checkList         []graph.Iterator
 	result            graph.Value
-	err               error
 	runstats          graph.IteratorStats
+	err               error
 }
 
 // Creates a new And iterator.
@@ -154,9 +154,7 @@ func (it *And) Next() bool {
 			return graph.NextLogOut(it, curr, true)
 		}
 	}
-	if err := it.primaryIt.Err(); err != nil {
+	it.err = it.primaryIt.Err()
-		it.err = err
-	}
 	return graph.NextLogOut(it, nil, false)
 }
 
@@ -190,14 +188,26 @@ func (it *And) checkContainsList(val graph.Value, lastResult graph.Value) bool {
 	for i, c := range it.checkList {
 		ok = c.Contains(val)
 		if !ok {
-			if err := c.Err(); err != nil {
+			it.err = c.Err()
-				it.err = err
+			if it.err != nil {
 				return false
 			}
+
 			if lastResult != nil {
 				for j := 0; j < i; j++ {
-					// TODO(andrew-d): Should this result actually be used?
+					// One of the iterators has determined that this value doesn't
+					// match. However, the iterators that came before in the list
+					// may have returned "ok" to Contains().  We need to set all
+					// the tags back to what the previous result was -- effectively
+					// seeking back exactly one -- so we check all the prior iterators
+					// with the (already verified) result and throw away the result,
+					// which will be 'true'
 					it.checkList[j].Contains(lastResult)
+
+					it.err = it.checkList[j].Err()
+					if it.err != nil {
+						return false
+					}
 				}
 			}
 			break
@@ -254,16 +264,17 @@ func (it *And) NextPath() bool {
 	if it.primaryIt.NextPath() {
 		return true
 	}
-	if err := it.primaryIt.Err(); err != nil {
+	it.err = it.primaryIt.Err()
-		it.err = err
+	if it.err != nil {
 		return false
 	}
 	for _, sub := range it.internalIterators {
 		if sub.NextPath() {
 			return true
 		}
-		if err := sub.Err(); err != nil {
+
-			it.err = err
+		it.err = sub.Err()
+		if it.err != nil {
 			return false
 		}
 	}
@@ -275,23 +286,20 @@ func (it *And) cleanUp() {}
 
 // Close this iterator, and, by extension, close the subiterators.
 // Close should be idempotent, and it follows that if it's subiterators
-// follow this contract, the And follows the contract.
+// follow this contract, the And follows the contract.  It closes all
-//
+// subiterators it can, but returns the first error it encounters.
-// Note: as this potentially involves closing multiple subiterators, only
-// the first error encountered while closing will be reported (if any).
 func (it *And) Close() error {
 	it.cleanUp()
 
-	var ret error
+	err := it.primaryIt.Close()
-
-	ret = it.primaryIt.Close()
 	for _, sub := range it.internalIterators {
-		if err := sub.Close(); err != nil && ret != nil {
+		serr := sub.Close()
-			ret = err
+		if serr != nil && err == nil {
+			err = serr
 		}
 	}
 
-	return ret
+	return err
 }
 
 // Register this as an "and" iterator.

graph/iterator/and_iterator_test.go

@@ -142,8 +142,8 @@ func TestAllIterators(t *testing.T) {
 }
 
 func TestAndIteratorErr(t *testing.T) {
-	retErr := errors.New("unique")
+	wantErr := errors.New("unique")
-	allErr := newTestIterator(false, retErr)
+	allErr := newTestIterator(false, wantErr)
 
 	and := NewAnd()
 	and.AddSubIterator(allErr)
@@ -152,7 +152,7 @@ func TestAndIteratorErr(t *testing.T) {
 	if and.Next() != false {
 		t.Errorf("And iterator did not pass through initial 'false'")
 	}
-	if and.Err() != retErr {
+	if and.Err() != wantErr {
 		t.Errorf("And iterator did not pass through underlying Err")
 	}
 }

graph/iterator/fixed_iterator.go

@@ -146,7 +146,6 @@ func (it *Fixed) Next() bool {
 }
 
 func (it *Fixed) Err() error {
-	// This iterator should never error.
 	return nil
 }
 

graph/iterator/hasa_iterator.go

@@ -51,8 +51,8 @@ type HasA struct {
 	dir       quad.Direction
 	resultIt  graph.Iterator
 	result    graph.Value
-	err       error
 	runstats  graph.IteratorStats
+	err       error
 }
 
 // Construct a new HasA iterator, given the quad subiterator, and the quad
@@ -153,11 +153,11 @@ func (it *HasA) Contains(val graph.Value) bool {
 		it.resultIt.Close()
 	}
 	it.resultIt = it.qs.QuadIterator(it.dir, val)
-	ret := it.NextContains()
+	ok := it.NextContains()
 	if it.err != nil {
 		return false
 	}
-	return graph.ContainsLogOut(it, val, ret)
+	return graph.ContainsLogOut(it, val, ok)
 }
 
 // NextContains() is shared code between Contains() and GetNextResult() -- calls next on the
@@ -175,9 +175,7 @@ func (it *HasA) NextContains() bool {
 			return true
 		}
 	}
-	if err := it.resultIt.Err(); err != nil {
+	it.err = it.resultIt.Err()
-		it.err = err
-	}
 	return false
 }
 
@@ -193,8 +191,8 @@ func (it *HasA) NextPath() bool {
 	if it.primaryIt.NextPath() {
 		return true
 	}
-	if err := it.primaryIt.Err(); err != nil {
+	it.err = it.primaryIt.Err()
-		it.err = err
+	if it.err != nil {
 		return false
 	}
 
@@ -218,9 +216,7 @@ func (it *HasA) Next() bool {
 	it.resultIt = &Null{}
 
 	if !graph.Next(it.primaryIt) {
-		if err := it.primaryIt.Err(); err != nil {
+		it.err = it.primaryIt.Err()
-			it.err = err
-		}
 		return graph.NextLogOut(it, 0, false)
 	}
 	tID := it.primaryIt.Result()
@@ -261,21 +257,19 @@ func (it *HasA) Stats() graph.IteratorStats {
 	}
 }
 
-// Close the subiterator, the result iterator (if any) and the HasA.
+// Close the subiterator, the result iterator (if any) and the HasA. It closes
-//
+// all subiterators it can, but returns the first error it encounters.
-// Note: as this involves closing multiple iterators, only the first error
-// encountered while closing will be reported (if any).
 func (it *HasA) Close() error {
-	var ret error
+	err := it.primaryIt.Close()
 
 	if it.resultIt != nil {
-		ret = it.resultIt.Close()
+		err2 := it.resultIt.Close()
-	}
+		if err == nil {
-	if err := it.primaryIt.Close(); err != nil && ret != nil {
+			err = err2
-		ret = err
+		}
 	}
 
-	return ret
+	return err
 }
 
 // Register this iterator as a HasA.

graph/iterator/hasa_iterator_test.go

@@ -22,8 +22,8 @@ import (
 )
 
 func TestHasAIteratorErr(t *testing.T) {
-	retErr := errors.New("unique")
+	wantErr := errors.New("unique")
-	errIt := newTestIterator(false, retErr)
+	errIt := newTestIterator(false, wantErr)
 
 	// TODO(andrew-d): pass a non-nil quadstore
 	hasa := NewHasA(nil, errIt, quad.Subject)
@@ -31,7 +31,7 @@ func TestHasAIteratorErr(t *testing.T) {
 	if hasa.Next() != false {
 		t.Errorf("HasA iterator did not pass through initial 'false'")
 	}
-	if hasa.Err() != retErr {
+	if hasa.Err() != wantErr {
 		t.Errorf("HasA iterator did not pass through underlying Err")
 	}
 }

graph/iterator/linksto_iterator.go

@@ -45,8 +45,8 @@ type LinksTo struct {
 	dir       quad.Direction
 	nextIt    graph.Iterator
 	result    graph.Value
-	err       error
 	runstats  graph.IteratorStats
+	err       error
 }
 
 // Construct a new LinksTo iterator around a direction and a subiterator of
@@ -126,9 +126,7 @@ func (it *LinksTo) Contains(val graph.Value) bool {
 		it.result = val
 		return graph.ContainsLogOut(it, val, true)
 	}
-	if err := it.primaryIt.Err(); err != nil {
+	it.err = it.primaryIt.Err()
-		it.err = err
-	}
 	return graph.ContainsLogOut(it, val, false)
 }
 
@@ -169,8 +167,8 @@ func (it *LinksTo) Next() bool {
 	}
 
 	// If there's an error in the 'next' iterator, we save it and we're done.
-	if err := it.nextIt.Err(); err != nil {
+	it.err = it.nextIt.Err()
-		it.err = err
+	if it.err != nil {
 		return false
 	}
 
@@ -197,30 +195,26 @@ func (it *LinksTo) Result() graph.Value {
 	return it.result
 }
 
-// Close our subiterators.
+// Close our subiterators.  It closes all subiterators it can, but
-//
+// returns the first error it encounters.
-// Note: as this involves closing multiple subiterators, only the first error
-// encountered while closing will be reported (if any).
 func (it *LinksTo) Close() error {
-	var ret error
+	err := it.nextIt.Close()
 
-	if err := it.nextIt.Close(); err != nil && ret != nil {
+	err2 := it.primaryIt.Close()
-		ret = err
+	if err2 != nil && err == nil {
-	}
+		err = err2
-	if err := it.primaryIt.Close(); err != nil && ret != nil {
-		ret = err
 	}
 
-	return ret
+	return err
 }
 
 // We won't ever have a new result, but our subiterators might.
 func (it *LinksTo) NextPath() bool {
-	ret := it.primaryIt.NextPath()
+	ok := it.primaryIt.NextPath()
-	if !ret {
+	if !ok {
 		it.err = it.primaryIt.Err()
 	}
-	return ret
+	return ok
 }
 
 // Register the LinksTo.

graph/iterator/materialize_iterator.go

@@ -48,8 +48,8 @@ type Materialize struct {
 	subIt       graph.Iterator
 	hasRun      bool
 	aborted     bool
-	err         error
 	runstats    graph.IteratorStats
+	err         error
 }
 
 func NewMaterialize(sub graph.Iterator) *Materialize {
@@ -206,9 +206,7 @@ func (it *Materialize) Next() bool {
 	}
 	if it.aborted {
 		n := graph.Next(it.subIt)
-		if err := it.subIt.Err(); err != nil {
+		it.err = it.subIt.Err()
-			it.err = err
-		}
 		return n
 	}
 
@@ -302,9 +300,8 @@ func (it *Materialize) materializeSet() {
 			it.actualSize += 1
 		}
 	}
-	if err := it.subIt.Err(); err != nil {
+	it.err = it.subIt.Err()
-		it.err = err
+	if it.err == nil && it.aborted {
-	} else if it.aborted {
 		if glog.V(2) {
 			glog.V(2).Infoln("Aborting subiterator")
 		}

graph/iterator/materialize_iterator_test.go

@@ -17,13 +17,11 @@ package iterator
 import (
 	"errors"
 	"testing"
-
-	//"github.com/google/cayley/graph"
 )
 
 func TestMaterializeIteratorError(t *testing.T) {
-	retErr := errors.New("unique")
+	wantErr := errors.New("unique")
-	errIt := newTestIterator(false, retErr)
+	errIt := newTestIterator(false, wantErr)
 
 	// This tests that we properly return 0 results and the error when the
 	// underlying iterator returns an error.
@@ -32,14 +30,14 @@ func TestMaterializeIteratorError(t *testing.T) {
 	if mIt.Next() != false {
 		t.Errorf("Materialize iterator did not pass through underlying 'false'")
 	}
-	if mIt.Err() != retErr {
+	if mIt.Err() != wantErr {
 		t.Errorf("Materialize iterator did not pass through underlying Err")
 	}
 }
 
 func TestMaterializeIteratorErrorAbort(t *testing.T) {
-	retErr := errors.New("unique")
+	wantErr := errors.New("unique")
-	errIt := newTestIterator(false, retErr)
+	errIt := newTestIterator(false, wantErr)
 
 	// This tests that we properly return 0 results and the error when the
 	// underlying iterator is larger than our 'abort at' value, and then
@@ -50,7 +48,7 @@ func TestMaterializeIteratorErrorAbort(t *testing.T) {
 
 	mIt := NewMaterialize(or)
 
-	// Should get all the underlying values...
+	// We should get all the underlying values...
 	for i := 0; i < abortMaterializeAt+1; i++ {
 		if !mIt.Next() {
 			t.Errorf("Materialize iterator returned spurious 'false' on iteration %d", i)
@@ -62,11 +60,11 @@ func TestMaterializeIteratorErrorAbort(t *testing.T) {
 		}
 	}
 
-	// ... and then the error value
+	// ... and then the error value.
 	if mIt.Next() != false {
 		t.Errorf("Materialize iterator did not pass through underlying 'false'")
 	}
-	if mIt.Err() != retErr {
+	if mIt.Err() != wantErr {
 		t.Errorf("Materialize iterator did not pass through underlying Err")
 	}
 }

graph/iterator/not_iterator.go

@@ -12,8 +12,8 @@ type Not struct {
 	primaryIt graph.Iterator
 	allIt     graph.Iterator
 	result    graph.Value
-	err       error
 	runstats  graph.IteratorStats
+	err       error
 }
 
 func NewNot(primaryIt, allIt graph.Iterator) *Not {
@@ -88,9 +88,7 @@ func (it *Not) Next() bool {
 			return graph.NextLogOut(it, curr, true)
 		}
 	}
-	if err := it.allIt.Err(); err != nil {
+	it.err = it.allIt.Err()
-		it.err = err
-	}
 	return graph.NextLogOut(it, nil, false)
 }
 
@@ -113,9 +111,8 @@ func (it *Not) Contains(val graph.Value) bool {
 		return graph.ContainsLogOut(it, val, false)
 	}
 
-	if err := it.primaryIt.Err(); err != nil {
+	it.err = it.primaryIt.Err()
-		it.err = err
+	if it.err != nil {
-
 		// Explicitly return 'false', since an error occurred.
 		return false
 	}
@@ -130,19 +127,17 @@ func (it *Not) NextPath() bool {
 	return false
 }
 
-// Close closes the primary and all iterators.  If an error occurs, only the
+// Close closes the primary and all iterators.  It closes all subiterators
-// first one will be returned.
+// it can, but returns the first error it encounters.
 func (it *Not) Close() error {
-	var ret error
+	err := it.primaryIt.Close()
 
-	if err := it.primaryIt.Close(); err != nil && ret != nil {
+	err2 := it.allIt.Close()
-		ret = err
+	if err2 != nil && err == nil {
-	}
+		err = err2
-	if err := it.allIt.Close(); err != nil && ret != nil {
-		ret = err
 	}
 
-	return ret
+	return err
 }
 
 func (it *Not) Type() graph.Type { return graph.Not }

graph/iterator/not_iterator_test.go

@@ -45,8 +45,8 @@ func TestNotIteratorBasics(t *testing.T) {
 }
 
 func TestNotIteratorErr(t *testing.T) {
-	retErr := errors.New("unique")
+	wantErr := errors.New("unique")
-	allIt := newTestIterator(false, retErr)
+	allIt := newTestIterator(false, wantErr)
 
 	toComplementIt := NewFixed(Identity)
 
@@ -55,7 +55,7 @@ func TestNotIteratorErr(t *testing.T) {
 	if not.Next() != false {
 		t.Errorf("Not iterator did not pass through initial 'false'")
 	}
-	if not.Err() != retErr {
+	if not.Err() != wantErr {
 		t.Errorf("Not iterator did not pass through underlying Err")
 	}
 }

graph/iterator/optional_iterator.go

@@ -90,11 +90,11 @@ func (it *Optional) Result() graph.Value {
 // optional subbranch.
 func (it *Optional) NextPath() bool {
 	if it.lastCheck {
-		ret := it.subIt.NextPath()
+		ok := it.subIt.NextPath()
-		if !ret {
+		if !ok {
 			it.err = it.subIt.Err()
 		}
-		return ret
+		return ok
 	}
 	return false
 }

graph/iterator/or_iterator.go

@@ -148,8 +148,8 @@ func (it *Or) Next() bool {
 			return graph.NextLogOut(it, it.result, true)
 		}
 
-		if err := curIt.Err(); err != nil {
+		it.err = curIt.Err()
-			it.err = err
+		if it.err != nil {
 			return graph.NextLogOut(it, nil, false)
 		}
 
@@ -182,7 +182,9 @@ func (it *Or) subItsContain(val graph.Value) (bool, error) {
 			it.currentIterator = i
 			break
 		}
-		if err := sub.Err(); err != nil {
+
+		err := sub.Err()
+		if err != nil {
 			return false, err
 		}
 	}
@@ -238,11 +240,11 @@ func (it *Or) Size() (int64, bool) {
 func (it *Or) NextPath() bool {
 	if it.currentIterator != -1 {
 		currIt := it.internalIterators[it.currentIterator]
-		ret := currIt.NextPath()
+		ok := currIt.NextPath()
-		if !ret {
+		if !ok {
 			it.err = currIt.Err()
 		}
-		return ret
+		return ok
 	}
 	return false
 }
@@ -252,16 +254,15 @@ func (it *Or) cleanUp() {}
 
 // Close this graph.iterator, and, by extension, close the subiterators.
 // Close should be idempotent, and it follows that if it's subiterators
-// follow this contract, the Or follows the contract.
+// follow this contract, the Or follows the contract.  It closes all
-//
+// subiterators it can, but returns the first error it encounters.
-// Note: as this potentially involves closing multiple subiterators, only
-// the first error encountered while closing will be reported (if any).
 func (it *Or) Close() error {
 	it.cleanUp()
 
 	var ret error
 	for _, sub := range it.internalIterators {
-		if err := sub.Close(); err != nil && ret != nil {
+		err := sub.Close()
+		if err != nil && ret == nil {
 			ret = err
 		}
 	}

graph/iterator/or_iterator_test.go

@@ -151,8 +151,8 @@ func TestShortCircuitingOrBasics(t *testing.T) {
 }
 
 func TestOrIteratorErr(t *testing.T) {
-	retErr := errors.New("unique")
+	wantErr := errors.New("unique")
-	orErr := newTestIterator(false, retErr)
+	orErr := newTestIterator(false, wantErr)
 
 	fix1 := NewFixed(Identity)
 	fix1.Add(1)
@@ -172,14 +172,14 @@ func TestOrIteratorErr(t *testing.T) {
 	if or.Next() != false {
 		t.Errorf("Or iterator did not pass through underlying 'false'")
 	}
-	if or.Err() != retErr {
+	if or.Err() != wantErr {
 		t.Errorf("Or iterator did not pass through underlying Err")
 	}
 }
 
 func TestShortCircuitOrIteratorErr(t *testing.T) {
-	retErr := errors.New("unique")
+	wantErr := errors.New("unique")
-	orErr := newTestIterator(false, retErr)
+	orErr := newTestIterator(false, wantErr)
 
 	or := NewOr()
 	or.AddSubIterator(orErr)
@@ -188,7 +188,7 @@ func TestShortCircuitOrIteratorErr(t *testing.T) {
 	if or.Next() != false {
 		t.Errorf("Or iterator did not pass through underlying 'false'")
 	}
-	if or.Err() != retErr {
+	if or.Err() != wantErr {
 		t.Errorf("Or iterator did not pass through underlying Err")
 	}
 }

graph/iterator/value_comparison_iterator.go

@@ -132,9 +132,7 @@ func (it *Comparison) Next() bool {
 			return true
 		}
 	}
-	if err := it.subIt.Err(); err != nil {
+	it.err = it.subIt.Err()
-		it.err = err
-	}
 	return false
 }
 
@@ -175,11 +173,11 @@ func (it *Comparison) Contains(val graph.Value) bool {
 	if !it.doComparison(val) {
 		return false
 	}
-	ret := it.subIt.Contains(val)
+	ok := it.subIt.Contains(val)
-	if !ret {
+	if !ok {
 		it.err = it.subIt.Err()
 	}
-	return ret
+	return ok
 }
 
 // If we failed the check, then the subiterator should not contribute to the result

graph/iterator/value_comparison_iterator_test.go

@@ -121,15 +121,15 @@ func TestVCIContains(t *testing.T) {
 }
 
 func TestComparisonIteratorErr(t *testing.T) {
-	retErr := errors.New("unique")
+	wantErr := errors.New("unique")
-	errIt := newTestIterator(false, retErr)
+	errIt := newTestIterator(false, wantErr)
 
 	vc := NewComparison(errIt, compareLT, int64(2), simpleStore)
 
 	if vc.Next() != false {
 		t.Errorf("Comparison iterator did not pass through initial 'false'")
 	}
-	if vc.Err() != retErr {
+	if vc.Err() != wantErr {
 		t.Errorf("Comparison iterator did not pass through underlying Err")
 	}
 }

graph/primarykey.go

@@ -84,7 +84,7 @@ func (p *PrimaryKey) Int() int64 {
 	case sequential:
 		return p.sequentialID
 	case unique:
-		msg := "UUID cannot be cast to an int64"
+		msg := "UUID cannot be converted to an int64"
 		glog.Errorln(msg)
 		panic(msg)
 	}

writer/single.go

@@ -32,8 +32,11 @@ type Single struct {
 }
 
 func NewSingleReplication(qs graph.QuadStore, opts graph.Options) (graph.QuadWriter, error) {
-	var ignoreMissing, ignoreDuplicate bool
+	var (
-	var err error
+		ignoreMissing   bool
+		ignoreDuplicate bool
+		err             error
+	)
 
 	if *graph.IgnoreMissing {
 		ignoreMissing = true