Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Http Router doesn't work with URL parameters #55

Closed
stolsma opened this issue Dec 7, 2011 · 6 comments
Closed

Http Router doesn't work with URL parameters #55

stolsma opened this issue Dec 7, 2011 · 6 comments

Comments

@stolsma
Copy link
Contributor

stolsma commented Dec 7, 2011

When testing with: http://development:9090/test/foo?test=test the attached route function to /test/foo isn't executed. But if I change the request to http://development:9090/test/foo it is.

I debugged the code and found that the whole URL (+ the parameters after ?) is fed into the traverse function by the dispatch function. If I change that to only the part before ? then it works. Can make a patch for it but don't know if the current behavior is the correct one. If this is the correct behavior then what do I need to do to get URLs with parameters working with Director?

@pksunkara
Copy link
Contributor

Go ahead and make a pull request. @indexzero, @mmalecki, can you check this?

@indexzero
Copy link
Member

@stolsma Hmm ... right. In the tightly integrated scenario (i.e. director + union), union is already stripping these characters before it's passed to the router. I suppose we can reparse here, but it should definitely be an option or something.

@stolsma
Copy link
Contributor Author

stolsma commented Dec 8, 2011

@indexzero Are you sure that union is stripping these characters in a integrated scenario? I'm asking this because my test implementation is using union+director and there url parameters doesn't work.... Example how I use it:

var union = require('union');
var router = require('director').http.Router().configure({async: true});

router.get(/foo/, function () {
  this.res.writeHead(200, { 'Content-Type': 'text/plain' })
  if (this.req.token) {
    this.res.end('hello ' + this.req.token.user + '\n');
  } else {
    this.res.end('hello world\n');
  }
});

var server = union.createServer({
  before: [
    function (req, res) {
      var found = router.dispatch(req, res);
      if (!found) {
        res.emit('next');
      }
    }
  ]
});

server.listen(9090);

Is there an easier way to implement my example??

@indexzero
Copy link
Member

@stolsma Again, you seem to be right. Union is parsing them, but not stripping them.

https://github.com/flatiron/union/blob/master/lib/request-stream.js#L31-43

Your example is correct.

@stolsma
Copy link
Contributor Author

stolsma commented Dec 8, 2011

@indexzero Thanks for the confirmation! :-)

In my opinion union must not strip anything from req.url because then we loose the original request url and thats also something I need... ;-)

@indexzero
Copy link
Member

yeah, we should probably put the parse somewhere though so we don't have to reparse it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants