Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(pubsub): allow updating topic schema fields individually #7362

Merged
merged 11 commits into from
Feb 27, 2023
Merged

fix(pubsub): allow updating topic schema fields individually #7362

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
697e237
fix(pubsub): allow updating topic schema fields individually
hongalex Feb 3, 2023
c444eeb
fix schema settings field mask setting
hongalex Feb 6, 2023
cd1b90f
revert to specifying each field in mask individually
hongalex Feb 6, 2023
f7513d8
Merge branch 'main' into fix-pubsub-schema-revision
hongalex Feb 7, 2023
c9beae8
support update field masks in fake
hongalex Feb 7, 2023
e980aa9
Merge branch 'main' into fix-pubsub-schema-revision
hongalex Feb 8, 2023
209fea0
allow clearing schema
hongalex Feb 24, 2023
c3c5f01
Merge branch 'main' into fix-pubsub-schema-revision
hongalex Feb 24, 2023
e2a0f6a
Merge branch 'main' into fix-pubsub-schema-revision
hongalex Feb 27, 2023
04347d8
Merge branch 'main' into fix-pubsub-schema-revision
hongalex Feb 27, 2023
4a827ea
Merge branch 'main' into fix-pubsub-schema-revision
hongalex Feb 27, 2023
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
22 changes: 20 additions & 2 deletions pubsub/pstest/fake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,8 +368,26 @@ func (s *GServer) UpdateTopic(_ context.Context, req *pb.UpdateTopicRequest) (*p
return nil, err
}
t.proto.MessageRetentionDuration = req.Topic.MessageRetentionDuration
case "schema_settings":
t.proto.SchemaSettings = req.Topic.SchemaSettings
case "schema_settings.schema":
if t.proto.SchemaSettings == nil {
t.proto.SchemaSettings = &pb.SchemaSettings{}
}
t.proto.SchemaSettings.Schema = req.Topic.SchemaSettings.Schema
case "schema_settings.encoding":
if t.proto.SchemaSettings == nil {
t.proto.SchemaSettings = &pb.SchemaSettings{}
}
t.proto.SchemaSettings.Encoding = req.Topic.SchemaSettings.Encoding
case "schema_settings.first_revision_id":
if t.proto.SchemaSettings == nil {
t.proto.SchemaSettings = &pb.SchemaSettings{}
}
t.proto.SchemaSettings.FirstRevisionId = req.Topic.SchemaSettings.FirstRevisionId
case "schema_settings.last_revision_id":
if t.proto.SchemaSettings == nil {
t.proto.SchemaSettings = &pb.SchemaSettings{}
}
t.proto.SchemaSettings.LastRevisionId = req.Topic.SchemaSettings.LastRevisionId
default:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is also possible to just have schema_settings, which will update all of them. Though I guess the Go library is setting each field explicitly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah each field is set explicitly so updating all of them at once is possible too.

return nil, status.Errorf(codes.InvalidArgument, "unknown field name %q", path)
}
Expand Down
13 changes: 12 additions & 1 deletion pubsub/topic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,18 @@ func (t *Topic) updateRequest(cfg TopicConfigToUpdate) *pb.UpdateTopicRequest {
}
if cfg.SchemaSettings != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does one remove the schema config entirely?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I missed this. I added a section that handles clearing SchemaSettings when it detects the zero value, similar to MessageStoragePolicy.

pt.SchemaSettings = schemaSettingsToProto(cfg.SchemaSettings)
paths = append(paths, "schema_settings")
if pt.SchemaSettings.Schema != "" {
paths = append(paths, "schema_settings.schema")
}
if pt.SchemaSettings.Encoding != pb.Encoding_ENCODING_UNSPECIFIED {
paths = append(paths, "schema_settings.encoding")
}
if pt.SchemaSettings.FirstRevisionId != "" {
paths = append(paths, "schema_settings.first_revision_id")
}
if pt.SchemaSettings.LastRevisionId != "" {
paths = append(paths, "schema_settings.last_revision_id")
}
}
return &pb.UpdateTopicRequest{
Topic: pt,
Expand Down
4 changes: 2 additions & 2 deletions pubsub/topic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,8 +331,8 @@ func TestUpdateTopic_SchemaSettings(t *testing.T) {
if err != nil {
t.Fatal(err)
}
if !testutil.Equal(config2.SchemaSettings, settings) {
t.Errorf("\ngot %+v\nwant %+v", config2, settings)
if !testutil.Equal(config2.SchemaSettings, settings, opt) {
t.Errorf("\ngot %+v\nwant %+v", config2.SchemaSettings, settings)
}
}

Expand Down