Add mutable, implicit headers for easy middleware

Add-some-mutability-to-headers-in-node-for-easy-midd.patch

From 761f0f03b16f4df0112f4990c38b4f59b2786d04 Mon Sep 17 00:00:00 2001
From: Tim Caswell <tim@creationix.com>
Date: Thu, 10 Feb 2011 02:18:13 -0800
Subject: [PATCH] Add support for mutable/implicit headers for http.

This works for both ServerResponse and ClientRequest.
Adds three new methods as a couple properties to to OutgoingMessage objects.
Tests by Charlie Robbins.

Change-Id: Ib6f3829798e8f11dd2b6136e61df254f1564807e
---
 doc/api/http.markdown                    |   58 ++++++++++++++-
 lib/http.js                              |  104 ++++++++++++++++++++++++--
 test/simple/test-http-mutable-headers.js |  119 ++++++++++++++++++++++++++++++
 3 files changed, 269 insertions(+), 12 deletions(-)
 create mode 100644 test/simple/test-http-mutable-headers.js

diff --git a/doc/api/http.markdown b/doc/api/http.markdown
index 53476d7..6f68eeb 100644
--- a/doc/api/http.markdown
+++ b/doc/api/http.markdown
@@ -261,10 +261,59 @@ Example:
 This method must only be called once on a message and it must
 be called before `response.end()` is called.
 
+If you call `response.write()` or `response.end()` before calling this, the
+implicit/mutable headers will be calculated and call this function for you.
+
+### response.statusCode
+
+When using implicit headers (not calling `response.writeHead()` explicitly), this property
+controlls the status code that will be send to the client when the headers get
+flushed.
+
+Example:
+
+    response.statusCode = 404;
+
+### response.setHeader(name, value)
+
+Sets a single header value for implicit headers.  If this header already exists
+in the to-be-sent headers, it's value will be replaced.  Use an array of strings
+here if you need to send multiple headers with the same name.
+
+Example:
+
+    response.setHeader("Content-Type", "text/html");
+
+or
+
+    response.setHeader("Set-Cookie", ["type=ninja", "language=javascript"]);
+
+
+### response.getHeader(name)
+
+Reads out a header that's already been queued but not sent to the client.  Note
+that the name is case insensitive.  This can only be called before headers get
+implicitly flushed.
+
+Example:
+
+    var contentType = response.getHeader('content-type');
+
+### response.removeHeader(name)
+
+Removes a header that's queued for implicit sending.
+
+Example:
+
+    response.removeHeader("Content-Encoding");
+
+
 ### response.write(chunk, encoding='utf8')
 
-This method must be called after `writeHead` was
-called. It sends a chunk of the response body. This method may
+If this method is called and `response.writeHead()` has not been called, it will
+switch to implicit header mode and flush the implicit headers.
+
+This sends a chunk of the response body. This method may
 be called multiple times to provide successive parts of the body.
 
 `chunk` can be a string or a buffer. If `chunk` is a string,
@@ -436,7 +485,10 @@ A queue of requests waiting to be sent to sockets.
 ## http.ClientRequest
 
 This object is created internally and returned from `http.request()`.  It
-represents an _in-progress_ request whose header has already been sent.
+represents an _in-progress_ request whose header has already been queued.  The 
+header is still mutable using the `setHeader(name, value)`, `getHeader(name)`,
+`removeHeader(name)` API.  The actual header will be sent along with the first
+data chunk or when closing the connection.
 
 To get the response, add a listener for `'response'` to the request object.
 `'response'` will be emitted from the request object when the response
diff --git a/lib/http.js b/lib/http.js
index e373166..9506a8c 100644
--- a/lib/http.js
+++ b/lib/http.js
@@ -303,6 +303,9 @@ function OutgoingMessage() {
   this._trailer = '';
 
   this.finished = false;
+  
+  this._headers = {};
+  this._headerNames = {};
 }
 util.inherits(OutgoingMessage, stream.Stream);
 
@@ -432,7 +435,6 @@ OutgoingMessage.prototype._storeHeader = function(firstLine, headers) {
 
     } else if (expectExpression.test(field)) {
       sentExpect = true;
-
     }
   }
 
@@ -495,9 +497,68 @@ OutgoingMessage.prototype._storeHeader = function(firstLine, headers) {
 };
 
 
+OutgoingMessage.prototype.setHeader = function(name, value) {
+  if (arguments.length < 2) {
+    throw new Error("`name` and `value` are required for setHeader().");
+  }
+
+  if (this._header) {
+    throw new Error("Can't set headers after they are sent.");
+  }
+
+  var key = name.toLowerCase();
+  this._headers[key] = value;
+  this._headerNames[key] = name;
+};
+
+
+OutgoingMessage.prototype.getHeader = function(name) {
+  if (arguments.length < 1) {
+    throw new Error("`name` is required for getHeader().");
+  }
+
+  if (this._header) {
+    throw new Error("Can't use mutable header APIs after sent.");
+  }
+
+  var key = name.toLowerCase();
+  return this._headers[key];
+};
+
+
+OutgoingMessage.prototype.removeHeader = function(name) {
+  if (arguments.length < 1) {
+    throw new Error("`name` is required for removeHeader().");
+  }
+
+  if (this._header) {
+    throw new Error("Can't remove headers after they are sent.");
+  }
+
+  var key = name.toLowerCase();
+  delete this._headers[key];
+  delete this._headerNames[key];
+};
+
+
+OutgoingMessage.prototype._renderHeaders = function() {
+  if (this._header) {
+    throw new Error("Can't render headers after they are sent to the client.");
+  }
+  var headers = {};
+  var keys = Object.keys(this._headers);
+  for (var i = 0, l = keys.length; i < l; i++) {
+    var key = keys[i];
+    headers[this._headerNames[key]] = this._headers[key];
+  }
+  return headers;
+};
+
+
+
 OutgoingMessage.prototype.write = function(chunk, encoding) {
   if (!this._header) {
-    throw new Error('You have to call writeHead() before write()');
+    this._implicitHeader();
   }
 
   if (!this._hasBody) {
@@ -557,6 +618,10 @@ OutgoingMessage.prototype.addTrailers = function(headers) {
 
 
 OutgoingMessage.prototype.end = function(data, encoding) {
+  if (!this._header) {
+    this._implicitHeader();
+  }
+
   var ret;
 
   var hot = this._headerSent === false &&
@@ -681,12 +746,16 @@ util.inherits(ServerResponse, OutgoingMessage);
 
 exports.ServerResponse = ServerResponse;
 
+ServerResponse.prototype.statusCode = 200;
 
 ServerResponse.prototype.writeContinue = function() {
   this._writeRaw('HTTP/1.1 100 Continue' + CRLF + CRLF, 'ascii');
   this._sent100 = true;
 };
 
+ServerResponse.prototype._implicitHeader = function() {
+  this.writeHead(this.statusCode, this._renderHeaders());
+};
 
 ServerResponse.prototype.writeHead = function(statusCode) {
   var reasonPhrase, headers, headerIndex;
@@ -742,12 +811,21 @@ function ClientRequest(options) {
   OutgoingMessage.call(this);
 
   var method = this.method = (options.method || 'GET').toUpperCase();
-  var path = options.path || '/';
-  var headers = options.headers || {};
-
-  // Host header set by default.
-  if (options.host && !(headers.host || headers.Host || headers.HOST)) {
-    headers.Host = options.host;
+  this.path = options.path || '/';
+
+  if (!Array.isArray(headers)) {
+    if (options.headers) {
+      var headers = options.headers;
+      var keys = Object.keys(headers);
+      for (var i = 0, l = keys.length; i < l; i++) {
+        var key = keys[i];
+        this.setHeader(key, headers[key]);
+      }
+    }
+    // Host header set by default.
+    if (options.host && !this.getHeader('host')) {
+      this.setHeader("Host", options.host);
+    }
   }
 
   this.shouldKeepAlive = false;
@@ -761,13 +839,21 @@ function ClientRequest(options) {
   // specified.
   this._last = true;
 
-  this._storeHeader(method + ' ' + path + ' HTTP/1.1\r\n', headers);
+  if (Array.isArray(headers)) {
+    this._storeHeader(this.method + ' ' + this.path + ' HTTP/1.1\r\n', headers);
+  } else if (this.getHeader('expect')) {
+    this._storeHeader(this.method + ' ' + this.path + ' HTTP/1.1\r\n', this._renderHeaders());
+  }
+
 }
 util.inherits(ClientRequest, OutgoingMessage);
 
 
 exports.ClientRequest = ClientRequest;
 
+ClientRequest.prototype._implicitHeader = function() {
+  this._storeHeader(this.method + ' ' + this.path + ' HTTP/1.1\r\n', this._renderHeaders());
+}
 
 ClientRequest.prototype.abort = function() {
   if (this._queue) {
diff --git a/test/simple/test-http-mutable-headers.js b/test/simple/test-http-mutable-headers.js
new file mode 100644
index 0000000..8b44cd0
--- /dev/null
+++ b/test/simple/test-http-mutable-headers.js
@@ -0,0 +1,119 @@
+var common = require('../common');
+var assert = require('assert');
+var http = require('http');
+
+// Simple test of Node's HTTP Client mutable headers
+// OutgoingMessage.prototype.setHeader(name, value)
+// OutgoingMessage.prototype.getHeader(name)
+// OutgoingMessage.prototype.removeHeader(name, value)
+// ServerResponse.prototype.statusCode
+// <ClientRequest>.method
+// <ClientRequest>.path
+
+var testsComplete = 0;
+var test = 'headers';
+var content = 'hello world\n';
+var cookies = [
+  'session_token=; path=/; expires=Sun, 15-Sep-2030 13:48:52 GMT',
+  'prefers_open_id=; path=/; expires=Thu, 01-Jan-1970 00:00:00 GMT'
+];
+
+var s = http.createServer(function(req, res) {
+  switch (test) {
+    case 'headers':
+      assert.throws(function () { res.setHeader() });
+      assert.throws(function () { res.setHeader('someHeader') });
+      assert.throws(function () { res.getHeader() });
+      assert.throws(function () { res.removeHeader() });
+
+      res.setHeader('x-test-header', 'testing');
+      res.setHeader('X-TEST-HEADER2', 'testing');
+      res.setHeader('set-cookie', cookies);
+      res.setHeader('x-test-array-header', [1, 2, 3]);
+
+      var val1 = res.getHeader('x-test-header');
+      var val2 = res.getHeader('x-test-header2');
+      assert.equal(val1, 'testing');
+      assert.equal(val2, 'testing');
+
+      res.removeHeader('x-test-header2');
+      break;
+
+    case 'contentLength':
+      res.setHeader('content-length', content.length);
+      assert.equal(content.length, res.getHeader('Content-Length'));
+      break;
+
+    case 'transferEncoding':
+      res.setHeader('transfer-encoding', 'chunked');
+      assert.equal(res.getHeader('Transfer-Encoding'), 'chunked');
+      break;
+  }
+
+  res.statusCode = 201;
+  res.end(content);
+});
+
+s.listen(common.PORT, nextTest);
+
+
+function nextTest () {
+  if (test === 'end') {
+    return s.close();
+  }
+
+  var bufferedResponse = '';
+
+  http.get({ port: common.PORT }, function(response) {
+    console.log('TEST: ' + test);
+    console.log('STATUS: ' + response.statusCode);
+    console.log('HEADERS: ');
+    console.dir(response.headers);
+
+    switch (test) {
+      case 'headers':
+        assert.equal(response.statusCode, 201);
+        assert.equal(response.headers['x-test-header'],
+                     'testing');
+        assert.equal(response.headers['x-test-array-header'],
+                     [1,2,3].join(', '));
+        assert.deepEqual(cookies,
+                         response.headers['set-cookie']);
+        assert.equal(response.headers['x-test-header2'] !== undefined, false);
+        // Make the next request
+        test = 'contentLength';
+        console.log('foobar');
+        break;
+
+      case 'contentLength':
+        assert.equal(response.headers['content-length'], content.length);
+        test = 'transferEncoding';
+        break;
+
+      case 'transferEncoding':
+        assert.equal(response.headers['transfer-encoding'], 'chunked');
+        test = 'end';
+        break;
+
+      default:
+        throw Error("?");
+    }
+
+    response.setEncoding('utf8');
+    response.on('data', function(s) {
+      bufferedResponse += s;
+    });
+
+    response.on('end', function() {
+      assert.equal(content, bufferedResponse);
+      testsComplete++;
+      nextTest();
+    });
+  });
+}
+
+
+process.on('exit', function() {
+  assert.equal(3, testsComplete);
+});
+
-- 
1.7.3.5

I'll go for the cookie api and make statusCode a simple property. I think we really should do the right thing with the header casing. It needs to both output in the case the user specified, but also be able to match case insensitively. I still think setHeader and getHeader are less magical and direct. They aren't plain property accesses so it's ok to hide them behind "java" style getters and setters.

What about the ability to have multiple headers with the same key? This is sometimes needed in HTTP and it's really hard to do with current node. PHP got around this issue by having a flag to set header that specified if it should overwrite or just append.

Here is how express handles multi headers: https://github.com/visionmedia/express/blob/master/lib/response.js#L256

yeah just a white-list, that is not even currently exposed haha. the only thing you could really do is either white-list or have setHeader(field, val[, multiple]) as a bool

or a combination of the two I suppose

+1 ... This is a step in the right direction.

Ok, this is my final version for tonight. Marak is working on unit tests...

Tim, I've added some tests on a fork of this gist: https://gist.github.com/818305

The functionality I pointed out to you was only half of the implementation for using Arrays as header values. The other half was in IncomingMessage.prototype._addHeaderLine. As opposed to changing that core functionality, I opted for a small change to ServerResponse.prototype.renderHeaders

  if (Array.isArray(value)) {
    var name = key.toLowerCase();
    value = name !== 'set-cookie' ? value.join(',') : value;
  }
  headers[this._headerNames[key]] = value;

There is a check for set-cookie, but it was necessary in order to keep 100% backwards compatibility with the existing API. This is because the existing API renders the response headers for set-cookie as an Array. See: https://github.com/ry/node/blob/master/test/simple/test-http-proxy.js#L36

Let me know what you think. I'd be happy to make any changes you see necessary.

I'd rather not put any value munging in the renderHeaders function. That kind of logic goes elsewhere. I saw the code where incoming multiple headers became a single header with commas, and that makes sense for that case. But for outgoing, I don't want to mess with the data any more than needed. The existing writeHeader seems to accept arrays as values. There is no reason to convert it to a single string.

The reason I made this change was because outgoing headers were not getting written correctly without it. I set an array header here (https://gist.github.com/818305#L114), then assert that it is set here (https://gist.github.com/818305#L131).

If I remove that code the value that is returned for test-array-header is "1" not "1,2,3" and definitely not "[1, 2, 3]" (i.e. an Array). I don't know where the problem is in http.js that allows for Set-Cookie to be handled special, but the code I pointed out before doesn't seem to be functioning correctly. I get an assertion error when running the test otherwise:

  $ node test/simple/test-http-mutable-headers.js
STATUS: 201

assert.js:81
  throw new assert.AssertionError({
        ^
AssertionError: "1,2,3" == "1"
    at ClientRequest. (/usr/src/node/test/simple/test-http-mutable-headers.js:43:12)
    at ClientRequest.emit (events.js:42:17)
    at HTTPParser.onIncoming (http.js:1452:9)
    at HTTPParser.onHeadersComplete (http.js:87:31)
    at Client.onData [as ondata] (http.js:1331:27)
    at Client._onReadable (net.js:648:27)
    at IOWatcher.onReadable [as callback] (net.js:156:10)

I don't know this code that well, hopefully you (or maybe Ry) can shed some light on what's going on in the Outgoing headers to make this patch a little more elegant.

Disregard the previous comment for I really didn't understand the test I was writing. I'm going to blame this on lack of sleep. Basically I was setting test-array-header to be an array, which by design should only return "1" since it is not marked with "x-". The implications of this are that we won't get multiple headers, but this is definitely a step in the right direction.

Updated test and hopefully final patch forthcoming

Ok:

1. All tests pass (100% backwards compatible)
2. Removed munging in renderHeaders() function that I added b/c I lost my understanding of HTTP at 6am >.<
3. Good test coverage of all edge cases in test/simple/test-http-mutable-headers.js

https://gist.github.com/818305

It's in. Success!

in 0.4.0?

Indeed. Last commit before the version bump :-D nodejs/node-v0.x-archive@b09c588