Return error on wrong use of the Paginator

`Paginate`now returns error when

1) `.Paginate` is called after `.Paginator`
2) `.Paginate` is repeatedly called with different arguments

This should help remove some confusion.

This commit also introduces DistinctErrorLogger, to prevent spamming the log for duplicate rendering errors from the pagers.

Fixes #993

3 files changed, 143 insertions, 21 deletions

diff --git a/hugolib/pagination.go b/hugolib/pagination.go
index 650a355cf..f5380a337 100644
--- a/hugolib/pagination.go
+++ b/hugolib/pagination.go
@@ -22,6 +22,7 @@ import (
 	"html/template"
 	"math"
 	"path"
+	"reflect"
 )
 
 type pager struct {
@@ -37,8 +38,10 @@ type paginator struct {
 	paginatedPages []Pages
 	pagers
 	paginationURLFactory
-	total int
-	size  int
+	total   int
+	size    int
+	source  interface{}
+	options []interface{}
 }
 
 type paginationURLFactory func(int) string
@@ -164,6 +167,8 @@ func (n *Node) Paginator(options ...interface{}) (*pager, error) {
 		if len(pagers) > 0 {
 			// the rest of the nodes will be created later
 			n.paginator = pagers[0]
+			n.paginator.source = "paginator"
+			n.paginator.options = options
 			n.Site.addToPaginationPageCount(uint64(n.paginator.TotalPages()))
 		}
 
@@ -212,6 +217,8 @@ func (n *Node) Paginate(seq interface{}, options ...interface{}) (*pager, error)
 		if len(pagers) > 0 {
 			// the rest of the nodes will be created later
 			n.paginator = pagers[0]
+			n.paginator.source = seq
+			n.paginator.options = options
 			n.Site.addToPaginationPageCount(uint64(n.paginator.TotalPages()))
 		}
 
@@ -221,6 +228,14 @@ func (n *Node) Paginate(seq interface{}, options ...interface{}) (*pager, error)
 		return nil, initError
 	}
 
+	if n.paginator.source == "paginator" {
+		return nil, errors.New("a Paginator was previously built for this Node without filters; look for earlier .Paginator usage")
+	} else {
+		if !reflect.DeepEqual(options, n.paginator.options) || !probablyEqualPageLists(n.paginator.source, seq) {
+			return nil, errors.New("invoked multiple times with different arguments")
+		}
+	}
+
 	return n.paginator, nil
 }
 
@@ -248,25 +263,64 @@ func paginatePages(seq interface{}, pagerSize int, section string) (pagers, erro
 		return nil, errors.New("'paginate' configuration setting must be positive to paginate")
 	}
 
-	var pages Pages
+	pages, err := toPages(seq)
+	if err != nil {
+		return nil, err
+	}
+
+	urlFactory := newPaginationURLFactory(section)
+	paginator, _ := newPaginator(pages, pagerSize, urlFactory)
+	pagers := paginator.Pagers()
+
+	return pagers, nil
+}
+
+func toPages(seq interface{}) (Pages, error) {
 	switch seq.(type) {
 	case Pages:
-		pages = seq.(Pages)
+		return seq.(Pages), nil
 	case *Pages:
-		pages = *(seq.(*Pages))
+		return *(seq.(*Pages)), nil
 	case WeightedPages:
-		pages = (seq.(WeightedPages)).Pages()
+		return (seq.(WeightedPages)).Pages(), nil
 	case PageGroup:
-		pages = (seq.(PageGroup)).Pages
+		return (seq.(PageGroup)).Pages, nil
 	default:
 		return nil, errors.New(fmt.Sprintf("unsupported type in paginate, got %T", seq))
 	}
+}
 
-	urlFactory := newPaginationURLFactory(section)
-	paginator, _ := newPaginator(pages, pagerSize, urlFactory)
-	pagers := paginator.Pagers()
+// probablyEqual checks page lists for probable equality.
+// It may return false positives.
+// The motivation behind this is to avoid potential costly reflect.DeepEqual
+// when "probably" is good enough.
+func probablyEqualPageLists(a1 interface{}, a2 interface{}) bool {
 
-	return pagers, nil
+	if a1 == nil || a2 == nil {
+		return a1 == a2
+	}
+
+	p1, err1 := toPages(a1)
+	p2, err2 := toPages(a2)
+
+	// probably the same wrong type
+	if err1 != nil && err2 != nil {
+		return true
+	}
+
+	if err1 != nil || err2 != nil {
+		return false
+	}
+
+	if len(p1) != len(p2) {
+		return false
+	}
+
+	if len(p1) == 0 {
+		return true
+	}
+
+	return p1[0] == p2[0]
 }
 
 func newPaginator(pages Pages, size int, urlFactory paginationURLFactory) (*paginator, error) {
diff --git a/hugolib/pagination_test.go b/hugolib/pagination_test.go
index fde81dfb2..45655ef94 100644
--- a/hugolib/pagination_test.go
+++ b/hugolib/pagination_test.go
@@ -184,7 +184,7 @@ func doTestPaginate(t *testing.T, useViper bool) {
 	n1 := s.newHomeNode()
 	n2 := s.newHomeNode()
 
-	var paginator1 *pager
+	var paginator1, paginator2 *pager
 	var err error
 
 	if useViper {
@@ -199,13 +199,14 @@ func doTestPaginate(t *testing.T, useViper bool) {
 	assert.Equal(t, 6, paginator1.TotalNumberOfElements())
 
 	n2.paginator = paginator1.Next()
-	paginator2, err := n2.Paginate(pages)
+	if useViper {
+		paginator2, err = n2.Paginate(pages)
+	} else {
+		paginator2, err = n2.Paginate(pages, pagerSize)
+	}
 	assert.Nil(t, err)
 	assert.Equal(t, paginator2, paginator1.Next())
 
-	samePaginator, err := n1.Paginate(createTestPages(2))
-	assert.Equal(t, paginator1, samePaginator)
-
 	p, _ := NewPage("test")
 	_, err = p.Paginate(pages)
 	assert.NotNil(t, err)
@@ -240,6 +241,71 @@ func TestPaginatePages(t *testing.T) {
 
 }
 
+// Issue #993
+func TestPaginatorFollowedByPaginateShouldFail(t *testing.T) {
+	viper.Set("paginate", 10)
+	s := &Site{}
+	n1 := s.newHomeNode()
+	n2 := s.newHomeNode()
+
+	_, err := n1.Paginator()
+	assert.Nil(t, err)
+	_, err = n1.Paginate(createTestPages(2))
+	assert.NotNil(t, err)
+
+	_, err = n2.Paginate(createTestPages(2))
+	assert.Nil(t, err)
+
+}
+
+func TestPaginateFollowedByDifferentPaginateShouldFail(t *testing.T) {
+
+	viper.Set("paginate", 10)
+	s := &Site{}
+	n1 := s.newHomeNode()
+	n2 := s.newHomeNode()
+
+	p1 := createTestPages(2)
+	p2 := createTestPages(10)
+
+	_, err := n1.Paginate(p1)
+	assert.Nil(t, err)
+
+	_, err = n1.Paginate(p1)
+	assert.Nil(t, err)
+
+	_, err = n1.Paginate(p2)
+	assert.NotNil(t, err)
+
+	_, err = n2.Paginate(p2)
+	assert.Nil(t, err)
+}
+
+func TestProbablyEqualPageLists(t *testing.T) {
+	fivePages := createTestPages(5)
+	zeroPages := createTestPages(0)
+	for i, this := range []struct {
+		v1     interface{}
+		v2     interface{}
+		expect bool
+	}{
+		{nil, nil, true},
+		{"a", "b", true},
+		{"a", fivePages, false},
+		{fivePages, "a", false},
+		{fivePages, createTestPages(2), false},
+		{fivePages, fivePages, true},
+		{zeroPages, zeroPages, true},
+	} {
+		result := probablyEqualPageLists(this.v1, this.v2)
+
+		if result != this.expect {
+			t.Errorf("[%d] Got %t but expected %t", i, result, this.expect)
+
+		}
+	}
+}
+
 func createTestPages(num int) Pages {
 	pages := make(Pages, num)
 
diff --git a/hugolib/site.go b/hugolib/site.go
index e05054847..4f3fd1a81 100644
--- a/hugolib/site.go
+++ b/hugolib/site.go
@@ -48,6 +48,8 @@ var _ = transform.AbsURL
 
 var DefaultTimer *nitro.B
 
+var distinctErrorLogger = helpers.NewDistinctErrorLogger()
+
 // Site contains all the information relevant for constructing a static
 // site.  The basic flow of information is as follows:
 //
@@ -1086,7 +1088,7 @@ func taxonomyRenderer(s *Site, taxes <-chan taxRenderInfo, results chan<- error,
 				}
 				pageNumber := i + 1
 				htmlBase := fmt.Sprintf("/%s/%s/%d", base, paginatePath, pageNumber)
-				if err := s.renderAndWritePage(fmt.Sprintf("taxononomy_%s_%d", t.singular, pageNumber), htmlBase, taxonomyPagerNode, layouts...); err != nil {
+				if err := s.renderAndWritePage(fmt.Sprintf("taxononomy %s", t.singular), htmlBase, taxonomyPagerNode, layouts...); err != nil {
 					results <- err
 					continue
 				}
@@ -1155,7 +1157,7 @@ func (s *Site) RenderSectionLists() error {
 
 		n := s.newSectionListNode(section, data)
 
-		if err := s.renderAndWritePage(fmt.Sprintf("section%s_%d", section, 1), fmt.Sprintf("/%s", section), n, s.appendThemeTemplates(layouts)...); err != nil {
+		if err := s.renderAndWritePage(fmt.Sprintf("section %s", section), fmt.Sprintf("/%s", section), n, s.appendThemeTemplates(layouts)...); err != nil {
 			return err
 		}
 
@@ -1181,7 +1183,7 @@ func (s *Site) RenderSectionLists() error {
 				}
 				pageNumber := i + 1
 				htmlBase := fmt.Sprintf("/%s/%s/%d", section, paginatePath, pageNumber)
-				if err := s.renderAndWritePage(fmt.Sprintf("section_%s_%d", section, pageNumber), filepath.FromSlash(htmlBase), sectionPagerNode, layouts...); err != nil {
+				if err := s.renderAndWritePage(fmt.Sprintf("section %s", section), filepath.FromSlash(htmlBase), sectionPagerNode, layouts...); err != nil {
 					return err
 				}
 			}
@@ -1238,7 +1240,7 @@ func (s *Site) RenderHomePage() error {
 			}
 			pageNumber := i + 1
 			htmlBase := fmt.Sprintf("/%s/%d", paginatePath, pageNumber)
-			if err := s.renderAndWritePage(fmt.Sprintf("homepage_%d", pageNumber), filepath.FromSlash(htmlBase), homePagerNode, layouts...); err != nil {
+			if err := s.renderAndWritePage(fmt.Sprintf("homepage"), filepath.FromSlash(htmlBase), homePagerNode, layouts...); err != nil {
 				return err
 			}
 		}
@@ -1424,7 +1426,7 @@ func (s *Site) render(name string, d interface{}, renderBuffer *bytes.Buffer, la
 
 	if err := s.renderThing(d, layout, renderBuffer); err != nil {
 		// Behavior here should be dependent on if running in server or watch mode.
-		jww.ERROR.Println(fmt.Errorf("Error while rendering %s: %v", name, err))
+		distinctErrorLogger.Printf("Error while rendering %s: %v", name, err)
 		if !s.Running() {
 			os.Exit(-1)
 		}