[API] generalize list header #16551

zeripath · 2021-07-26T16:50:48Z

6543 commented

2021-07-26 16:50:48 +00:00

(Migrated from github.com)

close #11114

zeripath reviewed 2021-07-26 18:07:57 +00:00

CONTRIBUTING.md

zeripath commented

2021-07-26 18:07:57 +00:00

 * add `X-Total-Count` header ([example](https://github.com/go-gitea/gitea/blob/e76f8cac9a2ba727bec6b5beab2496be9dafabef/routers/api/v1/repo/issue.go#L445))
 * add `X-Total-Count` to the `Access-Control-Expose-Headers` header

```suggestion * add `X-Total-Count` header ([example](https://github.com/go-gitea/gitea/blob/e76f8cac9a2ba727bec6b5beab2496be9dafabef/routers/api/v1/repo/issue.go#L445)) * add `X-Total-Count` to the `Access-Control-Expose-Headers` header ```

6543 (Migrated from github.com) reviewed 2021-07-26 20:02:39 +00:00

models/issue_stopwatch.go

6543 (Migrated from github.com) commented

2021-07-26 20:02:39 +00:00

	return x.Where("user_id = ?", userID).Count(&Stopwatch{})

```suggestion return x.Where("user_id = ?", userID).Count(&Stopwatch{}) ```

zeripath reviewed 2021-07-26 20:24:41 +00:00

models/issue_stopwatch.go

zeripath commented

2021-07-26 20:24:41 +00:00

	exists, err = e.

```suggestion exists, err = e. ```

zeripath reviewed 2021-07-26 20:35:25 +00:00

routers/api/v1/repo/key.go

						
				@ -89,2 +91,3 @@

					count, err := models.CountDeployKeys(opts)

					if err != nil {

						ctx.Error(http.StatusInternalServerError, "ListDeployKeys", err)

						ctx.InternalServerError(err)

zeripath commented

2021-07-26 20:35:25 +00:00

So there is a Xorm function called FindAndCount which doesn't pass the limits which may be better.

6543 (Migrated from github.com) reviewed 2021-07-26 21:16:25 +00:00

routers/api/v1/repo/key.go

						
				@ -89,2 +91,3 @@

					count, err := models.CountDeployKeys(opts)

					if err != nil {

						ctx.Error(http.StatusInternalServerError, "ListDeployKeys", err)

						ctx.InternalServerError(err)

6543 (Migrated from github.com) commented

2021-07-26 21:16:25 +00:00

oh was not aware of it ... will need to refactor a lot then

6543 (Migrated from github.com) reviewed 2021-07-26 22:59:21 +00:00

routers/api/v1/repo/key.go

						
				@ -89,2 +91,3 @@

					count, err := models.CountDeployKeys(opts)

					if err != nil {

						ctx.Error(http.StatusInternalServerError, "ListDeployKeys", err)

						ctx.InternalServerError(err)

6543 (Migrated from github.com) commented

2021-07-26 22:59:21 +00:00

FindAndCount exec two db querys so we should only use it if the function is always used next to a count function

**FindAndCount** exec two db querys so we should only use it if the function is always used next to a count function

zeripath reviewed 2021-07-27 18:08:13 +00:00

routers/api/v1/org/member.go

				`@ -18,15 +18,21 @@ import (`

zeripath commented

2021-07-27 18:08:12 +00:00

Generally prefer:

	for i, member := range members {
		apiMembers[i] = convert.ToUser(member, ctx.User)

Generally prefer: ```suggestion for i, member := range members { apiMembers[i] = convert.ToUser(member, ctx.User) ```

6543 (Migrated from github.com) reviewed 2021-07-27 18:11:07 +00:00

routers/api/v1/repo/status.go

				`@ -267,5 +266,6 @@ func GetCombinedCommitStatusByRef(ctx *context.APIContext) {`

6543 (Migrated from github.com) commented

2021-07-27 18:11:06 +00:00

intended ... follow up for me in next pull

zeripath reviewed 2021-07-27 18:11:23 +00:00

modules/git/repo_tag.go

						
				@ -170,3 +171,4 @@

					tagNames := strings.Split(strings.TrimRight(stdout, "\n"), "\n")

					tagsTotal := len(tagNames)

					if page != 0 {

zeripath commented

2021-07-27 18:11:23 +00:00

Probably need to fix this if we're going to offer to page this.

zeripath reviewed 2021-07-27 18:12:56 +00:00

routers/api/v1/repo/topic.go

						
				@ -164,15 +167,15 @@ func AddTopic(ctx *context.APIContext) {

					}

zeripath commented

2021-07-27 18:12:56 +00:00

This should be CountTopics

6543 (Migrated from github.com) reviewed 2021-07-27 18:13:39 +00:00

modules/git/repo_tag.go

						
				@ -170,3 +171,4 @@

					tagNames := strings.Split(strings.TrimRight(stdout, "\n"), "\n")

					tagsTotal := len(tagNames)

					if page != 0 {

6543 (Migrated from github.com) commented

2021-07-27 18:13:39 +00:00

what do you mean by fix? the git implementation to get tag info to truly paginate?

zeripath reviewed 2021-07-27 18:15:14 +00:00

routers/web/org/home.go

zeripath commented

2021-07-27 18:15:14 +00:00

should use the membersCount provided and drop the CountOrgMembers call below.

6543 (Migrated from github.com) reviewed 2021-07-27 18:16:27 +00:00

routers/api/v1/repo/topic.go

						
				@ -164,15 +167,15 @@ func AddTopic(ctx *context.APIContext) {

					}

6543 (Migrated from github.com) commented

2021-07-27 18:16:27 +00:00

do you mean separate to two functions?

zeripath reviewed 2021-07-27 18:46:47 +00:00

routers/api/v1/org/member.go

						
				@ -30,3 +35,4 @@

						ctx.InternalServerError(err)

						return

					}

zeripath commented

2021-07-27 18:46:46 +00:00

Can't we just get the count here?

	members, count, err := models.FindOrgMembers(opts)

Can't we just get the count here? ```suggestion members, count, err := models.FindOrgMembers(opts) ```

zeripath reviewed 2021-07-27 18:55:15 +00:00

modules/git/repo_tag.go

						
				@ -170,3 +171,4 @@

					tagNames := strings.Split(strings.TrimRight(stdout, "\n"), "\n")

					tagsTotal := len(tagNames)

					if page != 0 {

zeripath commented

2021-07-27 18:55:14 +00:00

leave it for this PR.

zeripath reviewed 2021-07-27 20:30:30 +00:00

routers/api/v1/repo/topic.go

						
				@ -164,15 +167,15 @@ func AddTopic(ctx *context.APIContext) {

					}

zeripath commented

2021-07-27 20:30:29 +00:00

Look at line 180 below.

6543 (Migrated from github.com) reviewed 2021-07-27 23:59:30 +00:00

routers/api/v1/repo/topic.go

						
				@ -164,15 +167,15 @@ func AddTopic(ctx *context.APIContext) {

					}

6543 (Migrated from github.com) commented

2021-07-27 23:59:30 +00:00

it doesnt mather if we use len or the returned value in this case

6543 (Migrated from github.com) reviewed 2021-07-28 00:05:10 +00:00

modules/git/repo_tag.go

						
				@ -170,3 +171,4 @@

					tagNames := strings.Split(strings.TrimRight(stdout, "\n"), "\n")

					tagsTotal := len(tagNames)

					if page != 0 {

6543 (Migrated from github.com) commented

2021-07-28 00:05:10 +00:00

heh I'm aware of that and the current code too - see TODO 😅

heh I'm aware of that and the current code too - see TODO :sweat_smile:

lunny reviewed 2021-07-28 14:06:42 +00:00

models/commit_status.go

						
				@ -162,3 +162,3 @@

					return statuses, x.In("id", ids).Find(&statuses)

					return statuses, e.In("id", ids).Find(&statuses)

				}

lunny commented

2021-07-28 14:06:42 +00:00

Good catch, this could be back ported

lunny reviewed 2021-07-28 14:11:24 +00:00

models/org.go

						
				@ -87,7 +83,7 @@ type FindOrgMembersOpts struct {

				}

lunny commented

2021-07-28 14:11:23 +00:00

Assume user has joined over 1000 teams? Or we should limit user max joined teams number.

zeripath reviewed 2021-07-28 18:24:52 +00:00

routers/api/v1/org/member.go

						
				@ -30,3 +35,4 @@

						ctx.InternalServerError(err)

						return

					}

zeripath commented

2021-07-28 18:24:52 +00:00

So looking at the code nope - the 2nd return of this function is a map[int64]bool{} saying whether the member is public or not.

zeripath approved these changes 2021-07-28 18:35:36 +00:00

6543 (Migrated from github.com) reviewed 2021-07-29 03:46:13 +00:00

models/commit_status.go

						
				@ -162,3 +162,3 @@

					return statuses, x.In("id", ids).Find(&statuses)

					return statuses, e.In("id", ids).Find(&statuses)

				}

6543 (Migrated from github.com) commented

2021-07-29 03:46:13 +00:00

-> #16553 😄

-> #16553 :smile:

6543 (Migrated from github.com) reviewed 2021-07-29 03:48:51 +00:00

models/org.go

						
				@ -87,7 +83,7 @@ type FindOrgMembersOpts struct {

				}

6543 (Migrated from github.com) commented

2021-07-29 03:48:51 +00:00

this org.getTeams is exactly used as other struct.loadIssues/loadX/loadY/.. functions so I renamed it.
removing it because of speed improvements is not in scope of this pull ... but If you wish I'll add a todo with an open issue to this line?

this org.getTeams is exactly used as other struct.**loadIssues/loadX/loadY/..** functions so I renamed it. removing it because of speed improvements is not in scope of this pull ... but If you wish I'll add a todo with an open issue to this line?

lunny reviewed 2021-07-29 14:37:54 +00:00

models/org.go

						
				@ -87,7 +83,7 @@ type FindOrgMembersOpts struct {

				}

lunny commented

2021-07-29 14:37:54 +00:00

Yes, I would like to leave it to another PR and not remove the paniation.

KN4CK3R (Migrated from github.com) approved these changes 2021-08-12 11:56:28 +00:00

This repo is archived. You cannot comment on pull requests.

No reviewers

KN4CK3R

No Label

hacktoberfest-accepted

kind/upstream-related

status/needs-feedback

No Milestone

No project

No Assignees

2 Participants

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: lunny/gitea#16551

				`@ -164,15 +167,15 @@ func AddTopic(ctx *context.APIContext) {`
				`}`