From f603b41d994c3ddd6e4c4b68418d4cec43be5ab3 Mon Sep 17 00:00:00 2001 From: konrad Date: Mon, 27 Jan 2020 17:28:17 +0000 Subject: [PATCH] Better efficency for loading teams (#128) Fix staticcheck Better performance for getting teams on a namespace Better performance for getting teams on a list Fix lint Fix swagger Signed-off-by: kolaente Make loading a single full team more efficent Signed-off-by: kolaente Make loading teams more efficent Signed-off-by: kolaente Co-authored-by: kolaente Reviewed-on: https://kolaente.dev/vikunja/api/pulls/128 --- go.mod | 1 + pkg/models/list_team.go | 10 ++++ pkg/models/namespace_team.go | 10 ++++ pkg/models/task_assignees.go | 2 +- pkg/models/teams.go | 88 +++++++++++++++++++++++----- pkg/routes/api/v1/login.go | 2 +- pkg/routes/api/v1/user_add_update.go | 4 +- pkg/routes/api/v1/user_list.go | 4 +- pkg/routes/api/v1/user_show.go | 2 +- pkg/swagger/docs.go | 48 +++++++-------- 10 files changed, 124 insertions(+), 47 deletions(-) diff --git a/go.mod b/go.mod index 6905e2a0..3b57fe66 100644 --- a/go.mod +++ b/go.mod @@ -58,6 +58,7 @@ require ( github.com/onsi/gomega v1.4.3 // indirect github.com/op/go-logging v0.0.0-20160315200505-970db520ece7 github.com/pelletier/go-toml v1.4.0 // indirect + github.com/pkg/errors v0.8.1 // indirect github.com/prometheus/client_golang v0.9.2 github.com/samedi/caldav-go v3.0.0+incompatible github.com/shurcooL/httpfs v0.0.0-20190527155220-6a4d4a70508b diff --git a/pkg/models/list_team.go b/pkg/models/list_team.go index 04b128a6..351b50f4 100644 --- a/pkg/models/list_team.go +++ b/pkg/models/list_team.go @@ -186,6 +186,16 @@ func (tl *TeamList) ReadAll(a web.Auth, search string, page int, perPage int) (r return nil, 0, 0, err } + teams := []*Team{} + for _, t := range all { + teams = append(teams, &t.Team) + } + + err = addMoreInfoToTeams(teams) + if err != nil { + return + } + totalItems, err = x. Table("teams"). Join("INNER", "team_list", "team_id = teams.id"). diff --git a/pkg/models/namespace_team.go b/pkg/models/namespace_team.go index 6c58c4a1..2f205bb2 100644 --- a/pkg/models/namespace_team.go +++ b/pkg/models/namespace_team.go @@ -171,6 +171,16 @@ func (tn *TeamNamespace) ReadAll(a web.Auth, search string, page int, perPage in return nil, 0, 0, err } + teams := []*Team{} + for _, t := range all { + teams = append(teams, &t.Team) + } + + err = addMoreInfoToTeams(teams) + if err != nil { + return + } + numberOfTotalItems, err = x.Table("teams"). Join("INNER", "team_namespaces", "team_id = teams.id"). Where("team_namespaces.namespace_id = ?", tn.NamespaceID). diff --git a/pkg/models/task_assignees.go b/pkg/models/task_assignees.go index 2a59569a..47aa7c59 100644 --- a/pkg/models/task_assignees.go +++ b/pkg/models/task_assignees.go @@ -236,7 +236,7 @@ func (t *Task) addNewAssigneeByID(newAssigneeID int64, list *List) (err error) { // @Param s query string false "Search assignees by their username." // @Param taskID path int true "Task ID" // @Security JWTKeyAuth -// @Success 200 {array} models.User "The assignees" +// @Success 200 {array} user.User "The assignees" // @Failure 500 {object} models.Message "Internal error" // @Router /tasks/{taskID}/assignees [get] func (la *TaskAssginee) ReadAll(a web.Auth, search string, page int, perPage int) (result interface{}, resultCount int, numberOfTotalItems int64, err error) { diff --git a/pkg/models/teams.go b/pkg/models/teams.go index b77047f7..25e626be 100644 --- a/pkg/models/teams.go +++ b/pkg/models/teams.go @@ -20,6 +20,7 @@ import ( "code.vikunja.io/api/pkg/metrics" "code.vikunja.io/api/pkg/user" "code.vikunja.io/web" + "github.com/go-xorm/builder" ) // Team holds a team object @@ -53,15 +54,6 @@ func (Team) TableName() string { // AfterLoad gets the created by user object func (t *Team) AfterLoad() { - // Get the owner - t.CreatedBy, _ = user.GetUserByID(t.CreatedByID) - - // Get all members - x.Select("*"). - Table("users"). - Join("INNER", "team_members", "team_members.user_id = users.id"). - Where("team_id = ?", t.ID). - Find(&t.Members) } // TeamMember defines the relationship between a user and a team @@ -93,23 +85,76 @@ func (TeamMember) TableName() string { type TeamUser struct { user.User `xorm:"extends"` // Whether or not the member is an admin of the team. See the docs for more about what a team admin can do - Admin bool `json:"admin"` + Admin bool `json:"admin"` + TeamID int64 `json:"-"` } // GetTeamByID gets a team by its ID -func GetTeamByID(id int64) (team Team, err error) { +func GetTeamByID(id int64) (team *Team, err error) { if id < 1 { return team, ErrTeamDoesNotExist{id} } - exists, err := x.Where("id = ?", id).Get(&team) + t := Team{} + + exists, err := x. + Where("id = ?", id). + Get(&t) if err != nil { return } if !exists { - return team, ErrTeamDoesNotExist{id} + return &t, ErrTeamDoesNotExist{id} } + teamSlice := []*Team{&t} + err = addMoreInfoToTeams(teamSlice) + if err != nil { + return + } + + team = &t + + return +} + +func addMoreInfoToTeams(teams []*Team) (err error) { + // Put the teams in a map to make assigning more info to it more efficient + teamMap := make(map[int64]*Team, len(teams)) + var teamIDs []int64 + var ownerIDs []int64 + for _, team := range teams { + teamMap[team.ID] = team + teamIDs = append(teamIDs, team.ID) + ownerIDs = append(ownerIDs, team.CreatedByID) + } + + // Get all owners and team members + users := []*TeamUser{} + err = x.Select("*"). + Table("users"). + Join("LEFT", "team_members", "team_members.user_id = users.id"). + Join("LEFT", "teams", "team_members.team_id = teams.id"). + Or( + builder.In("team_id", teamIDs), + builder.And( + builder.In("users.id", ownerIDs), + builder.Expr("teams.created_by_id = users.id"), + builder.In("teams.id", teamIDs), + ), + ). + Find(&users) + if err != nil { + return + } + for _, u := range users { + if _, exists := teamMap[u.TeamID]; !exists { + continue + } + u.Email = "" + teamMap[u.TeamID].CreatedBy = &u.User + teamMap[u.TeamID].Members = append(teamMap[u.TeamID].Members, u) + } return } @@ -124,9 +169,12 @@ func GetTeamByID(id int64) (team Team, err error) { // @Success 200 {object} models.Team "The team" // @Failure 403 {object} code.vikunja.io/web.HTTPError "The user does not have access to the team" // @Failure 500 {object} models.Message "Internal error" -// @Router /lists/{id} [get] +// @Router /teams/{id} [get] func (t *Team) ReadOne() (err error) { - *t, err = GetTeamByID(t.ID) + team, err := GetTeamByID(t.ID) + if team != nil { + *t = *team + } return } @@ -160,6 +208,11 @@ func (t *Team) ReadAll(a web.Auth, search string, page int, perPage int) (result return nil, 0, 0, err } + err = addMoreInfoToTeams(all) + if err != nil { + return nil, 0, 0, err + } + numberOfTotalItems, err = x. Table("teams"). Join("INNER", "team_members", "team_members.team_id = teams.id"). @@ -282,7 +335,10 @@ func (t *Team) Update() (err error) { } // Get the newly updated team - *t, err = GetTeamByID(t.ID) + team, err := GetTeamByID(t.ID) + if team != nil { + *t = *team + } return } diff --git a/pkg/routes/api/v1/login.go b/pkg/routes/api/v1/login.go index e92bad3b..cd9b2915 100644 --- a/pkg/routes/api/v1/login.go +++ b/pkg/routes/api/v1/login.go @@ -36,7 +36,7 @@ type Token struct { // @tags user // @Accept json // @Produce json -// @Param credentials body models.UserLogin true "The login credentials" +// @Param credentials body user.UserLogin true "The login credentials" // @Success 200 {object} v1.Token // @Failure 400 {object} models.Message "Invalid user password model." // @Failure 403 {object} models.Message "Invalid username or password." diff --git a/pkg/routes/api/v1/user_add_update.go b/pkg/routes/api/v1/user_add_update.go index 42e5a025..31bddeec 100644 --- a/pkg/routes/api/v1/user_add_update.go +++ b/pkg/routes/api/v1/user_add_update.go @@ -31,8 +31,8 @@ import ( // @tags user // @Accept json // @Produce json -// @Param credentials body models.APIUserPassword true "The user credentials" -// @Success 200 {object} models.User +// @Param credentials body user.APIUserPassword true "The user credentials" +// @Success 200 {object} user.User // @Failure 400 {object} code.vikunja.io/web.HTTPError "No or invalid user register object provided / User already exists." // @Failure 500 {object} models.Message "Internal error" // @Router /register [post] diff --git a/pkg/routes/api/v1/user_list.go b/pkg/routes/api/v1/user_list.go index abfca4c4..55776193 100644 --- a/pkg/routes/api/v1/user_list.go +++ b/pkg/routes/api/v1/user_list.go @@ -33,7 +33,7 @@ import ( // @Produce json // @Param s query string false "Search for a user by its name." // @Security JWTKeyAuth -// @Success 200 {array} models.User "All (found) users." +// @Success 200 {array} user.User "All (found) users." // @Failure 400 {object} code.vikunja.io/web.HTTPError "Something's invalid." // @Failure 500 {object} models.Message "Internal server error." // @Router /users [get] @@ -61,7 +61,7 @@ func UserList(c echo.Context) error { // @Param s query string false "Search for a user by its name." // @Security JWTKeyAuth // @Param id path int true "List ID" -// @Success 200 {array} models.User "All (found) users." +// @Success 200 {array} user.User "All (found) users." // @Failure 400 {object} code.vikunja.io/web.HTTPError "Something's invalid." // @Failure 401 {object} code.vikunja.io/web.HTTPError "The user does not have the right to see the list." // @Failure 500 {object} models.Message "Internal server error." diff --git a/pkg/routes/api/v1/user_show.go b/pkg/routes/api/v1/user_show.go index 390a5879..c8820687 100644 --- a/pkg/routes/api/v1/user_show.go +++ b/pkg/routes/api/v1/user_show.go @@ -30,7 +30,7 @@ import ( // @Accept json // @Produce json // @Security JWTKeyAuth -// @Success 200 {object} models.User +// @Success 200 {object} user.User // @Failure 404 {object} code.vikunja.io/web.HTTPError "User does not exist." // @Failure 500 {object} models.Message "Internal server error." // @Router /user [get] diff --git a/pkg/swagger/docs.go b/pkg/swagger/docs.go index b7dc4f95..c7ee5838 100644 --- a/pkg/swagger/docs.go +++ b/pkg/swagger/docs.go @@ -664,7 +664,7 @@ var doc = `{ "schema": { "type": "array", "items": { - "$ref": "#/definitions/models.User" + "$ref": "#/definitions/user.User" } } }, @@ -1589,7 +1589,7 @@ var doc = `{ "required": true, "schema": { "type": "object", - "$ref": "#/definitions/models.UserLogin" + "$ref": "#/definitions/user.UserLogin" } } ], @@ -2688,7 +2688,7 @@ var doc = `{ "200": { "description": "OK", "schema": { - "$ref": "#/definitions/models.User" + "$ref": "#/definitions/user.User" } }, "400": { @@ -3347,7 +3347,7 @@ var doc = `{ "schema": { "type": "array", "items": { - "$ref": "#/definitions/models.User" + "$ref": "#/definitions/user.User" } } }, @@ -4251,7 +4251,7 @@ var doc = `{ "200": { "description": "OK", "schema": { - "$ref": "#/definitions/models.User" + "$ref": "#/definitions/user.User" } }, "404": { @@ -4529,7 +4529,7 @@ var doc = `{ "schema": { "type": "array", "items": { - "$ref": "#/definitions/models.User" + "$ref": "#/definitions/user.User" } } }, @@ -4626,7 +4626,7 @@ var doc = `{ "description": "A list with all assignees", "type": "array", "items": { - "$ref": "#/definitions/models.User" + "$ref": "#/definitions/user.User" } } } @@ -4638,7 +4638,7 @@ var doc = `{ "description": "An array of users who are assigned to this task", "type": "array", "items": { - "$ref": "#/definitions/models.User" + "$ref": "#/definitions/user.User" } }, "attachments": { @@ -4655,7 +4655,7 @@ var doc = `{ "createdBy": { "description": "The user who initially created the task.", "type": "object", - "$ref": "#/definitions/models.User" + "$ref": "#/definitions/user.User" }, "description": { "description": "The task description.", @@ -4771,7 +4771,7 @@ var doc = `{ "created_by": { "description": "The user who created this label", "type": "object", - "$ref": "#/definitions/models.User" + "$ref": "#/definitions/user.User" }, "description": { "description": "The label description.", @@ -4847,7 +4847,7 @@ var doc = `{ "shared_by": { "description": "The user who shared this list", "type": "object", - "$ref": "#/definitions/models.User" + "$ref": "#/definitions/user.User" }, "sharing_type": { "description": "The kind of this link. 0 = undefined, 1 = without password, 2 = with password (currently not implemented).", @@ -4885,7 +4885,7 @@ var doc = `{ "owner": { "description": "The user who created this list.", "type": "object", - "$ref": "#/definitions/models.User" + "$ref": "#/definitions/user.User" }, "title": { "description": "The title of the list. You'll see this in the namespace overview.", @@ -4959,7 +4959,7 @@ var doc = `{ "owner": { "description": "The user who owns this namespace", "type": "object", - "$ref": "#/definitions/models.User" + "$ref": "#/definitions/user.User" }, "updated": { "description": "A unix timestamp when this namespace was last updated. You cannot change this value.", @@ -5024,7 +5024,7 @@ var doc = `{ "owner": { "description": "The user who owns this namespace", "type": "object", - "$ref": "#/definitions/models.User" + "$ref": "#/definitions/user.User" }, "updated": { "description": "A unix timestamp when this namespace was last updated. You cannot change this value.", @@ -5065,7 +5065,7 @@ var doc = `{ "description": "An array of users who are assigned to this task", "type": "array", "items": { - "$ref": "#/definitions/models.User" + "$ref": "#/definitions/user.User" } }, "attachments": { @@ -5082,7 +5082,7 @@ var doc = `{ "createdBy": { "description": "The user who initially created the task.", "type": "object", - "$ref": "#/definitions/models.User" + "$ref": "#/definitions/user.User" }, "description": { "description": "The task description.", @@ -5181,7 +5181,7 @@ var doc = `{ "description": "An array of users who are assigned to this task", "type": "array", "items": { - "$ref": "#/definitions/models.User" + "$ref": "#/definitions/user.User" } }, "attachments": { @@ -5198,7 +5198,7 @@ var doc = `{ "createdBy": { "description": "The user who initially created the task.", "type": "object", - "$ref": "#/definitions/models.User" + "$ref": "#/definitions/user.User" }, "description": { "description": "The task description.", @@ -5307,7 +5307,7 @@ var doc = `{ }, "created_by": { "type": "object", - "$ref": "#/definitions/models.User" + "$ref": "#/definitions/user.User" }, "file": { "type": "object", @@ -5331,7 +5331,7 @@ var doc = `{ "created_by": { "description": "The user who created this relation", "type": "object", - "$ref": "#/definitions/models.User" + "$ref": "#/definitions/user.User" }, "other_task_id": { "description": "The ID of the other task, the task which is being related.", @@ -5357,7 +5357,7 @@ var doc = `{ "createdBy": { "description": "The user who created this team.", "type": "object", - "$ref": "#/definitions/models.User" + "$ref": "#/definitions/user.User" }, "description": { "description": "The team's description.", @@ -5507,7 +5507,7 @@ var doc = `{ "createdBy": { "description": "The user who created this team.", "type": "object", - "$ref": "#/definitions/models.User" + "$ref": "#/definitions/user.User" }, "description": { "description": "The team's description.", @@ -5539,7 +5539,7 @@ var doc = `{ } } }, - "models.User": { + "user.User": { "type": "object", "properties": { "avatarUrl": { @@ -5571,7 +5571,7 @@ var doc = `{ } } }, - "models.UserLogin": { + "user.UserLogin": { "type": "object", "properties": { "password": {