diff --git a/cmd/subscribe.go b/cmd/subscribe.go index 91ace2c032..70938f86f5 100644 --- a/cmd/subscribe.go +++ b/cmd/subscribe.go @@ -59,25 +59,30 @@ func runSubscribe(cmd *cobra.Command) (err error) { } // add subscription to function - f.Deploy.Subscriptions = updateOrAddSubscription(f.Deploy.Subscriptions, cfg) + subscriptions, err := updateOrAddSubscription(f.Deploy.Subscriptions, cfg) + if err != nil { + return err + } + f.Deploy.Subscriptions = subscriptions // pump it return f.Write() } -func extractFilterMap(filters []string) map[string]string { +func extractFilterMap(filters []string) (map[string]string, error) { subscriptionFilters := make(map[string]string) for _, filter := range filters { - kv := strings.Split(filter, "=") + kv := strings.SplitN(filter, "=", 2) if len(kv) != 2 { - fmt.Println("Invalid pair:", filter) - continue + return nil, fmt.Errorf("invalid filter %q: must be in the form key=value", filter) + } + key, value := kv[0], kv[1] + if key == "" { + return nil, fmt.Errorf("invalid filter %q: key must not be empty", filter) } - key := kv[0] - value := kv[1] subscriptionFilters[key] = value } - return subscriptionFilters + return subscriptionFilters, nil } type subscibeConfig struct { @@ -85,9 +90,12 @@ type subscibeConfig struct { Source string } -func updateOrAddSubscription(subscriptions []fn.KnativeSubscription, cfg subscibeConfig) []fn.KnativeSubscription { +func updateOrAddSubscription(subscriptions []fn.KnativeSubscription, cfg subscibeConfig) ([]fn.KnativeSubscription, error) { found := false - newFilters := extractFilterMap(cfg.Filter) + newFilters, err := extractFilterMap(cfg.Filter) + if err != nil { + return nil, err + } // Iterate over subscriptions to find if one with the same source already exists for i, subscription := range subscriptions { @@ -114,7 +122,7 @@ func updateOrAddSubscription(subscriptions []fn.KnativeSubscription, cfg subscib Filters: newFilters, }) } - return subscriptions + return subscriptions, nil } func newSubscribeConfig(cmd *cobra.Command) (c subscibeConfig) { diff --git a/cmd/subscribe_test.go b/cmd/subscribe_test.go index b013cff914..8f559f4320 100644 --- a/cmd/subscribe_test.go +++ b/cmd/subscribe_test.go @@ -1,6 +1,7 @@ package cmd import ( + "reflect" "testing" fn "knative.dev/func/pkg/functions" @@ -286,3 +287,134 @@ func TestSubscribeWithDuplicated(t *testing.T) { } } + +// TestSubscribeRejectsMalformedFilter ensures that a --filter value which is +// not in key=value form fails the command rather than being silently dropped, +// and that nothing is written to func.yaml. +func TestSubscribeRejectsMalformedFilter(t *testing.T) { + root := FromTempDirectory(t) + + _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}) + if err != nil { + t.Fatal(err) + } + + cmd := NewSubscribeCmd() + cmd.SetArgs([]string{"--source", "my-broker", "--filter", "badfilter"}) + + if err := cmd.Execute(); err == nil { + t.Fatal("Expected an error for a malformed filter, but got nil") + } + + // The function on disk must be left untouched. + f, err := fn.NewFunction(root) + if err != nil { + t.Fatal(err) + } + + if len(f.Deploy.Subscriptions) != 0 { + t.Fatalf("Expected no subscriptions to be written, but got '%v'", f.Deploy.Subscriptions) + } +} + +// TestSubscribeAllowsFilterValueContainingEquals ensures a filter value which +// itself contains "=" is preserved in full rather than discarded. +func TestSubscribeAllowsFilterValueContainingEquals(t *testing.T) { + root := FromTempDirectory(t) + + _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}) + if err != nil { + t.Fatal(err) + } + + cmd := NewSubscribeCmd() + cmd.SetArgs([]string{"--source", "my-broker", "--filter", "foo=bar=baz"}) + + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + + f, err := fn.NewFunction(root) + if err != nil { + t.Fatal(err) + } + + if f.Deploy.Subscriptions == nil { + t.Fatal("Expected subscription to be present ") + } + if f.Deploy.Subscriptions[0].Filters["foo"] != "bar=baz" { + t.Fatalf("Expected subscription filter for 'foo' to be 'bar=baz', but got '%v'", f.Deploy.Subscriptions[0].Filters["foo"]) + } +} + +// TestExtractFilterMap covers filter parsing directly, including values which +// themselves contain "=" and the malformed forms which must be rejected. +func TestExtractFilterMap(t *testing.T) { + tests := []struct { + name string + filters []string + want map[string]string + wantErr bool + }{ + { + name: "single valid filter", + filters: []string{"type=com.example"}, + want: map[string]string{"type": "com.example"}, + }, + { + name: "multiple valid filters", + filters: []string{"type=com.example", "extension=my-value"}, + want: map[string]string{"type": "com.example", "extension": "my-value"}, + }, + { + name: "value containing equals is preserved", + filters: []string{"foo=bar=baz"}, + want: map[string]string{"foo": "bar=baz"}, + }, + { + name: "empty value is allowed", + filters: []string{"key="}, + want: map[string]string{"key": ""}, + }, + { + name: "no filters yields empty map", + filters: []string{}, + want: map[string]string{}, + }, + { + name: "missing equals is rejected", + filters: []string{"badfilter"}, + wantErr: true, + }, + { + name: "empty key is rejected", + filters: []string{"=value"}, + wantErr: true, + }, + { + name: "one malformed filter rejects the whole set", + filters: []string{"type=com.example", "badfilter"}, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := extractFilterMap(tt.filters) + + if tt.wantErr { + if err == nil { + t.Fatalf("Expected an error, but got nil (result '%v')", got) + } + return + } + + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("Expected '%v', but got '%v'", tt.want, got) + } + }) + } +}