Support comma-delimited string as labels in issue template (#21831) (#21873)

Backport #21831. The [labels in issue YAML templates](https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/syntax-for-issue-forms#top-level-syntax) can be a string array or a comma-delimited string, so a single string should be valid labels. The old codes committed in #20987 ignore this, that's why the warning is displayed: <img width="618" alt="image" src="https://user-images.githubusercontent.com/9418365/202112642-93dc72d0-71c3-40a2-9720-30fc2d48c97c.png"> Fixes #17877.
2 years ago · 7a004ad7eb
parent af8b2250c4
commit 7a004ad7eb
7 changed files with 362 additions and 133 deletions
--- a/modules/issue/template/template.go
+++ b/modules/issue/template/template.go
@ -165,7 +165,7 @@ func validateOptions(field *api.IssueFormField, idx int) error {
 				return position.Errorf("should be a string")
 			}
 		case api.IssueFormFieldTypeCheckboxes:
-			opt, ok := option.(map[interface{}]interface{})
+			opt, ok := option.(map[string]interface{})
 			if !ok {
 				return position.Errorf("should be a dictionary")
 			}
@ -351,7 +351,7 @@ func (o *valuedOption) Label() string {
 			return label
 		}
 	case api.IssueFormFieldTypeCheckboxes:
-		if vs, ok := o.data.(map[interface{}]interface{}); ok {
+		if vs, ok := o.data.(map[string]interface{}); ok {
 			if v, ok := vs["label"].(string); ok {
 				return v
 			}
--- a/modules/issue/template/template_test.go
+++ b/modules/issue/template/template_test.go
@ -6,18 +6,21 @@ package template

 import (
 	"net/url"
-	"reflect"
 	"testing"

 	"code.gitea.io/gitea/modules/json"
 	api "code.gitea.io/gitea/modules/structs"
+
+	"github.com/stretchr/testify/require"
 )

 func TestValidate(t *testing.T) {
 	tests := []struct {
-		name    string
-		content string
-		wantErr string
+		name     string
+		filename string
+		content  string
+		want     *api.IssueTemplate
+		wantErr  string
 	}{
 		{
 			name:    "miss name",
@ -316,21 +319,9 @@ body:
 `,
 			wantErr: "body[0](checkboxes), option[0]: 'required' should be a bool",
 		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			tmpl, err := unmarshal("test.yaml", []byte(tt.content))
-			if err != nil {
-				t.Fatal(err)
-			}
-			if err := Validate(tmpl); (err == nil) != (tt.wantErr == "") || err != nil && err.Error() != tt.wantErr {
-				t.Errorf("Validate() error = %v, wantErr %q", err, tt.wantErr)
-			}
-		})
-	}
-
-	t.Run("valid", func(t *testing.T) {
-		content := `
+		{
+			name: "valid",
+			content: `
 name: Name
 title: Title
 about: About
@ -386,96 +377,227 @@ body:
          required: false
        - label: Option 3 of checkboxes
          required: true
-`
-		want := &api.IssueTemplate{
-			Name:   "Name",
-			Title:  "Title",
-			About:  "About",
-			Labels: []string{"label1", "label2"},
-			Ref:    "Ref",
-			Fields: []*api.IssueFormField{
-				{
-					Type: "markdown",
-					ID:   "id1",
-					Attributes: map[string]interface{}{
-						"value": "Value of the markdown",
+`,
+			want: &api.IssueTemplate{
+				Name:   "Name",
+				Title:  "Title",
+				About:  "About",
+				Labels: []string{"label1", "label2"},
+				Ref:    "Ref",
+				Fields: []*api.IssueFormField{
+					{
+						Type: "markdown",
+						ID:   "id1",
+						Attributes: map[string]interface{}{
+							"value": "Value of the markdown",
+						},
 					},
-				},
-				{
-					Type: "textarea",
-					ID:   "id2",
-					Attributes: map[string]interface{}{
-						"label":       "Label of textarea",
-						"description": "Description of textarea",
-						"placeholder": "Placeholder of textarea",
-						"value":       "Value of textarea",
-						"render":      "bash",
+					{
+						Type: "textarea",
+						ID:   "id2",
+						Attributes: map[string]interface{}{
+							"label":       "Label of textarea",
+							"description": "Description of textarea",
+							"placeholder": "Placeholder of textarea",
+							"value":       "Value of textarea",
+							"render":      "bash",
+						},
+						Validations: map[string]interface{}{
+							"required": true,
+						},
 					},
-					Validations: map[string]interface{}{
-						"required": true,
+					{
+						Type: "input",
+						ID:   "id3",
+						Attributes: map[string]interface{}{
+							"label":       "Label of input",
+							"description": "Description of input",
+							"placeholder": "Placeholder of input",
+							"value":       "Value of input",
+						},
+						Validations: map[string]interface{}{
+							"required":  true,
+							"is_number": true,
+							"regex":     "[a-zA-Z0-9]+",
+						},
 					},
-				},
-				{
-					Type: "input",
-					ID:   "id3",
-					Attributes: map[string]interface{}{
-						"label":       "Label of input",
-						"description": "Description of input",
-						"placeholder": "Placeholder of input",
-						"value":       "Value of input",
+					{
+						Type: "dropdown",
+						ID:   "id4",
+						Attributes: map[string]interface{}{
+							"label":       "Label of dropdown",
+							"description": "Description of dropdown",
+							"multiple":    true,
+							"options": []interface{}{
+								"Option 1 of dropdown",
+								"Option 2 of dropdown",
+								"Option 3 of dropdown",
+							},
+						},
+						Validations: map[string]interface{}{
+							"required": true,
+						},
 					},
-					Validations: map[string]interface{}{
-						"required":  true,
-						"is_number": true,
-						"regex":     "[a-zA-Z0-9]+",
+					{
+						Type: "checkboxes",
+						ID:   "id5",
+						Attributes: map[string]interface{}{
+							"label":       "Label of checkboxes",
+							"description": "Description of checkboxes",
+							"options": []interface{}{
+								map[string]interface{}{"label": "Option 1 of checkboxes", "required": true},
+								map[string]interface{}{"label": "Option 2 of checkboxes", "required": false},
+								map[string]interface{}{"label": "Option 3 of checkboxes", "required": true},
+							},
+						},
 					},
 				},
-				{
-					Type: "dropdown",
-					ID:   "id4",
-					Attributes: map[string]interface{}{
-						"label":       "Label of dropdown",
-						"description": "Description of dropdown",
-						"multiple":    true,
-						"options": []interface{}{
-							"Option 1 of dropdown",
-							"Option 2 of dropdown",
-							"Option 3 of dropdown",
+				FileName: "test.yaml",
+			},
+			wantErr: "",
+		},
+		{
+			name: "single label",
+			content: `
+name: Name
+title: Title
+about: About
+labels: label1
+ref: Ref
+body:
+  - type: markdown
+    id: id1
+    attributes:
+      value: Value of the markdown
+`,
+			want: &api.IssueTemplate{
+				Name:   "Name",
+				Title:  "Title",
+				About:  "About",
+				Labels: []string{"label1"},
+				Ref:    "Ref",
+				Fields: []*api.IssueFormField{
+					{
+						Type: "markdown",
+						ID:   "id1",
+						Attributes: map[string]interface{}{
+							"value": "Value of the markdown",
 						},
 					},
-					Validations: map[string]interface{}{
-						"required": true,
+				},
+				FileName: "test.yaml",
+			},
+			wantErr: "",
+		},
+		{
+			name: "comma-delimited labels",
+			content: `
+name: Name
+title: Title
+about: About
+labels: label1,label2,,label3 ,,
+ref: Ref
+body:
+  - type: markdown
+    id: id1
+    attributes:
+      value: Value of the markdown
+`,
+			want: &api.IssueTemplate{
+				Name:   "Name",
+				Title:  "Title",
+				About:  "About",
+				Labels: []string{"label1", "label2", "label3"},
+				Ref:    "Ref",
+				Fields: []*api.IssueFormField{
+					{
+						Type: "markdown",
+						ID:   "id1",
+						Attributes: map[string]interface{}{
+							"value": "Value of the markdown",
+						},
 					},
 				},
-				{
-					Type: "checkboxes",
-					ID:   "id5",
-					Attributes: map[string]interface{}{
-						"label":       "Label of checkboxes",
-						"description": "Description of checkboxes",
-						"options": []interface{}{
-							map[interface{}]interface{}{"label": "Option 1 of checkboxes", "required": true},
-							map[interface{}]interface{}{"label": "Option 2 of checkboxes", "required": false},
-							map[interface{}]interface{}{"label": "Option 3 of checkboxes", "required": true},
+				FileName: "test.yaml",
+			},
+			wantErr: "",
+		},
+		{
+			name: "empty string as labels",
+			content: `
+name: Name
+title: Title
+about: About
+labels: ''
+ref: Ref
+body:
+  - type: markdown
+    id: id1
+    attributes:
+      value: Value of the markdown
+`,
+			want: &api.IssueTemplate{
+				Name:   "Name",
+				Title:  "Title",
+				About:  "About",
+				Labels: nil,
+				Ref:    "Ref",
+				Fields: []*api.IssueFormField{
+					{
+						Type: "markdown",
+						ID:   "id1",
+						Attributes: map[string]interface{}{
+							"value": "Value of the markdown",
 						},
 					},
 				},
+				FileName: "test.yaml",
 			},
-			FileName: "test.yaml",
-		}
-		got, err := unmarshal("test.yaml", []byte(content))
-		if err != nil {
-			t.Fatal(err)
-		}
-		if err := Validate(got); err != nil {
-			t.Errorf("Validate() error = %v", err)
-		}
-		if !reflect.DeepEqual(want, got) {
-			jsonWant, _ := json.Marshal(want)
-			jsonGot, _ := json.Marshal(got)
-			t.Errorf("want:\n%s\ngot:\n%s", jsonWant, jsonGot)
-		}
-	})
+			wantErr: "",
+		},
+		{
+			name:     "comma delimited labels in markdown",
+			filename: "test.md",
+			content: `---
+name: Name
+title: Title
+about: About
+labels: label1,label2,,label3 ,,
+ref: Ref
+---
+Content
+`,
+			want: &api.IssueTemplate{
+				Name:     "Name",
+				Title:    "Title",
+				About:    "About",
+				Labels:   []string{"label1", "label2", "label3"},
+				Ref:      "Ref",
+				Fields:   nil,
+				Content:  "Content\n",
+				FileName: "test.md",
+			},
+			wantErr: "",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			filename := "test.yaml"
+			if tt.filename != "" {
+				filename = tt.filename
+			}
+			tmpl, err := unmarshal(filename, []byte(tt.content))
+			require.NoError(t, err)
+			if tt.wantErr != "" {
+				require.EqualError(t, Validate(tmpl), tt.wantErr)
+			} else {
+				require.NoError(t, Validate(tmpl))
+				want, _ := json.Marshal(tt.want)
+				got, _ := json.Marshal(tmpl)
+				require.JSONEq(t, string(want), string(got))
+			}
+		})
+	}
 }

 func TestRenderToMarkdown(t *testing.T) {
--- a/modules/issue/template/unmarshal.go
+++ b/modules/issue/template/unmarshal.go
@ -16,7 +16,7 @@ import (
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/util"

-	"gopkg.in/yaml.v2"
+	"gopkg.in/yaml.v3"
 )

 // CouldBe indicates a file with the filename could be a template,
--- a/modules/markup/markdown/meta_test.go
+++ b/modules/markup/markdown/meta_test.go
@ -9,82 +9,86 @@ import (
 	"strings"
 	"testing"

-	"code.gitea.io/gitea/modules/structs"
-
 	"github.com/stretchr/testify/assert"
 )

-func validateMetadata(it structs.IssueTemplate) bool {
-	/*
-		A legacy to keep the unit tests working.
-		Copied from the method "func (it IssueTemplate) Valid() bool", the original method has been removed.
-		Because it becomes quite complicated to validate an issue template which is support yaml form now.
-		The new way to validate an issue template is to call the Validate in modules/issue/template,
-	*/
+/*
+IssueTemplate is a legacy to keep the unit tests working.
+Copied from structs.IssueTemplate, the original type has been changed a lot to support yaml template.
+*/
+type IssueTemplate struct {
+	Name   string   `json:"name" yaml:"name"`
+	Title  string   `json:"title" yaml:"title"`
+	About  string   `json:"about" yaml:"about"`
+	Labels []string `json:"labels" yaml:"labels"`
+	Ref    string   `json:"ref" yaml:"ref"`
+}
+
+func (it *IssueTemplate) Valid() bool {
 	return strings.TrimSpace(it.Name) != "" && strings.TrimSpace(it.About) != ""
 }

 func TestExtractMetadata(t *testing.T) {
 	t.Run("ValidFrontAndBody", func(t *testing.T) {
-		var meta structs.IssueTemplate
+		var meta IssueTemplate
 		body, err := ExtractMetadata(fmt.Sprintf("%s\n%s\n%s\n%s", sepTest, frontTest, sepTest, bodyTest), &meta)
 		assert.NoError(t, err)
 		assert.Equal(t, bodyTest, body)
 		assert.Equal(t, metaTest, meta)
-		assert.True(t, validateMetadata(meta))
+		assert.True(t, meta.Valid())
 	})

 	t.Run("NoFirstSeparator", func(t *testing.T) {
-		var meta structs.IssueTemplate
+		var meta IssueTemplate
 		_, err := ExtractMetadata(fmt.Sprintf("%s\n%s\n%s", frontTest, sepTest, bodyTest), &meta)
 		assert.Error(t, err)
 	})

 	t.Run("NoLastSeparator", func(t *testing.T) {
-		var meta structs.IssueTemplate
+		var meta IssueTemplate
 		_, err := ExtractMetadata(fmt.Sprintf("%s\n%s\n%s", sepTest, frontTest, bodyTest), &meta)
 		assert.Error(t, err)
 	})

 	t.Run("NoBody", func(t *testing.T) {
-		var meta structs.IssueTemplate
+		var meta IssueTemplate
 		body, err := ExtractMetadata(fmt.Sprintf("%s\n%s\n%s", sepTest, frontTest, sepTest), &meta)
 		assert.NoError(t, err)
 		assert.Equal(t, "", body)
 		assert.Equal(t, metaTest, meta)
-		assert.True(t, validateMetadata(meta))
+		assert.True(t, meta.Valid())
 	})
 }

 func TestExtractMetadataBytes(t *testing.T) {
 	t.Run("ValidFrontAndBody", func(t *testing.T) {
-		var meta structs.IssueTemplate
+		var meta IssueTemplate
 		body, err := ExtractMetadataBytes([]byte(fmt.Sprintf("%s\n%s\n%s\n%s", sepTest, frontTest, sepTest, bodyTest)), &meta)
 		assert.NoError(t, err)
 		assert.Equal(t, bodyTest, string(body))
 		assert.Equal(t, metaTest, meta)
-		assert.True(t, validateMetadata(meta))
+		assert.True(t, meta.Valid())
 	})

 	t.Run("NoFirstSeparator", func(t *testing.T) {
-		var meta structs.IssueTemplate
+		var meta IssueTemplate
 		_, err := ExtractMetadataBytes([]byte(fmt.Sprintf("%s\n%s\n%s", frontTest, sepTest, bodyTest)), &meta)
 		assert.Error(t, err)
 	})

 	t.Run("NoLastSeparator", func(t *testing.T) {
-		var meta structs.IssueTemplate
+		var meta IssueTemplate
 		_, err := ExtractMetadataBytes([]byte(fmt.Sprintf("%s\n%s\n%s", sepTest, frontTest, bodyTest)), &meta)
 		assert.Error(t, err)
 	})

 	t.Run("NoBody", func(t *testing.T) {
-		var meta structs.IssueTemplate
+		var meta IssueTemplate
 		body, err := ExtractMetadataBytes([]byte(fmt.Sprintf("%s\n%s\n%s", sepTest, frontTest, sepTest)), &meta)
 		assert.NoError(t, err)
 		assert.Equal(t, "", string(body))
 		assert.Equal(t, metaTest, meta)
-		assert.True(t, validateMetadata(meta))
+		assert.True(t, meta.Valid())
 	})
 }

@ -97,7 +101,7 @@ labels:
  - bug
  - "test label"`
 	bodyTest = "This is the body"
-	metaTest = structs.IssueTemplate{
+	metaTest = IssueTemplate{
 		Name:   "Test",
 		About:  "A Test",
 		Title:  "Test Title",
--- a/modules/structs/issue.go
+++ b/modules/structs/issue.go
@ -5,8 +5,12 @@
 package structs

 import (
+	"fmt"
 	"path"
+	"strings"
 	"time"
+
+	"gopkg.in/yaml.v3"
 )

 // StateType issue state type
@ -143,14 +147,47 @@ type IssueFormField struct {
 // IssueTemplate represents an issue template for a repository
 // swagger:model
 type IssueTemplate struct {
-	Name     string            `json:"name" yaml:"name"`
-	Title    string            `json:"title" yaml:"title"`
-	About    string            `json:"about" yaml:"about"` // Using "description" in a template file is compatible
-	Labels   []string          `json:"labels" yaml:"labels"`
-	Ref      string            `json:"ref" yaml:"ref"`
-	Content  string            `json:"content" yaml:"-"`
-	Fields   []*IssueFormField `json:"body" yaml:"body"`
-	FileName string            `json:"file_name" yaml:"-"`
+	Name     string              `json:"name" yaml:"name"`
+	Title    string              `json:"title" yaml:"title"`
+	About    string              `json:"about" yaml:"about"` // Using "description" in a template file is compatible
+	Labels   IssueTemplateLabels `json:"labels" yaml:"labels"`
+	Ref      string              `json:"ref" yaml:"ref"`
+	Content  string              `json:"content" yaml:"-"`
+	Fields   []*IssueFormField   `json:"body" yaml:"body"`
+	FileName string              `json:"file_name" yaml:"-"`
+}
+
+type IssueTemplateLabels []string
+
+func (l *IssueTemplateLabels) UnmarshalYAML(value *yaml.Node) error {
+	var labels []string
+	if value.IsZero() {
+		*l = labels
+		return nil
+	}
+	switch value.Kind {
+	case yaml.ScalarNode:
+		str := ""
+		err := value.Decode(&str)
+		if err != nil {
+			return err
+		}
+		for _, v := range strings.Split(str, ",") {
+			if v = strings.TrimSpace(v); v == "" {
+				continue
+			}
+			labels = append(labels, v)
+		}
+		*l = labels
+		return nil
+	case yaml.SequenceNode:
+		if err := value.Decode(&labels); err != nil {
+			return err
+		}
+		*l = labels
+		return nil
+	}
+	return fmt.Errorf("line %d: cannot unmarshal %s into IssueTemplateLabels", value.Line, value.ShortTag())
 }

 // IssueTemplateType defines issue template type
--- a/modules/structs/issue_test.go
+++ b/modules/structs/issue_test.go
@ -8,6 +8,7 @@ import (
 	"testing"

 	"github.com/stretchr/testify/assert"
+	"gopkg.in/yaml.v3"
 )

 func TestIssueTemplate_Type(t *testing.T) {
@ -41,3 +42,65 @@ func TestIssueTemplate_Type(t *testing.T) {
 		})
 	}
 }
+
+func TestIssueTemplateLabels_UnmarshalYAML(t *testing.T) {
+	tests := []struct {
+		name    string
+		content string
+		tmpl    *IssueTemplate
+		want    *IssueTemplate
+		wantErr string
+	}{
+		{
+			name:    "array",
+			content: `labels: ["a", "b", "c"]`,
+			tmpl: &IssueTemplate{
+				Labels: []string{"should_be_overwrote"},
+			},
+			want: &IssueTemplate{
+				Labels: []string{"a", "b", "c"},
+			},
+		},
+		{
+			name:    "string",
+			content: `labels: "a,b,c"`,
+			tmpl: &IssueTemplate{
+				Labels: []string{"should_be_overwrote"},
+			},
+			want: &IssueTemplate{
+				Labels: []string{"a", "b", "c"},
+			},
+		},
+		{
+			name:    "empty",
+			content: `labels:`,
+			tmpl: &IssueTemplate{
+				Labels: []string{"should_be_overwrote"},
+			},
+			want: &IssueTemplate{
+				Labels: nil,
+			},
+		},
+		{
+			name: "error",
+			content: `
+labels:
+  a: aa
+  b: bb
+`,
+			tmpl:    &IssueTemplate{},
+			wantErr: "line 3: cannot unmarshal !!map into IssueTemplateLabels",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := yaml.Unmarshal([]byte(tt.content), tt.tmpl)
+			if tt.wantErr != "" {
+				assert.EqualError(t, err, tt.wantErr)
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, tt.want, tt.tmpl)
+			}
+		})
+	}
+}
--- a/templates/swagger/v1_json.tmpl
+++ b/templates/swagger/v1_json.tmpl
@ -16806,11 +16806,7 @@
          "x-go-name": "FileName"
        },
        "labels": {
-          "type": "array",
-          "items": {
-            "type": "string"
-          },
-          "x-go-name": "Labels"
+          "$ref": "#/definitions/IssueTemplateLabels"
        },
        "name": {
          "type": "string",
@ -16827,6 +16823,13 @@
      },
      "x-go-package": "code.gitea.io/gitea/modules/structs"
    },
+    "IssueTemplateLabels": {
+      "type": "array",
+      "items": {
+        "type": "string"
+      },
+      "x-go-package": "code.gitea.io/gitea/modules/structs"
+    },
    "Label": {
      "description": "Label a label to an issue or a pr",
      "type": "object",