stefanprodan / WebApiThrottle

ASP.NET Web API rate limiter for IIS and Owin hosting
MIT License
1.28k stars 274 forks source link

Don't use OnSendingHeaders #92

Open JoachimC opened 7 years ago

JoachimC commented 7 years ago

As we've decided to terminate the middleware stack and initiate creating the response, we shouldn't use OnSendingHeaders. Particularly to set the status code as middleware further down the stack can't see what response we've decided to return.

I appreciate this might be a breaking change for some people but I think it's the 'right' answer - so hopefully you might consider accepting this change.

stefanprodan commented 7 years ago

To avoid a breaking change you could add an optional param to the middleware constructor and use a if to exclude the OnSendingHeaders section.

JoachimC commented 7 years ago

Hi, thanks for considering my PR.

To me that seems like a shame - to introduce the complexity of a flag to switch a behaviour that I don't believe is correct. To me the current behaviour feels like a bug - and this is a fix to that bug.

So maybe this change could wait for a major release - where breaking changes might be allowable?

lodicolo commented 4 years ago

I will concede that making this change breaks any code reliant on the last-minute mutation of the response, but...

The currently implemented behavior:

Based on those two things, to me it appears as though the currently released implementation is invalid and a bug, and that the last-minute mutation behavior just should not be permitted (and can be achieved by attaching this middleware last if it is truly desired).

In short -- I agree with JoachimC.