twitchax / AspNetCore.Proxy

ASP.NET Core Proxies made easy.
MIT License
505 stars 80 forks source link

Proxy Short Circuiting #80

Open rtkelly13 opened 2 years ago

rtkelly13 commented 2 years ago

I have been experimenting with the proxy to try to depreciate a MVC4 site. I can't interfere with the original paths in any way so my new website has to be prefixed in order to avoid conflicts with the older website. I was hoping I could use the intercept feature of the proxy to skip proxying a certain set of endpoints for the new site and proxy everything else to the old site but that caused issues. (I'll try and edit with a code sample)

The specific issue it caused was returning an empty response when I knew a later middleware should have returned the correct response in the pipeline. I've cloned your repository and created a new middleware from the subset of functionality, experimented a bit and found that it isn't calling the asynx next() when an intercept has blocked proxying certain paths.

I figured this out from the middleware page on the MS Docs

I'd be interested in making a change which stops this short circuiting if the proxy has been skipped, if you think it would be worthwhile addition to the library. It should be a no breaking change as an empty response in this case wouldn't make sense.

This is a really nice library and after I'd copied the code and tweaked it slightly did everything I needed it to do, really efficiently as well!

twitchax commented 2 years ago

Aha, very cool! :)

Honestly, Intercept was always meant to write a completely different response without proxying, but was not meant to call the next middleware. So, I would prefer not to change WithIntercept because it was specifically designed for returning some type of error response indicating why a proxy request was intercepted and rejected.

However, I think this is a good feature to add, so it might make sense to add a WithShortCircuit option which behaves like WithIntercept but only expects that that user provide a bool for whether or not to proceed with an attempt to proxy. If they return true, then it would behave like WithIntercept, except it would call the next middleware, and expect that the user did not muck with the response. Alternatively, we could change the return type of the WithIntercept predicate to be an enum of { ContinueProxy, RespondImmediately, CallNextMiddleware } (obviously not those names because they sound stupid, but something like that).

I can do this, but also happy to have you contribute if you are interested in personally making this contribution to the library.

Thoughts?

rtkelly13 commented 2 years ago

Glad you think this is a valid use case for your library!

I think I was absolutely abusing the WithIntercept functionality to try to achieve my goal, it makes absolute sense to not change that and think of an alternative approach. I think that is in keeping with the rest of the design too.

My intuitive approach would be to differentiate this functionality by using the Run/Use convention that the middleware system states in the article. So Run prefixed methods are middleware short circuit and use ones don't. But I think that would interfere with the UseProxies method you already have and just cause a lot of confusion

I would say a combination of two things might be appropriate?

I'm interested in making a change and giving it a go myself, if I couldn't get to it though, I'd be happy to test it for you against my use case if a pre-release package was available. Whatever works best for you!