From ded028c7b8c6565a99da362e5a00745417138f3e Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sat, 24 Aug 2019 14:50:20 +0200 Subject: [PATCH 01/29] Create API endpoints for repo topics. Signed-off-by: David Svantesson --- models/topic.go | 116 ++++++++++---- modules/structs/repo_topic.go | 18 +++ routers/api/v1/api.go | 6 + routers/api/v1/convert/convert.go | 11 ++ routers/api/v1/repo/repo.go | 42 ----- routers/api/v1/repo/topic.go | 250 ++++++++++++++++++++++++++++++ routers/api/v1/swagger/repo.go | 14 ++ 7 files changed, 386 insertions(+), 71 deletions(-) create mode 100644 modules/structs/repo_topic.go create mode 100644 routers/api/v1/repo/topic.go diff --git a/models/topic.go b/models/topic.go index 8a587acc3aea0..b62b809530d2b 100644 --- a/models/topic.go +++ b/models/topic.go @@ -70,6 +70,52 @@ func GetTopicByName(name string) (*Topic, error) { return &topic, nil } +// addTopicByNameToRepo adds a topic name to a repo and increments the topic count. +// Returns topic after the addition +func addTopicByNameToRepo(repoID int64, topicName string, e Engine) (*Topic, error) { + var topic Topic + if has, err := e.Where("name = ?", topicName).Get(&topic); err != nil { + return nil, err + } else if !has { + topic.Name = topicName + topic.RepoCount = 1 + if _, err := e.Insert(&topic); err != nil { + return nil, err + } + } else { + topic.RepoCount++ + if _, err := e.ID(topic.ID).Cols("repo_count").Update(&topic); err != nil { + return nil, err + } + } + + if _, err := e.Insert(&RepoTopic{ + RepoID: repoID, + TopicID: topic.ID, + }); err != nil { + return nil, err + } + + return &topic, nil +} + +// removeTopicFromRepo remove a topic from a repo and decrements the topic repo count +func removeTopicFromRepo(repoID int64, topic *Topic, e Engine) error { + topic.RepoCount-- + if _, err := e.ID(topic.ID).Cols("repo_count").Update(topic); err != nil { + return err + } + + if _, err := e.Delete(&RepoTopic{ + RepoID: repoID, + TopicID: topic.ID, + }); err != nil { + return err + } + + return nil +} + // FindTopicOptions represents the options when fdin topics type FindTopicOptions struct { RepoID int64 @@ -103,6 +149,43 @@ func FindTopics(opts *FindTopicOptions) (topics []*Topic, err error) { return topics, sess.Desc("topic.repo_count").Find(&topics) } +// AddTopic adds a topic name to a repository (if it does not already have it) +func AddTopic(repoID int64, topicName string) (*Topic, error) { + topics, err := FindTopics(&FindTopicOptions{ + RepoID: repoID, + Keyword: topicName, + }) + if err != nil { + return nil, err + } + if len(topics) != 0 { + // Repo already have topic + return topics[0], nil + } + + return addTopicByNameToRepo(repoID, topicName, x) +} + +// DeleteTopic removes a topic name from a repository (if it has it) +func DeleteTopic(repoID int64, topicName string) (*Topic, error) { + topics, err := FindTopics(&FindTopicOptions{ + RepoID: repoID, + Keyword: topicName, + }) + if err != nil { + return nil, err + } + if len(topics) == 0 { + // Repo doesn't have topic, can't be removed + return nil, nil + } + topic := topics[0] + + err = removeTopicFromRepo(repoID, topic, x) + + return topic, err +} + // SaveTopics save topics to a repository func SaveTopics(repoID int64, topicNames ...string) error { topics, err := FindTopics(&FindTopicOptions{ @@ -152,40 +235,15 @@ func SaveTopics(repoID int64, topicNames ...string) error { } for _, topicName := range addedTopicNames { - var topic Topic - if has, err := sess.Where("name = ?", topicName).Get(&topic); err != nil { - return err - } else if !has { - topic.Name = topicName - topic.RepoCount = 1 - if _, err := sess.Insert(&topic); err != nil { - return err - } - } else { - topic.RepoCount++ - if _, err := sess.ID(topic.ID).Cols("repo_count").Update(&topic); err != nil { - return err - } - } - - if _, err := sess.Insert(&RepoTopic{ - RepoID: repoID, - TopicID: topic.ID, - }); err != nil { + _, err := addTopicByNameToRepo(repoID, topicName, sess) + if err != nil { return err } } for _, topic := range removeTopics { - topic.RepoCount-- - if _, err := sess.ID(topic.ID).Cols("repo_count").Update(topic); err != nil { - return err - } - - if _, err := sess.Delete(&RepoTopic{ - RepoID: repoID, - TopicID: topic.ID, - }); err != nil { + err := removeTopicFromRepo(repoID, topic, sess) + if err != nil { return err } } diff --git a/modules/structs/repo_topic.go b/modules/structs/repo_topic.go new file mode 100644 index 0000000000000..0ff7a940fe5a7 --- /dev/null +++ b/modules/structs/repo_topic.go @@ -0,0 +1,18 @@ +// Copyright 2019 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package structs + +import ( + "time" +) + +// TopicResponse for returning topics +type TopicResponse struct { + ID int64 `json:"id"` + Name string `json:"topic_name"` + RepoCount int `json:"repo_count"` + Created time.Time `json:"created"` + Updated time.Time `json:"updated"` +} diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index cd308fce51824..b176997d6a86b 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -775,6 +775,12 @@ func RegisterRoutes(m *macaron.Macaron) { m.Delete("", bind(api.DeleteFileOptions{}), repo.DeleteFile) }, reqRepoWriter(models.UnitTypeCode), reqToken()) }, reqRepoReader(models.UnitTypeCode)) + m.Group("/topics", func() { + m.Get("", repo.ListTopics) + m.Combo("/:topic").Get(repo.HasTopic). + Put(reqToken(), reqRepoWriter(models.UnitTypeCode), repo.AddTopic). + Delete(reqToken(), reqRepoWriter(models.UnitTypeCode), repo.DeleteTopic) + }, reqRepoReader(models.UnitTypeCode)) }, repoAssignment()) }) diff --git a/routers/api/v1/convert/convert.go b/routers/api/v1/convert/convert.go index d2691f8238086..f3e3feee97d39 100644 --- a/routers/api/v1/convert/convert.go +++ b/routers/api/v1/convert/convert.go @@ -291,3 +291,14 @@ func ToCommitMeta(repo *models.Repository, tag *git.Tag) *api.CommitMeta { URL: util.URLJoin(repo.APIURL(), "git/commits", tag.ID.String()), } } + +// ToTopic convert from models.Topic to api.TopicResponse +func ToTopicResponse(topic *models.Topic) *api.TopicResponse { + return &api.TopicResponse{ + ID: topic.ID, + Name: topic.Name, + RepoCount: topic.RepoCount, + Created: topic.CreatedUnix.AsTime(), + Updated: topic.UpdatedUnix.AsTime(), + } +} diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go index 8d7e43edff8cf..8bbf02b17be19 100644 --- a/routers/api/v1/repo/repo.go +++ b/routers/api/v1/repo/repo.go @@ -863,45 +863,3 @@ func MirrorSync(ctx *context.APIContext) { go models.MirrorQueue.Add(repo.ID) ctx.Status(200) } - -// TopicSearch search for creating topic -func TopicSearch(ctx *context.Context) { - // swagger:operation GET /topics/search repository topicSearch - // --- - // summary: search topics via keyword - // produces: - // - application/json - // parameters: - // - name: q - // in: query - // description: keywords to search - // required: true - // type: string - // responses: - // "200": - // "$ref": "#/responses/Repository" - if ctx.User == nil { - ctx.JSON(403, map[string]interface{}{ - "message": "Only owners could change the topics.", - }) - return - } - - kw := ctx.Query("q") - - topics, err := models.FindTopics(&models.FindTopicOptions{ - Keyword: kw, - Limit: 10, - }) - if err != nil { - log.Error("SearchTopics failed: %v", err) - ctx.JSON(500, map[string]interface{}{ - "message": "Search topics failed.", - }) - return - } - - ctx.JSON(200, map[string]interface{}{ - "topics": topics, - }) -} diff --git a/routers/api/v1/repo/topic.go b/routers/api/v1/repo/topic.go new file mode 100644 index 0000000000000..b6221e260f560 --- /dev/null +++ b/routers/api/v1/repo/topic.go @@ -0,0 +1,250 @@ +// Copyright 2019 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package repo + +import ( + "net/http" + "strings" + + "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/context" + "code.gitea.io/gitea/modules/log" + api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/routers/api/v1/convert" +) + +func ListTopics(ctx *context.APIContext) { + // swagger:operation GET /repos/{owner}/{repo}/topics repository repoListTopics + // --- + // summary: Get list of topics that a repository has + // produces: + // - application/json + // parameters: + // - name: owner + // in: path + // description: owner of the repo + // type: string + // required: true + // - name: repo + // in: path + // description: name of the repo + // type: string + // required: true + // responses: + // "200": + // "$ref": "#/responses/TopicListResponse" + + topics, err := models.FindTopics(&models.FindTopicOptions{ + RepoID: ctx.Repo.Repository.ID, + }) + if err != nil { + log.Error("ListTopics failed: %v", err) + ctx.JSON(500, map[string]interface{}{ + "message": "ListTopics failed.", + }) + return + } + + topicResponses := make([]*api.TopicResponse, len(topics)) + for i, topic := range topics { + topicResponses[i] = convert.ToTopicResponse(topic) + } + ctx.JSON(200, map[string]interface{}{ + "topics": topicResponses, + }) +} + +func HasTopic(ctx *context.APIContext) { + // swagger:operation GET /repos/{owner}/{repo}/topics/{topic} repository repoHasTopic + // --- + // summary: Check if a repository has topic + // produces: + // - application/json + // parameters: + // - name: owner + // in: path + // description: owner of the repo + // type: string + // required: true + // - name: repo + // in: path + // description: name of the repo + // type: string + // required: true + // - name: topic + // in: path + // description: name of the topic to check for + // type: string + // required: true + // responses: + // "201": + // "$ref": "#/responses/TopicResponse" + // "404": + // "$ref": "#/responses/empty" + topicName := strings.TrimSpace(strings.ToLower(ctx.Params(":topic"))) + + topics, err := models.FindTopics(&models.FindTopicOptions{ + RepoID: ctx.Repo.Repository.ID, + Keyword: topicName, + }) + if err != nil { + log.Error("HasTopic failed: %v", err) + ctx.JSON(500, map[string]interface{}{ + "message": "HasTopic failed.", + }) + return + } + + if len(topics) == 0 { + ctx.NotFound() + } + + ctx.JSON(200, map[string]interface{}{ + "topic": convert.ToTopicResponse(topics[0]), + }) +} + +func AddTopic(ctx *context.APIContext) { + // swagger:operation PUT /repos/{owner}/{repo}/topics/{topic} repository repoAddTopíc + // --- + // summary: Add a topic from a repository + // produces: + // - application/json + // parameters: + // - name: owner + // in: path + // description: owner of the repo + // type: string + // required: true + // - name: repo + // in: path + // description: name of the repo + // type: string + // required: true + // - name: topic + // in: path + // description: name of the topic to add + // type: string + // required: true + // responses: + // "201": + // "$ref": "#/responses/TopicResponse" + + topicName := strings.TrimSpace(strings.ToLower(ctx.Params(":topic"))) + + if !models.ValidateTopic(topicName) { + ctx.Error(http.StatusUnprocessableEntity, "", "Topic name is invalid") + return + } + + topic, err := models.AddTopic(ctx.Repo.Repository.ID, topicName) + if err != nil { + log.Error("AddTopic failed: %v", err) + ctx.JSON(500, map[string]interface{}{ + "message": "AddTopic failed.", + }) + return + } + + ctx.JSON(201, map[string]interface{}{ + "topic": convert.ToTopicResponse(topic), + }) +} + +func DeleteTopic(ctx *context.APIContext) { + // swagger:operation DELETE /repos/{owner}/{repo}/topics/{topic} repository repoDeleteTopic + // --- + // summary: delete a topic from a repository + // produces: + // - application/json + // parameters: + // - name: owner + // in: path + // description: owner of the repo + // type: string + // required: true + // - name: repo + // in: path + // description: name of the repo + // type: string + // required: true + // - name: topic + // in: path + // description: name of the topic to delete + // type: string + // required: true + // responses: + // "201": + // "$ref": "#/responses/TopicResponse" + topicName := strings.TrimSpace(strings.ToLower(ctx.Params(":topic"))) + + if !models.ValidateTopic(topicName) { + ctx.Error(http.StatusUnprocessableEntity, "", "Topic name is invalid") + return + } + + topic, err := models.DeleteTopic(ctx.Repo.Repository.ID, topicName) + if err != nil { + log.Error("DeleteTopic failed: %v", err) + ctx.JSON(500, map[string]interface{}{ + "message": "DeleteTopic failed.", + }) + return + } + + if topic == nil { + ctx.NotFound() + } + + ctx.JSON(201, map[string]interface{}{ + "topic": convert.ToTopicResponse(topic), + }) +} + +// TopicSearch search for creating topic +func TopicSearch(ctx *context.Context) { + // swagger:operation GET /topics/search repository topicSearch + // --- + // summary: search topics via keyword + // produces: + // - application/json + // parameters: + // - name: q + // in: query + // description: keywords to search + // required: true + // type: string + // responses: + // "200": + // "$ref": "#/responses/TopicListResponse" + if ctx.User == nil { + ctx.JSON(403, map[string]interface{}{ + "message": "Only owners could change the topics.", + }) + return + } + + kw := ctx.Query("q") + + topics, err := models.FindTopics(&models.FindTopicOptions{ + Keyword: kw, + Limit: 10, + }) + if err != nil { + log.Error("SearchTopics failed: %v", err) + ctx.JSON(500, map[string]interface{}{ + "message": "Search topics failed.", + }) + return + } + + topicResponses := make([]*api.TopicResponse, len(topics)) + for i, topic := range topics { + topicResponses[i] = convert.ToTopicResponse(topic) + } + ctx.JSON(200, map[string]interface{}{ + "topics": topicResponses, + }) +} diff --git a/routers/api/v1/swagger/repo.go b/routers/api/v1/swagger/repo.go index 2cab5b0ed42f7..863aecfbe0a1d 100644 --- a/routers/api/v1/swagger/repo.go +++ b/routers/api/v1/swagger/repo.go @@ -217,3 +217,17 @@ type swaggerFileDeleteResponse struct { //in: body Body api.FileDeleteResponse `json:"body"` } + +// TopicResponse +// swagger:response TopicResponse +type swaggerTopicResponse struct { + //in: body + Body api.TopicResponse `json:"body"` +} + +// TopicListResponse +// swagger:response TopicListResponse +type swaggerTopicListResponse struct { + //in: body + Body []api.TopicResponse `json:"body"` +} From 6965aa8a4685767dc1017a0514aa0c045044f11c Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sat, 24 Aug 2019 14:32:25 +0000 Subject: [PATCH 02/29] Generate swagger Signed-off-by: David Svantesson --- templates/swagger/v1_json.tmpl | 200 ++++++++++++++++++++++++++++++++- 1 file changed, 199 insertions(+), 1 deletion(-) diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 2d5f8c5c9c253..8ad683e77b1af 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -5388,6 +5388,158 @@ } } }, + "/repos/{owner}/{repo}/topics": { + "get": { + "produces": [ + "application/json" + ], + "tags": [ + "repository" + ], + "summary": "Get list of topics that a repository has", + "operationId": "repoListTopics", + "parameters": [ + { + "type": "string", + "description": "owner of the repo", + "name": "owner", + "in": "path", + "required": true + }, + { + "type": "string", + "description": "name of the repo", + "name": "repo", + "in": "path", + "required": true + } + ], + "responses": { + "200": { + "$ref": "#/responses/TopicListResponse" + } + } + } + }, + "/repos/{owner}/{repo}/topics/{topic}": { + "get": { + "produces": [ + "application/json" + ], + "tags": [ + "repository" + ], + "summary": "Check if a repository has topic", + "operationId": "repoHasTopic", + "parameters": [ + { + "type": "string", + "description": "owner of the repo", + "name": "owner", + "in": "path", + "required": true + }, + { + "type": "string", + "description": "name of the repo", + "name": "repo", + "in": "path", + "required": true + }, + { + "type": "string", + "description": "name of the topic to check for", + "name": "topic", + "in": "path", + "required": true + } + ], + "responses": { + "201": { + "$ref": "#/responses/TopicResponse" + }, + "404": { + "$ref": "#/responses/empty" + } + } + }, + "put": { + "produces": [ + "application/json" + ], + "tags": [ + "repository" + ], + "summary": "Add a topic from a repository", + "operationId": "repoAddTopíc", + "parameters": [ + { + "type": "string", + "description": "owner of the repo", + "name": "owner", + "in": "path", + "required": true + }, + { + "type": "string", + "description": "name of the repo", + "name": "repo", + "in": "path", + "required": true + }, + { + "type": "string", + "description": "name of the topic to add", + "name": "topic", + "in": "path", + "required": true + } + ], + "responses": { + "201": { + "$ref": "#/responses/TopicResponse" + } + } + }, + "delete": { + "produces": [ + "application/json" + ], + "tags": [ + "repository" + ], + "summary": "delete a topic from a repository", + "operationId": "repoDeleteTopic", + "parameters": [ + { + "type": "string", + "description": "owner of the repo", + "name": "owner", + "in": "path", + "required": true + }, + { + "type": "string", + "description": "name of the repo", + "name": "repo", + "in": "path", + "required": true + }, + { + "type": "string", + "description": "name of the topic to delete", + "name": "topic", + "in": "path", + "required": true + } + ], + "responses": { + "201": { + "$ref": "#/responses/TopicResponse" + } + } + } + }, "/repositories/{id}": { "get": { "produces": [ @@ -5752,7 +5904,7 @@ ], "responses": { "200": { - "$ref": "#/responses/Repository" + "$ref": "#/responses/TopicListResponse" } } } @@ -9772,6 +9924,37 @@ "format": "int64", "x-go-package": "code.gitea.io/gitea/modules/timeutil" }, + "TopicResponse": { + "description": "TopicResponse for returning topics", + "type": "object", + "properties": { + "created": { + "type": "string", + "format": "date-time", + "x-go-name": "Created" + }, + "id": { + "type": "integer", + "format": "int64", + "x-go-name": "ID" + }, + "repo_count": { + "type": "integer", + "format": "int64", + "x-go-name": "RepoCount" + }, + "topic_name": { + "type": "string", + "x-go-name": "Name" + }, + "updated": { + "type": "string", + "format": "date-time", + "x-go-name": "Updated" + } + }, + "x-go-package": "code.gitea.io/gitea/modules/structs" + }, "TrackedTime": { "description": "TrackedTime worked time for an issue / pr", "type": "object", @@ -10341,6 +10524,21 @@ } } }, + "TopicListResponse": { + "description": "TopicListResponse", + "schema": { + "type": "array", + "items": { + "$ref": "#/definitions/TopicResponse" + } + } + }, + "TopicResponse": { + "description": "TopicResponse", + "schema": { + "$ref": "#/definitions/TopicResponse" + } + }, "TrackedTime": { "description": "TrackedTime", "schema": { From 5a87bab1c0febebe345e3cf25b43d13b65bce04e Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sat, 24 Aug 2019 17:09:13 +0200 Subject: [PATCH 03/29] Add documentation to functions Signed-off-by: David Svantesson --- routers/api/v1/repo/topic.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/routers/api/v1/repo/topic.go b/routers/api/v1/repo/topic.go index b6221e260f560..03c05ea6da0a6 100644 --- a/routers/api/v1/repo/topic.go +++ b/routers/api/v1/repo/topic.go @@ -15,6 +15,7 @@ import ( "code.gitea.io/gitea/routers/api/v1/convert" ) +// ListTopics returns list of current topics for repo func ListTopics(ctx *context.APIContext) { // swagger:operation GET /repos/{owner}/{repo}/topics repository repoListTopics // --- @@ -56,6 +57,7 @@ func ListTopics(ctx *context.APIContext) { }) } +// HasTopic check if repo has topic name func HasTopic(ctx *context.APIContext) { // swagger:operation GET /repos/{owner}/{repo}/topics/{topic} repository repoHasTopic // --- @@ -106,6 +108,7 @@ func HasTopic(ctx *context.APIContext) { }) } +// AddTopic adds a topic name to a repo func AddTopic(ctx *context.APIContext) { // swagger:operation PUT /repos/{owner}/{repo}/topics/{topic} repository repoAddTopíc // --- @@ -153,6 +156,7 @@ func AddTopic(ctx *context.APIContext) { }) } +// DeleteTopic removes topic name from repo func DeleteTopic(ctx *context.APIContext) { // swagger:operation DELETE /repos/{owner}/{repo}/topics/{topic} repository repoDeleteTopic // --- From 0839b96d4d3ac64d573ab2dfa1e1c9674e2f4c1f Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sat, 24 Aug 2019 17:12:57 +0200 Subject: [PATCH 04/29] Grammar fix Signed-off-by: David Svantesson --- routers/api/v1/repo/topic.go | 4 ++-- templates/swagger/v1_json.tmpl | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/routers/api/v1/repo/topic.go b/routers/api/v1/repo/topic.go index 03c05ea6da0a6..8aebcc95b4f4f 100644 --- a/routers/api/v1/repo/topic.go +++ b/routers/api/v1/repo/topic.go @@ -112,7 +112,7 @@ func HasTopic(ctx *context.APIContext) { func AddTopic(ctx *context.APIContext) { // swagger:operation PUT /repos/{owner}/{repo}/topics/{topic} repository repoAddTopíc // --- - // summary: Add a topic from a repository + // summary: Add a topic to a repository // produces: // - application/json // parameters: @@ -160,7 +160,7 @@ func AddTopic(ctx *context.APIContext) { func DeleteTopic(ctx *context.APIContext) { // swagger:operation DELETE /repos/{owner}/{repo}/topics/{topic} repository repoDeleteTopic // --- - // summary: delete a topic from a repository + // summary: Delete a topic from a repository // produces: // - application/json // parameters: diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 8ad683e77b1af..e3b7bfad7276e 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -5470,7 +5470,7 @@ "tags": [ "repository" ], - "summary": "Add a topic from a repository", + "summary": "Add a topic to a repository", "operationId": "repoAddTopíc", "parameters": [ { @@ -5508,7 +5508,7 @@ "tags": [ "repository" ], - "summary": "delete a topic from a repository", + "summary": "Delete a topic from a repository", "operationId": "repoDeleteTopic", "parameters": [ { From 244cbd39b129822c15f613128c7856f2fae16a8a Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sat, 24 Aug 2019 15:17:55 +0000 Subject: [PATCH 05/29] Fix function comment Signed-off-by: David Svantesson --- routers/api/v1/convert/convert.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/api/v1/convert/convert.go b/routers/api/v1/convert/convert.go index f3e3feee97d39..d9fead841a188 100644 --- a/routers/api/v1/convert/convert.go +++ b/routers/api/v1/convert/convert.go @@ -292,7 +292,7 @@ func ToCommitMeta(repo *models.Repository, tag *git.Tag) *api.CommitMeta { } } -// ToTopic convert from models.Topic to api.TopicResponse +// ToTopicResponse convert from models.Topic to api.TopicResponse func ToTopicResponse(topic *models.Topic) *api.TopicResponse { return &api.TopicResponse{ ID: topic.ID, From f0f49bd42c8eedde0d50a611069f1ba6834d392d Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sat, 24 Aug 2019 19:46:35 +0200 Subject: [PATCH 06/29] Can't use FindTopics when looking for a single repo topic, as it doesnt use exact match Signed-off-by: David Svantesson --- models/topic.go | 28 ++++++++++++++++------------ routers/api/v1/repo/topic.go | 9 +++------ 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/models/topic.go b/models/topic.go index b62b809530d2b..6784853a72f84 100644 --- a/models/topic.go +++ b/models/topic.go @@ -149,18 +149,26 @@ func FindTopics(opts *FindTopicOptions) (topics []*Topic, err error) { return topics, sess.Desc("topic.repo_count").Find(&topics) } +// GetRepoTopicByName retrives topic from name for a repo if it exist +func GetRepoTopicByName(repoID int64, topicName string) (topic *Topic, err error) { + sess := x.Select("topic.*").Where("repo_topic.repo_id = ?", repoID).And("topic.name = ?", topicName) + sess.Join("INNER", "repo_topic", "repo_topic.topic_id = topic.id") + has, err := sess.Get(&topic) + if has { + return topic, err + } + return nil, err +} + // AddTopic adds a topic name to a repository (if it does not already have it) func AddTopic(repoID int64, topicName string) (*Topic, error) { - topics, err := FindTopics(&FindTopicOptions{ - RepoID: repoID, - Keyword: topicName, - }) + topic, err := GetRepoTopicByName(repoID, topicName) if err != nil { return nil, err } - if len(topics) != 0 { + if topic != nil { // Repo already have topic - return topics[0], nil + return topic, nil } return addTopicByNameToRepo(repoID, topicName, x) @@ -168,18 +176,14 @@ func AddTopic(repoID int64, topicName string) (*Topic, error) { // DeleteTopic removes a topic name from a repository (if it has it) func DeleteTopic(repoID int64, topicName string) (*Topic, error) { - topics, err := FindTopics(&FindTopicOptions{ - RepoID: repoID, - Keyword: topicName, - }) + topic, err := GetRepoTopicByName(repoID, topicName) if err != nil { return nil, err } - if len(topics) == 0 { + if topic == nil { // Repo doesn't have topic, can't be removed return nil, nil } - topic := topics[0] err = removeTopicFromRepo(repoID, topic, x) diff --git a/routers/api/v1/repo/topic.go b/routers/api/v1/repo/topic.go index 8aebcc95b4f4f..eca2b981608bc 100644 --- a/routers/api/v1/repo/topic.go +++ b/routers/api/v1/repo/topic.go @@ -87,10 +87,7 @@ func HasTopic(ctx *context.APIContext) { // "$ref": "#/responses/empty" topicName := strings.TrimSpace(strings.ToLower(ctx.Params(":topic"))) - topics, err := models.FindTopics(&models.FindTopicOptions{ - RepoID: ctx.Repo.Repository.ID, - Keyword: topicName, - }) + topic, err := models.GetRepoTopicByName(ctx.Repo.Repository.ID, topicName) if err != nil { log.Error("HasTopic failed: %v", err) ctx.JSON(500, map[string]interface{}{ @@ -99,12 +96,12 @@ func HasTopic(ctx *context.APIContext) { return } - if len(topics) == 0 { + if topic == nil { ctx.NotFound() } ctx.JSON(200, map[string]interface{}{ - "topic": convert.ToTopicResponse(topics[0]), + "topic": convert.ToTopicResponse(topic), }) } From 6ec5a0c14db821ad59c507abcfa5bd28e1c154fd Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sun, 25 Aug 2019 21:29:38 +0200 Subject: [PATCH 07/29] =?UTF-8?q?Add=20PUT=20=E2=80=8B/repos=E2=80=8B/{own?= =?UTF-8?q?er}=E2=80=8B/{repo}=E2=80=8B/topics=20and=20remove=20GET=20?= =?UTF-8?q?=E2=80=8B/repos=E2=80=8B/{owner}=E2=80=8B/{repo}=E2=80=8B/topic?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- models/topic.go | 35 ++++++++++-- modules/structs/repo_topic.go | 6 +++ routers/api/v1/api.go | 10 ++-- routers/api/v1/repo/topic.go | 89 +++++++++++++++++++------------ routers/api/v1/swagger/options.go | 3 ++ routers/repo/topic.go | 15 +----- templates/swagger/v1_json.tmpl | 46 ++++++++++------ 7 files changed, 131 insertions(+), 73 deletions(-) diff --git a/models/topic.go b/models/topic.go index 6784853a72f84..f6ce1c1595443 100644 --- a/models/topic.go +++ b/models/topic.go @@ -54,11 +54,33 @@ func (err ErrTopicNotExist) Error() string { return fmt.Sprintf("topic is not exist [name: %s]", err.Name) } -// ValidateTopic checks topics by length and match pattern rules +// ValidateTopic checks a topic by length and match pattern rules func ValidateTopic(topic string) bool { return len(topic) <= 35 && topicPattern.MatchString(topic) } +// SanitizeAndValidateTopics sanitizes and checks an array or topics +func SanitizeAndValidateTopics(topics []string) (invalidTopics []string) { + invalidTopics = make([]string, 0) + + i := 0 + for _, topic := range topics { + topic = strings.TrimSpace(strings.ToLower(topic)) + // ignore empty string + if len(topic) == 0 { + continue + } + topics[i] = topic + i++ + if !ValidateTopic(topic) { + invalidTopics = append(invalidTopics, topic) + } + } + topics = topics[:i] + + return invalidTopics +} + // GetTopicByName retrieves topic by name func GetTopicByName(name string) (*Topic, error) { var topic Topic @@ -149,13 +171,16 @@ func FindTopics(opts *FindTopicOptions) (topics []*Topic, err error) { return topics, sess.Desc("topic.repo_count").Find(&topics) } -// GetRepoTopicByName retrives topic from name for a repo if it exist -func GetRepoTopicByName(repoID int64, topicName string) (topic *Topic, err error) { - sess := x.Select("topic.*").Where("repo_topic.repo_id = ?", repoID).And("topic.name = ?", topicName) +// GetRepoTopic retrives topic from name for a repo if it exist +func GetRepoTopicByName(repoID int64, topicName string) (*Topic, error) { + var cond = builder.NewCond() + var topic Topic + cond = cond.And(builder.Eq{"repo_topic.repo_id": repoID}).And(builder.Eq{"topic.name": topicName}) + sess := x.Table("topic").Where(cond) sess.Join("INNER", "repo_topic", "repo_topic.topic_id = topic.id") has, err := sess.Get(&topic) if has { - return topic, err + return &topic, err } return nil, err } diff --git a/modules/structs/repo_topic.go b/modules/structs/repo_topic.go index 0ff7a940fe5a7..f3ac9cf28c65b 100644 --- a/modules/structs/repo_topic.go +++ b/modules/structs/repo_topic.go @@ -16,3 +16,9 @@ type TopicResponse struct { Created time.Time `json:"created"` Updated time.Time `json:"updated"` } + +// RepoTopicOptions a collection of repo topic names +type RepoTopicOptions struct { + // list of topic names + Topics []string `json:"topics"` +} diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index abb97fd47ae90..f8b1caef84c91 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -776,10 +776,12 @@ func RegisterRoutes(m *macaron.Macaron) { }, reqRepoWriter(models.UnitTypeCode), reqToken()) }, reqRepoReader(models.UnitTypeCode)) m.Group("/topics", func() { - m.Get("", repo.ListTopics) - m.Combo("/:topic").Get(repo.HasTopic). - Put(reqToken(), reqRepoWriter(models.UnitTypeCode), repo.AddTopic). - Delete(reqToken(), reqRepoWriter(models.UnitTypeCode), repo.DeleteTopic) + m.Combo("").Get(repo.ListTopics). + Put(reqToken(), reqRepoWriter(models.UnitTypeCode), bind(api.RepoTopicOptions{}), repo.UpdateTopics) + m.Group("/:topic", func() { + m.Combo("").Put(reqToken(), reqRepoWriter(models.UnitTypeCode), repo.AddTopic). + Delete(reqToken(), reqRepoWriter(models.UnitTypeCode), repo.DeleteTopic) + }) }, reqRepoReader(models.UnitTypeCode)) }, repoAssignment()) }) diff --git a/routers/api/v1/repo/topic.go b/routers/api/v1/repo/topic.go index eca2b981608bc..c2aca2bdd8bb4 100644 --- a/routers/api/v1/repo/topic.go +++ b/routers/api/v1/repo/topic.go @@ -48,20 +48,20 @@ func ListTopics(ctx *context.APIContext) { return } - topicResponses := make([]*api.TopicResponse, len(topics)) + topicNames := make([]*string, len(topics)) for i, topic := range topics { - topicResponses[i] = convert.ToTopicResponse(topic) + topicNames[i] = &topic.Name } ctx.JSON(200, map[string]interface{}{ - "topics": topicResponses, + "topics": topicNames, }) } -// HasTopic check if repo has topic name -func HasTopic(ctx *context.APIContext) { - // swagger:operation GET /repos/{owner}/{repo}/topics/{topic} repository repoHasTopic +// UpdateTopics updates repo with a new set of topics +func UpdateTopics(ctx *context.APIContext, form api.RepoTopicOptions) { + // swagger:operation PUT /repos/{owner}/{repo}/topics repository repoUpdateTopics // --- - // summary: Check if a repository has topic + // summary: Replace list of topics for a repository // produces: // - application/json // parameters: @@ -75,34 +75,43 @@ func HasTopic(ctx *context.APIContext) { // description: name of the repo // type: string // required: true - // - name: topic - // in: path - // description: name of the topic to check for - // type: string - // required: true + // - name: body + // in: body + // schema: + // "$ref": "#/definitions/RepoTopicOptions" // responses: - // "201": - // "$ref": "#/responses/TopicResponse" - // "404": + // "200": // "$ref": "#/responses/empty" - topicName := strings.TrimSpace(strings.ToLower(ctx.Params(":topic"))) - topic, err := models.GetRepoTopicByName(ctx.Repo.Repository.ID, topicName) - if err != nil { - log.Error("HasTopic failed: %v", err) - ctx.JSON(500, map[string]interface{}{ - "message": "HasTopic failed.", + topicNames := form.Topics + invalidTopics := models.SanitizeAndValidateTopics(topicNames) + + if len(topicNames) > 25 { + ctx.JSON(422, map[string]interface{}{ + "invalidTopics": topicNames[:0], + "message": "Exceeding maximum number of topics per repo", }) return } - if topic == nil { - ctx.NotFound() + if len(invalidTopics) > 0 { + ctx.JSON(422, map[string]interface{}{ + "invalidTopics": invalidTopics, + "message": "Topic names are invalid", + }) + return } - ctx.JSON(200, map[string]interface{}{ - "topic": convert.ToTopicResponse(topic), - }) + err := models.SaveTopics(ctx.Repo.Repository.ID, topicNames...) + if err != nil { + log.Error("SaveTopics failed: %v", err) + ctx.JSON(500, map[string]interface{}{ + "message": "Save topics failed.", + }) + return + } + + ctx.Status(204) } // AddTopic adds a topic name to a repo @@ -139,7 +148,25 @@ func AddTopic(ctx *context.APIContext) { return } - topic, err := models.AddTopic(ctx.Repo.Repository.ID, topicName) + // Prevent adding more topics than allowed to repo + topics, err := models.FindTopics(&models.FindTopicOptions{ + RepoID: ctx.Repo.Repository.ID, + }) + if err != nil { + log.Error("AddTopic failed: %v", err) + ctx.JSON(500, map[string]interface{}{ + "message": "ListTopics failed.", + }) + return + } + if len(topics) >= 25 { + ctx.JSON(422, map[string]interface{}{ + "message": "Exceeding maximum allowed topics per repo.", + }) + return + } + + _, err = models.AddTopic(ctx.Repo.Repository.ID, topicName) if err != nil { log.Error("AddTopic failed: %v", err) ctx.JSON(500, map[string]interface{}{ @@ -148,9 +175,7 @@ func AddTopic(ctx *context.APIContext) { return } - ctx.JSON(201, map[string]interface{}{ - "topic": convert.ToTopicResponse(topic), - }) + ctx.Status(204) } // DeleteTopic removes topic name from repo @@ -199,9 +224,7 @@ func DeleteTopic(ctx *context.APIContext) { ctx.NotFound() } - ctx.JSON(201, map[string]interface{}{ - "topic": convert.ToTopicResponse(topic), - }) + ctx.Status(204) } // TopicSearch search for creating topic diff --git a/routers/api/v1/swagger/options.go b/routers/api/v1/swagger/options.go index c1196eeb71581..80e4bf422a93c 100644 --- a/routers/api/v1/swagger/options.go +++ b/routers/api/v1/swagger/options.go @@ -117,4 +117,7 @@ type swaggerParameterBodies struct { // in:body DeleteFileOptions api.DeleteFileOptions + + // in:body + RepoTopicOptions api.RepoTopicOptions } diff --git a/routers/repo/topic.go b/routers/repo/topic.go index 4a1194bc2dceb..ce65667f7604f 100644 --- a/routers/repo/topic.go +++ b/routers/repo/topic.go @@ -27,20 +27,7 @@ func TopicsPost(ctx *context.Context) { topics = strings.Split(topicsStr, ",") } - invalidTopics := make([]string, 0) - i := 0 - for _, topic := range topics { - topic = strings.TrimSpace(strings.ToLower(topic)) - // ignore empty string - if len(topic) > 0 { - topics[i] = topic - i++ - } - if !models.ValidateTopic(topic) { - invalidTopics = append(invalidTopics, topic) - } - } - topics = topics[:i] + invalidTopics := models.SanitizeAndValidateTopics(topics) if len(topics) > 25 { ctx.JSON(422, map[string]interface{}{ diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 062ff36caec82..7b66a3718c7d6 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -5425,18 +5425,16 @@ "$ref": "#/responses/TopicListResponse" } } - } - }, - "/repos/{owner}/{repo}/topics/{topic}": { - "get": { + }, + "put": { "produces": [ "application/json" ], "tags": [ "repository" ], - "summary": "Check if a repository has topic", - "operationId": "repoHasTopic", + "summary": "Replace list of topics for a repository", + "operationId": "repoUpdateTopics", "parameters": [ { "type": "string", @@ -5453,22 +5451,21 @@ "required": true }, { - "type": "string", - "description": "name of the topic to check for", - "name": "topic", - "in": "path", - "required": true + "name": "body", + "in": "body", + "schema": { + "$ref": "#/definitions/RepoTopicOptions" + } } ], "responses": { - "201": { - "$ref": "#/responses/TopicResponse" - }, - "404": { + "200": { "$ref": "#/responses/empty" } } - }, + } + }, + "/repos/{owner}/{repo}/topics/{topic}": { "put": { "produces": [ "application/json" @@ -9617,6 +9614,21 @@ }, "x-go-package": "code.gitea.io/gitea/modules/structs" }, + "RepoTopicOptions": { + "description": "RepoTopicOptions a collection of repo topic names", + "type": "object", + "properties": { + "topics": { + "description": "list of topic names", + "type": "array", + "items": { + "type": "string" + }, + "x-go-name": "Topics" + } + }, + "x-go-package": "code.gitea.io/gitea/modules/structs" + }, "Repository": { "description": "Repository represents a repository", "type": "object", @@ -10621,7 +10633,7 @@ "parameterBodies": { "description": "parameterBodies", "schema": { - "$ref": "#/definitions/DeleteFileOptions" + "$ref": "#/definitions/RepoTopicOptions" } }, "redirect": { From 8daf6412831b6c384507091d1659898a36dcb11b Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sun, 25 Aug 2019 23:34:35 +0200 Subject: [PATCH 08/29] Ignore if topic is sent twice in same request, refactoring. Signed-off-by: David Svantesson --- models/topic.go | 22 ++++++++++++++-------- routers/api/v1/repo/topic.go | 8 ++++---- routers/repo/topic.go | 8 ++++---- 3 files changed, 22 insertions(+), 16 deletions(-) diff --git a/models/topic.go b/models/topic.go index f6ce1c1595443..2aecbdc9bd7fd 100644 --- a/models/topic.go +++ b/models/topic.go @@ -60,25 +60,31 @@ func ValidateTopic(topic string) bool { } // SanitizeAndValidateTopics sanitizes and checks an array or topics -func SanitizeAndValidateTopics(topics []string) (invalidTopics []string) { +func SanitizeAndValidateTopics(topics []string) (validTopics []string, invalidTopics []string) { + validTopics = make([]string, 0) invalidTopics = make([]string, 0) - i := 0 + LOOP_TOPICS: for _, topic := range topics { topic = strings.TrimSpace(strings.ToLower(topic)) // ignore empty string if len(topic) == 0 { - continue + continue LOOP_TOPICS + } + // ignore same topic twice + for _, vTopic := range validTopics { + if topic == vTopic { + continue LOOP_TOPICS + } } - topics[i] = topic - i++ - if !ValidateTopic(topic) { + if ValidateTopic(topic) { + validTopics = append(validTopics, topic) + } else { invalidTopics = append(invalidTopics, topic) } } - topics = topics[:i] - return invalidTopics + return validTopics, invalidTopics } // GetTopicByName retrieves topic by name diff --git a/routers/api/v1/repo/topic.go b/routers/api/v1/repo/topic.go index c2aca2bdd8bb4..5916e530ef43a 100644 --- a/routers/api/v1/repo/topic.go +++ b/routers/api/v1/repo/topic.go @@ -84,11 +84,11 @@ func UpdateTopics(ctx *context.APIContext, form api.RepoTopicOptions) { // "$ref": "#/responses/empty" topicNames := form.Topics - invalidTopics := models.SanitizeAndValidateTopics(topicNames) + validTopics, invalidTopics := models.SanitizeAndValidateTopics(topicNames) - if len(topicNames) > 25 { + if len(validTopics) > 25 { ctx.JSON(422, map[string]interface{}{ - "invalidTopics": topicNames[:0], + "invalidTopics": nil, "message": "Exceeding maximum number of topics per repo", }) return @@ -102,7 +102,7 @@ func UpdateTopics(ctx *context.APIContext, form api.RepoTopicOptions) { return } - err := models.SaveTopics(ctx.Repo.Repository.ID, topicNames...) + err := models.SaveTopics(ctx.Repo.Repository.ID, validTopics...) if err != nil { log.Error("SaveTopics failed: %v", err) ctx.JSON(500, map[string]interface{}{ diff --git a/routers/repo/topic.go b/routers/repo/topic.go index ce65667f7604f..b23023ceba0a1 100644 --- a/routers/repo/topic.go +++ b/routers/repo/topic.go @@ -27,11 +27,11 @@ func TopicsPost(ctx *context.Context) { topics = strings.Split(topicsStr, ",") } - invalidTopics := models.SanitizeAndValidateTopics(topics) + validTopics, invalidTopics := models.SanitizeAndValidateTopics(topics) - if len(topics) > 25 { + if len(validTopics) > 25 { ctx.JSON(422, map[string]interface{}{ - "invalidTopics": topics[:0], + "invalidTopics": nil, "message": ctx.Tr("repo.topic.count_prompt"), }) return @@ -45,7 +45,7 @@ func TopicsPost(ctx *context.Context) { return } - err := models.SaveTopics(ctx.Repo.Repository.ID, topics...) + err := models.SaveTopics(ctx.Repo.Repository.ID, validTopics...) if err != nil { log.Error("SaveTopics failed: %v", err) ctx.JSON(500, map[string]interface{}{ From 9fdab250981df812642cfccd0dfc630f4f85b660 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Mon, 26 Aug 2019 00:45:10 +0200 Subject: [PATCH 09/29] Fix topic dropdown with api changes. Signed-off-by: David Svantesson --- public/js/index.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/public/js/index.js b/public/js/index.js index 15f8d02bbdfd6..882f19e13dbf7 100644 --- a/public/js/index.js +++ b/public/js/index.js @@ -2936,14 +2936,14 @@ function initTopicbar() { let found = false; for (let i=0;i < res.topics.length;i++) { // skip currently added tags - if (current_topics.indexOf(res.topics[i].Name) != -1){ + if (current_topics.indexOf(res.topics[i].topic_name) != -1){ continue; } - if (res.topics[i].Name.toLowerCase() === query.toLowerCase()){ + if (res.topics[i].topic_name.toLowerCase() === query.toLowerCase()){ found_query = true; } - formattedResponse.results.push({"description": res.topics[i].Name, "data-value": res.topics[i].Name}); + formattedResponse.results.push({"description": res.topics[i].topic_name, "data-value": res.topics[i].topic_name}); found = true; } formattedResponse.success = found; From 1cb206c9f3d97a2e80fc107325f0ae71c720cbb3 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sun, 25 Aug 2019 22:53:16 +0000 Subject: [PATCH 10/29] Style fix Signed-off-by: David Svantesson --- models/topic.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/topic.go b/models/topic.go index 2aecbdc9bd7fd..003af4295c949 100644 --- a/models/topic.go +++ b/models/topic.go @@ -64,7 +64,7 @@ func SanitizeAndValidateTopics(topics []string) (validTopics []string, invalidTo validTopics = make([]string, 0) invalidTopics = make([]string, 0) - LOOP_TOPICS: +LOOP_TOPICS: for _, topic := range topics { topic = strings.TrimSpace(strings.ToLower(topic)) // ignore empty string From c13a297353a0afd40fcace973bdd4541e304a1e3 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sun, 25 Aug 2019 23:14:40 +0000 Subject: [PATCH 11/29] Update API documentation Signed-off-by: David Svantesson --- modules/structs/repo_topic.go | 4 ++++ routers/api/v1/repo/topic.go | 12 ++++++------ routers/api/v1/swagger/repo.go | 14 +++++++------- templates/swagger/v1_json.tmpl | 31 ++++++++++++++++++++++--------- 4 files changed, 39 insertions(+), 22 deletions(-) diff --git a/modules/structs/repo_topic.go b/modules/structs/repo_topic.go index f3ac9cf28c65b..5b897c612d3e3 100644 --- a/modules/structs/repo_topic.go +++ b/modules/structs/repo_topic.go @@ -17,6 +17,10 @@ type TopicResponse struct { Updated time.Time `json:"updated"` } +type TopicName struct { + TopicNames string `json:"topics"` +} + // RepoTopicOptions a collection of repo topic names type RepoTopicOptions struct { // list of topic names diff --git a/routers/api/v1/repo/topic.go b/routers/api/v1/repo/topic.go index 5916e530ef43a..b204d78cfce5d 100644 --- a/routers/api/v1/repo/topic.go +++ b/routers/api/v1/repo/topic.go @@ -35,7 +35,7 @@ func ListTopics(ctx *context.APIContext) { // required: true // responses: // "200": - // "$ref": "#/responses/TopicListResponse" + // "$ref": "#/responses/TopicNames" topics, err := models.FindTopics(&models.FindTopicOptions{ RepoID: ctx.Repo.Repository.ID, @@ -80,7 +80,7 @@ func UpdateTopics(ctx *context.APIContext, form api.RepoTopicOptions) { // schema: // "$ref": "#/definitions/RepoTopicOptions" // responses: - // "200": + // "204": // "$ref": "#/responses/empty" topicNames := form.Topics @@ -138,8 +138,8 @@ func AddTopic(ctx *context.APIContext) { // type: string // required: true // responses: - // "201": - // "$ref": "#/responses/TopicResponse" + // "204": + // "$ref": "#/responses/empty" topicName := strings.TrimSpace(strings.ToLower(ctx.Params(":topic"))) @@ -202,8 +202,8 @@ func DeleteTopic(ctx *context.APIContext) { // type: string // required: true // responses: - // "201": - // "$ref": "#/responses/TopicResponse" + // "204": + // "$ref": "#/responses/empty" topicName := strings.TrimSpace(strings.ToLower(ctx.Params(":topic"))) if !models.ValidateTopic(topicName) { diff --git a/routers/api/v1/swagger/repo.go b/routers/api/v1/swagger/repo.go index 863aecfbe0a1d..1047ad27e30d4 100644 --- a/routers/api/v1/swagger/repo.go +++ b/routers/api/v1/swagger/repo.go @@ -218,16 +218,16 @@ type swaggerFileDeleteResponse struct { Body api.FileDeleteResponse `json:"body"` } -// TopicResponse -// swagger:response TopicResponse -type swaggerTopicResponse struct { - //in: body - Body api.TopicResponse `json:"body"` -} - // TopicListResponse // swagger:response TopicListResponse type swaggerTopicListResponse struct { //in: body Body []api.TopicResponse `json:"body"` } + +// TopicNames +// swagger:response TopicNames +type swaggerTopicNames struct { + //in: body + Body []api.TopicName `json:"body"` +} diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 7b66a3718c7d6..f8a04c3de58ea 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -5422,7 +5422,7 @@ ], "responses": { "200": { - "$ref": "#/responses/TopicListResponse" + "$ref": "#/responses/TopicNames" } } }, @@ -5459,7 +5459,7 @@ } ], "responses": { - "200": { + "204": { "$ref": "#/responses/empty" } } @@ -5499,8 +5499,8 @@ } ], "responses": { - "201": { - "$ref": "#/responses/TopicResponse" + "204": { + "$ref": "#/responses/empty" } } }, @@ -5537,8 +5537,8 @@ } ], "responses": { - "201": { - "$ref": "#/responses/TopicResponse" + "204": { + "$ref": "#/responses/empty" } } } @@ -9942,6 +9942,16 @@ "format": "int64", "x-go-package": "code.gitea.io/gitea/modules/timeutil" }, + "TopicName": { + "type": "object", + "properties": { + "topics": { + "type": "string", + "x-go-name": "TopicNames" + } + }, + "x-go-package": "code.gitea.io/gitea/modules/structs" + }, "TopicResponse": { "description": "TopicResponse for returning topics", "type": "object", @@ -10551,10 +10561,13 @@ } } }, - "TopicResponse": { - "description": "TopicResponse", + "TopicNames": { + "description": "TopicNames", "schema": { - "$ref": "#/definitions/TopicResponse" + "type": "array", + "items": { + "$ref": "#/definitions/TopicName" + } } }, "TrackedTime": { From 4a536818163493469a67eae363d36a9c8b86ff08 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Mon, 26 Aug 2019 06:53:34 +0000 Subject: [PATCH 12/29] Better way to handle duplicate topics in slice Signed-off-by: David Svantesson --- models/topic.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/models/topic.go b/models/topic.go index 003af4295c949..ff89394bec325 100644 --- a/models/topic.go +++ b/models/topic.go @@ -62,23 +62,22 @@ func ValidateTopic(topic string) bool { // SanitizeAndValidateTopics sanitizes and checks an array or topics func SanitizeAndValidateTopics(topics []string) (validTopics []string, invalidTopics []string) { validTopics = make([]string, 0) + mValidTopics := make(map[string]bool) invalidTopics = make([]string, 0) -LOOP_TOPICS: for _, topic := range topics { topic = strings.TrimSpace(strings.ToLower(topic)) // ignore empty string if len(topic) == 0 { - continue LOOP_TOPICS + continue } // ignore same topic twice - for _, vTopic := range validTopics { - if topic == vTopic { - continue LOOP_TOPICS - } + if _, exist := mValidTopics[topic]; exist { + continue } if ValidateTopic(topic) { validTopics = append(validTopics, topic) + mValidTopics[topic] = true } else { invalidTopics = append(invalidTopics, topic) } From 3705219585ab4ddca86fa0e7464a1ea4a27d7f2b Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Mon, 26 Aug 2019 22:56:32 +0200 Subject: [PATCH 13/29] Make response element TopicName an array of strings, instead of using an array of TopicName Signed-off-by: David Svantesson --- modules/structs/repo_topic.go | 2 +- routers/api/v1/swagger/repo.go | 2 +- templates/swagger/v1_json.tmpl | 10 +++++----- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/modules/structs/repo_topic.go b/modules/structs/repo_topic.go index 5b897c612d3e3..48f3a50df432e 100644 --- a/modules/structs/repo_topic.go +++ b/modules/structs/repo_topic.go @@ -18,7 +18,7 @@ type TopicResponse struct { } type TopicName struct { - TopicNames string `json:"topics"` + TopicNames []string `json:"topics"` } // RepoTopicOptions a collection of repo topic names diff --git a/routers/api/v1/swagger/repo.go b/routers/api/v1/swagger/repo.go index 1047ad27e30d4..bc8a1861840d2 100644 --- a/routers/api/v1/swagger/repo.go +++ b/routers/api/v1/swagger/repo.go @@ -229,5 +229,5 @@ type swaggerTopicListResponse struct { // swagger:response TopicNames type swaggerTopicNames struct { //in: body - Body []api.TopicName `json:"body"` + Body api.TopicName `json:"body"` } diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index f8a04c3de58ea..55b617350fd1b 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -9946,7 +9946,10 @@ "type": "object", "properties": { "topics": { - "type": "string", + "type": "array", + "items": { + "type": "string" + }, "x-go-name": "TopicNames" } }, @@ -10564,10 +10567,7 @@ "TopicNames": { "description": "TopicNames", "schema": { - "type": "array", - "items": { - "$ref": "#/definitions/TopicName" - } + "$ref": "#/definitions/TopicName" } }, "TrackedTime": { From ac936771105ba1e2cbd4bd2412e806220b8fe602 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Mon, 26 Aug 2019 23:01:12 +0200 Subject: [PATCH 14/29] Add test cases for API Repo Topics. Signed-off-by: David Svantesson --- integrations/api_repo_topic_test.go | 76 +++++++++++++++++++++++++++++ models/fixtures/repo_topic.yml | 8 +++ models/fixtures/topic.yml | 8 +++ 3 files changed, 92 insertions(+) create mode 100644 integrations/api_repo_topic_test.go diff --git a/integrations/api_repo_topic_test.go b/integrations/api_repo_topic_test.go new file mode 100644 index 0000000000000..e570d0cfc292c --- /dev/null +++ b/integrations/api_repo_topic_test.go @@ -0,0 +1,76 @@ +// Copyright 2019 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package integrations + +import ( + "fmt" + "net/http" + "testing" + + "code.gitea.io/gitea/models" + api "code.gitea.io/gitea/modules/structs" + + "github.com/stretchr/testify/assert" +) + +func TestAPIRepoTopic(t *testing.T) { + prepareTestEnv(t) + user2 := models.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User) // owner of repo1 + repo2 := models.AssertExistsAndLoadBean(t, &models.Repository{ID: 2}).(*models.Repository) + + // Get user2's token + session := loginUser(t, user2.Name) + token2 := getTokenForLoggedInUser(t, session) + session = emptyTestSession(t) + + url := fmt.Sprintf("/api/v1/repos/%s/%s/topics?token=%s", user2.Name, repo2.Name, token2) + + // Test read topics + req := NewRequest(t, "GET", url) + res := session.MakeRequest(t, req, http.StatusOK) + var topics *api.TopicName + DecodeJSON(t, res, &topics) + assert.ElementsMatch(t, []string{"topicname1","topicname2"}, topics.TopicNames) + + // Test delete a to7ic + req = NewRequestf(t, "DELETE", "/api/v1/repos/%s/%s/topics/%s?token=%s", user2.Name, repo2.Name, "Topicname1", token2) + res = session.MakeRequest(t, req, http.StatusNoContent) + + // Test add an existing topic + req = NewRequestf(t, "PUT", "/api/v1/repos/%s/%s/topics/%s?token=%s", user2.Name, repo2.Name, "Golang", token2) + res = session.MakeRequest(t, req, http.StatusNoContent) + + // Test add a topic + req = NewRequestf(t, "PUT", "/api/v1/repos/%s/%s/topics/%s?token=%s", user2.Name, repo2.Name, "topicName3", token2) + res = session.MakeRequest(t, req, http.StatusNoContent) + + // Read topics + req = NewRequest(t, "GET", url) + res = session.MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, res, &topics) + assert.ElementsMatch(t, []string{"topicname2","golang","topicname3"}, topics.TopicNames) + + // Test replace topics + newTopics := []string{" windows ", " ", "MAC "} + req = NewRequestWithJSON(t, "PUT", url, &api.RepoTopicOptions{ + Topics: newTopics, + }) + res = session.MakeRequest(t, req, http.StatusNoContent) + req = NewRequest(t, "GET", url) + res = session.MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, res, &topics) + assert.ElementsMatch(t, []string{"windows","mac"}, topics.TopicNames) + + // Test replace topics with something invalid + newTopics = []string{"topicname1", "topicname2", "topicname!"} + req = NewRequestWithJSON(t, "PUT", url, &api.RepoTopicOptions{ + Topics: newTopics, + }) + res = session.MakeRequest(t, req, http.StatusUnprocessableEntity) + req = NewRequest(t, "GET", url) + res = session.MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, res, &topics) + assert.ElementsMatch(t, []string{"windows","mac"}, topics.TopicNames) +} diff --git a/models/fixtures/repo_topic.yml b/models/fixtures/repo_topic.yml index 7041ccfd09446..f166faccc1d8f 100644 --- a/models/fixtures/repo_topic.yml +++ b/models/fixtures/repo_topic.yml @@ -17,3 +17,11 @@ - repo_id: 33 topic_id: 4 + +- + repo_id: 2 + topic_id: 5 + +- + repo_id: 2 + topic_id: 6 diff --git a/models/fixtures/topic.yml b/models/fixtures/topic.yml index c868b207cb143..6cd0b37fa1721 100644 --- a/models/fixtures/topic.yml +++ b/models/fixtures/topic.yml @@ -15,3 +15,11 @@ - id: 4 name: graphql repo_count: 1 + +- id: 5 + name: topicname1 + repo_count: 1 + +- id: 6 + name: topicname2 + repo_count: 2 From 8bc836c24b9c0e58b29ca8c002fa095035e4a59f Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Mon, 26 Aug 2019 21:18:57 +0000 Subject: [PATCH 15/29] Fix format of tests Signed-off-by: David Svantesson --- integrations/api_repo_topic_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/integrations/api_repo_topic_test.go b/integrations/api_repo_topic_test.go index e570d0cfc292c..32db882c70daf 100644 --- a/integrations/api_repo_topic_test.go +++ b/integrations/api_repo_topic_test.go @@ -17,7 +17,7 @@ import ( func TestAPIRepoTopic(t *testing.T) { prepareTestEnv(t) - user2 := models.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User) // owner of repo1 + user2 := models.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User) // owner of repo1 repo2 := models.AssertExistsAndLoadBean(t, &models.Repository{ID: 2}).(*models.Repository) // Get user2's token @@ -32,14 +32,14 @@ func TestAPIRepoTopic(t *testing.T) { res := session.MakeRequest(t, req, http.StatusOK) var topics *api.TopicName DecodeJSON(t, res, &topics) - assert.ElementsMatch(t, []string{"topicname1","topicname2"}, topics.TopicNames) + assert.ElementsMatch(t, []string{"topicname1", "topicname2"}, topics.TopicNames) // Test delete a to7ic req = NewRequestf(t, "DELETE", "/api/v1/repos/%s/%s/topics/%s?token=%s", user2.Name, repo2.Name, "Topicname1", token2) res = session.MakeRequest(t, req, http.StatusNoContent) // Test add an existing topic - req = NewRequestf(t, "PUT", "/api/v1/repos/%s/%s/topics/%s?token=%s", user2.Name, repo2.Name, "Golang", token2) + req = NewRequestf(t, "PUT", "/api/v1/repos/%s/%s/topics/%s?token=%s", user2.Name, repo2.Name, "Golang", token2) res = session.MakeRequest(t, req, http.StatusNoContent) // Test add a topic @@ -50,7 +50,7 @@ func TestAPIRepoTopic(t *testing.T) { req = NewRequest(t, "GET", url) res = session.MakeRequest(t, req, http.StatusOK) DecodeJSON(t, res, &topics) - assert.ElementsMatch(t, []string{"topicname2","golang","topicname3"}, topics.TopicNames) + assert.ElementsMatch(t, []string{"topicname2", "golang", "topicname3"}, topics.TopicNames) // Test replace topics newTopics := []string{" windows ", " ", "MAC "} @@ -61,7 +61,7 @@ func TestAPIRepoTopic(t *testing.T) { req = NewRequest(t, "GET", url) res = session.MakeRequest(t, req, http.StatusOK) DecodeJSON(t, res, &topics) - assert.ElementsMatch(t, []string{"windows","mac"}, topics.TopicNames) + assert.ElementsMatch(t, []string{"windows", "mac"}, topics.TopicNames) // Test replace topics with something invalid newTopics = []string{"topicname1", "topicname2", "topicname!"} @@ -72,5 +72,5 @@ func TestAPIRepoTopic(t *testing.T) { req = NewRequest(t, "GET", url) res = session.MakeRequest(t, req, http.StatusOK) DecodeJSON(t, res, &topics) - assert.ElementsMatch(t, []string{"windows","mac"}, topics.TopicNames) + assert.ElementsMatch(t, []string{"windows", "mac"}, topics.TopicNames) } From 3af43e2e31e22b1893d8b9fee2be9f4b57f98926 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Mon, 26 Aug 2019 23:28:43 +0200 Subject: [PATCH 16/29] Fix comments Signed-off-by: David Svantesson --- models/topic.go | 2 +- modules/structs/repo_topic.go | 1 + templates/swagger/v1_json.tmpl | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/models/topic.go b/models/topic.go index ff89394bec325..38b74d35564ff 100644 --- a/models/topic.go +++ b/models/topic.go @@ -176,7 +176,7 @@ func FindTopics(opts *FindTopicOptions) (topics []*Topic, err error) { return topics, sess.Desc("topic.repo_count").Find(&topics) } -// GetRepoTopic retrives topic from name for a repo if it exist +// GetRepoTopicByName retrives topic from name for a repo if it exist func GetRepoTopicByName(repoID int64, topicName string) (*Topic, error) { var cond = builder.NewCond() var topic Topic diff --git a/modules/structs/repo_topic.go b/modules/structs/repo_topic.go index 48f3a50df432e..294d56a953d06 100644 --- a/modules/structs/repo_topic.go +++ b/modules/structs/repo_topic.go @@ -17,6 +17,7 @@ type TopicResponse struct { Updated time.Time `json:"updated"` } +// TopicName a list of repo topic names type TopicName struct { TopicNames []string `json:"topics"` } diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 55b617350fd1b..b88597016737a 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -9943,6 +9943,7 @@ "x-go-package": "code.gitea.io/gitea/modules/timeutil" }, "TopicName": { + "description": "TopicName a list of repo topic names", "type": "object", "properties": { "topics": { From 5cdbbbc1cb326cda828b43bf4e8c40fefcd77d66 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Tue, 27 Aug 2019 00:07:25 +0200 Subject: [PATCH 17/29] Fix unit tests after adding some more topics to the test fixture. Signed-off-by: David Svantesson --- models/topic_test.go | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/models/topic_test.go b/models/topic_test.go index 65e52afb12f6c..c173c7bf2a575 100644 --- a/models/topic_test.go +++ b/models/topic_test.go @@ -11,11 +11,15 @@ import ( ) func TestAddTopic(t *testing.T) { + totalNrOfTopics := 6 + repo1NrOfTopics := 3 + repo2NrOfTopics := 2 + assert.NoError(t, PrepareTestDatabase()) topics, err := FindTopics(&FindTopicOptions{}) assert.NoError(t, err) - assert.EqualValues(t, 4, len(topics)) + assert.EqualValues(t, totalNrOfTopics, len(topics)) topics, err = FindTopics(&FindTopicOptions{ Limit: 2, @@ -27,33 +31,36 @@ func TestAddTopic(t *testing.T) { RepoID: 1, }) assert.NoError(t, err) - assert.EqualValues(t, 3, len(topics)) + assert.EqualValues(t, repo1NrOfTopics, len(topics)) assert.NoError(t, SaveTopics(2, "golang")) + repo2NrOfTopics = 1 topics, err = FindTopics(&FindTopicOptions{}) assert.NoError(t, err) - assert.EqualValues(t, 4, len(topics)) + assert.EqualValues(t, totalNrOfTopics, len(topics)) topics, err = FindTopics(&FindTopicOptions{ RepoID: 2, }) assert.NoError(t, err) - assert.EqualValues(t, 1, len(topics)) + assert.EqualValues(t, repo2NrOfTopics, len(topics)) assert.NoError(t, SaveTopics(2, "golang", "gitea")) + repo2NrOfTopics = 2 + totalNrOfTopics++ topic, err := GetTopicByName("gitea") assert.NoError(t, err) assert.EqualValues(t, 1, topic.RepoCount) topics, err = FindTopics(&FindTopicOptions{}) assert.NoError(t, err) - assert.EqualValues(t, 5, len(topics)) + assert.EqualValues(t, totalNrOfTopics, len(topics)) topics, err = FindTopics(&FindTopicOptions{ RepoID: 2, }) assert.NoError(t, err) - assert.EqualValues(t, 2, len(topics)) + assert.EqualValues(t, repo2NrOfTopics, len(topics)) } func TestTopicValidator(t *testing.T) { From db3f6f3f148cc2ce1c1e69aeba8af3707bbe65d6 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Tue, 27 Aug 2019 18:34:33 +0200 Subject: [PATCH 18/29] Update models/topic.go Limit multiple if else if ... Co-Authored-By: Antoine GIRARD --- models/topic.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/models/topic.go b/models/topic.go index 38b74d35564ff..45368cc60d120 100644 --- a/models/topic.go +++ b/models/topic.go @@ -103,7 +103,8 @@ func addTopicByNameToRepo(repoID int64, topicName string, e Engine) (*Topic, err var topic Topic if has, err := e.Where("name = ?", topicName).Get(&topic); err != nil { return nil, err - } else if !has { + } + if !has { topic.Name = topicName topic.RepoCount = 1 if _, err := e.Insert(&topic); err != nil { From 2ef6904dd764a847dd7776075420737c4cc0ffc6 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Tue, 27 Aug 2019 18:41:06 +0200 Subject: [PATCH 19/29] Engine as first parameter in function Co-Authored-By: Antoine GIRARD --- models/topic.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/models/topic.go b/models/topic.go index 45368cc60d120..314c699e49887 100644 --- a/models/topic.go +++ b/models/topic.go @@ -99,7 +99,7 @@ func GetTopicByName(name string) (*Topic, error) { // addTopicByNameToRepo adds a topic name to a repo and increments the topic count. // Returns topic after the addition -func addTopicByNameToRepo(repoID int64, topicName string, e Engine) (*Topic, error) { +func addTopicByNameToRepo(e Engine, repoID int64, topicName string) (*Topic, error) { var topic Topic if has, err := e.Where("name = ?", topicName).Get(&topic); err != nil { return nil, err @@ -202,7 +202,7 @@ func AddTopic(repoID int64, topicName string) (*Topic, error) { return topic, nil } - return addTopicByNameToRepo(repoID, topicName, x) + return addTopicByNameToRepo(x, repoID, topicName) } // DeleteTopic removes a topic name from a repository (if it has it) @@ -270,7 +270,7 @@ func SaveTopics(repoID int64, topicNames ...string) error { } for _, topicName := range addedTopicNames { - _, err := addTopicByNameToRepo(repoID, topicName, sess) + _, err := addTopicByNameToRepo(sess, repoID, topicName) if err != nil { return err } From 99eb4792b42dd7678118de31c43142fb1a204f83 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Tue, 27 Aug 2019 20:19:48 +0200 Subject: [PATCH 20/29] Replace magic numbers with http status code constants. Signed-off-by: David Svantesson --- routers/api/v1/repo/topic.go | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/routers/api/v1/repo/topic.go b/routers/api/v1/repo/topic.go index b204d78cfce5d..4d258dfcddc68 100644 --- a/routers/api/v1/repo/topic.go +++ b/routers/api/v1/repo/topic.go @@ -42,7 +42,7 @@ func ListTopics(ctx *context.APIContext) { }) if err != nil { log.Error("ListTopics failed: %v", err) - ctx.JSON(500, map[string]interface{}{ + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ "message": "ListTopics failed.", }) return @@ -52,7 +52,7 @@ func ListTopics(ctx *context.APIContext) { for i, topic := range topics { topicNames[i] = &topic.Name } - ctx.JSON(200, map[string]interface{}{ + ctx.JSON(http.StatusOK, map[string]interface{}{ "topics": topicNames, }) } @@ -87,7 +87,7 @@ func UpdateTopics(ctx *context.APIContext, form api.RepoTopicOptions) { validTopics, invalidTopics := models.SanitizeAndValidateTopics(topicNames) if len(validTopics) > 25 { - ctx.JSON(422, map[string]interface{}{ + ctx.JSON(http.StatusUnprocessableEntity, map[string]interface{}{ "invalidTopics": nil, "message": "Exceeding maximum number of topics per repo", }) @@ -95,7 +95,7 @@ func UpdateTopics(ctx *context.APIContext, form api.RepoTopicOptions) { } if len(invalidTopics) > 0 { - ctx.JSON(422, map[string]interface{}{ + ctx.JSON(http.StatusUnprocessableEntity, map[string]interface{}{ "invalidTopics": invalidTopics, "message": "Topic names are invalid", }) @@ -105,13 +105,13 @@ func UpdateTopics(ctx *context.APIContext, form api.RepoTopicOptions) { err := models.SaveTopics(ctx.Repo.Repository.ID, validTopics...) if err != nil { log.Error("SaveTopics failed: %v", err) - ctx.JSON(500, map[string]interface{}{ + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ "message": "Save topics failed.", }) return } - ctx.Status(204) + ctx.Status(http.StatusNoContent) } // AddTopic adds a topic name to a repo @@ -154,13 +154,13 @@ func AddTopic(ctx *context.APIContext) { }) if err != nil { log.Error("AddTopic failed: %v", err) - ctx.JSON(500, map[string]interface{}{ + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ "message": "ListTopics failed.", }) return } if len(topics) >= 25 { - ctx.JSON(422, map[string]interface{}{ + ctx.JSON(http.StatusUnprocessableEntity, map[string]interface{}{ "message": "Exceeding maximum allowed topics per repo.", }) return @@ -169,13 +169,13 @@ func AddTopic(ctx *context.APIContext) { _, err = models.AddTopic(ctx.Repo.Repository.ID, topicName) if err != nil { log.Error("AddTopic failed: %v", err) - ctx.JSON(500, map[string]interface{}{ + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ "message": "AddTopic failed.", }) return } - ctx.Status(204) + ctx.Status(http.StatusNoContent) } // DeleteTopic removes topic name from repo @@ -214,7 +214,7 @@ func DeleteTopic(ctx *context.APIContext) { topic, err := models.DeleteTopic(ctx.Repo.Repository.ID, topicName) if err != nil { log.Error("DeleteTopic failed: %v", err) - ctx.JSON(500, map[string]interface{}{ + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ "message": "DeleteTopic failed.", }) return @@ -224,7 +224,7 @@ func DeleteTopic(ctx *context.APIContext) { ctx.NotFound() } - ctx.Status(204) + ctx.Status(http.StatusNoContent) } // TopicSearch search for creating topic @@ -244,7 +244,7 @@ func TopicSearch(ctx *context.Context) { // "200": // "$ref": "#/responses/TopicListResponse" if ctx.User == nil { - ctx.JSON(403, map[string]interface{}{ + ctx.JSON(http.StatusForbidden, map[string]interface{}{ "message": "Only owners could change the topics.", }) return @@ -258,7 +258,7 @@ func TopicSearch(ctx *context.Context) { }) if err != nil { log.Error("SearchTopics failed: %v", err) - ctx.JSON(500, map[string]interface{}{ + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ "message": "Search topics failed.", }) return @@ -268,7 +268,7 @@ func TopicSearch(ctx *context.Context) { for i, topic := range topics { topicResponses[i] = convert.ToTopicResponse(topic) } - ctx.JSON(200, map[string]interface{}{ + ctx.JSON(http.StatusOK, map[string]interface{}{ "topics": topicResponses, }) } From ed7604a5bcfe338076a12655ec839babda24380a Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Tue, 27 Aug 2019 18:32:06 +0000 Subject: [PATCH 21/29] Fix variable scope Signed-off-by: David Svantesson --- models/topic.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/models/topic.go b/models/topic.go index 314c699e49887..0a5384df556c6 100644 --- a/models/topic.go +++ b/models/topic.go @@ -101,9 +101,10 @@ func GetTopicByName(name string) (*Topic, error) { // Returns topic after the addition func addTopicByNameToRepo(e Engine, repoID int64, topicName string) (*Topic, error) { var topic Topic - if has, err := e.Where("name = ?", topicName).Get(&topic); err != nil { + has, err := e.Where("name = ?", topicName).Get(&topic) + if err != nil { return nil, err - } + } if !has { topic.Name = topicName topic.RepoCount = 1 From 1088eacb3fe9e7685c5b00dadbfc0e3ba168b63d Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Tue, 27 Aug 2019 19:41:44 +0000 Subject: [PATCH 22/29] Test one read with login and one with token Signed-off-by: David Svantesson --- integrations/api_repo_topic_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/integrations/api_repo_topic_test.go b/integrations/api_repo_topic_test.go index 32db882c70daf..b217b12fddc33 100644 --- a/integrations/api_repo_topic_test.go +++ b/integrations/api_repo_topic_test.go @@ -25,16 +25,16 @@ func TestAPIRepoTopic(t *testing.T) { token2 := getTokenForLoggedInUser(t, session) session = emptyTestSession(t) + // Test read topics using login url := fmt.Sprintf("/api/v1/repos/%s/%s/topics?token=%s", user2.Name, repo2.Name, token2) - - // Test read topics req := NewRequest(t, "GET", url) res := session.MakeRequest(t, req, http.StatusOK) var topics *api.TopicName DecodeJSON(t, res, &topics) assert.ElementsMatch(t, []string{"topicname1", "topicname2"}, topics.TopicNames) - // Test delete a to7ic + // Test delete a topic + url = fmt.Sprintf("/api/v1/repos/%s/%s/topics?token=%s", user2.Name, repo2.Name, token2) req = NewRequestf(t, "DELETE", "/api/v1/repos/%s/%s/topics/%s?token=%s", user2.Name, repo2.Name, "Topicname1", token2) res = session.MakeRequest(t, req, http.StatusNoContent) @@ -46,7 +46,7 @@ func TestAPIRepoTopic(t *testing.T) { req = NewRequestf(t, "PUT", "/api/v1/repos/%s/%s/topics/%s?token=%s", user2.Name, repo2.Name, "topicName3", token2) res = session.MakeRequest(t, req, http.StatusNoContent) - // Read topics + // Test read topics using token req = NewRequest(t, "GET", url) res = session.MakeRequest(t, req, http.StatusOK) DecodeJSON(t, res, &topics) From 098af672098e5e8c07aaa170e1fdbeb0e8cfb03a Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Tue, 27 Aug 2019 20:02:25 +0000 Subject: [PATCH 23/29] Add some more tests Signed-off-by: David Svantesson --- integrations/api_repo_topic_test.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/integrations/api_repo_topic_test.go b/integrations/api_repo_topic_test.go index b217b12fddc33..e70bce9ed9426 100644 --- a/integrations/api_repo_topic_test.go +++ b/integrations/api_repo_topic_test.go @@ -73,4 +73,31 @@ func TestAPIRepoTopic(t *testing.T) { res = session.MakeRequest(t, req, http.StatusOK) DecodeJSON(t, res, &topics) assert.ElementsMatch(t, []string{"windows", "mac"}, topics.TopicNames) + + // Test with some topics multiple times, less than 25 unique + newTopics = []string{"t1", "t2", "t1", "t3", "t4", "t5", "t6", "t7", "t8", "t9", "t10", "t11", "t12", "t13", "t14", "t15", "t16", "17", "t18", "t19", "t20", "t21", "t22", "t23", "t24", "t25"} + req = NewRequestWithJSON(t, "PUT", url, &api.RepoTopicOptions{ + Topics: newTopics, + }) + res = session.MakeRequest(t, req, http.StatusNoContent) + req = NewRequest(t, "GET", url) + res = session.MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, res, &topics) + assert.Equal(t, 25, len(topics.TopicNames)) + + // Test writing more topics than allowed + newTopics = append(newTopics, "t26") + req = NewRequestWithJSON(t, "PUT", url, &api.RepoTopicOptions{ + Topics: newTopics, + }) + res = session.MakeRequest(t, req, http.StatusUnprocessableEntity) + + // Test add a topic when there is already maximum + req = NewRequestf(t, "PUT", "/api/v1/repos/%s/%s/topics/%s?token=%s", user2.Name, repo2.Name, "t26", token2) + res = session.MakeRequest(t, req, http.StatusUnprocessableEntity) + + // Test delete a topic that repo doesn't have + req = NewRequestf(t, "DELETE", "/api/v1/repos/%s/%s/topics/%s?token=%s", user2.Name, repo2.Name, "Topicname1", token2) + res = session.MakeRequest(t, req, http.StatusNotFound) + } From 41b6276c0807df36f358a0c525afe7db74933036 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Tue, 27 Aug 2019 22:46:42 +0200 Subject: [PATCH 24/29] Apply suggestions from code review Use empty struct for efficiency Co-Authored-By: Lauris BH --- models/topic.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/models/topic.go b/models/topic.go index 0a5384df556c6..e4fda03fc4930 100644 --- a/models/topic.go +++ b/models/topic.go @@ -62,7 +62,7 @@ func ValidateTopic(topic string) bool { // SanitizeAndValidateTopics sanitizes and checks an array or topics func SanitizeAndValidateTopics(topics []string) (validTopics []string, invalidTopics []string) { validTopics = make([]string, 0) - mValidTopics := make(map[string]bool) + mValidTopics := make(map[string]struct{}) invalidTopics = make([]string, 0) for _, topic := range topics { @@ -72,12 +72,12 @@ func SanitizeAndValidateTopics(topics []string) (validTopics []string, invalidTo continue } // ignore same topic twice - if _, exist := mValidTopics[topic]; exist { + if _, ok := mValidTopics[topic]; ok { continue } if ValidateTopic(topic) { validTopics = append(validTopics, topic) - mValidTopics[topic] = true + mValidTopics[topic] = struct{}{} } else { invalidTopics = append(invalidTopics, topic) } From b654ebf93eb41f7ef5c8295959901c75d396f63c Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Wed, 28 Aug 2019 17:38:14 +0000 Subject: [PATCH 25/29] Add test case to check access for user with write access Signed-off-by: David Svantesson --- integrations/api_repo_topic_test.go | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/integrations/api_repo_topic_test.go b/integrations/api_repo_topic_test.go index e70bce9ed9426..6aeb8e823bf0f 100644 --- a/integrations/api_repo_topic_test.go +++ b/integrations/api_repo_topic_test.go @@ -17,8 +17,11 @@ import ( func TestAPIRepoTopic(t *testing.T) { prepareTestEnv(t) - user2 := models.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User) // owner of repo1 + user2 := models.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User) // owner of repo2 + user3 := models.AssertExistsAndLoadBean(t, &models.User{ID: 3}).(*models.User) // owner of repo3 + user4 := models.AssertExistsAndLoadBean(t, &models.User{ID: 4}).(*models.User) // write access to repo 3 repo2 := models.AssertExistsAndLoadBean(t, &models.Repository{ID: 2}).(*models.Repository) + repo3 := models.AssertExistsAndLoadBean(t, &models.Repository{ID: 3}).(*models.Repository) // Get user2's token session := loginUser(t, user2.Name) @@ -100,4 +103,20 @@ func TestAPIRepoTopic(t *testing.T) { req = NewRequestf(t, "DELETE", "/api/v1/repos/%s/%s/topics/%s?token=%s", user2.Name, repo2.Name, "Topicname1", token2) res = session.MakeRequest(t, req, http.StatusNotFound) + // Get user4's token + session = loginUser(t, user4.Name) + token4 := getTokenForLoggedInUser(t, session) + session = emptyTestSession(t) + + // Test read topics with write access + url = fmt.Sprintf("/api/v1/repos/%s/%s/topics?token=%s", user3.Name, repo3.Name, token4) + req = NewRequest(t, "GET", url) + res = session.MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, res, &topics) + assert.Equal(t, 0, len(topics.TopicNames)) + + // Test add a topic to repo with write access (requires repo admin access) + req = NewRequestf(t, "PUT", "/api/v1/repos/%s/%s/topics/%s?token=%s", user3.Name, repo3.Name, "topicName", token4) + res = session.MakeRequest(t, req, http.StatusForbidden) + } From 48989c3a8504d1075f1e67a22ec7e8eb190495e8 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Wed, 28 Aug 2019 17:43:22 +0000 Subject: [PATCH 26/29] Fix access, repo admin required to change topics Signed-off-by: David Svantesson --- routers/api/v1/api.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 430d9f2e47ec8..46c0a066510df 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -773,11 +773,11 @@ func RegisterRoutes(m *macaron.Macaron) { }, reqRepoReader(models.UnitTypeCode)) m.Group("/topics", func() { m.Combo("").Get(repo.ListTopics). - Put(reqToken(), reqRepoWriter(models.UnitTypeCode), bind(api.RepoTopicOptions{}), repo.UpdateTopics) + Put(reqToken(), reqAdmin(), bind(api.RepoTopicOptions{}), repo.UpdateTopics) m.Group("/:topic", func() { - m.Combo("").Put(reqToken(), reqRepoWriter(models.UnitTypeCode), repo.AddTopic). - Delete(reqToken(), reqRepoWriter(models.UnitTypeCode), repo.DeleteTopic) - }) + m.Combo("").Put(reqToken(), repo.AddTopic). + Delete(reqToken(), repo.DeleteTopic) + }, reqAdmin()) }, reqRepoReader(models.UnitTypeCode)) }, repoAssignment()) }) From db48d14cdaaa5139e88948ebfb89cb855088df53 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Wed, 28 Aug 2019 18:26:24 +0000 Subject: [PATCH 27/29] Correct first test to be without token Signed-off-by: David Svantesson --- integrations/api_repo_topic_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/integrations/api_repo_topic_test.go b/integrations/api_repo_topic_test.go index 6aeb8e823bf0f..34c33d1b258ae 100644 --- a/integrations/api_repo_topic_test.go +++ b/integrations/api_repo_topic_test.go @@ -26,18 +26,20 @@ func TestAPIRepoTopic(t *testing.T) { // Get user2's token session := loginUser(t, user2.Name) token2 := getTokenForLoggedInUser(t, session) - session = emptyTestSession(t) // Test read topics using login - url := fmt.Sprintf("/api/v1/repos/%s/%s/topics?token=%s", user2.Name, repo2.Name, token2) + url := fmt.Sprintf("/api/v1/repos/%s/%s/topics", user2.Name, repo2.Name) req := NewRequest(t, "GET", url) res := session.MakeRequest(t, req, http.StatusOK) var topics *api.TopicName DecodeJSON(t, res, &topics) assert.ElementsMatch(t, []string{"topicname1", "topicname2"}, topics.TopicNames) - // Test delete a topic + // Log out user2 + session = emptyTestSession(t) url = fmt.Sprintf("/api/v1/repos/%s/%s/topics?token=%s", user2.Name, repo2.Name, token2) + + // Test delete a topic req = NewRequestf(t, "DELETE", "/api/v1/repos/%s/%s/topics/%s?token=%s", user2.Name, repo2.Name, "Topicname1", token2) res = session.MakeRequest(t, req, http.StatusNoContent) From fa8aa5dabf4cc7ce60b9fb54e481e1cbe081e119 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Thu, 29 Aug 2019 08:30:15 +0200 Subject: [PATCH 28/29] Any repo reader should be able to access topics. --- routers/api/v1/api.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 46c0a066510df..c57edf6a99284 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -778,7 +778,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Combo("").Put(reqToken(), repo.AddTopic). Delete(reqToken(), repo.DeleteTopic) }, reqAdmin()) - }, reqRepoReader(models.UnitTypeCode)) + }, reqAnyRepoReader()) }, repoAssignment()) }) From 9d9f8dcc1c11e663c63ba63079a2499c2a5f0699 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Thu, 29 Aug 2019 16:07:04 +0000 Subject: [PATCH 29/29] No need for string pointer Signed-off-by: David Svantesson --- routers/api/v1/repo/topic.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/routers/api/v1/repo/topic.go b/routers/api/v1/repo/topic.go index 4d258dfcddc68..6c3ac0020af19 100644 --- a/routers/api/v1/repo/topic.go +++ b/routers/api/v1/repo/topic.go @@ -48,9 +48,9 @@ func ListTopics(ctx *context.APIContext) { return } - topicNames := make([]*string, len(topics)) + topicNames := make([]string, len(topics)) for i, topic := range topics { - topicNames[i] = &topic.Name + topicNames[i] = topic.Name } ctx.JSON(http.StatusOK, map[string]interface{}{ "topics": topicNames,