Confusing docs around path rewrite

[brandonculver]

trafficstars

The documentation around the path rewrite config object is a bit confusing. One could infer that the object matches as many times it can against the path when in reality it stops as soon as it hits the first match.

https://github.com/chimurai/http-proxy-middleware/blob/master/lib/path-rewriter.js#L28

I am also not sure if _.forEach is the best choice here either as it further drives home the idea of cycling through everything.

Plus it seems like there could be use cases for looping through a set of simple rewrites against one path instead of overly complex keys to match in one go.

There is also no mention of priority, and how can priority even be guaranteed since you are using _.forIn on object keys.

[chimurai]

Interesting points...

Think _.forIn would be a better choice for object iteration.

Stopping at first match or cycle through everything are both confusing; Since iteration order is not guaranteed (_.forEach and _.forIn)

So looping through everything wouldn't improve the situation.

The majority in open source projects are using simple rewrite-rules to rewrite paths; They probably won't face any issues.

I'll update the documentation explaining the rewrite behavior (stopping at first match and order not guaranteed).

---

Perhaps allowing you to provide a custom function to do your own rewrite logic makes more sense to handle more complicated use-cases.

[chimurai]

I've no idea how pathRewrite is being used in private/closed projects. But if there is need for more control to handle more complicated path rewrites, I would expect a feature request in the issue tracker.

Keeping things simple and update the docs to remove the confusion would be my preference for now. What do you think @brandonculver ?

[brandonculver]

I agree. It would potentially be a breaking change for people already using it. Personally, I just removed the return false locally and it works as I expected in my head. Maybe there is a better solution for my use case. I am removing the original path, and stripping out an auth token (query parameter). Without doing that the proxied url ended up being /target/api/incoming/api?authToken&more&necessary&params.

The auth token may, or may not be directly after the original api in the string or the regex would be simple. There is probably a way to say match original api string and auth token, replace with ' ' but my regex fu is lacking.

[chimurai]

Could you share your pathRewrite config object and the request paths before and after rewrite? (Expected path after rewrite)

[brandonculver]

I really can't unfortunately do to some internal agreements. The pseudo code I posted is accurate though.

client calls /original/api/call?queryParam=abc&token=123

but token may or may not be the first,second,third, etc parameter

proxied to /a/totally/different/api?queryParam=abc

we strip off the the old api path and the token.

my config is simple

{
  target: 'http://host/a/totally/different/api',
  pathRewrite: {
    "/original/api/call": '',
    "[\?,\&]token=([^\&]+)": ''
  }
}

By removing return false, it hits both of those rewrites and strips out the original api base path and the token.

[chimurai]

Understandable. Thanks for sharing the pseudo code.

Another reason for exiting the loop early, was from performance point of view. With each request all pathRewrite rules would be checked if rewrite can/should be applied; This matches your expectation ;-) (I never thought of applying multiple rewrite rules on 1 path...)

Think changing the rewrite behaviour wouldn't hurt too much by removing the early iteration exit restriction, since I haven't seen extensive rewrite configurations in the wild. (Even though it might break existing use cases)

I'll have to think about this change...

[chimurai]

#73 should give you the possibility to provide a custom path rewrite function. It will be backwards compatible.

#72 should remove limitation of 1 rewrite. (Scheduled for the v1.0.0 release, since it might break some use cases)

Think this should cover a lot of use-cases.

@brandonculver Hopefully these changes will cover you use-cases.

[Lalem001]

Would picking up a syntax like in http-rewrite-middleware work?

Example from their docs:

var rewriteMiddleware = rewriteModule.getMiddleware([
    // Internal rewrite 
    {from: '^/index_dev.html$', to: '/src/index.html'},
    // Internal rewrite 
    {from: '^/js/(.*)$', to: '/src/js/$1'},
    // 301 Redirect 
    {from: '^/old-stuff/(.*)$', to: '/new-cool-stuff/$1', redirect: 'permanent'},
    // 302 Redirect 
    {from: '^/stuff/(.*)$', to: '/temporary-stuff/$1', redirect: 'temporary'}
]);

Having the paths in an array like this would guarantee order, and you can apply each rule in succession.

Thoughts?

P.S. I am actually using http-rewrite-middleware in my own projects, but it interferes with the operation of http-proxy-middleware@>=0.14.0 somehow.

[chimurai]

@Lalem001 I copied your question to keep the discussions separate.