From 6acc9ca18f322df2d446ac6e0a9cb8ef8b81b334 Mon Sep 17 00:00:00 2001 From: Ary Neto Date: Sun, 7 Jul 2024 10:48:34 -0600 Subject: [PATCH 1/3] Fix: Multi-select Custom Properties are not supported --- github/github-accessors.go | 8 ------- github/github-accessors_test.go | 10 --------- github/orgs_properties.go | 39 +++++++++++++++++++++++++++++++-- github/orgs_properties_test.go | 20 +++++++++++++++-- github/repos_properties_test.go | 22 ++++++++++++++++--- 5 files changed, 74 insertions(+), 25 deletions(-) diff --git a/github/github-accessors.go b/github/github-accessors.go index 5118791cdd2..eb939da3780 100644 --- a/github/github-accessors.go +++ b/github/github-accessors.go @@ -4766,14 +4766,6 @@ func (c *CustomProperty) GetValuesEditableBy() string { return *c.ValuesEditableBy } -// GetValue returns the Value field if it's non-nil, zero value otherwise. -func (c *CustomPropertyValue) GetValue() string { - if c == nil || c.Value == nil { - return "" - } - return *c.Value -} - // GetBaseRole returns the BaseRole field if it's non-nil, zero value otherwise. func (c *CustomRepoRoles) GetBaseRole() string { if c == nil || c.BaseRole == nil { diff --git a/github/github-accessors_test.go b/github/github-accessors_test.go index 03ebada64b1..d5dcabebcfb 100644 --- a/github/github-accessors_test.go +++ b/github/github-accessors_test.go @@ -5613,16 +5613,6 @@ func TestCustomProperty_GetValuesEditableBy(tt *testing.T) { c.GetValuesEditableBy() } -func TestCustomPropertyValue_GetValue(tt *testing.T) { - var zeroValue string - c := &CustomPropertyValue{Value: &zeroValue} - c.GetValue() - c = &CustomPropertyValue{} - c.GetValue() - c = nil - c.GetValue() -} - func TestCustomRepoRoles_GetBaseRole(tt *testing.T) { var zeroValue string c := &CustomRepoRoles{BaseRole: &zeroValue} diff --git a/github/orgs_properties.go b/github/orgs_properties.go index 124b89cb5db..f7416d53008 100644 --- a/github/orgs_properties.go +++ b/github/orgs_properties.go @@ -7,6 +7,7 @@ package github import ( "context" + "encoding/json" "fmt" ) @@ -40,8 +41,42 @@ type RepoCustomPropertyValue struct { // CustomPropertyValue represents a custom property value. type CustomPropertyValue struct { - PropertyName string `json:"property_name"` - Value *string `json:"value,omitempty"` + PropertyName string `json:"property_name"` + Value interface{} `json:"value,omitempty"` +} + +// UnmarshalJSON implements the json.Unmarshaler interface. +// This helps us handle the fact that Value can be either a string, []string, or nil. +func (cpv *CustomPropertyValue) UnmarshalJSON(data []byte) error { + type aliasCustomPropertyValue CustomPropertyValue + aux := &struct { + *aliasCustomPropertyValue + }{ + aliasCustomPropertyValue: (*aliasCustomPropertyValue)(cpv), + } + if err := json.Unmarshal(data, &aux); err != nil { + return err + } + + switch v := aux.Value.(type) { + case nil: + cpv.Value = nil + case string: + cpv.Value = v + case []interface{}: + strSlice := make([]string, len(v)) + for i, item := range v { + if str, ok := item.(string); ok { + strSlice[i] = str + } else { + return fmt.Errorf("non-string value in string array") + } + } + cpv.Value = strSlice + default: + return fmt.Errorf("unexpected value type: %T", v) + } + return nil } // GetAllCustomProperties gets all custom properties that are defined for the specified organization. diff --git a/github/orgs_properties_test.go b/github/orgs_properties_test.go index 59866fd98a7..3b296007eec 100644 --- a/github/orgs_properties_test.go +++ b/github/orgs_properties_test.go @@ -296,6 +296,14 @@ func TestOrganizationsService_ListCustomPropertyValues(t *testing.T) { { "property_name": "service", "value": "web" + }, + { + "property_name": "languages", + "value": ["Go", "JavaScript"] + }, + { + "property_name": "null_property", + "value": null } ] }]`) @@ -318,11 +326,19 @@ func TestOrganizationsService_ListCustomPropertyValues(t *testing.T) { Properties: []*CustomPropertyValue{ { PropertyName: "environment", - Value: String("production"), + Value: "production", }, { PropertyName: "service", - Value: String("web"), + Value: "web", + }, + { + PropertyName: "languages", + Value: []string{"Go", "JavaScript"}, + }, + { + PropertyName: "null_property", + Value: nil, }, }, }, diff --git a/github/repos_properties_test.go b/github/repos_properties_test.go index 5ce05c26d7b..4434856c8c2 100644 --- a/github/repos_properties_test.go +++ b/github/repos_properties_test.go @@ -28,6 +28,14 @@ func TestRepositoriesService_GetAllCustomPropertyValues(t *testing.T) { { "property_name": "service", "value": "web" + }, + { + "property_name": "languages", + "value": ["Go", "JavaScript"] + }, + { + "property_name": "null_property", + "value": null } ]`) }) @@ -41,11 +49,19 @@ func TestRepositoriesService_GetAllCustomPropertyValues(t *testing.T) { want := []*CustomPropertyValue{ { PropertyName: "environment", - Value: String("production"), + Value: "production", }, { PropertyName: "service", - Value: String("web"), + Value: "web", + }, + { + PropertyName: "languages", + Value: []string{"Go", "JavaScript"}, + }, + { + PropertyName: "null_property", + Value: nil, }, } @@ -77,7 +93,7 @@ func TestRepositoriesService_CreateOrUpdateCustomProperties(t *testing.T) { RepoCustomProperty := []*CustomPropertyValue{ { PropertyName: "environment", - Value: String("production"), + Value: "production", }, } _, err := client.Repositories.CreateOrUpdateCustomProperties(ctx, "usr", "r", RepoCustomProperty) From 23cecf9d3c58f89b927e3454238f77ad72cec9ea Mon Sep 17 00:00:00 2001 From: Ary Neto Date: Sun, 7 Jul 2024 14:40:01 -0600 Subject: [PATCH 2/3] Tests: Added test scenarios for invalid value types --- github/orgs_properties_test.go | 60 ++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/github/orgs_properties_test.go b/github/orgs_properties_test.go index 3b296007eec..a07471c4386 100644 --- a/github/orgs_properties_test.go +++ b/github/orgs_properties_test.go @@ -364,6 +364,66 @@ func TestOrganizationsService_ListCustomPropertyValues(t *testing.T) { }) } +func TestOrganizationsService_ListInvalidCustomPropertyValuesTypes(t *testing.T) { + client, mux, _, teardown := setup() + defer teardown() + + mux.HandleFunc("/orgs/o/properties/values", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + testFormValues(t, r, values{"page": "1", "per_page": "100"}) + fmt.Fprint(w, `[{ + "repository_id": 1296269, + "repository_name": "Hello-World", + "repository_full_name": "octocat/Hello-World", + "properties": [ + { + "property_name": "environment", + "value": { + "invalid": "type" + } + } + ]}]`) + }) + + ctx := context.Background() + _, _, err := client.Organizations.ListCustomPropertyValues(ctx, "o", &ListOptions{ + Page: 1, + PerPage: 100, + }) + if err == nil || err.Error() != "unexpected value type: map[string]interface {}" { + t.Errorf("Expected unexpected value type error, got %v", err) + } +} + +func TestOrganizationsService_ListCustomPropertyValues_NonStringValueInArray(t *testing.T) { + client, mux, _, teardown := setup() + defer teardown() + + mux.HandleFunc("/orgs/o/properties/values", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + testFormValues(t, r, values{"page": "1", "per_page": "100"}) + fmt.Fprint(w, `[{ + "repository_id": 1296269, + "repository_name": "Hello-World", + "repository_full_name": "octocat/Hello-World", + "properties": [ + { + "property_name": "languages", + "value": ["Go", 42] + } + ]}]`) + }) + + ctx := context.Background() + _, _, err := client.Organizations.ListCustomPropertyValues(ctx, "o", &ListOptions{ + Page: 1, + PerPage: 100, + }) + if err == nil || err.Error() != "non-string value in string array" { + t.Errorf("Expected non-string value in string array error, got %v", err) + } +} + func TestOrganizationsService_CreateOrUpdateRepoCustomPropertyValues(t *testing.T) { client, mux, _, teardown := setup() defer teardown() From ba66916a8bf3470c1f80fd8614d57e4f1ac7d6ec Mon Sep 17 00:00:00 2001 From: Ary Neto Date: Sun, 7 Jul 2024 14:46:22 -0600 Subject: [PATCH 3/3] Tests: Added tests for the CustomPropertValue UnmarshalJSON function --- github/orgs_properties_test.go | 122 ++++++++++++++++++--------------- 1 file changed, 67 insertions(+), 55 deletions(-) diff --git a/github/orgs_properties_test.go b/github/orgs_properties_test.go index a07471c4386..2ef1f4536b9 100644 --- a/github/orgs_properties_test.go +++ b/github/orgs_properties_test.go @@ -364,63 +364,75 @@ func TestOrganizationsService_ListCustomPropertyValues(t *testing.T) { }) } -func TestOrganizationsService_ListInvalidCustomPropertyValuesTypes(t *testing.T) { - client, mux, _, teardown := setup() - defer teardown() - - mux.HandleFunc("/orgs/o/properties/values", func(w http.ResponseWriter, r *http.Request) { - testMethod(t, r, "GET") - testFormValues(t, r, values{"page": "1", "per_page": "100"}) - fmt.Fprint(w, `[{ - "repository_id": 1296269, - "repository_name": "Hello-World", - "repository_full_name": "octocat/Hello-World", - "properties": [ - { - "property_name": "environment", - "value": { - "invalid": "type" - } - } - ]}]`) - }) - - ctx := context.Background() - _, _, err := client.Organizations.ListCustomPropertyValues(ctx, "o", &ListOptions{ - Page: 1, - PerPage: 100, - }) - if err == nil || err.Error() != "unexpected value type: map[string]interface {}" { - t.Errorf("Expected unexpected value type error, got %v", err) +func TestCustomPropertyValue_UnmarshalJSON(t *testing.T) { + tests := map[string]struct { + data string + want *CustomPropertyValue + wantErr bool + }{ + "Invalid JSON": { + data: `{`, + want: &CustomPropertyValue{}, + wantErr: true, + }, + "String value": { + data: `{ + "property_name": "environment", + "value": "production" + }`, + want: &CustomPropertyValue{ + PropertyName: "environment", + Value: "production", + }, + wantErr: false, + }, + "Array of strings value": { + data: `{ + "property_name": "languages", + "value": ["Go", "JavaScript"] + }`, + want: &CustomPropertyValue{ + PropertyName: "languages", + Value: []string{"Go", "JavaScript"}, + }, + wantErr: false, + }, + "Non-string value in array": { + data: `{ + "property_name": "languages", + "value": ["Go", 42] + }`, + want: &CustomPropertyValue{ + PropertyName: "languages", + Value: nil, + }, + wantErr: true, + }, + "Unexpected value type": { + data: `{ + "property_name": "environment", + "value": {"invalid": "type"} + }`, + want: &CustomPropertyValue{ + PropertyName: "environment", + Value: nil, + }, + wantErr: true, + }, } -} -func TestOrganizationsService_ListCustomPropertyValues_NonStringValueInArray(t *testing.T) { - client, mux, _, teardown := setup() - defer teardown() - - mux.HandleFunc("/orgs/o/properties/values", func(w http.ResponseWriter, r *http.Request) { - testMethod(t, r, "GET") - testFormValues(t, r, values{"page": "1", "per_page": "100"}) - fmt.Fprint(w, `[{ - "repository_id": 1296269, - "repository_name": "Hello-World", - "repository_full_name": "octocat/Hello-World", - "properties": [ - { - "property_name": "languages", - "value": ["Go", 42] - } - ]}]`) - }) - - ctx := context.Background() - _, _, err := client.Organizations.ListCustomPropertyValues(ctx, "o", &ListOptions{ - Page: 1, - PerPage: 100, - }) - if err == nil || err.Error() != "non-string value in string array" { - t.Errorf("Expected non-string value in string array error, got %v", err) + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + cpv := &CustomPropertyValue{} + err := cpv.UnmarshalJSON([]byte(tc.data)) + if (err != nil) != tc.wantErr { + t.Errorf("CustomPropertyValue.UnmarshalJSON error = %v, wantErr %v", err, tc.wantErr) + return + } + if !tc.wantErr && !cmp.Equal(tc.want, cpv) { + t.Errorf("CustomPropertyValue.UnmarshalJSON expected %+v, got %+v", tc.want, cpv) + } + }) } }