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 <[email protected]>
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'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