From d3f850cc0e791fa5ee5b25d824c475505fc12444 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Sat, 19 Nov 2022 23:22:15 +0800 Subject: [PATCH] Support comma-delimited string as labels in issue template (#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: image Fixes #17877. --- modules/issue/template/template.go | 4 +- modules/issue/template/template_test.go | 326 ++++++++++++++++-------- modules/issue/template/unmarshal.go | 2 +- modules/markup/markdown/meta_test.go | 48 ++-- modules/structs/issue.go | 53 +++- modules/structs/issue_test.go | 63 +++++ templates/swagger/v1_json.tmpl | 13 +- 7 files changed, 369 insertions(+), 140 deletions(-) diff --git a/modules/issue/template/template.go b/modules/issue/template/template.go index 3b33852cb5..0bdf5a1987 100644 --- 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 } diff --git a/modules/issue/template/template_test.go b/modules/issue/template/template_test.go index 883e1e0780..c3863a64a6 100644 --- 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", - }, - }, - { - 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, - }, - }, - { - 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: "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", +`, + 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", }, }, - Validations: map[string]interface{}{ - "required": true, + { + 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, + }, }, - }, - { - 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}, + { + 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: "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, + }, + }, + { + 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}, + }, }, }, }, + 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: "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", + }, + }, + }, + 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", + }, + }, + }, + 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", + }, + 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) { diff --git a/modules/issue/template/unmarshal.go b/modules/issue/template/unmarshal.go index 24587b0fed..3398719cf6 100644 --- 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, diff --git a/modules/markup/markdown/meta_test.go b/modules/markup/markdown/meta_test.go index 720d0066f4..1e9768e618 100644 --- 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", diff --git a/modules/structs/issue.go b/modules/structs/issue.go index 25c6251fbf..45c3f6294a 100644 --- 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 diff --git a/modules/structs/issue_test.go b/modules/structs/issue_test.go index 5312585d0f..72b40f7cf2 100644 --- 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) + } + }) + } +} diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index fe3185ea77..ddafc146a1 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -16818,11 +16818,7 @@ "x-go-name": "FileName" }, "labels": { - "type": "array", - "items": { - "type": "string" - }, - "x-go-name": "Labels" + "$ref": "#/definitions/IssueTemplateLabels" }, "name": { "type": "string", @@ -16839,6 +16835,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",