From 3fd17a6d4baab14ccb77d4fa211438bc5028794e Mon Sep 17 00:00:00 2001 From: Carlos Treminio Date: Sat, 12 Oct 2024 21:52:55 -0600 Subject: [PATCH] :bug: Fixed the Issue.Move() field injection (#328) --- jira/internal/issue_impl_adf.go | 57 +++++-------------- jira/internal/issue_impl_adf_test.go | 65 +++++++++++++++------- jira/internal/issue_impl_rich_text.go | 57 +++++-------------- jira/internal/issue_impl_rich_text_test.go | 60 ++++++++++++++------ pkg/infra/models/jira_issue_v2.go | 11 +++- pkg/infra/models/jira_issue_v2_test.go | 44 --------------- pkg/infra/models/jira_issue_v3.go | 23 +++++--- pkg/infra/models/jira_issue_v3_test.go | 44 --------------- pkg/infra/models/jira_resolution.go | 8 +-- 9 files changed, 142 insertions(+), 227 deletions(-) diff --git a/jira/internal/issue_impl_adf.go b/jira/internal/issue_impl_adf.go index a28bcbb8..ff7462cc 100644 --- a/jira/internal/issue_impl_adf.go +++ b/jira/internal/issue_impl_adf.go @@ -369,62 +369,33 @@ func (i *internalIssueADFServiceImpl) Move(ctx context.Context, issueKeyOrID, tr return nil, model.ErrNoTransitionID } - payloadUpdated := make(map[string]interface{}) - payloadUpdated["transition"] = map[string]interface{}{"id": transitionID} + payload := map[string]interface{}{"transition": map[string]interface{}{"id": transitionID}} - // Process logic only if the transition options are provided if options != nil { - if options.Fields == nil { return nil, model.ErrNoIssueScheme } - withCustomFields := options.CustomFields != nil - withOperations := options.Operations != nil - - var err error - - // Executed when the customfields and operations are provided - if withCustomFields && withOperations { - - payloadUpdated, err = options.Fields.MergeCustomFields(options.CustomFields) - if err != nil { - return nil, err - } - - payloadWithOperations, err := options.Fields.MergeOperations(options.Operations) - if err != nil { - return nil, err - } - - if err = mergo.Map(&payloadUpdated, &payloadWithOperations, mergo.WithOverride); err != nil { - return nil, err - } + // Merge the customfields and operations + payloadWithFields, err := options.Fields.MergeCustomFields(options.CustomFields) + if err != nil { + return nil, err } - - // Executed when only the customfields are provided, but not the operations - if withCustomFields && !withOperations { - - payloadUpdated, err = options.Fields.MergeCustomFields(options.CustomFields) - if err != nil { - return nil, err - } + if err = mergo.Map(&payload, &payloadWithFields, mergo.WithOverride); err != nil { + return nil, err } - // Executed when only the operations are provided, but not the customfields - if withOperations && !withCustomFields { - - payloadUpdated, err = options.Fields.MergeOperations(options.Operations) - if err != nil { - return nil, err - } + payloadWithOperation, err := options.Fields.MergeOperations(options.Operations) + if err != nil { + return nil, err + } + if err = mergo.Map(&payload, &payloadWithOperation, mergo.WithOverride); err != nil { + return nil, err } - } endpoint := fmt.Sprintf("rest/api/%v/issue/%v/transitions", i.version, issueKeyOrID) - - request, err := i.c.NewRequest(ctx, http.MethodPost, endpoint, "", payloadUpdated) + request, err := i.c.NewRequest(ctx, http.MethodPost, endpoint, "", payload) if err != nil { return nil, err } diff --git a/jira/internal/issue_impl_adf_test.go b/jira/internal/issue_impl_adf_test.go index 34b78eb1..bda19e4b 100644 --- a/jira/internal/issue_impl_adf_test.go +++ b/jira/internal/issue_impl_adf_test.go @@ -3,6 +3,7 @@ package internal import ( "context" "errors" + "github.com/stretchr/testify/mock" "net/http" "testing" @@ -1007,38 +1008,57 @@ func Test_internalIssueADFServiceImpl_Get(t *testing.T) { func Test_internalIssueADFServiceImpl_Move(t *testing.T) { + /* + "customfield_10042": 1000.2222, + "customfield_10052": [ + { + "name": "jira-administrators" + }, + { + "name": "jira-administrators-system" + } + ] + */ customFieldsMocked := &model.CustomFields{} - - err := customFieldsMocked.Groups("customfield_10052", []string{"jira-administrators", "jira-administrators-system"}) - if err != nil { + if err := customFieldsMocked.Groups("customfield_10052", []string{"jira-administrators", "jira-administrators-system"}); err != nil { t.Fatal(err) } - - err = customFieldsMocked.Number("customfield_10042", 1000.2222) - if err != nil { + if err := customFieldsMocked.Number("customfield_10042", 1000.2222); err != nil { t.Fatal(err) } + /* + "update": { + "labels": [ + { + "remove": "triaged" + } + ] + } + */ operationsMocked := &model.UpdateOperations{} - err = operationsMocked.AddArrayOperation("labels", map[string]string{ - "triaged": "remove", - }) + if err := operationsMocked.AddArrayOperation("labels", map[string]string{"triaged": "remove"}); err != nil { + t.Fatal(err) + } expectedPayloadWithCustomFieldsAndOperations := map[string]interface{}{ - "fields": map[string]interface{}{ "customfield_10042": 1000.2222, "customfield_10052": []map[string]interface{}{map[string]interface{}{ "name": "jira-administrators"}, map[string]interface{}{ "name": "jira-administrators-system"}}, - "issuetype": map[string]interface{}{"name": "Story"}, - "project": map[string]interface{}{"id": "10000"}, - "summary": "New summary test"}, + "issuetype": map[string]interface{}{"name": "Story"}, + "project": map[string]interface{}{"id": "10000"}, + "resolution": map[string]interface{}{"name": "Done"}, + "summary": "New summary test"}, "update": map[string]interface{}{ "labels": []map[string]interface{}{map[string]interface{}{ - "remove": "triaged"}}}} + "remove": "triaged"}}}, + + "transition": map[string]interface{}{"id": "10001"}, + } expectedPayloadWithCustomfields := map[string]interface{}{ "fields": map[string]interface{}{ @@ -1049,7 +1069,9 @@ func Test_internalIssueADFServiceImpl_Move(t *testing.T) { "issuetype": map[string]interface{}{"name": "Story"}, "project": map[string]interface{}{"id": "10000"}, - "summary": "New summary test"}} + "summary": "New summary test"}, + "transition": map[string]interface{}{"id": "10001"}, + } expectedPayloadWithOperations := map[string]interface{}{ "fields": map[string]interface{}{ @@ -1059,14 +1081,12 @@ func Test_internalIssueADFServiceImpl_Move(t *testing.T) { "update": map[string]interface{}{ "labels": []map[string]interface{}{map[string]interface{}{ - "remove": "triaged"}}}} + "remove": "triaged"}}}, + "transition": map[string]interface{}{"id": "10001"}, + } expectedPayloadWithNoOptions := map[string]interface{}{"transition": map[string]interface{}{"id": "10001"}} - if err != nil { - t.Fatal(err) - } - type fields struct { c service.Connector version string @@ -1099,6 +1119,9 @@ func Test_internalIssueADFServiceImpl_Move(t *testing.T) { Summary: "New summary test", Project: &model.ProjectScheme{ID: "10000"}, IssueType: &model.IssueTypeScheme{Name: "Story"}, + Resolution: &model.ResolutionScheme{ + Name: "Done", + }, }, }, CustomFields: customFieldsMocked, @@ -1332,7 +1355,7 @@ func Test_internalIssueADFServiceImpl_Move(t *testing.T) { http.MethodPost, "rest/api/3/issue/DUMMY-1/transitions", "", - expectedPayloadWithCustomFieldsAndOperations). + mock.Anything). Return(&http.Request{}, errors.New("error, unable to create the http request")) fields.c = client diff --git a/jira/internal/issue_impl_rich_text.go b/jira/internal/issue_impl_rich_text.go index 5e57e874..90d0692e 100644 --- a/jira/internal/issue_impl_rich_text.go +++ b/jira/internal/issue_impl_rich_text.go @@ -369,62 +369,33 @@ func (i *internalRichTextServiceImpl) Move(ctx context.Context, issueKeyOrID, tr return nil, model.ErrNoTransitionID } - payloadUpdated := make(map[string]interface{}) - payloadUpdated["transition"] = map[string]interface{}{"id": transitionID} + payload := map[string]interface{}{"transition": map[string]interface{}{"id": transitionID}} - // Process logic only if the transition options are provided if options != nil { - if options.Fields == nil { return nil, model.ErrNoIssueScheme } - withCustomFields := options.CustomFields != nil - withOperations := options.Operations != nil - - var err error - - // Executed when the customfields and operations are provided - if withCustomFields && withOperations { - - payloadUpdated, err = options.Fields.MergeCustomFields(options.CustomFields) - if err != nil { - return nil, err - } - - payloadWithOperations, err := options.Fields.MergeOperations(options.Operations) - if err != nil { - return nil, err - } - - if err = mergo.Map(&payloadUpdated, &payloadWithOperations, mergo.WithOverride); err != nil { - return nil, err - } + // Merge the customfields and operations + payloadWithFields, err := options.Fields.MergeCustomFields(options.CustomFields) + if err != nil { + return nil, err } - - // Executed when only the customfields are provided, but not the operations - if withCustomFields && !withOperations { - - payloadUpdated, err = options.Fields.MergeCustomFields(options.CustomFields) - if err != nil { - return nil, err - } + if err = mergo.Map(&payload, &payloadWithFields, mergo.WithOverride); err != nil { + return nil, err } - // Executed when only the operations are provided, but not the customfields - if withOperations && !withCustomFields { - - payloadUpdated, err = options.Fields.MergeOperations(options.Operations) - if err != nil { - return nil, err - } + payloadWithOperation, err := options.Fields.MergeOperations(options.Operations) + if err != nil { + return nil, err + } + if err = mergo.Map(&payload, &payloadWithOperation, mergo.WithOverride); err != nil { + return nil, err } - } endpoint := fmt.Sprintf("rest/api/%v/issue/%v/transitions", i.version, issueKeyOrID) - - request, err := i.c.NewRequest(ctx, http.MethodPost, endpoint, "", payloadUpdated) + request, err := i.c.NewRequest(ctx, http.MethodPost, endpoint, "", payload) if err != nil { return nil, err } diff --git a/jira/internal/issue_impl_rich_text_test.go b/jira/internal/issue_impl_rich_text_test.go index dcbf4afc..ad0cffc5 100644 --- a/jira/internal/issue_impl_rich_text_test.go +++ b/jira/internal/issue_impl_rich_text_test.go @@ -3,6 +3,7 @@ package internal import ( "context" "errors" + "github.com/stretchr/testify/mock" "net/http" "testing" @@ -1007,41 +1008,57 @@ func Test_internalRichTextServiceImpl_Get(t *testing.T) { func Test_internalRichTextServiceImpl_Move(t *testing.T) { + /* + "customfield_10042": 1000.2222, + "customfield_10052": [ + { + "name": "jira-administrators" + }, + { + "name": "jira-administrators-system" + } + ] + */ customFieldsMocked := &model.CustomFields{} - - err := customFieldsMocked.Groups("customfield_10052", []string{"jira-administrators", "jira-administrators-system"}) - if err != nil { + if err := customFieldsMocked.Groups("customfield_10052", []string{"jira-administrators", "jira-administrators-system"}); err != nil { t.Fatal(err) } - - err = customFieldsMocked.Number("customfield_10042", 1000.2222) - if err != nil { + if err := customFieldsMocked.Number("customfield_10042", 1000.2222); err != nil { t.Fatal(err) } + /* + "update": { + "labels": [ + { + "remove": "triaged" + } + ] + } + */ operationsMocked := &model.UpdateOperations{} - err = operationsMocked.AddArrayOperation("labels", map[string]string{ - "triaged": "remove", - }) - if err != nil { + if err := operationsMocked.AddArrayOperation("labels", map[string]string{"triaged": "remove"}); err != nil { t.Fatal(err) } expectedPayloadWithCustomFieldsAndOperations := map[string]interface{}{ - "fields": map[string]interface{}{ "customfield_10042": 1000.2222, "customfield_10052": []map[string]interface{}{map[string]interface{}{ "name": "jira-administrators"}, map[string]interface{}{ "name": "jira-administrators-system"}}, - "issuetype": map[string]interface{}{"name": "Story"}, - "project": map[string]interface{}{"id": "10000"}, - "summary": "New summary test"}, + "issuetype": map[string]interface{}{"name": "Story"}, + "project": map[string]interface{}{"id": "10000"}, + "resolution": map[string]interface{}{"name": "Done"}, + "summary": "New summary test"}, "update": map[string]interface{}{ "labels": []map[string]interface{}{map[string]interface{}{ - "remove": "triaged"}}}} + "remove": "triaged"}}}, + + "transition": map[string]interface{}{"id": "10001"}, + } expectedPayloadWithCustomfields := map[string]interface{}{ "fields": map[string]interface{}{ @@ -1052,7 +1069,9 @@ func Test_internalRichTextServiceImpl_Move(t *testing.T) { "issuetype": map[string]interface{}{"name": "Story"}, "project": map[string]interface{}{"id": "10000"}, - "summary": "New summary test"}} + "summary": "New summary test"}, + "transition": map[string]interface{}{"id": "10001"}, + } expectedPayloadWithOperations := map[string]interface{}{ "fields": map[string]interface{}{ @@ -1062,7 +1081,9 @@ func Test_internalRichTextServiceImpl_Move(t *testing.T) { "update": map[string]interface{}{ "labels": []map[string]interface{}{map[string]interface{}{ - "remove": "triaged"}}}} + "remove": "triaged"}}}, + "transition": map[string]interface{}{"id": "10001"}, + } expectedPayloadWithNoOptions := map[string]interface{}{"transition": map[string]interface{}{"id": "10001"}} @@ -1098,6 +1119,9 @@ func Test_internalRichTextServiceImpl_Move(t *testing.T) { Summary: "New summary test", Project: &model.ProjectScheme{ID: "10000"}, IssueType: &model.IssueTypeScheme{Name: "Story"}, + Resolution: &model.ResolutionScheme{ + Name: "Done", + }, }, }, CustomFields: customFieldsMocked, @@ -1331,7 +1355,7 @@ func Test_internalRichTextServiceImpl_Move(t *testing.T) { http.MethodPost, "rest/api/2/issue/DUMMY-1/transitions", "", - expectedPayloadWithCustomFieldsAndOperations). + mock.Anything). Return(&http.Request{}, errors.New("error, unable to create the http request")) fields.c = client diff --git a/pkg/infra/models/jira_issue_v2.go b/pkg/infra/models/jira_issue_v2.go index 819c4f50..2734cc4a 100644 --- a/pkg/infra/models/jira_issue_v2.go +++ b/pkg/infra/models/jira_issue_v2.go @@ -23,7 +23,7 @@ type IssueSchemeV2 struct { func (i *IssueSchemeV2) MergeCustomFields(fields *CustomFields) (map[string]interface{}, error) { if fields == nil || len(fields.Fields) == 0 { - return nil, ErrNoCustomField + return map[string]interface{}{}, nil } // Convert the IssueScheme struct to map[string]interface{} @@ -50,10 +50,17 @@ func (i *IssueSchemeV2) MergeCustomFields(fields *CustomFields) (map[string]inte // MergeOperations merges operations into the issue scheme. // It returns a map representation of the issue scheme with the merged operations. // If the provided operations are nil or empty, it returns an error. +// +// Parameters: +// - operations: A pointer to UpdateOperations containing the operations to be merged. +// +// Returns: +// - A map[string]interface{} representing the issue scheme with the merged operations. +// - An error if the operations are nil, empty, or if there is an issue during the merging process. func (i *IssueSchemeV2) MergeOperations(operations *UpdateOperations) (map[string]interface{}, error) { if operations == nil || len(operations.Fields) == 0 { - return nil, ErrNoOperator + return map[string]interface{}{}, nil } // Convert the IssueScheme struct to map[string]interface{} diff --git a/pkg/infra/models/jira_issue_v2_test.go b/pkg/infra/models/jira_issue_v2_test.go index fe52a3be..bdc6de79 100644 --- a/pkg/infra/models/jira_issue_v2_test.go +++ b/pkg/infra/models/jira_issue_v2_test.go @@ -48,28 +48,6 @@ func TestIssueSchemeV2_MergeCustomFields(t *testing.T) { wantErr: false, Err: nil, }, - - { - name: "when the custom-fields are not provided", - fields: fields{}, - args: args{ - fields: nil, - }, - want: nil, - wantErr: true, - Err: ErrNoCustomField, - }, - - { - name: "when the custom-field don't have information", - fields: fields{}, - args: args{ - fields: &CustomFields{}, - }, - want: nil, - wantErr: true, - Err: ErrNoCustomField, - }, } for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { @@ -143,28 +121,6 @@ func TestIssueSchemeV2_MergeOperations(t *testing.T) { wantErr: false, Err: nil, }, - - { - name: "when the operations are not provided", - fields: fields{}, - args: args{ - operations: nil, - }, - want: nil, - wantErr: true, - Err: ErrNoOperator, - }, - - { - name: "when the operations don't have information", - fields: fields{}, - args: args{ - operations: &UpdateOperations{}, - }, - want: nil, - wantErr: true, - Err: ErrNoOperator, - }, } for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { diff --git a/pkg/infra/models/jira_issue_v3.go b/pkg/infra/models/jira_issue_v3.go index e0cb727c..709b0bac 100644 --- a/pkg/infra/models/jira_issue_v3.go +++ b/pkg/infra/models/jira_issue_v3.go @@ -17,13 +17,13 @@ type IssueScheme struct { RenderedFields map[string]interface{} `json:"renderedFields,omitempty"` } -// MergeCustomFields merges the custom fields into the issue. -// fields is a pointer to CustomFields which represents the custom fields to be merged. -// It returns a map[string]interface{} representing the issue with the merged custom fields and an error if any occurred. +// MergeCustomFields merges custom fields into the issue scheme. +// It returns a map representation of the issue scheme with the merged fields. +// If the provided fields are nil or empty, it returns an error. func (i *IssueScheme) MergeCustomFields(fields *CustomFields) (map[string]interface{}, error) { if fields == nil || len(fields.Fields) == 0 { - return nil, ErrNoCustomField + return map[string]interface{}{}, nil } //Convert the IssueScheme struct to map[string]interface{} @@ -47,13 +47,20 @@ func (i *IssueScheme) MergeCustomFields(fields *CustomFields) (map[string]interf return issueSchemeAsMap, nil } -// MergeOperations merges the operations into the issue. -// operations is a pointer to UpdateOperations which represents the operations to be merged. -// It returns a map[string]interface{} representing the issue with the merged operations and an error if any occurred. +// MergeOperations merges operations into the issue scheme. +// It returns a map representation of the issue scheme with the merged operations. +// If the provided operations are nil or empty, it returns an error. +// +// Parameters: +// - operations: A pointer to UpdateOperations containing the operations to be merged. +// +// Returns: +// - A map[string]interface{} representing the issue scheme with the merged operations. +// - An error if the operations are nil, empty, or if there is an issue during the merging process. func (i *IssueScheme) MergeOperations(operations *UpdateOperations) (map[string]interface{}, error) { if operations == nil || len(operations.Fields) == 0 { - return nil, ErrNoOperator + return map[string]interface{}{}, nil } //Convert the IssueScheme struct to map[string]interface{} diff --git a/pkg/infra/models/jira_issue_v3_test.go b/pkg/infra/models/jira_issue_v3_test.go index 0f77cc27..e1a6641f 100644 --- a/pkg/infra/models/jira_issue_v3_test.go +++ b/pkg/infra/models/jira_issue_v3_test.go @@ -47,28 +47,6 @@ func TestIssueScheme_MergeCustomFields(t *testing.T) { wantErr: false, Err: nil, }, - - { - name: "when the custom-fields are not provided", - fields: fields{}, - args: args{ - fields: nil, - }, - want: nil, - wantErr: true, - Err: ErrNoCustomField, - }, - - { - name: "when the custom-field don't have information", - fields: fields{}, - args: args{ - fields: &CustomFields{}, - }, - want: nil, - wantErr: true, - Err: ErrNoCustomField, - }, } for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { @@ -142,28 +120,6 @@ func TestIssueScheme_MergeOperations(t *testing.T) { wantErr: false, Err: nil, }, - - { - name: "when the operations are not provided", - fields: fields{}, - args: args{ - operations: nil, - }, - want: nil, - wantErr: true, - Err: ErrNoOperator, - }, - - { - name: "when the operations don't have information", - fields: fields{}, - args: args{ - operations: &UpdateOperations{}, - }, - want: nil, - wantErr: true, - Err: ErrNoOperator, - }, } for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { diff --git a/pkg/infra/models/jira_resolution.go b/pkg/infra/models/jira_resolution.go index d0636e01..aaa9a25e 100644 --- a/pkg/infra/models/jira_resolution.go +++ b/pkg/infra/models/jira_resolution.go @@ -2,8 +2,8 @@ package models // ResolutionScheme represents a resolution in Jira. type ResolutionScheme struct { - Self string `json:"self"` // The URL of the resolution. - ID string `json:"id"` // The ID of the resolution. - Description string `json:"description"` // The description of the resolution. - Name string `json:"name"` // The name of the resolution. + Self string `json:"self,omitempty"` + ID string `json:"id,omitempty"` + Description string `json:"description,omitempty"` + Name string `json:"name,omitempty"` }