Skip to content

Disambiguate names of service methods in grpc output #487

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

Closed
wants to merge 4 commits into from
Closed

Disambiguate names of service methods in grpc output #487

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4b72ad5
Disambiguate names of service methods in grpc output
sigurdm Mar 2, 2021
45bdb8f
Use const
sigurdm Mar 4, 2021
76dab5f
Update changelog
sigurdm Mar 4, 2021
dd6a595
Also disambiguate service_generator and client_generator
sigurdm Mar 4, 2021
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
11 changes: 8 additions & 3 deletions protoc_plugin/lib/grpc_generator.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,9 @@ class GrpcServiceGenerator {
/// in [_undefinedDeps].
/// Precondition: messages have been registered and resolved.
void resolve(GenerationContext ctx) {
final usedNames = <String>{...serviceReservedMemberNames};
for (var method in _descriptor.method) {
_methods.add(_GrpcMethod(this, ctx, method));
_methods.add(_GrpcMethod(this, ctx, method, usedNames));
}
}

Expand Down Expand Up @@ -181,9 +182,13 @@ class _GrpcMethod {
this._serverReturnType);

factory _GrpcMethod(GrpcServiceGenerator service, GenerationContext ctx,
MethodDescriptorProto method) {
MethodDescriptorProto method, Set<String> usedNames) {
final grpcName = method.name;
final dartName = lowerCaseFirstLetter(grpcName);
final dartName = disambiguateName(
avoidInitialUnderscore(lowerCaseFirstLetter(grpcName)),
usedNames,
defaultSuffixes(),
generateVariants: (name) => [name, '${name}_Pre']);

final clientStreaming = method.clientStreaming;
final serverStreaming = method.serverStreaming;
Expand Down
17 changes: 17 additions & 0 deletions protoc_plugin/lib/names.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,23 @@ final List<String> forbiddenExtensionNames = <String>[
..._generatedMessageNames
];

final List<String> serviceReservedMemberNames = <String>[
..._dartReservedWords,
..._serviceNames,
];

final List<String> _serviceNames = <String>[
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be const? Also, do we need the explicit type on the left-hand side?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes!

Done - also in surrounding code.

// From `client`.
r'$createCall',
r'$createUnaryCall',
r'$createStreamingCall',
r'$addMethod',
// From `Service`.
r'$name',
r'$onMetadata',
r'$lookupMethod',
];

// List of Dart language reserved words in names which cannot be used in a
// subclass of GeneratedMessage.
const List<String> _dartReservedWords = [
Expand Down
20 changes: 19 additions & 1 deletion protoc_plugin/test/file_generator_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,9 +300,27 @@ void main() {
..clientStreaming = true
..serverStreaming = true;

// Some methods with names that overlap reserved words or existing methods
// in the base classes Client/Service of package:grpc.
final names = [
MethodDescriptorProto()
..name = 'New'
..inputType = '.Input'
..outputType = '.Output',
MethodDescriptorProto()
..name = r'$addMethod'
..inputType = '.Input'
..outputType = '.Output',
MethodDescriptorProto()
..name = r'_32SillyName'
..inputType = '.Input'
..outputType = '.Output',
];

var sd = ServiceDescriptorProto()
..name = 'Test'
..method.addAll([unary, clientStreaming, serverStreaming, bidirectional]);
..method.addAll(
[unary, clientStreaming, serverStreaming, bidirectional, ...names]);

var fd = FileDescriptorProto()
..name = 'test'
Expand Down
68 changes: 68 additions & 0 deletions protoc_plugin/test/goldens/grpc_service.pbgrpc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,18 @@ class TestClient extends $grpc.Client {
'/Test/Bidirectional',
($0.Input value) => value.writeToBuffer(),
($core.List<$core.int> value) => $0.Output.fromBuffer(value));
static final _$new_ = $grpc.ClientMethod<$0.Input, $0.Output>(
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I was curious of, is it possible to have a name collision with the library import aliases? i.e. do we also need to add r'$grpc', r'$async' etc to the reserved words?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

$ cannot occur in a protobuf identifier, (https://developers.google.com/protocol-buffers/docs/reference/proto3-spec#identifiers) - I also tried it out and the protoc compiler gave a syntax error.

I believe that is why we chose to use $ in the first place for these names.

That means we could actually remove all the reserved words with $!

I won't do that in this PR to keep the scope smaller.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(I removed it from those added).

'/Test/New',
($0.Input value) => value.writeToBuffer(),
($core.List<$core.int> value) => $0.Output.fromBuffer(value));
static final _$$addMethod_ = $grpc.ClientMethod<$0.Input, $0.Output>(
'/Test/$addMethod',
($0.Input value) => value.writeToBuffer(),
($core.List<$core.int> value) => $0.Output.fromBuffer(value));
static final _$x32SillyName_ = $grpc.ClientMethod<$0.Input, $0.Output>(
'/Test/_32SillyName',
($0.Input value) => value.writeToBuffer(),
($core.List<$core.int> value) => $0.Output.fromBuffer(value));

TestClient($grpc.ClientChannel channel,
{$grpc.CallOptions? options,
Expand Down Expand Up @@ -59,6 +71,21 @@ class TestClient extends $grpc.Client {
{$grpc.CallOptions? options}) {
return $createStreamingCall(_$bidirectional, request, options: options);
}

$grpc.ResponseFuture<$0.Output> new_($0.Input request,
{$grpc.CallOptions? options}) {
return $createUnaryCall(_$new_, request, options: options);
}

$grpc.ResponseFuture<$0.Output> $addMethod_($0.Input request,
{$grpc.CallOptions? options}) {
return $createUnaryCall(_$$addMethod_, request, options: options);
}

$grpc.ResponseFuture<$0.Output> x32SillyName_($0.Input request,
{$grpc.CallOptions? options}) {
return $createUnaryCall(_$x32SillyName_, request, options: options);
}
}

abstract class TestServiceBase extends $grpc.Service {
Expand Down Expand Up @@ -93,6 +120,27 @@ abstract class TestServiceBase extends $grpc.Service {
true,
($core.List<$core.int> value) => $0.Input.fromBuffer(value),
($0.Output value) => value.writeToBuffer()));
$addMethod($grpc.ServiceMethod<$0.Input, $0.Output>(
'New',
new__Pre,
false,
false,
($core.List<$core.int> value) => $0.Input.fromBuffer(value),
($0.Output value) => value.writeToBuffer()));
$addMethod($grpc.ServiceMethod<$0.Input, $0.Output>(
'$addMethod',
$addMethod__Pre,
false,
false,
($core.List<$core.int> value) => $0.Input.fromBuffer(value),
($0.Output value) => value.writeToBuffer()));
$addMethod($grpc.ServiceMethod<$0.Input, $0.Output>(
'_32SillyName',
x32SillyName__Pre,
false,
false,
($core.List<$core.int> value) => $0.Input.fromBuffer(value),
($0.Output value) => value.writeToBuffer()));
}

$async.Future<$0.Output> unary_Pre(
Expand All @@ -105,11 +153,31 @@ abstract class TestServiceBase extends $grpc.Service {
yield* serverStreaming(call, await request);
}

$async.Future<$0.Output> new__Pre(
$grpc.ServiceCall call, $async.Future<$0.Input> request) async {
return new_(call, await request);
}

$async.Future<$0.Output> $addMethod__Pre(
$grpc.ServiceCall call, $async.Future<$0.Input> request) async {
return $addMethod_(call, await request);
}

$async.Future<$0.Output> x32SillyName__Pre(
$grpc.ServiceCall call, $async.Future<$0.Input> request) async {
return x32SillyName_(call, await request);
}

$async.Future<$0.Output> unary($grpc.ServiceCall call, $0.Input request);
$async.Future<$0.Output> clientStreaming(
$grpc.ServiceCall call, $async.Stream<$0.Input> request);
$async.Stream<$0.Output> serverStreaming(
$grpc.ServiceCall call, $0.Input request);
$async.Stream<$0.Output> bidirectional(
$grpc.ServiceCall call, $async.Stream<$0.Input> request);
$async.Future<$0.Output> new_($grpc.ServiceCall call, $0.Input request);
$async.Future<$0.Output> $addMethod_(
$grpc.ServiceCall call, $0.Input request);
$async.Future<$0.Output> x32SillyName_(
$grpc.ServiceCall call, $0.Input request);
}