pubsub: Unable to update or clear SchemaSettings

[bcol-google]

The way SchemaSettings messages are added to UpdateTopicRequests is incorrect.

If you want to clear the schema_settings field on a topic, you pass a zero-valued SchemaSettings in the TopicConfigToUpdate struct per this comment

However, in this section of code which handles that field, it incorrectly passes the empty struct to the UpdateTopicRequest when it should be nil in the case where we wish to clear our schema. Also, when we wish to update a field in the SchemaSettings, we are neglecting to set the schema_settings update mask path. This section needs to be reworked to:

1. Have a nil schema_settings field and the schema_settings update mask path returned in the UpdateTopicRequest in the case where we pass a zero-valued SchemaSettings in the TopicConfigToUpdate struct.
2. Ensure we pass the schema_settings update mask path when we pass a non-nil SchemaSettings in the TopicConfigToUpdate struct.

[hongalex]

Thanks for filing this issue.

I agree with 1, and it is a bug that we aren't passing in a nil schema_settings. Will get a PR opened to fix this.

Also, when we wish to update a field in the SchemaSettings, we are neglecting to set the schema_settings update mask path

This should be correct. We are setting the individual field masks for the sub-fields (e.g. schema_settings.schema, schema_settings.first_revision_id, etc), which is supported so one field can be updated independently of the others.

[saurav1991]

Hey @hongalex looks like the fix released as part of thie issue does not fully solve the issue with SchemaSettings update. For e.g it does not allow us to unset the values of FirstRevisionID or LastRevisionID if they are set on a schema. Here is the code snippet we are using:

tc := pubsub.TopicConfigToUpdate{SchemaSettings: &pubsub.SchemaSettings{
		LastRevisionID:  "",
		FirstRevisionID: "",
		Schema:          fmt.Sprintf("projects/%s/schemas/%s", p.projectID, schemaID),
		Encoding:        pubsub.EncodingJSON,
	}}

	_, err = topic.Update(p.ctx, tc)

This does not clear the values of the FirstRevisionID and LastRevisionID set on a topic

[hongalex]

Thanks for bringing this up. As making a change to this would necessitate a breaking change to the client library, we don't support clearing individual fields and you must clear the whole schema object. To support clearing these fields, we would have to take in optional First/LastRevisionID, to allow nil and also zero value states. Any code that reads these values as strings would be broken unfortunately.

For this specific use case, I recommend using the underlying gRPC library to clear individual Last/FirstRevisionID. For completeness, here's an example of how to do so:

import (
	...
	pubsub "cloud.google.com/go/pubsub/apiv1"
	pb "cloud.google.com/go/pubsub/apiv1/pubsubpb"
	"google.golang.org/protobuf/types/known/fieldmaskpb"
)
...

ctx := context.Background()
c, err := pubsub.NewPublisherClient(ctx)
if err != nil {
	panic(err)
}

// Replace with your topic name.
topicName := "projects/p/topics/t"
topic := &pb.Topic{
	Name: topicName,
	SchemaSettings: &pb.SchemaSettings{
		FirstRevisionId: "",
		LastRevisionId:  "",
	},
}

updateTopicReq := &pb.UpdateTopicRequest{
	Topic: topic,
	UpdateMask: &fieldmaskpb.FieldMask{
		Paths: []string{"schema_settings.first_revision_id", "schema_settings.last_revision_id"},
	},
}
_, err = c.UpdateTopic(ctx, updateTopicReq)
if err != nil {
	panic(err)
}

[irfanhabib]

As making a change to this would necessitate a breaking change to the client library, we don't support clearing individual fields and you must clear the whole schema object.

Would it be possible to support this in a future major release? I understand that you would be hesitant to roll out a backwards incompatible change as part of a minor release.

[hongalex]

Although we are considering a major version release of the Pub/Sub client, we are leaning towards only supporting the autogenerated gRPC library (cloud.google.com/go/apiv1 in the example above) for all admin (create, get, update, delete, etc) operations to limit these kinds of issues where the implementation of the admin API does not expose the full functionality of the service itself. This isn't set in stone yet and we welcome feedback on this.

[meredithslota]

Since the bug described was fixed with #7980, I'm going to convert this to a feature request to capture the discussion around apiv1 usage — but in general the path that @hongalex describes is the preferred approach (so that the entirety of the admin API is exposed for users automatically).

[hongalex]

Since discussion seem quiet for now and since the original issue (can't clear) is resolved, I'll close this issue.