From 42096c63e8b4692e76e91700030f1a7892fe7134 Mon Sep 17 00:00:00 2001 From: Alexey Terentyev Date: Sat, 16 Jun 2018 05:57:58 +0300 Subject: [PATCH 01/18] Added topics validation, fixed repo topics duplication (#4031) Signed-off-by: Alexey Terentyev --- models/topic.go | 23 +++++++++++++++++++++++ options/locale/locale_en-US.ini | 2 ++ routers/repo/topic.go | 29 ++++++++++++++++++++++++++++- routers/routes/routes.go | 2 +- 4 files changed, 54 insertions(+), 2 deletions(-) diff --git a/models/topic.go b/models/topic.go index 3b1737f8afefe..c1c518de878cf 100644 --- a/models/topic.go +++ b/models/topic.go @@ -11,6 +11,7 @@ import ( "code.gitea.io/gitea/modules/util" "github.com/go-xorm/builder" + "regexp" ) func init() { @@ -20,6 +21,8 @@ func init() { ) } +var topicPattern = regexp.MustCompile(`^[a-z0-9+#_.-]+$`) + // Topic represents a topic of repositories type Topic struct { ID int64 @@ -51,6 +54,26 @@ func (err ErrTopicNotExist) Error() string { return fmt.Sprintf("topic is not exist [name: %s]", err.Name) } +func TopicValidator(topic string) bool { + return len(topic) <= 35 && topicPattern.MatchString(topic) +} + +// Remove duplicates from topics slice +func RemoveDuplicateTopics(topics []string) []string { + // Map to record duplicates + saved := make(map[string]struct{}, len(topics)) + i := 0 + for _, v := range topics { + v = strings.TrimSpace(strings.ToLower(v)) + if _, ok := saved[v]; !ok { + saved[v] = struct{}{} + topics[i] = v + i++ + } + } + return topics[:i] +} + // GetTopicByName retrieves topic by name func GetTopicByName(name string) (*Topic, error) { var topic Topic diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 8cf6111c6de24..db6c0b634aca9 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1167,6 +1167,8 @@ branch.protected_deletion_failed = Branch '%s' is protected. It cannot be delete topic.manage_topics = Manage Topics topic.done = Done +topic.count_prompt = You can't select more than 25 topics +topic.format_prompt = Topics must use letter or number and can include hyphen(-), underscore(_), plus(+), hash(#), dot(.) with max length of 35 [org] org_name_holder = Organization Name diff --git a/routers/repo/topic.go b/routers/repo/topic.go index 2a43d53ff0ba9..2a5dc8fa59854 100644 --- a/routers/repo/topic.go +++ b/routers/repo/topic.go @@ -13,7 +13,7 @@ import ( ) // TopicPost response for creating repository -func TopicPost(ctx *context.Context) { +func TopicsPost(ctx *context.Context) { if ctx.User == nil { ctx.JSON(403, map[string]interface{}{ "message": "Only owners could change the topics.", @@ -27,6 +27,33 @@ func TopicPost(ctx *context.Context) { topics = strings.Split(topicsStr, ",") } + topics = models.RemoveDuplicateTopics(topics) + + if len(topics) > 25 { + log.Error(2, "Incorrect number of topics(max 25): %v", ) + ctx.JSON(422, map[string]interface{}{ + "invalidTopics": topics[:0], + "message": ctx.Tr("repo.topic.count_error"), + }) + return + } + + var invalidTopics = make([]string, 0) + for _, topic := range topics { + if !models.TopicValidator(topic) { + invalidTopics = append(invalidTopics, topic) + } + } + + if len(invalidTopics) > 0 { + log.Error(2, "Invalid topics: %v", invalidTopics) + ctx.JSON(422, map[string]interface{}{ + "invalidTopics": invalidTopics, + "message": ctx.Tr("repo.topic.pattern_error"), + }) + return + } + err := models.SaveTopics(ctx.Repo.Repository.ID, topics...) if err != nil { log.Error(2, "SaveTopics failed: %v", err) diff --git a/routers/routes/routes.go b/routers/routes/routes.go index 15b91f1599708..558564a0d1bee 100644 --- a/routers/routes/routes.go +++ b/routers/routes/routes.go @@ -626,7 +626,7 @@ func RegisterRoutes(m *macaron.Macaron) { }, context.RepoAssignment(), context.UnitTypes(), context.LoadRepoUnits(), context.CheckUnit(models.UnitTypeReleases)) m.Group("/:username/:reponame", func() { - m.Post("/topics", repo.TopicPost) + m.Post("/topics", repo.TopicsPost) }, context.RepoAssignment(), reqRepoAdmin) m.Group("/:username/:reponame", func() { From f2546a76d98266039a729b85c39982391bf75af0 Mon Sep 17 00:00:00 2001 From: Alexey Terentyev Date: Sat, 16 Jun 2018 07:15:35 +0300 Subject: [PATCH 02/18] Added tests Signed-off-by: Alexey Terentyev --- models/topic_test.go | 25 +++++++++++++++++++++++++ routers/repo/topic.go | 6 +++--- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/models/topic_test.go b/models/topic_test.go index 472f4e52d9b5a..ecff6e083d2ed 100644 --- a/models/topic_test.go +++ b/models/topic_test.go @@ -5,6 +5,7 @@ package models import ( + "sort" "testing" "github.com/stretchr/testify/assert" @@ -55,3 +56,27 @@ func TestAddTopic(t *testing.T) { assert.NoError(t, err) assert.EqualValues(t, 2, len(topics)) } + +func TestTopicValidator(t *testing.T) { + assert.True(t, TopicValidator("first-topic")) + assert.True(t, TopicValidator("#second-test_topic")) + assert.True(t, TopicValidator("third+project+topic.with+max_length")) + + assert.False(t, TopicValidator("$fourth-topic")) + assert.False(t, TopicValidator("#fifth,test;topic")) + assert.False(t, TopicValidator("#sixth-go+project.topic+with+excess_length")) +} + +func TestRemoveDuplicateTopics(t *testing.T) { + topics := []string{"first", "second", "eleventh", "third", "fourth", "fifth", "sixth", "second", "seventh", + "eleventh", "first", "eighth", "ninth", "sixth", "tenth", "eleventh"} + + expectedSlice := []string{"first", "second", "third", "fourth", "fifth", "sixth", "seventh", "eighth", "ninth", "tenth", "eleventh"} + + topics = RemoveDuplicateTopics(topics) + assert.Len(t, topics, 11) + + sort.Strings(topics) + sort.Strings(expectedSlice) + assert.EqualValues(t, expectedSlice, topics) +} diff --git a/routers/repo/topic.go b/routers/repo/topic.go index 2a5dc8fa59854..013a5b7696c0e 100644 --- a/routers/repo/topic.go +++ b/routers/repo/topic.go @@ -30,10 +30,10 @@ func TopicsPost(ctx *context.Context) { topics = models.RemoveDuplicateTopics(topics) if len(topics) > 25 { - log.Error(2, "Incorrect number of topics(max 25): %v", ) + log.Error(2, "Incorrect number of topics(max 25)") ctx.JSON(422, map[string]interface{}{ "invalidTopics": topics[:0], - "message": ctx.Tr("repo.topic.count_error"), + "message": ctx.Tr("repo.topic.count_error"), }) return } @@ -49,7 +49,7 @@ func TopicsPost(ctx *context.Context) { log.Error(2, "Invalid topics: %v", invalidTopics) ctx.JSON(422, map[string]interface{}{ "invalidTopics": invalidTopics, - "message": ctx.Tr("repo.topic.pattern_error"), + "message": ctx.Tr("repo.topic.pattern_error"), }) return } From b0e7d001f88b25dfa26f7f762f8b6cfb739ec51a Mon Sep 17 00:00:00 2001 From: Alexey Terentyev Date: Sat, 16 Jun 2018 16:23:27 +0300 Subject: [PATCH 03/18] Fixed fmt Signed-off-by: Alexey Terentyev --- models/topic.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/topic.go b/models/topic.go index c1c518de878cf..3942e7f1a2fed 100644 --- a/models/topic.go +++ b/models/topic.go @@ -6,12 +6,12 @@ package models import ( "fmt" + "regexp" "strings" "code.gitea.io/gitea/modules/util" "github.com/go-xorm/builder" - "regexp" ) func init() { From 6579ddf9d83cb6e77e7298b2e4a876e990029e50 Mon Sep 17 00:00:00 2001 From: Alexey Terentyev Date: Sat, 16 Jun 2018 16:44:08 +0300 Subject: [PATCH 04/18] Added comments to exported functions Signed-off-by: Alexey Terentyev --- models/topic.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/models/topic.go b/models/topic.go index 3942e7f1a2fed..0a67fdd2fed0b 100644 --- a/models/topic.go +++ b/models/topic.go @@ -54,11 +54,12 @@ func (err ErrTopicNotExist) Error() string { return fmt.Sprintf("topic is not exist [name: %s]", err.Name) } +// TopicValidator checks topics by length and match pattern rules func TopicValidator(topic string) bool { return len(topic) <= 35 && topicPattern.MatchString(topic) } -// Remove duplicates from topics slice +// RemoveDuplicateTopics remove duplicates from topics slice func RemoveDuplicateTopics(topics []string) []string { // Map to record duplicates saved := make(map[string]struct{}, len(topics)) From 630ca5901c199da5e309868b1b872f622946882d Mon Sep 17 00:00:00 2001 From: Alexey Terentyev Date: Sun, 17 Jun 2018 14:48:20 +0300 Subject: [PATCH 05/18] Deleted RemoveDuplicateTopics function Signed-off-by: Alexey Terentyev --- models/topic.go | 16 ---------------- models/topic_test.go | 15 --------------- public/js/index.js | 5 +++-- routers/repo/topic.go | 24 +++++++++++++++--------- 4 files changed, 18 insertions(+), 42 deletions(-) diff --git a/models/topic.go b/models/topic.go index 0a67fdd2fed0b..d4610c52eec32 100644 --- a/models/topic.go +++ b/models/topic.go @@ -59,22 +59,6 @@ func TopicValidator(topic string) bool { return len(topic) <= 35 && topicPattern.MatchString(topic) } -// RemoveDuplicateTopics remove duplicates from topics slice -func RemoveDuplicateTopics(topics []string) []string { - // Map to record duplicates - saved := make(map[string]struct{}, len(topics)) - i := 0 - for _, v := range topics { - v = strings.TrimSpace(strings.ToLower(v)) - if _, ok := saved[v]; !ok { - saved[v] = struct{}{} - topics[i] = v - i++ - } - } - return topics[:i] -} - // GetTopicByName retrieves topic by name func GetTopicByName(name string) (*Topic, error) { var topic Topic diff --git a/models/topic_test.go b/models/topic_test.go index ecff6e083d2ed..93137fd7224ee 100644 --- a/models/topic_test.go +++ b/models/topic_test.go @@ -5,7 +5,6 @@ package models import ( - "sort" "testing" "github.com/stretchr/testify/assert" @@ -66,17 +65,3 @@ func TestTopicValidator(t *testing.T) { assert.False(t, TopicValidator("#fifth,test;topic")) assert.False(t, TopicValidator("#sixth-go+project.topic+with+excess_length")) } - -func TestRemoveDuplicateTopics(t *testing.T) { - topics := []string{"first", "second", "eleventh", "third", "fourth", "fifth", "sixth", "second", "seventh", - "eleventh", "first", "eighth", "ninth", "sixth", "tenth", "eleventh"} - - expectedSlice := []string{"first", "second", "third", "fourth", "fifth", "sixth", "seventh", "eighth", "ninth", "tenth", "eleventh"} - - topics = RemoveDuplicateTopics(topics) - assert.Len(t, topics, 11) - - sort.Strings(topics) - sort.Strings(expectedSlice) - assert.EqualValues(t, expectedSlice, topics) -} diff --git a/public/js/index.js b/public/js/index.js index e98a3fe6de2e8..6c760e3673abc 100644 --- a/public/js/index.js +++ b/public/js/index.js @@ -2336,8 +2336,9 @@ function initTopicbar() { }).done(function() { editDiv.hide(); viewDiv.show(); - }) - }) + }).fail(function(xhr) { + alert(xhr.responseJSON.message) + }); $('#topic_edit .dropdown').dropdown({ allowAdditions: true, diff --git a/routers/repo/topic.go b/routers/repo/topic.go index 013a5b7696c0e..85eaf674e5f56 100644 --- a/routers/repo/topic.go +++ b/routers/repo/topic.go @@ -12,7 +12,7 @@ import ( "code.gitea.io/gitea/modules/log" ) -// TopicPost response for creating repository +// TopicsPost response for creating repository func TopicsPost(ctx *context.Context) { if ctx.User == nil { ctx.JSON(403, map[string]interface{}{ @@ -27,7 +27,20 @@ func TopicsPost(ctx *context.Context) { topics = strings.Split(topicsStr, ",") } - topics = models.RemoveDuplicateTopics(topics) + 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.TopicValidator(topic) { + invalidTopics = append(invalidTopics, topic) + } + } + topics = topics[:i] if len(topics) > 25 { log.Error(2, "Incorrect number of topics(max 25)") @@ -38,13 +51,6 @@ func TopicsPost(ctx *context.Context) { return } - var invalidTopics = make([]string, 0) - for _, topic := range topics { - if !models.TopicValidator(topic) { - invalidTopics = append(invalidTopics, topic) - } - } - if len(invalidTopics) > 0 { log.Error(2, "Invalid topics: %v", invalidTopics) ctx.JSON(422, map[string]interface{}{ From c1364ea7179442bbcac2fab4ac9223a7ae71ad06 Mon Sep 17 00:00:00 2001 From: Alexey Terentyev Date: Sun, 17 Jun 2018 15:13:14 +0300 Subject: [PATCH 06/18] Fixed messages Signed-off-by: Alexey Terentyev --- public/js/index.js | 3 ++- routers/repo/topic.go | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/public/js/index.js b/public/js/index.js index 6c760e3673abc..823dd876695de 100644 --- a/public/js/index.js +++ b/public/js/index.js @@ -2338,7 +2338,8 @@ function initTopicbar() { viewDiv.show(); }).fail(function(xhr) { alert(xhr.responseJSON.message) - }); + }) + }); $('#topic_edit .dropdown').dropdown({ allowAdditions: true, diff --git a/routers/repo/topic.go b/routers/repo/topic.go index 85eaf674e5f56..82df2a49db226 100644 --- a/routers/repo/topic.go +++ b/routers/repo/topic.go @@ -46,7 +46,7 @@ func TopicsPost(ctx *context.Context) { log.Error(2, "Incorrect number of topics(max 25)") ctx.JSON(422, map[string]interface{}{ "invalidTopics": topics[:0], - "message": ctx.Tr("repo.topic.count_error"), + "message": ctx.Tr("repo.topic.count_prompt"), }) return } @@ -55,7 +55,7 @@ func TopicsPost(ctx *context.Context) { log.Error(2, "Invalid topics: %v", invalidTopics) ctx.JSON(422, map[string]interface{}{ "invalidTopics": invalidTopics, - "message": ctx.Tr("repo.topic.pattern_error"), + "message": ctx.Tr("repo.topic.format_prompt"), }) return } From 7f1ec1895dd43b8b2252fc3352d1f86d284a37c9 Mon Sep 17 00:00:00 2001 From: Alexey Terentyev Date: Mon, 18 Jun 2018 03:18:17 +0300 Subject: [PATCH 07/18] Added migration Signed-off-by: Alexey Terentyev --- models/migrations/migrations.go | 2 + models/migrations/v67.go | 126 ++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+) create mode 100644 models/migrations/v67.go diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 1300065ab4c75..7eec39b36533f 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -186,6 +186,8 @@ var migrations = []Migration{ NewMigration("add u2f", addU2FReg), // v66 -> v67 NewMigration("add login source id column for public_key table", addLoginSourceIDToPublicKeyTable), + // v67 -> V68 + NewMigration("Reformat and remove incorrect topics", reformatAndRemoveIncorrectTopics), } // Migrate database to current version diff --git a/models/migrations/v67.go b/models/migrations/v67.go new file mode 100644 index 0000000000000..0895b74d17e15 --- /dev/null +++ b/models/migrations/v67.go @@ -0,0 +1,126 @@ +package migrations + +import ( + "strings" + + "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/log" + + "github.com/go-xorm/xorm" +) + +func reformatAndRemoveIncorrectTopics(x *xorm.Engine) (err error) { + log.Info("This migration could take up to minutes, please be patient.") + type Topic struct { + ID int64 + Name string `xorm:"unique"` + } + + sess := x.NewSession() + defer sess.Close() + + const batchSize = 100 + var ids []int64 + touchedRepo := make(map[int64]struct{}) + topics := make([]*Topic, 0, batchSize) + + if err := sess.Begin(); err != nil { + return err + } + log.Info("Validating existed topics...") + for start := 0; ; start += batchSize { + topics = topics[:0] + if err := x.Asc("id").Limit(batchSize, start).Find(&topics); err != nil { + return err + } + if len(topics) == 0 { + break + } + for _, topic := range topics { + if models.TopicValidator(topic.Name) { + continue + } + topic.Name = strings.Replace(strings.TrimSpace(strings.ToLower(topic.Name)), " ", "-", -1) + + if err := sess.Table("repo_topic").Cols("repo_id").Where("topic_id = ?", topic.ID).Find(&ids); err != nil { + return err + } + for _, repoId := range ids { + touchedRepo[repoId] = struct{}{} + } + + if models.TopicValidator(topic.Name) { + log.Info("Updating topic: id = %v, name = %v", topic.ID, topic.Name) + if _, err := sess.Table("topic").ID(topic.ID).Update(&Topic{Name: topic.Name}); err != nil { + return err + } + } else { + log.Info("Deleting 'repo_topic' rows for 'topic' with id = %v and topicName = %v", topic.ID, topic.Name) + if _, err := sess.Where("topic_id = ?", topic.ID).Delete(&models.RepoTopic{}); err != nil { + return err + } + log.Info("Deleting 'topic' with id = %v and topicName = %v", topic.ID, topic.Name) + if _, err := sess.ID(topic.ID).Delete(&Topic{}); err != nil { + return err + } + } + } + } + + repoTopics := make([]*models.RepoTopic, 0, batchSize) + tmpRepoTopics := make([]*models.RepoTopic, 0, 25) + log.Info("Checking the number of topics in the repositories...") + for start := 0; ; start += batchSize { + repoTopics = repoTopics[:0] + if err := sess.Cols("repo_id").Asc("repo_id").Limit(batchSize, start). + GroupBy("repo_id").Having("COUNT(*) > 25").Find(&repoTopics); err != nil { + return err + } + if len(repoTopics) == 0 { + break + } + log.Info("Number of repositories with more than 25 topics: %v", len(repoTopics)) + + for _, repoTopic := range repoTopics { + touchedRepo[repoTopic.RepoID] = struct{}{} + + tmpRepoTopics = tmpRepoTopics[:0] + if err := sess.Where("repo_id = ?", repoTopic.RepoID).Find(&tmpRepoTopics); err != nil { + return err + } + + log.Info("Repository with id = %v has %v topics", repoTopic.RepoID, len(tmpRepoTopics)) + + for i := len(tmpRepoTopics) - 1; i > 24; i-- { + log.Info("Deleting 'repo_topic' rows for 'repository' with id = %v. Topic id = %v", tmpRepoTopics[i].RepoID, tmpRepoTopics[i].TopicID) + if _, err := sess.Where("repo_id = ? AND topic_id = ?", tmpRepoTopics[i].RepoID, tmpRepoTopics[i].TopicID). + Delete(&models.RepoTopic{}); err != nil { + return err + } + if _, err := sess.Exec("UPDATE topic SET repo_count = (SELECT repo_count FROM topic WHERE id = ?) - 1 WHERE id = ?", + tmpRepoTopics[i].TopicID, tmpRepoTopics[i].TopicID); err != nil { + return err + } + } + } + } + + var topicNames []string + log.Info("Updating repositories 'topics' fields...") + for repoId := range touchedRepo { + if err := sess.Table("topic").Cols("name"). + Join("INNER", "repo_topic", "topic.id = repo_topic.topic_id"). + Where("repo_topic.repo_id = ?", repoId).Find(&topicNames); err != nil { + return err + } + log.Info("Updating 'topics' field for repository with id = %v", repoId) + if _, err := sess.ID(repoId).Cols("topics").Update(&models.Repository{ID: repoId, Topics: topicNames}); err != nil { + return err + } + } + if err := sess.Commit(); err != nil { + return err + } + + return nil +} From 1811a6b272c87859238451bfbca563b514936524 Mon Sep 17 00:00:00 2001 From: Alexey Terentyev Date: Mon, 18 Jun 2018 03:45:22 +0300 Subject: [PATCH 08/18] fmt migration file Signed-off-by: Alexey Terentyev --- models/migrations/v67.go | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/models/migrations/v67.go b/models/migrations/v67.go index 0895b74d17e15..5d249faccfb2e 100644 --- a/models/migrations/v67.go +++ b/models/migrations/v67.go @@ -42,7 +42,8 @@ func reformatAndRemoveIncorrectTopics(x *xorm.Engine) (err error) { } topic.Name = strings.Replace(strings.TrimSpace(strings.ToLower(topic.Name)), " ", "-", -1) - if err := sess.Table("repo_topic").Cols("repo_id").Where("topic_id = ?", topic.ID).Find(&ids); err != nil { + if err := sess.Table("repo_topic").Cols("repo_id"). + Where("topic_id = ?", topic.ID).Find(&ids); err != nil { return err } for _, repoId := range ids { @@ -51,14 +52,19 @@ func reformatAndRemoveIncorrectTopics(x *xorm.Engine) (err error) { if models.TopicValidator(topic.Name) { log.Info("Updating topic: id = %v, name = %v", topic.ID, topic.Name) - if _, err := sess.Table("topic").ID(topic.ID).Update(&Topic{Name: topic.Name}); err != nil { + if _, err := sess.Table("topic").ID(topic.ID). + Update(&Topic{Name: topic.Name}); err != nil { return err } } else { - log.Info("Deleting 'repo_topic' rows for 'topic' with id = %v and topicName = %v", topic.ID, topic.Name) - if _, err := sess.Where("topic_id = ?", topic.ID).Delete(&models.RepoTopic{}); err != nil { + log.Info("Deleting 'repo_topic' rows for 'topic' with id = %v and topicName = %v", + topic.ID, topic.Name) + + if _, err := sess.Where("topic_id = ?", topic.ID). + Delete(&models.RepoTopic{}); err != nil { return err } + log.Info("Deleting 'topic' with id = %v and topicName = %v", topic.ID, topic.Name) if _, err := sess.ID(topic.ID).Delete(&Topic{}); err != nil { return err @@ -72,6 +78,7 @@ func reformatAndRemoveIncorrectTopics(x *xorm.Engine) (err error) { log.Info("Checking the number of topics in the repositories...") for start := 0; ; start += batchSize { repoTopics = repoTopics[:0] + if err := sess.Cols("repo_id").Asc("repo_id").Limit(batchSize, start). GroupBy("repo_id").Having("COUNT(*) > 25").Find(&repoTopics); err != nil { return err @@ -92,12 +99,15 @@ func reformatAndRemoveIncorrectTopics(x *xorm.Engine) (err error) { log.Info("Repository with id = %v has %v topics", repoTopic.RepoID, len(tmpRepoTopics)) for i := len(tmpRepoTopics) - 1; i > 24; i-- { - log.Info("Deleting 'repo_topic' rows for 'repository' with id = %v. Topic id = %v", tmpRepoTopics[i].RepoID, tmpRepoTopics[i].TopicID) - if _, err := sess.Where("repo_id = ? AND topic_id = ?", tmpRepoTopics[i].RepoID, tmpRepoTopics[i].TopicID). - Delete(&models.RepoTopic{}); err != nil { + log.Info("Deleting 'repo_topic' rows for 'repository' with id = %v. Topic id = %v", + tmpRepoTopics[i].RepoID, tmpRepoTopics[i].TopicID) + + if _, err := sess.Where("repo_id = ? AND topic_id = ?", tmpRepoTopics[i].RepoID, + tmpRepoTopics[i].TopicID).Delete(&models.RepoTopic{}); err != nil { return err } - if _, err := sess.Exec("UPDATE topic SET repo_count = (SELECT repo_count FROM topic WHERE id = ?) - 1 WHERE id = ?", + if _, err := sess.Exec( + "UPDATE topic SET repo_count = (SELECT repo_count FROM topic WHERE id = ?) - 1 WHERE id = ?", tmpRepoTopics[i].TopicID, tmpRepoTopics[i].TopicID); err != nil { return err } @@ -114,7 +124,8 @@ func reformatAndRemoveIncorrectTopics(x *xorm.Engine) (err error) { return err } log.Info("Updating 'topics' field for repository with id = %v", repoId) - if _, err := sess.ID(repoId).Cols("topics").Update(&models.Repository{ID: repoId, Topics: topicNames}); err != nil { + if _, err := sess.ID(repoId).Cols("topics"). + Update(&models.Repository{ID: repoId, Topics: topicNames}); err != nil { return err } } From cab094ec8d5aa619871362056203ce1e9e211524 Mon Sep 17 00:00:00 2001 From: Alexey Terentyev Date: Mon, 18 Jun 2018 04:19:41 +0300 Subject: [PATCH 09/18] fixed lint Signed-off-by: Alexey Terentyev --- models/migrations/v67.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/models/migrations/v67.go b/models/migrations/v67.go index 5d249faccfb2e..ff10b2fd325d2 100644 --- a/models/migrations/v67.go +++ b/models/migrations/v67.go @@ -46,8 +46,8 @@ func reformatAndRemoveIncorrectTopics(x *xorm.Engine) (err error) { Where("topic_id = ?", topic.ID).Find(&ids); err != nil { return err } - for _, repoId := range ids { - touchedRepo[repoId] = struct{}{} + for _, id := range ids { + touchedRepo[id] = struct{}{} } if models.TopicValidator(topic.Name) { @@ -117,15 +117,15 @@ func reformatAndRemoveIncorrectTopics(x *xorm.Engine) (err error) { var topicNames []string log.Info("Updating repositories 'topics' fields...") - for repoId := range touchedRepo { + for repoID := range touchedRepo { if err := sess.Table("topic").Cols("name"). Join("INNER", "repo_topic", "topic.id = repo_topic.topic_id"). - Where("repo_topic.repo_id = ?", repoId).Find(&topicNames); err != nil { + Where("repo_topic.repo_id = ?", repoID).Find(&topicNames); err != nil { return err } - log.Info("Updating 'topics' field for repository with id = %v", repoId) - if _, err := sess.ID(repoId).Cols("topics"). - Update(&models.Repository{ID: repoId, Topics: topicNames}); err != nil { + log.Info("Updating 'topics' field for repository with id = %v", repoID) + if _, err := sess.ID(repoID).Cols("topics"). + Update(&models.Repository{ID: repoID, Topics: topicNames}); err != nil { return err } } From 8fee5d3f64678bb1589a89c40e3d8e8942041f0f Mon Sep 17 00:00:00 2001 From: Alexey Terentyev Date: Mon, 18 Jun 2018 04:44:23 +0300 Subject: [PATCH 10/18] Added Copyright Signed-off-by: Alexey Terentyev --- models/migrations/v67.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/models/migrations/v67.go b/models/migrations/v67.go index ff10b2fd325d2..34df5166ab11d 100644 --- a/models/migrations/v67.go +++ b/models/migrations/v67.go @@ -1,3 +1,7 @@ +// Copyright 2018 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 migrations import ( From 9141482afde10adb8b8f96b8b616be26a8edac13 Mon Sep 17 00:00:00 2001 From: Alexey Terentyev Date: Mon, 18 Jun 2018 05:25:26 +0300 Subject: [PATCH 11/18] Added query solution for duplicates Signed-off-by: Alexey Terentyev --- models/migrations/v67.go | 2 +- models/topic.go | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/models/migrations/v67.go b/models/migrations/v67.go index 34df5166ab11d..23327a4f5ea98 100644 --- a/models/migrations/v67.go +++ b/models/migrations/v67.go @@ -129,7 +129,7 @@ func reformatAndRemoveIncorrectTopics(x *xorm.Engine) (err error) { } log.Info("Updating 'topics' field for repository with id = %v", repoID) if _, err := sess.ID(repoID).Cols("topics"). - Update(&models.Repository{ID: repoID, Topics: topicNames}); err != nil { + Update(&models.Repository{Topics: topicNames}); err != nil { return err } } diff --git a/models/topic.go b/models/topic.go index d4610c52eec32..d777c80b51a29 100644 --- a/models/topic.go +++ b/models/topic.go @@ -190,6 +190,13 @@ func SaveTopics(repoID int64, topicNames ...string) error { } } + topicNames = topicNames[:0] + if err := sess.Table("topic").Cols("name"). + Join("INNER", "repo_topic", "topic.id = repo_topic.topic_id"). + Where("repo_topic.repo_id = ?", repoID).Find(&topicNames); err != nil { + return err + } + if _, err := sess.ID(repoID).Cols("topics").Update(&Repository{ Topics: topicNames, }); err != nil { From 613f2b5bcbd0be6898a71faad0abdb03f2b79964 Mon Sep 17 00:00:00 2001 From: Alexey Terentyev Date: Mon, 18 Jun 2018 11:11:58 +0300 Subject: [PATCH 12/18] Fixed migration query Signed-off-by: Alexey Terentyev --- models/migrations/v67.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/migrations/v67.go b/models/migrations/v67.go index 23327a4f5ea98..4e6a775302313 100644 --- a/models/migrations/v67.go +++ b/models/migrations/v67.go @@ -34,7 +34,7 @@ func reformatAndRemoveIncorrectTopics(x *xorm.Engine) (err error) { log.Info("Validating existed topics...") for start := 0; ; start += batchSize { topics = topics[:0] - if err := x.Asc("id").Limit(batchSize, start).Find(&topics); err != nil { + if err := sess.Asc("id").Limit(batchSize, start).Find(&topics); err != nil { return err } if len(topics) == 0 { From c1da9ae887ab4d9f84dc937e7cdbf04b4c2b3b7e Mon Sep 17 00:00:00 2001 From: Alexey Terentyev Date: Tue, 19 Jun 2018 16:41:48 +0300 Subject: [PATCH 13/18] Changed RegExp. Fixed migration Signed-off-by: Alexey Terentyev --- models/migrations/v67.go | 75 +++++++++++++++++++++------------ models/topic.go | 2 +- options/locale/locale_en-US.ini | 2 +- 3 files changed, 49 insertions(+), 30 deletions(-) diff --git a/models/migrations/v67.go b/models/migrations/v67.go index 4e6a775302313..77bfa6ae95878 100644 --- a/models/migrations/v67.go +++ b/models/migrations/v67.go @@ -24,9 +24,10 @@ func reformatAndRemoveIncorrectTopics(x *xorm.Engine) (err error) { defer sess.Close() const batchSize = 100 - var ids []int64 touchedRepo := make(map[int64]struct{}) topics := make([]*Topic, 0, batchSize) + delTopicIDs := make([]int64, 0, batchSize) + ids := make([]int64, 0, 30) if err := sess.Begin(); err != nil { return err @@ -61,28 +62,41 @@ func reformatAndRemoveIncorrectTopics(x *xorm.Engine) (err error) { return err } } else { - log.Info("Deleting 'repo_topic' rows for 'topic' with id = %v and topicName = %v", - topic.ID, topic.Name) + delTopicIDs = append(delTopicIDs, topic.ID) + } + } + } - if _, err := sess.Where("topic_id = ?", topic.ID). - Delete(&models.RepoTopic{}); err != nil { - return err - } + log.Info("Deleting incorrect topics...") + for start := 0; ; start+=batchSize { + if (start + batchSize) < len(delTopicIDs) { + ids = delTopicIDs[start:(start + batchSize)] + } else { + ids = delTopicIDs[start:] + } - log.Info("Deleting 'topic' with id = %v and topicName = %v", topic.ID, topic.Name) - if _, err := sess.ID(topic.ID).Delete(&Topic{}); err != nil { - return err - } - } + log.Info("Deleting 'repo_topic' rows for topics with ids = %v", ids) + if _, err := sess.In("topic_id", ids).Delete(&models.RepoTopic{}); err != nil { + return err + } + + log.Info("Deleting topics with id = %v", ids) + if _, err := sess.In("id", ids).Delete(&Topic{}); err != nil { + return err + } + + if len(ids) < batchSize { + break } } repoTopics := make([]*models.RepoTopic, 0, batchSize) - tmpRepoTopics := make([]*models.RepoTopic, 0, 25) + delRepoTopics := make([]*models.RepoTopic, 0, batchSize) + tmpRepoTopics := make([]*models.RepoTopic, 0, 30) + log.Info("Checking the number of topics in the repositories...") for start := 0; ; start += batchSize { repoTopics = repoTopics[:0] - if err := sess.Cols("repo_id").Asc("repo_id").Limit(batchSize, start). GroupBy("repo_id").Having("COUNT(*) > 25").Find(&repoTopics); err != nil { return err @@ -90,8 +104,8 @@ func reformatAndRemoveIncorrectTopics(x *xorm.Engine) (err error) { if len(repoTopics) == 0 { break } - log.Info("Number of repositories with more than 25 topics: %v", len(repoTopics)) + log.Info("Number of repositories with more than 25 topics: %v", len(repoTopics)) for _, repoTopic := range repoTopics { touchedRepo[repoTopic.RepoID] = struct{}{} @@ -103,23 +117,28 @@ func reformatAndRemoveIncorrectTopics(x *xorm.Engine) (err error) { log.Info("Repository with id = %v has %v topics", repoTopic.RepoID, len(tmpRepoTopics)) for i := len(tmpRepoTopics) - 1; i > 24; i-- { - log.Info("Deleting 'repo_topic' rows for 'repository' with id = %v. Topic id = %v", - tmpRepoTopics[i].RepoID, tmpRepoTopics[i].TopicID) - - if _, err := sess.Where("repo_id = ? AND topic_id = ?", tmpRepoTopics[i].RepoID, - tmpRepoTopics[i].TopicID).Delete(&models.RepoTopic{}); err != nil { - return err - } - if _, err := sess.Exec( - "UPDATE topic SET repo_count = (SELECT repo_count FROM topic WHERE id = ?) - 1 WHERE id = ?", - tmpRepoTopics[i].TopicID, tmpRepoTopics[i].TopicID); err != nil { - return err - } + delRepoTopics = append(delRepoTopics, tmpRepoTopics[i]) } } } - var topicNames []string + log.Info("Deleting superfluous topics for repositories (more than 25 topics)...") + for _, repoTopic := range delRepoTopics { + log.Info("Deleting 'repo_topic' rows for 'repository' with id = %v. Topic id = %v", + repoTopic.RepoID, repoTopic.TopicID) + + if _, err := sess.Where("repo_id = ? AND topic_id = ?", repoTopic.RepoID, + repoTopic.TopicID).Delete(&models.RepoTopic{}); err != nil { + return err + } + if _, err := sess.Exec( + "UPDATE topic SET repo_count = (SELECT repo_count FROM topic WHERE id = ?) - 1 WHERE id = ?", + repoTopic.TopicID, repoTopic.TopicID); err != nil { + return err + } + } + + topicNames := make([]string, 0, 30) log.Info("Updating repositories 'topics' fields...") for repoID := range touchedRepo { if err := sess.Table("topic").Cols("name"). diff --git a/models/topic.go b/models/topic.go index d777c80b51a29..a2d87099f4961 100644 --- a/models/topic.go +++ b/models/topic.go @@ -21,7 +21,7 @@ func init() { ) } -var topicPattern = regexp.MustCompile(`^[a-z0-9+#_.-]+$`) +var topicPattern = regexp.MustCompile(`^[a-z0-9][a-z0-9-]*$`) // Topic represents a topic of repositories type Topic struct { diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index db6c0b634aca9..21ae775e4197a 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1168,7 +1168,7 @@ branch.protected_deletion_failed = Branch '%s' is protected. It cannot be delete topic.manage_topics = Manage Topics topic.done = Done topic.count_prompt = You can't select more than 25 topics -topic.format_prompt = Topics must use letter or number and can include hyphen(-), underscore(_), plus(+), hash(#), dot(.) with max length of 35 +topic.format_prompt = Topics must start with a letter or number, can include hyphens(-) and must be no more than 35 characters long [org] org_name_holder = Organization Name From e326026c8c5b46822b15e6206e738494036f036f Mon Sep 17 00:00:00 2001 From: Alexey Terentyev Date: Tue, 19 Jun 2018 16:49:39 +0300 Subject: [PATCH 14/18] fmt migration file Signed-off-by: Alexey Terentyev --- models/migrations/v67.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/migrations/v67.go b/models/migrations/v67.go index 77bfa6ae95878..4fb758a8585b6 100644 --- a/models/migrations/v67.go +++ b/models/migrations/v67.go @@ -68,7 +68,7 @@ func reformatAndRemoveIncorrectTopics(x *xorm.Engine) (err error) { } log.Info("Deleting incorrect topics...") - for start := 0; ; start+=batchSize { + for start := 0; ; start += batchSize { if (start + batchSize) < len(delTopicIDs) { ids = delTopicIDs[start:(start + batchSize)] } else { From 4fb3b1aea0b03557bcfcefd3adedbbeae5f0f61b Mon Sep 17 00:00:00 2001 From: Alexey Terentyev Date: Tue, 19 Jun 2018 17:57:53 +0300 Subject: [PATCH 15/18] Fixed test for changed regexp Signed-off-by: Alexey Terentyev --- models/topic_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/models/topic_test.go b/models/topic_test.go index 93137fd7224ee..d537e1aeef392 100644 --- a/models/topic_test.go +++ b/models/topic_test.go @@ -57,11 +57,11 @@ func TestAddTopic(t *testing.T) { } func TestTopicValidator(t *testing.T) { - assert.True(t, TopicValidator("first-topic")) - assert.True(t, TopicValidator("#second-test_topic")) - assert.True(t, TopicValidator("third+project+topic.with+max_length")) + assert.True(t, TopicValidator("first")) + assert.True(t, TopicValidator("second-test-topic")) + assert.True(t, TopicValidator("third-project-topic-with-max-length")) - assert.False(t, TopicValidator("$fourth-topic")) - assert.False(t, TopicValidator("#fifth,test;topic")) - assert.False(t, TopicValidator("#sixth-go+project.topic+with+excess_length")) + assert.False(t, TopicValidator("$fourth-test,topic")) + assert.False(t, TopicValidator("-fifth-test-topic")) + assert.False(t, TopicValidator("sixth-go-project-topic-with-excess-length")) } From 8c51851b80ce1ece63951c1749108eb43b120a94 Mon Sep 17 00:00:00 2001 From: Alexey Terentyev Date: Tue, 19 Jun 2018 18:29:44 +0300 Subject: [PATCH 16/18] Removed validation log messages Signed-off-by: Alexey Terentyev --- models/topic_test.go | 3 +++ routers/repo/topic.go | 2 -- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/models/topic_test.go b/models/topic_test.go index d537e1aeef392..8ed415899d4c6 100644 --- a/models/topic_test.go +++ b/models/topic_test.go @@ -57,6 +57,9 @@ func TestAddTopic(t *testing.T) { } func TestTopicValidator(t *testing.T) { + assert.True(t, TopicValidator("12345")) + assert.True(t, TopicValidator("2-test")) + assert.True(t, TopicValidator("test-3")) assert.True(t, TopicValidator("first")) assert.True(t, TopicValidator("second-test-topic")) assert.True(t, TopicValidator("third-project-topic-with-max-length")) diff --git a/routers/repo/topic.go b/routers/repo/topic.go index 82df2a49db226..20e15735fd5ce 100644 --- a/routers/repo/topic.go +++ b/routers/repo/topic.go @@ -43,7 +43,6 @@ func TopicsPost(ctx *context.Context) { topics = topics[:i] if len(topics) > 25 { - log.Error(2, "Incorrect number of topics(max 25)") ctx.JSON(422, map[string]interface{}{ "invalidTopics": topics[:0], "message": ctx.Tr("repo.topic.count_prompt"), @@ -52,7 +51,6 @@ func TopicsPost(ctx *context.Context) { } if len(invalidTopics) > 0 { - log.Error(2, "Invalid topics: %v", invalidTopics) ctx.JSON(422, map[string]interface{}{ "invalidTopics": invalidTopics, "message": ctx.Tr("repo.topic.format_prompt"), From ffc96b01c602a95761be3de0d71233017eb5de08 Mon Sep 17 00:00:00 2001 From: Alexey Terentyev Date: Wed, 20 Jun 2018 04:26:08 +0300 Subject: [PATCH 17/18] Renamed migration file Signed-off-by: Alexey Terentyev --- models/migrations/migrations.go | 2 +- models/migrations/{v67.go => v68.go} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename models/migrations/{v67.go => v68.go} (100%) diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 7eec39b36533f..858623c9d6398 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -186,7 +186,7 @@ var migrations = []Migration{ NewMigration("add u2f", addU2FReg), // v66 -> v67 NewMigration("add login source id column for public_key table", addLoginSourceIDToPublicKeyTable), - // v67 -> V68 + // v68 -> V69 NewMigration("Reformat and remove incorrect topics", reformatAndRemoveIncorrectTopics), } diff --git a/models/migrations/v67.go b/models/migrations/v68.go similarity index 100% rename from models/migrations/v67.go rename to models/migrations/v68.go From b166e695cc21cb66c6d17c7365e2398f0ab103d3 Mon Sep 17 00:00:00 2001 From: Alexey Terentyev Date: Wed, 20 Jun 2018 12:50:36 +0300 Subject: [PATCH 18/18] Renamed validate function Signed-off-by: Alexey Terentyev --- models/migrations/v68.go | 4 ++-- models/topic.go | 4 ++-- models/topic_test.go | 18 +++++++++--------- routers/repo/topic.go | 2 +- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/models/migrations/v68.go b/models/migrations/v68.go index 4fb758a8585b6..d6a0d04c537d4 100644 --- a/models/migrations/v68.go +++ b/models/migrations/v68.go @@ -42,7 +42,7 @@ func reformatAndRemoveIncorrectTopics(x *xorm.Engine) (err error) { break } for _, topic := range topics { - if models.TopicValidator(topic.Name) { + if models.ValidateTopic(topic.Name) { continue } topic.Name = strings.Replace(strings.TrimSpace(strings.ToLower(topic.Name)), " ", "-", -1) @@ -55,7 +55,7 @@ func reformatAndRemoveIncorrectTopics(x *xorm.Engine) (err error) { touchedRepo[id] = struct{}{} } - if models.TopicValidator(topic.Name) { + if models.ValidateTopic(topic.Name) { log.Info("Updating topic: id = %v, name = %v", topic.ID, topic.Name) if _, err := sess.Table("topic").ID(topic.ID). Update(&Topic{Name: topic.Name}); err != nil { diff --git a/models/topic.go b/models/topic.go index a2d87099f4961..247aac5fffe4e 100644 --- a/models/topic.go +++ b/models/topic.go @@ -54,8 +54,8 @@ func (err ErrTopicNotExist) Error() string { return fmt.Sprintf("topic is not exist [name: %s]", err.Name) } -// TopicValidator checks topics by length and match pattern rules -func TopicValidator(topic string) bool { +// ValidateTopic checks topics by length and match pattern rules +func ValidateTopic(topic string) bool { return len(topic) <= 35 && topicPattern.MatchString(topic) } diff --git a/models/topic_test.go b/models/topic_test.go index 8ed415899d4c6..ef374e557bba0 100644 --- a/models/topic_test.go +++ b/models/topic_test.go @@ -57,14 +57,14 @@ func TestAddTopic(t *testing.T) { } func TestTopicValidator(t *testing.T) { - assert.True(t, TopicValidator("12345")) - assert.True(t, TopicValidator("2-test")) - assert.True(t, TopicValidator("test-3")) - assert.True(t, TopicValidator("first")) - assert.True(t, TopicValidator("second-test-topic")) - assert.True(t, TopicValidator("third-project-topic-with-max-length")) + assert.True(t, ValidateTopic("12345")) + assert.True(t, ValidateTopic("2-test")) + assert.True(t, ValidateTopic("test-3")) + assert.True(t, ValidateTopic("first")) + assert.True(t, ValidateTopic("second-test-topic")) + assert.True(t, ValidateTopic("third-project-topic-with-max-length")) - assert.False(t, TopicValidator("$fourth-test,topic")) - assert.False(t, TopicValidator("-fifth-test-topic")) - assert.False(t, TopicValidator("sixth-go-project-topic-with-excess-length")) + assert.False(t, ValidateTopic("$fourth-test,topic")) + assert.False(t, ValidateTopic("-fifth-test-topic")) + assert.False(t, ValidateTopic("sixth-go-project-topic-with-excess-length")) } diff --git a/routers/repo/topic.go b/routers/repo/topic.go index 20e15735fd5ce..63fcf793f98a6 100644 --- a/routers/repo/topic.go +++ b/routers/repo/topic.go @@ -36,7 +36,7 @@ func TopicsPost(ctx *context.Context) { topics[i] = topic i++ } - if !models.TopicValidator(topic) { + if !models.ValidateTopic(topic) { invalidTopics = append(invalidTopics, topic) } }