From 6aad2b18184f99a1208ee433ef0fc14d67217ed4 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 14 Apr 2015 16:26:55 -0700 Subject: [PATCH 01/34] Add static type assertions Assert that various iterators satisfy the graph.Nexter interface --- graph/bolt/all_iterator.go | 2 ++ graph/bolt/iterator.go | 2 ++ graph/gaedatastore/iterator.go | 2 ++ graph/iterator/all_iterator.go | 2 ++ graph/iterator/and_iterator.go | 2 ++ graph/iterator/fixed_iterator.go | 2 ++ graph/iterator/hasa_iterator.go | 2 ++ graph/iterator/iterator.go | 2 ++ graph/iterator/linksto_iterator.go | 2 ++ graph/iterator/materialize_iterator.go | 2 ++ graph/iterator/not_iterator.go | 2 ++ graph/iterator/or_iterator.go | 2 ++ graph/iterator/value_comparison_iterator.go | 2 ++ graph/leveldb/all_iterator.go | 2 ++ graph/leveldb/iterator.go | 2 ++ graph/memstore/all_iterator.go | 3 +++ graph/memstore/iterator.go | 2 ++ graph/mongo/iterator.go | 2 ++ 18 files changed, 37 insertions(+) diff --git a/graph/bolt/all_iterator.go b/graph/bolt/all_iterator.go index 4cd017e..2c8d379 100644 --- a/graph/bolt/all_iterator.go +++ b/graph/bolt/all_iterator.go @@ -204,3 +204,5 @@ func (it *AllIterator) Stats() graph.IteratorStats { Size: s, } } + +var _ graph.Nexter = &AllIterator{} diff --git a/graph/bolt/iterator.go b/graph/bolt/iterator.go index efb11d6..b7df4e9 100644 --- a/graph/bolt/iterator.go +++ b/graph/bolt/iterator.go @@ -316,3 +316,5 @@ func (it *Iterator) Stats() graph.IteratorStats { Size: s, } } + +var _ graph.Nexter = &Iterator{} diff --git a/graph/gaedatastore/iterator.go b/graph/gaedatastore/iterator.go index ffdc8fe..8b8339d 100644 --- a/graph/gaedatastore/iterator.go +++ b/graph/gaedatastore/iterator.go @@ -323,3 +323,5 @@ func (it *Iterator) Stats() graph.IteratorStats { Size: size, } } + +var _ graph.Nexter = &Iterator{} diff --git a/graph/iterator/all_iterator.go b/graph/iterator/all_iterator.go index d937ce4..31d4b8e 100644 --- a/graph/iterator/all_iterator.go +++ b/graph/iterator/all_iterator.go @@ -160,3 +160,5 @@ func (it *Int64) Stats() graph.IteratorStats { Contains: it.runstats.Contains, } } + +var _ graph.Nexter = &Int64{} diff --git a/graph/iterator/and_iterator.go b/graph/iterator/and_iterator.go index 3764600..d8d45e5 100644 --- a/graph/iterator/and_iterator.go +++ b/graph/iterator/and_iterator.go @@ -265,3 +265,5 @@ func (it *And) Close() { // Register this as an "and" iterator. func (it *And) Type() graph.Type { return graph.And } + +var _ graph.Nexter = &And{} diff --git a/graph/iterator/fixed_iterator.go b/graph/iterator/fixed_iterator.go index 4a8afb9..fbfd310 100644 --- a/graph/iterator/fixed_iterator.go +++ b/graph/iterator/fixed_iterator.go @@ -186,3 +186,5 @@ func (it *Fixed) Stats() graph.IteratorStats { Size: int64(len(it.values)), } } + +var _ graph.Nexter = &Fixed{} diff --git a/graph/iterator/hasa_iterator.go b/graph/iterator/hasa_iterator.go index 3788b40..adac2c8 100644 --- a/graph/iterator/hasa_iterator.go +++ b/graph/iterator/hasa_iterator.go @@ -252,3 +252,5 @@ func (it *HasA) Type() graph.Type { return graph.HasA } func (it *HasA) Size() (int64, bool) { return it.Stats().Size, false } + +var _ graph.Nexter = &HasA{} diff --git a/graph/iterator/iterator.go b/graph/iterator/iterator.go index 954ddba..6495de0 100644 --- a/graph/iterator/iterator.go +++ b/graph/iterator/iterator.go @@ -116,3 +116,5 @@ func (it *Null) Close() {} func (it *Null) Stats() graph.IteratorStats { return graph.IteratorStats{} } + +var _ graph.Nexter = &Null{} diff --git a/graph/iterator/linksto_iterator.go b/graph/iterator/linksto_iterator.go index 99a8df5..cc1cfa5 100644 --- a/graph/iterator/linksto_iterator.go +++ b/graph/iterator/linksto_iterator.go @@ -214,3 +214,5 @@ func (it *LinksTo) Stats() graph.IteratorStats { func (it *LinksTo) Size() (int64, bool) { return it.Stats().Size, false } + +var _ graph.Nexter = &LinksTo{} diff --git a/graph/iterator/materialize_iterator.go b/graph/iterator/materialize_iterator.go index 00b2102..f771e89 100644 --- a/graph/iterator/materialize_iterator.go +++ b/graph/iterator/materialize_iterator.go @@ -293,3 +293,5 @@ func (it *Materialize) materializeSet() { } it.hasRun = true } + +var _ graph.Nexter = &Materialize{} diff --git a/graph/iterator/not_iterator.go b/graph/iterator/not_iterator.go index db394f8..3a3a4c4 100644 --- a/graph/iterator/not_iterator.go +++ b/graph/iterator/not_iterator.go @@ -161,3 +161,5 @@ func (it *Not) Describe() graph.Description { Iterators: subIts, } } + +var _ graph.Nexter = &Not{} diff --git a/graph/iterator/or_iterator.go b/graph/iterator/or_iterator.go index 6c58238..3c07b18 100644 --- a/graph/iterator/or_iterator.go +++ b/graph/iterator/or_iterator.go @@ -289,3 +289,5 @@ func (it *Or) Stats() graph.IteratorStats { // Register this as an "or" graph.iterator. func (it *Or) Type() graph.Type { return graph.Or } + +var _ graph.Nexter = &Or{} diff --git a/graph/iterator/value_comparison_iterator.go b/graph/iterator/value_comparison_iterator.go index 627b853..b1a0842 100644 --- a/graph/iterator/value_comparison_iterator.go +++ b/graph/iterator/value_comparison_iterator.go @@ -218,3 +218,5 @@ func (it *Comparison) Stats() graph.IteratorStats { func (it *Comparison) Size() (int64, bool) { return 0, true } + +var _ graph.Nexter = &Comparison{} diff --git a/graph/leveldb/all_iterator.go b/graph/leveldb/all_iterator.go index 515133f..3a118f9 100644 --- a/graph/leveldb/all_iterator.go +++ b/graph/leveldb/all_iterator.go @@ -183,3 +183,5 @@ func (it *AllIterator) Stats() graph.IteratorStats { Size: s, } } + +var _ graph.Nexter = &AllIterator{} diff --git a/graph/leveldb/iterator.go b/graph/leveldb/iterator.go index 6ccd3fe..dcc4aeb 100644 --- a/graph/leveldb/iterator.go +++ b/graph/leveldb/iterator.go @@ -283,3 +283,5 @@ func (it *Iterator) Stats() graph.IteratorStats { Size: s, } } + +var _ graph.Nexter = &Iterator{} diff --git a/graph/memstore/all_iterator.go b/graph/memstore/all_iterator.go index 2d78ead..26cb643 100644 --- a/graph/memstore/all_iterator.go +++ b/graph/memstore/all_iterator.go @@ -69,3 +69,6 @@ func (it *quadsAllIterator) Next() bool { } return out } + +var _ graph.Nexter = &nodesAllIterator{} +var _ graph.Nexter = &quadsAllIterator{} diff --git a/graph/memstore/iterator.go b/graph/memstore/iterator.go index d79e6c8..d93ba62 100644 --- a/graph/memstore/iterator.go +++ b/graph/memstore/iterator.go @@ -191,3 +191,5 @@ func (it *Iterator) Stats() graph.IteratorStats { Size: int64(it.tree.Len()), } } + +var _ graph.Nexter = &Iterator{} diff --git a/graph/mongo/iterator.go b/graph/mongo/iterator.go index 876dff8..bdd3bab 100644 --- a/graph/mongo/iterator.go +++ b/graph/mongo/iterator.go @@ -230,3 +230,5 @@ func (it *Iterator) Stats() graph.IteratorStats { Size: size, } } + +var _ graph.Nexter = &Iterator{} From e07838857f4f4bee37e0a7b54313c80770b2e8a7 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 14 Apr 2015 16:33:32 -0700 Subject: [PATCH 02/34] Add Err() method to Nexter interface --- graph/iterator.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/graph/iterator.go b/graph/iterator.go index 01a0358..86d0fd9 100644 --- a/graph/iterator.go +++ b/graph/iterator.go @@ -155,9 +155,14 @@ type Description struct { type Nexter interface { // Next advances the iterator to the next value, which will then be available through - // the Result method. It returns false if no further advancement is possible. + // the Result method. It returns false if no further advancement is possible, or if an + // error was encountered during iteration. Err should be consulted to distinguish + // between the two cases. Next() bool + // Err returns the error (if any) encountered during iteration. + Err() error + Iterator } From c0133018a5ed737c6de2277f2af9d97a801aa8e7 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 14 Apr 2015 16:34:03 -0700 Subject: [PATCH 03/34] Fix Err fallout for graph/iterator.Int64 iterator --- graph/iterator/all_iterator.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/graph/iterator/all_iterator.go b/graph/iterator/all_iterator.go index 31d4b8e..0fc2694 100644 --- a/graph/iterator/all_iterator.go +++ b/graph/iterator/all_iterator.go @@ -103,6 +103,11 @@ func (it *Int64) Next() bool { return graph.NextLogOut(it, val, true) } +func (it *Int64) Err() error { + // This iterator should never error. + return nil +} + // DEPRECATED func (it *Int64) ResultTree() *graph.ResultTree { return graph.NewResultTree(it.Result()) From ebb45d4c1483f073e364a46290eea7e7eaf2188f Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 14 Apr 2015 16:35:08 -0700 Subject: [PATCH 04/34] Fix Err fallout for graph/iterator.Fixed iterator --- graph/iterator/fixed_iterator.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/graph/iterator/fixed_iterator.go b/graph/iterator/fixed_iterator.go index fbfd310..cef8e1b 100644 --- a/graph/iterator/fixed_iterator.go +++ b/graph/iterator/fixed_iterator.go @@ -143,6 +143,11 @@ func (it *Fixed) Next() bool { return graph.NextLogOut(it, out, true) } +func (it *Fixed) Err() error { + // This iterator should never error. + return nil +} + // DEPRECATED func (it *Fixed) ResultTree() *graph.ResultTree { return graph.NewResultTree(it.Result()) From f1d7600c347203224ff311a6deb4ac4c08a24fc2 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 14 Apr 2015 16:57:57 -0700 Subject: [PATCH 05/34] Fix Err fallout for graph/iterator.Null iterator --- graph/iterator/iterator.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/graph/iterator/iterator.go b/graph/iterator/iterator.go index 6495de0..e822e55 100644 --- a/graph/iterator/iterator.go +++ b/graph/iterator/iterator.go @@ -88,6 +88,10 @@ func (it *Null) Next() bool { return false } +func (it *Null) Err() error { + return nil +} + func (it *Null) Result() graph.Value { return nil } From 6cb67cdd8c1b4170147488d796e725a623f93884 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 14 Apr 2015 16:58:20 -0700 Subject: [PATCH 06/34] Add a helper function Err, similar to graph.Next --- graph/iterator.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/graph/iterator.go b/graph/iterator.go index 86d0fd9..f9edd52 100644 --- a/graph/iterator.go +++ b/graph/iterator.go @@ -177,6 +177,17 @@ func Next(it Iterator) bool { return false } +// Err is a convenience function that conditionally calls the Err method +// of an Iterator if it is a Nexter. If the Iterator is not a Nexter, Err +// returns nil. +func Err(it Iterator) error { + if n, ok := it.(Nexter); ok { + return n.Err() + } + glog.Errorln("Calling Err on an un-nextable iterator") + return nil +} + // Height is a convienence function to measure the height of an iterator tree. func Height(it Iterator, until Type) int { if it.Type() == until { From 1181e76ab0d87e277806c98b4d625e3275eed143 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 14 Apr 2015 18:38:37 -0700 Subject: [PATCH 07/34] Add testIterator for use in testing --- graph/iterator/iterator_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 graph/iterator/iterator_test.go diff --git a/graph/iterator/iterator_test.go b/graph/iterator/iterator_test.go new file mode 100644 index 0000000..d20635c --- /dev/null +++ b/graph/iterator/iterator_test.go @@ -0,0 +1,29 @@ +package iterator + +import ( + "github.com/google/cayley/graph" +) + +// A testing iterator that returns the given values for Next() and Err(). +type testIterator struct { + *Fixed + + NextVal bool + ErrVal error +} + +func newTestIterator(next bool, err error) graph.Iterator { + return &testIterator{ + Fixed: NewFixed(Identity), + NextVal: next, + ErrVal: err, + } +} + +func (it *testIterator) Next() bool { + return it.NextVal +} + +func (it *testIterator) Err() error { + return it.ErrVal +} From c156fd6b1bcd6d89f87a123c5bdb5ccb0a0150fc Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 14 Apr 2015 16:58:46 -0700 Subject: [PATCH 08/34] Fix Err fallout for graph/iterator.Not iterator --- graph/iterator/not_iterator.go | 8 ++++++++ graph/iterator/not_iterator_test.go | 17 +++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/graph/iterator/not_iterator.go b/graph/iterator/not_iterator.go index 3a3a4c4..7a9ea21 100644 --- a/graph/iterator/not_iterator.go +++ b/graph/iterator/not_iterator.go @@ -12,6 +12,7 @@ type Not struct { primaryIt graph.Iterator allIt graph.Iterator result graph.Value + err error runstats graph.IteratorStats } @@ -87,9 +88,16 @@ func (it *Not) Next() bool { return graph.NextLogOut(it, curr, true) } } + if err := graph.Err(it.allIt); err != nil { + it.err = err + } return graph.NextLogOut(it, nil, false) } +func (it *Not) Err() error { + return it.err +} + func (it *Not) Result() graph.Value { return it.result } diff --git a/graph/iterator/not_iterator_test.go b/graph/iterator/not_iterator_test.go index ac63b7c..e63a0a0 100644 --- a/graph/iterator/not_iterator_test.go +++ b/graph/iterator/not_iterator_test.go @@ -1,6 +1,7 @@ package iterator import ( + "errors" "reflect" "testing" ) @@ -42,3 +43,19 @@ func TestNotIteratorBasics(t *testing.T) { } } } + +func TestNotIteratorErr(t *testing.T) { + retErr := errors.New("unique") + allIt := newTestIterator(false, retErr) + + toComplementIt := NewFixed(Identity) + + not := NewNot(toComplementIt, allIt) + + if not.Next() != false { + t.Errorf("Not iterator did not pass through initial 'false'") + } + if not.Err() != retErr { + t.Errorf("Not iterator did not pass through underlying Err") + } +} From 742277e72e4eb4b0ef7a04b27795ce2933adc657 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 14 Apr 2015 17:07:25 -0700 Subject: [PATCH 09/34] Fix Err fallout for graph/iterator.And iterator --- graph/iterator/and_iterator.go | 8 ++++++++ graph/iterator/and_iterator_test.go | 18 +++++++++++++++++- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/graph/iterator/and_iterator.go b/graph/iterator/and_iterator.go index d8d45e5..0cd6cfb 100644 --- a/graph/iterator/and_iterator.go +++ b/graph/iterator/and_iterator.go @@ -29,6 +29,7 @@ type And struct { primaryIt graph.Iterator checkList []graph.Iterator result graph.Value + err error runstats graph.IteratorStats } @@ -153,9 +154,16 @@ func (it *And) Next() bool { return graph.NextLogOut(it, curr, true) } } + if err := graph.Err(it.primaryIt); err != nil { + it.err = err + } return graph.NextLogOut(it, nil, false) } +func (it *And) Err() error { + return it.err +} + func (it *And) Result() graph.Value { return it.result } diff --git a/graph/iterator/and_iterator_test.go b/graph/iterator/and_iterator_test.go index 3f96519..1bc2182 100644 --- a/graph/iterator/and_iterator_test.go +++ b/graph/iterator/and_iterator_test.go @@ -15,6 +15,7 @@ package iterator import ( + "errors" "testing" "github.com/google/cayley/graph" @@ -138,5 +139,20 @@ func TestAllIterators(t *testing.T) { if and.Next() { t.Error("Too many values") } - +} + +func TestAndIteratorErr(t *testing.T) { + retErr := errors.New("unique") + allErr := newTestIterator(false, retErr) + + and := NewAnd() + and.AddSubIterator(allErr) + and.AddSubIterator(NewInt64(1, 5)) + + if and.Next() != false { + t.Errorf("And iterator did not pass through initial 'false'") + } + if and.Err() != retErr { + t.Errorf("And iterator did not pass through underlying Err") + } } From bb5fd55e21710f35d7ccd2dacbfd4f733c19c9e1 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 14 Apr 2015 17:17:13 -0700 Subject: [PATCH 10/34] Fix Err fallout for graph/iterator.HasA iterator --- graph/iterator/hasa_iterator.go | 8 ++++++++ graph/iterator/hasa_iterator_test.go | 37 ++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) create mode 100644 graph/iterator/hasa_iterator_test.go diff --git a/graph/iterator/hasa_iterator.go b/graph/iterator/hasa_iterator.go index adac2c8..879e031 100644 --- a/graph/iterator/hasa_iterator.go +++ b/graph/iterator/hasa_iterator.go @@ -51,6 +51,7 @@ type HasA struct { dir quad.Direction resultIt graph.Iterator result graph.Value + err error runstats graph.IteratorStats } @@ -202,6 +203,9 @@ func (it *HasA) Next() bool { it.resultIt = &Null{} if !graph.Next(it.primaryIt) { + if err := graph.Err(it.primaryIt); err != nil { + it.err = err + } return graph.NextLogOut(it, 0, false) } tID := it.primaryIt.Result() @@ -210,6 +214,10 @@ func (it *HasA) Next() bool { return graph.NextLogOut(it, val, true) } +func (it *HasA) Err() error { + return it.err +} + func (it *HasA) Result() graph.Value { return it.result } diff --git a/graph/iterator/hasa_iterator_test.go b/graph/iterator/hasa_iterator_test.go new file mode 100644 index 0000000..fbeb228 --- /dev/null +++ b/graph/iterator/hasa_iterator_test.go @@ -0,0 +1,37 @@ +// Copyright 2014 The Cayley Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package iterator + +import ( + "errors" + "testing" + + "github.com/google/cayley/quad" +) + +func TestHasAIteratorErr(t *testing.T) { + retErr := errors.New("unique") + errIt := newTestIterator(false, retErr) + + // TODO(andrew-d): pass a non-nil quadstore + hasa := NewHasA(nil, errIt, quad.Subject) + + if hasa.Next() != false { + t.Errorf("HasA iterator did not pass through initial 'false'") + } + if hasa.Err() != retErr { + t.Errorf("HasA iterator did not pass through underlying Err") + } +} From 40cbbfcc1b2bbd0d16e5c2388da6d0c728a939ec Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 14 Apr 2015 17:21:12 -0700 Subject: [PATCH 11/34] Fix Err fallout for graph/iterator.Comparison iterator --- graph/iterator/value_comparison_iterator.go | 8 ++++++++ graph/iterator/value_comparison_iterator_test.go | 15 +++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/graph/iterator/value_comparison_iterator.go b/graph/iterator/value_comparison_iterator.go index b1a0842..19abab7 100644 --- a/graph/iterator/value_comparison_iterator.go +++ b/graph/iterator/value_comparison_iterator.go @@ -51,6 +51,7 @@ type Comparison struct { val interface{} qs graph.QuadStore result graph.Value + err error } func NewComparison(sub graph.Iterator, op Operator, val interface{}, qs graph.QuadStore) *Comparison { @@ -133,9 +134,16 @@ func (it *Comparison) Next() bool { return true } } + if err := graph.Err(it.subIt); err != nil { + it.err = err + } return false } +func (it *Comparison) Err() error { + return it.err +} + // DEPRECATED func (it *Comparison) ResultTree() *graph.ResultTree { return graph.NewResultTree(it.Result()) diff --git a/graph/iterator/value_comparison_iterator_test.go b/graph/iterator/value_comparison_iterator_test.go index b8a6cc8..64ce5e9 100644 --- a/graph/iterator/value_comparison_iterator_test.go +++ b/graph/iterator/value_comparison_iterator_test.go @@ -15,6 +15,7 @@ package iterator import ( + "errors" "reflect" "testing" @@ -118,3 +119,17 @@ func TestVCIContains(t *testing.T) { } } } + +func TestComparisonIteratorErr(t *testing.T) { + retErr := errors.New("unique") + errIt := newTestIterator(false, retErr) + + 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 { + t.Errorf("Comparison iterator did not pass through underlying Err") + } +} From aaa3f2775466cf3df1d1154164dfb30980667cbb Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 14 Apr 2015 18:49:02 -0700 Subject: [PATCH 12/34] Fix Err fallout for graph/iterator.Or iterator --- graph/iterator/or_iterator.go | 10 +++++++++ graph/iterator/or_iterator_test.go | 44 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/graph/iterator/or_iterator.go b/graph/iterator/or_iterator.go index 3c07b18..48875dc 100644 --- a/graph/iterator/or_iterator.go +++ b/graph/iterator/or_iterator.go @@ -33,6 +33,7 @@ type Or struct { itCount int currentIterator int result graph.Value + err error } func NewOr() *Or { @@ -147,6 +148,11 @@ func (it *Or) Next() bool { return graph.NextLogOut(it, it.result, true) } + if err := graph.Err(curIt); err != nil { + it.err = err + return graph.NextLogOut(it, nil, false) + } + if it.isShortCircuiting && !first { break } @@ -159,6 +165,10 @@ func (it *Or) Next() bool { return graph.NextLogOut(it, nil, false) } +func (it *Or) Err() error { + return it.err +} + func (it *Or) Result() graph.Value { return it.result } diff --git a/graph/iterator/or_iterator_test.go b/graph/iterator/or_iterator_test.go index 27cdc7d..edbd6d4 100644 --- a/graph/iterator/or_iterator_test.go +++ b/graph/iterator/or_iterator_test.go @@ -15,6 +15,7 @@ package iterator import ( + "errors" "reflect" "testing" @@ -148,3 +149,46 @@ func TestShortCircuitingOrBasics(t *testing.T) { t.Errorf("Failed to iterate optimized Or correctly, got:%v expect:%v", got, expect) } } + +func TestOrIteratorErr(t *testing.T) { + retErr := errors.New("unique") + orErr := newTestIterator(false, retErr) + + fix1 := NewFixed(Identity) + fix1.Add(1) + + or := NewOr() + or.AddSubIterator(fix1) + or.AddSubIterator(orErr) + or.AddSubIterator(NewInt64(1, 5)) + + if !or.Next() { + t.Errorf("Failed to iterate Or correctly") + } + if got := or.Result(); got != 1 { + t.Errorf("Failed to iterate Or correctly, got:%v expect:1", got) + } + + if or.Next() != false { + t.Errorf("Or iterator did not pass through underlying 'false'") + } + if or.Err() != retErr { + t.Errorf("Or iterator did not pass through underlying Err") + } +} + +func TestShortCircuitOrIteratorErr(t *testing.T) { + retErr := errors.New("unique") + orErr := newTestIterator(false, retErr) + + or := NewOr() + or.AddSubIterator(orErr) + or.AddSubIterator(NewInt64(1, 5)) + + if or.Next() != false { + t.Errorf("Or iterator did not pass through underlying 'false'") + } + if or.Err() != retErr { + t.Errorf("Or iterator did not pass through underlying Err") + } +} From 3ce9adbc0b5af8073ce143f522849cece4852547 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 14 Apr 2015 18:55:48 -0700 Subject: [PATCH 13/34] Fix Err fallout for graph/bolt iterators --- graph/bolt/all_iterator.go | 6 ++++++ graph/bolt/iterator.go | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/graph/bolt/all_iterator.go b/graph/bolt/all_iterator.go index 2c8d379..4ae6fca 100644 --- a/graph/bolt/all_iterator.go +++ b/graph/bolt/all_iterator.go @@ -33,6 +33,7 @@ type AllIterator struct { dir quad.Direction qs *QuadStore result *Token + err error buffer [][]byte offset int done bool @@ -121,6 +122,7 @@ func (it *AllIterator) Next() bool { }) if err != nil { glog.Error("Error nexting in database: ", err) + it.err = err it.done = true return false } @@ -134,6 +136,10 @@ func (it *AllIterator) Next() bool { return true } +func (it *AllIterator) Err() error { + return it.err +} + func (it *AllIterator) ResultTree() *graph.ResultTree { return graph.NewResultTree(it.Result()) } diff --git a/graph/bolt/iterator.go b/graph/bolt/iterator.go index b7df4e9..50b2588 100644 --- a/graph/bolt/iterator.go +++ b/graph/bolt/iterator.go @@ -46,6 +46,7 @@ type Iterator struct { dir quad.Direction qs *QuadStore result *Token + err error buffer [][]byte offset int done bool @@ -170,6 +171,7 @@ func (it *Iterator) Next() bool { if err != nil { if err != errNotExist { glog.Errorf("Error nexting in database: %v", err) + it.err = err } it.done = true return false @@ -184,6 +186,10 @@ func (it *Iterator) Next() bool { return true } +func (it *Iterator) Err() error { + return it.err +} + func (it *Iterator) ResultTree() *graph.ResultTree { return graph.NewResultTree(it.Result()) } From 9cdeb519d7b390f9f30da2e209f1fee13927f0c4 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 14 Apr 2015 18:56:05 -0700 Subject: [PATCH 14/34] Fix Err fallout for graph/memstore iterators --- graph/memstore/all_iterator.go | 4 ++++ graph/memstore/iterator.go | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/graph/memstore/all_iterator.go b/graph/memstore/all_iterator.go index 26cb643..abfc105 100644 --- a/graph/memstore/all_iterator.go +++ b/graph/memstore/all_iterator.go @@ -52,6 +52,10 @@ func (it *nodesAllIterator) Next() bool { return true } +func (it *nodesAllIterator) Err() error { + return nil +} + func newQuadsAllIterator(qs *QuadStore) *quadsAllIterator { var out quadsAllIterator out.Int64 = *iterator.NewInt64(1, qs.nextQuadID-1) diff --git a/graph/memstore/iterator.go b/graph/memstore/iterator.go index d93ba62..96911af 100644 --- a/graph/memstore/iterator.go +++ b/graph/memstore/iterator.go @@ -30,6 +30,7 @@ type Iterator struct { iter *b.Enumerator data string result graph.Value + err error } func cmp(a, b int64) int { @@ -118,6 +119,7 @@ func (it *Iterator) Next() bool { } result, _, err := it.iter.Next() if err != nil { + it.err = err return graph.NextLogOut(it, nil, false) } if !it.checkValid(result) { @@ -127,6 +129,10 @@ func (it *Iterator) Next() bool { return graph.NextLogOut(it, it.result, true) } +func (it *Iterator) Err() error { + return it.err +} + func (it *Iterator) ResultTree() *graph.ResultTree { return graph.NewResultTree(it.Result()) } From 5efd90a6513b1cf9977476560ce20e6dece5e35b Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 14 Apr 2015 19:11:23 -0700 Subject: [PATCH 15/34] Fix Err fallout for graph/leveldb iterators --- graph/leveldb/all_iterator.go | 4 ++++ graph/leveldb/iterator.go | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/graph/leveldb/all_iterator.go b/graph/leveldb/all_iterator.go index 3a118f9..2940a0d 100644 --- a/graph/leveldb/all_iterator.go +++ b/graph/leveldb/all_iterator.go @@ -119,6 +119,10 @@ func (it *AllIterator) Next() bool { return true } +func (it *AllIterator) Err() error { + return it.iter.Error() +} + func (it *AllIterator) ResultTree() *graph.ResultTree { return graph.NewResultTree(it.Result()) } diff --git a/graph/leveldb/iterator.go b/graph/leveldb/iterator.go index dcc4aeb..3a77188 100644 --- a/graph/leveldb/iterator.go +++ b/graph/leveldb/iterator.go @@ -154,6 +154,10 @@ func (it *Iterator) Next() bool { return false } +func (it *Iterator) Err() error { + return it.iter.Error() +} + func (it *Iterator) ResultTree() *graph.ResultTree { return graph.NewResultTree(it.Result()) } From bd2b2b73cff3e8cb4a697c43401170bf055bb62d Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 14 Apr 2015 19:11:39 -0700 Subject: [PATCH 16/34] Fix Err fallout for graph/mongo iterator --- graph/mongo/iterator.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/graph/mongo/iterator.go b/graph/mongo/iterator.go index bdd3bab..bbe1b4f 100644 --- a/graph/mongo/iterator.go +++ b/graph/mongo/iterator.go @@ -39,6 +39,7 @@ type Iterator struct { constraint bson.M collection string result graph.Value + err error } func NewIterator(qs *QuadStore, collection string, d quad.Direction, val graph.Value) *Iterator { @@ -137,6 +138,7 @@ func (it *Iterator) Next() bool { if !found { err := it.iter.Err() if err != nil { + it.err = err glog.Errorln("Error Nexting Iterator: ", err) } return false @@ -148,6 +150,10 @@ func (it *Iterator) Next() bool { return true } +func (it *Iterator) Err() error { + return it.err +} + func (it *Iterator) ResultTree() *graph.ResultTree { return graph.NewResultTree(it.Result()) } From 5c9979ec8bfc7679d9c66aa21f921cdeeb8d3962 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 14 Apr 2015 19:58:39 -0700 Subject: [PATCH 17/34] Fix Err fallout for graph/gaedatastore iterator --- graph/gaedatastore/iterator.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/graph/gaedatastore/iterator.go b/graph/gaedatastore/iterator.go index 8b8339d..316207d 100644 --- a/graph/gaedatastore/iterator.go +++ b/graph/gaedatastore/iterator.go @@ -41,6 +41,7 @@ type Iterator struct { offset int last string result graph.Value + err error } var ( @@ -267,7 +268,8 @@ func (it *Iterator) Next() bool { } if err != nil { glog.Errorf("Error fetching next entry %v", err) - break + it.err = err + return false } if !skip { it.buffer = append(it.buffer, k.StringID()) @@ -288,6 +290,10 @@ func (it *Iterator) Next() bool { return true } +func (it *Iterator) Err() error { + return it.err +} + func (it *Iterator) Size() (int64, bool) { return it.size, true } From 1b6395ed0af3c1bfc53d1f644f5dbd4d79803b54 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 14 Apr 2015 20:17:31 -0700 Subject: [PATCH 18/34] Make Close() method on Iterators return an error --- graph/bolt/all_iterator.go | 3 ++- graph/bolt/iterator.go | 3 ++- graph/gaedatastore/iterator.go | 3 ++- graph/iterator.go | 2 +- graph/iterator/all_iterator.go | 4 +++- graph/iterator/and_iterator.go | 16 +++++++++++++--- graph/iterator/fixed_iterator.go | 4 +++- graph/iterator/hasa_iterator.go | 12 +++++++++--- graph/iterator/iterator.go | 4 +++- graph/iterator/linksto_iterator.go | 14 +++++++++++--- graph/iterator/materialize_iterator.go | 4 ++-- graph/iterator/not_iterator.go | 16 +++++++++++++--- graph/iterator/optional_iterator.go | 4 ++-- graph/iterator/or_iterator.go | 15 ++++++++++++--- graph/iterator/value_comparison_iterator.go | 4 ++-- graph/leveldb/all_iterator.go | 3 ++- graph/leveldb/iterator.go | 3 ++- graph/memstore/iterator.go | 4 +++- graph/mongo/iterator.go | 4 ++-- 19 files changed, 89 insertions(+), 33 deletions(-) diff --git a/graph/bolt/all_iterator.go b/graph/bolt/all_iterator.go index 4ae6fca..d8fae27 100644 --- a/graph/bolt/all_iterator.go +++ b/graph/bolt/all_iterator.go @@ -174,10 +174,11 @@ func (it *AllIterator) Contains(v graph.Value) bool { return true } -func (it *AllIterator) Close() { +func (it *AllIterator) Close() error { it.result = nil it.buffer = nil it.done = true + return nil } func (it *AllIterator) Size() (int64, bool) { diff --git a/graph/bolt/iterator.go b/graph/bolt/iterator.go index 50b2588..6a28422 100644 --- a/graph/bolt/iterator.go +++ b/graph/bolt/iterator.go @@ -106,10 +106,11 @@ func (it *Iterator) Clone() graph.Iterator { return out } -func (it *Iterator) Close() { +func (it *Iterator) Close() error { it.result = nil it.buffer = nil it.done = true + return nil } func (it *Iterator) isLiveValue(val []byte) bool { diff --git a/graph/gaedatastore/iterator.go b/graph/gaedatastore/iterator.go index 316207d..8f3ef18 100644 --- a/graph/gaedatastore/iterator.go +++ b/graph/gaedatastore/iterator.go @@ -130,12 +130,13 @@ func (it *Iterator) Reset() { it.result = nil } -func (it *Iterator) Close() { +func (it *Iterator) Close() error { it.buffer = nil it.offset = 0 it.done = true it.last = "" it.result = nil + return nil } func (it *Iterator) Tagger() *graph.Tagger { diff --git a/graph/iterator.go b/graph/iterator.go index f9edd52..63ccade 100644 --- a/graph/iterator.go +++ b/graph/iterator.go @@ -136,7 +136,7 @@ type Iterator interface { Describe() Description // Close the iterator and do internal cleanup. - Close() + Close() error // UID returns the unique identifier of the iterator. UID() uint64 diff --git a/graph/iterator/all_iterator.go b/graph/iterator/all_iterator.go index 0fc2694..3e37563 100644 --- a/graph/iterator/all_iterator.go +++ b/graph/iterator/all_iterator.go @@ -55,7 +55,9 @@ func (it *Int64) Reset() { it.at = it.min } -func (it *Int64) Close() {} +func (it *Int64) Close() error { + return nil +} func (it *Int64) Clone() graph.Iterator { out := NewInt64(it.min, it.max) diff --git a/graph/iterator/and_iterator.go b/graph/iterator/and_iterator.go index 0cd6cfb..956c21c 100644 --- a/graph/iterator/and_iterator.go +++ b/graph/iterator/and_iterator.go @@ -263,12 +263,22 @@ 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. -func (it *And) Close() { +// +// 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() - it.primaryIt.Close() + + var ret error + + ret = it.primaryIt.Close() for _, sub := range it.internalIterators { - sub.Close() + if err := sub.Close(); err != nil && ret != nil { + ret = err + } } + + return ret } // Register this as an "and" iterator. diff --git a/graph/iterator/fixed_iterator.go b/graph/iterator/fixed_iterator.go index cef8e1b..35c2690 100644 --- a/graph/iterator/fixed_iterator.go +++ b/graph/iterator/fixed_iterator.go @@ -63,7 +63,9 @@ func (it *Fixed) Reset() { it.lastIndex = 0 } -func (it *Fixed) Close() {} +func (it *Fixed) Close() error { + return nil +} func (it *Fixed) Tagger() *graph.Tagger { return &it.tags diff --git a/graph/iterator/hasa_iterator.go b/graph/iterator/hasa_iterator.go index 879e031..031c3b0 100644 --- a/graph/iterator/hasa_iterator.go +++ b/graph/iterator/hasa_iterator.go @@ -247,11 +247,17 @@ func (it *HasA) Stats() graph.IteratorStats { } // Close the subiterator, the result iterator (if any) and the HasA. -func (it *HasA) Close() { +func (it *HasA) Close() error { + var ret error + if it.resultIt != nil { - it.resultIt.Close() + ret = it.resultIt.Close() } - it.primaryIt.Close() + if err := it.primaryIt.Close(); err != nil && ret != nil { + ret = err + } + + return ret } // Register this iterator as a HasA. diff --git a/graph/iterator/iterator.go b/graph/iterator/iterator.go index e822e55..371025c 100644 --- a/graph/iterator/iterator.go +++ b/graph/iterator/iterator.go @@ -114,7 +114,9 @@ func (it *Null) Size() (int64, bool) { func (it *Null) Reset() {} -func (it *Null) Close() {} +func (it *Null) Close() error { + return nil +} // A null iterator costs nothing. Use it! func (it *Null) Stats() graph.IteratorStats { diff --git a/graph/iterator/linksto_iterator.go b/graph/iterator/linksto_iterator.go index cc1cfa5..bf51293 100644 --- a/graph/iterator/linksto_iterator.go +++ b/graph/iterator/linksto_iterator.go @@ -181,9 +181,17 @@ func (it *LinksTo) Result() graph.Value { } // Close our subiterators. -func (it *LinksTo) Close() { - it.nextIt.Close() - it.primaryIt.Close() +func (it *LinksTo) Close() error { + var ret error + + if err := it.nextIt.Close(); err != nil && ret != nil { + ret = err + } + if err := it.primaryIt.Close(); err != nil && ret != nil { + ret = err + } + + return ret } // We won't ever have a new result, but our subiterators might. diff --git a/graph/iterator/materialize_iterator.go b/graph/iterator/materialize_iterator.go index f771e89..871df88 100644 --- a/graph/iterator/materialize_iterator.go +++ b/graph/iterator/materialize_iterator.go @@ -69,11 +69,11 @@ func (it *Materialize) Reset() { it.index = -1 } -func (it *Materialize) Close() { - it.subIt.Close() +func (it *Materialize) Close() error { it.containsMap = nil it.values = nil it.hasRun = false + return it.subIt.Close() } func (it *Materialize) Tagger() *graph.Tagger { diff --git a/graph/iterator/not_iterator.go b/graph/iterator/not_iterator.go index 7a9ea21..3bee077 100644 --- a/graph/iterator/not_iterator.go +++ b/graph/iterator/not_iterator.go @@ -123,9 +123,19 @@ func (it *Not) NextPath() bool { return false } -func (it *Not) Close() { - it.primaryIt.Close() - it.allIt.Close() +// Close closes the primary and all iterators. If an error occurs, only the +// first one will be returned. +func (it *Not) Close() error { + var ret error + + if err := it.primaryIt.Close(); err != nil && ret != nil { + ret = err + } + if err := it.allIt.Close(); err != nil && ret != nil { + ret = err + } + + return ret } func (it *Not) Type() graph.Type { return graph.Not } diff --git a/graph/iterator/optional_iterator.go b/graph/iterator/optional_iterator.go index bc65acc..f39f57d 100644 --- a/graph/iterator/optional_iterator.go +++ b/graph/iterator/optional_iterator.go @@ -57,8 +57,8 @@ func (it *Optional) Reset() { it.lastCheck = false } -func (it *Optional) Close() { - it.subIt.Close() +func (it *Optional) Close() error { + return it.subIt.Close() } func (it *Optional) Tagger() *graph.Tagger { diff --git a/graph/iterator/or_iterator.go b/graph/iterator/or_iterator.go index 48875dc..0093eb5 100644 --- a/graph/iterator/or_iterator.go +++ b/graph/iterator/or_iterator.go @@ -241,12 +241,21 @@ 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 And follows the contract. -func (it *Or) Close() { +// follow this contract, the Or follows the contract. +// +// 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 { - sub.Close() + if err := sub.Close(); err != nil && ret != nil { + ret = err + } } + + return ret } func (it *Or) Optimize() (graph.Iterator, bool) { diff --git a/graph/iterator/value_comparison_iterator.go b/graph/iterator/value_comparison_iterator.go index 19abab7..0299fdd 100644 --- a/graph/iterator/value_comparison_iterator.go +++ b/graph/iterator/value_comparison_iterator.go @@ -92,8 +92,8 @@ func (it *Comparison) doComparison(val graph.Value) bool { } } -func (it *Comparison) Close() { - it.subIt.Close() +func (it *Comparison) Close() error { + return it.subIt.Close() } func RunIntOp(a int64, op Operator, b int64) bool { diff --git a/graph/leveldb/all_iterator.go b/graph/leveldb/all_iterator.go index 2940a0d..e0d1bdc 100644 --- a/graph/leveldb/all_iterator.go +++ b/graph/leveldb/all_iterator.go @@ -145,11 +145,12 @@ func (it *AllIterator) Contains(v graph.Value) bool { return true } -func (it *AllIterator) Close() { +func (it *AllIterator) Close() error { if it.open { it.iter.Release() it.open = false } + return nil } func (it *AllIterator) Size() (int64, bool) { diff --git a/graph/leveldb/iterator.go b/graph/leveldb/iterator.go index 3a77188..83321ad 100644 --- a/graph/leveldb/iterator.go +++ b/graph/leveldb/iterator.go @@ -109,11 +109,12 @@ func (it *Iterator) Clone() graph.Iterator { return out } -func (it *Iterator) Close() { +func (it *Iterator) Close() error { if it.open { it.iter.Release() it.open = false } + return nil } func (it *Iterator) isLiveValue(val []byte) bool { diff --git a/graph/memstore/iterator.go b/graph/memstore/iterator.go index 96911af..17bd8e2 100644 --- a/graph/memstore/iterator.go +++ b/graph/memstore/iterator.go @@ -105,7 +105,9 @@ func (it *Iterator) Clone() graph.Iterator { return m } -func (it *Iterator) Close() {} +func (it *Iterator) Close() error { + return nil +} func (it *Iterator) checkValid(index int64) bool { return it.qs.log[index].DeletedBy == 0 diff --git a/graph/mongo/iterator.go b/graph/mongo/iterator.go index bbe1b4f..6409901 100644 --- a/graph/mongo/iterator.go +++ b/graph/mongo/iterator.go @@ -99,8 +99,8 @@ func (it *Iterator) Reset() { } -func (it *Iterator) Close() { - it.iter.Close() +func (it *Iterator) Close() error { + return it.iter.Close() } func (it *Iterator) Tagger() *graph.Tagger { From 0148f6ef120fc58a042f32012e07c74996ddfeff Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 14 Apr 2015 22:13:56 -0700 Subject: [PATCH 19/34] More Close() docs --- graph/iterator/hasa_iterator.go | 3 +++ graph/iterator/linksto_iterator.go | 3 +++ 2 files changed, 6 insertions(+) diff --git a/graph/iterator/hasa_iterator.go b/graph/iterator/hasa_iterator.go index 031c3b0..21dbb16 100644 --- a/graph/iterator/hasa_iterator.go +++ b/graph/iterator/hasa_iterator.go @@ -247,6 +247,9 @@ func (it *HasA) Stats() graph.IteratorStats { } // Close the subiterator, the result iterator (if any) and the HasA. +// +// 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 diff --git a/graph/iterator/linksto_iterator.go b/graph/iterator/linksto_iterator.go index bf51293..0b2f9c4 100644 --- a/graph/iterator/linksto_iterator.go +++ b/graph/iterator/linksto_iterator.go @@ -181,6 +181,9 @@ func (it *LinksTo) Result() graph.Value { } // Close our subiterators. +// +// 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 From 912b126e92507620203046a2fdcd28f24b1e0b70 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 15 Apr 2015 10:48:50 -0700 Subject: [PATCH 20/34] Fix Err fallout for graph/iterator.Materialize iterator --- graph/iterator/materialize_iterator.go | 19 +++++++- graph/iterator/materialize_iterator_test.go | 72 +++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 2 deletions(-) create mode 100644 graph/iterator/materialize_iterator_test.go diff --git a/graph/iterator/materialize_iterator.go b/graph/iterator/materialize_iterator.go index 871df88..2b8a464 100644 --- a/graph/iterator/materialize_iterator.go +++ b/graph/iterator/materialize_iterator.go @@ -48,6 +48,7 @@ type Materialize struct { subIt graph.Iterator hasRun bool aborted bool + err error runstats graph.IteratorStats } @@ -108,6 +109,7 @@ func (it *Materialize) Clone() graph.Iterator { if it.hasRun { out.hasRun = true out.aborted = it.aborted + out.err = it.err out.values = it.values out.containsMap = it.containsMap out.actualSize = it.actualSize @@ -199,8 +201,15 @@ func (it *Materialize) Next() bool { if !it.hasRun { it.materializeSet() } + if it.err != nil { + return false + } if it.aborted { - return graph.Next(it.subIt) + n := graph.Next(it.subIt) + if err := graph.Err(it.subIt); err != nil { + it.err = err + } + return n } it.index++ @@ -211,6 +220,10 @@ func (it *Materialize) Next() bool { return graph.NextLogOut(it, it.Result(), true) } +func (it *Materialize) Err() error { + return it.err +} + func (it *Materialize) Contains(v graph.Value) bool { graph.ContainsLogIn(it, v) it.runstats.Contains += 1 @@ -283,7 +296,9 @@ func (it *Materialize) materializeSet() { it.actualSize += 1 } } - if it.aborted { + if err := graph.Err(it.subIt); err != nil { + it.err = err + } else if it.aborted { if glog.V(2) { glog.V(2).Infoln("Aborting subiterator") } diff --git a/graph/iterator/materialize_iterator_test.go b/graph/iterator/materialize_iterator_test.go new file mode 100644 index 0000000..aa749c5 --- /dev/null +++ b/graph/iterator/materialize_iterator_test.go @@ -0,0 +1,72 @@ +// Copyright 2014 The Cayley Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package iterator + +import ( + "errors" + "testing" + + //"github.com/google/cayley/graph" +) + +func TestMaterializeIteratorError(t *testing.T) { + retErr := errors.New("unique") + errIt := newTestIterator(false, retErr) + + // This tests that we properly return 0 results and the error when the + // underlying iterator returns an error. + mIt := NewMaterialize(errIt) + + if mIt.Next() != false { + t.Errorf("Materialize iterator did not pass through underlying 'false'") + } + if mIt.Err() != retErr { + t.Errorf("Materialize iterator did not pass through underlying Err") + } +} + +func TestMaterializeIteratorErrorAbort(t *testing.T) { + retErr := errors.New("unique") + errIt := newTestIterator(false, retErr) + + // This tests that we properly return 0 results and the error when the + // underlying iterator is larger than our 'abort at' value, and then + // returns an error. + or := NewOr() + or.AddSubIterator(NewInt64(1, int64(abortMaterializeAt+1))) + or.AddSubIterator(errIt) + + mIt := NewMaterialize(or) + + // 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) + return + } + if mIt.Err() != nil { + t.Errorf("Materialize iterator returned non-nil Err on iteration %d", i) + return + } + } + + // ... and then the error value + if mIt.Next() != false { + t.Errorf("Materialize iterator did not pass through underlying 'false'") + } + if mIt.Err() != retErr { + t.Errorf("Materialize iterator did not pass through underlying Err") + } +} From accbc6007e6f86a497aef48186261bf08b9922e5 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 15 Apr 2015 10:51:22 -0700 Subject: [PATCH 21/34] Move Err() method to Iterator interface, fix fallout --- graph/iterator.go | 6 +++--- graph/iterator/optional_iterator.go | 4 ++++ 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/graph/iterator.go b/graph/iterator.go index 63ccade..46cc9f9 100644 --- a/graph/iterator.go +++ b/graph/iterator.go @@ -100,6 +100,9 @@ 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() error + // Start iteration from the beginning Reset() @@ -160,9 +163,6 @@ type Nexter interface { // between the two cases. Next() bool - // Err returns the error (if any) encountered during iteration. - Err() error - Iterator } diff --git a/graph/iterator/optional_iterator.go b/graph/iterator/optional_iterator.go index f39f57d..c8c7fec 100644 --- a/graph/iterator/optional_iterator.go +++ b/graph/iterator/optional_iterator.go @@ -76,6 +76,10 @@ func (it *Optional) ResultTree() *graph.ResultTree { return graph.NewResultTree(it.Result()) } +func (it *Optional) Err() error { + return it.subIt.Err() +} + func (it *Optional) Result() graph.Value { return it.result } From 745d4874e624705d5c10ba86b0631fec7807e68f Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 15 Apr 2015 10:58:37 -0700 Subject: [PATCH 22/34] Fix Err fallout for graph/iterator.LinksTo iterator --- graph/iterator/linksto_iterator.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/graph/iterator/linksto_iterator.go b/graph/iterator/linksto_iterator.go index 0b2f9c4..005fc37 100644 --- a/graph/iterator/linksto_iterator.go +++ b/graph/iterator/linksto_iterator.go @@ -45,6 +45,7 @@ type LinksTo struct { dir quad.Direction nextIt graph.Iterator result graph.Value + err error runstats graph.IteratorStats } @@ -164,8 +165,17 @@ func (it *LinksTo) Next() bool { return graph.NextLogOut(it, it.nextIt, true) } + // If there's an error in the 'next' iterator, we save it and we're done. + if err := graph.Err(it.nextIt); err != nil { + it.err = err + return false + } + // Subiterator is empty, get another one if !graph.Next(it.primaryIt) { + // Possibly save error + it.err = it.primaryIt.Err() + // We're out of nodes in our subiterator, so we're done as well. return graph.NextLogOut(it, 0, false) } @@ -176,6 +186,10 @@ func (it *LinksTo) Next() bool { return it.Next() } +func (it *LinksTo) Err() error { + return it.err +} + func (it *LinksTo) Result() graph.Value { return it.result } From 33dd596ab4c8082efcb1ea53c6cb74f2c38e769c Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 15 Apr 2015 11:15:55 -0700 Subject: [PATCH 23/34] Remove graph.Err helper function Since Err() is now a member of the Iterator interface, we don't need this helper function anymore. --- graph/iterator.go | 11 ----------- graph/iterator/and_iterator.go | 2 +- graph/iterator/hasa_iterator.go | 2 +- graph/iterator/linksto_iterator.go | 2 +- graph/iterator/materialize_iterator.go | 4 ++-- graph/iterator/not_iterator.go | 2 +- graph/iterator/or_iterator.go | 2 +- graph/iterator/value_comparison_iterator.go | 2 +- 8 files changed, 8 insertions(+), 19 deletions(-) diff --git a/graph/iterator.go b/graph/iterator.go index 46cc9f9..9204c5c 100644 --- a/graph/iterator.go +++ b/graph/iterator.go @@ -177,17 +177,6 @@ func Next(it Iterator) bool { return false } -// Err is a convenience function that conditionally calls the Err method -// of an Iterator if it is a Nexter. If the Iterator is not a Nexter, Err -// returns nil. -func Err(it Iterator) error { - if n, ok := it.(Nexter); ok { - return n.Err() - } - glog.Errorln("Calling Err on an un-nextable iterator") - return nil -} - // Height is a convienence function to measure the height of an iterator tree. func Height(it Iterator, until Type) int { if it.Type() == until { diff --git a/graph/iterator/and_iterator.go b/graph/iterator/and_iterator.go index 956c21c..f7da02f 100644 --- a/graph/iterator/and_iterator.go +++ b/graph/iterator/and_iterator.go @@ -154,7 +154,7 @@ func (it *And) Next() bool { return graph.NextLogOut(it, curr, true) } } - if err := graph.Err(it.primaryIt); err != nil { + if err := it.primaryIt.Err(); err != nil { it.err = err } return graph.NextLogOut(it, nil, false) diff --git a/graph/iterator/hasa_iterator.go b/graph/iterator/hasa_iterator.go index 21dbb16..f9f555b 100644 --- a/graph/iterator/hasa_iterator.go +++ b/graph/iterator/hasa_iterator.go @@ -203,7 +203,7 @@ func (it *HasA) Next() bool { it.resultIt = &Null{} if !graph.Next(it.primaryIt) { - if err := graph.Err(it.primaryIt); err != nil { + if err := it.primaryIt.Err(); err != nil { it.err = err } return graph.NextLogOut(it, 0, false) diff --git a/graph/iterator/linksto_iterator.go b/graph/iterator/linksto_iterator.go index 005fc37..16affac 100644 --- a/graph/iterator/linksto_iterator.go +++ b/graph/iterator/linksto_iterator.go @@ -166,7 +166,7 @@ func (it *LinksTo) Next() bool { } // If there's an error in the 'next' iterator, we save it and we're done. - if err := graph.Err(it.nextIt); err != nil { + if err := it.nextIt.Err(); err != nil { it.err = err return false } diff --git a/graph/iterator/materialize_iterator.go b/graph/iterator/materialize_iterator.go index 2b8a464..cff1c88 100644 --- a/graph/iterator/materialize_iterator.go +++ b/graph/iterator/materialize_iterator.go @@ -206,7 +206,7 @@ func (it *Materialize) Next() bool { } if it.aborted { n := graph.Next(it.subIt) - if err := graph.Err(it.subIt); err != nil { + if err := it.subIt.Err(); err != nil { it.err = err } return n @@ -296,7 +296,7 @@ func (it *Materialize) materializeSet() { it.actualSize += 1 } } - if err := graph.Err(it.subIt); err != nil { + if err := it.subIt.Err(); err != nil { it.err = err } else if it.aborted { if glog.V(2) { diff --git a/graph/iterator/not_iterator.go b/graph/iterator/not_iterator.go index 3bee077..2617a98 100644 --- a/graph/iterator/not_iterator.go +++ b/graph/iterator/not_iterator.go @@ -88,7 +88,7 @@ func (it *Not) Next() bool { return graph.NextLogOut(it, curr, true) } } - if err := graph.Err(it.allIt); err != nil { + if err := it.allIt.Err(); err != nil { it.err = err } return graph.NextLogOut(it, nil, false) diff --git a/graph/iterator/or_iterator.go b/graph/iterator/or_iterator.go index 0093eb5..e5b4e09 100644 --- a/graph/iterator/or_iterator.go +++ b/graph/iterator/or_iterator.go @@ -148,7 +148,7 @@ func (it *Or) Next() bool { return graph.NextLogOut(it, it.result, true) } - if err := graph.Err(curIt); err != nil { + if err := curIt.Err(); err != nil { it.err = err return graph.NextLogOut(it, nil, false) } diff --git a/graph/iterator/value_comparison_iterator.go b/graph/iterator/value_comparison_iterator.go index 0299fdd..9197172 100644 --- a/graph/iterator/value_comparison_iterator.go +++ b/graph/iterator/value_comparison_iterator.go @@ -134,7 +134,7 @@ func (it *Comparison) Next() bool { return true } } - if err := graph.Err(it.subIt); err != nil { + if err := it.subIt.Err(); err != nil { it.err = err } return false From 52d0b8779a421a90fb3aaffe9ca24b1fadfdf4e4 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 15 Apr 2015 11:17:33 -0700 Subject: [PATCH 24/34] Add an additional static type assertion --- graph/iterator/optional_iterator.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/graph/iterator/optional_iterator.go b/graph/iterator/optional_iterator.go index c8c7fec..84e7154 100644 --- a/graph/iterator/optional_iterator.go +++ b/graph/iterator/optional_iterator.go @@ -156,3 +156,5 @@ func (it *Optional) Stats() graph.IteratorStats { func (it *Optional) Size() (int64, bool) { return 0, true } + +var _ graph.Iterator = &Optional{} From 0355b89f54e71dbe3807ed47753fb0ef7074aa46 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 15 Apr 2015 11:46:26 -0700 Subject: [PATCH 25/34] Don't save non-errors in the memstore iterator As per the following link, io.EOF is returned to signal "no more items". Thus, we don't treat this as an error, and don't save it. https://godoc.org/github.com/cznic/b#Enumerator --- graph/memstore/iterator.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/graph/memstore/iterator.go b/graph/memstore/iterator.go index 17bd8e2..b39b4bf 100644 --- a/graph/memstore/iterator.go +++ b/graph/memstore/iterator.go @@ -15,6 +15,7 @@ package memstore import ( + "io" "math" "github.com/google/cayley/graph" @@ -121,7 +122,9 @@ func (it *Iterator) Next() bool { } result, _, err := it.iter.Next() if err != nil { - it.err = err + if err != io.EOF { + it.err = err + } return graph.NextLogOut(it, nil, false) } if !it.checkValid(result) { From cacdb74e41974a0e45725a816df355a54b665a37 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 15 Apr 2015 13:51:46 -0700 Subject: [PATCH 26/34] Handle errors in more places in HasA Iterator --- graph/iterator/hasa_iterator.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/graph/iterator/hasa_iterator.go b/graph/iterator/hasa_iterator.go index f9f555b..037772b 100644 --- a/graph/iterator/hasa_iterator.go +++ b/graph/iterator/hasa_iterator.go @@ -153,7 +153,11 @@ func (it *HasA) Contains(val graph.Value) bool { it.resultIt.Close() } it.resultIt = it.qs.QuadIterator(it.dir, val) - return graph.ContainsLogOut(it, val, it.NextContains()) + ret := it.NextContains() + if it.err != nil { + return false + } + return graph.ContainsLogOut(it, val, ret) } // NextContains() is shared code between Contains() and GetNextResult() -- calls next on the @@ -171,6 +175,9 @@ func (it *HasA) NextContains() bool { return true } } + if err := it.resultIt.Err(); err != nil { + it.err = err + } return false } @@ -188,6 +195,9 @@ func (it *HasA) NextPath() bool { } result := it.NextContains() glog.V(4).Infoln("HASA", it.UID(), "NextPath Returns", result, "") + if it.err != nil { + return false + } return result } From 1990eba0551b0b018c5b998f7a09dcd694f33b2a Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 15 Apr 2015 14:07:45 -0700 Subject: [PATCH 27/34] Stop calling glog.Fatal* in a bunch of places --- graph/bolt/quadstore.go | 5 ++++- graph/iterator/value_comparison_iterator.go | 4 +--- graph/leveldb/quadstore.go | 10 ++++++++-- graph/mongo/quadstore.go | 10 ++++++++-- graph/quadstore.go | 26 +++++++++++++------------- query/mql/build_iterator.go | 3 +-- writer/single.go | 11 +++++++++-- 7 files changed, 44 insertions(+), 25 deletions(-) diff --git a/graph/bolt/quadstore.go b/graph/bolt/quadstore.go index 5b0e2e2..ecc9a81 100644 --- a/graph/bolt/quadstore.go +++ b/graph/bolt/quadstore.go @@ -92,7 +92,10 @@ func newQuadStore(path string, options graph.Options) (graph.QuadStore, error) { } qs.db = db // BoolKey returns false on non-existence. IE, Sync by default. - qs.db.NoSync, _ = options.BoolKey("nosync") + qs.db.NoSync, _, err = options.BoolKey("nosync") + if err != nil { + return nil, err + } err = qs.getMetadata() if err != nil { return nil, err diff --git a/graph/iterator/value_comparison_iterator.go b/graph/iterator/value_comparison_iterator.go index 9197172..bffd640 100644 --- a/graph/iterator/value_comparison_iterator.go +++ b/graph/iterator/value_comparison_iterator.go @@ -27,7 +27,6 @@ package iterator // In MQL terms, this is the [{"age>=": 21}] concept. import ( - "log" "strconv" "github.com/google/cayley/graph" @@ -107,8 +106,7 @@ func RunIntOp(a int64, op Operator, b int64) bool { case compareGTE: return a >= b default: - log.Fatal("Unknown operator type") - return false + panic("Unknown operator type") } } diff --git a/graph/leveldb/quadstore.go b/graph/leveldb/quadstore.go index 799688d..73cf386 100644 --- a/graph/leveldb/quadstore.go +++ b/graph/leveldb/quadstore.go @@ -90,7 +90,10 @@ func newQuadStore(path string, options graph.Options) (graph.QuadStore, error) { var err error qs.path = path cacheSize := DefaultCacheSize - if val, ok := options.IntKey("cache_size_mb"); ok { + val, ok, err := options.IntKey("cache_size_mb") + if err != nil { + return nil, err + } else if ok { cacheSize = val } qs.dbOpts = &opt.Options{ @@ -99,7 +102,10 @@ func newQuadStore(path string, options graph.Options) (graph.QuadStore, error) { qs.dbOpts.ErrorIfMissing = true writeBufferSize := DefaultWriteBufferSize - if val, ok := options.IntKey("writeBufferSize"); ok { + val, ok, err = options.IntKey("writeBufferSize") + if err != nil { + return nil, err + } else if ok { writeBufferSize = val } qs.dbOpts.WriteBuffer = writeBufferSize * opt.MiB diff --git a/graph/mongo/quadstore.go b/graph/mongo/quadstore.go index ce05efe..2a74be9 100644 --- a/graph/mongo/quadstore.go +++ b/graph/mongo/quadstore.go @@ -57,7 +57,10 @@ func createNewMongoGraph(addr string, options graph.Options) error { } conn.SetSafe(&mgo.Safe{}) dbName := DefaultDBName - if val, ok := options.StringKey("database_name"); ok { + val, ok, err := options.StringKey("database_name") + if err != nil { + return err + } else if ok { dbName = val } db := conn.DB(dbName) @@ -94,7 +97,10 @@ func newQuadStore(addr string, options graph.Options) (graph.QuadStore, error) { } conn.SetSafe(&mgo.Safe{}) dbName := DefaultDBName - if val, ok := options.StringKey("database_name"); ok { + val, ok, err := options.StringKey("database_name") + if err != nil { + return nil, err + } else if ok { dbName = val } qs.db = conn.DB(dbName) diff --git a/graph/quadstore.go b/graph/quadstore.go index e9a1a4f..8a89885 100644 --- a/graph/quadstore.go +++ b/graph/quadstore.go @@ -23,8 +23,8 @@ package graph import ( "errors" + "fmt" - "github.com/barakmich/glog" "github.com/google/cayley/quad" ) @@ -103,40 +103,40 @@ type QuadStore interface { type Options map[string]interface{} -func (d Options) IntKey(key string) (int, bool) { +func (d Options) IntKey(key string) (int, bool, error) { if val, ok := d[key]; ok { switch vv := val.(type) { case float64: - return int(vv), true + return int(vv), true, nil default: - glog.Fatalln("Invalid", key, "parameter type from config.") + return 0, false, fmt.Errorf("Invalid %s parameter type from config: %T", key, val) } } - return 0, false + return 0, false, nil } -func (d Options) StringKey(key string) (string, bool) { +func (d Options) StringKey(key string) (string, bool, error) { if val, ok := d[key]; ok { switch vv := val.(type) { case string: - return vv, true + return vv, true, nil default: - glog.Fatalln("Invalid", key, "parameter type from config.") + return "", false, fmt.Errorf("Invalid %s parameter type from config: %T", key, val) } } - return "", false + return "", false, nil } -func (d Options) BoolKey(key string) (bool, bool) { +func (d Options) BoolKey(key string) (bool, bool, error) { if val, ok := d[key]; ok { switch vv := val.(type) { case bool: - return vv, true + return vv, true, nil default: - glog.Fatalln("Invalid", key, "parameter type from config.") + return false, false, fmt.Errorf("Invalid %s parameter type from config: %T", key, val) } } - return false, false + return false, false, nil } var ErrCannotBulkLoad = errors.New("quadstore: cannot bulk load") diff --git a/query/mql/build_iterator.go b/query/mql/build_iterator.go index d1469d7..8d81717 100644 --- a/query/mql/build_iterator.go +++ b/query/mql/build_iterator.go @@ -17,7 +17,6 @@ package mql import ( "errors" "fmt" - "log" "math" "strings" @@ -93,7 +92,7 @@ func (q *Query) buildIteratorTreeInternal(query interface{}, path Path) (it grap it = q.buildResultIterator(path) optional = true default: - log.Fatal("Unknown JSON type?", query) + err = fmt.Errorf("Unknown JSON type: %T") } if err != nil { return nil, false, err diff --git a/writer/single.go b/writer/single.go index 53d46fa..49083c2 100644 --- a/writer/single.go +++ b/writer/single.go @@ -33,17 +33,24 @@ type Single struct { func NewSingleReplication(qs graph.QuadStore, opts graph.Options) (graph.QuadWriter, error) { var ignoreMissing, ignoreDuplicate bool + var err error if *graph.IgnoreMissing { ignoreMissing = true } else { - ignoreMissing, _ = opts.BoolKey("ignore_missing") + ignoreMissing, _, err = opts.BoolKey("ignore_missing") + if err != nil { + return nil, err + } } if *graph.IgnoreDup { ignoreDuplicate = true } else { - ignoreDuplicate, _ = opts.BoolKey("ignore_duplicate") + ignoreDuplicate, _, err = opts.BoolKey("ignore_duplicate") + if err != nil { + return nil, err + } } return &Single{ From 7de923d40a25796c5d2466b6eef9c8dba3366339 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 15 Apr 2015 14:14:33 -0700 Subject: [PATCH 28/34] Remove even more Fatal* calls --- appengine/appengine.go | 17 +++++++++++------ graph/gaedatastore/quadstore.go | 2 +- graph/primarykey.go | 5 +++-- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/appengine/appengine.go b/appengine/appengine.go index dcd363c..1f8706e 100644 --- a/appengine/appengine.go +++ b/appengine/appengine.go @@ -17,6 +17,7 @@ package main import ( + "fmt" "github.com/barakmich/glog" "os" "time" @@ -45,11 +46,11 @@ var ( timeout = 30 * time.Second ) -func configFrom(file string) *config.Config { +func configFrom(file string) (*config.Config, error) { // Find the file... if file != "" { if _, err := os.Stat(file); os.IsNotExist(err) { - glog.Fatalln("Cannot find specified configuration file", file, ", aborting.") + return nil, fmt.Errorf("Cannot find specified configuration file", file) } } else if _, err := os.Stat("/cayley_appengine.cfg"); err == nil { file = "/cayley_appengine.cfg" @@ -59,7 +60,7 @@ func configFrom(file string) *config.Config { } cfg, err := config.Load(file) if err != nil { - glog.Fatalln(err) + return nil, err } if cfg.DatabasePath == "" { @@ -92,15 +93,19 @@ func configFrom(file string) *config.Config { cfg.ReadOnly = cfg.ReadOnly || readOnly - return cfg + return cfg, nil } func init() { glog.SetToStderr(true) - cfg := configFrom("cayley_appengine.cfg") + cfg, err := configFrom("cayley_appengine.cfg") + if err != nil { + glog.Fatalln("Error loading config:", err) + } + handle, err := db.Open(cfg) if err != nil { - glog.Fatal(err) + glog.Fatalln("Error opening database:", err) } http.SetupRoutes(handle, cfg) } diff --git a/graph/gaedatastore/quadstore.go b/graph/gaedatastore/quadstore.go index 10dfb39..8a7b12d 100644 --- a/graph/gaedatastore/quadstore.go +++ b/graph/gaedatastore/quadstore.go @@ -154,7 +154,7 @@ func getContext(opts graph.Options) (appengine.Context, error) { req := opts["HTTPRequest"].(*http.Request) if req == nil { err := errors.New("HTTP Request needed") - glog.Fatalln(err) + glog.Errorln(err) return nil, err } return appengine.NewContext(req), nil diff --git a/graph/primarykey.go b/graph/primarykey.go index 1df1c51..0696055 100644 --- a/graph/primarykey.go +++ b/graph/primarykey.go @@ -84,8 +84,9 @@ func (p *PrimaryKey) Int() int64 { case sequential: return p.sequentialID case unique: - glog.Fatal("UUID cannot be cast to an int64") - return -1 + msg := "UUID cannot be cast to an int64" + glog.Errorln(msg) + panic(msg) } return -1 } From 430ff507f0908b9749102131137914d09ae10ecd Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 15 Apr 2015 15:28:46 -0700 Subject: [PATCH 29/34] Test for errors in .Contains() and .NextPath() --- graph/iterator/and_iterator.go | 13 +++++++++++++ graph/iterator/hasa_iterator.go | 9 +++++++-- graph/iterator/linksto_iterator.go | 9 ++++++++- graph/iterator/materialize_iterator.go | 6 ++++++ graph/iterator/not_iterator.go | 7 +++++++ graph/iterator/optional_iterator.go | 10 ++++++++-- graph/iterator/or_iterator.go | 21 ++++++++++++++++----- graph/iterator/value_comparison_iterator.go | 7 ++++++- 8 files changed, 71 insertions(+), 11 deletions(-) diff --git a/graph/iterator/and_iterator.go b/graph/iterator/and_iterator.go index f7da02f..2cf8691 100644 --- a/graph/iterator/and_iterator.go +++ b/graph/iterator/and_iterator.go @@ -190,8 +190,13 @@ 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 = err + return false + } if lastResult != nil { for j := 0; j < i; j++ { + // TODO(andrew-d): Should this result actually be used? it.checkList[j].Contains(lastResult) } } @@ -249,10 +254,18 @@ func (it *And) NextPath() bool { if it.primaryIt.NextPath() { return true } + if err := it.primaryIt.Err(); err != nil { + it.err = err + return false + } for _, sub := range it.internalIterators { if sub.NextPath() { return true } + if err := sub.Err(); err != nil { + it.err = err + return false + } } return false } diff --git a/graph/iterator/hasa_iterator.go b/graph/iterator/hasa_iterator.go index 037772b..8647dbd 100644 --- a/graph/iterator/hasa_iterator.go +++ b/graph/iterator/hasa_iterator.go @@ -193,11 +193,16 @@ func (it *HasA) NextPath() bool { if it.primaryIt.NextPath() { return true } - result := it.NextContains() - glog.V(4).Infoln("HASA", it.UID(), "NextPath Returns", result, "") + if err := it.primaryIt.Err(); err != nil { + it.err = err + return false + } + + result := it.NextContains() // Sets it.err if there's an error if it.err != nil { return false } + glog.V(4).Infoln("HASA", it.UID(), "NextPath Returns", result, "") return result } diff --git a/graph/iterator/linksto_iterator.go b/graph/iterator/linksto_iterator.go index 16affac..0eebe78 100644 --- a/graph/iterator/linksto_iterator.go +++ b/graph/iterator/linksto_iterator.go @@ -126,6 +126,9 @@ 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 = err + } return graph.ContainsLogOut(it, val, false) } @@ -213,7 +216,11 @@ func (it *LinksTo) Close() error { // We won't ever have a new result, but our subiterators might. func (it *LinksTo) NextPath() bool { - return it.primaryIt.NextPath() + ret := it.primaryIt.NextPath() + if !ret { + it.err = it.primaryIt.Err() + } + return ret } // Register the LinksTo. diff --git a/graph/iterator/materialize_iterator.go b/graph/iterator/materialize_iterator.go index cff1c88..e38ac58 100644 --- a/graph/iterator/materialize_iterator.go +++ b/graph/iterator/materialize_iterator.go @@ -230,6 +230,9 @@ func (it *Materialize) Contains(v graph.Value) bool { if !it.hasRun { it.materializeSet() } + if it.err != nil { + return false + } if it.aborted { return it.subIt.Contains(v) } @@ -249,6 +252,9 @@ func (it *Materialize) NextPath() bool { if !it.hasRun { it.materializeSet() } + if it.err != nil { + return false + } if it.aborted { return it.subIt.NextPath() } diff --git a/graph/iterator/not_iterator.go b/graph/iterator/not_iterator.go index 2617a98..a54b4bb 100644 --- a/graph/iterator/not_iterator.go +++ b/graph/iterator/not_iterator.go @@ -113,6 +113,13 @@ func (it *Not) Contains(val graph.Value) bool { return graph.ContainsLogOut(it, val, false) } + if err := it.primaryIt.Err(); err != nil { + it.err = err + + // Explicitly return 'false', since an error occurred. + return false + } + it.result = val return graph.ContainsLogOut(it, val, true) } diff --git a/graph/iterator/optional_iterator.go b/graph/iterator/optional_iterator.go index 84e7154..d645dc7 100644 --- a/graph/iterator/optional_iterator.go +++ b/graph/iterator/optional_iterator.go @@ -38,6 +38,7 @@ type Optional struct { subIt graph.Iterator lastCheck bool result graph.Value + err error } // Creates a new optional iterator. @@ -77,7 +78,7 @@ func (it *Optional) ResultTree() *graph.ResultTree { } func (it *Optional) Err() error { - return it.subIt.Err() + return it.err } func (it *Optional) Result() graph.Value { @@ -89,7 +90,11 @@ func (it *Optional) Result() graph.Value { // optional subbranch. func (it *Optional) NextPath() bool { if it.lastCheck { - return it.subIt.NextPath() + ret := it.subIt.NextPath() + if !ret { + it.err = it.subIt.Err() + } + return ret } return false } @@ -105,6 +110,7 @@ func (it *Optional) SubIterators() []graph.Iterator { func (it *Optional) Contains(val graph.Value) bool { checked := it.subIt.Contains(val) it.lastCheck = checked + it.err = it.subIt.Err() it.result = val return true } diff --git a/graph/iterator/or_iterator.go b/graph/iterator/or_iterator.go index e5b4e09..024ce74 100644 --- a/graph/iterator/or_iterator.go +++ b/graph/iterator/or_iterator.go @@ -174,7 +174,7 @@ func (it *Or) Result() graph.Value { } // Checks a value against the iterators, in order. -func (it *Or) subItsContain(val graph.Value) bool { +func (it *Or) subItsContain(val graph.Value) (bool, error) { var subIsGood = false for i, sub := range it.internalIterators { subIsGood = sub.Contains(val) @@ -182,15 +182,21 @@ func (it *Or) subItsContain(val graph.Value) bool { it.currentIterator = i break } + if err := sub.Err(); err != nil { + return false, err + } } - return subIsGood + return subIsGood, nil } // Check a value against the entire graph.iterator, in order. func (it *Or) Contains(val graph.Value) bool { graph.ContainsLogIn(it, val) - anyGood := it.subItsContain(val) - if !anyGood { + anyGood, err := it.subItsContain(val) + if err != nil { + it.err = err + return false + } else if !anyGood { return graph.ContainsLogOut(it, val, false) } it.result = val @@ -231,7 +237,12 @@ func (it *Or) Size() (int64, bool) { // shortcircuiting, only allow new results from the currently checked graph.iterator func (it *Or) NextPath() bool { if it.currentIterator != -1 { - return it.internalIterators[it.currentIterator].NextPath() + currIt := it.internalIterators[it.currentIterator] + ret := currIt.NextPath() + if !ret { + it.err = currIt.Err() + } + return ret } return false } diff --git a/graph/iterator/value_comparison_iterator.go b/graph/iterator/value_comparison_iterator.go index bffd640..84d949d 100644 --- a/graph/iterator/value_comparison_iterator.go +++ b/graph/iterator/value_comparison_iterator.go @@ -155,6 +155,7 @@ func (it *Comparison) NextPath() bool { for { hasNext := it.subIt.NextPath() if !hasNext { + it.err = it.subIt.Err() return false } if it.doComparison(it.subIt.Result()) { @@ -174,7 +175,11 @@ func (it *Comparison) Contains(val graph.Value) bool { if !it.doComparison(val) { return false } - return it.subIt.Contains(val) + ret := it.subIt.Contains(val) + if !ret { + it.err = it.subIt.Err() + } + return ret } // If we failed the check, then the subiterator should not contribute to the result From 5eed4d966775c61fb05a69ee8c5352de5b5a2602 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 15 Apr 2015 16:28:13 -0700 Subject: [PATCH 30/34] Address review comments --- graph/iterator.go | 2 +- graph/iterator/all_iterator.go | 1 - graph/iterator/and_iterator.go | 50 ++++++++++++++---------- graph/iterator/and_iterator_test.go | 6 +-- graph/iterator/fixed_iterator.go | 1 - graph/iterator/hasa_iterator.go | 36 +++++++---------- graph/iterator/hasa_iterator_test.go | 6 +-- graph/iterator/linksto_iterator.go | 34 +++++++--------- graph/iterator/materialize_iterator.go | 11 ++---- graph/iterator/materialize_iterator_test.go | 18 ++++----- graph/iterator/not_iterator.go | 27 ++++++------- graph/iterator/not_iterator_test.go | 6 +-- graph/iterator/optional_iterator.go | 6 +-- graph/iterator/or_iterator.go | 23 +++++------ graph/iterator/or_iterator_test.go | 12 +++--- graph/iterator/value_comparison_iterator.go | 10 ++--- graph/iterator/value_comparison_iterator_test.go | 6 +-- graph/primarykey.go | 2 +- writer/single.go | 7 +++- 19 files changed, 125 insertions(+), 139 deletions(-) diff --git a/graph/iterator.go b/graph/iterator.go index 9204c5c..281ea64 100644 --- a/graph/iterator.go +++ b/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 diff --git a/graph/iterator/all_iterator.go b/graph/iterator/all_iterator.go index 3e37563..26e6302 100644 --- a/graph/iterator/all_iterator.go +++ b/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 } diff --git a/graph/iterator/and_iterator.go b/graph/iterator/and_iterator.go index 2cf8691..c6aedc5 100644 --- a/graph/iterator/and_iterator.go +++ b/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 = err - } + it.err = it.primaryIt.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 = err + it.err = c.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 = err + it.err = it.primaryIt.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. -// -// Note: as this potentially involves closing multiple subiterators, only -// the first error encountered while closing will be reported (if any). +// follow this contract, the And follows the contract. It closes all +// subiterators it can, but returns the first error it encounters. func (it *And) Close() error { it.cleanUp() - var ret error - - ret = it.primaryIt.Close() + err := it.primaryIt.Close() for _, sub := range it.internalIterators { - if err := sub.Close(); err != nil && ret != nil { - ret = err + serr := sub.Close() + if serr != nil && err == nil { + err = serr } } - return ret + return err } // Register this as an "and" iterator. diff --git a/graph/iterator/and_iterator_test.go b/graph/iterator/and_iterator_test.go index 1bc2182..1a9c131 100644 --- a/graph/iterator/and_iterator_test.go +++ b/graph/iterator/and_iterator_test.go @@ -142,8 +142,8 @@ func TestAllIterators(t *testing.T) { } func TestAndIteratorErr(t *testing.T) { - retErr := errors.New("unique") - allErr := newTestIterator(false, retErr) + wantErr := errors.New("unique") + 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") } } diff --git a/graph/iterator/fixed_iterator.go b/graph/iterator/fixed_iterator.go index 35c2690..fe112bc 100644 --- a/graph/iterator/fixed_iterator.go +++ b/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 } diff --git a/graph/iterator/hasa_iterator.go b/graph/iterator/hasa_iterator.go index 8647dbd..0a6b051 100644 --- a/graph/iterator/hasa_iterator.go +++ b/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 = err - } + it.err = it.resultIt.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 = err + it.err = it.primaryIt.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 = err - } + it.err = it.primaryIt.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. -// -// Note: as this involves closing multiple iterators, only the first error -// encountered while closing will be reported (if any). +// Close the subiterator, the result iterator (if any) and the HasA. It closes +// all subiterators it can, but returns the first error it encounters. func (it *HasA) Close() error { - var ret error + err := it.primaryIt.Close() if it.resultIt != nil { - ret = it.resultIt.Close() - } - if err := it.primaryIt.Close(); err != nil && ret != nil { - ret = err + err2 := it.resultIt.Close() + if err == nil { + err = err2 + } } - return ret + return err } // Register this iterator as a HasA. diff --git a/graph/iterator/hasa_iterator_test.go b/graph/iterator/hasa_iterator_test.go index fbeb228..d7498cf 100644 --- a/graph/iterator/hasa_iterator_test.go +++ b/graph/iterator/hasa_iterator_test.go @@ -22,8 +22,8 @@ import ( ) func TestHasAIteratorErr(t *testing.T) { - retErr := errors.New("unique") - errIt := newTestIterator(false, retErr) + wantErr := errors.New("unique") + 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") } } diff --git a/graph/iterator/linksto_iterator.go b/graph/iterator/linksto_iterator.go index 0eebe78..cd65c5c 100644 --- a/graph/iterator/linksto_iterator.go +++ b/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 = err - } + it.err = it.primaryIt.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 = err + it.err = it.nextIt.Err() + if it.err != nil { return false } @@ -197,30 +195,26 @@ func (it *LinksTo) Result() graph.Value { return it.result } -// Close our subiterators. -// -// Note: as this involves closing multiple subiterators, only the first error -// encountered while closing will be reported (if any). +// Close our subiterators. It closes all subiterators it can, but +// returns the first error it encounters. func (it *LinksTo) Close() error { - var ret error + err := it.nextIt.Close() - if err := it.nextIt.Close(); err != nil && ret != nil { - ret = err - } - if err := it.primaryIt.Close(); err != nil && ret != nil { - ret = err + err2 := it.primaryIt.Close() + if err2 != nil && err == nil { + err = err2 } - 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() - if !ret { + ok := it.primaryIt.NextPath() + if !ok { it.err = it.primaryIt.Err() } - return ret + return ok } // Register the LinksTo. diff --git a/graph/iterator/materialize_iterator.go b/graph/iterator/materialize_iterator.go index e38ac58..f6421d8 100644 --- a/graph/iterator/materialize_iterator.go +++ b/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 = err - } + it.err = it.subIt.Err() return n } @@ -302,9 +300,8 @@ func (it *Materialize) materializeSet() { it.actualSize += 1 } } - if err := it.subIt.Err(); err != nil { - it.err = err - } else if it.aborted { + it.err = it.subIt.Err() + if it.err == nil && it.aborted { if glog.V(2) { glog.V(2).Infoln("Aborting subiterator") } diff --git a/graph/iterator/materialize_iterator_test.go b/graph/iterator/materialize_iterator_test.go index aa749c5..8188584 100644 --- a/graph/iterator/materialize_iterator_test.go +++ b/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") - errIt := newTestIterator(false, retErr) + wantErr := errors.New("unique") + 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") - errIt := newTestIterator(false, retErr) + wantErr := errors.New("unique") + 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") } } diff --git a/graph/iterator/not_iterator.go b/graph/iterator/not_iterator.go index a54b4bb..fc1e1e3 100644 --- a/graph/iterator/not_iterator.go +++ b/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 = err - } + it.err = it.allIt.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 = err - + it.err = it.primaryIt.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 -// first one will be returned. +// Close closes the primary and all iterators. It closes all subiterators +// 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 { - ret = err - } - if err := it.allIt.Close(); err != nil && ret != nil { - ret = err + err2 := it.allIt.Close() + if err2 != nil && err == nil { + err = err2 } - return ret + return err } func (it *Not) Type() graph.Type { return graph.Not } diff --git a/graph/iterator/not_iterator_test.go b/graph/iterator/not_iterator_test.go index e63a0a0..d701513 100644 --- a/graph/iterator/not_iterator_test.go +++ b/graph/iterator/not_iterator_test.go @@ -45,8 +45,8 @@ func TestNotIteratorBasics(t *testing.T) { } func TestNotIteratorErr(t *testing.T) { - retErr := errors.New("unique") - allIt := newTestIterator(false, retErr) + wantErr := errors.New("unique") + 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") } } diff --git a/graph/iterator/optional_iterator.go b/graph/iterator/optional_iterator.go index d645dc7..dbf654c 100644 --- a/graph/iterator/optional_iterator.go +++ b/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() - if !ret { + ok := it.subIt.NextPath() + if !ok { it.err = it.subIt.Err() } - return ret + return ok } return false } diff --git a/graph/iterator/or_iterator.go b/graph/iterator/or_iterator.go index 024ce74..6bfa54c 100644 --- a/graph/iterator/or_iterator.go +++ b/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 = err + it.err = curIt.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() - if !ret { + ok := currIt.NextPath() + 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. -// -// Note: as this potentially involves closing multiple subiterators, only -// the first error encountered while closing will be reported (if any). +// follow this contract, the Or follows the contract. It closes all +// subiterators it can, but returns the first error it encounters. 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 } } diff --git a/graph/iterator/or_iterator_test.go b/graph/iterator/or_iterator_test.go index edbd6d4..7c4f2ef 100644 --- a/graph/iterator/or_iterator_test.go +++ b/graph/iterator/or_iterator_test.go @@ -151,8 +151,8 @@ func TestShortCircuitingOrBasics(t *testing.T) { } func TestOrIteratorErr(t *testing.T) { - retErr := errors.New("unique") - orErr := newTestIterator(false, retErr) + wantErr := errors.New("unique") + 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") - orErr := newTestIterator(false, retErr) + wantErr := errors.New("unique") + 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") } } diff --git a/graph/iterator/value_comparison_iterator.go b/graph/iterator/value_comparison_iterator.go index 84d949d..0351d60 100644 --- a/graph/iterator/value_comparison_iterator.go +++ b/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 = err - } + it.err = it.subIt.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) - if !ret { + ok := it.subIt.Contains(val) + 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 diff --git a/graph/iterator/value_comparison_iterator_test.go b/graph/iterator/value_comparison_iterator_test.go index 64ce5e9..ccb8c54 100644 --- a/graph/iterator/value_comparison_iterator_test.go +++ b/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") - errIt := newTestIterator(false, retErr) + wantErr := errors.New("unique") + 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") } } diff --git a/graph/primarykey.go b/graph/primarykey.go index 0696055..5332a8f 100644 --- a/graph/primarykey.go +++ b/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) } diff --git a/writer/single.go b/writer/single.go index 49083c2..c62788b 100644 --- a/writer/single.go +++ b/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 err error + var ( + ignoreMissing bool + ignoreDuplicate bool + err error + ) if *graph.IgnoreMissing { ignoreMissing = true From 131f5de7ea33821bef29ef1a8dd23cbc86efd0fc Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 15 Apr 2015 16:30:01 -0700 Subject: [PATCH 31/34] Fix a missed review comment --- graph/iterator/or_iterator.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/graph/iterator/or_iterator.go b/graph/iterator/or_iterator.go index 6bfa54c..9cb1660 100644 --- a/graph/iterator/or_iterator.go +++ b/graph/iterator/or_iterator.go @@ -259,15 +259,15 @@ func (it *Or) cleanUp() {} func (it *Or) Close() error { it.cleanUp() - var ret error + var err error for _, sub := range it.internalIterators { - err := sub.Close() - if err != nil && ret == nil { - ret = err + serr := sub.Close() + if serr != nil && err == nil { + err = serr } } - return ret + return err } func (it *Or) Optimize() (graph.Iterator, bool) { From c9de029225730419bab0019e3e173d2f1c0ff6f4 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 15 Apr 2015 16:39:51 -0700 Subject: [PATCH 32/34] s/(serr|err2)/_err/g --- graph/iterator/and_iterator.go | 6 +++--- graph/iterator/hasa_iterator.go | 4 ++-- graph/iterator/linksto_iterator.go | 6 +++--- graph/iterator/not_iterator.go | 6 +++--- graph/iterator/or_iterator.go | 6 +++--- 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/graph/iterator/and_iterator.go b/graph/iterator/and_iterator.go index c6aedc5..418a469 100644 --- a/graph/iterator/and_iterator.go +++ b/graph/iterator/and_iterator.go @@ -293,9 +293,9 @@ func (it *And) Close() error { err := it.primaryIt.Close() for _, sub := range it.internalIterators { - serr := sub.Close() - if serr != nil && err == nil { - err = serr + _err := sub.Close() + if _err != nil && err == nil { + err = _err } } diff --git a/graph/iterator/hasa_iterator.go b/graph/iterator/hasa_iterator.go index 0a6b051..a799bdd 100644 --- a/graph/iterator/hasa_iterator.go +++ b/graph/iterator/hasa_iterator.go @@ -263,9 +263,9 @@ func (it *HasA) Close() error { err := it.primaryIt.Close() if it.resultIt != nil { - err2 := it.resultIt.Close() + _err := it.resultIt.Close() if err == nil { - err = err2 + err = _err } } diff --git a/graph/iterator/linksto_iterator.go b/graph/iterator/linksto_iterator.go index cd65c5c..2a4cd7d 100644 --- a/graph/iterator/linksto_iterator.go +++ b/graph/iterator/linksto_iterator.go @@ -200,9 +200,9 @@ func (it *LinksTo) Result() graph.Value { func (it *LinksTo) Close() error { err := it.nextIt.Close() - err2 := it.primaryIt.Close() - if err2 != nil && err == nil { - err = err2 + _err := it.primaryIt.Close() + if _err != nil && err == nil { + err = _err } return err diff --git a/graph/iterator/not_iterator.go b/graph/iterator/not_iterator.go index fc1e1e3..98232d6 100644 --- a/graph/iterator/not_iterator.go +++ b/graph/iterator/not_iterator.go @@ -132,9 +132,9 @@ func (it *Not) NextPath() bool { func (it *Not) Close() error { err := it.primaryIt.Close() - err2 := it.allIt.Close() - if err2 != nil && err == nil { - err = err2 + _err := it.allIt.Close() + if _err != nil && err == nil { + err = _err } return err diff --git a/graph/iterator/or_iterator.go b/graph/iterator/or_iterator.go index 9cb1660..25524e6 100644 --- a/graph/iterator/or_iterator.go +++ b/graph/iterator/or_iterator.go @@ -261,9 +261,9 @@ func (it *Or) Close() error { var err error for _, sub := range it.internalIterators { - serr := sub.Close() - if serr != nil && err == nil { - err = serr + _err := sub.Close() + if _err != nil && err == nil { + err = _err } } From 8abb3807cbbe688309aa3d5283cd00b4a091eeea Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 15 Apr 2015 16:49:47 -0700 Subject: [PATCH 33/34] Move 'err' to the end of the struct --- graph/bolt/iterator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graph/bolt/iterator.go b/graph/bolt/iterator.go index 6a28422..d080391 100644 --- a/graph/bolt/iterator.go +++ b/graph/bolt/iterator.go @@ -46,11 +46,11 @@ type Iterator struct { dir quad.Direction qs *QuadStore result *Token - err error buffer [][]byte offset int done bool size int64 + err error } func NewIterator(bucket []byte, d quad.Direction, value graph.Value, qs *QuadStore) *Iterator { From d91bb5cb3f3b29ca0da0a194d8c76376ca676082 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 15 Apr 2015 16:51:30 -0700 Subject: [PATCH 34/34] Fix phrasing in a comment --- graph/iterator/linksto_iterator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graph/iterator/linksto_iterator.go b/graph/iterator/linksto_iterator.go index 2a4cd7d..930e929 100644 --- a/graph/iterator/linksto_iterator.go +++ b/graph/iterator/linksto_iterator.go @@ -195,7 +195,7 @@ func (it *LinksTo) Result() graph.Value { return it.result } -// Close our subiterators. It closes all subiterators it can, but +// Close closes the iterator. It closes all subiterators it can, but // returns the first error it encounters. func (it *LinksTo) Close() error { err := it.nextIt.Close()