Resolves pagination issues (#99)

Author: arekkas

Gopkg.lock

@@ -18,6 +18,7 @@ import (
 	"sync"
 
 	. "github.com/ory/ladon"
+	"github.com/ory/pagination"
 	"github.com/pkg/errors"
 )
 
@@ -46,17 +47,14 @@ func (m *MemoryManager) Update(policy Policy) error {
 func (m *MemoryManager) GetAll(limit, offset int64) (Policies, error) {
 	ps := make(Policies, len(m.Policies))
 	i := 0
+
 	for _, p := range m.Policies {
 		ps[i] = p
 		i++
 	}
 
-	if offset+limit > int64(len(m.Policies)) {
-		limit = int64(len(m.Policies))
-		offset = 0
-	}
-
-	return ps[offset:limit], nil
+	start, end := pagination.Index(int(limit), int(offset), len(ps))
+	return ps[start:end], nil
 }
 
 // Create a new pollicy to MemoryManager.

manager/memory/manager_memory.go

@@ -278,11 +278,25 @@ LEFT JOIN ladon_subject as subject ON rs.subject = subject.id
 LEFT JOIN ladon_action as action ON ra.action = action.id
 LEFT JOIN ladon_resource as resource ON rr.resource = resource.id
 
-`
+WHERE p.id=?`
+
+var getAllQuery = `SELECT
+	p.id, p.effect, p.conditions, p.description,
+	subject.template as subject, resource.template as resource, action.template as action
+FROM
+	(SELECT * from ladon_policy ORDER BY id LIMIT ? OFFSET ?) as p
+
+LEFT JOIN ladon_policy_subject_rel as rs ON rs.policy = p.id
+LEFT JOIN ladon_policy_action_rel as ra ON ra.policy = p.id
+LEFT JOIN ladon_policy_resource_rel as rr ON rr.policy = p.id
+
+LEFT JOIN ladon_subject as subject ON rs.subject = subject.id
+LEFT JOIN ladon_action as action ON ra.action = action.id
+LEFT JOIN ladon_resource as resource ON rr.resource = resource.id`
 
 // GetAll returns all policies
 func (s *SQLManager) GetAll(limit, offset int64) (Policies, error) {
-	query := s.db.Rebind(getQuery + "ORDER BY p.id LIMIT ? OFFSET ?")
+	query := s.db.Rebind(getAllQuery)
 
 	rows, err := s.db.Query(query, limit, offset)
 	if err != nil {
@@ -295,7 +309,7 @@ func (s *SQLManager) GetAll(limit, offset int64) (Policies, error) {
 
 // Get retrieves a policy.
 func (s *SQLManager) Get(id string) (Policy, error) {
-	query := s.db.Rebind(getQuery + "WHERE p.id=?")
+	query := s.db.Rebind(getQuery)
 
 	rows, err := s.db.Query(query, id)
 	if err == sql.ErrNoRows {

manager/sql/manager_sql.go

@@ -25,6 +25,7 @@ import (
 	"github.com/ory/ladon/integration"
 	. "github.com/ory/ladon/manager/memory"
 	. "github.com/ory/ladon/manager/sql"
+	"github.com/stretchr/testify/require"
 )
 
 var managers = map[string]Manager{}
@@ -78,14 +79,56 @@ func connectMySQL(wg *sync.WaitGroup) {
 	}
 }
 
-func TestGetErrors(t *testing.T) {
-	for k, s := range managers {
-		t.Run("manager="+k, TestHelperGetErrors(s))
-	}
-}
+func TestManagers(t *testing.T) {
+	t.Run("type=get errors", func(t *testing.T) {
+		for k, s := range managers {
+			t.Run("manager="+k, TestHelperGetErrors(s))
+		}
+	})
 
-func TestCreateGetDelete(t *testing.T) {
-	for k, s := range managers {
-		t.Run(fmt.Sprintf("manager=%s", k), TestHelperCreateGetDelete(s))
-	}
+	t.Run("type=CRUD", func(t *testing.T) {
+		for k, s := range managers {
+			t.Run(fmt.Sprintf("manager=%s", k), TestHelperCreateGetDelete(s))
+		}
+	})
+
+	t.Run("type=find", func(t *testing.T) {
+		for k, s := range map[string]Manager{
+			"postgres": managers["postgres"],
+			"mysql":    managers["mysql"],
+		} {
+			t.Run(fmt.Sprintf("manager=%s", k), TestHelperFindPoliciesForSubject(k, s))
+		}
+	})
+
+	t.Run("type=migrate 6 to 7", func(t *testing.T) {
+		for k, s := range map[string]ManagerMigrator{
+			"postgres": migrators["postgres"],
+			"mysql":    migrators["mysql"],
+		} {
+			t.Run(fmt.Sprintf("manager=%s", k), func(t *testing.T) {
+
+				// This create part is only necessary to populate the data store with some values. If you
+				// migrate you won't need this
+				for _, c := range TestManagerPolicies {
+					t.Run(fmt.Sprintf("create=%s", k), func(t *testing.T) {
+						require.NoError(t, s.Create(c))
+					})
+				}
+
+				require.NoError(t, s.Migrate())
+
+				for _, c := range TestManagerPolicies {
+					t.Run(fmt.Sprintf("fetch=%s", k), func(t *testing.T) {
+						get, err := s.GetManager().Get(c.GetID())
+						require.NoError(t, err)
+
+						AssertPolicyEqual(t, c, get)
+
+						require.NoError(t, s.GetManager().Delete(c.GetID()))
+					})
+				}
+			})
+		}
+	})
 }

manager_all_test.go

@@ -300,7 +300,6 @@ func TestHelperGetErrors(s Manager) func(t *testing.T) {
 
 func TestHelperCreateGetDelete(s Manager) func(t *testing.T) {
 	return func(t *testing.T) {
-
 		for i, c := range TestManagerPolicies {
 			t.Run(fmt.Sprintf("case=%d/id=%s/type=create", i, c.GetID()), func(t *testing.T) {
 				_, err := s.Get(c.GetID())
@@ -334,10 +333,24 @@ func TestHelperCreateGetDelete(s Manager) func(t *testing.T) {
 		}
 
 		t.Run("type=query-all", func(t *testing.T) {
+			count := int64(len(TestManagerPolicies))
+
 			pols, err := s.GetAll(100, 0)
 			require.NoError(t, err)
 			assert.Len(t, pols, len(TestManagerPolicies))
 
+			pols4, err := s.GetAll(1, 0)
+			require.NoError(t, err)
+			assert.Len(t, pols4, 1)
+
+			pols2, err := s.GetAll(100, count-1)
+			require.NoError(t, err)
+			assert.Len(t, pols2, 1)
+
+			pols3, err := s.GetAll(100, count)
+			require.NoError(t, err)
+			assert.Len(t, pols3, 0)
+
 			found := map[string]int{}
 			for _, got := range pols {
 				for _, expect := range TestManagerPolicies {
@@ -360,6 +373,5 @@ func TestHelperCreateGetDelete(s Manager) func(t *testing.T) {
 				assert.Error(t, err)
 			})
 		}
-
 	}
 }