From f15b4ba317e7b4e3768a1a2002133329b5faa77a Mon Sep 17 00:00:00 2001 From: Jacek Kucharczyk Date: Fri, 20 Sep 2019 17:37:00 +0200 Subject: [PATCH 1/2] Fixed pagination issues. Added more tests on pagination and sorts. --- query/pagination.go | 59 ++++-- query/pagination_test.go | 372 ++++++++++++++++++++++++++++++++++++++ query/scope-sorts_test.go | 50 +++++ query/scope.go | 2 +- 4 files changed, 468 insertions(+), 15 deletions(-) create mode 100644 query/scope-sorts_test.go diff --git a/query/pagination.go b/query/pagination.go index c2b8383..70af69f 100644 --- a/query/pagination.go +++ b/query/pagination.go @@ -39,6 +39,24 @@ const ( PageNumberPagination ) +// NewPaginationLimitOffset creates new Limit Offset pagination +func NewPaginationLimitOffset(limit, offset int64) (*Pagination, error) { + pagination := &Pagination{Type: LimitOffsetPagination, Size: limit, Offset: offset} + if err := pagination.IsValid(); err != nil { + return nil, err + } + return pagination, nil +} + +// NewPaginationNumberSize sets the pagination of the type TpPage with the page 'number' and page 'size'. +func NewPaginationNumberSize(number, size int64) (*Pagination, error) { + pagination := &Pagination{Size: size, Offset: number, Type: PageNumberPagination} + if err := pagination.IsValid(); err != nil { + return nil, err + } + return pagination, nil +} + // Pagination defines the query limits and offsets. // It defines the maximum size (Limit) as well as an offset at which // the query should start. @@ -199,14 +217,25 @@ func (p *Pagination) Last(total int64) (*Pagination, error) { var last *Pagination switch p.Type { case LimitOffsetPagination: - offset := total - p.Size - // in case when total size is lower then the pagination size set the offset to 0 - if offset < 0 { - offset = 0 + var offset int64 + + // check if the last page is not partial + if partialSize := p.Offset % p.Size; partialSize != 0 { + lastFull := (total - partialSize) / p.Size + offset = lastFull*p.Size + partialSize + } else { + // the last should be total/p.Size - (10-3)/2 = 3 + // 3 * size = 3 * 2 = 6 + offset = total - p.Size + // in case when total size is lower then the pagination size set the offset to 0 + if offset < 0 { + offset = 0 + } } if offset == p.Offset { return p, nil } + // the offset should be total / p.Size last = &Pagination{Size: p.Size, Offset: offset, Type: LimitOffsetPagination} case PageNumberPagination: // divide total number of instances by the page size. @@ -215,9 +244,8 @@ func (p *Pagination) Last(total int64) (*Pagination, error) { // pageSize - 10 // computedPageNumber = 52/10 = 5 pageNumber := total / p.Size - if pageNumber == 0 { - // in case when 'total' < pageSize the pageNumber = 1 - pageNumber = 1 + if total%p.Size != 0 || pageNumber == 0 { + pageNumber++ } last = &Pagination{Size: p.Size, Offset: pageNumber, Type: PageNumberPagination} default: @@ -246,7 +274,7 @@ func (p *Pagination) Next(total int64) (*Pagination, error) { // in example: // total 52; p.Offset = 50; p.Size = 10 // the next offset would be 60 which overflows possible total values. - if offset > total { + if offset >= total { return p, nil } next = &Pagination{Offset: offset, Size: p.Size, Type: LimitOffsetPagination} @@ -256,10 +284,13 @@ func (p *Pagination) Next(total int64) (*Pagination, error) { // in example: // total: 52; pageNumber: 6; pageSize: 10; // nextTotal = pageNumber * pageSize = 60 - if p.Size*(p.Offset) > total { + // 50 - (10 * 4+1) <= 0 ? + // + nextPageNumber := p.Offset + 1 + if total-(p.Size*(nextPageNumber)) <= 0 { return p, nil } - next = &Pagination{Offset: p.Offset + 1, Size: p.Size, Type: PageNumberPagination} + next = &Pagination{Offset: nextPageNumber, Size: p.Size, Type: PageNumberPagination} default: return nil, errors.NewDet(class.QueryPaginationType, "invalid pagination type") } @@ -280,12 +311,12 @@ func (p *Pagination) Previous() (*Pagination, error) { if p.Offset == 0 { return p, nil } - offset := p.Offset - p.Size - if offset < 0 { - return p, nil + if p.Offset < p.Size { + prev = &Pagination{Offset: 0, Size: p.Offset, Type: LimitOffsetPagination} + return prev, nil } // keep the same size but change the offset - prev = &Pagination{Offset: offset, Size: p.Size, Type: LimitOffsetPagination} + prev = &Pagination{Offset: p.Offset - p.Size, Size: p.Size, Type: LimitOffsetPagination} case PageNumberPagination: if p.Offset <= 1 { return p, nil diff --git a/query/pagination_test.go b/query/pagination_test.go index 1747e13..57c7840 100644 --- a/query/pagination_test.go +++ b/query/pagination_test.go @@ -107,3 +107,375 @@ func TestGetNumberSize(t *testing.T) { assert.Equal(t, int64(1), number) }) } + +// TestPaginationNext tests the pagination next function. +func TestPaginationNext(t *testing.T) { + t.Run("InvalidTotal", func(t *testing.T) { + p := &Pagination{Type: LimitOffsetPagination, Size: 10, Offset: 0} + _, err := p.Next(-1) + assert.Error(t, err) + }) + + t.Run("Invalid", func(t *testing.T) { + p := &Pagination{Type: LimitOffsetPagination, Size: 10, Offset: -1} + _, err := p.Next(100) + assert.Error(t, err) + }) + + t.Run("LimitOffset", func(t *testing.T) { + t.Run("Simple", func(t *testing.T) { + p := &Pagination{Type: LimitOffsetPagination, Size: 10, Offset: 10} + next, err := p.Next(100) + require.NoError(t, err) + + assert.Equal(t, int64(10), next.Size) + assert.Equal(t, int64(20), next.Offset) + }) + + t.Run("Last", func(t *testing.T) { + p := &Pagination{Type: LimitOffsetPagination, Size: 10, Offset: 90} + next, err := p.Next(100) + require.NoError(t, err) + + // the pagination should not change + assert.Equal(t, p, next) + }) + + t.Run("PartialLast", func(t *testing.T) { + p := &Pagination{Type: LimitOffsetPagination, Size: 5, Offset: 22} + next, err := p.Next(30) + require.NoError(t, err) + + assert.Equal(t, int64(22+5), next.Offset) + assert.Equal(t, int64(5), next.Size) + }) + }) + + t.Run("PageSizeNumber", func(t *testing.T) { + t.Run("Simple", func(t *testing.T) { + p := &Pagination{Type: PageNumberPagination, Size: 10, Offset: 3} + next, err := p.Next(50) + require.NoError(t, err) + + assert.NotEqual(t, p, next) + + if assert.Equal(t, PageNumberPagination, next.Type) { + number, size := next.GetNumberSize() + assert.Equal(t, int64(10), size) + assert.Equal(t, int64(4), number) + } + }) + + t.Run("Last", func(t *testing.T) { + p := &Pagination{Type: PageNumberPagination, Size: 10, Offset: 4} + next, err := p.Next(50) + require.NoError(t, err) + + assert.Equal(t, p, next) + }) + + t.Run("Partial", func(t *testing.T) { + p := &Pagination{Type: PageNumberPagination, Size: 10, Offset: 4} + next, err := p.Next(48) + require.NoError(t, err) + + assert.Equal(t, p, next) + }) + }) +} + +// TestPaginationPrevious tests the pagination 'Previous' function. +func TestPaginationPrevious(t *testing.T) { + t.Run("LimitOffset", func(t *testing.T) { + t.Run("Simple#01", func(t *testing.T) { + p := Pagination{Type: LimitOffsetPagination, Offset: 20, Size: 10} + prev, err := p.Previous() + require.NoError(t, err) + + limit, offset := prev.GetLimitOffset() + assert.Equal(t, int64(10), limit) + assert.Equal(t, int64(10), offset) + }) + + t.Run("Simple#02", func(t *testing.T) { + p := Pagination{Type: LimitOffsetPagination, Offset: 10, Size: 10} + prev, err := p.Previous() + require.NoError(t, err) + + limit, offset := prev.GetLimitOffset() + assert.Equal(t, int64(10), limit) + assert.Equal(t, int64(0), offset) + }) + + t.Run("CurrentFirst", func(t *testing.T) { + p := Pagination{Type: LimitOffsetPagination, Offset: 0, Size: 10} + prev, err := p.Previous() + require.NoError(t, err) + + assert.Equal(t, &p, prev) + }) + + t.Run("Partial", func(t *testing.T) { + p := Pagination{Type: LimitOffsetPagination, Offset: 5, Size: 10} + prev, err := p.Previous() + require.NoError(t, err) + + limit, offset := prev.GetLimitOffset() + assert.Equal(t, int64(5), limit) + assert.Equal(t, int64(0), offset) + }) + }) + + t.Run("Invalid", func(t *testing.T) { + p := &Pagination{Type: PageNumberPagination, Size: -10, Offset: 1} + _, err := p.Previous() + assert.Error(t, err) + }) + + t.Run("PageNumberSize", func(t *testing.T) { + t.Run("Simple", func(t *testing.T) { + p := &Pagination{Type: PageNumberPagination, Size: 10, Offset: 4} + prev, err := p.Previous() + require.NoError(t, err) + + number, size := prev.GetNumberSize() + assert.Equal(t, int64(3), number) + assert.Equal(t, int64(10), size) + }) + + t.Run("First", func(t *testing.T) { + p := &Pagination{Type: PageNumberPagination, Size: 10, Offset: 1} + prev, err := p.Previous() + require.NoError(t, err) + + assert.Equal(t, p, prev) + }) + }) +} + +// TestPaginationLast tests the pagination 'Last' function. +func TestPaginationLast(t *testing.T) { + t.Run("LimitOffset", func(t *testing.T) { + t.Run("Simple", func(t *testing.T) { + p := &Pagination{Type: LimitOffsetPagination, Size: 10, Offset: 50} + last, err := p.Last(100) + require.NoError(t, err) + + if assert.NotEqual(t, p, last) { + limit, offset := last.GetLimitOffset() + assert.Equal(t, int64(90), offset) + assert.Equal(t, int64(10), limit) + } + }) + + t.Run("CurrentLast", func(t *testing.T) { + p := &Pagination{Type: LimitOffsetPagination, Size: 10, Offset: 90} + last, err := p.Last(100) + require.NoError(t, err) + + assert.Equal(t, p, last) + }) + + t.Run("Partial#01", func(t *testing.T) { + p := &Pagination{Type: LimitOffsetPagination, Size: 10, Offset: 55} + last, err := p.Last(100) + require.NoError(t, err) + + if assert.NotEqual(t, p, last) { + limit, offset := last.GetLimitOffset() + assert.Equal(t, int64(95), offset) + // page size doesn't change even though there is no more values + assert.Equal(t, int64(10), limit) + } + }) + t.Run("Partial#02", func(t *testing.T) { + p := &Pagination{Type: LimitOffsetPagination, Size: 10, Offset: 57} + last, err := p.Last(100) + require.NoError(t, err) + + if assert.NotEqual(t, p, last) { + limit, offset := last.GetLimitOffset() + assert.Equal(t, int64(97), offset) + // page size doesn't change even though there is no more values + assert.Equal(t, int64(10), limit) + } + }) + + t.Run("TotalZero", func(t *testing.T) { + p := &Pagination{Type: LimitOffsetPagination, Size: 10, Offset: 10} + last, err := p.Last(0) + require.NoError(t, err) + + limit, offset := last.GetLimitOffset() + assert.Equal(t, int64(10), limit) + assert.Equal(t, int64(0), offset) + }) + }) + + t.Run("Invalid", func(t *testing.T) { + p := &Pagination{Type: LimitOffsetPagination, Size: -10, Offset: 0} + _, err := p.Last(10) + assert.Error(t, err) + }) + + t.Run("InvalidTotal", func(t *testing.T) { + p := &Pagination{Type: LimitOffsetPagination, Size: 10, Offset: 0} + _, err := p.Last(-1) + assert.Error(t, err) + }) + + t.Run("PageNumberSize", func(t *testing.T) { + t.Run("Simple", func(t *testing.T) { + p := &Pagination{Type: PageNumberPagination, Size: 10, Offset: 4} + last, err := p.Last(100) + require.NoError(t, err) + + if assert.NotEqual(t, p, last) { + number, size := last.GetNumberSize() + assert.Equal(t, int64(10), number) + assert.Equal(t, int64(10), size) + } + }) + + t.Run("CurrentLast", func(t *testing.T) { + // PageSize: 10, PageNumber: 10 (instances 91-100) + p := &Pagination{Type: PageNumberPagination, Size: 10, Offset: 10} + last, err := p.Last(100) + require.NoError(t, err) + + assert.Equal(t, p, last) + }) + + t.Run("Partial", func(t *testing.T) { + p := &Pagination{Type: PageNumberPagination, Size: 10, Offset: 4} + last, err := p.Last(95) + require.NoError(t, err) + + if assert.NotEqual(t, p, last) { + number, size := last.GetNumberSize() + assert.Equal(t, int64(10), number) + assert.Equal(t, int64(10), size) + } + }) + + t.Run("First", func(t *testing.T) { + p := &Pagination{Type: PageNumberPagination, Size: 10, Offset: 5} + last, err := p.Last(5) + require.NoError(t, err) + + if assert.NotEqual(t, p, last) { + number, size := last.GetNumberSize() + assert.Equal(t, int64(1), number) + assert.Equal(t, int64(10), size) + } + }) + }) +} + +// TestPaginationFirst tests the pagination 'First' function. +func TestPaginationFirst(t *testing.T) { + t.Run("LimitOffset", func(t *testing.T) { + t.Run("Simple", func(t *testing.T) { + p := &Pagination{Type: LimitOffsetPagination, Size: 10, Offset: 200} + first, err := p.First() + require.NoError(t, err) + + limit, offset := first.GetLimitOffset() + assert.Equal(t, int64(10), limit) + assert.Equal(t, int64(0), offset) + }) + + t.Run("Current", func(t *testing.T) { + p := &Pagination{Type: LimitOffsetPagination, Size: 10, Offset: 0} + first, err := p.First() + require.NoError(t, err) + + assert.Equal(t, p, first) + }) + }) + + t.Run("Invalid", func(t *testing.T) { + p := &Pagination{Type: LimitOffsetPagination, Size: 10, Offset: -1} + _, err := p.First() + assert.Error(t, err) + }) + + t.Run("PageNumberSize", func(t *testing.T) { + t.Run("Simple", func(t *testing.T) { + p := &Pagination{Type: PageNumberPagination, Size: 10, Offset: 10} + first, err := p.First() + require.NoError(t, err) + + number, size := first.GetNumberSize() + assert.Equal(t, int64(10), size) + assert.Equal(t, int64(1), number) + }) + + t.Run("Current", func(t *testing.T) { + p := &Pagination{Type: PageNumberPagination, Size: 10, Offset: 1} + first, err := p.First() + require.NoError(t, err) + + assert.Equal(t, p, first) + }) + + t.Run("Invalid", func(t *testing.T) { + p := &Pagination{Type: PageNumberPagination, Size: 10, Offset: 0} + _, err := p.First() + assert.Error(t, err) + }) + }) +} + +// TestPaginationNew tests the pagination creator functions. +func TestPaginationNew(t *testing.T) { + t.Run("LimitOffset", func(t *testing.T) { + // test the NewPaginationLimitOffset function. + p, err := NewPaginationLimitOffset(100, 10) + require.NoError(t, err) + + limit, offset := p.GetLimitOffset() + assert.Equal(t, int64(100), limit) + assert.Equal(t, int64(10), offset) + + t.Run("Invalid", func(t *testing.T) { + _, err := NewPaginationLimitOffset(100, -10) + assert.Error(t, err) + }) + }) + + // test the NewPaginationNumberSize function. + t.Run("PageNumberSize", func(t *testing.T) { + p, err := NewPaginationNumberSize(10, 100) + require.NoError(t, err) + + number, size := p.GetNumberSize() + assert.Equal(t, int64(10), number) + assert.Equal(t, int64(100), size) + + t.Run("Invalid", func(t *testing.T) { + // Page number can't be 0. + _, err := NewPaginationNumberSize(0, 10) + assert.Error(t, err) + }) + }) +} + +// TestPaginationString tests the String function of the pagination. +func TestPaginationString(t *testing.T) { + p, err := NewPaginationNumberSize(10, 10) + require.NoError(t, err) + + s := p.String() + assert.Contains(t, s, "PageNumber: 10") + assert.Contains(t, s, "PageSize: 10") + + p, err = NewPaginationLimitOffset(10, 10) + require.NoError(t, err) + + s = p.String() + + assert.Contains(t, s, "Limit: 10") + assert.Contains(t, s, "Offset: 10") + +} diff --git a/query/scope-sorts_test.go b/query/scope-sorts_test.go new file mode 100644 index 0000000..b4a6a4c --- /dev/null +++ b/query/scope-sorts_test.go @@ -0,0 +1,50 @@ +package query + +import ( + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "testing" +) + +// TestScopeSorts tests the scope sort functions. +func TestScopeSorts(t *testing.T) { + type SortForeignModel struct { + ID int + Length int + } + + type SortModel struct { + ID int + Name string + Age int + Relation *SortForeignModel + RelationID int + } + + c := newController(t) + + err := c.RegisterModels(SortModel{}, SortForeignModel{}) + require.NoError(t, err) + + t.Run("EmptyFields", func(t *testing.T) { + s, err := NewC(c, &SortModel{}) + require.NoError(t, err) + + err = s.SortField("-id") + require.NoError(t, err) + + err = s.Sort("-age") + assert.NoError(t, err) + }) + + t.Run("Duplicated", func(t *testing.T) { + s, err := NewC(c, &SortModel{}) + require.NoError(t, err) + + err = s.Sort("age") + require.NoError(t, err) + + err = s.Sort("-id", "id", "-id") + assert.Error(t, err) + }) +} diff --git a/query/scope.go b/query/scope.go index 2dc9e97..ee46008 100644 --- a/query/scope.go +++ b/query/scope.go @@ -584,7 +584,7 @@ func (s *Scope) copy(isRoot bool, root *Scope) *Scope { scope := &Scope{ id: uuid.New(), mStruct: s.mStruct, - store: make(map[interface{}]interface{}), + store: map[interface{}]interface{}{internal.ControllerStoreKey: s.Controller()}, isMany: s.isMany, kind: s.kind, From 17b8b9f361d602dd854e020385e9f9e8afab56b6 Mon Sep 17 00:00:00 2001 From: Jacek Kucharczyk Date: Fri, 20 Sep 2019 17:51:58 +0200 Subject: [PATCH 2/2] Minor godoc fixes. Import form fixes. --- query/pagination.go | 11 ++++++----- query/scope-pagination.go | 5 ++++- query/scope-sorts.go | 3 ++- query/scope-sorts_test.go | 3 ++- query/scope.go | 6 ++++-- 5 files changed, 18 insertions(+), 10 deletions(-) diff --git a/query/pagination.go b/query/pagination.go index 70af69f..d7149bb 100644 --- a/query/pagination.go +++ b/query/pagination.go @@ -11,7 +11,7 @@ import ( "github.com/neuronlabs/neuron-core/log" ) -// Pagination constants +// Pagination defined constants used for formatting the query. const ( // ParamPage is a JSON API query parameter used as for pagination. ParamPage = "page" @@ -39,7 +39,7 @@ const ( PageNumberPagination ) -// NewPaginationLimitOffset creates new Limit Offset pagination +// NewPaginationLimitOffset creates new Limit Offset pagination for given 'limit' and 'offset'. func NewPaginationLimitOffset(limit, offset int64) (*Pagination, error) { pagination := &Pagination{Type: LimitOffsetPagination, Size: limit, Offset: offset} if err := pagination.IsValid(); err != nil { @@ -48,7 +48,7 @@ func NewPaginationLimitOffset(limit, offset int64) (*Pagination, error) { return pagination, nil } -// NewPaginationNumberSize sets the pagination of the type TpPage with the page 'number' and page 'size'. +// NewPaginationNumberSize sets the pagination of the type PageNumberSize with the page 'number' and page 'size'. func NewPaginationNumberSize(number, size int64) (*Pagination, error) { pagination := &Pagination{Size: size, Offset: number, Type: PageNumberPagination} if err := pagination.IsValid(); err != nil { @@ -97,7 +97,7 @@ func (p *Pagination) First() (*Pagination, error) { return first, nil } -// FormatQuery formats the pagination for the url query. +// FormatQuery formats the pagination for the url query with respect to JSONAPI specification. func (p *Pagination) FormatQuery(q ...url.Values) url.Values { var query url.Values if len(q) != 0 { @@ -196,7 +196,7 @@ func (p *Pagination) IsValid() error { return p.checkValues() } -// IsZero checks if the pagination is already set. +// IsZero checks if the pagination is zero valued. func (p *Pagination) IsZero() bool { return p.Size == 0 && p.Offset == 0 } @@ -245,6 +245,7 @@ func (p *Pagination) Last(total int64) (*Pagination, error) { // computedPageNumber = 52/10 = 5 pageNumber := total / p.Size if total%p.Size != 0 || pageNumber == 0 { + // total % p.Size = 52 % 10 = 2 pageNumber++ } last = &Pagination{Size: p.Size, Offset: pageNumber, Type: PageNumberPagination} diff --git a/query/scope-pagination.go b/query/scope-pagination.go index 25787e3..6cf54fa 100644 --- a/query/scope-pagination.go +++ b/query/scope-pagination.go @@ -9,6 +9,7 @@ import ( // Limit sets the maximum number of objects returned by the List process, // Offset sets the query result's offset. It says to skip as many object's from the repository // before beginning to return the result. 'Offset' 0 is the same as omitting the 'Offset' clause. +// Returns error if the scope 's' already has a pagination or the newly created pagination is not valid. func (s *Scope) Limit(limit, offset int64) error { if s.Pagination != nil { return errors.NewDet(class.QueryPaginationAlreadySet, "pagination already set") @@ -21,7 +22,9 @@ func (s *Scope) Limit(limit, offset int64) error { return nil } -// Page sets the pagination of the type TpPage with the page 'number' and page 'size'. +// Page sets the pagination of the type PageNumberSizePagination with the page 'number' and page 'size'. +// Returns error if the pagination is already set for the given scope 's' or the newly created +// pagination is not valid. func (s *Scope) Page(number, size int64) error { if s.Pagination != nil { return errors.NewDet(class.QueryPaginationAlreadySet, "pagination already set") diff --git a/query/scope-sorts.go b/query/scope-sorts.go index cfb3d17..b41e6f3 100644 --- a/query/scope-sorts.go +++ b/query/scope-sorts.go @@ -8,7 +8,8 @@ import ( ) // Sort adds the sort fields into given scope. -// If the scope already have sorted fields or the fields are duplicated returns error. +// If the scope already have sorted fields the function appends newly created sort fields. +// If the fields are duplicated returns error. func (s *Scope) Sort(fields ...string) (err error) { if log.Level().IsAllowed(log.LDEBUG3) { log.Debug3f("[SCOPE][%s] Sorting by fields: %v ", s.ID(), fields) diff --git a/query/scope-sorts_test.go b/query/scope-sorts_test.go index b4a6a4c..9798820 100644 --- a/query/scope-sorts_test.go +++ b/query/scope-sorts_test.go @@ -1,9 +1,10 @@ package query import ( + "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "testing" ) // TestScopeSorts tests the scope sort functions. diff --git a/query/scope.go b/query/scope.go index ee46008..21c6293 100644 --- a/query/scope.go +++ b/query/scope.go @@ -24,8 +24,10 @@ import ( "github.com/neuronlabs/neuron-core/internal/safemap" ) -// Scope is the query's structure that contains all information required to -// get, create, patch or delete the data in the repository. +// Scope is the query's structure that contains information required +// for the processor to operate. +// The scope has its unique 'ID', contains predefined model, operational value, fieldset, filters, sorts and pagination. +// It also contains the mapping of the included scopes. type Scope struct { id uuid.UUID // Struct is a modelStruct this scope is based on