Topics added using API don't display on explore page

[mooror]

• Gitea version: 1.12.3
• Git version: 2.7.4
• Operating system: Ubuntu 16.04 (xenial) LTS
• Database (use [x]):
  • PostgreSQL
  • MySQL
  • MSSQL
  • SQLite
• Can you reproduce the bug at https://try.gitea.io:
  • Yes (provide example URL)
  • No
  • Not relevant

Description

If you use the /repos/{owner}/{repo}/topics/{topic} API route to add a topic to a repository it will not be displayed on the explore page, but will show up properly on the repository page.

Steps to reproduce

1. Go to the API route on your Gitea installation (/api/swagger)
2. Click the Authorize button and provide some form of authentication (if the repository is private)
3. Expand the "Add a topic to a repository" section (under the "Repository" heading)
4. Click the "Try it out" button, fill in the fields, and click "Execute"
5. Go to the explore page and find the repository that you have added a topic to (notice that the topic is not displayed)
6. Click on the repository to go to its repository page (notice that the topics were added successfully)

Screenshots

[mooror]

This issue does not apply to the the /repos/{owner}/{repo}/topics API route. If you use that route to update the entire topic list for a repository the new topics will show up on the explore page after a page refresh.

[mooror]

I also just confirmed that this problem occurs with repositories that have previously added topics.

Previously I had only tested this with repositories that didn't have any topics at all.

[mooror]

Another interesting thing to note, if you add a topic using the web interface or using the /repos/{owner}/{repo}/topics API route the "last updated" time gets changed which will bump the repository to the top of the explore page. However, using the /repos/{owner}/{repo}/topics/{topic} API route will add a new topic to the list but will NOT change the "last updated" value or bump the repository up to the top of the explore page.

[jolheiser]

I think I've figured out what's happening. Topics can be manipulated with multiple funcs, but not all are equal. 😅

Using the topics endpoint or the UI will invoke

gitea/models/topic.go

Lines 242 to 318 in 502e38c

	// SaveTopics save topics to a repository
	func SaveTopics(repoID int64, topicNames ...string) error {
	topics, err := FindTopics(&FindTopicOptions{
	RepoID: repoID,
	})
	if err != nil {
	return err
	}
	sess := x.NewSession()
	defer sess.Close()
	if err := sess.Begin(); err != nil {
	return err
	}
	var addedTopicNames []string
	for _, topicName := range topicNames {
	if strings.TrimSpace(topicName) == "" {
	continue
	}
	var found bool
	for _, t := range topics {
	if strings.EqualFold(topicName, t.Name) {
	found = true
	break
	}
	}
	if !found {
	addedTopicNames = append(addedTopicNames, topicName)
	}
	}
	var removeTopics []*Topic
	for _, t := range topics {
	var found bool
	for _, topicName := range topicNames {
	if strings.EqualFold(topicName, t.Name) {
	found = true
	break
	}
	}
	if !found {
	removeTopics = append(removeTopics, t)
	}
	}
	for _, topicName := range addedTopicNames {
	_, err := addTopicByNameToRepo(sess, repoID, topicName)
	if err != nil {
	return err
	}
	}
	for _, topic := range removeTopics {
	err := removeTopicFromRepo(sess, repoID, topic)
	if err != nil {
	return err
	}
	}
	topicNames = make([]string, 0, 25)
	if err := sess.Table("topic").Cols("name").
	Join("INNER", "repo_topic", "repo_topic.topic_id = topic.id").
	Where("repo_topic.repo_id = ?", repoID).Desc("topic.repo_count").Find(&topicNames); err != nil {
	return err
	}
	if _, err := sess.ID(repoID).Cols("topics").Update(&Repository{
	Topics: topicNames,
	}); err != nil {
	return err
	}
	return sess.Commit()
	}

Whereas the API invokes

gitea/models/topic.go

Lines 212 to 224 in 502e38c

	// AddTopic adds a topic name to a repository (if it does not already have it)
	func AddTopic(repoID int64, topicName string) (*Topic, error) {
	topic, err := GetRepoTopicByName(repoID, topicName)
	if err != nil {
	return nil, err
	}
	if topic != nil {
	// Repo already have topic
	return topic, nil
	}
	return addTopicByNameToRepo(x, repoID, topicName)
	}

gitea/models/topic.go

Lines 100 to 129 in 502e38c

	// addTopicByNameToRepo adds a topic name to a repo and increments the topic count.
	// Returns topic after the addition
	func addTopicByNameToRepo(e Engine, repoID int64, topicName string) (*Topic, error) {
	var topic Topic
	has, err := e.Where("name = ?", topicName).Get(&topic)
	if err != nil {
	return nil, err
	}
	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
	}

The top code invokes the bottom, but also seems to update the repo table as well, because we store topics on the repo table as well as the topics table. (Presumably to save on queries later??)

[mooror]

@jolheiser Hmm, I see what you mean. That's tricky. Ideally you'd have one function to rule them all (Lord of the Rings style 😉 ). Do you think this will just be a simple function call replacement? Or are there other complications?

[jolheiser]

@mooror This should be easy to fix. Unfortunately we'll need to retrieve the topics for a repo, add/delete a topic, then serialize it back to the DB unless there's more context further up.

[mooror]

Unfortunately we'll need to retrieve the topics for a repo, add/delete a topic, then serialize it back to the DB unless there's more context further up.

Are you saying we will have to up the number of database queries for this API route?

[jolheiser]

At a glance I think so. Unless we have the data available earlier in the code. I've only taken a glance so far.

[mooror]

@jolheiser In this case, you gotta do what you gotta do I suppose.

Also I wish I could be of more use to you in fixing this issue but I haven't fully learned GO lang quite yet. Its on the list 😄

[zeripath]

@jolheiser:

Why do we still have the this Topics field?

If we still need it then I think:

diff --git a/models/topic.go b/models/topic.go
index 4a5bffa08..7f7c7cb9a 100644
--- a/models/topic.go
+++ b/models/topic.go
@@ -197,10 +197,13 @@ func FindTopics(opts *FindTopicOptions) (topics []*Topic, err error) {
 
 // GetRepoTopicByName retrives topic from name for a repo if it exist
 func GetRepoTopicByName(repoID int64, topicName string) (*Topic, error) {
+	return getRepoTopicByName(x, repoID, topicName)
+}
+func getRepoTopicByName(e Engine, 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 := e.Table("topic").Where(cond)
 	sess.Join("INNER", "repo_topic", "repo_topic.topic_id = topic.id")
 	has, err := sess.Get(&topic)
 	if has {
@@ -211,7 +214,12 @@ func GetRepoTopicByName(repoID int64, topicName string) (*Topic, error) {
 
 // AddTopic adds a topic name to a repository (if it does not already have it)
 func AddTopic(repoID int64, topicName string) (*Topic, error) {
-	topic, err := GetRepoTopicByName(repoID, topicName)
+	sess := x.NewSession()
+	if err := sess.Begin(); err != nil {
+		return nil, err
+	}
+
+	topic, err := getRepoTopicByName(sess, repoID, topicName)
 	if err != nil {
 		return nil, err
 	}
@@ -220,7 +228,25 @@ func AddTopic(repoID int64, topicName string) (*Topic, error) {
 		return topic, nil
 	}
 
-	return addTopicByNameToRepo(x, repoID, topicName)
+	topic, err = addTopicByNameToRepo(sess, repoID, topicName)
+	if err != nil {
+		return nil, err
+	}
+
+	topicNames := make([]string, 0, 25)
+	if err := sess.Table("topic").Cols("name").
+		Join("INNER", "repo_topic", "repo_topic.topic_id = topic.id").
+		Where("repo_topic.repo_id = ?", repoID).Desc("topic.repo_count").Find(&topicNames); err != nil {
+		return nil, err
+	}
+
+	if _, err := sess.ID(repoID).Cols("topics").Update(&Repository{
+		Topics: topicNames,
+	}); err != nil {
+		return nil, err
+	}
+
+	return topic, sess.Commit()
 }
 
 // DeleteTopic removes a topic name from a repository (if it has it)

would probably fix this but we'd also need a migration to ensure that the repository.Topics are up-to-date.

That would probably need to use something like SQLITEs GROUP_CONCAT to set the Topics.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. I am here to help clear issues left open even if solved or waiting for more insight. This issue will be closed if no further activity occurs during the next 2 weeks. If the issue is still valid just add a comment to keep it alive. Thank you for your contributions.

[mooror]

Thanks @lafriks for keeping the issue from going stale

[mooror]

Thank you @zeripath for working on fixing this issue