From 84e3cdeb970dae5840ab0453536a7a53428a8e65 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 20 Aug 2015 11:27:05 -0700 Subject: [PATCH 01/10] Added new Metadata class to abstract over internal representation and normalize keys --- src/node/index.js | 15 +-- src/node/interop/interop_client.js | 8 +- src/node/src/client.js | 65 +++++++---- src/node/src/metadata.js | 167 ++++++++++++++++++++++++++++ src/node/src/server.js | 60 ++++++---- src/node/test/health_test.js | 2 +- src/node/test/metadata_test.js | 172 +++++++++++++++++++++++++++++ src/node/test/surface_test.js | 61 +++++----- 8 files changed, 460 insertions(+), 90 deletions(-) create mode 100644 src/node/src/metadata.js create mode 100644 src/node/test/metadata_test.js diff --git a/src/node/index.js b/src/node/index.js index 889b0ac0e92..51d3fa590cd 100644 --- a/src/node/index.js +++ b/src/node/index.js @@ -41,6 +41,8 @@ var client = require('./src/client.js'); var server = require('./src/server.js'); +var Metadata = require('./src/metadata.js'); + var grpc = require('bindings')('grpc'); /** @@ -107,18 +109,12 @@ exports.getGoogleAuthDelegate = function getGoogleAuthDelegate(credential) { * @param {function(Error, Object)} callback */ return function updateMetadata(authURI, metadata, callback) { - metadata = _.clone(metadata); - if (metadata.Authorization) { - metadata.Authorization = _.clone(metadata.Authorization); - } else { - metadata.Authorization = []; - } credential.getRequestMetadata(authURI, function(err, header) { if (err) { callback(err); return; } - metadata.Authorization.push(header.Authorization); + metadata.add('authorization', header.Authorization); callback(null, metadata); }); }; @@ -129,6 +125,11 @@ exports.getGoogleAuthDelegate = function getGoogleAuthDelegate(credential) { */ exports.Server = server.Server; +/** + * @see module:src/metadata + */ +exports.Metadata = Metadata; + /** * Status name to code number mapping */ diff --git a/src/node/interop/interop_client.js b/src/node/interop/interop_client.js index 612dcf01f65..8fb8d669206 100644 --- a/src/node/interop/interop_client.js +++ b/src/node/interop/interop_client.js @@ -321,13 +321,7 @@ function oauth2Test(expected_user, scope, per_rpc, client, done) { credential.getAccessToken(function(err, token) { assert.ifError(err); var updateMetadata = function(authURI, metadata, callback) { - metadata = _.clone(metadata); - if (metadata.Authorization) { - metadata.Authorization = _.clone(metadata.Authorization); - } else { - metadata.Authorization = []; - } - metadata.Authorization.push('Bearer ' + token); + metadata.Add('authorization', 'Bearer ' + token); callback(null, metadata); }; var makeTestCall = function(error, client_metadata) { diff --git a/src/node/src/client.js b/src/node/src/client.js index 48fe0dd3b77..6fafad251a2 100644 --- a/src/node/src/client.js +++ b/src/node/src/client.js @@ -42,7 +42,9 @@ var _ = require('lodash'); var grpc = require('bindings')('grpc.node'); -var common = require('./common.js'); +var common = require('./common'); + +var Metadata = require('./metadata'); var EventEmitter = require('events').EventEmitter; @@ -254,8 +256,7 @@ function makeUnaryRequestFunction(method, serialize, deserialize) { * serialize * @param {function(?Error, value=)} callback The callback to for when the * response is received - * @param {array=} metadata Array of metadata key/value pairs to add to the - * call + * @param {Metadata=} metadata Metadata to add to the call * @param {Object=} options Options map * @return {EventEmitter} An event emitter for stream related events */ @@ -264,7 +265,9 @@ function makeUnaryRequestFunction(method, serialize, deserialize) { var emitter = new EventEmitter(); var call = getCall(this.channel, method, options); if (metadata === null || metadata === undefined) { - metadata = {}; + metadata = new Metadata(); + } else { + metadata = metadata.clone(); } emitter.cancel = function cancel() { call.cancel(); @@ -281,7 +284,8 @@ function makeUnaryRequestFunction(method, serialize, deserialize) { var client_batch = {}; var message = serialize(argument); message.grpcWriteFlags = options.flags; - client_batch[grpc.opType.SEND_INITIAL_METADATA] = metadata; + client_batch[grpc.opType.SEND_INITIAL_METADATA] = + metadata._getCoreRepresentation(); client_batch[grpc.opType.SEND_MESSAGE] = message; client_batch[grpc.opType.SEND_CLOSE_FROM_CLIENT] = true; client_batch[grpc.opType.RECV_INITIAL_METADATA] = true; @@ -292,7 +296,8 @@ function makeUnaryRequestFunction(method, serialize, deserialize) { if (response.status.code !== grpc.status.OK) { var error = new Error(response.status.details); error.code = response.status.code; - error.metadata = response.status.metadata; + error.metadata = Metadata._fromCoreRepresentation( + response.status.metadata); callback(error); return; } else { @@ -302,7 +307,8 @@ function makeUnaryRequestFunction(method, serialize, deserialize) { return; } } - emitter.emit('metadata', response.metadata); + emitter.emit('metadata', Metadata._fromCoreRepresentation( + response.metadata)); callback(null, deserialize(response.read)); }); }); @@ -326,7 +332,7 @@ function makeClientStreamRequestFunction(method, serialize, deserialize) { * @this {Client} Client object. Must have a channel member. * @param {function(?Error, value=)} callback The callback to for when the * response is received - * @param {array=} metadata Array of metadata key/value pairs to add to the + * @param {Metadata=} metadata Array of metadata key/value pairs to add to the * call * @param {Object=} options Options map * @return {EventEmitter} An event emitter for stream related events @@ -335,7 +341,9 @@ function makeClientStreamRequestFunction(method, serialize, deserialize) { /* jshint validthis: true */ var call = getCall(this.channel, method, options); if (metadata === null || metadata === undefined) { - metadata = {}; + metadata = new Metadata(); + } else { + metadata = metadata.clone(); } var stream = new ClientWritableStream(call, serialize); this.updateMetadata(this.auth_uri, metadata, function(error, metadata) { @@ -345,7 +353,8 @@ function makeClientStreamRequestFunction(method, serialize, deserialize) { return; } var metadata_batch = {}; - metadata_batch[grpc.opType.SEND_INITIAL_METADATA] = metadata; + metadata_batch[grpc.opType.SEND_INITIAL_METADATA] = + metadata._getCoreRepresentation(); metadata_batch[grpc.opType.RECV_INITIAL_METADATA] = true; call.startBatch(metadata_batch, function(err, response) { if (err) { @@ -353,7 +362,8 @@ function makeClientStreamRequestFunction(method, serialize, deserialize) { // in the other batch. return; } - stream.emit('metadata', response.metadata); + stream.emit('metadata', Metadata._fromCoreRepresentation( + response.metadata)); }); var client_batch = {}; client_batch[grpc.opType.RECV_MESSAGE] = true; @@ -363,7 +373,8 @@ function makeClientStreamRequestFunction(method, serialize, deserialize) { if (response.status.code !== grpc.status.OK) { var error = new Error(response.status.details); error.code = response.status.code; - error.metadata = response.status.metadata; + error.metadata = Metadata._fromCoreRepresentation( + response.status.metadata); callback(error); return; } else { @@ -396,7 +407,7 @@ function makeServerStreamRequestFunction(method, serialize, deserialize) { * @this {SurfaceClient} Client object. Must have a channel member. * @param {*} argument The argument to the call. Should be serializable with * serialize - * @param {array=} metadata Array of metadata key/value pairs to add to the + * @param {Metadata=} metadata Array of metadata key/value pairs to add to the * call * @param {Object} options Options map * @return {EventEmitter} An event emitter for stream related events @@ -405,7 +416,9 @@ function makeServerStreamRequestFunction(method, serialize, deserialize) { /* jshint validthis: true */ var call = getCall(this.channel, method, options); if (metadata === null || metadata === undefined) { - metadata = {}; + metadata = new Metadata(); + } else { + metadata = metadata.clone(); } var stream = new ClientReadableStream(call, deserialize); this.updateMetadata(this.auth_uri, metadata, function(error, metadata) { @@ -417,7 +430,8 @@ function makeServerStreamRequestFunction(method, serialize, deserialize) { var start_batch = {}; var message = serialize(argument); message.grpcWriteFlags = options.flags; - start_batch[grpc.opType.SEND_INITIAL_METADATA] = metadata; + start_batch[grpc.opType.SEND_INITIAL_METADATA] = + metadata._getCoreRepresentation(); start_batch[grpc.opType.RECV_INITIAL_METADATA] = true; start_batch[grpc.opType.SEND_MESSAGE] = message; start_batch[grpc.opType.SEND_CLOSE_FROM_CLIENT] = true; @@ -427,7 +441,8 @@ function makeServerStreamRequestFunction(method, serialize, deserialize) { // in the other batch. return; } - stream.emit('metadata', response.metadata); + stream.emit('metadata', Metadata._fromCoreRepresentation( + response.metadata)); }); var status_batch = {}; status_batch[grpc.opType.RECV_STATUS_ON_CLIENT] = true; @@ -436,7 +451,8 @@ function makeServerStreamRequestFunction(method, serialize, deserialize) { if (response.status.code !== grpc.status.OK) { var error = new Error(response.status.details); error.code = response.status.code; - error.metadata = response.status.metadata; + error.metadata = Metadata._fromCoreRepresentation( + response.status.metadata); stream.emit('error', error); return; } else { @@ -466,7 +482,7 @@ function makeBidiStreamRequestFunction(method, serialize, deserialize) { /** * Make a bidirectional stream request with this method on the given channel. * @this {SurfaceClient} Client object. Must have a channel member. - * @param {array=} metadata Array of metadata key/value pairs to add to the + * @param {Metadata=} metadata Array of metadata key/value pairs to add to the * call * @param {Options} options Options map * @return {EventEmitter} An event emitter for stream related events @@ -475,7 +491,9 @@ function makeBidiStreamRequestFunction(method, serialize, deserialize) { /* jshint validthis: true */ var call = getCall(this.channel, method, options); if (metadata === null || metadata === undefined) { - metadata = {}; + metadata = new Metadata(); + } else { + metadata = metadata.clone(); } var stream = new ClientDuplexStream(call, serialize, deserialize); this.updateMetadata(this.auth_uri, metadata, function(error, metadata) { @@ -485,7 +503,8 @@ function makeBidiStreamRequestFunction(method, serialize, deserialize) { return; } var start_batch = {}; - start_batch[grpc.opType.SEND_INITIAL_METADATA] = metadata; + start_batch[grpc.opType.SEND_INITIAL_METADATA] = + metadata._getCoreRepresentation(); start_batch[grpc.opType.RECV_INITIAL_METADATA] = true; call.startBatch(start_batch, function(err, response) { if (err) { @@ -493,7 +512,8 @@ function makeBidiStreamRequestFunction(method, serialize, deserialize) { // in the other batch. return; } - stream.emit('metadata', response.metadata); + stream.emit('metadata', Metadata._fromCoreRepresentation( + response.metadata)); }); var status_batch = {}; status_batch[grpc.opType.RECV_STATUS_ON_CLIENT] = true; @@ -502,7 +522,8 @@ function makeBidiStreamRequestFunction(method, serialize, deserialize) { if (response.status.code !== grpc.status.OK) { var error = new Error(response.status.details); error.code = response.status.code; - error.metadata = response.status.metadata; + error.metadata = Metadata._fromCoreRepresentation( + response.status.metadata); stream.emit('error', error); return; } else { diff --git a/src/node/src/metadata.js b/src/node/src/metadata.js new file mode 100644 index 00000000000..ae7112f36e4 --- /dev/null +++ b/src/node/src/metadata.js @@ -0,0 +1,167 @@ +/* + * + * Copyright 2015, Google Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + */ + +/** + * Metadata module + * @module + */ + +'use strict'; + +var _ = require('lodash'); + +/** + * Class for storing metadata. Keys are normalized to lowercase ASCII. + * @constructor + */ +function Metadata() { + this._internal_repr = {}; +} + +function normalizeKey(key) { + return _.deburr(key).toLowerCase(); +} + +function validate(key, value) { + if (_.endsWith(key, '-bin')) { + if (!(value instanceof Buffer)) { + throw new Error('keys that end with \'-bin\' must have Buffer values'); + } + } else { + if (!_.isString(value)) { + throw new Error( + 'keys that don\'t end with \'-bin\' must have String values'); + } + } +} + +/** + * Sets the given value for the given key, replacing any other values associated + * with that key. Normalizes the key. + * @param {String} key The key to set + * @param {String|Buffer} value The value to set. Must be a buffer if and only + * if the normalized key ends with '-bin' + */ +Metadata.prototype.set = function(key, value) { + key = normalizeKey(key); + validate(key, value); + this._internal_repr[key] = [value]; +}; + +/** + * Adds the given value for the given key. Normalizes the key. + * @param {String} key The key to add to. + * @param {String|Buffer} value The value to add. Must be a buffer if and only + * if the normalized key ends with '-bin' + */ +Metadata.prototype.add = function(key, value) { + key = normalizeKey(key); + validate(key, value); + if (!this._internal_repr[key]) { + this._internal_repr[key] = []; + } + this._internal_repr[key].push(value); +}; + +/** + * Remove the given key and any associated values. + * @param {String} key The key to remove + */ +Metadata.prototype.remove = function(key) { + if (Object.prototype.hasOwnProperty.call(this._internal_repr, key)) { + delete this._internal_repr[key]; + } +}; + +/** + * Gets a list of all values associated with the key. + * @param {String} key The key to get + * @return {Array.} The values associated with that key + */ +Metadata.prototype.get = function(key) { + if (Object.prototype.hasOwnProperty.call(this._internal_repr, key)) { + return this._internal_repr[key]; + } else { + return []; + } +}; + +/** + * Get a map of each key to a single associated value. This reflects the most + * common way that people will want to see metadata. + * @return {Object.} A key/value mapping of the metadata + */ +Metadata.prototype.getMap = function() { + var result = {}; + _.forOwn(this._internal_repr, function(values, key) { + if(values.length > 0) { + result[key] = values[0]; + } + }); + return result; +}; + +/** + * Clone the metadata object. + * @return {Metadata} The new cloned object + */ +Metadata.prototype.clone = function() { + var copy = new Metadata(); + copy._internal_repr = _.cloneDeep(this._internal_repr); + return copy; +} + +/** + * Gets the metadata in the format used by interal code. Intended for internal + * use only. API stability is not guaranteed. + * @private + * @return {Object.>} The metadata + */ +Metadata.prototype._getCoreRepresentation = function() { + return this._internal_repr; +}; + +/** + * Creates a Metadata object from a metadata map in the internal format. + * Intended for internal use only. API stability is not guaranteed. + * @private + * @param {Object.>} The metadata + * @return {Metadata} The new Metadata object + */ +Metadata._fromCoreRepresentation = function(metadata) { + var newMetadata = new Metadata(); + newMetadata._internal_repr = _.cloneDeep(metadata); + return newMetadata; +}; + +module.exports = Metadata; diff --git a/src/node/src/server.js b/src/node/src/server.js index 5037abae434..5d76b31f157 100644 --- a/src/node/src/server.js +++ b/src/node/src/server.js @@ -44,6 +44,8 @@ var grpc = require('bindings')('grpc.node'); var common = require('./common'); +var Metadata = require('./metadata'); + var stream = require('stream'); var Readable = stream.Readable; @@ -60,10 +62,10 @@ var EventEmitter = require('events').EventEmitter; * @param {Object} error The error object */ function handleError(call, error) { + var statusMetadata = new Metadata(); var status = { code: grpc.status.UNKNOWN, - details: 'Unknown Error', - metadata: {} + details: 'Unknown Error' }; if (error.hasOwnProperty('message')) { status.details = error.message; @@ -75,11 +77,13 @@ function handleError(call, error) { } } if (error.hasOwnProperty('metadata')) { - status.metadata = error.metadata; + statusMetadata = error.metadata; } + status.metadata = statusMetadata._getCoreRepresentation(); var error_batch = {}; if (!call.metadataSent) { - error_batch[grpc.opType.SEND_INITIAL_METADATA] = {}; + error_batch[grpc.opType.SEND_INITIAL_METADATA] = + (new Metadata())._getCoreRepresentation(); } error_batch[grpc.opType.SEND_STATUS_FROM_SERVER] = status; call.startBatch(error_batch, function(){}); @@ -114,22 +118,24 @@ function waitForCancel(call, emitter) { * @param {*} value The value to respond with * @param {function(*):Buffer=} serialize Serialization function for the * response - * @param {Object=} metadata Optional trailing metadata to send with status + * @param {Metadata=} metadata Optional trailing metadata to send with status * @param {number=} flags Flags for modifying how the message is sent. * Defaults to 0. */ function sendUnaryResponse(call, value, serialize, metadata, flags) { var end_batch = {}; + var statusMetadata = new Metadata(); var status = { code: grpc.status.OK, - details: 'OK', - metadata: {} + details: 'OK' }; if (metadata) { - status.metadata = metadata; + statusMetadata = metadata; } + status.metadata = statusMetadata._getCoreRepresentation(); if (!call.metadataSent) { - end_batch[grpc.opType.SEND_INITIAL_METADATA] = {}; + end_batch[grpc.opType.SEND_INITIAL_METADATA] = + (new Metadata())._getCoreRepresentation(); call.metadataSent = true; } var message = serialize(value); @@ -151,15 +157,17 @@ function setUpWritable(stream, serialize) { stream.status = { code : grpc.status.OK, details : 'OK', - metadata : {} + metadata : new Metadata() }; stream.serialize = common.wrapIgnoreNull(serialize); function sendStatus() { var batch = {}; if (!stream.call.metadataSent) { stream.call.metadataSent = true; - batch[grpc.opType.SEND_INITIAL_METADATA] = {}; + batch[grpc.opType.SEND_INITIAL_METADATA] = + (new Metadata())._getCoreRepresentation(); } + stream.status.metadata = stream.status.metadata._getCoreRepresentation(); batch[grpc.opType.SEND_STATUS_FROM_SERVER] = stream.status; stream.call.startBatch(batch, function(){}); } @@ -203,7 +211,7 @@ function setUpWritable(stream, serialize) { /** * Override of Writable#end method that allows for sending metadata with a * success status. - * @param {Object=} metadata Metadata to send with the status + * @param {Metadata=} metadata Metadata to send with the status */ stream.end = function(metadata) { if (metadata) { @@ -266,7 +274,8 @@ function _write(chunk, encoding, callback) { /* jshint validthis: true */ var batch = {}; if (!this.call.metadataSent) { - batch[grpc.opType.SEND_INITIAL_METADATA] = {}; + batch[grpc.opType.SEND_INITIAL_METADATA] = + (new Metadata())._getCoreRepresentation(); this.call.metadataSent = true; } var message = this.serialize(chunk); @@ -289,15 +298,15 @@ ServerWritableStream.prototype._write = _write; /** * Send the initial metadata for a writable stream. - * @param {Object>} responseMetadata Metadata - * to send + * @param {Metadata} responseMetadata Metadata to send */ function sendMetadata(responseMetadata) { /* jshint validthis: true */ if (!this.call.metadataSent) { this.call.metadataSent = true; var batch = []; - batch[grpc.opType.SEND_INITIAL_METADATA] = responseMetadata; + batch[grpc.opType.SEND_INITIAL_METADATA] = + responseMetadata._getCoreRepresentation(); this.call.startBatch(batch, function(err) { if (err) { this.emit('error', err); @@ -422,7 +431,7 @@ ServerDuplexStream.prototype.getPeer = getPeer; * @access private * @param {grpc.Call} call The call to handle * @param {Object} handler Request handler object for the method that was called - * @param {Object} metadata Metadata from the client + * @param {Metadata} metadata Metadata from the client */ function handleUnary(call, handler, metadata) { var emitter = new EventEmitter(); @@ -430,7 +439,8 @@ function handleUnary(call, handler, metadata) { if (!call.metadataSent) { call.metadataSent = true; var batch = {}; - batch[grpc.opType.SEND_INITIAL_METADATA] = responseMetadata; + batch[grpc.opType.SEND_INITIAL_METADATA] = + responseMetadata._getCoreRepresentation(); call.startBatch(batch, function() {}); } }; @@ -478,7 +488,7 @@ function handleUnary(call, handler, metadata) { * @access private * @param {grpc.Call} call The call to handle * @param {Object} handler Request handler object for the method that was called - * @param {Object} metadata Metadata from the client + * @param {Metadata} metadata Metadata from the client */ function handleServerStreaming(call, handler, metadata) { var stream = new ServerWritableStream(call, handler.serialize); @@ -507,7 +517,7 @@ function handleServerStreaming(call, handler, metadata) { * @access private * @param {grpc.Call} call The call to handle * @param {Object} handler Request handler object for the method that was called - * @param {Object} metadata Metadata from the client + * @param {Metadata} metadata Metadata from the client */ function handleClientStreaming(call, handler, metadata) { var stream = new ServerReadableStream(call, handler.deserialize); @@ -515,7 +525,8 @@ function handleClientStreaming(call, handler, metadata) { if (!call.metadataSent) { call.metadataSent = true; var batch = {}; - batch[grpc.opType.SEND_INITIAL_METADATA] = responseMetadata; + batch[grpc.opType.SEND_INITIAL_METADATA] = + responseMetadata._getCoreRepresentation(); call.startBatch(batch, function() {}); } }; @@ -542,7 +553,7 @@ function handleClientStreaming(call, handler, metadata) { * @access private * @param {grpc.Call} call The call to handle * @param {Object} handler Request handler object for the method that was called - * @param {Object} metadata Metadata from the client + * @param {Metadata} metadata Metadata from the client */ function handleBidiStreaming(call, handler, metadata) { var stream = new ServerDuplexStream(call, handler.serialize, @@ -599,7 +610,7 @@ function Server(options) { var details = event.new_call; var call = details.call; var method = details.method; - var metadata = details.metadata; + var metadata = Metadata._fromCoreRepresentation(details.metadata); if (method === null) { return; } @@ -609,7 +620,8 @@ function Server(options) { handler = handlers[method]; } else { var batch = {}; - batch[grpc.opType.SEND_INITIAL_METADATA] = {}; + batch[grpc.opType.SEND_INITIAL_METADATA] = + (new Metadata())._getCoreRepresentation(); batch[grpc.opType.SEND_STATUS_FROM_SERVER] = { code: grpc.status.UNIMPLEMENTED, details: 'This method is not available on this server.', diff --git a/src/node/test/health_test.js b/src/node/test/health_test.js index be4ef1d251b..5e6a0ba4670 100644 --- a/src/node/test/health_test.js +++ b/src/node/test/health_test.js @@ -39,7 +39,7 @@ var health = require('../health_check/health.js'); var grpc = require('../'); -describe('Health Checking', function() { +describe.only('Health Checking', function() { var statusMap = { '': { '': 'SERVING', diff --git a/src/node/test/metadata_test.js b/src/node/test/metadata_test.js new file mode 100644 index 00000000000..f1859b674d8 --- /dev/null +++ b/src/node/test/metadata_test.js @@ -0,0 +1,172 @@ +/* + * + * Copyright 2015, Google Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + */ + +'use strict'; + +var Metadata = require('../src/metadata.js'); + +var assert = require('assert'); + +describe('Metadata', function() { + var metadata; + beforeEach(function() { + metadata = new Metadata(); + }); + describe('#set', function() { + it('Only accepts string values for non "-bin" keys', function() { + assert.throws(function() { + metadata.set('key', new Buffer('value')); + }); + assert.doesNotThrow(function() { + metadata.set('key', 'value'); + }); + }); + it('Only accepts Buffer values for "-bin" keys', function() { + assert.throws(function() { + metadata.set('key-bin', 'value'); + }); + assert.doesNotThrow(function() { + metadata.set('key-bin', new Buffer('value')); + }); + }); + it('Saves values that can be retrieved', function() { + metadata.set('key', 'value'); + assert.deepEqual(metadata.get('key'), ['value']); + }); + it('Overwrites previous values', function() { + metadata.set('key', 'value1'); + metadata.set('key', 'value2'); + assert.deepEqual(metadata.get('key'), ['value2']); + }); + it('Normalizes keys', function() { + metadata.set('Key', 'value1'); + assert.deepEqual(metadata.get('key'), ['value1']); + metadata.set('KEY', 'value2'); + assert.deepEqual(metadata.get('key'), ['value2']); + }); + }); + describe('#add', function() { + it('Only accepts string values for non "-bin" keys', function() { + assert.throws(function() { + metadata.add('key', new Buffer('value')); + }); + assert.doesNotThrow(function() { + metadata.add('key', 'value'); + }); + }); + it('Only accepts Buffer values for "-bin" keys', function() { + assert.throws(function() { + metadata.add('key-bin', 'value'); + }); + assert.doesNotThrow(function() { + metadata.add('key-bin', new Buffer('value')); + }); + }); + it('Saves values that can be retrieved', function() { + metadata.add('key', 'value'); + assert.deepEqual(metadata.get('key'), ['value']); + }); + it('Combines with previous values', function() { + metadata.add('key', 'value1'); + metadata.add('key', 'value2'); + assert.deepEqual(metadata.get('key'), ['value1', 'value2']); + }); + it('Normalizes keys', function() { + metadata.add('Key', 'value1'); + assert.deepEqual(metadata.get('key'), ['value1']); + metadata.add('KEY', 'value2'); + assert.deepEqual(metadata.get('key'), ['value1', 'value2']); + }); + }); + describe('#remove', function() { + it('clears values from a key', function() { + metadata.add('key', 'value'); + metadata.remove('key'); + assert.deepEqual(metadata.get('key'), []); + }); + it('does not normalize keys', function() { + metadata.add('key', 'value'); + metadata.remove('KEY'); + assert.deepEqual(metadata.get('key'), ['value']); + }); + }); + describe('#get', function() { + beforeEach(function() { + metadata.add('key', 'value1'); + metadata.add('key', 'value2'); + metadata.add('key-bin', new Buffer('value')); + }); + it('gets all values associated with a key', function() { + assert.deepEqual(metadata.get('key'), ['value1', 'value2']); + }); + it('does not normalize keys', function() { + assert.deepEqual(metadata.get('KEY'), []); + }); + it('returns an empty list for non-existent keys', function() { + assert.deepEqual(metadata.get('non-existent-key'), []); + }); + it('returns Buffers for "-bin" keys', function() { + assert(metadata.get('key-bin')[0] instanceof Buffer); + }); + }); + describe('#getMap', function() { + it('gets a map of keys to values', function() { + metadata.add('key1', 'value1'); + metadata.add('Key2', 'value2'); + metadata.add('KEY3', 'value3'); + assert.deepEqual(metadata.getMap(), + {key1: 'value1', + key2: 'value2', + key3: 'value3'}); + }); + }); + describe('#clone', function() { + it('retains values from the original', function() { + metadata.add('key', 'value'); + var copy = metadata.clone(); + assert.deepEqual(copy.get('key'), ['value']); + }); + it('Does not see newly added values', function() { + metadata.add('key', 'value1'); + var copy = metadata.clone(); + metadata.add('key', 'value2'); + assert.deepEqual(copy.get('key'), ['value1']); + }); + it('Does not add new values to the original', function() { + metadata.add('key', 'value1'); + var copy = metadata.clone(); + copy.add('key', 'value2'); + assert.deepEqual(metadata.get('key'), ['value1']); + }); + }); +}); diff --git a/src/node/test/surface_test.js b/src/node/test/surface_test.js index ec7ed87728d..c3caa4d5a38 100644 --- a/src/node/test/surface_test.js +++ b/src/node/test/surface_test.js @@ -262,6 +262,7 @@ describe('Generic client and server', function() { describe('Echo metadata', function() { var client; var server; + var metadata; before(function() { var test_proto = ProtoBuf.loadProtoFile(__dirname + '/test_service.proto'); var test_service = test_proto.lookup('TestService'); @@ -294,6 +295,8 @@ describe('Echo metadata', function() { var Client = surface_client.makeProtobufClientConstructor(test_service); client = new Client('localhost:' + port, grpc.Credentials.createInsecure()); server.start(); + metadata = new grpc.Metadata(); + metadata.set('key', 'value'); }); after(function() { server.shutdown(); @@ -301,35 +304,35 @@ describe('Echo metadata', function() { it('with unary call', function(done) { var call = client.unary({}, function(err, data) { assert.ifError(err); - }, {key: ['value']}); + }, metadata); call.on('metadata', function(metadata) { - assert.deepEqual(metadata.key, ['value']); + assert.deepEqual(metadata.get('key'), ['value']); done(); }); }); it('with client stream call', function(done) { var call = client.clientStream(function(err, data) { assert.ifError(err); - }, {key: ['value']}); + }, metadata); call.on('metadata', function(metadata) { - assert.deepEqual(metadata.key, ['value']); + assert.deepEqual(metadata.get('key'), ['value']); done(); }); call.end(); }); it('with server stream call', function(done) { - var call = client.serverStream({}, {key: ['value']}); + var call = client.serverStream({}, metadata); call.on('data', function() {}); call.on('metadata', function(metadata) { - assert.deepEqual(metadata.key, ['value']); + assert.deepEqual(metadata.get('key'), ['value']); done(); }); }); it('with bidi stream call', function(done) { - var call = client.bidiStream({key: ['value']}); + var call = client.bidiStream(metadata); call.on('data', function() {}); call.on('metadata', function(metadata) { - assert.deepEqual(metadata.key, ['value']); + assert.deepEqual(metadata.get('key'), ['value']); done(); }); call.end(); @@ -337,9 +340,10 @@ describe('Echo metadata', function() { it('shows the correct user-agent string', function(done) { var version = require('../package.json').version; var call = client.unary({}, function(err, data) { assert.ifError(err); }, - {key: ['value']}); + metadata); call.on('metadata', function(metadata) { - assert(_.startsWith(metadata['user-agent'], 'grpc-node/' + version)); + assert(_.startsWith(metadata.get('user-agent')[0], + 'grpc-node/' + version)); done(); }); }); @@ -354,13 +358,14 @@ describe('Other conditions', function() { var test_proto = ProtoBuf.loadProtoFile(__dirname + '/test_service.proto'); test_service = test_proto.lookup('TestService'); server = new grpc.Server(); + var trailer_metadata = new grpc.Metadata(); server.addProtoService(test_service, { unary: function(call, cb) { var req = call.request; if (req.error) { - cb(new Error('Requested error'), null, {trailer_present: ['yes']}); + cb(new Error('Requested error'), null, trailer_metadata); } else { - cb(null, {count: 1}, {trailer_present: ['yes']}); + cb(null, {count: 1}, trailer_metadata); } }, clientStream: function(stream, cb){ @@ -369,14 +374,14 @@ describe('Other conditions', function() { stream.on('data', function(data) { if (data.error) { errored = true; - cb(new Error('Requested error'), null, {trailer_present: ['yes']}); + cb(new Error('Requested error'), null, trailer_metadata); } else { count += 1; } }); stream.on('end', function() { if (!errored) { - cb(null, {count: count}, {trailer_present: ['yes']}); + cb(null, {count: count}, trailer_metadata); } }); }, @@ -384,13 +389,13 @@ describe('Other conditions', function() { var req = stream.request; if (req.error) { var err = new Error('Requested error'); - err.metadata = {trailer_present: ['yes']}; + err.metadata = trailer_metadata; stream.emit('error', err); } else { for (var i = 0; i < 5; i++) { stream.write({count: i}); } - stream.end({trailer_present: ['yes']}); + stream.end(trailer_metadata); } }, bidiStream: function(stream) { @@ -398,10 +403,8 @@ describe('Other conditions', function() { stream.on('data', function(data) { if (data.error) { var err = new Error('Requested error'); - err.metadata = { - trailer_present: ['yes'], - count: ['' + count] - }; + err.metadata = trailer_metadata.clone(); + err.metadata.add('count', '' + count); stream.emit('error', err); } else { stream.write({count: count}); @@ -409,7 +412,7 @@ describe('Other conditions', function() { } }); stream.on('end', function() { - stream.end({trailer_present: ['yes']}); + stream.end(trailer_metadata); }); } }); @@ -510,7 +513,7 @@ describe('Other conditions', function() { assert.ifError(err); }); call.on('status', function(status) { - assert.deepEqual(status.metadata.trailer_present, ['yes']); + assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); done(); }); }); @@ -519,7 +522,7 @@ describe('Other conditions', function() { assert(err); }); call.on('status', function(status) { - assert.deepEqual(status.metadata.trailer_present, ['yes']); + assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); done(); }); }); @@ -531,7 +534,7 @@ describe('Other conditions', function() { call.write({error: false}); call.end(); call.on('status', function(status) { - assert.deepEqual(status.metadata.trailer_present, ['yes']); + assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); done(); }); }); @@ -543,7 +546,7 @@ describe('Other conditions', function() { call.write({error: true}); call.end(); call.on('status', function(status) { - assert.deepEqual(status.metadata.trailer_present, ['yes']); + assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); done(); }); }); @@ -552,7 +555,7 @@ describe('Other conditions', function() { call.on('data', function(){}); call.on('status', function(status) { assert.strictEqual(status.code, grpc.status.OK); - assert.deepEqual(status.metadata.trailer_present, ['yes']); + assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); done(); }); }); @@ -560,7 +563,7 @@ describe('Other conditions', function() { var call = client.serverStream({error: true}); call.on('data', function(){}); call.on('error', function(error) { - assert.deepEqual(error.metadata.trailer_present, ['yes']); + assert.deepEqual(error.metadata.get('trailer_present'), ['yes']); done(); }); }); @@ -572,7 +575,7 @@ describe('Other conditions', function() { call.on('data', function(){}); call.on('status', function(status) { assert.strictEqual(status.code, grpc.status.OK); - assert.deepEqual(status.metadata.trailer_present, ['yes']); + assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); done(); }); }); @@ -583,7 +586,7 @@ describe('Other conditions', function() { call.end(); call.on('data', function(){}); call.on('error', function(error) { - assert.deepEqual(error.metadata.trailer_present, ['yes']); + assert.deepEqual(error.metadata.get('trailer_present'), ['yes']); done(); }); }); From e796e1fd7c6a01825745a308309ad8621202e53a Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 20 Aug 2015 11:31:56 -0700 Subject: [PATCH 02/10] Re-enabled tests --- src/node/test/health_test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node/test/health_test.js b/src/node/test/health_test.js index 5e6a0ba4670..be4ef1d251b 100644 --- a/src/node/test/health_test.js +++ b/src/node/test/health_test.js @@ -39,7 +39,7 @@ var health = require('../health_check/health.js'); var grpc = require('../'); -describe.only('Health Checking', function() { +describe('Health Checking', function() { var statusMap = { '': { '': 'SERVING', From f441b3fdf2fb85076380502dd50025246e76130e Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 20 Aug 2015 11:44:52 -0700 Subject: [PATCH 03/10] Fixed test and lint errors --- src/node/src/client.js | 20 ++++++++++++-------- src/node/src/metadata.js | 6 ++++-- src/node/src/server.js | 7 +++++-- src/node/test/surface_test.js | 1 + 4 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/node/src/client.js b/src/node/src/client.js index b90af42054a..e1bed3512e9 100644 --- a/src/node/src/client.js +++ b/src/node/src/client.js @@ -294,12 +294,13 @@ function makeUnaryRequestFunction(method, serialize, deserialize) { client_batch[grpc.opType.RECV_MESSAGE] = true; client_batch[grpc.opType.RECV_STATUS_ON_CLIENT] = true; call.startBatch(client_batch, function(err, response) { + response.status.metadata = Metadata._fromCoreRepresentation( + response.status.metadata); emitter.emit('status', response.status); if (response.status.code !== grpc.status.OK) { var error = new Error(response.status.details); error.code = response.status.code; - error.metadata = Metadata._fromCoreRepresentation( - response.status.metadata); + error.metadata = response.status.metadata; callback(error); return; } else { @@ -371,12 +372,13 @@ function makeClientStreamRequestFunction(method, serialize, deserialize) { client_batch[grpc.opType.RECV_MESSAGE] = true; client_batch[grpc.opType.RECV_STATUS_ON_CLIENT] = true; call.startBatch(client_batch, function(err, response) { + response.status.metadata = Metadata._fromCoreRepresentation( + response.status.metadata); stream.emit('status', response.status); if (response.status.code !== grpc.status.OK) { var error = new Error(response.status.details); error.code = response.status.code; - error.metadata = Metadata._fromCoreRepresentation( - response.status.metadata); + error.metadata = response.status.metadata; callback(error); return; } else { @@ -451,12 +453,13 @@ function makeServerStreamRequestFunction(method, serialize, deserialize) { var status_batch = {}; status_batch[grpc.opType.RECV_STATUS_ON_CLIENT] = true; call.startBatch(status_batch, function(err, response) { + response.status.metadata = Metadata._fromCoreRepresentation( + response.status.metadata); stream.emit('status', response.status); if (response.status.code !== grpc.status.OK) { var error = new Error(response.status.details); error.code = response.status.code; - error.metadata = Metadata._fromCoreRepresentation( - response.status.metadata); + error.metadata = response.status.metadata; stream.emit('error', error); return; } else { @@ -522,12 +525,13 @@ function makeBidiStreamRequestFunction(method, serialize, deserialize) { var status_batch = {}; status_batch[grpc.opType.RECV_STATUS_ON_CLIENT] = true; call.startBatch(status_batch, function(err, response) { + response.status.metadata = Metadata._fromCoreRepresentation( + response.status.metadata); stream.emit('status', response.status); if (response.status.code !== grpc.status.OK) { var error = new Error(response.status.details); error.code = response.status.code; - error.metadata = Metadata._fromCoreRepresentation( - response.status.metadata); + error.metadata = response.status.metadata; stream.emit('error', error); return; } else { diff --git a/src/node/src/metadata.js b/src/node/src/metadata.js index ae7112f36e4..39514b25476 100644 --- a/src/node/src/metadata.js +++ b/src/node/src/metadata.js @@ -139,7 +139,7 @@ Metadata.prototype.clone = function() { var copy = new Metadata(); copy._internal_repr = _.cloneDeep(this._internal_repr); return copy; -} +}; /** * Gets the metadata in the format used by interal code. Intended for internal @@ -160,7 +160,9 @@ Metadata.prototype._getCoreRepresentation = function() { */ Metadata._fromCoreRepresentation = function(metadata) { var newMetadata = new Metadata(); - newMetadata._internal_repr = _.cloneDeep(metadata); + if (metadata) { + newMetadata._internal_repr = _.cloneDeep(metadata); + } return newMetadata; }; diff --git a/src/node/src/server.js b/src/node/src/server.js index 7ef28428b45..b6f162adf85 100644 --- a/src/node/src/server.js +++ b/src/node/src/server.js @@ -167,7 +167,10 @@ function setUpWritable(stream, serialize) { batch[grpc.opType.SEND_INITIAL_METADATA] = (new Metadata())._getCoreRepresentation(); } - stream.status.metadata = stream.status.metadata._getCoreRepresentation(); + + if (stream.status.metadata) { + stream.status.metadata = stream.status.metadata._getCoreRepresentation(); + } batch[grpc.opType.SEND_STATUS_FROM_SERVER] = stream.status; stream.call.startBatch(batch, function(){}); } @@ -181,7 +184,7 @@ function setUpWritable(stream, serialize) { function setStatus(err) { var code = grpc.status.UNKNOWN; var details = 'Unknown Error'; - var metadata = {}; + var metadata = new Metadata(); if (err.hasOwnProperty('message')) { details = err.message; } diff --git a/src/node/test/surface_test.js b/src/node/test/surface_test.js index f000983a4aa..c7e63e98141 100644 --- a/src/node/test/surface_test.js +++ b/src/node/test/surface_test.js @@ -359,6 +359,7 @@ describe('Other conditions', function() { test_service = test_proto.lookup('TestService'); server = new grpc.Server(); var trailer_metadata = new grpc.Metadata(); + trailer_metadata.add('trailer_present', 'yes'); server.addProtoService(test_service, { unary: function(call, cb) { var req = call.request; From cc248a27f2e87c0cbc649c78f6db323982a6c7ba Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 20 Aug 2015 14:47:15 -0700 Subject: [PATCH 04/10] Added string value validation, modified key normalization and validation --- src/node/src/metadata.js | 10 +++++++++- src/node/test/metadata_test.js | 21 +++++++++++++++++++++ src/node/test/surface_test.js | 18 +++++++++--------- 3 files changed, 39 insertions(+), 10 deletions(-) diff --git a/src/node/src/metadata.js b/src/node/src/metadata.js index 39514b25476..8e0884acea6 100644 --- a/src/node/src/metadata.js +++ b/src/node/src/metadata.js @@ -49,10 +49,14 @@ function Metadata() { } function normalizeKey(key) { - return _.deburr(key).toLowerCase(); + return key.toLowerCase(); } function validate(key, value) { + if (!(/^[a-z\d-]+$/.test(key))) { + throw new Error('Metadata keys must be nonempty strings containing only ' + + 'alphanumeric characters and hyphens'); + } if (_.endsWith(key, '-bin')) { if (!(value instanceof Buffer)) { throw new Error('keys that end with \'-bin\' must have Buffer values'); @@ -62,6 +66,10 @@ function validate(key, value) { throw new Error( 'keys that don\'t end with \'-bin\' must have String values'); } + if (!(/^[\x20-\x7E]*$/.test(value))) { + throw new Error('Metadata string values can only contain printable ' + + 'ASCII characters and space'); + } } } diff --git a/src/node/test/metadata_test.js b/src/node/test/metadata_test.js index f1859b674d8..227fa9c9949 100644 --- a/src/node/test/metadata_test.js +++ b/src/node/test/metadata_test.js @@ -59,6 +59,19 @@ describe('Metadata', function() { metadata.set('key-bin', new Buffer('value')); }); }); + it('Rejects invalid keys', function() { + assert.throws(function() { + metadata.set('key$', 'value'); + }); + assert.throws(function() { + metadata.set('', 'value'); + }); + }); + it('Rejects values with non-ASCII characters', function() { + assert.throws(function() { + metadata.set('key', 'résumé'); + }); + }); it('Saves values that can be retrieved', function() { metadata.set('key', 'value'); assert.deepEqual(metadata.get('key'), ['value']); @@ -92,6 +105,14 @@ describe('Metadata', function() { metadata.add('key-bin', new Buffer('value')); }); }); + it('Rejects invalid keys', function() { + assert.throws(function() { + metadata.add('key$', 'value'); + }); + assert.throws(function() { + metadata.add('', 'value'); + }); + }); it('Saves values that can be retrieved', function() { metadata.add('key', 'value'); assert.deepEqual(metadata.get('key'), ['value']); diff --git a/src/node/test/surface_test.js b/src/node/test/surface_test.js index c7e63e98141..7c2a8d72583 100644 --- a/src/node/test/surface_test.js +++ b/src/node/test/surface_test.js @@ -359,7 +359,7 @@ describe('Other conditions', function() { test_service = test_proto.lookup('TestService'); server = new grpc.Server(); var trailer_metadata = new grpc.Metadata(); - trailer_metadata.add('trailer_present', 'yes'); + trailer_metadata.add('trailer-present', 'yes'); server.addProtoService(test_service, { unary: function(call, cb) { var req = call.request; @@ -514,7 +514,7 @@ describe('Other conditions', function() { assert.ifError(err); }); call.on('status', function(status) { - assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); + assert.deepEqual(status.metadata.get('trailer-present'), ['yes']); done(); }); }); @@ -523,7 +523,7 @@ describe('Other conditions', function() { assert(err); }); call.on('status', function(status) { - assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); + assert.deepEqual(status.metadata.get('trailer-present'), ['yes']); done(); }); }); @@ -535,7 +535,7 @@ describe('Other conditions', function() { call.write({error: false}); call.end(); call.on('status', function(status) { - assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); + assert.deepEqual(status.metadata.get('trailer-present'), ['yes']); done(); }); }); @@ -547,7 +547,7 @@ describe('Other conditions', function() { call.write({error: true}); call.end(); call.on('status', function(status) { - assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); + assert.deepEqual(status.metadata.get('trailer-present'), ['yes']); done(); }); }); @@ -556,7 +556,7 @@ describe('Other conditions', function() { call.on('data', function(){}); call.on('status', function(status) { assert.strictEqual(status.code, grpc.status.OK); - assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); + assert.deepEqual(status.metadata.get('trailer-present'), ['yes']); done(); }); }); @@ -564,7 +564,7 @@ describe('Other conditions', function() { var call = client.serverStream({error: true}); call.on('data', function(){}); call.on('error', function(error) { - assert.deepEqual(error.metadata.get('trailer_present'), ['yes']); + assert.deepEqual(error.metadata.get('trailer-present'), ['yes']); done(); }); }); @@ -576,7 +576,7 @@ describe('Other conditions', function() { call.on('data', function(){}); call.on('status', function(status) { assert.strictEqual(status.code, grpc.status.OK); - assert.deepEqual(status.metadata.get('trailer_present'), ['yes']); + assert.deepEqual(status.metadata.get('trailer-present'), ['yes']); done(); }); }); @@ -587,7 +587,7 @@ describe('Other conditions', function() { call.end(); call.on('data', function(){}); call.on('error', function(error) { - assert.deepEqual(error.metadata.get('trailer_present'), ['yes']); + assert.deepEqual(error.metadata.get('trailer-present'), ['yes']); done(); }); }); From 6b8a3a74f27a81940023ba3a8c822078d51621ee Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 20 Aug 2015 14:51:59 -0700 Subject: [PATCH 05/10] Normalize keys when getting and removing metadata items --- src/node/src/metadata.js | 6 ++++-- src/node/test/metadata_test.js | 8 ++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/node/src/metadata.js b/src/node/src/metadata.js index 8e0884acea6..a7b1ee3810d 100644 --- a/src/node/src/metadata.js +++ b/src/node/src/metadata.js @@ -102,21 +102,23 @@ Metadata.prototype.add = function(key, value) { }; /** - * Remove the given key and any associated values. + * Remove the given key and any associated values. Normalizes the key. * @param {String} key The key to remove */ Metadata.prototype.remove = function(key) { + key = normalizeKey(key); if (Object.prototype.hasOwnProperty.call(this._internal_repr, key)) { delete this._internal_repr[key]; } }; /** - * Gets a list of all values associated with the key. + * Gets a list of all values associated with the key. Normalizes the key. * @param {String} key The key to get * @return {Array.} The values associated with that key */ Metadata.prototype.get = function(key) { + key = normalizeKey(key); if (Object.prototype.hasOwnProperty.call(this._internal_repr, key)) { return this._internal_repr[key]; } else { diff --git a/src/node/test/metadata_test.js b/src/node/test/metadata_test.js index 227fa9c9949..86383f1badc 100644 --- a/src/node/test/metadata_test.js +++ b/src/node/test/metadata_test.js @@ -135,10 +135,10 @@ describe('Metadata', function() { metadata.remove('key'); assert.deepEqual(metadata.get('key'), []); }); - it('does not normalize keys', function() { + it('Normalizes keys', function() { metadata.add('key', 'value'); metadata.remove('KEY'); - assert.deepEqual(metadata.get('key'), ['value']); + assert.deepEqual(metadata.get('key'), []); }); }); describe('#get', function() { @@ -150,8 +150,8 @@ describe('Metadata', function() { it('gets all values associated with a key', function() { assert.deepEqual(metadata.get('key'), ['value1', 'value2']); }); - it('does not normalize keys', function() { - assert.deepEqual(metadata.get('KEY'), []); + it('Normalizes keys', function() { + assert.deepEqual(metadata.get('KEY'), ['value1', 'value2']); }); it('returns an empty list for non-existent keys', function() { assert.deepEqual(metadata.get('non-existent-key'), []); From 01a772028041577099e406019b731b3540a6fb2f Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 20 Aug 2015 15:40:03 -0700 Subject: [PATCH 06/10] Moved key character check to before key transformation --- src/node/src/metadata.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/node/src/metadata.js b/src/node/src/metadata.js index a7b1ee3810d..d4a8b2669ff 100644 --- a/src/node/src/metadata.js +++ b/src/node/src/metadata.js @@ -49,14 +49,14 @@ function Metadata() { } function normalizeKey(key) { + if (!(/^[A-Za-z\d-]+$/.test(key))) { + throw new Error('Metadata keys must be nonempty strings containing only ' + + 'alphanumeric characters and hyphens'); + } return key.toLowerCase(); } function validate(key, value) { - if (!(/^[a-z\d-]+$/.test(key))) { - throw new Error('Metadata keys must be nonempty strings containing only ' + - 'alphanumeric characters and hyphens'); - } if (_.endsWith(key, '-bin')) { if (!(value instanceof Buffer)) { throw new Error('keys that end with \'-bin\' must have Buffer values'); From 5df6ebd0c585756467c128dc20b60be515163f5e Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 20 Aug 2015 15:52:57 -0700 Subject: [PATCH 07/10] Replaced toLowerCase with local-insensitive downcasing function --- src/node/src/metadata.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/node/src/metadata.js b/src/node/src/metadata.js index d4a8b2669ff..77ababb65df 100644 --- a/src/node/src/metadata.js +++ b/src/node/src/metadata.js @@ -48,12 +48,19 @@ function Metadata() { this._internal_repr = {}; } +function downcaseString(str) { + var capitals = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'; + var lowercase = 'abcdefghijklmnopqrstuvwxyz'; + var charMap = _.zipObject(capitals, lowercase); + return str.replace(/[A-Z]/g, _.curry(_.get)(charMap)); +} + function normalizeKey(key) { if (!(/^[A-Za-z\d-]+$/.test(key))) { throw new Error('Metadata keys must be nonempty strings containing only ' + 'alphanumeric characters and hyphens'); } - return key.toLowerCase(); + return downcaseString(key); } function validate(key, value) { From 61bb21ab2c18fb7db403ae4c762c719d42c11680 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Fri, 21 Aug 2015 09:15:19 -0700 Subject: [PATCH 08/10] Reversed toLowerCase removal --- src/node/src/metadata.js | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/node/src/metadata.js b/src/node/src/metadata.js index 77ababb65df..d4a8b2669ff 100644 --- a/src/node/src/metadata.js +++ b/src/node/src/metadata.js @@ -48,19 +48,12 @@ function Metadata() { this._internal_repr = {}; } -function downcaseString(str) { - var capitals = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'; - var lowercase = 'abcdefghijklmnopqrstuvwxyz'; - var charMap = _.zipObject(capitals, lowercase); - return str.replace(/[A-Z]/g, _.curry(_.get)(charMap)); -} - function normalizeKey(key) { if (!(/^[A-Za-z\d-]+$/.test(key))) { throw new Error('Metadata keys must be nonempty strings containing only ' + 'alphanumeric characters and hyphens'); } - return downcaseString(key); + return key.toLowerCase(); } function validate(key, value) { From dde19d835e024aa39a778990b5efffef58b4fb8d Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Fri, 21 Aug 2015 09:24:33 -0700 Subject: [PATCH 09/10] Allowed underscore in metadata keys --- src/node/src/metadata.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node/src/metadata.js b/src/node/src/metadata.js index d4a8b2669ff..65fd91f3672 100644 --- a/src/node/src/metadata.js +++ b/src/node/src/metadata.js @@ -49,7 +49,7 @@ function Metadata() { } function normalizeKey(key) { - if (!(/^[A-Za-z\d-]+$/.test(key))) { + if (!(/^[A-Za-z\d_-]+$/.test(key))) { throw new Error('Metadata keys must be nonempty strings containing only ' + 'alphanumeric characters and hyphens'); } From 4ad400341e624947abbd08dd8ab8f06a6610b7e5 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 27 Aug 2015 10:02:24 -0700 Subject: [PATCH 10/10] Fixed handling of binary metadata values --- src/node/ext/call.cc | 23 ++++++++++++----------- src/node/src/metadata.js | 4 +++- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/node/ext/call.cc b/src/node/ext/call.cc index 18858fa334a..fddc1e214f5 100644 --- a/src/node/ext/call.cc +++ b/src/node/ext/call.cc @@ -111,17 +111,19 @@ bool CreateMetadataArray(Handle metadata, grpc_metadata_array *array, NanAssignPersistent(*handle, value); resources->handles.push_back(unique_ptr( new PersistentHolder(handle))); - continue; + } else { + return false; } - } - if (value->IsString()) { - Handle string_value = value->ToString(); - NanUtf8String *utf8_value = new NanUtf8String(string_value); - resources->strings.push_back(unique_ptr(utf8_value)); - current->value = **utf8_value; - current->value_length = string_value->Length(); } else { - return false; + if (value->IsString()) { + Handle string_value = value->ToString(); + NanUtf8String *utf8_value = new NanUtf8String(string_value); + resources->strings.push_back(unique_ptr(utf8_value)); + current->value = **utf8_value; + current->value_length = string_value->Length(); + } else { + return false; + } } array->count += 1; } @@ -156,8 +158,7 @@ Handle ParseMetadata(const grpc_metadata_array *metadata_array) { } if (EndsWith(elem->key, "-bin")) { array->Set(index_map[elem->key], - MakeFastBuffer( - NanNewBufferHandle(elem->value, elem->value_length))); + NanNewBufferHandle(elem->value, elem->value_length)); } else { array->Set(index_map[elem->key], NanNew(elem->value)); } diff --git a/src/node/src/metadata.js b/src/node/src/metadata.js index 65fd91f3672..c1da70b1974 100644 --- a/src/node/src/metadata.js +++ b/src/node/src/metadata.js @@ -147,7 +147,9 @@ Metadata.prototype.getMap = function() { */ Metadata.prototype.clone = function() { var copy = new Metadata(); - copy._internal_repr = _.cloneDeep(this._internal_repr); + _.forOwn(this._internal_repr, function(value, key) { + copy._internal_repr[key] = _.clone(value); + }); return copy; };