mjackson / strata

A modular, streaming HTTP server for node.js
http://stratajs.org
366 stars 35 forks source link

strata.lint reporting issue with remotePort #35

Closed mvolkmann closed 11 years ago

mvolkmann commented 12 years ago

When I use strata.lint to detect problems in my middleware, it outputs the following:

Unhandled error! StrataError: Property "remotePort" must be a string

My middleware doesn't do anything and doesn't reference that property. Here is a very simple example that reproduces this.

noop-app.js

var strata = require('strata');
var noopMw = require('noop-middleware');

strata.use(strata.lint);
strata.use(noopMw, 'foo');
strata.use(strata.lint);

strata.run(function (env, cb) {
  var content = 'Hello from noop-app!';
  var headers = {
    'Content-Type': 'text/plain',
    'Content-Length': content.length.toString()
  };
  cb(200, headers, content);
});

noop-middleware.js

module.exports = function (app) {
  return app;
};

To run this, enter "node noop-app.js" and browse http://localhost:1982.

mvolkmann commented 12 years ago

In looking through the code for lib/lint.js, I see that it verifies that env.requestTime is a Date twice. The second assert can be deleted.

mvolkmann commented 12 years ago

I see that env.remotePort is a number rather than a string, but serverPort is a string. lint.js could be modified to expect env.remotePort to be a number or Strata could be modified to set that property with a string value. The strata.lint middleware seems to work properly with this change.

mjackson commented 11 years ago

Sorry to be so slow to fix this bug. Thanks for reporting!

mvolkmann commented 11 years ago

Thanks for fixing this! I assume the fix isn't in npm yet. I did a git clone to get the latest code and reran my test which does "strata.use(strata.lint);". Now the error I get is "StrataError: Value for header "Content-Length" must be a string". I get that when I start my server before I issue a request.

mjackson commented 11 years ago

Yes, it's not in npm yet. I keep bleeding edge changes on the master branch in GitHub and cut a stable release when it's ready. I should be cutting a new stable release of strata very shortly, but be aware that some of the API has changed significantly on master. Specifically, there are no more any http* values in the environment. Instead, there is a headers object, similar to what you get on a node ServerRequest object.

As for your specific error, the values of all headers should be strings, strictly speaking. The lint middleware is designed to be very strict, so that's why it complains. Of course, your app would probably run fine if you use a number as the value of a Content-Length header because everything gets cast to strings in the final message anyway, and numbers convert just fine when concat'ed with strings.

mjackson commented 11 years ago

Just following up here: This fix is included in version 0.18.0, now on npm. Enjoy!

mvolkmann commented 11 years ago

I was wrong when I said I was getting an error without issuing a requests. I was issuing a request.

However, when the Content-Length header has an integer value, I do get an error. Here's the snippet of my code that sends the request:

strata.get('/*', function (env, cb) {
  var content = 'Hello, World!';
  var headers = {
    'Content-Type': 'text/plain',
    'Content-Length': content.length
  };
  cb(200, headers, content);
});

Do I really have to do something like call .toString() on content.length?

mjackson commented 11 years ago

You bring up a good point. When I first wrote the lint middleware I thought it would be a good idea to ensure that all header values were strings. That way middleware could always know what to expect when dealing with the headers object. However I've changed my mind on that. Node actually lets you use an array to indicate multiple values for a single header, and it's annoying to convert numbers to strings every time, especially when setting Content-Length. I'm going to change this behavior to make it so that you can pass back a string, number, or array.

mvolkmann commented 11 years ago

Thanks! Is the change to allow Content-Length to be specified with an integer pushed out to NPM now?

mjackson commented 11 years ago

Yes, I just pushed it out this morning. Version 0.19.0.