Merge pull request #2051 from jcanizales/simplify-metadata

Simplify metadata usage

gRPC.podspec

@@ -1,6 +1,6 @@
 Pod::Spec.new do |s|
   s.name     = 'gRPC'
-  s.version  = '0.5.1'
+  s.version  = '0.6.0'
   s.summary  = 'gRPC client library for iOS/OSX'
   s.homepage = 'http://www.grpc.io'
   s.license  = 'New BSD'

src/objective-c/GRPCClient/GRPCCall.h

@@ -31,43 +31,55 @@
  *
  */
 
+// The gRPC protocol is an RPC protocol on top of HTTP2.
+//
+// While the most common type of RPC receives only one request message and returns only one response
+// message, the protocol also supports RPCs that return multiple individual messages in a streaming
+// fashion, RPCs that accept a stream of request messages, or RPCs with both streaming requests and
+// responses.
+//
+// Conceptually, each gRPC call consists of a bidirectional stream of binary messages, with RPCs of
+// the "non-streaming type" sending only one message in the corresponding direction (the protocol
+// doesn't make any distinction).
+//
+// Each RPC uses a different HTTP2 stream, and thus multiple simultaneous RPCs can be multiplexed
+// transparently on the same TCP connection.
+
 #import <Foundation/Foundation.h>
 #import <gRPC/GRXWriter.h>
 
 @class GRPCMethodName;
 
-@class GRPCCall;
+// Key used in |NSError|'s |userInfo| dictionary to store the response metadata sent by the server.
+extern id const kGRPCStatusMetadataKey;
 
-// The gRPC protocol is an RPC protocol on top of HTTP2.
-//
-// While the most common type of RPC receives only one request message and
-// returns only one response message, the protocol also supports RPCs that
-// return multiple individual messages in a streaming fashion, RPCs that
-// accept a stream of request messages, or RPCs with both streaming requests
-// and responses.
-//
-// Conceptually, each gRPC call consists of a bidirectional stream of binary
-// messages, with RPCs of the "non-streaming type" sending only one message in
-// the corresponding direction (the protocol doesn't make any distinction).
-//
-// Each RPC uses a different HTTP2 stream, and thus multiple simultaneous RPCs
-// can be multiplexed transparently on the same TCP connection.
+// Represents a single gRPC remote call.
 @interface GRPCCall : NSObject<GRXWriter>
 
-// These HTTP2 headers will be passed to the server as part of this call. Each
-// HTTP2 header is a name-value pair with string names and either string or binary values.
+// These HTTP headers will be passed to the server as part of this call. Each HTTP header is a
+// name-value pair with string names and either string or binary values.
+//
 // The passed dictionary has to use NSString keys, corresponding to the header names. The
 // value associated to each can be a NSString object or a NSData object. E.g.:
 //
-// call.requestMetadata = @{
-//     @"Authorization": @"Bearer ...",
-//     @"SomeBinaryHeader": someData
-// };
+// call.requestMetadata = @{@"Authorization": @"Bearer ..."};
+//
+// call.requestMetadata[@"SomeBinaryHeader"] = someData;
 //
 // After the call is started, modifying this won't have any effect.
-@property(nonatomic, readwrite) NSMutableDictionary *requestMetadata;
+//
+// For convenience, the property is initialized to an empty NSMutableDictionary, and the setter
+// accepts (and copies) both mutable and immutable dictionaries.
+- (NSMutableDictionary *)requestMetadata; // nonatomic
+- (void)setRequestMetadata:(NSDictionary *)requestMetadata; // nonatomic, copy
 
-// This isn't populated until the first event is delivered to the handler.
+// This dictionary is populated with the HTTP headers received from the server. When the RPC ends,
+// the HTTP trailers received are added to the dictionary too. It has the same structure as the
+// request metadata dictionary.
+//
+// The first time this object calls |writeValue| on the writeable passed to |startWithWriteable|,
+// the |responseMetadata| dictionary already contains the response headers. When it calls
+// |writesFinishedWithError|, the dictionary contains both the response headers and trailers.
 @property(atomic, readonly) NSDictionary *responseMetadata;
 
 // The request writer has to write NSData objects into the provided Writeable. The server will

src/objective-c/GRPCClient/GRPCCall.m

@@ -46,9 +46,9 @@
 #import "private/NSDictionary+GRPC.h"
 #import "private/NSError+GRPC.h"
 
+NSString * const kGRPCStatusMetadataKey = @"io.grpc.StatusMetadataKey";
+
 @interface GRPCCall () <GRXWriteable>
-// Makes it readwrite.
-@property(atomic, strong) NSDictionary *responseMetadata;
 @end
 
 // The following methods of a C gRPC call object aren't reentrant, and thus
@@ -82,6 +82,9 @@
   // correct ordering.
   GRPCDelegateWrapper *_responseWriteable;
   id<GRXWriter> _requestWriter;
+
+  NSMutableDictionary *_requestMetadata;
+  NSMutableDictionary *_responseMetadata;
 }
 
 @synthesize state = _state;
@@ -116,10 +119,27 @@
     _callQueue = dispatch_queue_create("org.grpc.call", NULL);
 
     _requestWriter = requestWriter;
+
+    _requestMetadata = [NSMutableDictionary dictionary];
+    _responseMetadata = [NSMutableDictionary dictionary];
   }
   return self;
 }
 
+#pragma mark Metadata
+
+- (NSMutableDictionary *)requestMetadata {
+  return _requestMetadata;
+}
+
+- (void)setRequestMetadata:(NSDictionary *)requestMetadata {
+  _requestMetadata = [NSMutableDictionary dictionaryWithDictionary:requestMetadata];
+}
+
+- (NSDictionary *)responseMetadata {
+  return _responseMetadata;
+}
+
 #pragma mark Finish
 
 - (void)finishWithError:(NSError *)errorOrNil {
@@ -277,7 +297,7 @@
 // The first one (metadataHandler), when the response headers are received.
 // The second one (completionHandler), whenever the RPC finishes for any reason.
 - (void)invokeCallWithMetadataHandler:(void(^)(NSDictionary *))metadataHandler
-                    completionHandler:(void(^)(NSError *))completionHandler {
+                    completionHandler:(void(^)(NSError *, NSDictionary *))completionHandler {
   // TODO(jcanizales): Add error handlers for async failures
   [_wrappedCall startBatchWithOperations:@[[[GRPCOpRecvMetadata alloc]
                                             initWithHandler:metadataHandler]]];
@@ -287,16 +307,23 @@
 
 - (void)invokeCall {
   __weak GRPCCall *weakSelf = self;
-  [self invokeCallWithMetadataHandler:^(NSDictionary *metadata) {
-    // Response metadata received.
+  [self invokeCallWithMetadataHandler:^(NSDictionary *headers) {
+    // Response headers received.
     GRPCCall *strongSelf = weakSelf;
     if (strongSelf) {
-      strongSelf.responseMetadata = metadata;
+      [strongSelf->_responseMetadata addEntriesFromDictionary:headers];
       [strongSelf startNextRead];
     }
-  } completionHandler:^(NSError *error) {
-    // TODO(jcanizales): Merge HTTP2 trailers into response metadata.
-    [weakSelf finishWithError:error];
+  } completionHandler:^(NSError *error, NSDictionary *trailers) {
+    GRPCCall *strongSelf = weakSelf;
+    if (strongSelf) {
+      [strongSelf->_responseMetadata addEntriesFromDictionary:trailers];
+
+      NSMutableDictionary *userInfo = [NSMutableDictionary dictionaryWithDictionary:error.userInfo];
+      userInfo[kGRPCStatusMetadataKey] = strongSelf->_responseMetadata;
+      error = [NSError errorWithDomain:error.domain code:error.code userInfo:userInfo];
+      [strongSelf finishWithError:error];
+    }
   }];
   // Now that the RPC has been initiated, request writes can start.
   [_requestWriter startWithWriteable:self];

src/objective-c/GRPCClient/private/GRPCWrappedCall.h

@@ -79,7 +79,7 @@ typedef void(^GRPCCompletionHandler)(NSDictionary *);
 
 @interface GRPCOpRecvStatus : NSObject <GRPCOp>
 
-- (instancetype)initWithHandler:(void(^)(NSError *))handler NS_DESIGNATED_INITIALIZER;
+- (instancetype)initWithHandler:(void(^)(NSError *, NSDictionary *))handler NS_DESIGNATED_INITIALIZER;
 
 @end
 

src/objective-c/GRPCClient/private/GRPCWrappedCall.m

@@ -165,9 +165,7 @@
 }
 
 - (void)finish {
-  NSDictionary *metadata = [NSDictionary
-                            grpc_dictionaryFromMetadata:_recvInitialMetadata.metadata
-                            count:_recvInitialMetadata.count];
+  NSDictionary *metadata = [NSDictionary grpc_dictionaryFromMetadataArray:_recvInitialMetadata];
   if (_handler) {
     _handler(metadata);
   }
@@ -209,41 +207,44 @@
 @end
 
 @implementation GRPCOpRecvStatus{
-  void(^_handler)(NSError *);
+  void(^_handler)(NSError *, NSDictionary *);
+  grpc_status_code _statusCode;
+  char *_details;
   size_t _detailsCapacity;
-  grpc_status _status;
+  grpc_metadata_array _metadata;
 }
 
 - (instancetype) init {
   return [self initWithHandler:nil];
 }
 
-- (instancetype) initWithHandler:(void (^)(NSError *))handler {
+- (instancetype) initWithHandler:(void (^)(NSError *, NSDictionary *))handler {
   if (self = [super init]) {
     _handler = handler;
-    grpc_metadata_array_init(&_status.metadata);
+    grpc_metadata_array_init(&_metadata);
   }
   return self;
 }
 
 - (void)getOp:(grpc_op *)op {
   op->op = GRPC_OP_RECV_STATUS_ON_CLIENT;
-  op->data.recv_status_on_client.status = &_status.status;
-  op->data.recv_status_on_client.status_details = &_status.details;
+  op->data.recv_status_on_client.status = &_statusCode;
+  op->data.recv_status_on_client.status_details = &_details;
   op->data.recv_status_on_client.status_details_capacity = &_detailsCapacity;
-  op->data.recv_status_on_client.trailing_metadata = &_status.metadata;
+  op->data.recv_status_on_client.trailing_metadata = &_metadata;
 }
 
 - (void)finish {
   if (_handler) {
-    NSError *error = [NSError grpc_errorFromStatus:&_status];
-    _handler(error);
+    NSError *error = [NSError grpc_errorFromStatusCode:_statusCode details:_details];
+    NSDictionary *trailers = [NSDictionary grpc_dictionaryFromMetadataArray:_metadata];
+    _handler(error, trailers);
   }
 }
 
 - (void)dealloc {
-  grpc_metadata_array_destroy(&_status.metadata);
-  gpr_free(_status.details);
+  grpc_metadata_array_destroy(&_metadata);
+  gpr_free(_details);
 }
 
 @end

src/objective-c/GRPCClient/private/NSDictionary+GRPC.h

@@ -35,6 +35,7 @@
 #include <grpc/grpc.h>
 
 @interface NSDictionary (GRPC)
-+ (instancetype)grpc_dictionaryFromMetadata:(struct grpc_metadata *)entries count:(size_t)count;
++ (instancetype)grpc_dictionaryFromMetadataArray:(grpc_metadata_array)array;
++ (instancetype)grpc_dictionaryFromMetadata:(grpc_metadata *)entries count:(size_t)count;
 - (grpc_metadata *)grpc_metadataArray;
 @end

src/objective-c/GRPCClient/private/NSDictionary+GRPC.m

@@ -98,14 +98,18 @@
 #pragma mark Category for metadata arrays
 
 @implementation NSDictionary (GRPC)
++ (instancetype)grpc_dictionaryFromMetadataArray:(grpc_metadata_array)array {
+  return [self grpc_dictionaryFromMetadata:array.metadata count:array.count];
+}
+
 + (instancetype)grpc_dictionaryFromMetadata:(grpc_metadata *)entries count:(size_t)count {
   NSMutableDictionary *metadata = [NSMutableDictionary dictionaryWithCapacity:count];
   for (grpc_metadata *entry = entries; entry < entries + count; entry++) {
     // TODO(jcanizales): Verify in a C library test that it's converting header names to lower case
     // automatically.
     NSString *name = [NSString stringWithCString:entry->key encoding:NSASCIIStringEncoding];
-    if (!name) {
-      // log?
+    if (!name || metadata[name]) {
+      // Log if name is nil?
       continue;
     }
     id value;
@@ -115,10 +119,7 @@
     } else {
       value = [NSString grpc_stringFromMetadataValue:entry];
     }
-    if (!metadata[name]) {
-      metadata[name] = [NSMutableArray array];
-    }
-    [metadata[name] addObject:value];
+    metadata[name] = value;
   }
   return metadata;
 }

src/objective-c/GRPCClient/private/NSError+GRPC.h

@@ -32,6 +32,7 @@
  */
 
 #import <Foundation/Foundation.h>
+#include <grpc/grpc.h>
 
 // TODO(jcanizales): Make the domain string public.
 extern NSString *const kGRPCErrorDomain;
@@ -56,17 +57,8 @@ typedef NS_ENUM(NSInteger, GRPCErrorCode) {
   GRPCErrorCodeDataLoss = 15
 };
 
-// TODO(jcanizales): This is conflating trailing metadata with Status details. Fix it once there's
-// a decision on how to codify Status.
-#include <grpc/grpc.h>
-typedef struct grpc_status {
-    grpc_status_code status;
-    char *details;
-    grpc_metadata_array metadata;
-} grpc_status;
-
 @interface NSError (GRPC)
-// Returns nil if the status is OK. Otherwise, a NSError whose code is one of
-// GRPCErrorCode and whose domain is kGRPCErrorDomain.
-+ (instancetype)grpc_errorFromStatus:(struct grpc_status *)status;
+// Returns nil if the status code is OK. Otherwise, a NSError whose code is one of |GRPCErrorCode|
+// and whose domain is |kGRPCErrorDomain|.
++ (instancetype)grpc_errorFromStatusCode:(grpc_status_code)statusCode details:(char *)details;
 @end

src/objective-c/GRPCClient/private/NSError+GRPC.m

@@ -35,17 +35,16 @@
 
 #include <grpc.h>
 
-NSString *const kGRPCErrorDomain = @"org.grpc";
+NSString * const kGRPCErrorDomain = @"io.grpc";
 
 @implementation NSError (GRPC)
-+ (instancetype)grpc_errorFromStatus:(struct grpc_status *)status {
-  if (status->status == GRPC_STATUS_OK) {
++ (instancetype)grpc_errorFromStatusCode:(grpc_status_code)statusCode details:(char *)details {
+  if (statusCode == GRPC_STATUS_OK) {
     return nil;
   }
-  NSString *message =
-      [NSString stringWithFormat:@"Code=%i Message='%s'", status->status, status->details];
+  NSString *message = [NSString stringWithCString:details encoding:NSASCIIStringEncoding];
   return [NSError errorWithDomain:kGRPCErrorDomain
-                             code:status->status
+                             code:statusCode
                          userInfo:@{NSLocalizedDescriptionKey: message}];
 }
 @end

src/objective-c/tests/GRPCClientTests.m

@@ -43,24 +43,38 @@
 // These are a few tests similar to InteropTests, but which use the generic gRPC client (GRPCCall)
 // rather than a generated proto library on top of it.
 
+static NSString * const kHostAddress = @"grpc-test.sandbox.google.com";
+static NSString * const kPackage = @"grpc.testing";
+static NSString * const kService = @"TestService";
+
+static GRPCMethodName *kInexistentMethod;
+static GRPCMethodName *kEmptyCallMethod;
+static GRPCMethodName *kUnaryCallMethod;
+
 @interface GRPCClientTests : XCTestCase
 @end
 
 @implementation GRPCClientTests
 
-- (void)testConnectionToRemoteServer {
-  __weak XCTestExpectation *expectation = [self expectationWithDescription:@"Server reachable."];
-
+- (void)setUp {
   // This method isn't implemented by the remote server.
-  GRPCMethodName *method = [[GRPCMethodName alloc] initWithPackage:@"grpc.testing"
-                                                         interface:@"TestService"
-                                                            method:@"Nonexistent"];
+  kInexistentMethod = [[GRPCMethodName alloc] initWithPackage:kPackage
+                                                    interface:kService
+                                                       method:@"Inexistent"];
+  kEmptyCallMethod = [[GRPCMethodName alloc] initWithPackage:kPackage
+                                                   interface:kService
+                                                      method:@"EmptyCall"];
+  kUnaryCallMethod = [[GRPCMethodName alloc] initWithPackage:kPackage
+                                                   interface:kService
+                                                      method:@"UnaryCall"];
+}
 
-  id<GRXWriter> requestsWriter = [GRXWriter writerWithValue:[NSData data]];
+- (void)testConnectionToRemoteServer {
+  __weak XCTestExpectation *expectation = [self expectationWithDescription:@"Server reachable."];
 
-  GRPCCall *call = [[GRPCCall alloc] initWithHost:@"grpc-test.sandbox.google.com"
-                                           method:method
-                                   requestsWriter:requestsWriter];
+  GRPCCall *call = [[GRPCCall alloc] initWithHost:kHostAddress
+                                           method:kInexistentMethod
+                                   requestsWriter:[GRXWriter writerWithValue:[NSData data]]];
 
   id<GRXWriteable> responsesWriteable = [[GRXWriteable alloc] initWithValueHandler:^(NSData *value) {
     XCTFail(@"Received unexpected response: %@", value);
@@ -80,15 +94,9 @@
   __weak XCTestExpectation *response = [self expectationWithDescription:@"Empty response received."];
   __weak XCTestExpectation *completion = [self expectationWithDescription:@"Empty RPC completed."];
 
-  GRPCMethodName *method = [[GRPCMethodName alloc] initWithPackage:@"grpc.testing"
-                                                         interface:@"TestService"
-                                                            method:@"EmptyCall"];
-
-  id<GRXWriter> requestsWriter = [GRXWriter writerWithValue:[NSData data]];
-
-  GRPCCall *call = [[GRPCCall alloc] initWithHost:@"grpc-test.sandbox.google.com"
-                                           method:method
-                                   requestsWriter:requestsWriter];
+  GRPCCall *call = [[GRPCCall alloc] initWithHost:kHostAddress
+                                           method:kEmptyCallMethod
+                                   requestsWriter:[GRXWriter writerWithValue:[NSData data]]];
 
   id<GRXWriteable> responsesWriteable = [[GRXWriteable alloc] initWithValueHandler:^(NSData *value) {
     XCTAssertNotNil(value, @"nil value received as response.");
@@ -105,34 +113,27 @@
 }
 
 - (void)testSimpleProtoRPC {
-  __weak XCTestExpectation *response = [self expectationWithDescription:@"Response received."];
-  __weak XCTestExpectation *expectedResponse =
-  [self expectationWithDescription:@"Expected response."];
+  __weak XCTestExpectation *response = [self expectationWithDescription:@"Expected response."];
   __weak XCTestExpectation *completion = [self expectationWithDescription:@"RPC completed."];
 
-  GRPCMethodName *method = [[GRPCMethodName alloc] initWithPackage:@"grpc.testing"
-                                                         interface:@"TestService"
-                                                            method:@"UnaryCall"];
-
-  RMTSimpleRequest *request = [[RMTSimpleRequest alloc] init];
+  RMTSimpleRequest *request = [RMTSimpleRequest message];
   request.responseSize = 100;
   request.fillUsername = YES;
   request.fillOauthScope = YES;
   id<GRXWriter> requestsWriter = [GRXWriter writerWithValue:[request data]];
 
-  GRPCCall *call = [[GRPCCall alloc] initWithHost:@"grpc-test.sandbox.google.com"
-                                           method:method
+  GRPCCall *call = [[GRPCCall alloc] initWithHost:kHostAddress
+                                           method:kUnaryCallMethod
                                    requestsWriter:requestsWriter];
 
   id<GRXWriteable> responsesWriteable = [[GRXWriteable alloc] initWithValueHandler:^(NSData *value) {
     XCTAssertNotNil(value, @"nil value received as response.");
-    [response fulfill];
     XCTAssertGreaterThan(value.length, 0, @"Empty response received.");
-    RMTSimpleResponse *response = [RMTSimpleResponse parseFromData:value error:NULL];
+    RMTSimpleResponse *responseProto = [RMTSimpleResponse parseFromData:value error:NULL];
     // We expect empty strings, not nil:
-    XCTAssertNotNil(response.username, @"Response's username is nil.");
-    XCTAssertNotNil(response.oauthScope, @"Response's OAuth scope is nil.");
-    [expectedResponse fulfill];
+    XCTAssertNotNil(responseProto.username, @"Response's username is nil.");
+    XCTAssertNotNil(responseProto.oauthScope, @"Response's OAuth scope is nil.");
+    [response fulfill];
   } completionHandler:^(NSError *errorOrNil) {
     XCTAssertNil(errorOrNil, @"Finished with unexpected error: %@", errorOrNil);
     [completion fulfill];
@@ -143,4 +144,36 @@
   [self waitForExpectationsWithTimeout:2. handler:nil];
 }
 
+- (void)testMetadata {
+  __weak XCTestExpectation *expectation = [self expectationWithDescription:@"RPC unauthorized."];
+
+  RMTSimpleRequest *request = [RMTSimpleRequest message];
+  request.fillUsername = YES;
+  request.fillOauthScope = YES;
+  id<GRXWriter> requestsWriter = [GRXWriter writerWithValue:[request data]];
+
+  GRPCCall *call = [[GRPCCall alloc] initWithHost:kHostAddress
+                                           method:kUnaryCallMethod
+                                   requestsWriter:requestsWriter];
+
+  call.requestMetadata[@"Authorization"] = @"Bearer bogusToken";
+
+  id<GRXWriteable> responsesWriteable = [[GRXWriteable alloc] initWithValueHandler:^(NSData *value) {
+    XCTFail(@"Received unexpected response: %@", value);
+  } completionHandler:^(NSError *errorOrNil) {
+    XCTAssertNotNil(errorOrNil, @"Finished without error!");
+    XCTAssertEqual(errorOrNil.code, 16, @"Finished with unexpected error: %@", errorOrNil);
+    XCTAssertEqualObjects(call.responseMetadata, errorOrNil.userInfo[kGRPCStatusMetadataKey],
+                          @"Metadata in the NSError object and call object differ.");
+    NSString *challengeHeader = call.responseMetadata[@"www-authenticate"];
+    XCTAssertGreaterThan(challengeHeader.length, 0,
+                         @"No challenge in response headers %@", call.responseMetadata);
+    [expectation fulfill];
+  }];
+
+  [call startWithWriteable:responsesWriteable];
+
+  [self waitForExpectationsWithTimeout:2. handler:nil];
+}
+
 @end