Skip to content
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

Jump to bottom

Added ReflectionService initialiser using file paths #1699

Merged
merged 15 commits into from
Nov 3, 2023
Merged

Added ReflectionService initialiser using file paths #1699

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
92a8f2d
Added ReflectionService initializer using file paths
stefanadranca Nov 1, 2023
1fea48d
added OS versions availability check
stefanadranca Nov 1, 2023
b2a0e95
availability check fix?
stefanadranca Nov 1, 2023
f808eea
checking os
stefanadranca Nov 1, 2023
2c003ce
checking os for integration tests
stefanadranca Nov 1, 2023
76bb1f1
Merge branch 'main' into sd-reflection-service
stefanadranca Nov 1, 2023
8e0fa46
changed tmp directory name for linux
stefanadranca Nov 1, 2023
2431bd1
still need to make some changes
stefanadranca Nov 1, 2023
c738569
implemented feedback
stefanadranca Nov 2, 2023
38b7a6f
Merge branch 'main' into sd-reflection-service
stefanadranca Nov 2, 2023
f30f52a
Merge branch 'main' into sd-reflection-service
stefanadranca Nov 2, 2023
cb7ab5d
implemented feedback and added test for error
stefanadranca Nov 2, 2023
65d66fb
Merge branch 'main' into sd-reflection-service
stefanadranca Nov 2, 2023
c05eba6
added missing option
stefanadranca Nov 3, 2023
2beacc0
Merge branch 'main' into sd-reflection-service
glbrntt Nov 3, 2023
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
25 changes: 25 additions & 0 deletions Sources/GRPCReflectionService/Server/ReflectionService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ public final class ReflectionService: CallHandlerProvider, Sendable {
self.reflectionService.serviceName
}

public init(protoFilePaths: [String]) throws {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

public init(serializedFileDescriptorProtoFilePaths: [String]) throws {

Copy link
Collaborator

Choose a reason for hiding this comment

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

This also needs documentation

self.reflectionService = try ReflectionServiceProvider(protoFilePaths: protoFilePaths)
}

public init(fileDescriptors: [Google_Protobuf_FileDescriptorProto]) throws {
self.reflectionService = try ReflectionServiceProvider(fileDescriptorProtos: fileDescriptors)
}
Expand Down Expand Up @@ -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 {

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: unnecessary blank line

Suggested change

let binaryFilesURLs = protoFilePaths.map {
#if os(Linux)
URL(fileURLWithPath: $0)
#else
if #available(macOS 13.0, iOS 16.0, tvOS 16.0, watchOS 9.0, *) {
URL(filePath: $0, directoryHint: .notDirectory)
} else {
URL(fileURLWithPath: $0)
}
#endif
}
let binaryData = binaryFilesURLs.map { Data(base64Encoded: try! Data(contentsOf: $0)) }
let fileDescriptorProtos = binaryData.map {
try! Google_Protobuf_FileDescriptorProto(serializedData: $0!)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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(...)
}


self.protoRegistry = try ReflectionServiceData(fileDescriptors: fileDescriptorProtos)
}

internal init(fileDescriptorProtos: [Google_Protobuf_FileDescriptorProto]) throws {
self.protoRegistry = try ReflectionServiceData(
fileDescriptors: fileDescriptorProtos
Expand Down
25 changes: 22 additions & 3 deletions Tests/GRPCTests/GRPCReflectionServiceTests/ReflectionServiceIntegrationTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,28 @@ final class ReflectionServiceIntegrationTests: GRPCTestCase {
)

private func setUpServerAndChannel() throws {
let reflectionServiceProvider = try ReflectionService(
fileDescriptors: self.protos + [self.independentProto]
)
var filePaths: [String] = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

let fileManager = FileManager.default

let fileDescriptorProtos = self.protos + [self.independentProto]
for fileDescriptorProto in fileDescriptorProtos {
let data = try fileDescriptorProto.serializedData()
.base64EncodedData()
#if os(Linux)
let temporaryDirectory = "/tmp/"
#else
let temporaryDirectory = fileManager.temporaryDirectory.path()
#endif
let filePath = temporaryDirectory + fileDescriptorProto.name
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

fileManager.createFile(atPath: filePath, contents: data)
filePaths.append(filePath)
}
defer {
for filePath in filePaths {
XCTAssertNoThrow(try fileManager.removeItem(atPath: filePath))
}
}
let reflectionServiceProvider = try ReflectionService(protoFilePaths: filePaths)

let server = try Server.insecure(group: MultiThreadedEventLoopGroup.singleton)
.withServiceProviders([reflectionServiceProvider])
Expand Down
Loading