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.

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

Added support for Error Responses from the Reflection Service #1682

Merged
merged 14 commits into from
Oct 27, 2023

Conversation

stefanadranca
Copy link
Collaborator

Motivation:

Whenever an error occurs during the exchange between the server implementing the Reflection Service and a client, the client should receive an Error Message.

Modifications:

Replaced throwing errors with creating an Error Reponse and sending it back to the client. Also, changed the APIs to use the Result type and added new extensions for the Reflection_ServerReflectionResponse initialisation. Also modified the tests.

Result:

When an error occurs within the Reflection Service, the clients will be sent an Error Response.

stefanadranca and others added 4 commits October 24, 2023 14:11
Motivation:

Whenever an error occurs during the exchange between the server implementing the Reflection Service and a client, the client should receive an Error Message.

Modifications:

Replaced throwing errors with creating an Error Reponse and sending it back to the client. Also, changed the APIs to use the Result type and added new extensions for the Reflection_ServerReflectionResponse initialisation. Also modified the tests.

Result:

When an error occurs within the Reflection Service, the clients will be sent an Error Response.
@stefanadranca stefanadranca marked this pull request as ready for review October 25, 2023 09:55
)
}
return fieldNumbers
return .success((typeName, fieldNumbers))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we returning the type name 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.

We need it in the Server Response, so if we want to have an init for it with only a result type and the initial request as parameters we need to include it in the result. If not we would need a third parameter for the init

switch nameOfFileContainingSymbolResult {
case .success(let fileName):
return findFileByFileName(fileName, request: request)
case .failure(let gRPCStatus):
Copy link
Collaborator

Choose a reason for hiding this comment

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

just call this status, no need to spell it out as gRPCStatus

$0.validHost = request.host
$0.originalRequest = request
$0.listServicesResponse = listServicesResponse
switch result {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have a bunch of repeated code in all of these inits. We can simplify this quite a lot and remove a lot of the redundancy and avoid switching in a bunch of places.

The thing that all responses have in common is the OneOf_MessageResponse and the original request, so we can work backwards from that. Our starting point in most cases is Result<X, GRPCStatus> (and if not, that's very easy to get to).

The first step we can do is getting from X to Reflection_ServerReflectionResponse.OneOf_MessageResponse, that's quite easy, we don't need helpers for that, e.g.:

let name = "Foo"
let result = self.registry.extensionsFieldNumbersOfType(named: name).map { fieldNumbers in
  Reflection_ExtensionNumberResponse.with { 
    $0.fieldNumbers = fieldNumbers
    $0.typeName = name
  }
}

The next step is getting from GRPCStatus as the Failure type of the Result to Reflection_ServerReflectionResponse.OneOf_MessageResponse as the Success type. We can do this with an extension on Result:

extension Result where Success == Reflection_ServerReflectionResponse.OneOf_MessageResponse, Failure == GRPCStatus {
  func recover() -> Result<Reflection_ServerReflectionResponse.OneOf_MessageResponse, Never> {
    self.flatMapError { status in
      let error = Reflection_ErrorResponse.with {
        $0.errorCode = Int32(status.code.rawValue)
        $0.errorMessage = status.message ?? ""
      }

      return .success(.errorResponse(error))
    }
  }
}

Now we can quite easily wrangle Result<X, GRPCStatus> into Result< Reflection_ServerReflectionResponse.OneOf_MessageResponse, Never>.

The next bit is mapping Reflection_ServerReflectionResponse.OneOf_MessageResponse into a Reflection_ServerReflectionResponse, another extension helps us out here:

extension Result where Success == Reflection_ServerReflectionResponse.OneOf_MessageResponse {
  func attachRequest(_ request: Reflection_ServerReflectionRequest) -> Result<Reflection_ServerReflectionResponse, Failure> {
    self.map { message in
      Reflection_ServerReflectionResponse.with {
        $0.validHost = request.host
        $0.originalRequest = request
        $0.messageResponse = message
      }
    }
  }
}

We can actually combine the above two functions into one which also unwraps the response result into a response:

extension Result where Success == Reflection_ServerReflectionResponse.OneOf_MessageResponse, Failure == GRPCStatus {
  func makeResponse(request: Reflection_ServerReflectionRequest) -> Reflection_ServerReflectionResponse {
    let result = self.recover().attachingRequest(request)
    // Safe to '!' as the failure type is 'Never'.
    return try! result.get()
  }
}

Going back to the original example, this is all we need:

let name = "Foo"
let result = self.registry.extensionsFieldNumbersOfType(named: name).map { fieldNumbers in
  Reflection_ExtensionNumberResponse.with { 
    $0.fieldNumbers = fieldNumbers
    $0.typeName = name
  }
}

return result.makeResponse(request: request)

Comment on lines 223 to 227
Reflection_FileDescriptorResponse.with {
$0.fileDescriptorProto = fileDescriptorProtos
}
}.map {
Reflection_ServerReflectionResponse.OneOf_MessageResponse.fileDescriptorResponse($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 merge these into a single map:

.map { fileDescriptorProtos in 
  Reflection_ServerReflectionResponse.OneOf_MessageResponse.fileDescriptorResponse(.with{
    $0.fileDescriptorProto = fileDescriptorProtos
  })
}

Comment on lines 438 to 444
func makeResponse(
request: Reflection_ServerReflectionRequest
) -> Reflection_ServerReflectionResponse {
let result = self.recover().attachRequest(request)
// Safe to '!' as the failure type is 'Never'.
return try! result.get()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move this one up to where recover() is defined, the extension has the same requirements

Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

This looks much better! I left a couple of nits but this is basically good to go otherwise.

Comment on lines 249 to 252
messageResponse: Reflection_ServerReflectionResponse.OneOf_MessageResponse
.listServicesResponse(
listServicesResponse
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use type inference here:

Suggested change
messageResponse: Reflection_ServerReflectionResponse.OneOf_MessageResponse
.listServicesResponse(
listServicesResponse
)
messageResponse: .listServicesResponse(listServicesResponse)

throw GRPCStatus(code: .unimplemented)
let response = Reflection_ServerReflectionResponse(
request: request,
messageResponse: Reflection_ServerReflectionResponse.OneOf_MessageResponse.errorResponse(
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: use type-inference here

Suggested change
messageResponse: Reflection_ServerReflectionResponse.OneOf_MessageResponse.errorResponse(
messageResponse: .errorResponse(

Comment on lines 409 to 410
}
func makeResponse(
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: leave a blank line between functions

Suggested change
}
func makeResponse(
}
func makeResponse(

let nameOfFileContainingSymbolResult = registry.nameOfFileContainingSymbol(
named: "packagebar2.enumType2"
)
switch nameOfFileContainingSymbolResult {
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to change these now, but it would've been much easier to just do:

XCTAssertEqual(try nameOfFileContainingSymbolResult.get(), "bar2.proto")

@@ -117,134 +117,199 @@ final class ReflectionServiceUnitTests: GRPCTestCase {
func testNameOfFileContainingSymbolEnum() throws {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be great to add some integration tests which validate we get appropriate error responses too. Feel free to do it in a separate PR if you prefer.

Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Nice one!

@glbrntt glbrntt merged commit cdb9fc6 into grpc:main Oct 27, 2023
14 checks passed
@glbrntt glbrntt added the semver-patch • Requires a SemVer Patch version change label Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch • Requires a SemVer Patch version change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants