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 1 commit
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
Prev Previous commit
Next Next commit
Move type Feeds to datastore, remove attribute Feed.Id and apply sort… 
… by Feed.Created.
  • Loading branch information
@thiago
thiago committed Sep 14, 2016
commit 56ef552d1c9d77a58b04de523c0e8cb3a867c156
15 changes: 0 additions & 15 deletions model/feed.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ type Feed struct {
Name string `json:"name" meddler:"repo_name"`
FullName string `json:"full_name" meddler:"repo_full_name"`

Id int `json:"id,omitempty" meddler:"build_id,zeroisnull"`
Number int `json:"number,omitempty" meddler:"build_number,zeroisnull"`
Event string `json:"event,omitempty" meddler:"build_event,zeroisnull"`
Status string `json:"status,omitempty" meddler:"build_status,zeroisnull"`
Expand All @@ -26,17 +25,3 @@ type Feed struct {
Avatar string `json:"author_avatar,omitempty" meddler:"build_avatar,zeroisnull"`
Email string `json:"author_email,omitempty" meddler:"build_email,zeroisnull"`
}

type Feeds []*Feed

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

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

func (slice Feeds) Swap(i, j int) {
slice[i], slice[j] = slice[j], slice[i]
}
26 changes: 20 additions & 6 deletions store/datastore/users.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@ package datastore

import (
"fmt"
"sort"

"github.com/drone/drone/model"
"github.com/russross/meddler"
"sort"
)
Copy link

Choose a reason for hiding this comment

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

imports should be grouped and sorted based on Go style guide. See https://github.com/golang/go/wiki/CodeReviewComments#imports

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I'm newbie in go.

Copy link

Choose a reason for hiding this comment

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

no worries :)

appreciate you taking the time to fix the issue and learn Go along the way!


func (db *datastore) GetUser(id int64) (*model.User, error) {
Expand All @@ -30,7 +30,7 @@ func (db *datastore) GetUserFeed(listof []*model.RepoLite) ([]*model.Feed, error
var (
args []interface{}
err error
feed = model.Feeds{}
feed = Feeds{}
)
_stmt, _args := toList(listof)

Expand All @@ -45,11 +45,12 @@ func (db *datastore) GetUserFeed(listof []*model.RepoLite) ([]*model.Feed, error
feed = append(feed, _feed...)
}
}
sort.Sort(sort.Reverse(feed))
limit := 50
if len(feed) < limit{
limit = len(feed)
// avoid sorting for less than 50
if len(feed) <= limit{
return feed, err
}
sort.Sort(sort.Reverse(feed))
return feed[:limit], err
}

Expand Down Expand Up @@ -95,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 Expand Up @@ -125,7 +140,6 @@ SELECT
repo_owner
,repo_name
,repo_full_name
,build_id
,build_number
,build_event
,build_status
Expand Down
4 changes: 2 additions & 2 deletions store/datastore/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
package datastore

import (
"math"
"strconv"
"strings"

"github.com/drone/drone/model"
"github.com/russross/meddler"
"math"
)
Copy link

Choose a reason for hiding this comment

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

import grouping / ordering


// rebind is a helper function that changes the sql
Expand Down Expand Up @@ -40,8 +40,8 @@ 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{}) {
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 limit = 999
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.


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

Expand Down