diff --git a/internal/jsonutil/clone_test.go b/internal/jsonutil/clone_test.go deleted file mode 100644 index 8be3618ba..000000000 --- a/internal/jsonutil/clone_test.go +++ /dev/null @@ -1,50 +0,0 @@ -package jsonutil - -import ( - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestDeepCloneJSON_Map(t *testing.T) { - original := map[string]interface{}{ - "id": float64(1), - "nested": map[string]interface{}{ - "value": "original", - }, - "tags": []interface{}{"go", "test"}, - } - - cloned := DeepCloneJSON(original) - clonedMap, ok := cloned.(map[string]interface{}) - require.True(t, ok) - - original["nested"].(map[string]interface{})["value"] = "mutated" - original["tags"].([]interface{})[0] = "mutated-tag" - - assert.Equal(t, "original", clonedMap["nested"].(map[string]interface{})["value"]) - assert.Equal(t, "go", clonedMap["tags"].([]interface{})[0]) -} - -func TestDeepCloneJSON_Slice(t *testing.T) { - original := []interface{}{ - map[string]interface{}{"id": float64(1)}, - "hello", - } - - cloned := DeepCloneJSON(original) - clonedSlice, ok := cloned.([]interface{}) - require.True(t, ok) - assert.Len(t, clonedSlice, 2) - - original[0].(map[string]interface{})["id"] = float64(99) - assert.Equal(t, float64(1), clonedSlice[0].(map[string]interface{})["id"]) -} - -func TestDeepCloneJSON_Primitives(t *testing.T) { - assert.Equal(t, "hello", DeepCloneJSON("hello")) - assert.Equal(t, float64(3.14), DeepCloneJSON(float64(3.14))) - assert.Equal(t, true, DeepCloneJSON(true)) - assert.Nil(t, DeepCloneJSON(nil)) -} diff --git a/internal/logger/jsonl_logger.go b/internal/logger/jsonl_logger.go index 7ef259a79..cde487835 100644 --- a/internal/logger/jsonl_logger.go +++ b/internal/logger/jsonl_logger.go @@ -25,8 +25,7 @@ var ( ) const ( - logTimestampLayout = "2006-01-02T15:04:05.000Z07:00" - jsonTimestampLayout = logTimestampLayout + jsonTimestampLayout = "2006-01-02T15:04:05.000Z07:00" rpcMessageSchemaV2 = "rpc-message/v2" difcSchemaV2 = "difc-filtered/v2" ) diff --git a/internal/proxy/proxy_test.go b/internal/proxy/proxy_test.go index 4f08b5a9b..b715f6c41 100644 --- a/internal/proxy/proxy_test.go +++ b/internal/proxy/proxy_test.go @@ -8,7 +8,7 @@ import ( "testing" "github.com/github/gh-aw-mcpg/internal/difc" - "github.com/github/gh-aw-mcpg/internal/jsonutil" + "github.com/github/gh-aw-mcpg/internal/strutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -780,7 +780,7 @@ func TestDeepCloneJSON(t *testing.T) { "a": []interface{}{float64(1), float64(2)}, "b": map[string]interface{}{"c": "d"}, } - cloned := jsonutil.DeepCloneJSON(original) + cloned := strutil.DeepCloneJSON(original) // Mutate original original["a"].([]interface{})[0] = float64(99) original["b"].(map[string]interface{})["c"] = "mutated" diff --git a/internal/proxy/response_transform.go b/internal/proxy/response_transform.go index 44ada245e..ba7c1a0ff 100644 --- a/internal/proxy/response_transform.go +++ b/internal/proxy/response_transform.go @@ -4,8 +4,8 @@ import ( "reflect" "github.com/github/gh-aw-mcpg/internal/difc" - "github.com/github/gh-aw-mcpg/internal/jsonutil" "github.com/github/gh-aw-mcpg/internal/logger" + "github.com/github/gh-aw-mcpg/internal/strutil" ) var logTransform = logger.New("proxy:response_transform") @@ -111,7 +111,7 @@ func rebuildGraphQLResponse(originalData interface{}, filtered *difc.FilteredCol logTransform.Printf("rebuildGraphQLResponse: rebuilding with %d accessible items", filtered.GetAccessibleCount()) // Deep-clone the original data structure - cloned := jsonutil.DeepCloneJSON(original) + cloned := strutil.DeepCloneJSON(original) // Build accessible items set accessibleItems := make([]interface{}, 0, len(filtered.Accessible)) diff --git a/internal/proxy/response_transform_coverage_test.go b/internal/proxy/response_transform_coverage_test.go index 11357fe60..4738bf9ed 100644 --- a/internal/proxy/response_transform_coverage_test.go +++ b/internal/proxy/response_transform_coverage_test.go @@ -4,7 +4,7 @@ import ( "testing" "github.com/github/gh-aw-mcpg/internal/difc" - "github.com/github/gh-aw-mcpg/internal/jsonutil" + "github.com/github/gh-aw-mcpg/internal/strutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -363,7 +363,7 @@ func TestDeepCloneJSON_Map(t *testing.T) { "tags": []interface{}{"go", "test"}, } - cloned := jsonutil.DeepCloneJSON(original) + cloned := strutil.DeepCloneJSON(original) clonedMap, ok := cloned.(map[string]interface{}) require.True(t, ok, "cloned value should be a map") @@ -390,7 +390,7 @@ func TestDeepCloneJSON_NestedMapAndSlice(t *testing.T) { }, } - cloned := jsonutil.DeepCloneJSON(original) + cloned := strutil.DeepCloneJSON(original) // Mutate deeply nested value. original["level1"].(map[string]interface{})["level2"].([]interface{})[0].(map[string]interface{})["x"] = float64(99) diff --git a/internal/proxy/response_transform_test.go b/internal/proxy/response_transform_test.go index 217a24cba..e386019eb 100644 --- a/internal/proxy/response_transform_test.go +++ b/internal/proxy/response_transform_test.go @@ -4,7 +4,7 @@ import ( "testing" "github.com/github/gh-aw-mcpg/internal/difc" - "github.com/github/gh-aw-mcpg/internal/jsonutil" + "github.com/github/gh-aw-mcpg/internal/strutil" "github.com/stretchr/testify/assert" ) @@ -111,7 +111,7 @@ func TestDeepCloneJSON_Slice(t *testing.T) { float64(42), } - cloned := jsonutil.DeepCloneJSON(original) + cloned := strutil.DeepCloneJSON(original) clonedSlice, ok := cloned.([]interface{}) assert.True(t, ok, "cloned value should be a []interface{}") @@ -128,12 +128,12 @@ func TestDeepCloneJSON_Slice(t *testing.T) { // TestDeepCloneJSON_Primitive verifies that deepCloneJSON returns primitive values // (string, number, bool, nil) unchanged — they are immutable so no copy is needed. func TestDeepCloneJSON_Primitive(t *testing.T) { - assert.Equal(t, "hello", jsonutil.DeepCloneJSON("hello")) - assert.Equal(t, float64(3.14), jsonutil.DeepCloneJSON(float64(3.14))) - clonedBool, ok := jsonutil.DeepCloneJSON(true).(bool) + assert.Equal(t, "hello", strutil.DeepCloneJSON("hello")) + assert.Equal(t, float64(3.14), strutil.DeepCloneJSON(float64(3.14))) + clonedBool, ok := strutil.DeepCloneJSON(true).(bool) assert.True(t, ok, "cloned bool should remain a bool") assert.True(t, clonedBool) - assert.Nil(t, jsonutil.DeepCloneJSON(nil)) + assert.Nil(t, strutil.DeepCloneJSON(nil)) } // TestUnwrapSingleObject_NilFilteredData verifies that unwrapSingleObject handles diff --git a/internal/server/difc_log.go b/internal/server/difc_log.go index 040b77270..f944c03bf 100644 --- a/internal/server/difc_log.go +++ b/internal/server/difc_log.go @@ -7,6 +7,7 @@ import ( "github.com/github/gh-aw-mcpg/internal/difc" "github.com/github/gh-aw-mcpg/internal/logger" + "github.com/github/gh-aw-mcpg/internal/strutil" ) var logDifcLog = logger.New("server:difc_log") @@ -48,32 +49,17 @@ func buildFilteredItemLogEntry(serverID, toolName string, detail difc.FilteredIt // Extract identifying metadata from the raw item data. // Data is interface{} from JSON parsing — typically map[string]interface{}. if m, ok := detail.Item.Data.(map[string]interface{}); ok { - entry.AuthorAssociation = getFilteredItemStringField(m, "author_association", "authorAssociation") + entry.AuthorAssociation = strutil.GetStringFromMap(m, "author_association", "authorAssociation") entry.AuthorLogin = extractAuthorLogin(m) - entry.HTMLURL = getFilteredItemStringField(m, "html_url", "htmlUrl") + entry.HTMLURL = strutil.GetStringFromMap(m, "html_url", "htmlUrl") entry.Number = extractNumberField(m) - entry.SHA = getFilteredItemStringField(m, "sha") + entry.SHA = strutil.GetStringFromMap(m, "sha") logDifcLog.Printf("Filtered item metadata: author=%s, number=%s, url=%s", entry.AuthorLogin, entry.Number, entry.HTMLURL) } return entry } -// getFilteredItemStringField returns the first non-empty string value from the map -// matching any of the given field names. -// NOTE: This helper is intentionally generic; if similar untyped JSON map extraction -// patterns appear elsewhere, consider moving it to a shared utility package. -func getFilteredItemStringField(m map[string]interface{}, fields ...string) string { - for _, f := range fields { - if v, ok := m[f]; ok { - if s, ok := v.(string); ok && s != "" { - return s - } - } - } - return "" -} - // extractAuthorLogin extracts the author login from nested user/author objects. func extractAuthorLogin(m map[string]interface{}) string { if user, ok := m["user"].(map[string]interface{}); ok { diff --git a/internal/server/difc_log_helpers_test.go b/internal/server/difc_log_helpers_test.go index 80542d8bd..a53a98945 100644 --- a/internal/server/difc_log_helpers_test.go +++ b/internal/server/difc_log_helpers_test.go @@ -7,103 +7,6 @@ import ( "github.com/stretchr/testify/assert" ) -// TestGetFilteredItemStringField exercises all code paths of the -// getFilteredItemStringField helper. -func TestGetFilteredItemStringField(t *testing.T) { - tests := []struct { - name string - m map[string]interface{} - fields []string - want string - }{ - { - name: "empty map returns empty string", - m: map[string]interface{}{}, - fields: []string{"title"}, - want: "", - }, - { - name: "single field found returns value", - m: map[string]interface{}{"title": "My Issue"}, - fields: []string{"title"}, - want: "My Issue", - }, - { - name: "single field present but empty string is skipped", - m: map[string]interface{}{"title": ""}, - fields: []string{"title"}, - want: "", - }, - { - name: "first field missing second field found", - m: map[string]interface{}{"htmlUrl": "https://example.com"}, - fields: []string{"html_url", "htmlUrl"}, - want: "https://example.com", - }, - { - name: "first field found returns first match", - m: map[string]interface{}{"html_url": "https://first.com", "htmlUrl": "https://second.com"}, - fields: []string{"html_url", "htmlUrl"}, - want: "https://first.com", - }, - { - name: "all fields missing returns empty string", - m: map[string]interface{}{"other": "value"}, - fields: []string{"html_url", "htmlUrl"}, - want: "", - }, - { - name: "field present with non-string value is skipped", - m: map[string]interface{}{"number": 42}, - fields: []string{"number"}, - want: "", - }, - { - name: "field present with bool value is skipped", - m: map[string]interface{}{"active": true}, - fields: []string{"active"}, - want: "", - }, - { - name: "field present with nil value is skipped", - m: map[string]interface{}{"login": nil}, - fields: []string{"login"}, - want: "", - }, - { - name: "first field non-string second field string returns second", - m: map[string]interface{}{"author_association": 99, "authorAssociation": "CONTRIBUTOR"}, - fields: []string{"author_association", "authorAssociation"}, - want: "CONTRIBUTOR", - }, - { - name: "no fields argument returns empty string", - m: map[string]interface{}{"title": "value"}, - fields: []string{}, - want: "", - }, - { - name: "nil map returns empty string", - m: nil, - fields: []string{"field"}, - want: "", - }, - { - name: "sha field extraction", - m: map[string]interface{}{"sha": "abc123def456"}, - fields: []string{"sha"}, - want: "abc123def456", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := getFilteredItemStringField(tt.m, tt.fields...) - assert.Equal(t, tt.want, got) - }) - } -} - // TestExtractAuthorLogin exercises all code paths of the extractAuthorLogin helper. func TestExtractAuthorLogin(t *testing.T) { tests := []struct { diff --git a/internal/jsonutil/clone.go b/internal/strutil/deepclone.go similarity index 82% rename from internal/jsonutil/clone.go rename to internal/strutil/deepclone.go index b28599f34..0bf648dfc 100644 --- a/internal/jsonutil/clone.go +++ b/internal/strutil/deepclone.go @@ -1,5 +1,4 @@ -// Package jsonutil provides utilities for working with JSON-compatible values. -package jsonutil +package strutil // DeepCloneJSON creates a deep copy of a JSON-compatible value. func DeepCloneJSON(v interface{}) interface{} { diff --git a/internal/strutil/maputil.go b/internal/strutil/maputil.go new file mode 100644 index 000000000..54a382d59 --- /dev/null +++ b/internal/strutil/maputil.go @@ -0,0 +1,26 @@ +package strutil + +// GetStringFromMap returns the first non-empty string value found for any of +// the given keys in m. For each key, the value must be present, typed as +// string, and non-empty to be returned. Returns an empty string when no +// matching non-empty string value is found, when the map is nil, or when no +// keys are provided. +// +// With a single key the behaviour is equivalent to `v, _ := m[key].(string)`: +// +// GetStringFromMap(m, "owner") +// +// With multiple keys the function returns the first non-empty match, which is +// useful for maps that may use either snake_case or camelCase field names: +// +// GetStringFromMap(m, "html_url", "htmlUrl") +func GetStringFromMap(m map[string]interface{}, keys ...string) string { + for _, k := range keys { + if v, ok := m[k]; ok { + if s, ok := v.(string); ok && s != "" { + return s + } + } + } + return "" +} diff --git a/internal/strutil/truncate.go b/internal/strutil/truncate.go index db282b4dc..eb5772220 100644 --- a/internal/strutil/truncate.go +++ b/internal/strutil/truncate.go @@ -54,10 +54,3 @@ func TruncateRunes(s string, maxRunes int) string { } return string(r[:maxRunes]) } - -// GetStringFromMap returns the string value for key when it is present and typed as string. -// It returns an empty string for missing keys, nil maps, and non-string values. -func GetStringFromMap(m map[string]interface{}, key string) string { - v, _ := m[key].(string) - return v -} diff --git a/internal/strutil/util_test.go b/internal/strutil/util_test.go index 7b0721e00..5bbf987e5 100644 --- a/internal/strutil/util_test.go +++ b/internal/strutil/util_test.go @@ -32,4 +32,46 @@ func TestGetStringFromMap(t *testing.T) { var m map[string]interface{} assert.Equal(t, "", GetStringFromMap(m, "owner")) }) + + t.Run("returns empty string when value is empty string", func(t *testing.T) { + t.Parallel() + m := map[string]interface{}{"owner": ""} + assert.Equal(t, "", GetStringFromMap(m, "owner")) + }) + + t.Run("variadic: returns first non-empty match", func(t *testing.T) { + t.Parallel() + m := map[string]interface{}{"htmlUrl": "https://example.com"} + assert.Equal(t, "https://example.com", GetStringFromMap(m, "html_url", "htmlUrl")) + }) + + t.Run("variadic: first key takes priority when both present", func(t *testing.T) { + t.Parallel() + m := map[string]interface{}{"html_url": "https://first.com", "htmlUrl": "https://second.com"} + assert.Equal(t, "https://first.com", GetStringFromMap(m, "html_url", "htmlUrl")) + }) + + t.Run("variadic: skips empty first key and returns second", func(t *testing.T) { + t.Parallel() + m := map[string]interface{}{"html_url": "", "htmlUrl": "https://second.com"} + assert.Equal(t, "https://second.com", GetStringFromMap(m, "html_url", "htmlUrl")) + }) + + t.Run("variadic: all keys missing returns empty string", func(t *testing.T) { + t.Parallel() + m := map[string]interface{}{"other": "value"} + assert.Equal(t, "", GetStringFromMap(m, "html_url", "htmlUrl")) + }) + + t.Run("variadic: skips non-string first key and returns second", func(t *testing.T) { + t.Parallel() + m := map[string]interface{}{"html_url": 99, "htmlUrl": "https://second.com"} + assert.Equal(t, "https://second.com", GetStringFromMap(m, "html_url", "htmlUrl")) + }) + + t.Run("no keys returns empty string", func(t *testing.T) { + t.Parallel() + m := map[string]interface{}{"owner": "octo"} + assert.Equal(t, "", GetStringFromMap(m)) + }) }