From 45e1ce70e2757b78b868768b93e05da8858bab85 Mon Sep 17 00:00:00 2001
From: Alex Hong <9397363+hongalex@users.noreply.github.com>
Date: Thu, 7 Nov 2024 14:39:47 -0800
Subject: [PATCH] fix(pubsub/pstest): make invalid filter return error instead
 of panic (#11087)

---
 pubsub/pstest/fake.go      | 15 ++++++++-------
 pubsub/pstest/fake_test.go | 31 ++++++++++++++++++++++++++++++-
 2 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/pubsub/pstest/fake.go b/pubsub/pstest/fake.go
index afcfe4a2a6b5..f9b16a675185 100644
--- a/pubsub/pstest/fake.go
+++ b/pubsub/pstest/fake.go
@@ -551,6 +551,14 @@ func (s *GServer) CreateSubscription(_ context.Context, ps *pb.Subscription) (*p
 	}
 
 	sub := newSubscription(top, &s.mu, s.now, deadLetterTopic, ps)
+	if ps.Filter != "" {
+		filter, err := parseFilter(ps.Filter)
+		if err != nil {
+			return nil, status.Errorf(codes.InvalidArgument, "bad filter: %v", err)
+		}
+		sub.filter = &filter
+	}
+
 	top.subs[ps.Name] = sub
 	s.subs[ps.Name] = sub
 	sub.start(&s.wg)
@@ -905,13 +913,6 @@ func newSubscription(t *topic, mu *sync.Mutex, timeNowFunc func() time.Time, dea
 		done:            make(chan struct{}),
 		timeNowFunc:     timeNowFunc,
 	}
-	if ps.Filter != "" {
-		filter, err := parseFilter(ps.Filter)
-		if err != nil {
-			panic(fmt.Sprintf("pstest: bad filter: %v", err))
-		}
-		sub.filter = &filter
-	}
 	return sub
 }
 
diff --git a/pubsub/pstest/fake_test.go b/pubsub/pstest/fake_test.go
index 98504606a6e6..a1c9aed80ace 100644
--- a/pubsub/pstest/fake_test.go
+++ b/pubsub/pstest/fake_test.go
@@ -1115,12 +1115,28 @@ func TestUpdateRetryPolicy(t *testing.T) {
 	}
 }
 
-func TestUpdateFilter(t *testing.T) {
+func TestSubscriptionFilter(t *testing.T) {
 	ctx := context.Background()
 	pclient, sclient, _, cleanup := newFake(ctx, t)
 	defer cleanup()
 
 	top := mustCreateTopic(ctx, t, pclient, &pb.Topic{Name: "projects/P/topics/T"})
+
+	// Creating a subscription with invalid filter should return an error.
+	_, err := sclient.CreateSubscription(ctx, &pb.Subscription{
+		Name:                  "projects/p/subscriptions/s",
+		Topic:                 top.Name,
+		AckDeadlineSeconds:    30,
+		EnableMessageOrdering: true,
+		Filter:                "bad filter",
+	})
+	if err == nil {
+		t.Fatal("expected bad filter error, got nil")
+	}
+	if st := status.Convert(err); st.Code() != codes.InvalidArgument {
+		t.Fatalf("got err status: %v, want: %v", st.Code(), codes.InvalidArgument)
+	}
+
 	sub := mustCreateSubscription(ctx, t, sclient, &pb.Subscription{
 		AckDeadlineSeconds: minAckDeadlineSecs,
 		Name:               "projects/P/subscriptions/S",
@@ -1143,6 +1159,19 @@ func TestUpdateFilter(t *testing.T) {
 	if got, want := updated.Filter, update.Filter; got != want {
 		t.Fatalf("got %v, want %v", got, want)
 	}
+
+	// Updating a subscription with bad filter should return an error.
+	update.Filter = "bad filter"
+	updated, err = sclient.UpdateSubscription(ctx, &pb.UpdateSubscriptionRequest{
+		Subscription: update,
+		UpdateMask:   &field_mask.FieldMask{Paths: []string{"filter"}},
+	})
+	if err == nil {
+		t.Fatal("expected bad filter error, got nil")
+	}
+	if st := status.Convert(err); st.Code() != codes.InvalidArgument {
+		t.Fatalf("got err status: %v, want: %v", st.Code(), codes.InvalidArgument)
+	}
 }
 
 func TestUpdateEnableExactlyOnceDelivery(t *testing.T) {