feat: Pub/sub - #244
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new Google Cloud Pub/Sub client library, including core client functionality, topic and subscription management, and support for the Pub/Sub emulator. The review identified several critical issues: a race condition and token refresh problem in the authentication logic, a resource leak where the auth client is not closed, an overly restrictive project ID requirement for the emulator, an incorrect parsing logic for IPv6 emulator hosts, and code duplication in message mapping. I have recommended addressing these issues to improve the reliability and maintainability of the client.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…add license/changelog
…tage test deletion
…ll integration tests
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds an experimental Google Cloud Pub/Sub client for Dart, supporting topic and subscription management along with message publishing and pull operations. Key feedback includes supporting full resource names for cross-project access, optimizing StreamingPull with bidirectional communication, and correcting Windows-specific environment variable retrieval logic. A minor style correction for the package description was also noted.
|
/gcbrun |
jonasfj
left a comment
There was a problem hiding this comment.
Mostly just nits.
We could consider landing all the generated files first, if you want a more detailed review of the public API.
Introduce the handwritten client code (PubSub, Topic, Subscription, Message) on top of the generated gRPC code. - Implement PubSub client with emulator support. - Implement Topic and Subscription resource management. - Implement streaming pull and unary fallback for message acknowledgments. - Add unit and integration tests. - Add usage examples in README and example/example.dart. TAG=agy CONV=06ddc172-e8da-4e30-bab8-139c539520f9
- Make ReceivedMessage constructor private (ReceivedMessage._). - Introduce package-private createReceivedMessage helper annotated with @internal. - Update client.dart and unit_test.dart to use the helper. TAG=agy CONV=06ddc172-e8da-4e30-bab8-139c539520f9
Simplify ReceivedMessage by removing ack() and modifyAckDeadline() methods, which removes the need for storing internal callback handlers. - Make ReceivedMessage constructor public and simple. - Update client.dart to map ReceivedMessage without callbacks. - Update unit tests and integration tests to use Subscription.acknowledge() and Subscription.modifyAckDeadline() instead of message methods. - Fix linter warning in client.dart (use tearoff). TAG=agy CONV=06ddc172-e8da-4e30-bab8-139c539520f9
Add inline comment to _calculateProjectId explaining that we only fall back to 'test-project' when the emulator is active. TAG=agy CONV=06ddc172-e8da-4e30-bab8-139c539520f9
…tegy to resolve divergence
…with latest API design - Changed acknowledge and modifyAckDeadline to take ReceivedMessage and return void (fire-and-forget). - Added acknowledgeNow and modifyAckDeadlineNow to support immediate, awaitable operations on List<ReceivedMessage>. - Updated tests and examples accordingly. TAG=agy CONV=bc7c6164-e9cb-4a0f-b326-7752c814e1ce
| @@ -0,0 +1,93 @@ | |||
| // Copyright 2026 Google LLC | |||
There was a problem hiding this comment.
This looks copied from pkgs/google_cloud_storage/lib/src/storage_emulator_host_vm.dart
Maybe this functionality should be moved into package:google_cloud?
There was a problem hiding this comment.
Agreed it's duplicated from package:google_cloud_storage. I'd like to propose punting that refactoring for now and addressing it in a follow up across both packages.
| final resolvedProjectId = _calculateProjectId(projectId, emulatorHost); | ||
| if (resolvedProjectId == null) { | ||
| throw ArgumentError( | ||
| 'A project ID is required, but none was provided or could be ' |
There was a problem hiding this comment.
Isn't this a StateError?
There was a problem hiding this comment.
Good point. I've updated this to throw an ArgumentError from the constructor if the project ID is missing.
| /// For authentication, an [authenticator] can be supplied to obtain | ||
| /// and refresh access credentials for authenticating gRPC requests. | ||
| /// | ||
| /// If no [authenticator] is provided, requests are made without |
There was a problem hiding this comment.
Would it make sense to use ADC like the GCS and the other languages' pub sub clients?
There was a problem hiding this comment.
Done. The client now falls back to using applicationDefaultCredentialsAuthenticator if the emulator is not in use.
| await _publisher.deleteTopic(request, options: await _callOptions); | ||
| return true; | ||
| } on GrpcError catch (e) { | ||
| if (e.code == StatusCode.notFound) { |
There was a problem hiding this comment.
In package:google_cloud_storage, I throw in this case. The other client libraries with exceptions also throw here. I think that we should be consistent, at least within our own packages.
Does an api expert (@natebosch ?) have an opinion?
There was a problem hiding this comment.
I think either design is OK. Since we have precedent I lean towards matching what we already do elsewhere.
| options: await _callOptions, | ||
| ); | ||
| return subscriptionName(name); | ||
| } on GrpcError catch (e) { |
There was a problem hiding this comment.
Maybe you want to handle this exception mapping in a function?
There was a problem hiding this comment.
Done. I've extracted this into a _mapGrpcError helper function.
| // limitations under the License. | ||
|
|
||
| /// Base class for all exceptions thrown by the google_cloud_pubsub package. | ||
| abstract final class PubSubException implements Exception {} |
There was a problem hiding this comment.
Do you want to define your own exception hierarchy? The other languages' clients use their generic rpc exception types (as does package:google_cloud_storage).
There was a problem hiding this comment.
I've defined a custom PubSubException hierarchy to map from the underlying gRPC exceptions (matching what we do in packages like google_cloud_storage).
There was a problem hiding this comment.
google_cloud_storage doesn't define it's own exceptions, it uses the exceptions defined in package:google_cloud_rpc.
I just checked and the Python pubsub client throws google.api_core.exceptions.NotFound if a topic is not found.
If we want to diverge from other languages' clients and a client-specific exception hierarchy for each client, then I think that we need a reason why.
There was a problem hiding this comment.
Yeah, using google_cloud_rpc exceptions is a good idea.
However, when I went to integrate it, ServiceException (and all its subclasses like BadRequestException, NotFoundException, etc.) requires a non-optional http.BaseResponse response. Because google_cloud_pubsub uses gRPC over HTTP/2, we don't have an http.BaseResponse to pass.
Making response and responseBody optional in google_cloud_rpc would be a breaking change for existing google_cloud_rpc users (changing final http.BaseResponse response to http.BaseResponse?).
Are you open to making that breaking change in google_cloud_rpc (e.g. bumping to 0.6.0), or do you have another preferred way to integrate gRPC clients with the google_cloud_rpc exception hierarchy?
| @@ -0,0 +1,334 @@ | |||
| @TestOn('vm') | |||
There was a problem hiding this comment.
Do you want to have one test file per method under test? This is going to get pretty big quickly.
There was a problem hiding this comment.
Done. Split integration_test.dart into individual test files per operation under test (create_topic_test.dart, delete_topic_test.dart, publish_test.dart, create_subscription_test.dart, pull_test.dart, streaming_pull_test.dart, acknowledge_test.dart, modify_ack_deadline_test.dart) and created test_utils.dart for shared setup.
There was a problem hiding this comment.
Yes, I've gone ahead and split integration_test.dart into multiple smaller test files by method so it stays manageable.
| if (_isEmulator) { | ||
| return CallOptions(); | ||
| } | ||
| final authenticator = _authenticator; | ||
| if (authenticator == null) { | ||
| return CallOptions(); | ||
| } | ||
| return authenticator.toCallOptions; |
There was a problem hiding this comment.
[optional] I like to collapse early return guard clauses when they fit on one line. Also we can use ?? for the second null check.
| if (_isEmulator) { | |
| return CallOptions(); | |
| } | |
| final authenticator = _authenticator; | |
| if (authenticator == null) { | |
| return CallOptions(); | |
| } | |
| return authenticator.toCallOptions; | |
| if (_isEmulator) return CallOptions(); | |
| return authenticator?.toCallOptions ?? CallOptions(); |
| await _publisher.deleteTopic(request, options: await _callOptions); | ||
| return true; | ||
| } on GrpcError catch (e) { | ||
| if (e.code == StatusCode.notFound) { |
There was a problem hiding this comment.
I think either design is OK. Since we have precedent I lean towards matching what we already do elsewhere.
| import 'package:grpc/grpc.dart'; | ||
| import 'package:meta/meta.dart'; | ||
| import '../google_cloud_pubsub.dart'; | ||
| import 'generated/google/pubsub/v1/pubsub.pbgrpc.dart' as grpc; | ||
| import 'pubsub_emulator_host_vm.dart'; | ||
| import 'subscription.dart' show newSubscription, newSubscriptionName; | ||
| import 'topic.dart' show newTopic, newTopicName; |
There was a problem hiding this comment.
[nit]
| import 'package:grpc/grpc.dart'; | |
| import 'package:meta/meta.dart'; | |
| import '../google_cloud_pubsub.dart'; | |
| import 'generated/google/pubsub/v1/pubsub.pbgrpc.dart' as grpc; | |
| import 'pubsub_emulator_host_vm.dart'; | |
| import 'subscription.dart' show newSubscription, newSubscriptionName; | |
| import 'topic.dart' show newTopic, newTopicName; | |
| import 'package:grpc/grpc.dart'; | |
| import 'package:meta/meta.dart'; | |
| import '../google_cloud_pubsub.dart'; | |
| import 'generated/google/pubsub/v1/pubsub.pbgrpc.dart' as grpc; | |
| import 'pubsub_emulator_host_vm.dart'; | |
| import 'subscription.dart' show newSubscription, newSubscriptionName; | |
| import 'topic.dart' show newTopic, newTopicName; |
| ); | ||
| } | ||
|
|
||
| if (emulatorHost case String host) { |
There was a problem hiding this comment.
[nit] Avoid the appearance of inputs that may be other types by only using the syntax for the null check.
| if (emulatorHost case String host) { | |
| if (emulatorHost case final host?) { |
| /// and the emulator configuration might not be available until the application | ||
| /// is launched. | ||
| @internal | ||
| String? get pubsubEmulatorHost { |
There was a problem hiding this comment.
[nit] inconsistent capitalization - other code treats "pub" and "sub" as different words but they are joined here.
| if (invocation.memberName == #cancel) { | ||
| return Future<void>.value(); | ||
| } |
There was a problem hiding this comment.
How about implementing the cancel method instead?
| class FakeResponseStream<T> extends Stream<T> | ||
| implements grpc.ResponseStream<T> { | ||
| final Stream<T> _stream; |
There was a problem hiding this comment.
Why are we both extending Stream and including a stream member?
| // Listen to request stream to prevent sender from hanging on close() | ||
| request.listen((_) {}, onDone: () {}); |
There was a problem hiding this comment.
[nit]
| // Listen to request stream to prevent sender from hanging on close() | |
| request.listen((_) {}, onDone: () {}); | |
| // Listen to request stream to prevent sender from hanging on close() | |
| request.drain(); |
| if (acknowledgeBehavior != null) { | ||
| acknowledgeBehavior!(request.ackIds) |
There was a problem hiding this comment.
[optional]
| if (acknowledgeBehavior != null) { | |
| acknowledgeBehavior!(request.ackIds) | |
| if (acknowledgeBehavior case final acknowledge?) { | |
| acknowledge(request.ackIds) |
| class FakeClientChannel implements grpc.ClientChannel { | ||
| @override | ||
| Future<void> shutdown() async { | ||
| // No-op for testing | ||
| } | ||
|
|
||
| @override | ||
| dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); | ||
| } |
There was a problem hiding this comment.
You can extend Fake from package:test/test.dart to get this override
| class FakeClientChannel implements grpc.ClientChannel { | |
| @override | |
| Future<void> shutdown() async { | |
| // No-op for testing | |
| } | |
| @override | |
| dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); | |
| } | |
| class FakeClientChannel extends Fake implements grpc.ClientChannel { | |
| @override | |
| Future<void> shutdown() async { | |
| // No-op for testing | |
| } | |
| } |
Similar in the other fakes.
- Fix finalSize check in Windows environment variable reader to handle errors/race conditions. - Update documentation to follow style guidelines for programmer errors (use "It is an error if..." instead of "Throws..."). TAG=agy CONV=ad5bf873-d53c-4338-9b86-1b6a21376d4d
- Default to ADC when not using emulator - Default emulator project ID to test-project - Parse emulator host as Uri? with validation - Update deleteTopic and deleteSubscription to throw on notFound - Rename name parameter to topic/subscription in PubSub client methods - Centralize GrpcError mapping - Rename topicId and subscriptionId to id - Split integration tests into individual files per operation - Rename unit_test.dart to error_propagation_test.dart and refactor fakes - Expand and clarify doc comments across public APIs
Initial implementation. Only basic functionality implemented (see todos).
Streaming pull is implemented.
Rely on the package:grpc BaseAuthenticator abstraction for authentication. Rexports those.
No built-in retry yet.