greim / hoxy

Web-hacking proxy API for node
http://greim.github.io/hoxy/
MIT License
597 stars 97 forks source link

Pass variables gained from route-pattern to request object #58

Open editedredx opened 9 years ago

editedredx commented 9 years ago

It would be nice to be able to use the route variables mentioned in the url/fullUrl interception parameters. Something in the line of this:

proxy.intercept({
  phase: 'request',
  method: 'GET',
  url: '/dir/:page'
}, function(req, resp, cycle) {
  console.log(req.route.namedParams.page);
});
greim commented 9 years ago

It might also be nice to also support regex capturing groups:

proxy.intercept({
  phase: 'request',
  url: /bar\-(\d+)/
}, function(req) {
  console.log(req.urlData[1]);
});
proxy.intercept({
  phase: 'request',
  url: '/dir/:page'
}, function(req) {
  console.log(req.urlData.namedParams.page);
});

Thoughts?

ysf commented 9 years ago

I assume that ...namedParams.name is used because route-patterns already provides it as this? Mixing the namedParams and matches might be tricky if someone uses (for whatever reason!):

url: /bar\-(?P<namedParams>\d+)/

If mixing is the way to go, I'd find

req.matches[1]
req.matches.page

more intuitive and cleaner. Which makes me think if it'll be of use to match regexes on other request/response fields too. If so naming it urlData or alike would be safer indeed.

editedredx commented 9 years ago

Another way to prevent mixing problems could be to use a getter:

proxy.intercept({
  phase: 'request',
  url: /bar\-(\d+)\/(?P<foo>\d+)/
}, function(req) {
  var urlData = req.getUrlData();
  console.log(urlData[0]);
  console.log(urlData.foo);
});
proxy.intercept({
  phase: 'request',
  url: '/dir/:page'
}, function(req) {
  var urlData = req.getUrlData();
  console.log(urlData.namedParams.page);
});

What getUrlData returns depends on the datatype used, regex or (route)string.

This is more in line with the getFullUrl method. It could also prevent problems when using a url and a fullUrl to intercept (although I'm not sure why you would do that) by also providing a getFullUrlData method.

EDIT Yes, namedParams was used because route-patterns already provides it that way.

greim commented 9 years ago

Which makes me think if it'll be of use to match regexes on other request/response fields too

Interesting. What about a more general approach where, for any filtering option, req.matches.thing is always just whatever the matching operation returned for the "thing" option. Same for resp.matches.thing. It would always be truthy, but otherwise it could be any of:

proxy.intercept({
  phase: 'request',
  hostname: hostname => hostname.split('-')[1]
}, function(req) {
  console.log(req.matches.hostname); // "bar" for hostname "foo-bar"
});
ysf commented 9 years ago

I like the general approach. I'm pretty new to node/io.js and english is not my mother tongue. So excuse me asking further: I don't see yet why the hostname example makes a big difference with beeing there and not in the callback. AFAIK the main reason was to DRY code, because if you filter with a capture on something in the intercept-parameter, the chance is high that you would've to to this again in the handling callback. How I understand the API is that I could provide a regex/string/function to filter on multiple headerfields and if all those match/are equal to/return true on something then the callbackfunction handling the request/response would be called, is this right? If so, how would this behave:

proxy.intercept({
  hostname: hostname => hostname.split('-')[1]
  url: '/user/:username/',
}, function(req) {
  console.log(req.matches.hostname);
  console.log(req.matches.url.username);
});

Will it be called only when the url starts with /user/ and hostname.split('-')[1] is actually something? Or would hostname be 'undefined' on a hostname like 'nodashinhere'? The latter one would be unintuitive or breaking the current API.

greim commented 9 years ago

It's buried in the docs, but yes, filtering options are logically ANDed together, and would definitely remain so if this new feature is added.

proxy.intercept({
  hostname: hostname => hostname.split('-')[1]
  url: '/user/:username/',
}, function(req) {
  // if this interceptor callback is called, then...
  console.log(req.matches.hostname); // this will always be truthy...
  console.log(req.matches.url.username); // this will always be truthy...
});

Does that answer the question?

greim commented 9 years ago

Although, now that I think of it:

proxy.intercept({
  hostname: hostname => hostname.split('-')[1]
  url: '/user/:username/',
}, function(req) {
  // if this interceptor callback is called, then...
  console.log(req.matches.hostname); // this will always be truthy...
  console.log(req.matches.url.username); // this will always be truthy...

  // what about this:
  console.log(req.matches.fullUrl); // should this be undefined?
});
ysf commented 9 years ago

I know that they are ANDed but here is "hostname: hostname => hostname.split('-')[1]" like the other settings a condition that has to me met? Or only a utility syntax to provide preprocessed data on headers. Means, using the examples given, the callback will only be called if the hostname contains a dash? Or will it be called anyway without magic matches for "hostname". Hope that I can explain my point somehow. Thanks for your time and work!

greim commented 9 years ago

a condition that has to me met?

Correct. The intent would be that hostname => hostname.split('-')[1] is just a conditional. It was probably a bad example. Maybe a better one would be:

var myHosts = new Set();
myHosts.add('foo.com');
myHosts.add('bar.com');

proxy.intercept({
  hostname: name => myHosts.has(name)
}, function(req) {
  console.log(req.matches.hostname); // true
}); 
ysf commented 9 years ago

Ok, yeah, that sounds nice to me.