Added ReflectionService initialiser using file paths #1699

stefanadranca · 2023-11-01T12:58:43Z

Motivation:

Initialising the Reflection Service using file descriptor protos is too laborious for users, so simply passing the file paths of the binary files containing the serialised file descriptor protos and letting the Service extract the data from those paths is more convenient.

Modifications:

Added a new initializer that takes in file paths and extracts and converts to file descriptor proto format the Data from the files. Also modified the integration tests to create temporary files containing the serialized file descriptor protos.

Result:

GRPC users can add the ReflectionService to their Server in a more convenient way.

Motivation: Initializing the Reflection Service using file descriptor protos is too laborious for users, so simply passing the file paths of the binary files containing the serialized file descriptor protos and letting the Service extract the data from those paths is more convenient. Modifications: Added a new initializer that takes in file paths and extracts and converts to file descriptor proto format the Data from the files. Also modified the integration tests to create temporary files containing the serialized file descriptor protos. Result: GRPCUsers can add the ReflectionService to their Server in a more convenient way.

glbrntt · 2023-11-01T15:52:21Z

Sources/GRPCReflectionService/Server/ReflectionService.swift

@@ -26,6 +26,10 @@ public final class ReflectionService: CallHandlerProvider, Sendable {
    self.reflectionService.serviceName
  }

+  public init(protoFilePaths: [String]) throws {


We aren't loading proto files (i.e. .proto) we're loading serialised file descriptor protos:

public init(serializedFileDescriptorProtoFilePaths: [String]) throws {

This also needs documentation

glbrntt · 2023-11-01T15:52:51Z

Sources/GRPCReflectionService/Server/ReflectionService.swift

@@ -207,6 +211,27 @@ internal struct ReflectionServiceData: Sendable {
 internal final class ReflectionServiceProvider: Grpc_Reflection_V1_ServerReflectionAsyncProvider {
  private let protoRegistry: ReflectionServiceData

+  internal init(protoFilePaths: [String]) throws {
+


nit: unnecessary blank line

Suggested change

glbrntt · 2023-11-01T15:55:07Z

Sources/GRPCReflectionService/Server/ReflectionService.swift

+    let binaryData = binaryFilesURLs.map { Data(base64Encoded: try! Data(contentsOf: $0)) }
+    let fileDescriptorProtos = binaryData.map {
+      try! Google_Protobuf_FileDescriptorProto(serializedData: $0!)
+    }


We can't try! these, they may legitimately fail and this would cause a crash.

We should just do the basic thing:

var fileDescriptorProtos = [Google_Protobuf_FileDescriptorProto]() for path in paths { // ... fileDescriptorProtos.append(...) }

glbrntt · 2023-11-01T15:58:46Z

Tests/GRPCTests/GRPCReflectionServiceTests/ReflectionServiceIntegrationTests.swift

+      #else
+      let temporaryDirectory = fileManager.temporaryDirectory.path()
+      #endif
+      let filePath = temporaryDirectory + fileDescriptorProto.name


I don't think this is sufficiently unique (especially on linux): if we ran these tests in parallel we could end up removing files which another test is expecting to be there. One workaround which is probably fine is to add some randomness to the name.

glbrntt · 2023-11-01T15:59:41Z

Tests/GRPCTests/GRPCReflectionServiceTests/ReflectionServiceIntegrationTests.swift

-    let reflectionServiceProvider = try ReflectionService(
-      fileDescriptors: self.protos + [self.independentProto]
-    )
+    var filePaths: [String] = []


We actually don't need to do this in the integration tests: we can do it in the unit tests because the actual work is done by the registry rather than the service. This also means we can do this in a separate test as well.

At the moment, the ReflectionServiceProvider (not the wrapper, the class that contains the registry) is doing the conversion of Data from the file paths to serialised file descriptor protos and then initialises the registry (using the file descriptor ports). This is why I implemented the testing in the integration tests. So if I'm moving it in the Unit tests I should also move the conversion in a Proto initialiser. Should I do this?

And my second question if I do this: Should I only implement unit tests for this conversion from binary data to file descriptor photos and leave all the other tests as they were before (using directly file descriptor protos)?

Oh, my mistake. Thinking about this some more, I think we can implement this as a couple of helpers on the ReflectionService which would be called by the init:

extension ReflectionService { static func readSerializedFileDescriptorProto(atPath path: String) throws -> Google_Protobuf_FileDescriptorProto { // ... } static func readSerializedFileDescriptorProtos(atPaths paths: [String]) throws -> [Google_Protobuf_FileDescriptorProto] { // ... } }

This way we can test the loading independently of the registry and operation of the service. I.e. we can test that these functions can load and deserialize the serialized descriptors correctly.

glbrntt · 2023-11-02T12:42:38Z

Sources/GRPCReflectionService/Server/ReflectionService.swift

+  ///  Receives as parameter an array of strings representing the file paths to the binary files containing
+  ///  the serialised file descriptor protos describing the services supported by the Server and
+  ///  discovarable through Server Reflection.


Docs should follow a particular format, take a look at https://www.swift.org/documentation/api-design-guidelines/ in particular the "More detail" part of "Write a documentation comment".

The documentation should start with a brief description of what the function does:

Creates a ReflectionService by loading serialized reflection data created by protoc-gen-grpc-swift.

Then you can go into more information about what the expectations are etc. and how they can generate this data:

You can generate serialized reflection data using the protoc-gen-grpc-swift plugin for protoc by
setting the to True. The paths provided should be absolute or relative
to the current working directory.

Then you can detail the parameters:

Parameter filePaths: The paths to files containing serialized reflection data.

And when the function throws:

Throws: When a file can't be read from disk or parsed.

glbrntt · 2023-11-02T12:43:02Z

Sources/GRPCReflectionService/Server/ReflectionService.swift

+  ///  Receives as parameter an array of strings representing the file paths to the binary files containing
+  ///  the serialised file descriptor protos describing the services supported by the Server and
+  ///  discovarable through Server Reflection.
+  public init(serializedFileDescriptorProtoFilePaths: [String]) throws {


We should use a shorter label internally:

Suggested change

public init(serializedFileDescriptorProtoFilePaths: [String]) throws {

public init(serializedFileDescriptorProtoFilePaths filePaths: [String]) throws {

glbrntt · 2023-11-02T12:49:50Z

Sources/GRPCReflectionService/Server/ReflectionService.swift

+        code: .invalidArgument,
+        message:
+          """
+          The \(fileURL.lastPathComponent) file contents could not be transformed \


We should include the whole file path. The user could have two files with the same name in different directories which would make this error unclear.

Sources/GRPCReflectionService/Server/ReflectionService.swift

glbrntt

Doc needs fixing but looks fine otherwise

glbrntt · 2023-11-03T09:46:16Z

Sources/GRPCReflectionService/Server/ReflectionService.swift

+  /// Creates a `ReflectionService` by loading serialized reflection data created by `protoc-gen-grpc-swift`.
+  ///
+  /// You can generate serialized reflection data using the `protoc-gen-grpc-swift` plugin for `protoc` by
+  /// setting the to `True`. The paths provided should be absolute or relative to the current working directory.


"setting the to True" ...?

stefanadranca requested a review from glbrntt November 1, 2023 12:58

stefanadranca and others added 6 commits November 1, 2023 13:35

added OS versions availability check

1fea48d

availability check fix?

b2a0e95

checking os

f808eea

checking os for integration tests

2c003ce

Merge branch 'main' into sd-reflection-service

76bb1f1

changed tmp directory name for linux

8e0fa46

glbrntt requested changes Nov 1, 2023

View reviewed changes

stefanadranca and others added 3 commits November 1, 2023 17:31

still need to make some changes

2431bd1

implemented feedback

c738569

Merge branch 'main' into sd-reflection-service

38b7a6f

stefanadranca requested a review from glbrntt November 2, 2023 11:06

Merge branch 'main' into sd-reflection-service

f30f52a

glbrntt requested changes Nov 2, 2023

View reviewed changes

stefanadranca and others added 2 commits November 2, 2023 14:05

implemented feedback and added test for error

cb7ab5d

Merge branch 'main' into sd-reflection-service

65d66fb

stefanadranca requested a review from glbrntt November 2, 2023 14:07

glbrntt requested changes Nov 3, 2023

View reviewed changes

added missing option

c05eba6

stefanadranca requested a review from glbrntt November 3, 2023 09:57

glbrntt approved these changes Nov 3, 2023

View reviewed changes

Merge branch 'main' into sd-reflection-service

2beacc0

glbrntt merged commit 5519602 into grpc:main Nov 3, 2023
12 of 14 checks passed

glbrntt added the semver/minor Adds new public API. label Dec 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ReflectionService initialiser using file paths #1699

Added ReflectionService initialiser using file paths #1699

stefanadranca commented Nov 1, 2023 •

edited

Loading

glbrntt Nov 1, 2023

glbrntt Nov 1, 2023

glbrntt Nov 1, 2023

glbrntt Nov 1, 2023

glbrntt Nov 1, 2023

glbrntt Nov 1, 2023

stefanadranca Nov 1, 2023

glbrntt Nov 1, 2023

glbrntt Nov 2, 2023

glbrntt Nov 2, 2023

glbrntt Nov 2, 2023

glbrntt left a comment

glbrntt Nov 3, 2023

	public init(serializedFileDescriptorProtoFilePaths: [String]) throws {
	public init(serializedFileDescriptorProtoFilePaths filePaths: [String]) throws {

Added ReflectionService initialiser using file paths #1699

Added ReflectionService initialiser using file paths #1699

Conversation

stefanadranca commented Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glbrntt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanadranca commented Nov 1, 2023 •

edited

Loading