Adding support for v1alpha reflection #1703

stefanadranca · 2023-11-01T16:47:45Z

Motivation:

v1alpha reflection is also used by GRPC users, so we should provide support for it.

Modifications:

Generated the .grpc and .pb files for the v1alpha reflection.
Added an extension for the ReflectionService that represents the version, that is set by the users.
Created an enum to represent the 2 possible Reflection Service Providers and changed the methods that were calling provider's methods to switch between the possible cases.

Result:

Users will be able to set the version for the Reflection Service that they use, v1alpha becoming a possibility.

Motivation: v1alpha reflection is also used by GRPC users, so we should provide support for it. Modifications: - Generated the .grpc and .pb files for the v1alpha reflection. - Added an extension for the ReflectionService that represents the version, that is set by the users. - Created an enum to represent the 2 possible Reflection Service Providers and changed the methods that were calling provider's methods to switch between the possible cases. Result: Users will be able to set the version for the Reflection Service that they use, v1alpha becoming a possibility.

…to avoid build errors when generating the .grpc and .pb files

sd-v1alpha-temp solved the conflict between main and v1alpha.

glbrntt · 2023-11-03T15:30:10Z

Sources/GRPCReflectionService/Server/ReflectionService.swift

@@ -21,9 +21,15 @@ import SwiftProtobuf

 @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
 public final class ReflectionService: CallHandlerProvider, Sendable {
-  private let reflectionService: ReflectionServiceProvider
+  private let reflectionServiceProvider: ProviderType


This naming doesn't need to be so verbose: we're within the ReflectionService so we can just use "provider".

glbrntt · 2023-11-03T15:31:04Z

Sources/GRPCReflectionService/Server/ReflectionService.swift

  public var serviceName: Substring {
-    self.reflectionService.serviceName
+    switch self.reflectionServiceProvider {
+    case .v1Provider(let reflectionV1Provider):


Same with this naming: we're switching over providers so each case doesn't need to have "provider" in the name, "v1" and "v1Alpha" are fine. Same goes for the associated value labels, provider is fine there too.

switch self.provider { case .v1(let provider): // ... case .v1Alpha(let provider): // ... }

Sources/GRPCReflectionService/Server/ReflectionService.swift

glbrntt · 2023-11-03T15:32:46Z

Sources/GRPCReflectionService/Server/ReflectionService.swift

+    public static var v1: Self { Self(.v1) }
+    public static var v1Alpha: Self { Self(.v1Alpha) }


These need docs

These ones still need docs. I'd also link to the source protos:

https://github.com/grpc/grpc/blob/master/src/proto/grpc/reflection/v1alpha/reflection.proto

https://github.com/grpc/grpc/blob/master/src/proto/grpc/reflection/v1/reflection.proto

glbrntt · 2023-11-03T15:33:00Z

Sources/GRPCReflectionService/Server/ReflectionService.swift

-      return .success(.errorResponse(error))
+@available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
+extension ReflectionService {
+  public struct Version: Sendable {


This should be Hashable as well

glbrntt · 2023-11-03T15:33:25Z

Sources/GRPCReflectionService/Server/ReflectionService.swift

-    self.map { message in
-      Grpc_Reflection_V1_ServerReflectionResponse(request: request, messageResponse: message)
-    }
+  private enum ProviderType {


Provider is fine here

glbrntt · 2023-11-03T15:43:25Z

Tests/GRPCTests/GRPCReflectionServiceTests/ReflectionServiceV1AlphaIntegrationTests.swift

+  func testFileByFileName() async throws {
+    try self.setUpServerAndChannel()
+    let client = Grpc_Reflection_V1alpha_ServerReflectionAsyncClient(channel: self.channel!)
+    let serviceReflectionInfo = client.makeServerReflectionInfoCall()
+    try await serviceReflectionInfo.requestStream.send(
+      .with {
+        $0.host = "127.0.0.1"
+        $0.fileByFilename = "bar1.proto"
+      }
+    )
+    serviceReflectionInfo.requestStream.finish()
+
+    var iterator = serviceReflectionInfo.responseStream.makeAsyncIterator()
+    guard let message = try await iterator.next() else {
+      return XCTFail("Could not get a response message.")
+    }
+
+    let receivedFileDescriptorProto =
+      try Google_Protobuf_FileDescriptorProto(
+        serializedData: (message.fileDescriptorResponse
+          .fileDescriptorProto[0])
+      )
+
+    XCTAssertEqual(receivedFileDescriptorProto.name, "bar1.proto")
+    XCTAssertEqual(receivedFileDescriptorProto.service.count, 1)
+
+    guard let service = receivedFileDescriptorProto.service.first else {
+      return XCTFail("The received file descriptor proto doesn't have any services.")
+    }
+    guard let method = service.method.first else {
+      return XCTFail("The service of the received file descriptor proto doesn't have any methods.")
+    }
+    XCTAssertEqual(method.name, "testMethod1")
+    XCTAssertEqual(message.fileDescriptorResponse.fileDescriptorProto.count, 4)
+  }


I don't love that we've essentially copied-and-pasted all the tests. It's going to be difficult to maintain going forward.

I think we can be a bit smarter here by having each case iterate over the two versions and having some convenience functions to switch between v1 and v1 alpha. This obviously won't compile but hopefully gives an idea of what we can do:

Suggested change

func testFileByFileName() async throws {

try self.setUpServerAndChannel()

let client = Grpc_Reflection_V1alpha_ServerReflectionAsyncClient(channel: self.channel!)

let serviceReflectionInfo = client.makeServerReflectionInfoCall()

try await serviceReflectionInfo.requestStream.send(

.with {

$0.host = "127.0.0.1"

$0.fileByFilename = "bar1.proto"

}

)

serviceReflectionInfo.requestStream.finish()

var iterator = serviceReflectionInfo.responseStream.makeAsyncIterator()

guard let message = try await iterator.next() else {

return XCTFail("Could not get a response message.")

}

let receivedFileDescriptorProto =

try Google_Protobuf_FileDescriptorProto(

serializedData: (message.fileDescriptorResponse

.fileDescriptorProto[0])

)

XCTAssertEqual(receivedFileDescriptorProto.name, "bar1.proto")

XCTAssertEqual(receivedFileDescriptorProto.service.count, 1)

guard let service = receivedFileDescriptorProto.service.first else {

return XCTFail("The received file descriptor proto doesn't have any services.")

}

guard let method = service.method.first else {

return XCTFail("The service of the received file descriptor proto doesn't have any methods.")

}

XCTAssertEqual(method.name, "testMethod1")

XCTAssertEqual(message.fileDescriptorResponse.fileDescriptorProto.count, 4)

}

func testFileByFileName() async throws {

for version in versions {

try self.setUpServerAndChannel(version: version)

let request = V1Request.with {

$0.host = "127.0.0.1"

$0.fileByFilename = "bar1.proto"

}

let response: V1Response?

switch version {

case .v1:

let client = V1Client(channel: self.channel!)

let call = client.makeServerReflectionInfoCall()

try await call.send(request)

call.finish()

var iterator = serviceReflectionInfo.responseStream.makeAsyncIterator()

response = try await iterator.next()

case .v2:

let client = V1Client(channel: self.channel!)

let call = client.makeServerReflectionInfoCall()

try await call.send(V1AlphaRequest(request)) // convert from v1 to alpha

call.finish()

response = try await iterator.next().map { V1AlphaResponse($0) } // convert from v1 to alpha

}

guard let message = response else {

return XCTFail("Could not get a response message.")

}

let receivedFileDescriptorProto =

try Google_Protobuf_FileDescriptorProto(

serializedData: (message.fileDescriptorResponse

.fileDescriptorProto[0])

)

XCTAssertEqual(receivedFileDescriptorProto.name, "bar1.proto")

XCTAssertEqual(receivedFileDescriptorProto.service.count, 1)

guard let service = receivedFileDescriptorProto.service.first else {

return XCTFail("The received file descriptor proto doesn't have any services.")

}

guard let method = service.method.first else {

return XCTFail("The service of the received file descriptor proto doesn't have any methods.")

}

XCTAssertEqual(method.name, "testMethod1")

XCTAssertEqual(message.fileDescriptorResponse.fileDescriptorProto.count, 4)

}

…hanged the testFileByFileName test

…ions

glbrntt

Looks good! You need to avoid leaking some resources in the tests now that we do two iterations.

glbrntt · 2023-11-06T17:25:35Z

Sources/GRPCReflectionService/Server/ReflectionService.swift

@@ -33,26 +39,49 @@ public final class ReflectionService: CallHandlerProvider, Sendable {
  /// current working directory.
  ///
  /// - Parameter filePaths: The paths to files containing serialized reflection data.
+  /// - Parameter version: The version of the reflection, `v1` or `v1alpha`.


I'd recommend against documenting individual cases; the documentation can only become outdated.

Suggested change

/// - Parameter version: The version of the reflection, `v1` or `v1alpha`.

/// - Parameter version: The version of the reflection service to create.

glbrntt · 2023-11-06T17:27:20Z

Sources/GRPCReflectionService/Server/ReflectionService.swift

+  /// Represents the two possible `Providers`.
+  ///
+  /// Depending on the reflection version `v1` or `v1alpha` you provided to the initialiser,
+  /// ``init(serializedFileDescriptorProtoFilePaths:version:)``,
+  /// or ``init(fileDescriptorProtos:version:)``,
+  /// the Reflection Service will select its provider accordingly.


No need to document private types. General rule of thumb is to document all public types and any internal types which are non-trivial.

glbrntt · 2023-11-06T17:28:15Z

Sources/GRPCReflectionService/Server/ReflectionService.swift

-      return .success(.errorResponse(error))
+@available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
+extension ReflectionService {
+  /// Represents the version of the `reflection.proto`.


Suggested change

/// Represents the version of the `reflection.proto`.

/// The version of the reflection service.

glbrntt · 2023-11-06T17:31:10Z

Sources/GRPCReflectionService/Server/ReflectionService.swift

+    public static var v1: Self { Self(.v1) }
+    public static var v1Alpha: Self { Self(.v1Alpha) }


These ones still need docs. I'd also link to the source protos:

https://github.com/grpc/grpc/blob/master/src/proto/grpc/reflection/v1alpha/reflection.proto

https://github.com/grpc/grpc/blob/master/src/proto/grpc/reflection/v1/reflection.proto

glbrntt · 2023-11-07T11:01:32Z

Tests/GRPCTests/GRPCReflectionServiceTests/ReflectionServiceIntegrationTests.swift

-    try await serviceReflectionInfo.requestStream.send(
-      .with {
+    for version in self.versions {
+      try self.setUpServerAndChannel(version: version)


Now that we do the test twice, in the second iteration we'll be overwriting the existing client and server but never shutting them down. We also need to do the teardown at the end of each loop.

You can probably do this with a helper:

func forEachVersion(_ body: (GRPCChannel) async throws -> Void) async throws { for version in self.versions { let server = ... let channel = ... let result: Result<Void, Error> do { try await body(channel) result = .success(()) } catch { result = .failure(error) } // tear down client and server // ... try result.get() } }

Then each test can just do:

self.forEachVersion { client in // ... }

glbrntt · 2023-11-07T11:12:54Z

Tests/GRPCTests/GRPCReflectionServiceTests/ReflectionServiceIntegrationTests.swift

+        return XCTFail(
+          "The service of the received file descriptor proto doesn't have any methods."
+        )


You can also use let method = try XCTUnwrap(service.method.first) to unwrap optionals in tests

Tests/GRPCTests/GRPCReflectionServiceTests/ReflectionServiceIntegrationTests.swift

stefanadranca and others added 8 commits November 1, 2023 16:40

geberated clients for tests

3cc2493

added integration tests for v1alpha, changed the names of the protos …

0914a66

…to avoid build errors when generating the .grpc and .pb files

Merge branch 'main' into sd-v1alpha

af314aa

added return to switch cases

a081ed4

solved conflicts

ec4327e

Merge branch 'sd-v1alpha-temp' into sd-v1alpha

7931c93

sd-v1alpha-temp solved the conflict between main and v1alpha.

formatted

80e14d7

stefanadranca marked this pull request as ready for review November 3, 2023 14:29

fiexd naming for v1 proto file in Makefile

75961a4

stefanadranca requested a review from glbrntt November 3, 2023 14:47

glbrntt requested changes Nov 3, 2023

View reviewed changes

stefanadranca added 3 commits November 6, 2023 15:37

added extensions that allow converting from v1alpha types to v1 and c…

7cab00f

…hanged the testFileByFileName test

changed integration tests so both versions are test in the same funct…

8e9187c

…ions

added documentation for Version

6e6fa21

stefanadranca requested a review from glbrntt November 6, 2023 16:58

glbrntt requested changes Nov 7, 2023

View reviewed changes

implemented feedback

91d6a01

stefanadranca requested a review from glbrntt November 7, 2023 14:38

stefanadranca and others added 2 commits November 9, 2023 11:08

Merge branch 'main' into sd-v1alpha

406f066

fixed problem

f1e8780

glbrntt reviewed Nov 9, 2023

View reviewed changes

Tests/GRPCTests/GRPCReflectionServiceTests/ReflectionServiceIntegrationTests.swift Outdated Show resolved Hide resolved

Tests/GRPCTests/GRPCReflectionServiceTests/ReflectionServiceIntegrationTests.swift Outdated Show resolved Hide resolved

Apply suggestions from code review

c9ad0b1

glbrntt approved these changes Nov 9, 2023

View reviewed changes

glbrntt enabled auto-merge (squash) November 9, 2023 14:20

glbrntt added the semver/patch No public API change. label Nov 9, 2023

formatted

f88569b

glbrntt merged commit 187b609 into grpc:main Nov 9, 2023
12 of 14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for v1alpha reflection #1703

Adding support for v1alpha reflection #1703

stefanadranca commented Nov 1, 2023

glbrntt Nov 3, 2023

glbrntt Nov 3, 2023

glbrntt Nov 3, 2023

glbrntt Nov 6, 2023

glbrntt Nov 3, 2023

glbrntt Nov 3, 2023

glbrntt Nov 3, 2023

glbrntt left a comment

glbrntt Nov 6, 2023

glbrntt Nov 6, 2023

glbrntt Nov 6, 2023

glbrntt Nov 6, 2023

glbrntt Nov 7, 2023

glbrntt Nov 7, 2023

		public static var v1: Self { Self(.v1) }
		public static var v1Alpha: Self { Self(.v1Alpha) }

	/// - Parameter version: The version of the reflection, `v1` or `v1alpha`.
	/// - Parameter version: The version of the reflection service to create.

	/// Represents the version of the `reflection.proto`.
	/// The version of the reflection service.

Adding support for v1alpha reflection #1703

Adding support for v1alpha reflection #1703

Conversation

stefanadranca commented Nov 1, 2023

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

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