fix(pubsub): allow clearing of topic schema (#7980)

hongalex · acocuzzo · web-flow · commit 46fc06099852 · 2023-05-24T09:32:59.000-07:00
Co-authored-by: Anna Cocuzzo &lt;63511057+acocuzzo@users.noreply.github.com&gt;
diff --git a/pubsub/pstest/fake.go b/pubsub/pstest/fake.go
@@ -362,8 +362,7 @@ func (s *GServer) UpdateTopic(_ context.Context, req *pb.UpdateTopicRequest) (*p
 			}
 			t.proto.MessageRetentionDuration = req.Topic.MessageRetentionDuration
 		case "schema_settings":
-			// Clear this field.
-			t.proto.SchemaSettings = &pb.SchemaSettings{}
+			t.proto.SchemaSettings = req.Topic.SchemaSettings
 		case "schema_settings.schema":
 			if t.proto.SchemaSettings == nil {
 				t.proto.SchemaSettings = &pb.SchemaSettings{}
diff --git a/pubsub/topic.go b/pubsub/topic.go
@@ -407,6 +407,11 @@ func (t *Topic) updateRequest(cfg TopicConfigToUpdate) *pb.UpdateTopicRequest {
 		}
 		paths = append(paths, "message_retention_duration")
 	}
+	// Updating SchemaSettings' field masks are more complicated here
+	// since each field should be able to be independently edited, while
+	// preserving the current values for everything else. We also denote
+	// the zero value SchemaSetting to mean clearing or removing schema
+	// from the topic.
 	if cfg.SchemaSettings != nil {
 		pt.SchemaSettings = schemaSettingsToProto(cfg.SchemaSettings)
 		clearSchema := true
@@ -426,9 +431,10 @@ func (t *Topic) updateRequest(cfg TopicConfigToUpdate) *pb.UpdateTopicRequest {
 			paths = append(paths, "schema_settings.last_revision_id")
 			clearSchema = false
 		}
-		// Clear the schema if none of it's value changes.
+		// Clear the schema if all of its values are equal to the zero value.
 		if clearSchema {
 			paths = append(paths, "schema_settings")
+			pt.SchemaSettings = nil
 		}
 	}
 	return &pb.UpdateTopicRequest{
diff --git a/pubsub/topic_test.go b/pubsub/topic_test.go
@@ -342,8 +342,8 @@ func TestUpdateTopic_SchemaSettings(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if !testutil.Equal(config3.SchemaSettings, settings, opt) {
-		t.Errorf("\ngot  %+v\nwant %+v", config3.SchemaSettings, settings)
+	if config3.SchemaSettings != nil {
+		t.Errorf("got: %+v, want nil", config3.SchemaSettings)
 	}
 }
 

Original file line number	Diff line number	Diff line change
`@@ -407,6 +407,11 @@ func (t Topic) updateRequest(cfg TopicConfigToUpdate) pb.UpdateTopicRequest {`
`407`	`407`	`}`
`408`	`408`	`paths = append(paths, "message_retention_duration")`
`409`	`409`	`}`
	`410`	`+ // Updating SchemaSettings' field masks are more complicated here`
	`411`	`+ // since each field should be able to be independently edited, while`
	`412`	`+ // preserving the current values for everything else. We also denote`
	`413`	`+ // the zero value SchemaSetting to mean clearing or removing schema`
	`414`	`+ // from the topic.`
`410`	`415`	`if cfg.SchemaSettings != nil {`
`411`	`416`	`pt.SchemaSettings = schemaSettingsToProto(cfg.SchemaSettings)`
`412`	`417`	`clearSchema := true`
`@@ -426,9 +431,10 @@ func (t Topic) updateRequest(cfg TopicConfigToUpdate) pb.UpdateTopicRequest {`
`426`	`431`	`paths = append(paths, "schema_settings.last_revision_id")`
`427`	`432`	`clearSchema = false`
`428`	`433`	`}`
`429`		`- // Clear the schema if none of it's value changes.`
	`434`	`+ // Clear the schema if all of its values are equal to the zero value.`
`430`	`435`	`if clearSchema {`
`431`	`436`	`paths = append(paths, "schema_settings")`
	`437`	`+ pt.SchemaSettings = nil`
`432`	`438`	`}`
`433`	`439`	`}`
`434`	`440`	`return &pb.UpdateTopicRequest{`
Original file line number	Diff line number	Diff line change
`@@ -342,8 +342,8 @@ func TestUpdateTopic_SchemaSettings(t *testing.T) {`
`342`	`342`	`if err != nil {`
`343`	`343`	`t.Fatal(err)`
`344`	`344`	`}`
`345`		`- if !testutil.Equal(config3.SchemaSettings, settings, opt) {`
`346`		`- t.Errorf("\ngot %+v\nwant %+v", config3.SchemaSettings, settings)`
	`345`	`+ if config3.SchemaSettings != nil {`
	`346`	`+ t.Errorf("got: %+v, want nil", config3.SchemaSettings)`
`347`	`347`	`}`
`348`	`348`	`}`
`349`	`349`