Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

adding paging to get repositories from the database #1793

Closed
wants to merge 6 commits into from
Closed

adding paging to get repositories from the database #1793

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8ed8515
adding paging to get repositories from the database
thiago Sep 13, 2016
56ef552
Move type Feeds to datastore, remove attribute Feed.Id and apply sort…
thiago Sep 14, 2016
239c364
adding paging to get repositories from the database
thiago Sep 15, 2016
670f37e
removing underscore prefix
thiago Sep 15, 2016
64db811
fix merge
Sep 20, 2016
46ceeb6
git push origin remove_limit_reposMerge branch 'bugfix/paginated_repo…
Sep 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions store/datastore/repos.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,20 @@ func (db *datastore) GetRepoListOf(listof []*model.RepoLite) ([]*model.Repo, err
var (
repos []*model.Repo
args []interface{}
stmt string
err error
)
switch meddler.Default {
case meddler.PostgreSQL:
stmt, args = toListPosgres(listof)
default:
stmt, args = toList(listof)
}
if len(args) > 0 {
err = meddler.QueryAll(db, &repos, fmt.Sprintf(repoListOfQuery, stmt), args...)
_stmt, _args := toList(listof)

for i, stmt := range _stmt{
args = _args[i]
if len(args) > 0 {
var _repos []*model.Repo
err = meddler.QueryAll(db, &_repos, fmt.Sprintf(repoListOfQuery, stmt), args...)
if err != nil{
break
}
repos = append(repos, _repos...)
}
}
return repos, err
}
Expand Down
47 changes: 47 additions & 0 deletions store/datastore/repos_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

"github.com/drone/drone/model"
"github.com/franela/goblin"
"fmt"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import grouping and sorting :)

)

func TestRepos(t *testing.T) {
Expand Down Expand Up @@ -138,6 +139,52 @@ func TestRepos(t *testing.T) {
g.Assert(count).Equal(2)
})

g.It("Should Get a Repo List Paginated", func() {
_repos := []*model.RepoLite{
{FullName: "bradrydzewski/drone"},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the newer versions of the Go linter will complain when underscores are used in variable names, so we've tried to start removing these from our code. So perhaps repoLiteList instead?

{FullName: "drone/drone"},
}
_repo := []*model.Repo{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps repoList instead of _repo since it is an array. When I see _repo I think it is a single repo

{
UserID: 1,
Owner: "bradrydzewski",
Name: "drone",
FullName: "bradrydzewski/drone",
},
{
UserID: 2,
Owner: "drone",
Name: "drone",
FullName: "drone/drone",
},
}
s.CreateRepo(_repo[0])
s.CreateRepo(_repo[1])

for i := 0; i < 3000; i++{
// find by 3000 repos
_repos = append(_repos, &model.RepoLite{FullName: "octocat/hello-world"+ fmt.Sprint(i)})
// but create only 2000 repos
if i >= 1999 && i < 2999{
continue
}
repo := &model.Repo{
UserID: 2,
Owner: "octocat",
Name: "hello-world" + fmt.Sprint(i),
FullName: "octocat/hello-world" + fmt.Sprint(i),
}
_repo = append(_repo, repo)
s.CreateRepo(repo)
}
repos, err := s.GetRepoListOf(_repos)
g.Assert(err == nil).IsTrue()
g.Assert(len(repos)).Equal(2002)
g.Assert(repos[0].ID).Equal(_repo[0].ID)
g.Assert(repos[1].ID).Equal(_repo[1].ID)
g.Assert(repos[2001].ID).Equal(_repo[2001].ID)
})

g.It("Should Delete a Repo", func() {
repo := model.Repo{
UserID: 1,
Expand Down
66 changes: 46 additions & 20 deletions store/datastore/users.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package datastore

import (
"fmt"
"sort"

"github.com/drone/drone/model"
"github.com/russross/meddler"
Expand All @@ -28,39 +29,50 @@ func (db *datastore) GetUserList() ([]*model.User, error) {
func (db *datastore) GetUserFeed(listof []*model.RepoLite) ([]*model.Feed, error) {
var (
args []interface{}
stmt string
err error

feed = []*model.Feed{}
feed = Feeds{}
)
switch meddler.Default {
case meddler.PostgreSQL:
stmt, args = toListPosgres(listof)
default:
stmt, args = toList(listof)
_stmt, _args := toList(listof)

for i, stmt := range _stmt{
args = _args[i]
if len(args) > 0 {
var _feed []*model.Feed
err = meddler.QueryAll(db, &_feed, fmt.Sprintf(userFeedQuery, stmt), args...)
if err != nil{
break
}
feed = append(feed, _feed...)
}
}
if len(args) > 0 {
err = meddler.QueryAll(db, &feed, fmt.Sprintf(userFeedQuery, stmt), args...)
limit := 50
// avoid sorting for less than 50
if len(feed) <= limit{
return feed, err
}
return feed, err
sort.Sort(sort.Reverse(feed))
return feed[:limit], err
}

func (db *datastore) GetUserFeedLatest(listof []*model.RepoLite) ([]*model.Feed, error) {
var (
args []interface{}
stmt string
err error

feed = []*model.Feed{}
)
switch meddler.Default {
case meddler.PostgreSQL:
stmt, args = toListPosgres(listof)
default:
stmt, args = toList(listof)
}
if len(args) > 0 {
err = meddler.QueryAll(db, &feed, fmt.Sprintf(userFeedLatest, stmt), args...)
_stmt, _args := toList(listof)

for i, stmt := range(_stmt){
args = _args[i]
if len(args) > 0 {
var _feed []*model.Feed
err = meddler.QueryAll(db, &_feed, fmt.Sprintf(userFeedLatest, stmt), args...)
if err != nil{
break
}
feed = append(feed, _feed...)
}
}
return feed, err
}
Expand All @@ -84,6 +96,20 @@ func (db *datastore) DeleteUser(user *model.User) error {
return err
}

type Feeds []*model.Feed
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can probably be a private type since it won't be exported


func (slice Feeds) Len() int {
return len(slice)
}

func (slice Feeds) Less(i, j int) bool {
return slice[i].Created < slice[j].Created;
}

func (slice Feeds) Swap(i, j int) {
slice[i], slice[j] = slice[j], slice[i]
}

const userTable = "users"

const userLoginQuery = `
Expand Down
60 changes: 30 additions & 30 deletions store/datastore/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package datastore

import (
"math"
"strconv"
"strings"

Expand All @@ -17,7 +18,7 @@ func rebind(query string) string {

qb := []byte(query)
// Add space enough for 5 params before we have to allocate
rqb := make([]byte, 0, len(qb)+5)
rqb := make([]byte, 0, len(qb) + 5)
j := 1
for _, b := range qb {
switch b {
Expand All @@ -38,34 +39,33 @@ func rebind(query string) string {

// helper function that converts a simple repsitory list
// to a sql IN statment.
func toList(listof []*model.RepoLite) (string, []interface{}) {
var size = len(listof)
if size > 999 {
size = 999
listof = listof[:999]
}
var qs = make([]string, size, size)
var in = make([]interface{}, size, size)
for i, repo := range listof {
qs[i] = "?"
in[i] = repo.FullName
}
return strings.Join(qs, ","), in
}
func toList(listof []*model.RepoLite) ([]string, [][]interface{}) {
const limit = 999
var total = len(listof)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we avoid returning a [][]interface{} and instead do something like:

const limit = 999

total := len(listof)
if total == 0 {
    return feed
}

pages := total / limit
if total % limit != 0 {
  pages++
}

for i:=0;i<pages;i++ {
    stmt, args := toList(listof, i*limit, limit)

    var tmpFeed []*model.Feed
    err = meddler.QueryAll(db, &tmpFeed, 
      fmt.Sprintf(userFeedQuery, stmt), args...)
    if err != nil {
      return nil, err
    }

    feed = append(feed, tmpFeed...)
}

if len(feed) <= 50 {
   return feed, nil
}

// sort

return feed[:50], nil

where start and limit indicate where to slice

func toList(listof []*model.RepoLite, start, limit int) ([]string, []interface{}) {
   if len(listof) != limit {
     listof = listof[start,limit]
   }

it could make sense to slice the listof before invoking toList as well ... I don't have big preference either way

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was my first solution... It did not seem attractive for me because the limit variable and the core logic for pagination was replicated on the repos and users functions.

Copy link

@bradrydzewski bradrydzewski Sep 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It did not seem attractive for me because the limit variable

this limit could be a global constant, for example:

const maxPlaceholder = 999

core logic for pagination was replicated

I think this is OK

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

core logic for pagination was replicated

note that it could always get split out into a separate helper function:

func calculatePagination(total, limit int) (pages int) {
  pages = total / limit
  if total % limit != 0 {
    pages++
  }
  return
}

var pagesCount = int(math.Ceil(float64(total) / float64(limit)))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we avoid the float64 allocation and conversion here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, because 10 / 3 = 3 but 10.0 / 3.0 = 3.3333... with ceiling to 4 pages.


// helper function that converts a simple repository list
// to a sql IN statement compatible with postgres.
func toListPosgres(listof []*model.RepoLite) (string, []interface{}) {
var size = len(listof)
if size > 999 {
size = 999
listof = listof[:999]
}
var qs = make([]string, size, size)
var in = make([]interface{}, size, size)
for i, repo := range listof {
qs[i] = "$" + strconv.Itoa(i+1)
in[i] = repo.FullName
var qs = make([]string, pagesCount, pagesCount)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we have [][]string to correspond to [][]interface{}? [0][]string may have a different length than [1][]interface{} right? The first page, index [0] could have 999 items while the second page, index [1] could have != 999 items and therefore needs a different placeholder lenght

var in = make([][]interface{}, pagesCount, pagesCount)

for page := 0; page < int(pagesCount); page++ {
var start = page * limit
var end = (page * limit) + limit
if end > total{
end = total
}
var size = end - start
var _qs = make([]string, size, size)
var _in = make([]interface{}, size, size)
for i, repo := range listof[start:end] {
if meddler.Default == meddler.PostgreSQL {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems a bit inefficient since it could add thousands of extra evaluations depending on the size of the list. This was the benefit of having this split into two functions before. The prior implementation was more efficient at the expense of being dry (which was on purpose)

_qs[i] = "$" + strconv.Itoa(i + 1)
}else{
_qs[i] = "?"
}
_in[i] = repo.FullName
}
qs = append(qs, strings.Join(_qs, ","))
in = append(in, _in)
}
return strings.Join(qs, ","), in
}
return qs, in
}