Skip to content

Commit

Permalink
improve code readability
Browse files Browse the repository at this point in the history
  • Loading branch information
SimoneDutto committed Sep 4, 2024
1 parent 0c68eb4 commit 15ca509
Show file tree
Hide file tree
Showing 5 changed files with 43 additions and 36 deletions.
29 changes: 14 additions & 15 deletions internal/common/pagination/pagination.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,38 +44,37 @@ func (l LimitOffsetPagination) Offset() int {
}

// CreatePagination returns the current page, the next page if exists, and the pagination.LimitOffsetPagination.
func CreatePagination(sizeP, pageP *int, total int) (int, *int, LimitOffsetPagination) {
func CreatePagination(sizeP, pageP *int, total int) (currentPage int, nextPage *int, _ LimitOffsetPagination) {
pageSize := -1
offset := 0
page := 0
var nextPage *int

if sizeP != nil && pageP != nil {
pageSize = *sizeP
page = *pageP
offset = pageSize * page
currentPage = *pageP
offset = pageSize * currentPage
}
if (page+1)*pageSize < total {
nPage := page + 1
if (currentPage+1)*pageSize < total {
nPage := currentPage + 1
nextPage = &nPage
}
return page, nextPage, NewOffsetFilter(pageSize, offset)
return currentPage, nextPage, NewOffsetFilter(pageSize, offset)
}

// CreatePagination returns the current page, the page size, and the pagination.LimitOffsetPagination.
// CreatePagination returns the current page, the expected page size, and the pagination.LimitOffsetPagination.
// This method is different approach to the method `CreatePagination` when we don't have the total number of records.
func CreatePaginationWithoutTotal(sizeP, pageP *int) (int, int, LimitOffsetPagination) {
// We return the expectedPageSize, which is pageSize +1, so we fetch one record more from the db.
// We then check the resulting records are enough to advice the consumers to ask for one more page or not.
func CreatePaginationWithoutTotal(sizeP, pageP *int) (currentPage int, expectedPageSize int, _ LimitOffsetPagination) {
pageSize := -1
offset := 0
page := 0

if sizeP != nil && pageP != nil {
pageSize = *sizeP
page = *pageP
offset = pageSize * page
currentPage = *pageP
offset = pageSize * currentPage
}

return page, pageSize + 1, NewOffsetFilter(pageSize+1, offset)
expectedPageSize = pageSize + 1
return currentPage, expectedPageSize, NewOffsetFilter(pageSize+1, offset)
}

type OpenFGAPagination struct {
Expand Down
14 changes: 7 additions & 7 deletions internal/db/resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@ import (
"github.com/canonical/jimm/v3/internal/dbmodel"
)

func (s *dbSuite) SetupDB(c *qt.C) (dbmodel.Model, dbmodel.Controller, dbmodel.Cloud) {
func SetupDB(c *qt.C, database *db.Database) (dbmodel.Model, dbmodel.Controller, dbmodel.Cloud) {
u, err := dbmodel.NewIdentity("[email protected]")
c.Assert(err, qt.IsNil)
c.Assert(s.Database.DB.Create(&u).Error, qt.IsNil)
c.Assert(database.DB.Create(&u).Error, qt.IsNil)

cloud := dbmodel.Cloud{
Name: "test-cloud",
Expand All @@ -24,23 +24,23 @@ func (s *dbSuite) SetupDB(c *qt.C) (dbmodel.Model, dbmodel.Controller, dbmodel.C
Name: "test-region",
}},
}
c.Assert(s.Database.DB.Create(&cloud).Error, qt.IsNil)
c.Assert(database.DB.Create(&cloud).Error, qt.IsNil)

cred := dbmodel.CloudCredential{
Name: "test-cred",
Cloud: cloud,
Owner: *u,
AuthType: "empty",
}
c.Assert(s.Database.DB.Create(&cred).Error, qt.IsNil)
c.Assert(database.DB.Create(&cred).Error, qt.IsNil)

controller := dbmodel.Controller{
Name: "test-controller",
UUID: "00000000-0000-0000-0000-0000-0000000000001",
CloudName: "test-cloud",
CloudRegion: "test-region",
}
err = s.Database.AddController(context.Background(), &controller)
err = database.AddController(context.Background(), &controller)
c.Assert(err, qt.Equals, nil)

model := dbmodel.Model{
Expand All @@ -64,7 +64,7 @@ func (s *dbSuite) SetupDB(c *qt.C) (dbmodel.Model, dbmodel.Controller, dbmodel.C
Level: "unsupported",
},
}
err = s.Database.AddModel(context.Background(), &model)
err = database.AddModel(context.Background(), &model)
c.Assert(err, qt.Equals, nil)
return model, controller, cloud
}
Expand All @@ -77,7 +77,7 @@ func (s *dbSuite) TestGetResources(c *qt.C) {
c.Assert(err, qt.Equals, nil)
c.Assert(res, qt.HasLen, 0)
// create one model, one controller, one cloud
model, controller, cloud := s.SetupDB(c)
model, controller, cloud := SetupDB(c, s.Database)
res, err = s.Database.ListResources(ctx, 10, 0)
c.Assert(err, qt.Equals, nil)
c.Assert(res, qt.HasLen, 3)
Expand Down
30 changes: 19 additions & 11 deletions internal/rebac_admin/resources.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/canonical/rebac-admin-ui-handlers/v1/resources"

"github.com/canonical/jimm/v3/internal/common/pagination"
"github.com/canonical/jimm/v3/internal/db"
"github.com/canonical/jimm/v3/internal/jujuapi"
"github.com/canonical/jimm/v3/internal/rebac_admin/utils"
)
Expand All @@ -28,28 +29,21 @@ func (s *resourcesService) ListResources(ctx context.Context, params *resources.
if err != nil {
return nil, err
}
page, pageSize, pagination := pagination.CreatePaginationWithoutTotal(params.Size, params.Page)
currentPage, expectedPageSize, pagination := pagination.CreatePaginationWithoutTotal(params.Size, params.Page)
res, err := s.jimm.ListResources(ctx, user, pagination)
if err != nil {
return nil, err
}
// We fetch one record more than the page size. Then, we set the next page if we have this many records.
// Otherwise next page is empty.
var nextPage *int
if len(res) == pageSize {
nPage := page + 1
nextPage = &nPage
res = res[:len(res)-1]
}
nextPage, res := getNextPageAndResources(currentPage, expectedPageSize, res)
rRes := make([]resources.Resource, len(res))
for i, u := range res {
rRes[i] = utils.FromDbResourcesToResources(u)
rRes[i] = utils.ToRebacResource(u)
}

return &resources.PaginatedResponse[resources.Resource]{
Data: rRes,
Meta: resources.ResponseMeta{
Page: &page,
Page: &currentPage,
Size: len(rRes),
Total: nil,
},
Expand All @@ -58,3 +52,17 @@ func (s *resourcesService) ListResources(ctx context.Context, params *resources.
},
}, nil
}

// We fetch one record more than the page size.
// Then, we set the next page if we have this many records.
// If it does we return the records minus 1 and advide the consumer there is another page.
// Otherwise we return the records we have and set next page as empty.
func getNextPageAndResources(currentPage, expectedPageSize int, resources []db.Resource) (*int, []db.Resource) {
var nextPage *int
if len(resources) == expectedPageSize {
nPage := currentPage + 1
nextPage = &nPage
resources = resources[:len(resources)-1]
}
return nextPage, resources
}
2 changes: 1 addition & 1 deletion internal/rebac_admin/resources_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ type resourcesSuite struct {

var _ = gc.Suite(&resourcesSuite{})

// patchIdentitiesEntitlementTestEnv is used to create entries in JIMM's database.
// resourcesTestEnv is used to create entries in JIMM's database.
// The rebacAdminSuite does not spin up a Juju controller so we cannot use
// regular JIMM methods to create resources. It is also necessary to have resources
// present in the database in order for ListRelationshipTuples to work correctly.
Expand Down
4 changes: 2 additions & 2 deletions internal/rebac_admin/utils/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ func FromUserToIdentity(user openfga.User) resources.Identity {
}
}

// FromUserToIdentity parses openfga.User into resources.Identity .
func FromDbResourcesToResources(res db.Resource) resources.Resource {
// ToRebacResource parses db.Resource into resources.Resource.
func ToRebacResource(res db.Resource) resources.Resource {
r := resources.Resource{
Entity: resources.Entity{
Id: res.ID.String,
Expand Down

0 comments on commit 15ca509

Please sign in to comment.