Skip to content

Commit 786f359

Browse files
bjorkstrommbjorkstromm
authored
Fix ordering of FileDescriptors in reflection response (#143)
* Fix ordering of FileDescriptors in reflection response - Fixes #141 * Fix FileDescriptor comparer * Use SchemaGenerator when constructing the FileDescriptorSet * Use separate namespaces for dependent message types in test. * Update tests. - Check that all message types are included. - Check that FileDescriptorSet can be processed without errors.
1 parent 3b42f6f commit 786f359

File tree

6 files changed

+239
-86
lines changed

6 files changed

+239
-86
lines changed

‎examples/pb-net-grpc/Server_CS/MyTimeService.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ private static async IAsyncEnumerable<TimeResult> SubscribeAsyncImpl([Enumerator
2525
{
2626
break;
2727
}
28-
yield return new TimeResult { Time = DateTime.UtcNow };
28+
yield return new TimeResult { Time = DateTime.UtcNow, Id = Guid.NewGuid() };
2929
}
3030
}
3131
}

‎examples/pb-net-grpc/Shared_CS/TimeService.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,5 +18,8 @@ public class TimeResult
1818
{
1919
[ProtoMember(1, DataFormat = DataFormat.WellKnown)]
2020
public DateTime Time { get; set; }
21+
22+
[ProtoMember(2)]
23+
public Guid Id { get; set; }
2124
}
2225
}

‎src/protobuf-net.Grpc.Reflection/FileDescriptorSetFactory.cs

Lines changed: 9 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -34,89 +34,21 @@ private static void Populate(FileDescriptorSet fileDescriptorSet, Type serviceTy
3434
var serviceContracts = typeof(IGrpcService).IsAssignableFrom(serviceType)
3535
? new HashSet<Type> { serviceType }
3636
: ContractOperation.ExpandInterfaces(serviceType);
37-
38-
foreach (var serviceContract in serviceContracts)
37+
var schemaGenerator = new SchemaGenerator
3938
{
40-
if (!binderConfiguration.Binder.IsServiceContract(serviceContract, out string? serviceName)) continue;
41-
42-
var serviceDescriptor = new ServiceDescriptorProto
43-
{
44-
Name = ServiceBinder.GetNameParts(serviceName, serviceType, out var package),
45-
};
46-
47-
var dependencies = new HashSet<string>();
48-
foreach (var op in ContractOperation.FindOperations(binderConfiguration, serviceContract, null))
49-
{
50-
// TODO: Validate op
51-
if (TryGetType(binderConfiguration, op.From, fileDescriptorSet, out var inputType, out var inputFile)
52-
&& TryGetType(binderConfiguration, op.To, fileDescriptorSet, out var outputType, out var outputFile))
53-
{
54-
serviceDescriptor.Methods.Add(new MethodDescriptorProto
55-
{
56-
Name = op.Name,
57-
InputType = inputType,
58-
OutputType = outputType,
59-
ClientStreaming = op.MethodType == MethodType.ClientStreaming ||
60-
op.MethodType == MethodType.DuplexStreaming,
61-
ServerStreaming = op.MethodType == MethodType.ServerStreaming ||
62-
op.MethodType == MethodType.DuplexStreaming
63-
});
64-
65-
dependencies.Add(inputFile);
66-
dependencies.Add(outputFile);
67-
}
68-
}
69-
70-
var fileDescriptor = new FileDescriptorProto
71-
{
72-
Name = serviceName + ".proto",
73-
Services = { serviceDescriptor },
74-
Syntax = "proto3",
75-
Package = package
76-
};
77-
78-
foreach (var dependency in dependencies)
79-
{
80-
fileDescriptor.Dependencies.Add(dependency);
81-
fileDescriptor.AddImport(dependency, true, default);
82-
}
83-
84-
fileDescriptorSet.Files.Add(fileDescriptor);
85-
}
86-
}
87-
88-
private static bool TryGetType(BinderConfiguration binderConfiguration, Type type, FileDescriptorSet fileDescriptorSet, out string fullyQualifiedName, out string descriptorProto)
89-
{
90-
var typeName = type.Name;
91-
var fileName = type.FullName + ".proto";
92-
var fileDescriptor = fileDescriptorSet.Files.SingleOrDefault(f => f.Name.Equals(fileName, StringComparison.Ordinal));
39+
BinderConfiguration = binderConfiguration,
40+
ProtoSyntax = ProtoSyntax.Proto3
41+
};
9342

94-
TypeModel model = binderConfiguration.MarshallerCache.TryGetFactory(type) is ProtoBufMarshallerFactory factory ? factory.Model : RuntimeTypeModel.Default;
95-
96-
if (fileDescriptor is null)
43+
foreach (var serviceContract in serviceContracts)
9744
{
98-
var schema = model.GetSchema(type, ProtoSyntax.Proto3);
99-
45+
if (!binderConfiguration.Binder.IsServiceContract(serviceContract, out var serviceName)) continue;
46+
47+
var schema = schemaGenerator.GetSchema(serviceContract);
10048
using var reader = new StringReader(schema);
10149

102-
fileDescriptorSet.Add(fileName, includeInOutput: true, reader);
103-
fileDescriptor = fileDescriptorSet.Files.SingleOrDefault(f => f.Name.Equals(fileName, StringComparison.Ordinal));
104-
if (fileDescriptor is null)
105-
{
106-
fullyQualifiedName = descriptorProto = "";
107-
return false;
108-
}
109-
}
110-
111-
var msgType = fileDescriptor.MessageTypes.SingleOrDefault(m => m.Name.Equals(typeName, StringComparison.OrdinalIgnoreCase));
112-
if (msgType is null)
113-
{
114-
fullyQualifiedName = descriptorProto = "";
115-
return false;
50+
fileDescriptorSet.Add(serviceName + ".proto", includeInOutput: true, reader);
11651
}
117-
descriptorProto = fileDescriptor.Name;
118-
fullyQualifiedName = "." + fileDescriptor.Package + "." + msgType.Name;
119-
return true;
12052
}
12153
}
12254
}

‎src/protobuf-net.Grpc.Reflection/ReflectionService.cs

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -155,24 +155,38 @@ public int Compare(FileDescriptorProto? left, FileDescriptorProto? right)
155155
{
156156
return right is null ? 0 : -1;
157157
}
158-
else if (right is null)
158+
if (right is null)
159159
{
160160
return 1;
161161
}
162-
if (left.Dependencies.Contains(right.Name))
162+
if (GetTransitiveDependencies(left).Contains(right.Name))
163163
{
164164
return 1;
165165
}
166-
if (right.Dependencies.Contains(left.Name))
166+
if (GetTransitiveDependencies(right).Contains(left.Name))
167167
{
168168
return -1;
169169
}
170-
if (left.Name == right.Name)
170+
171+
return string.Compare(left.Name, right.Name, StringComparison.Ordinal);
172+
173+
static IReadOnlyCollection<string> GetTransitiveDependencies(FileDescriptorProto? descriptor)
171174
{
172-
return 0;
173-
}
175+
if (descriptor is null)
176+
{
177+
return Array.Empty<string>();
178+
}
174179

175-
return 1;
180+
var dependencies = new List<string>();
181+
182+
foreach (var dependency in descriptor.GetDependencies())
183+
{
184+
dependencies.Add(dependency.Name);
185+
dependencies.AddRange(GetTransitiveDependencies(dependency));
186+
}
187+
188+
return dependencies;
189+
}
176190
}
177191
}
178192
}

‎tests/protobuf-net.Grpc.Reflection.Test/FileDescriptorSetFactoryTests.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ public void SimpleService()
2020
Assert.Empty(fileDescriptorSet.GetErrors());
2121
Assert.Equal(new[] { "GreeterService" },
2222
fileDescriptorSet.Files.SelectMany(x => x.Services).Select(x => x.Name).ToArray());
23-
Assert.Equal(new[] { "HelloRequest", "HelloReply" },
23+
Assert.Equal(new[] { "HelloReply", "HelloRequest" },
2424
fileDescriptorSet.Files.SelectMany(x => x.MessageTypes).Select(x => x.Name).ToArray());
2525
}
2626
}

‎tests/protobuf-net.Grpc.Reflection.Test/ReflectionServiceTests.cs

Lines changed: 204 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,204 @@
1+
using System;
2+
using System.Collections.Generic;
3+
using System.Linq;
4+
using System.Reflection;
5+
using System.Threading.Tasks;
6+
using Google.Protobuf.Reflection;
7+
using grpc.reflection.v1alpha;
8+
using ProtoBuf;
9+
using ProtoBuf.Grpc.Configuration;
10+
using ProtoBuf.Grpc.Reflection;
11+
using ProtoBuf.Grpc.Reflection.Internal;
12+
using Xunit;
13+
14+
namespace protobuf_net.Grpc.Reflection.Test
15+
{
16+
using ReflectionTest;
17+
18+
public class ReflectionServiceTests
19+
{
20+
private static Lazy<MethodInfo> AddImportMethod = new Lazy<MethodInfo>(() => typeof(FileDescriptorProto).GetMethod("AddImport", BindingFlags.NonPublic | BindingFlags.Instance));
21+
22+
[Theory]
23+
[MemberData(nameof(Dependencies))]
24+
public async Task ShouldIncludeDependenciesInCorrectOrder(Type service, string symbolName, string[] expectedDescriptors, string[] expectedMessageTypes)
25+
{
26+
IServerReflection reflectionService = new ReflectionService(service);
27+
28+
await foreach (var response in reflectionService.ServerReflectionInfoAsync(GetRequest()))
29+
{
30+
var fileDescriptors = response
31+
.FileDescriptorResponse
32+
.FileDescriptorProtoes
33+
.Select(x => Serializer.Deserialize<FileDescriptorProto>(x.AsSpan()))
34+
.ToArray();
35+
36+
var fileDescriptorSet = new FileDescriptorSet();
37+
38+
foreach (var fileDescriptor in fileDescriptors)
39+
{
40+
// We need to add dependency as import, otherwise FileDescriptorSet.GetErrors() will return error about not finding imports.
41+
foreach (var dependency in fileDescriptor.Dependencies)
42+
{
43+
// Use reflection.
44+
var addImportMethod = AddImportMethod.Value;
45+
addImportMethod.Invoke(fileDescriptor, new object[] {dependency, true, default});
46+
}
47+
48+
fileDescriptorSet.Files.Add(fileDescriptor);
49+
}
50+
51+
fileDescriptorSet.Process();
52+
53+
Assert.Empty(fileDescriptorSet.GetErrors());
54+
Assert.Equal(expectedDescriptors, fileDescriptorSet.Files.Select(x => x.Name));
55+
Assert.Equal(expectedMessageTypes, fileDescriptorSet.Files.SelectMany(x => x.MessageTypes).Select(x => $".{x.GetFile().Package}.{x.Name}").OrderBy(x => x));
56+
}
57+
58+
async IAsyncEnumerable<ServerReflectionRequest> GetRequest()
59+
{
60+
yield return new ServerReflectionRequest
61+
{
62+
FileContainingSymbol = symbolName
63+
};
64+
65+
await Task.CompletedTask;
66+
}
67+
}
68+
69+
public static IEnumerable<object[]> Dependencies => new[]
70+
{
71+
new object[]
72+
{
73+
typeof(BclService),
74+
".ReflectionTest.BclService",
75+
new[]
76+
{
77+
"google/protobuf/empty.proto",
78+
"protobuf-net/bcl.proto",
79+
"ReflectionTest.BclService.proto"
80+
}
81+
,
82+
new[]
83+
{
84+
".bcl.DateTime",
85+
".bcl.Decimal",
86+
".bcl.Guid",
87+
".bcl.NetObjectProxy",
88+
".bcl.TimeSpan",
89+
".google.protobuf.Empty",
90+
".ReflectionTest.BclMessage",
91+
}
92+
},
93+
new object[]
94+
{
95+
typeof(ReflectionTest.Service.Nested),
96+
".ReflectionTest.Service.Nested",
97+
new[]
98+
{
99+
"ReflectionTest.Service.Nested.proto",
100+
},
101+
new[]
102+
{
103+
".ReflectionTest.Service.Four",
104+
".ReflectionTest.Service.One",
105+
".ReflectionTest.Service.Three",
106+
".ReflectionTest.Service.Two",
107+
}
108+
},
109+
};
110+
}
111+
}
112+
113+
namespace ReflectionTest
114+
{
115+
[ProtoContract]
116+
public class BclMessage
117+
{
118+
[ProtoMember(1)]
119+
[CompatibilityLevel(CompatibilityLevel.Level200)]
120+
public Guid Id { get; set; } = Guid.Empty;
121+
122+
[ProtoMember(2)]
123+
[CompatibilityLevel(CompatibilityLevel.Level200)]
124+
public DateTime DateTime { get; set; } = DateTime.MinValue;
125+
126+
[ProtoMember(3)]
127+
[CompatibilityLevel(CompatibilityLevel.Level200)]
128+
public TimeSpan TimeSpan { get; set; } = TimeSpan.MinValue;
129+
130+
[ProtoMember(4)]
131+
[CompatibilityLevel(CompatibilityLevel.Level200)]
132+
public decimal Decimal { get; set; } = 0M;
133+
}
134+
135+
[Service]
136+
public interface IBclService
137+
{
138+
ValueTask M(BclMessage request);
139+
}
140+
141+
public class BclService : IBclService
142+
{
143+
public ValueTask M(BclMessage request) => throw new NotImplementedException();
144+
}
145+
}
146+
147+
namespace ReflectionTest.One
148+
{
149+
[ProtoContract]
150+
public class One
151+
{
152+
[ProtoMember(1)] public string P { get; set; } = string.Empty;
153+
}
154+
}
155+
156+
namespace ReflectionTest.Two
157+
{
158+
using One;
159+
160+
[ProtoContract]
161+
public class Two
162+
{
163+
[ProtoMember(1)] public One P { get; set; } = new One();
164+
}
165+
}
166+
167+
namespace ReflectionTest.Three
168+
{
169+
using Two;
170+
171+
[ProtoContract]
172+
public class Three
173+
{
174+
[ProtoMember(1)] public Two P { get; set; } = new Two();
175+
}
176+
}
177+
178+
namespace ReflectionTest.Four
179+
{
180+
using Three;
181+
182+
[ProtoContract]
183+
public class Four
184+
{
185+
[ProtoMember(1)] public Three P { get; set; } = new Three();
186+
}
187+
}
188+
189+
namespace ReflectionTest.Service
190+
{
191+
using Three;
192+
using Four;
193+
194+
[Service]
195+
public interface INested
196+
{
197+
ValueTask<Three> M(Four request);
198+
}
199+
200+
public class Nested : INested
201+
{
202+
public ValueTask<Three> M(Four request) => throw new NotImplementedException();
203+
}
204+
}

0 commit comments

Comments
 (0)