Skip to content

Commit 139fc7c

Browse files
lunnylunny
authored
Fix milestones too many SQL variables bug (#10880) (#10904)
* Fix milestones too many SQL variables bug * Fix test * Don't display repositories with no milestone and fix tests * Remove unused code and add some comments
1 parent 596eebb commit 139fc7c

5 files changed

Lines changed: 144 additions & 108 deletions

File tree

‎models/issue_milestone.go‎

Lines changed: 45 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -521,10 +521,12 @@ func DeleteMilestoneByRepoID(repoID, id int64) error {
521521
return sess.Commit()
522522
}
523523

524-
// CountMilestonesByRepoIDs map from repoIDs to number of milestones matching the options`
525-
func CountMilestonesByRepoIDs(repoIDs []int64, isClosed bool) (map[int64]int64, error) {
524+
// CountMilestones map from repo conditions to number of milestones matching the options`
525+
func CountMilestones(repoCond builder.Cond, isClosed bool) (map[int64]int64, error) {
526526
sess := x.Where("is_closed = ?", isClosed)
527-
sess.In("repo_id", repoIDs)
527+
if repoCond.IsValid() {
528+
sess.In("repo_id", builder.Select("id").From("repository").Where(repoCond))
529+
}
528530

529531
countsSlice := make([]*struct {
530532
RepoID int64
@@ -544,11 +546,21 @@ func CountMilestonesByRepoIDs(repoIDs []int64, isClosed bool) (map[int64]int64,
544546
return countMap, nil
545547
}
546548

547-
// GetMilestonesByRepoIDs returns a list of milestones of given repositories and status.
548-
func GetMilestonesByRepoIDs(repoIDs []int64, page int, isClosed bool, sortType string) (MilestoneList, error) {
549+
// CountMilestonesByRepoIDs map from repoIDs to number of milestones matching the options`
550+
func CountMilestonesByRepoIDs(repoIDs []int64, isClosed bool) (map[int64]int64, error) {
551+
return CountMilestones(
552+
builder.In("repo_id", repoIDs),
553+
isClosed,
554+
)
555+
}
556+
557+
// SearchMilestones search milestones
558+
func SearchMilestones(repoCond builder.Cond, page int, isClosed bool, sortType string) (MilestoneList, error) {
549559
miles := make([]*Milestone, 0, setting.UI.IssuePagingNum)
550560
sess := x.Where("is_closed = ?", isClosed)
551-
sess.In("repo_id", repoIDs)
561+
if repoCond.IsValid() {
562+
sess.In("repo_id", builder.Select("id").From("repository").Where(repoCond))
563+
}
552564
if page > 0 {
553565
sess = sess.Limit(setting.UI.IssuePagingNum, (page-1)*setting.UI.IssuePagingNum)
554566
}
@@ -570,25 +582,45 @@ func GetMilestonesByRepoIDs(repoIDs []int64, page int, isClosed bool, sortType s
570582
return miles, sess.Find(&miles)
571583
}
572584

585+
// GetMilestonesByRepoIDs returns a list of milestones of given repositories and status.
586+
func GetMilestonesByRepoIDs(repoIDs []int64, page int, isClosed bool, sortType string) (MilestoneList, error) {
587+
return SearchMilestones(
588+
builder.In("repo_id", repoIDs),
589+
page,
590+
isClosed,
591+
sortType,
592+
)
593+
}
594+
573595
// MilestonesStats represents milestone statistic information.
574596
type MilestonesStats struct {
575597
OpenCount, ClosedCount int64
576598
}
577599

600+
// Total returns the total counts of milestones
601+
func (m MilestonesStats) Total() int64 {
602+
return m.OpenCount + m.ClosedCount
603+
}
604+
578605
// GetMilestonesStats returns milestone statistic information for dashboard by given conditions.
579-
func GetMilestonesStats(userRepoIDs []int64) (*MilestonesStats, error) {
606+
func GetMilestonesStats(repoCond builder.Cond) (*MilestonesStats, error) {
580607
var err error
581608
stats := &MilestonesStats{}
582609

583-
stats.OpenCount, err = x.Where("is_closed = ?", false).
584-
And(builder.In("repo_id", userRepoIDs)).
585-
Count(new(Milestone))
610+
sess := x.Where("is_closed = ?", false)
611+
if repoCond.IsValid() {
612+
sess.And(builder.In("repo_id", builder.Select("id").From("repository").Where(repoCond)))
613+
}
614+
stats.OpenCount, err = sess.Count(new(Milestone))
586615
if err != nil {
587616
return nil, err
588617
}
589-
stats.ClosedCount, err = x.Where("is_closed = ?", true).
590-
And(builder.In("repo_id", userRepoIDs)).
591-
Count(new(Milestone))
618+
619+
sess = x.Where("is_closed = ?", true)
620+
if repoCond.IsValid() {
621+
sess.And(builder.In("repo_id", builder.Select("id").From("repository").Where(repoCond)))
622+
}
623+
stats.ClosedCount, err = sess.Count(new(Milestone))
592624
if err != nil {
593625
return nil, err
594626
}

‎models/issue_milestone_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111

1212
api "code.gitea.io/gitea/modules/structs"
1313
"code.gitea.io/gitea/modules/timeutil"
14+
"xorm.io/builder"
1415

1516
"github.com/stretchr/testify/assert"
1617
)
@@ -370,7 +371,7 @@ func TestGetMilestonesStats(t *testing.T) {
370371
repo1 := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
371372
repo2 := AssertExistsAndLoadBean(t, &Repository{ID: 2}).(*Repository)
372373

373-
milestoneStats, err := GetMilestonesStats([]int64{repo1.ID, repo2.ID})
374+
milestoneStats, err := GetMilestonesStats(builder.In("repo_id", []int64{repo1.ID, repo2.ID}))
374375
assert.NoError(t, err)
375376
assert.EqualValues(t, repo1.NumOpenMilestones+repo2.NumOpenMilestones, milestoneStats.OpenCount)
376377
assert.EqualValues(t, repo1.NumClosedMilestones+repo2.NumClosedMilestones, milestoneStats.ClosedCount)

‎models/repo_list.go‎

Lines changed: 34 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,10 @@ type SearchRepoOptions struct {
144144
TopicOnly bool
145145
// include description in keyword search
146146
IncludeDescription bool
147+
// None -> include has milestones AND has no milestone
148+
// True -> include just has milestones
149+
// False -> include just has no milestone
150+
HasMilestones util.OptionalBool
147151
}
148152

149153
//SearchOrderBy is used to sort the result
@@ -171,12 +175,9 @@ const (
171175
SearchOrderByForksReverse SearchOrderBy = "num_forks DESC"
172176
)
173177

174-
// SearchRepository returns repositories based on search options,
178+
// SearchRepositoryCondition returns repositories based on search options,
175179
// it returns results in given range and number of total results.
176-
func SearchRepository(opts *SearchRepoOptions) (RepositoryList, int64, error) {
177-
if opts.Page <= 0 {
178-
opts.Page = 1
179-
}
180+
func SearchRepositoryCondition(opts *SearchRepoOptions) builder.Cond {
180181
var cond = builder.NewCond()
181182

182183
if opts.Private {
@@ -276,6 +277,29 @@ func SearchRepository(opts *SearchRepoOptions) (RepositoryList, int64, error) {
276277
cond = cond.And(builder.Eq{"is_mirror": opts.Mirror == util.OptionalBoolTrue})
277278
}
278279

280+
switch opts.HasMilestones {
281+
case util.OptionalBoolTrue:
282+
cond = cond.And(builder.Gt{"num_milestones": 0})
283+
case util.OptionalBoolFalse:
284+
cond = cond.And(builder.Eq{"num_milestones": 0}.Or(builder.IsNull{"num_milestones"}))
285+
}
286+
287+
return cond
288+
}
289+
290+
// SearchRepository returns repositories based on search options,
291+
// it returns results in given range and number of total results.
292+
func SearchRepository(opts *SearchRepoOptions) (RepositoryList, int64, error) {
293+
cond := SearchRepositoryCondition(opts)
294+
return SearchRepositoryByCondition(opts, cond)
295+
}
296+
297+
// SearchRepositoryByCondition search repositories by condition
298+
func SearchRepositoryByCondition(opts *SearchRepoOptions, cond builder.Cond) (RepositoryList, int64, error) {
299+
if opts.Page <= 0 {
300+
opts.Page = 1
301+
}
302+
279303
if len(opts.OrderBy) == 0 {
280304
opts.OrderBy = SearchOrderByAlphabetically
281305
}
@@ -296,11 +320,11 @@ func SearchRepository(opts *SearchRepoOptions) (RepositoryList, int64, error) {
296320
}
297321

298322
repos := make(RepositoryList, 0, opts.PageSize)
299-
if err = sess.
300-
Where(cond).
301-
OrderBy(opts.OrderBy.String()).
302-
Limit(opts.PageSize, (opts.Page-1)*opts.PageSize).
303-
Find(&repos); err != nil {
323+
sess.Where(cond).OrderBy(opts.OrderBy.String())
324+
if opts.PageSize > 0 {
325+
sess.Limit(opts.PageSize, (opts.Page-1)*opts.PageSize)
326+
}
327+
if err = sess.Find(&repos); err != nil {
304328
return nil, 0, fmt.Errorf("Repo: %v", err)
305329
}
306330

‎routers/user/home.go‎

Lines changed: 61 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ import (
2424

2525
"github.com/keybase/go-crypto/openpgp"
2626
"github.com/keybase/go-crypto/openpgp/armor"
27-
"github.com/unknwon/com"
27+
"xorm.io/builder"
2828
)
2929

3030
const (
@@ -171,135 +171,114 @@ func Milestones(ctx *context.Context) {
171171
return
172172
}
173173

174-
sortType := ctx.Query("sort")
175-
page := ctx.QueryInt("page")
176-
if page <= 1 {
177-
page = 1
178-
}
174+
var (
175+
repoOpts = models.SearchRepoOptions{
176+
OwnerID: ctxUser.ID,
177+
Private: true,
178+
AllPublic: false, // Include also all public repositories of users and public organisations
179+
AllLimited: false, // Include also all public repositories of limited organisations
180+
HasMilestones: util.OptionalBoolTrue, // Just needs display repos has milestones
181+
IsProfile: false,
182+
}
179183

180-
reposQuery := ctx.Query("repos")
181-
isShowClosed := ctx.Query("state") == "closed"
184+
userRepoCond = models.SearchRepositoryCondition(&repoOpts) // all repo condition user could visit
185+
repoCond = userRepoCond
186+
repoIDs []int64
182187

183-
// Get repositories.
184-
var err error
185-
var userRepoIDs []int64
186-
if ctxUser.IsOrganization() {
187-
env, err := ctxUser.AccessibleReposEnv(ctx.User.ID)
188-
if err != nil {
189-
ctx.ServerError("AccessibleReposEnv", err)
190-
return
191-
}
192-
userRepoIDs, err = env.RepoIDs(1, ctxUser.NumRepos)
193-
if err != nil {
194-
ctx.ServerError("env.RepoIDs", err)
195-
return
196-
}
197-
userRepoIDs, err = models.FilterOutRepoIdsWithoutUnitAccess(ctx.User, userRepoIDs, models.UnitTypeIssues, models.UnitTypePullRequests)
198-
if err != nil {
199-
ctx.ServerError("FilterOutRepoIdsWithoutUnitAccess", err)
200-
return
201-
}
202-
} else {
203-
userRepoIDs, err = ctxUser.GetAccessRepoIDs(models.UnitTypeIssues, models.UnitTypePullRequests)
204-
if err != nil {
205-
ctx.ServerError("ctxUser.GetAccessRepoIDs", err)
206-
return
207-
}
208-
}
209-
if len(userRepoIDs) == 0 {
210-
userRepoIDs = []int64{-1}
188+
reposQuery = ctx.Query("repos")
189+
isShowClosed = ctx.Query("state") == "closed"
190+
sortType = ctx.Query("sort")
191+
page = ctx.QueryInt("page")
192+
)
193+
194+
if page <= 1 {
195+
page = 1
211196
}
212197

213-
var repoIDs []int64
214198
if len(reposQuery) != 0 {
215199
if issueReposQueryPattern.MatchString(reposQuery) {
216200
// remove "[" and "]" from string
217201
reposQuery = reposQuery[1 : len(reposQuery)-1]
218202
//for each ID (delimiter ",") add to int to repoIDs
219-
reposSet := false
203+
220204
for _, rID := range strings.Split(reposQuery, ",") {
221205
// Ensure nonempty string entries
222206
if rID != "" && rID != "0" {
223-
reposSet = true
224207
rIDint64, err := strconv.ParseInt(rID, 10, 64)
225208
// If the repo id specified by query is not parseable or not accessible by user, just ignore it.
226-
if err == nil && com.IsSliceContainsInt64(userRepoIDs, rIDint64) {
209+
if err == nil {
227210
repoIDs = append(repoIDs, rIDint64)
228211
}
229212
}
230213
}
231-
if reposSet && len(repoIDs) == 0 {
232-
// force an empty result
233-
repoIDs = []int64{-1}
214+
if len(repoIDs) > 0 {
215+
// Don't just let repoCond = builder.In("id", repoIDs) because user may has no permission on repoIDs
216+
// But the original repoCond has a limitation
217+
repoCond = repoCond.And(builder.In("id", repoIDs))
234218
}
235219
} else {
236220
log.Warn("issueReposQueryPattern not match with query")
237221
}
238222
}
239223

240-
if len(repoIDs) == 0 {
241-
repoIDs = userRepoIDs
242-
}
243-
244-
counts, err := models.CountMilestonesByRepoIDs(userRepoIDs, isShowClosed)
224+
counts, err := models.CountMilestones(userRepoCond, isShowClosed)
245225
if err != nil {
246226
ctx.ServerError("CountMilestonesByRepoIDs", err)
247227
return
248228
}
249229

250-
milestones, err := models.GetMilestonesByRepoIDs(repoIDs, page, isShowClosed, sortType)
230+
milestones, err := models.SearchMilestones(repoCond, page, isShowClosed, sortType)
251231
if err != nil {
252232
ctx.ServerError("GetMilestonesByRepoIDs", err)
253233
return
254234
}
255235

256-
showReposMap := make(map[int64]*models.Repository, len(counts))
257-
for rID := range counts {
258-
if rID == -1 {
259-
break
260-
}
261-
repo, err := models.GetRepositoryByID(rID)
262-
if err != nil {
263-
if models.IsErrRepoNotExist(err) {
264-
ctx.NotFound("GetRepositoryByID", err)
265-
return
266-
} else if err != nil {
267-
ctx.ServerError("GetRepositoryByID", fmt.Errorf("[%d]%v", rID, err))
268-
return
269-
}
270-
}
271-
showReposMap[rID] = repo
272-
}
273-
274-
showRepos := models.RepositoryListOfMap(showReposMap)
275-
sort.Sort(showRepos)
276-
if err = showRepos.LoadAttributes(); err != nil {
277-
ctx.ServerError("LoadAttributes", err)
236+
showRepos, _, err := models.SearchRepositoryByCondition(&repoOpts, userRepoCond)
237+
if err != nil {
238+
ctx.ServerError("SearchRepositoryByCondition", err)
278239
return
279240
}
241+
sort.Sort(showRepos)
242+
243+
for i := 0; i < len(milestones); {
244+
for _, repo := range showRepos {
245+
if milestones[i].RepoID == repo.ID {
246+
milestones[i].Repo = repo
247+
break
248+
}
249+
}
250+
if milestones[i].Repo == nil {
251+
log.Warn("Cannot find milestone %d 's repository %d", milestones[i].ID, milestones[i].RepoID)
252+
milestones = append(milestones[:i], milestones[i+1:]...)
253+
continue
254+
}
280255

281-
for _, m := range milestones {
282-
m.Repo = showReposMap[m.RepoID]
283-
m.RenderedContent = string(markdown.Render([]byte(m.Content), m.Repo.Link(), m.Repo.ComposeMetas()))
284-
if m.Repo.IsTimetrackerEnabled() {
285-
err := m.LoadTotalTrackedTime()
256+
milestones[i].RenderedContent = string(markdown.Render([]byte(milestones[i].Content), milestones[i].Repo.Link(), milestones[i].Repo.ComposeMetas()))
257+
if milestones[i].Repo.IsTimetrackerEnabled() {
258+
err := milestones[i].LoadTotalTrackedTime()
286259
if err != nil {
287260
ctx.ServerError("LoadTotalTrackedTime", err)
288261
return
289262
}
290263
}
264+
i++
291265
}
292266

293-
milestoneStats, err := models.GetMilestonesStats(repoIDs)
267+
milestoneStats, err := models.GetMilestonesStats(repoCond)
294268
if err != nil {
295269
ctx.ServerError("GetMilestoneStats", err)
296270
return
297271
}
298272

299-
totalMilestoneStats, err := models.GetMilestonesStats(userRepoIDs)
300-
if err != nil {
301-
ctx.ServerError("GetMilestoneStats", err)
302-
return
273+
var totalMilestoneStats *models.MilestonesStats
274+
if len(repoIDs) == 0 {
275+
totalMilestoneStats = milestoneStats
276+
} else {
277+
totalMilestoneStats, err = models.GetMilestonesStats(userRepoCond)
278+
if err != nil {
279+
ctx.ServerError("GetMilestoneStats", err)
280+
return
281+
}
303282
}
304283

305284
var pagerCount int
@@ -318,7 +297,7 @@ func Milestones(ctx *context.Context) {
318297
ctx.Data["Counts"] = counts
319298
ctx.Data["MilestoneStats"] = milestoneStats
320299
ctx.Data["SortType"] = sortType
321-
if len(repoIDs) != len(userRepoIDs) {
300+
if milestoneStats.Total() != totalMilestoneStats.Total() {
322301
ctx.Data["RepoIDs"] = repoIDs
323302
}
324303
ctx.Data["IsShowClosed"] = isShowClosed

0 commit comments

Comments
 (0)