Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion pkg/gateway/mcp/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1014,7 +1014,14 @@ func TestEventsList_PayloadFilters(t *testing.T) {
func TestEventsList_MetadataFilters(t *testing.T) {
session := mockAPIWithClient(t, map[string]http.HandlerFunc{
hookdeck.APIPathPrefix + "/events": func(w http.ResponseWriter, r *http.Request) {
assert.Equal(t, "evt_1,evt_2", r.URL.Query().Get("id"))
// This assertion used to be `"evt_1,evt_2" == Get("id")` -- the
// comma-joined scalar. That is the wire format of bug #411: sent to
// the live API it matches nothing and returns zero rows, where the
// repeated id[] form returns both. So the test was certifying the
// bug rather than guarding against it. Verified against the real API
// on event, request and transformation list before changing it.
assert.Equal(t, []string{"evt_1", "evt_2"}, r.URL.Query()["id[]"])
assert.Empty(t, r.URL.Query().Get("id"), "the comma-joined scalar matches nothing")
assert.Equal(t, "3", r.URL.Query().Get("attempts"))
assert.Equal(t, "cli_abc", r.URL.Query().Get("cli_id"))
assert.Equal(t, "cus_123", r.URL.Query().Get("delivery_group"))
Expand Down
5 changes: 1 addition & 4 deletions pkg/hookdeck/events.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,7 @@ type EventListResponse struct {

// ListEvents retrieves events with optional filters (params: webhook_id, status, source_id, destination_id, limit, order_by, dir, next, prev, etc.)
func (c *Client) ListEvents(ctx context.Context, params map[string]string) (*EventListResponse, error) {
queryParams := url.Values{}
for k, v := range params {
queryParams.Add(k, v)
}
queryParams := listQuery(params)
resp, err := c.Get(ctx, APIPathPrefix+"/events", queryParams.Encode(), nil)
if err != nil {
return nil, err
Expand Down
38 changes: 38 additions & 0 deletions pkg/hookdeck/list_query.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package hookdeck

import (
"net/url"
"strings"
)

// listValuedParams are list-endpoint filters the CLI documents as accepting a
// comma-separated list ("Filter by event ID(s) (comma-separated)").
//
// 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.
var listValuedParams = map[string]bool{
"id": true,
}

// listParamKey is how a repeated value is spelled on the wire.
var listParamKey = func(key string) string { return key + "[]" }

// listQuery builds the query for a list endpoint, expanding list-valued
// filters into one query value per item. Everything else passes through
// unchanged, so a single id is sent exactly as it always was.
func listQuery(params map[string]string) url.Values {
q := url.Values{}
for k, v := range params {
if listValuedParams[k] && strings.Contains(v, ",") {
for _, part := range strings.Split(v, ",") {
if part = strings.TrimSpace(part); part != "" {
q.Add(listParamKey(k), part)
}
}
continue
}
q.Add(k, v)
}
return q
}
70 changes: 70 additions & 0 deletions pkg/hookdeck/list_query_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
package hookdeck

import (
"context"
"encoding/json"
"net/http"
"net/http/httptest"
"net/url"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

// `--id evt_A,evt_B` sent the comma-joined string as one scalar `id`, which the
// API matched against nothing: zero rows, exit 0, while each id on its own
// returned its row (#411). The API takes a list as repeated `id[]` values --
// verified against the live API on event, request and transformation list
// before this fix was written, rather than inferred from another endpoint.
func TestListQueryExpandsCommaSeparatedIDs(t *testing.T) {
q := listQuery(map[string]string{"id": "evt_A,evt_B"})

assert.Equal(t, []string{"evt_A", "evt_B"}, q["id[]"], "each id becomes its own id[] value")
assert.Empty(t, q["id"], "the comma-joined scalar must not be sent")
}

func TestListQueryLeavesOtherShapesAlone(t *testing.T) {
for _, tt := range []struct {
name string
params map[string]string
want url.Values
}{
{"a single id is sent exactly as before", map[string]string{"id": "evt_A"},
url.Values{"id": {"evt_A"}}},
{"spaces around commas are trimmed", map[string]string{"id": "evt_A, evt_B"},
url.Values{"id[]": {"evt_A", "evt_B"}}},
{"empty items are dropped", map[string]string{"id": "evt_A,,evt_B,"},
url.Values{"id[]": {"evt_A", "evt_B"}}},
// Only filters documented as lists are expanded. A comma elsewhere is
// data, and splitting it would be a different silent-wrong-answer bug.
{"a comma in a non-list filter is passed through", map[string]string{"search_term": "a,b"},
url.Values{"search_term": {"a,b"}}},
} {
t.Run(tt.name, func(t *testing.T) {
assert.Equal(t, tt.want, listQuery(tt.params))
})
}
}

// The unit tests above cover the builder; this covers ListEvents actually using
// it. Without a test at this level, reverting ListEvents to the old loop leaves
// the builder tests green while `--id A,B` goes back to returning nothing.
func TestListEventsSendsIDsAsAList(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.ListEvents(context.Background(), map[string]string{"id": "evt_A,evt_B"})
require.NoError(t, err)

assert.Equal(t, []string{"evt_A", "evt_B"}, got["id[]"])
assert.Empty(t, got["id"])
}
5 changes: 1 addition & 4 deletions pkg/hookdeck/requests.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,7 @@ type RequestRetryRequest struct {

// ListRequests retrieves requests with optional filters
func (c *Client) ListRequests(ctx context.Context, params map[string]string) (*RequestListResponse, error) {
queryParams := url.Values{}
for k, v := range params {
queryParams.Add(k, v)
}
queryParams := listQuery(params)
resp, err := c.Get(ctx, APIPathPrefix+"/requests", queryParams.Encode(), nil)
if err != nil {
return nil, err
Expand Down
5 changes: 1 addition & 4 deletions pkg/hookdeck/transformations.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,10 +149,7 @@ type TransformationExecutionListResponse struct {

// ListTransformations retrieves a list of transformations with optional filters
func (c *Client) ListTransformations(ctx context.Context, params map[string]string) (*TransformationListResponse, error) {
queryParams := url.Values{}
for k, v := range params {
queryParams.Add(k, v)
}
queryParams := listQuery(params)

resp, err := c.Get(ctx, APIPathPrefix+"/transformations", queryParams.Encode(), nil)
if err != nil {
Expand Down
Loading