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.
| ReceivedMessage({ | ||
| required this.ackId, | ||
| required this.messageId, | ||
| required this.publishTime, | ||
| required this.message, | ||
| FutureOr<void> Function(List<String> ackIds)? ackHandler, | ||
| FutureOr<void> Function(List<String> ackIds, int seconds)? | ||
| modifyDeadlineHandler, | ||
| }) : _ackHandler = ackHandler, |
There was a problem hiding this comment.
Is this a constructor you want to make part of the public API?
Consider making it private and expose a top-level createReceivedMessage and annotated it @internal.
This way, you can avoid exporting createReceivedMessage from lib/google_cloud_pubsub.dart.
There was a problem hiding this comment.
I moved the callbacks out of ReceivedMessage. Now it is just a dumb data class, that feels a lot more clean.
You now need to do Subscription.ack(receivedMessage) I think that is acceptable. WDYT?
| static String? _calculateProjectId(String? projectId, String? emulatorHost) => | ||
| switch ((projectId, emulatorHost)) { | ||
| (final String projectId, _) => projectId, | ||
| (null, _?) => projectFromEnvironment ?? 'test-project', |
There was a problem hiding this comment.
Are we always falling back to test-project? Ah, only when PUBSUB_EMULATOR_HOST is set, right?
There was a problem hiding this comment.
Yeah - that's confusing. Added a comment.
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?
| final isEmulator = Platform.environment['PUBSUB_EMULATOR_HOST'] != null; | ||
|
|
||
| group('PubSub Integration', () { | ||
| PubSub? client; |
There was a problem hiding this comment.
| PubSub? client; | |
| late final PubSub client; |
| 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?
| /// 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?
| 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?
| // 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).
| /// | ||
| /// See the [official documentation](https://cloud.google.com/pubsub/docs/reference/rpc/google.pubsub.v1#google.pubsub.v1.Subscriber.ModifyAckDeadline). | ||
| Future<void> modifyAckDeadline( | ||
| String name, |
There was a problem hiding this comment.
maybe call this subscription because, at first, I thought that this was a message name.
| @@ -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.
| } | ||
|
|
||
| void main() { | ||
| group('PubSub Unit Tests (Error Propagation)', () { |
There was a problem hiding this comment.
Maybe call this file error_propogation_test.dart.
| 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.