diff --git a/pkg/cmd/comma_separated_flags_test.go b/pkg/cmd/comma_separated_flags_test.go new file mode 100644 index 00000000..7b5299bd --- /dev/null +++ b/pkg/cmd/comma_separated_flags_test.go @@ -0,0 +1,162 @@ +package cmd + +import ( + "regexp" + "testing" + + "github.com/spf13/cobra" + "github.com/spf13/pflag" + "github.com/stretchr/testify/assert" + + "github.com/hookdeck/hookdeck-cli/pkg/hookdeck" +) + +// A flag whose help says "comma-separated" is a promise that a list reaches the +// API. Twice that promise was broken the same way: the value went out as one +// query parameter, which the API matched against nothing, so the command +// returned zero rows with exit 0. --id did it (#411), and --delivery-group did +// it again after that fix because nothing tied the promise to the encoding. +// +// So every such flag must appear below, with how its value is actually sent, +// and a query filter must be one the client expands into a list. A new +// comma-separated flag fails here until someone checks it and says how. + +type commaFlagHandling int + +const ( + // A filter on a list endpoint, sent through hookdeck.listQuery. The param + // must be list-valued there, or a comma-joined value goes out as one. + sentAsQueryList commaFlagHandling = iota + // Split into an array in the JSON request body before sending. + splitIntoBodyArray + // Split into a slice the client sends as a list (metrics measures and + // dimensions; every Outpost filter, via splitCommaList). + splitIntoList + // Sent as one string on purpose: the API turns the commas into spaces + // (OAuth2 scopes, which the token endpoint expects space-separated). + sentAsStringAPIConvertsCommas +) + +type commaFlag struct { + kind commaFlagHandling + param string // for sentAsQueryList: the query parameter the flag sets +} + +var commaSeparatedFlags = map[string]commaFlag{ + "hookdeck gateway event list --delivery-group": {kind: sentAsQueryList, param: "delivery_group"}, + "hookdeck gateway event list --id": {kind: sentAsQueryList, param: "id"}, + "hookdeck gateway request events --delivery-group": {kind: sentAsQueryList, param: "delivery_group"}, + "hookdeck gateway request list --id": {kind: sentAsQueryList, param: "id"}, + "hookdeck connection create --rule-deduplicate-exclude-fields": {kind: splitIntoBodyArray}, + "hookdeck connection create --rule-deduplicate-include-fields": {kind: splitIntoBodyArray}, + "hookdeck connection create --rule-retry-response-status-codes": {kind: splitIntoBodyArray}, + "hookdeck connection create --source-allowed-http-methods": {kind: splitIntoBodyArray}, + "hookdeck connection update --rule-deduplicate-exclude-fields": {kind: splitIntoBodyArray}, + "hookdeck connection update --rule-deduplicate-include-fields": {kind: splitIntoBodyArray}, + "hookdeck connection update --rule-retry-response-status-codes": {kind: splitIntoBodyArray}, + "hookdeck connection upsert --rule-deduplicate-exclude-fields": {kind: splitIntoBodyArray}, + "hookdeck connection upsert --rule-deduplicate-include-fields": {kind: splitIntoBodyArray}, + "hookdeck connection upsert --rule-retry-response-status-codes": {kind: splitIntoBodyArray}, + "hookdeck connection upsert --source-allowed-http-methods": {kind: splitIntoBodyArray}, + "hookdeck gateway connection create --rule-deduplicate-exclude-fields": {kind: splitIntoBodyArray}, + "hookdeck gateway connection create --rule-deduplicate-include-fields": {kind: splitIntoBodyArray}, + "hookdeck gateway connection create --rule-retry-response-status-codes": {kind: splitIntoBodyArray}, + "hookdeck gateway connection create --source-allowed-http-methods": {kind: splitIntoBodyArray}, + "hookdeck gateway connection update --rule-deduplicate-exclude-fields": {kind: splitIntoBodyArray}, + "hookdeck gateway connection update --rule-deduplicate-include-fields": {kind: splitIntoBodyArray}, + "hookdeck gateway connection update --rule-retry-response-status-codes": {kind: splitIntoBodyArray}, + "hookdeck gateway connection upsert --rule-deduplicate-exclude-fields": {kind: splitIntoBodyArray}, + "hookdeck gateway connection upsert --rule-deduplicate-include-fields": {kind: splitIntoBodyArray}, + "hookdeck gateway connection upsert --rule-retry-response-status-codes": {kind: splitIntoBodyArray}, + "hookdeck gateway connection upsert --source-allowed-http-methods": {kind: splitIntoBodyArray}, + "hookdeck gateway request retry --connection-ids": {kind: splitIntoBodyArray}, + "hookdeck gateway source create --allowed-http-methods": {kind: splitIntoBodyArray}, + "hookdeck gateway source update --allowed-http-methods": {kind: splitIntoBodyArray}, + "hookdeck gateway source upsert --allowed-http-methods": {kind: splitIntoBodyArray}, + "hookdeck connection create --destination-oauth2-scopes": {kind: sentAsStringAPIConvertsCommas}, + "hookdeck connection upsert --destination-oauth2-scopes": {kind: sentAsStringAPIConvertsCommas}, + "hookdeck gateway connection create --destination-oauth2-scopes": {kind: sentAsStringAPIConvertsCommas}, + "hookdeck gateway connection upsert --destination-oauth2-scopes": {kind: sentAsStringAPIConvertsCommas}, + "hookdeck gateway metrics attempts --dimensions": {kind: splitIntoList}, + "hookdeck gateway metrics attempts --measures": {kind: splitIntoList}, + "hookdeck gateway metrics events --dimensions": {kind: splitIntoList}, + "hookdeck gateway metrics events --measures": {kind: splitIntoList}, + "hookdeck gateway metrics requests --dimensions": {kind: splitIntoList}, + "hookdeck gateway metrics requests --measures": {kind: splitIntoList}, + "hookdeck gateway metrics transformations --dimensions": {kind: splitIntoList}, + "hookdeck gateway metrics transformations --measures": {kind: splitIntoList}, + "hookdeck outpost attempt get --include": {kind: splitIntoList}, + "hookdeck outpost attempt list --destination-id": {kind: splitIntoList}, + "hookdeck outpost attempt list --destination-type": {kind: splitIntoList}, + "hookdeck outpost attempt list --event-id": {kind: splitIntoList}, + "hookdeck outpost attempt list --include": {kind: splitIntoList}, + "hookdeck outpost attempt list --tenant-id": {kind: splitIntoList}, + "hookdeck outpost attempt list --topic": {kind: splitIntoList}, + "hookdeck outpost destination create --topics": {kind: splitIntoList}, + "hookdeck outpost destination list --topics": {kind: splitIntoList}, + "hookdeck outpost destination list --type": {kind: splitIntoList}, + "hookdeck outpost destination update --topics": {kind: splitIntoList}, + "hookdeck outpost event list --destination-id": {kind: splitIntoList}, + "hookdeck outpost event list --id": {kind: splitIntoList}, + "hookdeck outpost event list --tenant-id": {kind: splitIntoList}, + "hookdeck outpost event list --topic": {kind: splitIntoList}, + "hookdeck outpost metrics attempts --dimensions": {kind: splitIntoList}, + "hookdeck outpost metrics attempts --measures": {kind: splitIntoList}, + "hookdeck outpost metrics events --dimensions": {kind: splitIntoList}, + "hookdeck outpost metrics events --measures": {kind: splitIntoList}, + "hookdeck outpost tenant list --id": {kind: splitIntoList}, +} + +var commaSeparated = regexp.MustCompile(`(?i)comma[- ]separated`) + +func discoverCommaSeparatedFlags() map[string]bool { + found := map[string]bool{} + var walk func(c *cobra.Command) + walk = func(c *cobra.Command) { + // Flags() and PersistentFlags(), not LocalFlags(): LocalFlags merges + // every parent's persistent flags into the command as a side effect, + // which mutates the shared rootCmd for every later test in the package. + // It put root's hidden --api-key onto outpost mcp and failed + // TestOutpostMCPCommandIsRegistered. + visit := func(f *pflag.Flag) { + if commaSeparated.MatchString(f.Usage) { + found[c.CommandPath()+" --"+f.Name] = true + } + } + c.Flags().VisitAll(visit) + c.PersistentFlags().VisitAll(visit) + for _, sub := range c.Commands() { + walk(sub) + } + } + walk(rootCmd) + return found +} + +func TestEveryCommaSeparatedFlagSendsAList(t *testing.T) { + found := discoverCommaSeparatedFlags() + if len(found) < 40 { + t.Fatalf("found only %d comma-separated flags; the walk is probably broken and the guard would pass vacuously", len(found)) + } + + for flag := range found { + handling, ok := commaSeparatedFlags[flag] + if !assert.True(t, ok, + "%s says comma-separated but is not in commaSeparatedFlags. Check how its value "+ + "reaches the API -- a query filter sent as one value returns nothing (#411) -- "+ + "and add it with its handling.", flag) { + continue + } + if handling.kind == sentAsQueryList { + assert.True(t, hookdeck.IsListValuedParam(handling.param), + "%s is a comma-separated query filter, but %q is not list-valued in hookdeck.listQuery, "+ + "so \"a,b\" is sent as one value and matches nothing", flag, handling.param) + } + } + + // A stale entry would quietly cover a different flag added under the same + // name later, so every entry must still exist. + for flag := range commaSeparatedFlags { + assert.True(t, found[flag], "%s is in commaSeparatedFlags but no longer exists or no longer says comma-separated", flag) + } +} diff --git a/pkg/hookdeck/list_query.go b/pkg/hookdeck/list_query.go index b30be2aa..01690f55 100644 --- a/pkg/hookdeck/list_query.go +++ b/pkg/hookdeck/list_query.go @@ -11,10 +11,21 @@ import ( // The comma-joined string used to be sent as one scalar value, which the API // matched against nothing: `--id evt_A,evt_B` returned zero rows with exit 0, // while each id on its own returned its row. See #411. +// +// delivery_group is declared the same way as id in the API -- a single value or +// an array, with no comma splitting -- and was sent as one value in the same +// way, so --delivery-group a,b returned nothing too. var listValuedParams = map[string]bool{ - "id": true, + "id": true, + "delivery_group": true, } +// IsListValuedParam reports whether a list-endpoint filter is sent as a list +// when given a comma-separated value. The CLI's guard over flags documented as +// comma-separated uses it, so a flag cannot promise a list the query does not +// send. +func IsListValuedParam(name string) bool { return listValuedParams[name] } + // listParamKey is how a repeated value is spelled on the wire. var listParamKey = func(key string) string { return key + "[]" } diff --git a/pkg/hookdeck/list_query_test.go b/pkg/hookdeck/list_query_test.go index 6016f64b..23c79870 100644 --- a/pkg/hookdeck/list_query_test.go +++ b/pkg/hookdeck/list_query_test.go @@ -68,3 +68,38 @@ func TestListEventsSendsIDsAsAList(t *testing.T) { assert.Equal(t, []string{"evt_A", "evt_B"}, got["id[]"]) assert.Empty(t, got["id"]) } + +// delivery_group is declared exactly like id in the API -- a single value or an +// array, no comma splitting -- so "a,b" as one value matched nothing and +// --delivery-group a,b returned zero rows with exit 0, the #411 shape. Found by +// the guard over flags documented as comma-separated. +func TestListQueryExpandsCommaSeparatedDeliveryGroups(t *testing.T) { + q := listQuery(map[string]string{"delivery_group": "cus_1,cus_2"}) + assert.Equal(t, []string{"cus_1", "cus_2"}, q["delivery_group[]"]) + assert.Empty(t, q["delivery_group"]) +} + +// GetRequestEvents built its own query and so was missed by the #411 fix. This +// covers it using the shared builder, for both list-valued filters. +func TestGetRequestEventsSendsListValuedFiltersAsLists(t *testing.T) { + var got url.Values + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + got = r.URL.Query() + _ = json.NewEncoder(w).Encode(map[string]any{"models": []any{}, "pagination": map[string]any{}}) + })) + defer srv.Close() + + base, err := url.Parse(srv.URL) + require.NoError(t, err) + c := &Client{BaseURL: base} + + _, err = c.GetRequestEvents(context.Background(), "req_1", map[string]string{ + "id": "evt_A,evt_B", + "delivery_group": "cus_1,cus_2", + }) + require.NoError(t, err) + + assert.Equal(t, []string{"evt_A", "evt_B"}, got["id[]"]) + assert.Equal(t, []string{"cus_1", "cus_2"}, got["delivery_group[]"]) + assert.Empty(t, got["delivery_group"]) +} diff --git a/pkg/hookdeck/requests.go b/pkg/hookdeck/requests.go index 53ec6de2..a91b9343 100644 --- a/pkg/hookdeck/requests.go +++ b/pkg/hookdeck/requests.go @@ -133,13 +133,12 @@ func (c *Client) GetRequestEvents(ctx context.Context, requestID string, params if err != nil { return nil, err } + // Through listQuery like the other event lists, so a comma-separated + // list-valued filter is sent as a list. This built its own query and was + // missed by #411, so --delivery-group a,b still went out as one value. queryStr := "" if len(params) > 0 { - q := url.Values{} - for k, v := range params { - q.Add(k, v) - } - queryStr = q.Encode() + queryStr = listQuery(params).Encode() } resp, err := c.Get(ctx, path, queryStr, nil) if err != nil { diff --git a/pkg/mcpcore/input.go b/pkg/mcpcore/input.go index e35e7ca4..1f4da200 100644 --- a/pkg/mcpcore/input.go +++ b/pkg/mcpcore/input.go @@ -163,8 +163,17 @@ func (in Input) BoolOrStringE(key string) (*bool, error) { return nil, fmt.Errorf("%s must be true or false, got %v", key, v) } -// StringSlice returns the string slice for a key, or nil if missing. -func (in Input) StringSlice(key string) []string { +// stringSlice reads a value only when it is a JSON array, and returns nil for +// anything else -- including the bare or comma-separated string a model sends +// where an array is declared. +// +// It is unexported on purpose. As a public method, tool handlers read +// caller-supplied lists with it, and a string was dropped without an error: +// gateway_metrics_read told callers "measures is required" for a measures they +// had just passed (#440), and a merge later reverted the fix unnoticed. Tool +// code must use StringList, which accepts both forms; with this unexported, a +// handler that reaches for the array-only read no longer compiles. +func (in Input) stringSlice(key string) []string { v, ok := in[key] if !ok { return nil @@ -240,7 +249,7 @@ func SetPayloadSearchFilters(params map[string]string, in Input) error { // StringList reads a value that may be given either as an array of strings or, // mirroring the CLI's comma-separated flags, as a single string. func StringList(in Input, key string) []string { - if values := in.StringSlice(key); len(values) > 0 { + if values := in.stringSlice(key); len(values) > 0 { return values } raw := in.String(key) diff --git a/pkg/mcpcore/input_shape_guard_test.go b/pkg/mcpcore/input_shape_guard_test.go new file mode 100644 index 00000000..5ca4a187 --- /dev/null +++ b/pkg/mcpcore/input_shape_guard_test.go @@ -0,0 +1,73 @@ +package mcpcore + +import ( + "os" + "path/filepath" + "regexp" + "strings" + "testing" +) + +// Models routinely send a bare or comma-separated string where a tool declares +// an array. mcpcore.StringList accepts both; reading a caller-supplied list any +// other way drops the string without an error. That is how gateway_metrics_read +// came to tell callers "measures is required" for a measures they had passed +// (#440) -- and how a merge later put the bug back unnoticed. +// +// stringSlice is unexported, so tool code cannot call the array-only reader. +// This covers the remaining way round it: asserting .([]interface{}) directly +// on a tool argument. Each existing use is listed with the reason it is right; +// a new one fails here until someone decides it belongs on that list. +var arrayAssertionAllowed = map[string]string{ + // rules is an array of rule OBJECTS, not strings, so StringList does not + // apply -- and ruleList errors on a non-array rather than dropping it. + "gateway/mcp/tool_connections.go": "rules: array of objects, and a non-array is an error", +} + +var arrayAssertion = regexp.MustCompile(`\.\(\s*\[\](interface\{\}|any)\s*\)`) + +func TestToolArgumentsAreNotReadAsArraysDirectly(t *testing.T) { + for _, dir := range []string{"../gateway/mcp", "../outpost/mcp"} { + files, err := filepath.Glob(filepath.Join(dir, "*.go")) + if err != nil { + t.Fatal(err) + } + if len(files) == 0 { + t.Fatalf("no Go files found in %s; the guard would pass vacuously", dir) + } + for _, f := range files { + if strings.HasSuffix(f, "_test.go") { + continue + } + src, err := os.ReadFile(f) + if err != nil { + t.Fatal(err) + } + if !arrayAssertion.Match(src) { + continue + } + key := strings.TrimPrefix(filepath.ToSlash(f), "../") + if _, ok := arrayAssertionAllowed[key]; ok { + continue + } + t.Errorf("%s asserts .([]interface{}) on tool input. Read a list argument with "+ + "mcpcore.StringList, which also accepts the string forms models send; "+ + "if this really is not a list of strings, add the file to arrayAssertionAllowed with the reason.", key) + } + } +} + +// An allow-list entry for a file that no longer has the assertion is stale, and +// a stale entry would silently cover the next use added to that file. +func TestArrayAssertionAllowListIsCurrent(t *testing.T) { + for key := range arrayAssertionAllowed { + src, err := os.ReadFile(filepath.Join("..", key)) + if err != nil { + t.Errorf("allow-listed file %s: %v", key, err) + continue + } + if !arrayAssertion.Match(src) { + t.Errorf("%s is allow-listed but no longer asserts .([]interface{}); remove the entry", key) + } + } +} diff --git a/pkg/mcpcore/input_test.go b/pkg/mcpcore/input_test.go index e23814d1..d76aa321 100644 --- a/pkg/mcpcore/input_test.go +++ b/pkg/mcpcore/input_test.go @@ -69,8 +69,8 @@ func TestInput_Accessors(t *testing.T) { assert.Equal(t, 99, in.Int("nonexistent", 99)) assert.Equal(t, true, in.Bool("active")) assert.Equal(t, false, in.Bool("nonexistent")) - assert.Equal(t, []string{"a", "b"}, in.StringSlice("tags")) - assert.Nil(t, in.StringSlice("nonexistent")) + assert.Equal(t, []string{"a", "b"}, in.stringSlice("tags")) + assert.Nil(t, in.stringSlice("nonexistent")) bp := in.BoolOrString("active") require.NotNil(t, bp)